From 972a1f2c248a51740091c1fdf3526f0eb676639e Mon Sep 17 00:00:00 2001 From: Kai Moritz Date: Sun, 8 Jan 2023 21:30:57 +0100 Subject: [PATCH] refactor: Moved business-logic from `ChatRoomService` into `ChatRoom` - Some essential business-logic -- the identification of mutated messages -- was buried in `InMemoryChatRoomService`. - This business-logic was moved into `ChatRoom`, becaus otherwise, it would have been necessary, to reproduce this logic in each and every new implementation of `ChatRoomService`, which would have been exhausting and errorprone. - This allowed also cleaner code in `InMemoryChatRoomService`, that can focus on the persistence-logic. - The implementation of `MessageMutationException` and the look of the accompanying problem-details hat to be refined accordingly. --- .../backend/api/ChatBackendController.java | 1 - .../api/ChatBackendControllerAdvice.java | 4 +- .../kafka/chat/backend/domain/ChatRoom.java | 25 ++-- .../chat/backend/domain/ChatRoomService.java | 2 +- .../domain/MessageMutationException.java | 8 +- .../persistence/InMemoryChatRoomService.java | 22 +-- .../api/ChatBackendControllerTest.java | 8 +- .../chat/backend/domain/ChatRoomTest.java | 130 ++++++++++++++++++ 8 files changed, 159 insertions(+), 41 deletions(-) create mode 100644 src/test/java/de/juplo/kafka/chat/backend/domain/ChatRoomTest.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 68d056ba..51ed6a20 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 @@ -73,7 +73,6 @@ public class ChatBackendController messageId, username, text) - .switchIfEmpty(chatroom.getMessage(username, messageId)) .map(message -> MessageTo.from(message)); } 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 ab8c13d4..fa3a0e16 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 @@ -77,10 +77,10 @@ public class ChatBackendControllerAdvice stringBuilder.append(" cannot be mutated!"); problem.setDetail(stringBuilder.toString()); - problem.setProperty("mutatedMessage", MessageTo.from(e.getMutated())); - problem.setProperty("existingMessage", MessageTo.from(e.getExisting())); + problem.setProperty("mutatedText", e.getMutatedText()); + return problem; } } diff --git a/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoom.java b/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoom.java index 22eebffd..58efa54f 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoom.java +++ b/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoom.java @@ -48,16 +48,23 @@ public class ChatRoom String user, String text) { + Message.MessageKey key = Message.MessageKey.of(user, id); return service - .persistMessage(Message.MessageKey.of(user, id), LocalDateTime.now(clock), text) - .doOnNext(message -> - { - Sinks.EmitResult result = sink.tryEmitNext(message); - if (result.isFailure()) - { - log.warn("Emitting of message failed with {} for {}", result.name(), message); - } - }); + .getMessage(key) + .flatMap(existing -> text.equals(existing.getMessageText()) + ? Mono.just(existing) + : Mono.error(() -> new MessageMutationException(existing, text))) + .switchIfEmpty( + Mono + .just(service.persistMessage(key, LocalDateTime.now(clock), text)) + .doOnNext(m -> + { + Sinks.EmitResult result = sink.tryEmitNext(m); + if (result.isFailure()) + { + log.warn("Emitting of message failed with {} for {}", result.name(), m); + } + })); } diff --git a/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoomService.java b/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoomService.java index 374a442b..c70ffe4e 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoomService.java +++ b/src/main/java/de/juplo/kafka/chat/backend/domain/ChatRoomService.java @@ -8,7 +8,7 @@ import java.time.LocalDateTime; public interface ChatRoomService { - Mono persistMessage( + Message persistMessage( Message.MessageKey key, LocalDateTime timestamp, String text); diff --git a/src/main/java/de/juplo/kafka/chat/backend/domain/MessageMutationException.java b/src/main/java/de/juplo/kafka/chat/backend/domain/MessageMutationException.java index 0408de3a..3f027923 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/domain/MessageMutationException.java +++ b/src/main/java/de/juplo/kafka/chat/backend/domain/MessageMutationException.java @@ -5,15 +5,15 @@ import lombok.Getter; public class MessageMutationException extends RuntimeException { - @Getter - private final Message mutated; @Getter private final Message existing; + @Getter + private final String mutatedText; - public MessageMutationException(Message mutated, Message existing) + public MessageMutationException(Message existing, String mutatedText) { super("Messages are imutable!"); - this.mutated = mutated; this.existing = existing; + this.mutatedText = mutatedText; } } diff --git a/src/main/java/de/juplo/kafka/chat/backend/persistence/InMemoryChatRoomService.java b/src/main/java/de/juplo/kafka/chat/backend/persistence/InMemoryChatRoomService.java index 1831037e..49d400b9 100644 --- a/src/main/java/de/juplo/kafka/chat/backend/persistence/InMemoryChatRoomService.java +++ b/src/main/java/de/juplo/kafka/chat/backend/persistence/InMemoryChatRoomService.java @@ -1,7 +1,6 @@ package de.juplo.kafka.chat.backend.persistence; import de.juplo.kafka.chat.backend.domain.Message; -import de.juplo.kafka.chat.backend.domain.MessageMutationException; import de.juplo.kafka.chat.backend.domain.ChatRoomService; import lombok.extern.slf4j.Slf4j; import reactor.core.publisher.Flux; @@ -26,32 +25,17 @@ public class InMemoryChatRoomService implements ChatRoomService { log.debug("Creating InMemoryChatroomService"); messages = new LinkedHashMap<>(); - messageFlux.subscribe(message -> persistMessage(message)); + messageFlux.subscribe(message -> messages.put(message.getKey(), message)); } @Override - public Mono persistMessage( + public Message persistMessage( Message.MessageKey key, LocalDateTime timestamp, String text) { Message message = new Message(key, (long)messages.size(), timestamp, text); - return Mono.justOrEmpty(persistMessage(message)); - } - - private Message persistMessage(Message message) - { - Message.MessageKey key = message.getKey(); - Message existing = messages.get(key); - if (existing != null) - { - log.info("Message with key {} already exists; {}", key, existing); - if (!message.equals(existing)) - throw new MessageMutationException(message, existing); - return null; - } - - messages.put(key, message); + messages.put(message.getKey(), message); return message; } 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 404fab25..8947a03c 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 @@ -13,7 +13,6 @@ import org.springframework.test.web.reactive.server.WebTestClient; import reactor.core.publisher.Mono; import java.time.LocalDateTime; -import java.util.Optional; import java.util.UUID; import static org.mockito.ArgumentMatchers.any; @@ -167,10 +166,9 @@ public class ChatBackendControllerTest .thenReturn(Mono.just(chatRoom)); Message.MessageKey key = Message.MessageKey.of("foo", 1l); LocalDateTime timestamp = LocalDateTime.now(); - Message mutated = new Message(key, 0l, timestamp, "Mutated!"); Message existing = new Message(key, 0l, timestamp, "Existing"); when(chatRoom.addMessage(any(Long.class), any(String.class), any(String.class))) - .thenThrow(new MessageMutationException(mutated, existing)); + .thenReturn(Mono.error(() -> new MessageMutationException(existing, "Mutated!"))); // When client @@ -187,7 +185,7 @@ public class ChatBackendControllerTest .expectStatus().is4xxClientError() .expectBody() .jsonPath("$.type").isEqualTo("/problem/message-mutation") - .jsonPath("$.mutatedMessage.text").isEqualTo("Mutated!") - .jsonPath("$.existingMessage.text").isEqualTo("Existing"); + .jsonPath("$.existingMessage.text").isEqualTo("Existing") + .jsonPath("$.mutatedText").isEqualTo("Mutated!"); } } diff --git a/src/test/java/de/juplo/kafka/chat/backend/domain/ChatRoomTest.java b/src/test/java/de/juplo/kafka/chat/backend/domain/ChatRoomTest.java new file mode 100644 index 00000000..f513c407 --- /dev/null +++ b/src/test/java/de/juplo/kafka/chat/backend/domain/ChatRoomTest.java @@ -0,0 +1,130 @@ +package de.juplo.kafka.chat.backend.domain; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import reactor.core.publisher.Mono; + +import java.time.Clock; +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.UUID; + +import static org.mockito.Mockito.*; +import static pl.rzrz.assertj.reactor.Assertions.assertThat; + + +public class ChatRoomTest +{ + @Test + @DisplayName("Assert, that Mono emits expected message, if it exists") + void testGetExistingMessage() + { + // Given + String user = "foo"; + Long messageId = 1l; + ChatRoomService chatRoomService = mock(ChatRoomService.class); + ChatRoom chatRoom = new ChatRoom(UUID.randomUUID(), "Foo", Clock.systemDefaultZone(), chatRoomService, 8); + Message.MessageKey key = Message.MessageKey.of(user, messageId); + LocalDateTime timestamp = LocalDateTime.now(); + Message message = new Message(key, 0l, timestamp, "Bar"); + when(chatRoomService.getMessage(any(Message.MessageKey.class))).thenReturn(Mono.just(message)); + + // When + Mono mono = chatRoom.getMessage(user, messageId); + + // Then + assertThat(mono).emitsExactly(message); + } + + @Test + @DisplayName("Assert, that Mono if empty, if message does not exists") + void testGetNonExistentMessage() + { + // Given + String user = "foo"; + Long messageId = 1l; + ChatRoomService chatRoomService = mock(ChatRoomService.class); + ChatRoom chatRoom = new ChatRoom(UUID.randomUUID(), "Foo", Clock.systemDefaultZone(), chatRoomService, 8); + when(chatRoomService.getMessage(any(Message.MessageKey.class))).thenReturn(Mono.empty()); + + // When + Mono mono = chatRoom.getMessage(user, messageId); + + // Then + assertThat(mono).emitsCount(0); + } + + @Test + @DisplayName("Assert, that Mono emits expected message, if a new message is added") + void testAddNewMessage() + { + // Given + String user = "foo"; + Long messageId = 1l; + ChatRoomService chatRoomService = mock(ChatRoomService.class); + ChatRoom chatRoom = new ChatRoom(UUID.randomUUID(), "Foo", Clock.systemDefaultZone(), chatRoomService, 8); + Message.MessageKey key = Message.MessageKey.of(user, messageId); + Clock now = Clock.fixed(Instant.now(), ZoneId.systemDefault()); + LocalDateTime timestamp = LocalDateTime.now(now); + String messageText = "Bar"; + Message message = new Message(key, 0l, timestamp, messageText); + when(chatRoomService.getMessage(any(Message.MessageKey.class))).thenReturn(Mono.empty()); + when(chatRoomService.persistMessage(any(Message.MessageKey.class), any(LocalDateTime.class), any(String.class))).thenReturn(message); + + // When + Mono mono = chatRoom.addMessage(messageId, user, messageText); + + // Then + assertThat(mono).emitsExactly(message); + } + + @Test + @DisplayName("Assert, that Mono emits expected message, if an unchanged message is added") + void testAddUnchangedMessage() + { + // Given + String user = "foo"; + Long messageId = 1l; + ChatRoomService chatRoomService = mock(ChatRoomService.class); + ChatRoom chatRoom = new ChatRoom(UUID.randomUUID(), "Foo", Clock.systemDefaultZone(), chatRoomService, 8); + Message.MessageKey key = Message.MessageKey.of(user, messageId); + Clock now = Clock.fixed(Instant.now(), ZoneId.systemDefault()); + LocalDateTime timestamp = LocalDateTime.now(now); + String messageText = "Bar"; + Message message = new Message(key, 0l, timestamp, messageText); + when(chatRoomService.getMessage(any(Message.MessageKey.class))).thenReturn(Mono.just(message)); + when(chatRoomService.persistMessage(any(Message.MessageKey.class), any(LocalDateTime.class), any(String.class))).thenReturn(message); + + // When + Mono mono = chatRoom.addMessage(messageId, user, messageText); + + // Then + assertThat(mono).emitsExactly(message); + } + + @Test + @DisplayName("Assert, that Mono sends an error, if a message is added again with mutated text") + void testAddMutatedMessage() + { + // Given + String user = "foo"; + Long messageId = 1l; + ChatRoomService chatRoomService = mock(ChatRoomService.class); + ChatRoom chatRoom = new ChatRoom(UUID.randomUUID(), "Foo", Clock.systemDefaultZone(), chatRoomService, 8); + Message.MessageKey key = Message.MessageKey.of(user, messageId); + Clock now = Clock.fixed(Instant.now(), ZoneId.systemDefault()); + LocalDateTime timestamp = LocalDateTime.now(now); + String messageText = "Bar"; + String mutatedText = "Boom!"; + Message message = new Message(key, 0l, timestamp, messageText); + when(chatRoomService.getMessage(any(Message.MessageKey.class))).thenReturn(Mono.just(message)); + when(chatRoomService.persistMessage(any(Message.MessageKey.class), any(LocalDateTime.class), any(String.class))).thenReturn(message); + + // When + Mono mono = chatRoom.addMessage(messageId, user, mutatedText); + + // Then + assertThat(mono).sendsError(); + } +} -- 2.20.1