Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 공유 기능의 데이터 전송 객체(DTO) 구조를 개선하는 첫 번째 단계입니다. 기존의 간단한 시간 정보 대신, 타이머 이벤트 유형과 상세 정보를 포함하는 새로운 DTO를 도입하여 공유되는 데이터의 명확성과 유연성을 높였습니다. 이는 향후 공유 기능 확장을 위한 기반을 마련하며, 요청 데이터에 대한 유효성 검증을 강화했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
WalkthroughWebSocket 공유 메커니즘이 시간 기반 이벤트 시스템으로 리팩토링되었습니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SharingController
participant SharingService
participant TimerEventType
participant TimerEvent
participant SharingResponse
Client->>SharingController: share(roomId, SharingRequest)
activate SharingController
SharingController->>SharingRequest: toTimerEvent()
activate SharingRequest
SharingRequest->>TimerEventType: validateEventData(data)
activate TimerEventType
TimerEventType-->>SharingRequest: validation result
deactivate TimerEventType
SharingRequest->>TimerEvent: new TimerEvent(eventType, data)
activate TimerEvent
TimerEvent->>TimerEventType: validateEventData(timerEventData)
activate TimerEventType
TimerEventType-->>TimerEvent: validation result
deactivate TimerEventType
TimerEvent-->>SharingRequest: TimerEvent instance
deactivate TimerEvent
SharingRequest-->>SharingController: TimerEvent
deactivate SharingRequest
SharingController->>SharingService: share(request)
activate SharingService
SharingService->>SharingService: extract eventType & timerEventData
alt timerEventData present
SharingService->>SharingResponse: new SharingResponse(eventType, TimerEventDataResponse)
else timerEventData absent
SharingService->>SharingResponse: new SharingResponse(eventType, null)
end
activate SharingResponse
SharingResponse-->>SharingService: SharingResponse instance
deactivate SharingResponse
SharingService-->>SharingController: SharingResponse
deactivate SharingService
SharingController-->>Client: SharingResponse
deactivate SharingController
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 실시간 공유 기능을 위해 DTO를 리팩토링하고, 다양한 타이머 이벤트를 처리하기 위해 TimerEventType과 TimerEventInfo를 도입했습니다. 변경 사항은 구조적으로 잘 구성되어 있으며, 유효성 검사와 관련 테스트를 추가한 점이 좋습니다. NullPointerException으로 이어질 수 있는 심각한 문제와 코드 명확성 및 일관성을 개선하기 위한 몇 가지 제안 사항을 발견했습니다. 자세한 내용은 아래 댓글을 참고해주세요.
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | ||
| this( | ||
| eventType, | ||
| new TimerEventInfoResponse(timerEventInfo) | ||
| ); | ||
| } |
There was a problem hiding this comment.
timerEventInfo가 null일 경우, new TimerEventInfoResponse(timerEventInfo) 호출 시 TimerEventInfoResponse 생성자 내부에서 NullPointerException이 발생합니다. 이는 서버 오류를 유발하고 응답 전송을 막게 됩니다. 이 문제를 피하기 위해 null 케이스를 처리해야 합니다.
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | |
| this( | |
| eventType, | |
| new TimerEventInfoResponse(timerEventInfo) | |
| ); | |
| } | |
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | |
| this( | |
| eventType, | |
| timerEventInfo == null ? null : new TimerEventInfoResponse(timerEventInfo) | |
| ); | |
| } |
References
- Prevent
NullPointerExceptionby handling potential null values, similar to usinggetOrDefaultfor map lookups.
There was a problem hiding this comment.
파라미터도 null이 될 수 있는지 확인해주시고, null일 수 있다면 @Nullable 붙여주세요
There was a problem hiding this comment.
아예 서비스 로직에서 Null일 경우에는 Null 넣도록 명시적으로 표기했습니다.
필드값에도 Nullable 표기 완료했습니다.
public SharingResponse share(SharingRequest request) {
return request.toTimerEventInfo()
.map(eventInfo -> new SharingResponse(
request.eventType(),
new TimerEventInfoResponse(eventInfo)
))
.orElse(new SharingResponse(request.eventType(), null));
}public record SharingResponse(
TimerEventType eventType,
@Nullable
TimerEventInfoResponse data
) {
}There was a problem hiding this comment.
[선택 반영]
public SharingResponse share(SharingRequest request) {
return request.toTimerEventInfo()
.map(eventInfo -> new SharingResponse(
request.eventType(),
new TimerEventInfoResponse(eventInfo)
))
.orElse(new SharingResponse(request.eventType()));
}public record SharingResponse(
TimerEventType eventType,
@Nullable
TimerEventInfoResponse data
) {
public SharingResponse(TimerEventType eventType) {
this(eventType, null);
}
}- 저는 위와 같이 생성자 만드는 것을 더 좋아하긴 합니다.
- 하지만 그렇게 된다면, 특정 eventType에서 사용하는 생성자가 달라야 될 것 같아요. 그래서 오히려 null을 명시하는게 괜찮을 수도 있겠네요.
- Q. 현재 어떤 eventType에서 data가 null 인지 아닌지 검증하는 로직이 있나요?
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
도메인 모델(TimerEventInfo.remainingTime)과의 명확성과 일관성을 위해 이 필드의 이름을 time 대신 remainingTime으로 하는 것이 좋습니다. toTimerEventInfo() 메소드에서도 이 필드 사용 부분을 함께 수정해주세요.
| long time | |
| long remainingTime |
There was a problem hiding this comment.
(제안) 이것도 반영해 보는 것이 좋지 않을까요? 꽤 합리적이라고 생각합니다.
There was a problem hiding this comment.
✅ [반영 완료]
dto가 숀과의 협의가 필요한 부분이긴 한데 저도 합리적이라 생각하여 반영하였습니다.
src/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.java
Outdated
Show resolved
Hide resolved
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
도메인 모델(TimerEventInfo.remainingTime)과의 명확성과 일관성을 위해 이 필드의 이름을 time 대신 remainingTime으로 하는 것이 좋습니다.
| long time | |
| long remainingTime |
Test Results137 files 137 suites 16s ⏱️ Results for commit c197ad5. ♻️ 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 (1)
src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java (1)
60-83:FINISHED이벤트 테스트 케이스 추가 권장
data가 null인FINISHED이벤트에 대한 테스트가 없습니다. API 명세에 따르면FINISHED이벤트 시data는 null이어야 하는데, 이 시나리오를 테스트하면 앞서 언급한 NPE 문제를 발견할 수 있습니다.🧪 FINISHED 이벤트 테스트 추가 제안
`@Test` void FINISHED_이벤트는_data가_null이다() throws ExecutionException, InterruptedException, TimeoutException { long roomId = 1L; MessageFrameHandler<SharingResponse> handler = new MessageFrameHandler<>(SharingResponse.class); Member member = memberGenerator.generate("example@email.com"); StompHeaders headers = headerGenerator.generateAccessTokenHeader("/app/event/" + roomId, member); SharingRequest request = new SharingRequest(TimerEventType.FINISHED, null); stompSession.subscribe("/room/" + roomId, handler); stompSession.send(headers, request); SharingResponse response = handler.getCompletableFuture() .get(3L, TimeUnit.SECONDS); assertAll( () -> assertThat(response.eventType()).isEqualTo(TimerEventType.FINISHED), () -> assertThat(response.data()).isNull() ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java` around lines 60 - 83, Add a test in SharingControllerTest that sends a SharingRequest with TimerEventType.FINISHED and null data and verifies the response from MessageFrameHandler<SharingResponse> (use stompSession.subscribe and stompSession.send as in existing tests); specifically create a SharingRequest(TimerEventType.FINISHED, null), await handler.getCompletableFuture().get(...), and assert that response.eventType() == TimerEventType.FINISHED and response.data() == null to cover the null-data FINISHED path and catch the NPE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/debatetimer/dto/sharing/request/SharingRequest.java`:
- Around line 12-17: SharingRequest.toTimerEventInfo() can return null causing a
NullPointerException when SharingResponse constructs new
TimerEventInfoResponse(timerEventInfo); modify the SharingResponse constructor
to null-check the incoming TimerEventInfo (from
SharingRequest.toTimerEventInfo()) before instantiating TimerEventInfoResponse
(e.g., if timerEventInfo == null set the response field to null or a safe
empty/placeholder), or alternatively update TimerEventInfoResponse to accept a
nullable input and guard all internal calls (getTimerType(), getSequence(),
etc.) against null; locate SharingResponse constructor and
TimerEventInfoResponse class to implement the chosen null-safety approach.
In `@src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java`:
- Around line 11-16: The SharingResponse(TimerEventType eventType,
TimerEventInfo timerEventInfo) constructor fails with NPE because it always
calls new TimerEventInfoResponse(timerEventInfo) even when timerEventInfo is
null; update this constructor to guard against null (e.g., pass null into the
delegated constructor or create the TimerEventInfoResponse only when
timerEventInfo != null) so that SharingResponse(eventType, timerEventInfo)
becomes SharingResponse(eventType, timerEventInfo == null ? null : new
TimerEventInfoResponse(timerEventInfo)), keeping the existing overloaded
constructor behavior and avoiding NPE in TimerEventInfoResponse.
---
Nitpick comments:
In `@src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java`:
- Around line 60-83: Add a test in SharingControllerTest that sends a
SharingRequest with TimerEventType.FINISHED and null data and verifies the
response from MessageFrameHandler<SharingResponse> (use stompSession.subscribe
and stompSession.send as in existing tests); specifically create a
SharingRequest(TimerEventType.FINISHED, null), await
handler.getCompletableFuture().get(...), and assert that response.eventType() ==
TimerEventType.FINISHED and response.data() == null to cover the null-data
FINISHED path and catch the NPE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ba6238e-ba57-41d9-86c3-789f9cc9fc13
📒 Files selected for processing (9)
src/main/java/com/debatetimer/controller/sharing/SharingController.javasrc/main/java/com/debatetimer/domain/sharing/TimerEventInfo.javasrc/main/java/com/debatetimer/domain/sharing/TimerEventType.javasrc/main/java/com/debatetimer/dto/sharing/request/SharingRequest.javasrc/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.javasrc/main/java/com/debatetimer/dto/sharing/response/SharingResponse.javasrc/main/java/com/debatetimer/dto/sharing/response/TimerEventInfoResponse.javasrc/test/java/com/debatetimer/BaseStompTest.javasrc/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java
src/main/java/com/debatetimer/dto/sharing/request/SharingRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java
Outdated
Show resolved
Hide resolved
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
Null 좀 신경써주세요. 소마 면접 끝나고 천천히 이야기해보시죠.
| @Controller | ||
| public class SharingController { |
There was a problem hiding this comment.
(의문) 공유라는 것을 도메인으로 봐야할까라는 근본적인 의문이 들기는 합니다. 근데 아마 이것이 최선이었겠죠?
There was a problem hiding this comment.
[개인 의견]
조금 넓은 범위로 규정되었다고 생각하기는 하는데, 다음과 같은 이유로 분리했던 것 같아요.
- 세부 기획 이전에
화면 공유피쳐가 잡혀있었음 - 기존에 RestController로 선언된 도메인별 Controller는 Http Rest API를 기준으로 선언된 Controller이기에 웹소켓 프로토콜에서는 사용되지 못함
마지막으로 화면 공유가 도메인으로 바라볼 수 있다는 사견입니다. 커찬 말처럼 CustomizeTable 도메인 중심으로 가되 CustomizeRestController(http용), CustomizeWebSocketController(websocket용)으로 분리하는 방안도 차안으로 있었습니다.
다만 특정 자원을 대상으로 GET, POST, PUT, DELETE등의 요청을 주고받는 Controller와 달리 CustomizeWebScoketController의 경우는 성격이 다른 부분이 많았어요.
대표적 예로 요청하는 사람과 응답을 받는 사람이 다를 수 있었습니다.(사회자의 타이머 이벤트 발생 상황 등등)
또한, TimerEventInfo처럼 타이머 공유로부터 도메인 객체들의 이름이 정해져 있어서 도메인으로 바라보아도 충분하다는 생각을 했습니다.
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
(제안) 이것도 반영해 보는 것이 좋지 않을까요? 꽤 합리적이라고 생각합니다.
| @Getter | ||
| @RequiredArgsConstructor | ||
| public class TimerEventInfo { | ||
|
|
||
| private final CustomizeBoxType timerType; | ||
| private final int sequence; | ||
| private final Stance currentTeam; | ||
| private final long remainingTime; | ||
| } |
There was a problem hiding this comment.
(질문) 여기 전부 다 NotNull 맞아요? 특정 부분은 Nullable 이라면 @Nullable 표시해주는 것이 좋지 않나요?
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | ||
| this( | ||
| eventType, | ||
| new TimerEventInfoResponse(timerEventInfo) | ||
| ); | ||
| } |
There was a problem hiding this comment.
파라미터도 null이 될 수 있는지 확인해주시고, null일 수 있다면 @Nullable 붙여주세요
| public record SharingRequest( | ||
| LocalDateTime time | ||
| TimerEventType eventType, | ||
| @Valid TimerEventInfoRequest data | ||
| ) { | ||
|
|
||
| public TimerEventInfo toTimerEventInfo() { | ||
| if (data == null) { | ||
| return null; | ||
| } | ||
| return data.toTimerEventInfo(); | ||
| } | ||
| } |
There was a problem hiding this comment.
- Nullable을 다룰 때는 Optional 쓰는 게 더 좋지 않을까요?
| public record SharingRequest( | |
| LocalDateTime time | |
| TimerEventType eventType, | |
| @Valid TimerEventInfoRequest data | |
| ) { | |
| public TimerEventInfo toTimerEventInfo() { | |
| if (data == null) { | |
| return null; | |
| } | |
| return data.toTimerEventInfo(); | |
| } | |
| } | |
| public record SharingRequest( | |
| TimerEventType eventType, | |
| @Valid @Nullable TimerEventInfoRequest data | |
| ) { | |
| public Optional<TimerEventInfo> toTimerEventInfo() { | |
| return Optional.ofNullable(data) | |
| .map(TimerEventInfoRequest::toTimerEventInfo); | |
| } | |
| } |
There was a problem hiding this comment.
- 이 외 코드에서는 조금 불편할 것 같은데, 어디까지 Null을 쓰고 어디까지 Null을 쓰지 않을지 잘 정해야 될 듯
- null을 쓸거면
@Nullable을 어노테이션을 붙이고, null을 허용 안할꺼면 코드를 조금 더 복잡하게 하면서 Optional을 써야할 듯
There was a problem hiding this comment.
✅ [반영 완료]
Optional 반환하도록 수정했습니다
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo approve 했으요. 개인적으로 너무 pr을 잘개 쪼갠것 아닌가 하는 생각이 드네요
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
| @NotNull CustomizeBoxType timerType, | |
| int sequence, | |
| @Nullable Stance currentTeam, | |
| long time | |
| @NotNull | |
| CustomizeBoxType timerType, | |
| @Nullable | |
| Stance currentTeam, | |
| int sequence, | |
| long time |
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
| @NotNull CustomizeBoxType timerType, | |
| int sequence, | |
| @Nullable Stance currentTeam, | |
| long time | |
| @NotNull | |
| CustomizeBoxType timerType, | |
| @Nullable | |
| Stance currentTeam, | |
| int sequence, | |
| long time |
우리 이거 관련해서 컨벤션 있지 않았나? 다른 DTO에선 항상 이렇게 짜왔던 걸로 기억하는데
There was a problem hiding this comment.
✅ 반영 완료
DTO 애너테이션에 따른 순서에도 컨벤션이 있었나?
찾아봤는데 별다른 협의는 없었던 것 같은데 일관성 좋은게 저도 좋으니 반영했습다
coli-geonwoo
left a comment
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
해당 PR 사이즈가 작아보일 수 있겠지만 이런 이유 때문에 양해 부탁합니다..
- 일단 내 정신상태 문제(디베이트 코리아 컨택, 신규 기획, 인프라, 글로벌화 번역 피드백 받기, 숀과의 공유 기능 개발)를 동시에 생각하고 있어서 컨텍스트 스위칭이 잦은 상황이고 피폐해지고 있음
- 둘째로 취업 공고 시즌이라 짧게 하나하나씩 최대한 잘게 나누려 했음다..
한번 리뷰 반영했으니 재리뷰 부탁!
| @Controller | ||
| public class SharingController { |
There was a problem hiding this comment.
[개인 의견]
조금 넓은 범위로 규정되었다고 생각하기는 하는데, 다음과 같은 이유로 분리했던 것 같아요.
- 세부 기획 이전에
화면 공유피쳐가 잡혀있었음 - 기존에 RestController로 선언된 도메인별 Controller는 Http Rest API를 기준으로 선언된 Controller이기에 웹소켓 프로토콜에서는 사용되지 못함
마지막으로 화면 공유가 도메인으로 바라볼 수 있다는 사견입니다. 커찬 말처럼 CustomizeTable 도메인 중심으로 가되 CustomizeRestController(http용), CustomizeWebSocketController(websocket용)으로 분리하는 방안도 차안으로 있었습니다.
다만 특정 자원을 대상으로 GET, POST, PUT, DELETE등의 요청을 주고받는 Controller와 달리 CustomizeWebScoketController의 경우는 성격이 다른 부분이 많았어요.
대표적 예로 요청하는 사람과 응답을 받는 사람이 다를 수 있었습니다.(사회자의 타이머 이벤트 발생 상황 등등)
또한, TimerEventInfo처럼 타이머 공유로부터 도메인 객체들의 이름이 정해져 있어서 도메인으로 바라보아도 충분하다는 생각을 했습니다.
| @Getter | ||
| @RequiredArgsConstructor | ||
| public class TimerEventInfo { | ||
|
|
||
| private final CustomizeBoxType timerType; | ||
| private final int sequence; | ||
| private final Stance currentTeam; | ||
| private final long remainingTime; | ||
| } |
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
✅ [반영 완료]
dto가 숀과의 협의가 필요한 부분이긴 한데 저도 합리적이라 생각하여 반영하였습니다.
| public record SharingRequest( | ||
| LocalDateTime time | ||
| TimerEventType eventType, | ||
| @Valid TimerEventInfoRequest data | ||
| ) { | ||
|
|
||
| public TimerEventInfo toTimerEventInfo() { | ||
| if (data == null) { | ||
| return null; | ||
| } | ||
| return data.toTimerEventInfo(); | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ [반영 완료]
Optional 반환하도록 수정했습니다
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | ||
| this( | ||
| eventType, | ||
| new TimerEventInfoResponse(timerEventInfo) | ||
| ); | ||
| } |
There was a problem hiding this comment.
아예 서비스 로직에서 Null일 경우에는 Null 넣도록 명시적으로 표기했습니다.
필드값에도 Nullable 표기 완료했습니다.
public SharingResponse share(SharingRequest request) {
return request.toTimerEventInfo()
.map(eventInfo -> new SharingResponse(
request.eventType(),
new TimerEventInfoResponse(eventInfo)
))
.orElse(new SharingResponse(request.eventType(), null));
}public record SharingResponse(
TimerEventType eventType,
@Nullable
TimerEventInfoResponse data
) {
}| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
| @NotNull CustomizeBoxType timerType, | ||
| int sequence, | ||
| @Nullable Stance currentTeam, | ||
| long time |
There was a problem hiding this comment.
✅ 반영 완료
DTO 애너테이션에 따른 순서에도 컨벤션이 있었나?
찾아봤는데 별다른 협의는 없었던 것 같은데 일관성 좋은게 저도 좋으니 반영했습다
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/controller/sharing/SharingController.java (1)
23-27:roomId파라미터가 사용되지 않습니다.
roomId가@DestinationVariable로 추출되지만sharingService.share(request)에 전달되지 않아 실제로 사용되지 않습니다. 라우팅/구독 목적으로만 필요한 것이라면 괜찮지만, 추후 방 소유권 검증이나 권한 체크에 필요할 수 있으니 의도를 확인해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/debatetimer/controller/sharing/SharingController.java` around lines 23 - 27, The method parameter roomId (annotated with `@DestinationVariable`) is never used—either remove the unused parameter and its `@DestinationVariable` annotation from SharingController.share if it's only for routing, or propagate it into the service call by changing sharingService.share(request) to sharingService.share(roomId, request) and update the SharingService/SharingServiceImpl signature and any downstream logic (e.g., ownership/permission checks) to validate the room context using roomId; reference: SharingController, roomId, sharingService.share, SharingRequest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/debatetimer/controller/sharing/SharingController.java`:
- Around line 22-27: The WebSocket endpoint SharingController.share lacks
parameter-level authentication and ignores the captured roomId; update the
method to accept an `@AuthMember-annotated` principal parameter so
WebSocketAuthMemberResolver performs auth (e.g., add a parameter like
`@AuthMember` AuthMember authMember) and then pass the roomId (and auth if needed)
into the service call instead of calling sharingService.share(request) — update
SharingService.share signature to accept the roomId and/or auth principal (or
alternatively remove the unused `@DestinationVariable` roomId if the room context
truly isn't required).
---
Nitpick comments:
In `@src/main/java/com/debatetimer/controller/sharing/SharingController.java`:
- Around line 23-27: The method parameter roomId (annotated with
`@DestinationVariable`) is never used—either remove the unused parameter and its
`@DestinationVariable` annotation from SharingController.share if it's only for
routing, or propagate it into the service call by changing
sharingService.share(request) to sharingService.share(roomId, request) and
update the SharingService/SharingServiceImpl signature and any downstream logic
(e.g., ownership/permission checks) to validate the room context using roomId;
reference: SharingController, roomId, sharingService.share, SharingRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: caa757f2-5180-4af4-89f9-7a504714cd3a
📒 Files selected for processing (9)
src/main/java/com/debatetimer/controller/sharing/SharingController.javasrc/main/java/com/debatetimer/domain/sharing/TimerEventInfo.javasrc/main/java/com/debatetimer/dto/sharing/request/SharingRequest.javasrc/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.javasrc/main/java/com/debatetimer/dto/sharing/response/SharingResponse.javasrc/main/java/com/debatetimer/dto/sharing/response/TimerEventInfoResponse.javasrc/main/java/com/debatetimer/service/sharing/SharingService.javasrc/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.javasrc/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java
✅ Files skipped from review due to trivial changes (2)
- src/test/java/com/debatetimer/controller/customize/CustomizeDocumentTest.java
- src/main/java/com/debatetimer/domain/sharing/TimerEventInfo.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java
- src/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.java
- src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java
- src/main/java/com/debatetimer/dto/sharing/response/TimerEventInfoResponse.java
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
늦은 리뷰 죄송합니다. (어제 예비군 이슈)
코멘트 한 개만 남겼어요~ 나머지는 크게 문제 없습니다. Approve 남깁니다~
| public SharingResponse(TimerEventType eventType, TimerEventInfo timerEventInfo) { | ||
| this( | ||
| eventType, | ||
| new TimerEventInfoResponse(timerEventInfo) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[선택 반영]
public SharingResponse share(SharingRequest request) {
return request.toTimerEventInfo()
.map(eventInfo -> new SharingResponse(
request.eventType(),
new TimerEventInfoResponse(eventInfo)
))
.orElse(new SharingResponse(request.eventType()));
}public record SharingResponse(
TimerEventType eventType,
@Nullable
TimerEventInfoResponse data
) {
public SharingResponse(TimerEventType eventType) {
this(eventType, null);
}
}- 저는 위와 같이 생성자 만드는 것을 더 좋아하긴 합니다.
- 하지만 그렇게 된다면, 특정 eventType에서 사용하는 생성자가 달라야 될 것 같아요. 그래서 오히려 null을 명시하는게 괜찮을 수도 있겠네요.
- Q. 현재 어떤 eventType에서 data가 null 인지 아닌지 검증하는 로직이 있나요?
|
/noti event 별 데이터 여부를 검증하기 위해서 + eventType과 data를 하나의 도메인 객체로 바라볼 필요가 있어서 TimerEvent라는 도메인 객체를 만드는 과정에서 코드변경이 조금 있었어요. 해당 Commit 범위에서만 간단히 검토 부탁드립니다 🙇 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/debatetimer/domain/sharing/TimerEventTypeTest.java (1)
14-49: 유효한 케이스에 대한 테스트 추가를 권장합니다.현재 테스트는 예외가 발생하는 잘못된 케이스만 검증하고 있습니다. 정상 동작을 보장하기 위해 예외가 발생하지 않는 유효한 케이스도 테스트하면 좋겠습니다.
♻️ 추가 테스트 케이스 제안
`@EnumSource`( value = TimerEventType.class, names = {"FINISHED"}, mode = EnumSource.Mode.EXCLUDE ) `@ParameterizedTest` void 타이머_이벤트_데이터가_존재하면_검증에_성공한다(TimerEventType eventType) { TimerEventData timerEventData = new TimerEventData( CustomizeBoxType.NORMAL, 2, null, 30L ); assertThatCode(() -> eventType.validateEventData(timerEventData)) .doesNotThrowAnyException(); } `@EnumSource`(value = TimerEventType.class, names = {"FINISHED"}) `@ParameterizedTest` void 타이머_이벤트_데이터가_null이면_검증에_성공한다(TimerEventType eventType) { assertThatCode(() -> eventType.validateEventData(null)) .doesNotThrowAnyException(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/debatetimer/domain/sharing/TimerEventTypeTest.java` around lines 14 - 49, Add positive-path parameterized tests to TimerEventTypeTest to assert no exception is thrown for valid inputs: add one `@ParameterizedTest` using EnumSource(mode = EnumSource.Mode.EXCLUDE, names = {"FINISHED"}) that calls TimerEventType.validateEventData(timerEventData) with a non-null TimerEventData and asserts doesNotThrowAnyException, and add another `@ParameterizedTest` for names = {"FINISHED"} that calls validateEventData(null) and asserts doesNotThrowAnyException; reference the existing test class TimerEventTypeTest and the method TimerEventType.validateEventData to place these new tests alongside the current invalid-case tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/java/com/debatetimer/domain/sharing/TimerEventTypeTest.java`:
- Around line 14-49: Add positive-path parameterized tests to TimerEventTypeTest
to assert no exception is thrown for valid inputs: add one `@ParameterizedTest`
using EnumSource(mode = EnumSource.Mode.EXCLUDE, names = {"FINISHED"}) that
calls TimerEventType.validateEventData(timerEventData) with a non-null
TimerEventData and asserts doesNotThrowAnyException, and add another
`@ParameterizedTest` for names = {"FINISHED"} that calls validateEventData(null)
and asserts doesNotThrowAnyException; reference the existing test class
TimerEventTypeTest and the method TimerEventType.validateEventData to place
these new tests alongside the current invalid-case tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b65be84-0017-4400-8dcd-e6ae31ec9ec0
📒 Files selected for processing (12)
src/main/java/com/debatetimer/domain/sharing/TimerEvent.javasrc/main/java/com/debatetimer/domain/sharing/TimerEventData.javasrc/main/java/com/debatetimer/domain/sharing/TimerEventType.javasrc/main/java/com/debatetimer/dto/sharing/request/SharingRequest.javasrc/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.javasrc/main/java/com/debatetimer/dto/sharing/response/SharingResponse.javasrc/main/java/com/debatetimer/dto/sharing/response/TimerEventDataResponse.javasrc/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.javasrc/main/java/com/debatetimer/service/sharing/SharingService.javasrc/test/java/com/debatetimer/controller/sharing/SharingControllerTest.javasrc/test/java/com/debatetimer/domain/sharing/TimerEventDataTest.javasrc/test/java/com/debatetimer/domain/sharing/TimerEventTypeTest.java
✅ Files skipped from review due to trivial changes (3)
- src/test/java/com/debatetimer/domain/sharing/TimerEventDataTest.java
- src/main/java/com/debatetimer/service/sharing/SharingService.java
- src/main/java/com/debatetimer/dto/sharing/request/TimerEventInfoRequest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/debatetimer/dto/sharing/request/SharingRequest.java
- src/main/java/com/debatetimer/domain/sharing/TimerEventType.java
- src/main/java/com/debatetimer/exception/errorcode/ClientErrorCode.java
- src/main/java/com/debatetimer/dto/sharing/response/SharingResponse.java
- src/test/java/com/debatetimer/controller/sharing/SharingControllerTest.java
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @coli-geonwoo
확인했습니다. 이걸 또 반영해주네요. ㄷㄷ
3 participants
🚩 연관 이슈
closed #242
🗣️ 리뷰 요구사항 (선택)
공유 기능 PR은 짧게 짧게 끊어서 다음 3개로 나누어 올릴 예정
현재 PR은 응답형식 변경으로 API 문서 참고하면 어떤 정보를 주고 받는지 확인 가능
의견이 필요한 부분
기존 컨벤션에 따라 Valid 검증이 잘되는지 확인하기 위해 null 테스트를 진행함
그러나, 현재 검증문은 응답이 오지 않음을 검증하여 엄밀한 검증은 아님.
이렇게 검증할 수 밖에 없던 이유는 웹소켓 STOMP가 요청의 전달만을 기대하지 Http 처럼 응답을 기대하지 않기 떄문임.
즉, 클라이언트 --> 웹소켓 요청 --> 서버 과정이 수행되면 서버는 해당 요청을 받고 응답을 반환하지 않을수도 있으며 다른 구독자들을 대상으로 응답을 반환할수도 있음 웹소켓 통신 자체가 채널을 기반으로 한 단방향성 통신이다보니 요청-응답 모델이 아니기 때문에 Http E2E테스트처럼 '특정 에러를 반환한다' 를 검증하지 못하고 있는 상황임.
해당 처리를 위해 두번째 PR에서 숀과 에러 처리를 협의하여 반영할 예정임. 이전에 좋은 아이디어 있으면 PR에 남겨주십쇼..
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
테스트