Feat/295 assigned coupon#296
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough지정 쿠폰 도메인을 새로 추가하여 대상자 등록, 자격 확인, 동시성 제어 기반 발급 처리를 구현합니다. Redis 해시와 Redisson 분산락을 사용하며, 발급 이벤트를 S3로 비동기 로깅하고 REST API 및 단위 테스트를 제공합니다. ChangesAssigned Coupon Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/controller/AssignedCouponController.java`:
- Around line 94-99: The registerOneTarget method in AssignedCouponController
can NPE when target is null because List.of(target) throws; add a null guard in
AssignedCouponController.registerOneTarget to return a 400 response instead of
letting an exception bubble (e.g., check if target == null and return
ApiResponse.onErrorBadRequest(...) or ResponseEntity.badRequest() with an
appropriate ApiResponse), or annotate the request with validation (`@Valid` and
add `@NotNull` on AssignedCouponTargetRequest) and ensure validation is enabled so
assignedCouponService.registerTargets is only called with a non-null list.
- Around line 71-77: The controller currently reads sensitive storePin via
`@RequestParam` in AssignedCouponController.issueCoupon; change the API to accept
a JSON request body instead: create a small DTO (e.g., IssueCouponRequest with a
storePin field, add validation like `@NotBlank`), replace the method parameter
from `@RequestParam` String storePin to `@RequestBody` `@Valid` IssueCouponRequest
request and pass request.getStorePin() into
assignedCouponService.issueCoupon(authentication.getName(),
request.getStorePin()); update any controller annotations/docs/tests that assert
a query parameter to reflect the new JSON body, and adjust the service signature
if your API layer previously relied on framework binding semantics.
In
`@src/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.java`:
- Around line 66-71: In AssignedCouponService inside the loop over targets, add
null-safety and stricter validation: skip and record targets that are null
before calling target.phoneNum(), treat empty or blank target.name() (after
trimming) as invalid, and ensure normalizePhoneNum(target.phoneNum()) handles
nulls or is only called after null check; when validation fails, add the
original target identifier to failed and continue. Update logic around the
for-loop and the use of normalizePhoneNum, target.phoneNum(), target.name(), and
the failed collection so no NullPointerException is thrown and blank names are
rejected (optionally log the reason).
- Around line 165-167: AssignedCouponService uses lock.tryLock(3, 3,
TimeUnit.SECONDS) which disables Redisson's watchdog (fixed 3s lease) and risks
early expiry; change the call on the lock object to the wait-only overload
(e.g., lock.tryLock(3, TimeUnit.SECONDS)) so the watchdog/auto-renewal remains
enabled during processing, ensure the same lock variable and surrounding
try/finally that unlocks the lock remain intact (update any uses in methods like
the coupon issuance path that currently call tryLock(3, 3, TimeUnit.SECONDS)).
In
`@src/test/java/com/soongsil/CoffeeChat/domain/assignedcoupon/message/AssignedCouponIssueEventListenerTest.java`:
- Around line 85-97: The test handleEvent_swallowsS3UploadException currently
only asserts no exception; also verify that upload was actually attempted by
adding a Mockito verify call for amazonS3Service.uploadJsonFile with the
expected arguments (use the same JSON from objectMapper.writeValueAsString and
the event identifiers) so the test fails if the upload call is omitted; locate
this in AssignedCouponIssueEventListenerTest near
handleAssignedCouponIssuedEvent and add
verify(amazonS3Service).uploadJsonFile(...) referencing
amazonS3Service.uploadJsonFile and the AssignedCouponIssuedEvent values.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1f614779-cc1f-4ed3-906b-63891b3e2d56
📒 Files selected for processing (12)
src/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/controller/AssignedCouponController.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponCheckResponse.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponRegisterResult.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponResponse.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponTargetRequest.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/message/AssignedCouponIssueEventListener.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/message/AssignedCouponIssuedEvent.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.javasrc/main/java/com/soongsil/CoffeeChat/global/exception/GlobalErrorCode.javasrc/main/java/com/soongsil/CoffeeChat/global/exception/GlobalExceptionAdvice.javasrc/test/java/com/soongsil/CoffeeChat/domain/assignedcoupon/message/AssignedCouponIssueEventListenerTest.javasrc/test/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponServiceTest.java
Comment on lines
+85
to
+97
| void handleEvent_swallowsS3UploadException() throws Exception { | ||
| // given | ||
| AssignedCouponIssuedEvent event = | ||
| new AssignedCouponIssuedEvent( | ||
| "u1", "n", "01000000000", "AC-0001", LocalDateTime.now()); | ||
| given(objectMapper.writeValueAsString(event)).willReturn("{}"); | ||
| given(amazonS3Service.uploadJsonFile(anyString(), anyString(), anyString())) | ||
| .willThrow(new RuntimeException("S3 error")); | ||
|
|
||
| // when & then | ||
| assertThatCode(() -> listener.handleAssignedCouponIssuedEvent(event)) | ||
| .doesNotThrowAnyException(); | ||
| } |
There was a problem hiding this comment.
S3 실패 테스트에서 “업로드 시도” 자체를 함께 검증해 주세요.
현재는 예외 미전파만 확인해서, 업로드 호출이 누락돼도 테스트가 통과할 수 있습니다. verify(amazonS3Service).uploadJsonFile(...)를 추가해 경로를 고정해 주세요.
보강 예시
`@Test`
`@DisplayName`("S3 업로드 실패해도 예외를 외부로 던지지 않음 (로깅만)")
void handleEvent_swallowsS3UploadException() throws Exception {
@@
assertThatCode(() -> listener.handleAssignedCouponIssuedEvent(event))
.doesNotThrowAnyException();
+
+ verify(amazonS3Service).uploadJsonFile(anyString(), anyString(), anyString());
}🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/message/AssignedCouponIssueEventListenerTest.java`
around lines 85 - 97, The test handleEvent_swallowsS3UploadException currently
only asserts no exception; also verify that upload was actually attempted by
adding a Mockito verify call for amazonS3Service.uploadJsonFile with the
expected arguments (use the same JSON from objectMapper.writeValueAsString and
the event identifiers) so the test fails if the upload call is omitted; locate
this in AssignedCouponIssueEventListenerTest near
handleAssignedCouponIssuedEvent and add
verify(amazonS3Service).uploadJsonFile(...) referencing
amazonS3Service.uploadJsonFile and the AssignedCouponIssuedEvent values.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.java`:
- Around line 231-235: The catch block in AssignedCouponService that currently
catches InterruptedException, restores the interrupt flag, and throws an
IllegalStateException should instead throw a domain-level exception that maps to
your 503 concurrency error so the API contract remains consistent; update the
catch to keep Thread.currentThread().interrupt(), then throw your domain
exception (e.g., AssignedCouponException or ConcurrencyDomainException used in
your project) with the appropriate CONCURRENCY/LOCKING error code and pass the
InterruptedException as the cause so the stacktrace is preserved and the error
mapper will return 503.
- Around line 74-101: In AssignedCouponService replace the raw target.name()
usage with a trimmed value: compute a trimmedName (e.g., target.name() == null ?
null : target.name().trim()), use trimmedName for the blank/null checks (instead
of the current name variable) and pass trimmedName into the Redis hash putAll
call (replace target.name() in the Map) so stored names match later exact-match
lookups; ensure you guard trimming against null to avoid NPEs and update any
other places in this method that still reference target.name() to use
trimmedName.
- Around line 168-170: normalizePhoneNum can return an empty string and you must
guard against creating shared keys for multiple users; after calling
normalizePhoneNum(user.getPhoneNum()) in AssignedCouponService, validate that
phoneNum is non-empty and if it is empty either throw a clear exception (e.g.,
IllegalArgumentException) or return an error response, and avoid creating
targetKey ("assigned-coupon:target:" + phoneNum) or the related lock key when
phoneNum is blank; update the code around the normalizePhoneNum call and any
lock/key creation to perform this check and include a descriptive log message
referencing phoneNum, normalizePhoneNum, targetKey and the lock key variable.
In
`@src/test/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponServiceTest.java`:
- Line 387: The test in AssignedCouponServiceTest currently only verifies that
valueOperations.increment(...) was never called, but doesn't assert that the
Redis value-access path wasn't entered; update the test to also verify that
redisTemplate.opsForValue() was never invoked (e.g., add a verify(redisTemplate,
never()).opsForValue() check) so both the opsForValue() call and
valueOperations.increment(...) are asserted as not executed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a0be90d-4153-4510-94cc-afb7f157a087
📒 Files selected for processing (11)
src/main/java/com/soongsil/CoffeeChat/CoffeeChatApplication.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/controller/AssignedCouponController.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponCheckResponse.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponIssueRequest.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponResponse.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponTargetRequest.javasrc/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.javasrc/main/java/com/soongsil/CoffeeChat/domain/chat/entity/ChatRoomUser.javasrc/main/java/com/soongsil/CoffeeChat/domain/possibleDate/entity/PossibleDate.javasrc/main/java/com/soongsil/CoffeeChat/global/dev/DevDataInitializer.javasrc/test/java/com/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponServiceTest.java
💤 Files with no reviewable changes (2)
- src/main/java/com/soongsil/CoffeeChat/domain/chat/entity/ChatRoomUser.java
- src/main/java/com/soongsil/CoffeeChat/CoffeeChatApplication.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/soongsil/CoffeeChat/domain/possibleDate/entity/PossibleDate.java
- src/main/java/com/soongsil/CoffeeChat/global/dev/DevDataInitializer.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponTargetRequest.java
- src/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponResponse.java
- src/main/java/com/soongsil/CoffeeChat/domain/assignedcoupon/dto/AssignedCouponCheckResponse.java
Comment on lines
+74
to
+101
| String name = target.name(); | ||
|
|
||
| // name, phoneNum blank 검증 | ||
| if (phoneNum == null | ||
| || phoneNum.isBlank() | ||
| || name == null | ||
| || name.isBlank()) { | ||
| failed.add(target.phoneNum()); | ||
| continue; | ||
| } | ||
|
|
||
| String targetKey = "assigned-coupon:target:" + phoneNum; | ||
|
|
||
| try { | ||
| // hash 등록 시도 -> ticket TARGETED 로 초기화 | ||
| if (redisTemplate.hasKey(targetKey)) { | ||
| dupCount++; | ||
| continue; | ||
| } | ||
|
|
||
| redisTemplate | ||
| .opsForHash() | ||
| .putAll( | ||
| targetKey, | ||
| Map.of( | ||
| "name", target.name(), | ||
| "status", "TARGETED", | ||
| "registeredAt", LocalDateTime.now().toString())); |
There was a problem hiding this comment.
이름 공백 정리 누락으로 자격 조회가 오탐될 수 있습니다.
Line 74에서 name을 검증만 하고 저장 시(Line 99) 원본 target.name()을 그대로 써서, 앞뒤 공백이 포함된 대상자는 Line 133의 정확 일치 비교에서 탈락할 수 있습니다. 입력 단계에서 trim 후 동일 값을 저장하세요.
수정 예시
- String name = target.name();
+ String name = target.name() == null ? null : target.name().trim();
// name, phoneNum blank 검증
if (phoneNum == null
|| phoneNum.isBlank()
|| name == null
|| name.isBlank()) {
failed.add(target.phoneNum());
continue;
}
@@
redisTemplate
.opsForHash()
.putAll(
targetKey,
Map.of(
- "name", target.name(),
+ "name", name,
"status", "TARGETED",
"registeredAt", LocalDateTime.now().toString()));🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.java`
around lines 74 - 101, In AssignedCouponService replace the raw target.name()
usage with a trimmed value: compute a trimmedName (e.g., target.name() == null ?
null : target.name().trim()), use trimmedName for the blank/null checks (instead
of the current name variable) and pass trimmedName into the Redis hash putAll
call (replace target.name() in the Map) so stored names match later exact-match
lookups; ensure you guard trimming against null to avoid NPEs and update any
other places in this method that still reference target.name() to use
trimmedName.
Comment on lines
+168
to
+170
| String phoneNum = normalizePhoneNum(user.getPhoneNum()); | ||
| String targetKey = "assigned-coupon:target:" + phoneNum; | ||
|
|
There was a problem hiding this comment.
정규화된 전화번호 빈값 가드가 없어 락/키 충돌 위험이 있습니다.
Line 168에서 숫자 외 문자를 제거한 결과가 빈 문자열일 수 있는데, 현재는 그대로 Line 172 lock key와 Line 169 target key를 생성합니다. 잘못된 사용자 데이터가 들어오면 서로 다른 사용자가 동일 빈 key를 공유할 수 있습니다.
수정 예시
String phoneNum = normalizePhoneNum(user.getPhoneNum());
+ if (phoneNum == null || phoneNum.isBlank()) {
+ throw new GlobalException(GlobalErrorCode.ASSIGNED_COUPON_PHONE_NOT_SET);
+ }
String targetKey = "assigned-coupon:target:" + phoneNum;🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponService.java`
around lines 168 - 170, normalizePhoneNum can return an empty string and you
must guard against creating shared keys for multiple users; after calling
normalizePhoneNum(user.getPhoneNum()) in AssignedCouponService, validate that
phoneNum is non-empty and if it is empty either throw a clear exception (e.g.,
IllegalArgumentException) or return an error response, and avoid creating
targetKey ("assigned-coupon:target:" + phoneNum) or the related lock key when
phoneNum is blank; update the code around the normalizePhoneNum call and any
lock/key creation to perform this check and include a descriptive log message
referencing phoneNum, normalizePhoneNum, targetKey and the lock key variable.
|
|
||
| verify(rLock).unlock(); | ||
| verifyNoInteractions(eventPublisher); | ||
| verify(valueOperations, never()).increment(anyString()); |
There was a problem hiding this comment.
재발급 차단 검증에서 opsForValue() 미호출도 함께 검증해 주세요.
현재 검증은 valueOperations.increment만 확인해서, 값 연산 경로 진입 자체를 더 명확히 막지는 못합니다.
수정 예시
verify(rLock).unlock();
verifyNoInteractions(eventPublisher);
verify(valueOperations, never()).increment(anyString());
+verify(redisTemplate, never()).opsForValue();🤖 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/soongsil/CoffeeChat/domain/assignedcoupon/service/AssignedCouponServiceTest.java`
at line 387, The test in AssignedCouponServiceTest currently only verifies that
valueOperations.increment(...) was never called, but doesn't assert that the
Redis value-access path wasn't entered; update the test to also verify that
redisTemplate.opsForValue() was never invoked (e.g., add a verify(redisTemplate,
never()).opsForValue() check) so both the opsForValue() call and
valueOperations.increment(...) are asserted as not executed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔎 Resolved Issue
event도메인과 기능 혼동 방지를 위해 도메인 분리 → 차후 컨벤션 논의 필요✅ Title
📄 Content
GET /api/v2/assigned-coupons/eligibility— 보관함 진입 시 발급 자격 확인 (이름 + 전화번호 이중 검증)POST /api/v2/assigned-coupons/coupons— 매장 PIN 인증 + 쿠폰 발급 (=사용 처리)POST /api/v2/assigned-coupons/admin/register— 관리자: 대상자 일괄 등록POST /api/v2/assigned-coupons/admin/register/one— 관리자: 단건 등록checkEligibility(username)— Redis Hashassigned-coupon:target:{phoneNum}조회 후 이름 일치까지 확인 →phoneNum만으로 매칭되지 않게 이중 검증issueCoupon(username, storePin)— PIN 검증 → User 조회 → Redisson 분산락 획득 → 락 안 재검증 (TOCTOU 방어) →INCRatomic 으로 쿠폰 번호 발급 → 상태를USED로 단일 갱신 → S3 로깅 이벤트 발행registerTargets(targets)— 전화번호 정규화 후 Redis Hash 등록, 중복/실패 카운트 반환assigned-coupon:target:{phoneNum}(Hash) —name,status,couponNumber,issuedAt,usedAt,claimedBy,registeredAt→HSET putAll로 부분 갱신, ziplist 인코딩으로 메모리 효율assigned-coupon:seq(String) —INCRatomic 으로 쿠폰 번호 유일성 보장 (AC-%04d포맷)assigned-coupon:used:count(String) — 발급 통계용 카운터lock:assigned-coupon:issue:{phoneNum}(Redisson RLock) —phoneNum단위 fine-grained locking 으로 동시 발급 차단하면서 처리량 유지TARGETED → USED)tryLock(3, 3, SECONDS)— waitTime/leaseTime 분리, 클라이언트 장애 시 자동 해제로 deadlock 방어HGETALL재실행 → Time-Of-Check, Time-Of-Use 갭 동안 다른 발급에 의해 상태가 바뀌었을 가능성 차단phoneNum신뢰성: 기존UserService.updateUser흐름에서 SMS 인증 강제됨 →User.phoneNum은 신뢰 가능한 값name + phoneNum이중 매칭: phoneNum 단독 매칭 시 우연/악의적 일치 위험 → 사전 등록 명단의name과User.name모두 일치해야 통과username은Authentication에서 추출한 OAuth identity → 클라이언트 조작 불가claimedBy필드에username기록 → 누가 발급받았는지 추적 가능ApplicationEventPublisher로AssignedCouponIssuedEvent발행 →@Async @EventListener로 비동기 처리event-logs/assigned-coupon/assigned-coupon-{couponNumber}_{timestamp}.jsonASSIGNED_COUPON_PHONE_NOT_SET(400) — User.phoneNum 미등록ASSIGNED_COUPON_NOT_TARGET(404) — 사전 등록 명단에 없음 또는 이름 불일치ASSIGNED_COUPON_ALREADY_ISSUED(409) — 이미 발급(=사용) 처리됨EVENT_PIN_MISMATCH,EVENT_CONCURRENCY_ERROR는 기존event도메인 코드 재사용AssignedCouponServiceTest18개 —@Nested로RegisterTargets/CheckEligibility/IssueCoupon그룹화ArgumentCaptor로 발행된AssignedCouponIssuedEvent의 username/name/phoneNum/couponNumber 모두 검증AssignedCouponIssueEventListenerTest3개 — 정상 S3 업로드 경로 검증, 직렬화/업로드 실패 시 예외 swallow 검증Summary by CodeRabbit
New Features
Errors
Tests