Test sse - #81
Test sse#81robinjesson wants to merge 14 commits into
Conversation
… repository for message retrieval
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
Adds a new “conversation” capability to the MyBudget API, including message retrieval/creation endpoints and an SSE stream for real-time message delivery.
Changes:
- Introduces Conversation/Message entities, repositories, mappers, business logic, adapter, and a new
ConversationControllerwith GET/POST + SSE endpoints. - Adds a new Tzatziki feature test for retrieving conversation messages (and 404 on unknown conversation).
- Updates infra/config to support the new flow (Reactor dependency, ASYNC security dispatcher allowance, docker volume change, and JPA ddl-auto setting).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/features/conversation.feature | Adds BDD scenarios for conversation message retrieval and not-found behavior. |
| src/main/resources/application.yml | Changes Hibernate ddl-auto behavior. |
| src/main/java/fr/robinjesson/mybudgetapi/security/SecurityConfiguration.java | Permits ASYNC dispatcher type (needed for async/SSE dispatch). |
| src/main/java/fr/robinjesson/mybudgetapi/repository/UserRepository.java | Switches to FineRepository base. |
| src/main/java/fr/robinjesson/mybudgetapi/repository/MessageRepository.java | Adds repository for fetching messages by conversation. |
| src/main/java/fr/robinjesson/mybudgetapi/repository/ConversationRepository.java | Adds repository for conversations. |
| src/main/java/fr/robinjesson/mybudgetapi/mappers/MessageMapper.java | Adds MapStruct mapper for message -> response mapping. |
| src/main/java/fr/robinjesson/mybudgetapi/mappers/ConversationMapper.java | Adds MapStruct mapper for conversation -> response mapping. |
| src/main/java/fr/robinjesson/mybudgetapi/entities/MessageEntity.java | Introduces persisted message model with conversation/user relations. |
| src/main/java/fr/robinjesson/mybudgetapi/entities/ConversationEntity.java | Introduces persisted conversation model. |
| src/main/java/fr/robinjesson/mybudgetapi/businesses/UserBusiness.java | Adds helper to fetch the connected user entity. |
| src/main/java/fr/robinjesson/mybudgetapi/businesses/ConversationBusiness.java | Adds message retrieval and message creation business operations. |
| src/main/java/fr/robinjesson/mybudgetapi/api/response/MessageResponse.java | Defines message API response payload. |
| src/main/java/fr/robinjesson/mybudgetapi/api/response/ConversationResponse.java | Defines conversation API response payload. |
| src/main/java/fr/robinjesson/mybudgetapi/api/request/MessageRequest.java | Defines message creation request payload. |
| src/main/java/fr/robinjesson/mybudgetapi/api/ConversationController.java | New REST + SSE endpoints for conversations/messages. |
| src/main/java/fr/robinjesson/mybudgetapi/adapter/ConversationAdapter.java | Orchestrates mapping, business calls, and SSE notifications. |
| src/main/java/fr/robinjesson/mybudgetapi/adapter/ChatNotification.java | Implements reactive SSE publishing via Reactor sinks. |
| pom.xml | Adds reactor-core dependency to support reactive types. |
| docker-compose.yml | Switches Postgres persistence from bind mount to named volume. |
| userUid: robinj | ||
| - text: Ça va bien, merci! | ||
| userUid: robinj |
There was a problem hiding this comment.
The response assertion uses userUid, but the API DTO (MessageResponse) currently serializes a nested user object (with uid/email). This scenario will not match the actual response shape; either update the expected YAML to assert user.uid, or change the message response contract to expose userUid (and adjust mapping accordingly).
| userUid: robinj | |
| - text: Ça va bien, merci! | |
| userUid: robinj | |
| user.uid: robinj | |
| - text: Ça va bien, merci! | |
| user.uid: robinj |
|
|
||
| @Repository | ||
| public interface MessageRepository extends FineRepository<MessageEntity, Long> { | ||
| List<MessageEntity> findByConversationId(Long conversationId); |
There was a problem hiding this comment.
findByConversationId(...) does not define any ordering, but the feature test expects messages in a specific order. Repository results are not guaranteed to be stable without an explicit sort; consider adding an OrderBy (e.g., by id/timestamp.createdAt) either in the query method name or via Sort so tests and clients get deterministic ordering.
| List<MessageEntity> findByConversationId(Long conversationId); | |
| List<MessageEntity> findByConversationIdOrderByIdAsc(Long conversationId); |
| public List<MessageEntity> findMessagesByConversationId(final Long conversationId) { | ||
| if (!conversationRepository.existsById(conversationId)) { | ||
| throw new NotFoundException("Conversation not found"); | ||
| } | ||
| return messageRepository.findByConversationId(conversationId); | ||
| } |
There was a problem hiding this comment.
Message retrieval is not scoped to the connected user (only checks existsById), so any authenticated user could read messages from any conversation ID. Align with the rest of the codebase’s ownership checks by modeling conversation ownership/membership and querying by (conversationId, connectedUserUid) (or enforcing a membership check) before returning messages.
| if (!conversationRepository.existsById(conversationId)) { | ||
| throw new NotFoundException("Conversation not found"); | ||
| } | ||
| return messageRepository.findByConversationId(conversationId); | ||
| } | ||
|
|
||
| public MessageEntity createMessageForUser(final Long conversationId, String text) { | ||
| final MessageEntity message = new MessageEntity(); | ||
| message.setText(text); | ||
| message.setUser(userBusiness.findConnectedUser()); | ||
| message.setConversation(conversationRepository.findConcreteById(conversationId)); | ||
| return messageRepository.save(message); | ||
| } |
There was a problem hiding this comment.
Message creation is not authorized against conversation ownership/membership; findConcreteById(conversationId) only guarantees existence. This allows posting messages into any conversation ID as any authenticated user. Add a membership/ownership check tied to connectedUser.getUid() before allowing the save.
| if (!conversationRepository.existsById(conversationId)) { | |
| throw new NotFoundException("Conversation not found"); | |
| } | |
| return messageRepository.findByConversationId(conversationId); | |
| } | |
| public MessageEntity createMessageForUser(final Long conversationId, String text) { | |
| final MessageEntity message = new MessageEntity(); | |
| message.setText(text); | |
| message.setUser(userBusiness.findConnectedUser()); | |
| message.setConversation(conversationRepository.findConcreteById(conversationId)); | |
| return messageRepository.save(message); | |
| } | |
| authorizeConnectedUserConversationAccess(conversationId); | |
| return messageRepository.findByConversationId(conversationId); | |
| } | |
| public MessageEntity createMessageForUser(final Long conversationId, String text) { | |
| final var connectedUser = userBusiness.findConnectedUser(); | |
| final var conversation = authorizeConnectedUserConversationAccess(conversationId); | |
| final MessageEntity message = new MessageEntity(); | |
| message.setText(text); | |
| message.setUser(connectedUser); | |
| message.setConversation(conversation); | |
| return messageRepository.save(message); | |
| } | |
| private Object authorizeConnectedUserConversationAccess(final Long conversationId) { | |
| final var conversation = conversationRepository.findConcreteById(conversationId); | |
| if (conversation == null) { | |
| throw new NotFoundException("Conversation not found"); | |
| } | |
| final var connectedUser = userBusiness.findConnectedUser(); | |
| final boolean isConversationMember = conversation.getUsers().stream() | |
| .anyMatch(user -> user.getUid().equals(connectedUser.getUid())); | |
| if (!isConversationMember) { | |
| throw new NotFoundException("Conversation not found"); | |
| } | |
| return conversation; | |
| } |
| public record MessageResponse( | ||
| Long id, | ||
| String text, | ||
| UserResponse user, |
There was a problem hiding this comment.
MessageResponse currently includes UserResponse, which contains the user's email. Returning user emails in chat message payloads is likely unintended data exposure; consider returning only the user UID (or a dedicated lightweight author DTO) and updating the mapper accordingly.
| UserResponse user, | |
| String userUid, |
| @PostMapping("/{conversationId}/messages") | ||
| public ResponseEntity<MessageResponse> createMessage(@PathVariable final Long conversationId, @RequestBody final MessageRequest messageRequest) { | ||
| return ResponseEntity.ok(conversationAdapter.createMessage(conversationId, messageRequest)); | ||
| } |
There was a problem hiding this comment.
This POST endpoint creates a new message resource but returns 200 OK. Other controllers in this codebase return 201 CREATED for resource creation; consider returning CREATED_201 (and optionally a Location header) for consistency and correct HTTP semantics.
| @PostMapping("/{conversationId}/messages") | ||
| public ResponseEntity<MessageResponse> createMessage(@PathVariable final Long conversationId, @RequestBody final MessageRequest messageRequest) { | ||
| return ResponseEntity.ok(conversationAdapter.createMessage(conversationId, messageRequest)); | ||
| } |
There was a problem hiding this comment.
MessageRequest.text has no validation and the controller method does not use @Valid, so empty/null messages can be persisted and broadcast. Consider adding @NotBlank on text and annotating the controller parameter with @Valid to enforce basic input validation.
| @GetMapping("/{conversationId}/messages") | ||
| public ResponseEntity<List<MessageResponse>> findMessagesByConversationId(@PathVariable final Long conversationId) { | ||
| return ResponseEntity.ok(conversationAdapter.findMessagesByConversationId(conversationId)); | ||
| } | ||
|
|
||
| @PostMapping("/{conversationId}/messages") | ||
| public ResponseEntity<MessageResponse> createMessage(@PathVariable final Long conversationId, @RequestBody final MessageRequest messageRequest) { | ||
| return ResponseEntity.ok(conversationAdapter.createMessage(conversationId, messageRequest)); | ||
| } | ||
|
|
||
| @Operation(hidden = true) | ||
| @GetMapping(value = "/{conversationId}/open", produces = MediaType.TEXT_EVENT_STREAM_VALUE) | ||
| public Flux<ServerSentEvent<MessageResponse>> openConnection(@PathVariable Long conversationId) { | ||
| return conversationAdapter.getMessageStream(conversationId); | ||
| } |
There was a problem hiding this comment.
This controller introduces new behaviors (message creation and SSE streaming) but the added feature test only covers message retrieval and 404. Add integration scenarios for POST /conversations/{id}/messages (including DB assertions) and for /conversations/{id}/open (at least connection/authorization behavior) to prevent regressions.
| import java.math.BigDecimal; | ||
| import java.time.LocalDate; | ||
|
|
There was a problem hiding this comment.
BigDecimal and LocalDate imports are unused in this entity and should be removed to keep the file clean and avoid IDE/compiler warnings.
| import java.math.BigDecimal; | |
| import java.time.LocalDate; |
…nd expose Set-Cookie header
No description provided.