From 8837fa6b1caed563ef8fb1929e8d66609477153c Mon Sep 17 00:00:00 2001 From: Kai Moritz Date: Fri, 18 Aug 2023 14:09:02 +0200 Subject: [PATCH] fix: GREEN - Fixed NPE in `ShardedChatHome.getChatRoom()` for foreign shard * `ShardedChatHome.getChatRoom(UUID)` know checks, if a `ChatHome` exists for the selected shard. * If no `ChatHome` exists, a `ShardNotOwnedException` is thrown. * The `ChatBackendControllerAdvice` translates the exception to an error of type 404 - NOT FOUND, to fullfill the defined expectations. --- .../backend/api/ChatBackendController.java | 2 +- .../api/ChatBackendControllerAdvice.java | 31 +++ .../domain/ShardNotOwnedException.java | 17 ++ .../chat/backend/domain/ShardedChatHome.java | 5 +- .../api/ChatBackendControllerTest.java | 185 +++++++++++++++++- 5 files changed, 228 insertions(+), 12 deletions(-) create mode 100644 src/main/java/de/juplo/kafka/chat/backend/domain/ShardNotOwnedException.java diff --git a/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendController.java b/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendController.java index 4db77ee2..339451a8 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendController.java +++ b/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendController.java @@ -70,7 +70,7 @@ public class ChatBackendController .flatMap(chatroom -> put(chatroom, username, messageId, text)); } - public Mono put( + private Mono put( ChatRoom chatroom, String username, Long messageId, diff --git a/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerAdvice.java b/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerAdvice.java index 55350f17..ad90c4b6 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerAdvice.java +++ b/src/main/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerAdvice.java @@ -2,6 +2,7 @@ package de.juplo.kafka.chat.backend.api; import de.juplo.kafka.chat.backend.domain.InvalidUsernameException; import de.juplo.kafka.chat.backend.domain.MessageMutationException; +import de.juplo.kafka.chat.backend.domain.ShardNotOwnedException; import de.juplo.kafka.chat.backend.domain.UnknownChatroomException; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpStatus; @@ -50,6 +51,36 @@ public class ChatBackendControllerAdvice return problem; } + @ExceptionHandler(ShardNotOwnedException.class) + public final ProblemDetail handleException( + ShardNotOwnedException e, + ServerWebExchange exchange, + UriComponentsBuilder uriComponentsBuilder) + { + final HttpStatus status = HttpStatus.NOT_FOUND; + ProblemDetail problem = ProblemDetail.forStatus(status); + + problem.setProperty("timestamp", new Date()); + + problem.setProperty("requestId", exchange.getRequest().getId()); + + problem.setType(uriComponentsBuilder.replacePath(contextPath).path("/problem/shard-not-owned").build().toUri()); + StringBuilder stringBuilder = new StringBuilder(); + stringBuilder.append(status.getReasonPhrase()); + stringBuilder.append(" - "); + stringBuilder.append(e.getMessage()); + problem.setTitle(stringBuilder.toString()); + + stringBuilder.setLength(0); + stringBuilder.append("Shard not owned: "); + stringBuilder.append(e.getShard()); + problem.setDetail(stringBuilder.toString()); + + problem.setProperty("shard", e.getShard()); + + return problem; + } + @ExceptionHandler(MessageMutationException.class) public final ProblemDetail handleException( MessageMutationException e, diff --git a/src/main/java/de/juplo/kafka/chat/backend/domain/ShardNotOwnedException.java b/src/main/java/de/juplo/kafka/chat/backend/domain/ShardNotOwnedException.java new file mode 100644 index 00000000..3b638331 --- /dev/null +++ b/src/main/java/de/juplo/kafka/chat/backend/domain/ShardNotOwnedException.java @@ -0,0 +1,17 @@ +package de.juplo.kafka.chat.backend.domain; + +import lombok.Getter; + + +public class ShardNotOwnedException extends IllegalStateException +{ + @Getter + private final int shard; + + + public ShardNotOwnedException(int shard) + { + super("This instance does not own the shard " + shard); + this.shard = shard; + } +} diff --git a/src/main/java/de/juplo/kafka/chat/backend/domain/ShardedChatHome.java b/src/main/java/de/juplo/kafka/chat/backend/domain/ShardedChatHome.java index 4b8c7f16..6d2f0794 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/domain/ShardedChatHome.java +++ b/src/main/java/de/juplo/kafka/chat/backend/domain/ShardedChatHome.java @@ -40,7 +40,10 @@ public class ShardedChatHome implements ChatHome @Override public Mono getChatRoom(UUID id) { - return chatHomes[selectShard(id)].getChatRoom(id); + int shard = selectShard(id); + if (chatHomes[shard] == null) + throw new ShardNotOwnedException(shard); + return chatHomes[shard].getChatRoom(id); } @Override diff --git a/src/test/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerTest.java b/src/test/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerTest.java index b72294d9..f66be459 100644 --- a/src/test/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerTest.java +++ b/src/test/java/de/juplo/kafka/chat/backend/api/ChatBackendControllerTest.java @@ -1,5 +1,6 @@ package de.juplo.kafka.chat.backend.api; +import de.juplo.kafka.chat.backend.ChatBackendProperties; import de.juplo.kafka.chat.backend.domain.*; import de.juplo.kafka.chat.backend.persistence.inmemory.InMemoryChatHomeService; import lombok.extern.slf4j.Slf4j; @@ -15,7 +16,10 @@ import reactor.core.publisher.Mono; import java.time.Clock; import java.time.LocalDateTime; +import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -23,11 +27,19 @@ import static org.mockito.Mockito.*; @SpringBootTest(properties = { "spring.main.allow-bean-definition-overriding=true", - "chat.backend.inmemory.sharding-strategy=none" }) + "chat.backend.inmemory.sharding-strategy=kafkalike", + "chat.backend.inmemory.num-shards=10", + "chat.backend.inmemory.owned-shards=6", + }) @AutoConfigureWebTestClient @Slf4j public class ChatBackendControllerTest { + @Autowired + ChatBackendProperties properties; + @Autowired + ShardingStrategy shardingStrategy; + @MockBean InMemoryChatHomeService chatHomeService; @MockBean @@ -38,7 +50,7 @@ public class ChatBackendControllerTest void testUnknownChatroomExceptionForListChatroom(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); // When @@ -58,7 +70,7 @@ public class ChatBackendControllerTest void testUnknownChatroomExceptionForGetChatroom(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); // When @@ -77,7 +89,7 @@ public class ChatBackendControllerTest void testUnknownChatroomExceptionForPutMessage(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); String username = "foo"; Long messageId = 66l; when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); @@ -103,7 +115,7 @@ public class ChatBackendControllerTest void testUnknownChatroomExceptionForGetMessage(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); String username = "foo"; Long messageId = 66l; when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); @@ -128,7 +140,7 @@ public class ChatBackendControllerTest void testUnknownChatroomExceptionForListenChatroom(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); // When @@ -155,10 +167,10 @@ public class ChatBackendControllerTest @Test @DisplayName("Assert expected problem-details for message mutation on PUT /put/{chatroomId}/{username}/{messageId}") - void testMessageMutationException(@Autowired WebTestClient client) throws Exception + void testMessageMutationException(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); String user = "foo"; Long messageId = 66l; Message.MessageKey key = Message.MessageKey.of(user, messageId); @@ -211,10 +223,10 @@ public class ChatBackendControllerTest @Test @DisplayName("Assert expected problem-details for invalid username on PUT /put/{chatroomId}/{username}/{messageId}") - void testInvalidUsernameException(@Autowired WebTestClient client) throws Exception + void testInvalidUsernameException(@Autowired WebTestClient client) { // Given - UUID chatroomId = UUID.randomUUID(); + UUID chatroomId = getRandomIdForOwnedShard(); String user = "Foo"; Long messageId = 66l; Message.MessageKey key = Message.MessageKey.of(user, messageId); @@ -251,4 +263,157 @@ public class ChatBackendControllerTest .jsonPath("$.username").isEqualTo(user); verify(chatRoomService, never()).persistMessage(eq(key), any(LocalDateTime.class), any(String.class)); } + + @Test + @DisplayName("Assert expected problem-details for not owned shard on GET /{chatroomId}") + void testShardNotOwnedExceptionForGetChatroom(@Autowired WebTestClient client) + { + // Given + UUID chatroomId = getRandomIdForForeignShard(); + + // When + WebTestClient.ResponseSpec responseSpec = client + .get() + .uri("/{chatroomId}", chatroomId) + .accept(MediaType.APPLICATION_JSON) + .exchange(); + + // Then + assertProblemDetailsForShardNotOwnedException(responseSpec, shardingStrategy.selectShard(chatroomId)); + } + + @Test + @DisplayName("Assert expected problem-details for not owned shard on GET /list/{chatroomId}") + void testShardNotOwnedExceptionForListChatroom(@Autowired WebTestClient client) + { + // Given + UUID chatroomId = getRandomIdForForeignShard(); + + // When + WebTestClient.ResponseSpec responseSpec = client + .get() + .uri("/{chatroomId}/list", chatroomId) + .accept(MediaType.APPLICATION_JSON) + .exchange(); + + // Then + assertProblemDetailsForShardNotOwnedException(responseSpec, shardingStrategy.selectShard(chatroomId)); + } + + @Test + @DisplayName("Assert expected problem-details for now owned shard on PUT /put/{chatroomId}/{username}/{messageId}") + void testShardNotOwnedExceptionForPutMessage(@Autowired WebTestClient client) + { + // Given + UUID chatroomId = getRandomIdForForeignShard(); + String username = "foo"; + Long messageId = 66l; + when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); + + // When + WebTestClient.ResponseSpec responseSpec = client + .put() + .uri( + "/{chatroomId}/{username}/{messageId}", + chatroomId, + username, + messageId) + .bodyValue("bar") + .accept(MediaType.APPLICATION_JSON) + .exchange(); + + // Then + assertProblemDetailsForShardNotOwnedException(responseSpec, shardingStrategy.selectShard(chatroomId)); + } + + @Test + @DisplayName("Assert expected problem-details for not owned shard on GET /get/{chatroomId}/{username}/{messageId}") + void testShardNotOwnedExceptionForGetMessage(@Autowired WebTestClient client) + { + // Given + UUID chatroomId = getRandomIdForForeignShard(); + String username = "foo"; + Long messageId = 66l; + when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); + + // When + WebTestClient.ResponseSpec responseSpec = client + .get() + .uri( + "/{chatroomId}/{username}/{messageId}", + chatroomId, + username, + messageId) + .accept(MediaType.APPLICATION_JSON) + .exchange(); + + // Then + assertProblemDetailsForShardNotOwnedException(responseSpec, shardingStrategy.selectShard(chatroomId)); + } + + @Test + @DisplayName("Assert expected problem-details for not owned shard on GET /listen/{chatroomId}") + void testShardNotOwnedExceptionForListenChatroom(@Autowired WebTestClient client) + { + // Given + UUID chatroomId = getRandomIdForForeignShard(); + when(chatHomeService.getChatRoom(anyInt(), any(UUID.class))).thenReturn(Mono.empty()); + + // When + WebTestClient.ResponseSpec responseSpec = client + .get() + .uri("/{chatroomId}/listen", chatroomId) + // .accept(MediaType.TEXT_EVENT_STREAM, MediaType.APPLICATION_JSON) << TODO: Does not work! + .exchange(); + + // Then + assertProblemDetailsForShardNotOwnedException(responseSpec, shardingStrategy.selectShard(chatroomId)); + } + + private void assertProblemDetailsForShardNotOwnedException( + WebTestClient.ResponseSpec responseSpec, + int shard) + { + responseSpec + .expectStatus().isNotFound() + .expectBody() + .jsonPath("$.type").isEqualTo("/problem/shard-not-owned") + .jsonPath("$.shard").isEqualTo(shard); + } + + private UUID getRandomIdForOwnedShard() + { + Set ownedShards = ownedShards(); + UUID randomId; + + do + { + randomId = UUID.randomUUID(); + } + while (!ownedShards.contains(shardingStrategy.selectShard(randomId))); + + return randomId; + } + + private UUID getRandomIdForForeignShard() + { + Set ownedShards = ownedShards(); + UUID randomId; + + do + { + randomId = UUID.randomUUID(); + } + while (ownedShards.contains(shardingStrategy.selectShard(randomId))); + + return randomId; + } + + private Set ownedShards() + { + return IntStream + .of(properties.getInmemory().getOwnedShards()) + .mapToObj(shard -> Integer.valueOf(shard)) + .collect(Collectors.toSet()); + } } -- 2.20.1