-
Notifications
You must be signed in to change notification settings - Fork 0
[FIX] ApiGatewayHandler send 오류 해결 #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- send시에 header에 platform을 필수로 받지 않음에 따라 헤더 추출시에 Platform을 변환하지 않도록 수정
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/sopt/push/dto/RequestHeaderDto.java (1)
7-8: 리네이밍은 적절하나 nullable 필드에 대한 문서화가 필요합니다.
RegisterHeaderDto에서RequestHeaderDto로의 이름 변경은 이제 등록뿐만 아니라 여러 액션에서 사용되므로 더 적절합니다.그러나
platform필드가 이제 일부 액션(SEND, SEND_ALL)에서는 null일 수 있는데, 이에 대한 문서화나@Nullable어노테이션이 없습니다. 향후 개발자가 이 DTO를 사용할 때 혼란을 줄 수 있습니다.📝 nullable 필드 문서화 제안
+import javax.annotation.Nullable; + +/** + * @param platform REGISTER 및 CANCEL 액션에서는 필수, SEND 및 SEND_ALL 액션에서는 null + */ public record RequestHeaderDto( - String transactionId, Services service, Platform platform, Actions action) {} + String transactionId, Services service, @Nullable Platform platform, Actions action) {}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/sopt/push/dto/ApiGatewayRequestDto.javasrc/main/java/com/sopt/push/dto/RequestHeaderDto.javasrc/main/java/com/sopt/push/lambda/ApiGatewayHandler.java
🔇 Additional comments (4)
src/main/java/com/sopt/push/dto/ApiGatewayRequestDto.java (1)
5-5: 타입 업데이트가 올바르게 적용되었습니다.
RequestHeaderDto로의 타입 변경이 일관성 있게 적용되었습니다.src/main/java/com/sopt/push/lambda/ApiGatewayHandler.java (3)
27-27: Import 업데이트가 올바르게 적용되었습니다.
RequestHeaderDto로의 import 변경이 일관성 있게 적용되었습니다.
117-121: 조건부 platform 파싱 로직이 PR 목표를 올바르게 구현했습니다.REGISTER와 CANCEL 액션에서만 platform을 파싱하도록 하여, SEND 및 SEND_ALL 액션에서 platform이 필수가 아니도록 수정한 것은 PR 목표와 일치합니다.
Platform.fromValue 메서드는 null 또는 blank 값에 대해 "Platform value cannot be null or blank" 메시지와 함께 BusinessException을 명시적으로 발생시킵니다. 따라서 REGISTER 또는 CANCEL 액션에서 platformStr 헤더가 누락되거나 null인 경우 적절한 예외가 발생하며, 별도의 null 체크는 필요하지 않습니다.
123-124: 헤더 생성 로직이 올바르게 구현되었습니다.조건부로 설정된
platform값(REGISTER/CANCEL의 경우 non-null, SEND/SEND_ALL의 경우 null)을 사용하여RequestHeaderDto를 생성하는 것은 올바릅니다.
request.header().platform()에 접근하는handleRegister(line 137)와handleCancel(line 166)은 main handler의 switch 문 (lines 75-79)을 통해 action이 REGISTER 또는 CANCEL일 때만 호출되므로, 해당 메서드들이 실행될 때 platform은 항상 null이 아닙니다. extractRequest의 조건문 (lines 119-121)과 main handler의 라우팅 로직이 자연스럽게 일치하므로 NPE 위험은 없습니다.
| new RegisterHeaderDto(transactionId, Services.fromValue(serviceStr), platform, action); | ||
| Platform platform = null; | ||
|
|
||
| if (action == Actions.REGISTER || action == Actions.CANCEL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저번에 조건 2개 이상이면 따로 변수로 빼기로 했던 거 맞나영 ?? 컨벤션으로 정했는지 궁금합니당
seoyeonjin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니당 !!
Related Issue 🚀
Work Description ✏️
ApiGatewayHandler에서 REGISTER와 CANCEL 액션에서만 platform을 필수로 검증하고, SEND와 SEND_ALL 액션에서는 platform을 선택적으로 처리하도록 수정하였습니다!!PR Point 📸