Conversation
Walkthrough웹소켓 STOMP 엔드포인트( Changes
Sequence Diagram(s)sequenceDiagram
participant ClientA as 청중(Client)
participant ClientB as 사회자(Client)
participant Server as App (STOMP)
participant RoomListener as RoomSubscribeListener
participant SharingCtrl as SharingController
Note right of Server `#eef6ff`: STOMP 엔드포인트 `/ws`, 브로커 `/room`·`/chairman`
ClientA->>Server: /ws 연결 후 /room/{roomId} 구독\n(Authorization 헤더 포함)
Server-->>RoomListener: SessionSubscribeEvent 전달
RoomListener->>Server: /chairman/{roomId}에 ChairmanSharingRequest 발행
ClientB->>Server: /app/event/{roomId}로 SharingRequest 전송
Server->>SharingCtrl: `@MessageMapping` 처리 (WebSocketAuthMemberResolver로 AuthMember 주입)
SharingCtrl->>Server: /room/{roomId}으로 SharingResponse 발행
Server->>ClientA: SharingResponse 전달
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results124 files 124 suites 14s ⏱️ Results for commit 0dc1af8. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/java/com/debatetimer/dto/sharing/request/ChairmanSharingRequest.java (1)
3-5: roomId 유효성 검증을 고려하세요.현재는 문제가 없지만, 향후 유지보수성을 위해
@Positive또는@Min(1)어노테이션을 추가하여 유효하지 않은 roomId가 전달되는 것을 방지하는 것을 고려해보세요.package com.debatetimer.dto.sharing.request; +import jakarta.validation.constraints.Positive; + public record ChairmanSharingRequest( + @Positive long roomId ) { }src/main/java/com/debatetimer/dto/sharing/request/SharingRequest.java (1)
5-7: time 필드에 대한 유효성 검증을 고려하세요.현재는 문제가 없지만,
@NotNull어노테이션을 추가하여 null 값이 전달되는 것을 방지하는 것을 고려해보세요.package com.debatetimer.dto.sharing.request; +import jakarta.validation.constraints.NotNull; import java.time.LocalDateTime; public record SharingRequest( + @NotNull LocalDateTime time ) { }src/main/java/com/debatetimer/config/sharing/WebSocketAuthMemberResolver.java (1)
23-38: @AuthMember 파라미터 타입/토큰 포맷 계약 재확인 제안구현 자체는 단순하고 명확한데, 두 가지 정도만 한 번 더 확인하면 좋을 것 같습니다.
supportsParameter
현재는 애노테이션만 체크하고 있어, 실수로@AuthMember를 전혀 다른 타입 파라미터에 붙여도 통과합니다. 실제로 주입하려는 도메인 타입(예:Member)이 정해져 있다면, 타입까지 함께 검사하면 오용을 줄일 수 있습니다.Authorization 헤더 포맷
getFirstNativeHeader(HttpHeaders.AUTHORIZATION)로 읽은 값을 그대로resolveAccessToken(token)에 넘기고 있는데,
"Bearer <token>"형태인지- 혹은 순수 토큰 문자열인지
에 대한 계약이AuthManager쪽에 명확히 정의되어 있는지만 다시 한 번 점검 부탁드립니다. (빈 문자열 / 공백만 있는 값에 대한 처리도 함께요.)크게 막히는 부분은 아니지만, 나중에 WebSocket 클라이언트가 늘어날 때 디버깅 비용을 줄이는 데 도움이 될 것 같습니다.
src/test/java/com/debatetimer/MessageFrameHandler.java (1)
17-21: handleFrame의 빈 if 블록 제거 및 타입 캐스팅 명확화 제안
CompletableFuture.complete(...)의 반환값을 사용하지 않으면서 빈if블록으로 감싸고 있어 의도가 혼동될 수 있습니다. 단순 호출로 바꾸고,tClass.cast(payload)를 사용하면 의도와 타입 캐스팅이 더 명확해집니다.@Override public void handleFrame(StompHeaders headers, Object payload) { - if (completableFuture.complete((T) payload)) { - } + completableFuture.complete(tClass.cast(payload)); }테스트용 유틸이라도 이 정도 정리는 유지보수에 도움이 될 것 같습니다.
src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java (1)
24-35: WebSocket CORS 보안을 위해 allowedOriginPatterns 제한 고려현재 STOMP 엔드포인트가 모든 Origin을 허용하도록 설정되어 있습니다.
registry.addEndpoint("/ws") .setAllowedOriginPatterns("*") .withSockJS();개발 환경에서는 편하지만, 운영 환경에서는 불필요한 도메인에서의 WebSocket 접속을 모두 허용하게 되어 보안 측면에서 부담이 될 수 있습니다.
- 운영용 프로파일에서는 구체적인 도메인만 허용하도록 설정하거나
application-*.yml에서 허용 Origin을 외부 설정으로 분리하는 방식을 한 번 검토해 보시는 것을 추천드립니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
build.gradle(1 hunks)src/main/java/com/debatetimer/config/sharing/WebSocketAuthMemberResolver.java(1 hunks)src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java(1 hunks)src/main/java/com/debatetimer/controller/sharing/SharingController.java(1 hunks)src/main/java/com/debatetimer/dto/sharing/request/ChairmanSharingRequest.java(1 hunks)src/main/java/com/debatetimer/dto/sharing/request/SharingRequest.java(1 hunks)src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java(1 hunks)src/main/java/com/debatetimer/event/sharing/RoomSubscribeListener.java(1 hunks)src/test/java/com/debatetimer/BaseStompTest.java(1 hunks)src/test/java/com/debatetimer/MessageFrameHandler.java(1 hunks)src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java(1 hunks)src/test/java/com/debatetimer/event/sharing/RoomSubscribeListenerTest.java(1 hunks)src/test/java/com/debatetimer/fixture/HeaderGenerator.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/debatetimer/config/sharing/WebSocketAuthMemberResolver.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java (1)
src/test/java/com/debatetimer/MessageFrameHandler.java (1)
MessageFrameHandler(8-31)
src/test/java/com/debatetimer/event/sharing/RoomSubscribeListenerTest.java (1)
src/test/java/com/debatetimer/MessageFrameHandler.java (1)
MessageFrameHandler(8-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (6)
src/test/java/com/debatetimer/fixture/HeaderGenerator.java (1)
26-32: LGTM!STOMP 헤더 생성을 위한 메서드 오버로딩이 적절하게 구현되었습니다. 목적지(destination)와 인증 토큰을 포함한 헤더 생성 로직이 올바릅니다.
src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java (1)
5-7: LGTM!WebSocket 응답을 위한 간단하고 명확한 레코드 정의입니다.
src/test/java/com/debatetimer/event/sharing/RoomSubscribeListenerTest.java (1)
19-31: LGTM!구독 리스너의 동작을 올바르게 검증하는 테스트입니다.
/room/{roomId}구독 시/chairman/{roomId}로 메시지가 전달되는 것을 확인합니다.src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java (1)
23-37: LGTM!WebSocket 메시지 전송 및 수신 흐름을 올바르게 테스트합니다. 인증된 사용자의 이벤트 발생과 청중의 수신을 검증하는 로직이 적절합니다. 3초 타임아웃도 테스트 환경에 적합합니다.
build.gradle (1)
45-46: 검증 결과: WebSocket 의존성은 안전합니다.Spring Boot 3.4.0 버전은 Maven Central에 존재하며 확인되었습니다. GitHub 보안 취약점 데이터베이스 조회 결과, org.springframework.boot:spring-boot-starter-websocket에 알려진 보안 취약점이 없습니다. 현재 코드는 문제가 없습니다.
src/main/java/com/debatetimer/controller/sharing/SharingController.java (1)
18-24: 현재 메서드에 권한 검증 로직이나 비즈니스 로직 추가 필요성 재확인 권장코드 검증 결과,
member와roomId파라미터가 실제로 메서드 본문에서 사용되지 않음이 확인되었습니다. 다른 컨트롤러 메서드들과 비교해보면:
- MemberController:
member→member.getId()또는authService.logout(member, ...)전달- PollController:
member→pollService메서드로 전달- CustomizeController:
member→customizeService메서드로 전달이들은 모두 파라미터를 서비스 계층에 전달하여 권한 검증이나 비즈니스 로직 처리에 사용합니다.
현재
SharingController.share()메서드도 유사하게:
member로 방 접근 권한 검증이 필요한지roomId로 공유 대상을 명시적으로 검증해야 하는지재확인한 후, 향후 개발 계획이라면 TODO 코멘트를 추가하시기 바랍니다.
| @EventListener | ||
| public void handleSubscribeEvent(SessionSubscribeEvent event) { | ||
| StompHeaderAccessor accessor = StompHeaderAccessor.wrap(event.getMessage()); | ||
| String destination = accessor.getDestination(); | ||
| if (destination == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (destination.startsWith(AUDIENCE_SUBSCRIBE_PREFIX)) { | ||
| long roomId = Long.parseLong(destination.replace(AUDIENCE_SUBSCRIBE_PREFIX, "")); | ||
| messagingTemplate.convertAndSend(CHAIRMAN_CHANNEL_PREFIX + roomId, new ChairmanSharingRequest(roomId)); | ||
| } |
There was a problem hiding this comment.
구독 destination 파싱 시 잘못된 roomId에 대한 방어 코드 제안
현재 destination이 "/room/"로만 오거나, "/room/123/extra" 같이 예상하지 못한 값이 올 경우 Long.parseLong(...)에서 NumberFormatException이 발생해 리스너 전체가 실패할 수 있습니다.
예외를 로그로 남기고 무시하도록 방어 코드를 두면, 잘못된 구독 요청이 와도 다른 세션 처리에는 영향을 주지 않을 것 같습니다.
@EventListener
public void handleSubscribeEvent(SessionSubscribeEvent event) {
StompHeaderAccessor accessor = StompHeaderAccessor.wrap(event.getMessage());
String destination = accessor.getDestination();
if (destination == null) {
return;
}
if (destination.startsWith(AUDIENCE_SUBSCRIBE_PREFIX)) {
- long roomId = Long.parseLong(destination.replace(AUDIENCE_SUBSCRIBE_PREFIX, ""));
- messagingTemplate.convertAndSend(CHAIRMAN_CHANNEL_PREFIX + roomId, new ChairmanSharingRequest(roomId));
+ String roomIdPart = destination.substring(AUDIENCE_SUBSCRIBE_PREFIX.length());
+ try {
+ long roomId = Long.parseLong(roomIdPart);
+ messagingTemplate.convertAndSend(
+ CHAIRMAN_CHANNEL_PREFIX + roomId,
+ new ChairmanSharingRequest(roomId)
+ );
+ } catch (NumberFormatException e) {
+ log.warn("Invalid room subscription destination: {}", destination, e);
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/debatetimer/event/sharing/RoomSubscribeListener.java around
lines 22 to 33, the code calls Long.parseLong on the destination substring
without validation which can throw NumberFormatException for bad values like
"/room/" or "/room/123/extra"; add defensive validation: after confirming
destination.startsWith(AUDIENCE_SUBSCRIBE_PREFIX) extract the suffix, validate
it contains only digits (or split on '/' and take the first segment) and parse
that; wrap parsing in a try/catch(NumberFormatException) and on error log a
warning including the invalid destination and exception, then return/ignore so
the listener continues handling other events.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
반영해주었습니다... 값 객체로 감싸고 싶긴 했는데 너무 API 규약에 의존하는 느낌이라 순수한 VO 생성이 안될 것 같아 일단 private method로 처리해주었어요.
| @BeforeEach | ||
| public void connect() throws ExecutionException, InterruptedException, TimeoutException { | ||
| this.stompSession = this.websocketClient | ||
| .connectAsync(url + port + SOCKET_ENDPOINT, new StompSessionHandlerAdapter() { | ||
| }) | ||
| .get(3, TimeUnit.SECONDS); | ||
| } | ||
|
|
||
| @AfterEach | ||
| public void disconnect() { | ||
| if (this.stompSession.isConnected()) { | ||
| this.stompSession.disconnect(); | ||
| } | ||
| } |
There was a problem hiding this comment.
connect 실패 시 disconnect에서 NPE 발생 가능성
connect()에서 예외가 발생하면 stompSession이 초기화되지 않을 수 있는데, 이 경우 disconnect()의
if (this.stompSession.isConnected()) {에서 NullPointerException이 발생할 수 있습니다. 테스트 실패 원인을 흐리기도 해서, 널 체크를 추가하는 편이 안전해 보입니다.
@AfterEach
public void disconnect() {
- if (this.stompSession.isConnected()) {
- this.stompSession.disconnect();
- }
+ if (this.stompSession != null && this.stompSession.isConnected()) {
+ this.stompSession.disconnect();
+ }
}🤖 Prompt for AI Agents
In src/test/java/com/debatetimer/BaseStompTest.java around lines 62 to 75, the
AfterEach disconnect() can throw a NullPointerException if connect() failed and
stompSession was never initialized; update disconnect() to first check that
this.stompSession is not null before calling isConnected() (e.g., guard with
this.stompSession != null && this.stompSession.isConnected()) and then call
disconnect() only when both conditions hold.
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
전반적으로 고생 많으셨습니다 생각보다 겁나 빨리 했네요;;
어디 부분까지 구현되어 있고, 어디 부분까지 구현 안되어 있는지 적어주시면 더 좋을 것 같습니다!
| @Override | ||
| public void configureMessageBroker(MessageBrokerRegistry registry) { | ||
| registry.enableSimpleBroker("/room", "/chairman"); | ||
| registry.setApplicationDestinationPrefixes("/app"); | ||
| } |
There was a problem hiding this comment.
나중에 웹소켓 관련 로직이 추가되면, 해당 부분도 추가되야하는거죠?
There was a problem hiding this comment.
네 맞습니다.
-> 메모리에 내장된 Spring이 제공해주는 SimpleBroker에 위임할 prefix는 enableSimpleBroker에 추가
-> MessageMapping을 통해 애플리케이션에서 우리가 처리할 로직은 setApplicationDestinations에 추가
| @Component | ||
| @RequiredArgsConstructor | ||
| public class WebSocketAuthMemberResolver implements HandlerMethodArgumentResolver { | ||
|
|
||
| private final AuthManager authManager; | ||
| private final AuthService authService; | ||
|
|
||
| @Override | ||
| public boolean supportsParameter(MethodParameter parameter) { | ||
| return parameter.hasParameterAnnotation(AuthMember.class); | ||
| } |
There was a problem hiding this comment.
이렇게 하면 웹소켓을 사용하지 않는 @AuthMember가 붙은 곳에도 해당 ArgumentResolver가 적용되는 건가요?
There was a problem hiding this comment.
웹소켓 로직에만 적용됩니다.
웹소켓 MessageMapping 을 통해 애플리케이션에 위임된 메시지 핸들링 로직에서 @AuthMember가 붙은 엔드포인트에서 해당 로직을 인터셉트하여 실행해주어요.
package org.springframework.messaging.handler.invocation;
여기서 messaging이 Spring STOMP 소켓 관련 핸들링 로직이 있는 패키지입니다.
| @MessageMapping("/event/{roomId}") | ||
| @SendTo("/room/{roomId}") | ||
| public SharingResponse share( | ||
| @AuthMember Member member, | ||
| @DestinationVariable(value = "roomId") long roomId, | ||
| @Payload SharingRequest request | ||
| ) { | ||
| return new SharingResponse(request.time()); | ||
| } |
There was a problem hiding this comment.
혹시, 발행자-구독자가 모두 연결되어 있는 상황에서, 발행자가 갑자기 연결이 끊길 경우에 대한 처리가 있나요?
There was a problem hiding this comment.
없습니다
그런데 이걸 백에서 해야할지도 조금 더 알아봐야할 것 같아요. 최근에 테코톡에서 웹소켓 끊김 대응전략을 보아서 오히려 프론트 단의 처리가 더 좋을 수도 있을 것 같아서요.
어떻게 클라이언트가 비정상종료되었는지를 서버가 감지하고 대응할지는 저도 공부가 더 필요할 것 같습니다.
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo 리뷰 완료했습니다! 아직 전체 기능이 완료된 건 아니기도 하고 파일 체인지 수도 적어서 크게 리뷰할 건 없었습니다
| @Override | ||
| public void registerStompEndpoints(StompEndpointRegistry registry) { | ||
| registry.addEndpoint("/ws") | ||
| .setAllowedOriginPatterns("*") |
There was a problem hiding this comment.
아직 프론트와 협의가 안되서 그런 것 같지만 CorsConfig처럼 이 부분도 처리해주면 좋을 것 같아요
There was a problem hiding this comment.
CorsProeperties로 CofiguranProperties로 객체 안에 CorsOrigin 검증을 캡슐화하고
이걸 WebSockerConfig 와 corsConfig 각각 의존성 주입받아 쓰도록 했습니다.
다만 바인딩 규칙에 따라 필드명에 매핑되는 ConfigurationProperties 특징으로 yml depth가 달라져서
dev, prod 배포 전에 secret 파일 업데이트가 필요합니다.
기존 : cors.origin
변경 후 : cors.origin.cors-origin
| @EventListener | ||
| public void handleSubscribeEvent(SessionSubscribeEvent event) { | ||
| StompHeaderAccessor accessor = StompHeaderAccessor.wrap(event.getMessage()); | ||
| String destination = accessor.getDestination(); | ||
| if (destination == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (destination.startsWith(AUDIENCE_SUBSCRIBE_PREFIX)) { | ||
| long roomId = Long.parseLong(destination.replace(AUDIENCE_SUBSCRIBE_PREFIX, "")); | ||
| messagingTemplate.convertAndSend(CHAIRMAN_CHANNEL_PREFIX + roomId, new ChairmanSharingRequest(roomId)); | ||
| } |
| @@ -0,0 +1,7 @@ | |||
| package com.debatetimer.dto.sharing.request; | |||
|
|
|||
| public record ChairmanSharingRequest( | |||
There was a problem hiding this comment.
Chairman이 뭔지 궁금해서 검색해보니 기업의 회장, 이사회의 의장 이런 개념이던데 맞을까요?
There was a problem hiding this comment.
사회자, 의장 으로 의도했습니다.
There was a problem hiding this comment.
(선택) 체어맨하면 한 눈에 이해하기 어려우니까, Announcer 나 구독자/발행자로 해보는 것도... (근데 애매하네요;;)
개인적으로는 체어맨하면 차 밖에 안떠올라서
| String email = authManager.resolveAccessToken(token); | ||
| return authService.getMember(email); |
There was a problem hiding this comment.
단순 궁금증입니다.
- AccessToken을 확인해보니 유효기간이 1시간으로 되어있던데 만약 중간에 토큰이 만료되면 다시 로그인해서 접속해야 되나요?
- getMember에 DB 조회 로직이 있어서 매 요청마다 등록된 모든 사람에 대한 DB 조회가 발생할 것 같은데 이 부분에 대한 의견이 궁금합니다.
There was a problem hiding this comment.
음... 재발급 과정은 비토가 말한대로 꽤 크리티컬 할 것 같아요. 1초 이상이 지연이 생기면 잘 연동되는 느낌이 아니라서요.
뭔가 프론트랑 규약할 때 웹소켓용으로 토큰을 재발급해주거나, 웹소켓 시에는 인증을 약하게 유지하는 방식도 있을 것 같아요. 확실한 건 기존 어플리케이션 인증용 토큰을 그대로 사용하면 안될 것 같습니다.
WebScoket Security 도 있어서 Spring 에서 제공해주는 웹소켓 보안 관련 스펙 한번 확인해보고 적절한 대안을 생각해볼게요. 이건 추후 따로 이슈파서 해결하겠습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/java/com/debatetimer/config/CorsPropertiesTest.java (1)
12-37: CorsProperties 검증 테스트 케이스 구조 적절합니다
null, 빈 배열,null/empty요소에 대해 각각 다른 에러코드를 기대하도록 잘 분리돼 있어서, 프로퍼티 검증 로직 변경 시 회귀를 막는 데 도움이 될 것 같습니다.
여유가 된다면 정상 값(예:new String[] {"https://example.com"})에 대해 예외가 발생하지 않는 케이스도 하나 정도 추가해 두면, 설정 바인딩 변경 시 안심하고 리팩터링하기 더 좋을 것 같습니다.src/main/java/com/debatetimer/config/CorsProperties.java (1)
1-31: CORS origin 검증 로직은 명확하지만, 불변성·바인딩 방식 한 번만 점검해 보시면 좋겠습니다
validate에서null/빈 배열과null·isBlank()요소를 각각 다른 에러코드로 처리하는 부분은 의도가 잘 드러나고, 테스트와도 잘 맞습니다.- 다만
String[]를 그대로 필드에 보관하고 게터로 그대로 노출하면, 다른 빈에서 배열을 수정해 설정이 런타임에 변할 여지가 있습니다. 필요하다면List<String>로 바꾸고 불변 리스트로 감싸거나, 생성자/게터에서 defensive copy를 만드는 쪽도 고려해 볼 만합니다.- 사용하는 Spring Boot 버전에 따라
@ConfigurationProperties+ 생성자 하나/final필드 조합이 constructor-binding으로 자동 지원되지 않을 수도 있어, 혹시 2.x 일부 버전이라면@ConstructorBinding추가가 필요한지 한 번만 확인 부탁드립니다.src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java (1)
1-38: WebSocket/STOMP 설정이 공유 기능 요구사항에 잘 맞습니다
/room,/chairman를 simple broker 대상으로 두고/app을 application destination prefix로 지정한 구성은 현재 공유 플로우(RoomSubscribeListener,SharingController등)와 잘 정합됩니다./ws엔드포인트에corsProperties.getCorsOrigin()을 그대로 재사용해 HTTP CORS 설정과 WebSocket 핸드셰이크 origin 정책을 일원화한 것도 좋은 선택입니다.- 한 가지 정도만 제안하자면,
"/room"·"/chairman"prefix가 이 클래스와RoomSubscribeListener등 여러 곳에 하드코딩되어 있으니, 공용 상수 클래스로 모으거나 enum 등으로 한 번만 정의해 두면 향후 경로 변경 시 실수를 줄이는 데 도움이 될 것 같습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle(1 hunks)src/main/java/com/debatetimer/config/CorsConfig.java(1 hunks)src/main/java/com/debatetimer/config/CorsProperties.java(1 hunks)src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java(1 hunks)src/main/java/com/debatetimer/event/sharing/RoomSubscribeListener.java(1 hunks)src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java(1 hunks)src/test/java/com/debatetimer/config/CorsPropertiesTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/com/debatetimer/event/sharing/RoomSubscribeListener.java (1)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)
src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java (1)
src/main/java/com/debatetimer/config/CorsConfig.java (1)
Configuration(11-34)
src/main/java/com/debatetimer/config/CorsProperties.java (1)
src/main/java/com/debatetimer/exception/custom/DTInitializationException.java (1)
DTInitializationException(5-10)
src/main/java/com/debatetimer/config/CorsConfig.java (1)
src/main/java/com/debatetimer/config/sharing/WebSocketConfig.java (1)
Configuration(13-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (3)
src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java (1)
56-57:INVALID_ROOM_ID에러 코드 추가가 올바르게 구현되었습니다.검증 결과:
- ✅
RoomSubscribeListener.java라인 43에서 실제로 사용 중- ✅ 웹소켓 room ID 파싱 실패 시 적절하게 throw됨
- ✅ HTTP 상태 코드와 에러 메시지 모두 일관성 있음
src/main/java/com/debatetimer/event/sharing/RoomSubscribeListener.java (1)
1-46: 구독 이벤트 → 의장 채널 브로드캐스트 플로우가 명확하게 구현되어 있습니다
destination == null인 경우 바로 리턴하고,/room/prefix인 경우에만 처리하는 분기 구조가 STOMP 구독 이벤트용 리스너로 깔끔합니다.parseRoomId에서 prefix 길이 이후 부분만 잘라Long.parseLong으로 파싱한 뒤, 실패 시INVALID_ROOM_ID로 감싸는 것도 WebSocket 구독 요청에 대한 도메인 에러 모델링 관점에서 일관성이 있습니다.- 이후
CHAIRMAN_CHANNEL_PREFIX + roomId로ChairmanSharingRequest를 브로드캐스트하는 흐름도 WebSocketConfig의 브로커 설정(/room,/chairman)과 잘 맞아 보입니다.src/main/java/com/debatetimer/config/CorsConfig.java (1)
3-22: CORS 설정을 CorsProperties로 일원화한 방향이 좋습니다
CorsConfig가CorsProperties를 주입받아allowedOriginPatterns(corsProperties.getCorsOrigin())를 사용하는 형태로 바뀌면서, HTTP CORS와 WebSocketConfig 양쪽 모두가 동일한 origin 설정을 공유하게 된 점이 깔끔합니다.
@EnableConfigurationProperties(CorsProperties.class)와@RequiredArgsConstructor조합도 설정 빈 구성 방식과 잘 맞습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/com/debatetimer/controller/GlobalControllerTest.java(1 hunks)src/test/resources/application.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
src/test/java/com/debatetimer/controller/GlobalControllerTest.java (1)
11-11: 검증 결과: 프로퍼티 경로가 정확합니다.
src/test/resources/application.yml에서 확인한 결과,${cors.origin.cors-origin[0]}경로는 실제 구성과 정확히 일치합니다. YAML 구조가cors > origin > cors-origin순서로 중첩되어 있으며,[0]은cors-origin리스트의 첫 번째 요소를 올바르게 참조합니다.개발/프로덕션 환경에서는 단순 문자열 값을 사용하지만, 테스트 환경에서는 리스트 기반 구조를 사용하는 유효한 패턴입니다. 원래 검토 의견에서 제기한 "중복적" 구조에 대한 우려는 근거가 없습니다.
Likely an incorrect or invalid review comment.
|
/noti 1차 리뷰 반영했습니다. |
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti 보안쪽은 따로 이슈판다고 했기때문에 이만 approve합니다.
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
동아리 활동 끝나고 이력서 넣고 이제 여유 생겨서 리뷰 남깁니다.\
- 각종 구현 내용이나 소개하는 것은 작업 내용 공유 페이지에 넣어놓는 것으로 하죠. (웹소켓 문서 나중에 찾기 힘듦)
- 앞으로는 어느 정도 구현되어 있는지 코드만을 보고 명확하게 파악하기 힘들다면, PR에 대략적으로 정리해서 작성 부탁드립니다. (ex. 구현 내용, 앞으로 해야 될 내용, ...)
3 participants
🚩 연관 이슈
closed #220
🗣️ 리뷰 요구사항 (선택)
내용이 깁니다... 노션에 3부로 나누어 정리해놓았으니 읽어주세요!
https://bustling-bathtub-b3a.notion.site/intro-2ae1550c60cf80768d90c4a6c4401b62?source=copy_link
Summary by CodeRabbit
새로운 기능
설정 변경
테스트
오류 처리
✏️ Tip: You can customize this high-level summary in your review settings.