[feat] 배달팟 참여 API 구현#94
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds authenticated delivery-party joining and creator-authorized cancellation. Security rules distinguish public reads and creation from protected join/delete operations, while service logic validates status, duplication, capacity, and participant persistence. ChangesDelivery party lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/test/java/com/leets/tdd/party/service/DeliveryPartyServiceTest.java (1)
185-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the try/catch/printStackTrace/rethrow wrapper.
An uncaught exception already fails the test with a full stack trace; this wrapper adds no value.
♻️ Suggested simplification
- // when - Long result = null; - - try { - result = deliveryPartyService.deleteDeliveryParty(1L, 1L); - } catch (Exception e) { - e.printStackTrace(); - throw e; - } + // when + Long result = deliveryPartyService.deleteDeliveryParty(1L, 1L);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/leets/tdd/party/service/DeliveryPartyServiceTest.java` around lines 185 - 194, Remove the redundant try/catch wrapper around the deleteDeliveryParty invocation in the test method, allowing exceptions to propagate naturally and preserving the assignment to result.src/main/java/com/leets/tdd/party/service/DeliveryPartyService.java (1)
206-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the debug
System.out.println.Leftover debug print will pollute production logs on every cancellation; use the project's logger (or drop it) instead.
♻️ Suggested fix
deliveryParty.cancel(); - System.out.println("서비스 내부 상태 = " + deliveryParty.getStatus()); - deliveryPartyRepository.save(deliveryParty);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/leets/tdd/party/service/DeliveryPartyService.java` around lines 206 - 210, Remove the debug System.out.println between deliveryParty.cancel() and deliveryPartyRepository.save(deliveryParty) in the cancellation flow, leaving the cancellation and persistence behavior unchanged.src/main/java/com/leets/tdd/party/controller/DeliveryPartyController.java (1)
75-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResponse shape diverges from the rest of the controller.
joinDeliveryPartywraps its result inApiResponse<JoinDeliveryPartyResponse>, whilecreateDeliveryParty,getDeliveryParties,getDeliveryPartyDetail,updateDeliveryParty, anddeleteDeliveryPartyall return raw bodies. Consider standardizing on one response envelope across this controller for consistency once Swagger docs are updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/leets/tdd/party/controller/DeliveryPartyController.java` around lines 75 - 87, The DeliveryPartyController response shape is inconsistent because joinDeliveryParty wraps its result in ApiResponse while the other endpoints return raw bodies. Standardize joinDeliveryParty with the controller’s established raw-body response style, updating its return type and return statement while preserving the service result and Swagger documentation consistency.src/main/java/com/leets/tdd/global/config/JwtAuthenticationEntryPoint.java (1)
30-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winJoin endpoint collapses all auth-failure reasons into one generic message.
Any
AuthenticationExceptionon the join path (expired, banned, invalid, missing token) now short-circuits to the same "로그인이 필요합니다." response, instead of the per-reasonUserErrorCodemapping used by every other endpoint. A banned or expired-token user attempting to join gets a misleading "please log in" message instead of the actual reason.Additionally, the hardcoded regex duplicates route knowledge already declared in
DeliveryPartyController's@RequestMapping/@PostMapping("/{partyId}/join"); if that mapping changes, this check silently stops matching.♻️ Suggested approach: only override the missing-token case
- if (request.getRequestURI().matches("/api/v1/(parties|delivery-parties)/[^/]+/join")) { - response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); - response.setContentType(MediaType.APPLICATION_JSON_VALUE); - response.setCharacterEncoding("UTF-8"); - response.getWriter().write( - objectMapper.writeValueAsString(ApiResponse.fail("로그인이 필요합니다.")) - ); - return; - } - Object errorType = request.getAttribute(JwtAuthenticationFilter.JWT_ERROR_ATTRIBUTE); UserErrorCode errorCode; if (JwtAuthErrorType.MISSING.equals(errorType)) { - errorCode = UserErrorCode.TOKEN_MISSING; + errorCode = request.getRequestURI().matches("/api/v1/(parties|delivery-parties)/[^/]+/join") + ? UserErrorCode.LOGIN_REQUIRED /* or reuse TOKEN_MISSING with the desired message */ + : UserErrorCode.TOKEN_MISSING;Alternatively, configure this per-route via
HttpSecurity#exceptionHandling().defaultAuthenticationEntryPointFor(entryPoint, requestMatcher)inSecurityConfigso route knowledge lives in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/leets/tdd/global/config/JwtAuthenticationEntryPoint.java` around lines 30 - 38, Update JwtAuthenticationEntryPoint so the join-path override applies only to missing-token authentication failures; let expired, banned, invalid, and other AuthenticationException cases continue through the existing UserErrorCode-based response mapping. Remove the duplicated hardcoded URI regex and use the established route matcher or security configuration mechanism, preferably HttpSecurity#exceptionHandling().defaultAuthenticationEntryPointFor, to associate this behavior with DeliveryPartyController’s join endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/leets/tdd/party/controller/DeliveryPartyController.java`:
- Around line 106-122: Update deleteDeliveryParty to accept
`@AuthenticationPrincipal` UserPrincipal currentUser instead of Long
currentUserId, and pass currentUser.userId() to
deliveryPartyService.deleteDeliveryParty. Apply the same principal binding and
userId extraction to any nearby update/delete controller endpoints using
`@AuthenticationPrincipal` Long.
---
Nitpick comments:
In `@src/main/java/com/leets/tdd/global/config/JwtAuthenticationEntryPoint.java`:
- Around line 30-38: Update JwtAuthenticationEntryPoint so the join-path
override applies only to missing-token authentication failures; let expired,
banned, invalid, and other AuthenticationException cases continue through the
existing UserErrorCode-based response mapping. Remove the duplicated hardcoded
URI regex and use the established route matcher or security configuration
mechanism, preferably
HttpSecurity#exceptionHandling().defaultAuthenticationEntryPointFor, to
associate this behavior with DeliveryPartyController’s join endpoint.
In `@src/main/java/com/leets/tdd/party/controller/DeliveryPartyController.java`:
- Around line 75-87: The DeliveryPartyController response shape is inconsistent
because joinDeliveryParty wraps its result in ApiResponse while the other
endpoints return raw bodies. Standardize joinDeliveryParty with the controller’s
established raw-body response style, updating its return type and return
statement while preserving the service result and Swagger documentation
consistency.
In `@src/main/java/com/leets/tdd/party/service/DeliveryPartyService.java`:
- Around line 206-210: Remove the debug System.out.println between
deliveryParty.cancel() and deliveryPartyRepository.save(deliveryParty) in the
cancellation flow, leaving the cancellation and persistence behavior unchanged.
In `@src/test/java/com/leets/tdd/party/service/DeliveryPartyServiceTest.java`:
- Around line 185-194: Remove the redundant try/catch wrapper around the
deleteDeliveryParty invocation in the test method, allowing exceptions to
propagate naturally and preserving the assignment to result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba6e99ed-d435-4b5a-8e8f-b7cc2964a660
📒 Files selected for processing (9)
src/main/java/com/leets/tdd/global/config/JwtAuthenticationEntryPoint.javasrc/main/java/com/leets/tdd/global/config/SecurityConfig.javasrc/main/java/com/leets/tdd/party/controller/DeliveryPartyController.javasrc/main/java/com/leets/tdd/party/domain/DeliveryParty.javasrc/main/java/com/leets/tdd/party/dto/response/JoinDeliveryPartyResponse.javasrc/main/java/com/leets/tdd/party/exception/PartyErrorCode.javasrc/main/java/com/leets/tdd/party/repository/PartyParticipantRepository.javasrc/main/java/com/leets/tdd/party/service/DeliveryPartyService.javasrc/test/java/com/leets/tdd/party/service/DeliveryPartyServiceTest.java
|
|
||
|
|
||
| // 배달팟 삭제(취소) API | ||
| @DeleteMapping("/{partyId}") | ||
| public ResponseEntity<Long> deleteDeliveryParty( | ||
| @PathVariable Long partyId, | ||
| @AuthenticationPrincipal Long currentUserId | ||
| ) { | ||
|
|
||
| Long deletedPartyId = | ||
| deliveryPartyService.deleteDeliveryParty( | ||
| partyId, | ||
| currentUserId | ||
| ); | ||
|
|
||
| return ResponseEntity.ok(deletedPartyId); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actual principal type set by the JWT filter, and find other `@AuthenticationPrincipal` Long usages.
rg -n -B3 -A10 'UsernamePasswordAuthenticationToken' --type=java
rg -n '`@AuthenticationPrincipal`' --type=javaRepository: Leets-Official/TDD-BE
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -u
echo "Files:"
git ls-files | sed -n '1,120p'
echo
echo "Find DeliveryPartyController and likely security/filter files:"
fd -i 'DeliveryPartyController|.*Security|.*Filter|.*Auth' . || true
echo
echo "Search AuthenticationPrincipal usages:"
rg -n '`@AuthenticationPrincipal`|AuthenticationPrincipalArgumentResolver|errorOnInvalidType|setPrincipal|setAuthentication|getPrincipal|UserPrincipal|username|principal' --type=java || trueRepository: Leets-Official/TDD-BE
Length of output: 8200
Bind the JWT principal to the controller parameter consistently.
The JWT filter’s authenticated identity is built as UserPrincipal, while update/delete receive @AuthenticationPrincipal Long currentUserId. Spring’s principal resolver won’t automatically unbox that composite principal, so owner authorization can resolve to null and reject owners with NOT_OWNER. Use @AuthenticationPrincipal UserPrincipal currentUser here and read currentUser.userId() (and apply the same fix consistently if other controller endpoints need the current user ID).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/leets/tdd/party/controller/DeliveryPartyController.java`
around lines 106 - 122, Update deleteDeliveryParty to accept
`@AuthenticationPrincipal` UserPrincipal currentUser instead of Long
currentUserId, and pass currentUser.userId() to
deliveryPartyService.deleteDeliveryParty. Apply the same principal binding and
userId extraction to any nearby update/delete controller endpoints using
`@AuthenticationPrincipal` Long.
1 participant
작업 내용
변경 사항
리뷰 포인트
체크리스트
Closes #93
Summary by CodeRabbit
New Features
Security
Bug Fixes