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.

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 4db77ee..339451a 100644 (file)
--- 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<MessageTo> put(
+  private Mono<MessageTo> 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 55350f1..ad90c4b 100644 (file)
--- 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 (file)
index 0000000..3b63833
--- /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 4b8c7f1..6d2f079 100644 (file)
--- 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<ChatRoom> 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 b72294d..aea72dc 100644 (file)
--- 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 unknown chatroom 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<Integer> ownedShards = ownedShards();
+    UUID randomId;
+
+    do
+    {
+      randomId = UUID.randomUUID();
+    }
+    while (!ownedShards.contains(shardingStrategy.selectShard(randomId)));
+
+    return randomId;
+  }
+
+  private UUID getRandomIdForForeignShard()
+  {
+    Set<Integer> ownedShards = ownedShards();
+    UUID randomId;
+
+    do
+    {
+      randomId = UUID.randomUUID();
+    }
+    while (ownedShards.contains(shardingStrategy.selectShard(randomId)));
+
+    return randomId;
+  }
+
+  private Set<Integer> ownedShards()
+  {
+    return IntStream
+        .of(properties.getInmemory().getOwnedShards())
+        .mapToObj(shard -> Integer.valueOf(shard))
+        .collect(Collectors.toSet());
+  }
 }