refactor: 예약 취소 내 멱등성 구현 및 웨이팅 등록 유닛테스트 추가 #359
Merged
Conversation
개요대기 등록 및 취소 작업을 위한 중앙화된 멱등성(idempotency) 처리 메커니즘을 도입합니다. HttpServletRequest를 컨트롤러와 서비스에 전파하여 멱등성 키 검증을 수행하고, 전용 DTO 및 저장소 메서드를 통해 Redis에 응답을 캐싱합니다. 변경 사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant Controller as WaitingController
participant Service as WaitingService
participant Repo as WaitingIdempotencyRepository
participant Redis
participant DB
Client->>Controller: cancelWaiting(request, HttpServletRequest)
Controller->>Service: cancelWaiting(..., HttpServletRequest)
Service->>Service: validateCancelIdempotency(HttpServletRequest)
Service->>Repo: findByCancelKey(idempotencyKey)
Repo->>Redis: 멱등성 키 조회
alt 캐시된 응답 존재
Redis-->>Repo: WaitingCancelIdempotencyValue
Repo-->>Service: Optional<response>
Service-->>Controller: CachedResponse (로깅)
else 캐시 미존재
Redis-->>Repo: null
Repo-->>Service: Optional.empty()
Service->>DB: 취소 처리 로직
DB-->>Service: CancelWaitingResponse
Service->>Repo: saveCancelIdempotencyValue(key, response)
Repo->>Redis: 응답 저장 (TTL 포함)
Service-->>Controller: CancelWaitingResponse
end
Controller-->>Client: ResponseEntity
예상 코드 리뷰 소요 시간🎯 3 (보통) | ⏱️ ~20분 관련 PR
제안 검토자
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🤖 Fix all issues with AI agents
In
`@nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/service/WaitingService.java`:
- Around line 59-67: The idempotency race between validateIdempotency and saving
the response must be eliminated by atomically claiming the idempotency key
before doing work in registerWaiting: update registerWaiting to attempt an
atomic "claim" (e.g., Redis SETNX with a short TTL or acquire a distributed lock
via Redisson) using the incoming idempotency key; if the claim fails,
immediately return the existing response or an appropriate in-flight/duplicate
response; if the claim succeeds, proceed with the operation, then call
saveIdempotencyValue to persist the final response and release the claim (or let
TTL expire) and ensure failures always release the lock/clean the claim so other
requests may proceed; keep references to registerWaiting, validateIdempotency,
and saveIdempotencyValue when applying the change.
- Around line 149-153: The cancel path calls
waitingIdempotencyRepository.saveCancelIdempotencyValue(...) without guarding
the Idempotency-Key header, causing Redis errors when the key is null/blank; in
WaitingService's cancelWaiting method either replicate the null/blank check used
by registerWaiting (only call saveCancelIdempotencyValue when
httpServletRequest.getHeader("Idempotency-Key") is non-null and not blank) or
add a helper method (e.g., saveCancelIdempotencyResponse) that encapsulates the
same guard logic as saveIdempotencyResponse and use it from cancelWaiting to
ensure consistent idempotency-key handling.
- Around line 156-175: In validateIdempotency and validateCancelIdempotency,
guard against a missing or blank Idempotency-Key by reading
httpServletRequest.getHeader("Idempotency-Key"), checking for null or blank
(trimmed) and returning null immediately if absent; only call
waitingIdempotencyRepository.findByKey(idempotentKey) and
waitingIdempotencyRepository.findByCancelKey(idempotentKey) when the key is
non-empty to avoid passing null to the repository (and prevent
IllegalArgumentException/NullPointerException).
In
`@nowait-app-user-api/src/test/java/com/nowait/applicationuser/waiting/service/WaitingServiceTest.java`:
- Around line 144-188: Add unit tests for cancelWaiting in WaitingServiceTest
covering: (1) when httpServletRequest.getHeader("Idempotency-Key") returns an
existing key and waitingIdempotencyRepository.findByKey(...) returns a cached
CancelWaitingResponse, assert waitingService.cancelWaiting(...) returns that
cached CancelWaitingResponse and no cancellation logic runs; (2) when
idempotency key is absent or blank, exercise waitingService.cancelWaiting(...)
normal cancel flow, stub repositories (reservationRepository,
waitingRedisRepository) and verify reservation state changes and
eventPublisher.publishEvent(...) is called; (3) on successful cancel verify
waitingIdempotencyRepository.saveIdempotencyValue(anyString(),
any(CancelWaitingResponse.class)) is invoked to persist the idempotent response.
Reference waitingService.cancelWaiting, CancelWaitingResponse,
waitingIdempotencyRepository.findByKey/saveIdempotencyValue,
httpServletRequest.getHeader("Idempotency-Key"), reservationRepository and
eventPublisher to locate code under test.
🧹 Nitpick comments (4)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/dto/WaitingCancelIdempotencyValue.java (1)
7-13:WaitingIdempotencyValue와 구조가 중복됩니다 — 제네릭 기반 클래스로 통합을 고려해 보세요.
WaitingIdempotencyValue와WaitingCancelIdempotencyValue는 동일한state + response패턴을 사용합니다. 제네릭 클래스 하나로 통합하면 향후 새로운 멱등성 타입 추가 시에도 DTO를 추가하지 않아도 됩니다.♻️ 제네릭 기반 통합 예시
`@Getter` `@NoArgsConstructor` `@AllArgsConstructor` public class IdempotencyValue<T> { private String state; private T response; }사용 시:
IdempotencyValue<RegisterWaitingResponse> // 등록용 IdempotencyValue<CancelWaitingResponse> // 취소용단, Jackson 역직렬화 시
TypeReference사용이 필요할 수 있습니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/redis/WaitingIdempotencyRepository.java (1)
27-58:findByKey와findByCancelKey는 역직렬화 대상 클래스만 다릅니다 — 제네릭 메서드로 통합 가능합니다.두 메서드는 동일한 Redis 조회 → null 체크 →
ObjectMapper.readValue흐름을 반복하고 있습니다. 제네릭 private 헬퍼로 추출하면 중복을 제거할 수 있습니다.♻️ 제네릭 헬퍼 추출 예시
+ private <T> Optional<T> findByKey(String key, Class<T> clazz) { + String value = redisTemplate.opsForValue().get(key); + if (value == null) { + return Optional.empty(); + } + try { + return Optional.of(objectMapper.readValue(value, clazz)); + } catch (Exception e) { + throw new IllegalArgumentException("Failed to deserialize value from Redis", e); + } + } + public Optional<WaitingIdempotencyValue> findByKey(String key) { - String idempotencyValue = redisTemplate.opsForValue().get(key); - if (idempotencyValue == null) { - return Optional.empty(); - } - try { - return Optional.of( - objectMapper.readValue(idempotencyValue, WaitingIdempotencyValue.class) - ); - } catch (Exception e) { - throw new IllegalArgumentException("Failed to deserialize value from Redis", e); - } + return findByKey(key, WaitingIdempotencyValue.class); } public Optional<WaitingCancelIdempotencyValue> findByCancelKey(String key) { - ... + return findByKey(key, WaitingCancelIdempotencyValue.class); }
save메서드 쌍도 동일한 패턴으로 통합 가능합니다.nowait-app-user-api/src/test/java/com/nowait/applicationuser/waiting/service/WaitingServiceTest.java (1)
60-63:@BeforeEach에서 설정한 stub이 일부 테스트에서 중복 설정되거나 무시됩니다.
registerWaiting_success(Line 151)에서httpServletRequest.getHeader("Idempotency-Key")를 다시 stub하고 있지만,@BeforeEach에서 이미 동일한 값으로 설정되어 있어 불필요합니다.registerWaiting_idempotentKeyNotExists,registerWaiting_success,registerWaiting_exceedWaitingLimit에서customOAuth2User를 로컬로 다시mock()하여 클래스 레벨@Mock필드를 가리고(shadow) 있습니다. 일관성을 위해 클래스 레벨@Mock을 사용하거나@BeforeEach에서 초기화하는 방식으로 통일하는 것이 좋습니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/service/WaitingService.java (1)
76-78: 대기 제한 수3L이 하드코딩되어 있습니다.향후 주점별 정책에 따라 제한 수가 달라질 수 있으므로, 설정 값이나
Store엔티티의 속성으로 관리하는 것이 좋습니다.
Comment on lines
59
to
67
| @Transactional | ||
| public RegisterWaitingResponse registerWaiting(CustomOAuth2User oAuth2User, String publicCode, RegisterWaitingRequest waitingRequest, HttpServletRequest httpServletRequest) { | ||
|
|
||
| String idempotentKey = httpServletRequest.getHeader("Idempotency-Key"); | ||
|
|
||
| // TODO 멱등성 검증 로직 점검 필요 | ||
| Optional<WaitingIdempotencyValue> existingIdempotencyValue = waitingIdempotencyRepository.findByKey(idempotentKey); | ||
| if (existingIdempotencyValue.isPresent()) { | ||
| log.info("Existing idempotency key found: {}", idempotentKey); | ||
| return existingIdempotencyValue.get().getResponse(); | ||
| // TODO 멱등키 동시성 처리 로직 고려 필요 (분산락 등) | ||
| RegisterWaitingResponse registerWaitingResponse = validateIdempotency(httpServletRequest); | ||
| if (registerWaitingResponse != null) { | ||
| log.info("Idempotent request detected. Returning existing response."); | ||
| return registerWaitingResponse; | ||
| } |
There was a problem hiding this comment.
멱등성 검증과 저장 사이의 경합 조건(Race Condition)에 대해.
TODO 주석(Line 62)에 언급된 대로, 멱등키 검증(validateIdempotency)과 응답 저장(saveIdempotencyValue) 사이에 경합 조건이 존재합니다. 동일한 멱등키를 가진 두 요청이 동시에 도달하면 둘 다 검증을 통과하여 중복 처리가 발생할 수 있습니다.
일반적인 해결 방안:
- Redis
SETNX(SET if Not eXists)를 활용하여 멱등키를 "PROCESSING" 상태로 먼저 선점한 후 작업 수행 - 분산 락(Redisson 등) 사용
현재 TODO로 인지하고 계신 것으로 보이지만, 프로덕션 배포 전에 반드시 해결이 필요한 사항입니다.
🤖 Prompt for AI Agents
In
`@nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/service/WaitingService.java`
around lines 59 - 67, The idempotency race between validateIdempotency and
saving the response must be eliminated by atomically claiming the idempotency
key before doing work in registerWaiting: update registerWaiting to attempt an
atomic "claim" (e.g., Redis SETNX with a short TTL or acquire a distributed lock
via Redisson) using the incoming idempotency key; if the claim fails,
immediately return the existing response or an appropriate in-flight/duplicate
response; if the claim succeeds, proceed with the operation, then call
saveIdempotencyValue to persist the final response and release the claim (or let
TTL expire) and ensure failures always release the lock/clean the claim so other
requests may proceed; keep references to registerWaiting, validateIdempotency,
and saveIdempotencyValue when applying the change.
Comment on lines
+149
to
+153
|
|
||
| // 멱등키가 있다면 멱등 응답 저장 | ||
| waitingIdempotencyRepository.saveCancelIdempotencyValue(httpServletRequest.getHeader("Idempotency-Key"), response); | ||
|
|
||
| return response; |
There was a problem hiding this comment.
취소 경로에서 멱등키 null/blank 검사 없이 saveCancelIdempotencyValue를 호출합니다.
registerWaiting의 saveIdempotencyResponse (Line 178-181)는 키가 null이거나 blank인지 확인한 후 저장하지만, cancelWaiting에서는 (Line 151) 아무 검사 없이 saveCancelIdempotencyValue를 직접 호출하고 있습니다.
멱등키 없이 취소 요청이 들어오면 null 키로 Redis에 저장을 시도하여 예외가 발생합니다. saveIdempotencyResponse와 동일한 가드를 추가하거나, 취소용 saveCancelIdempotencyResponse 헬퍼를 만들어 일관성을 유지하세요.
🐛 수정 제안
- // 멱등키가 있다면 멱등 응답 저장
- waitingIdempotencyRepository.saveCancelIdempotencyValue(httpServletRequest.getHeader("Idempotency-Key"), response);
+ // 멱등키가 있다면 멱등 응답 저장
+ String idempotentKey = httpServletRequest.getHeader("Idempotency-Key");
+ if (idempotentKey != null && !idempotentKey.isBlank()) {
+ waitingIdempotencyRepository.saveCancelIdempotencyValue(idempotentKey, response);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 멱등키가 있다면 멱등 응답 저장 | |
| waitingIdempotencyRepository.saveCancelIdempotencyValue(httpServletRequest.getHeader("Idempotency-Key"), response); | |
| return response; | |
| // 멱등키가 있다면 멱등 응답 저장 | |
| String idempotentKey = httpServletRequest.getHeader("Idempotency-Key"); | |
| if (idempotentKey != null && !idempotentKey.isBlank()) { | |
| waitingIdempotencyRepository.saveCancelIdempotencyValue(idempotentKey, response); | |
| } | |
| return response; |
🤖 Prompt for AI Agents
In
`@nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/service/WaitingService.java`
around lines 149 - 153, The cancel path calls
waitingIdempotencyRepository.saveCancelIdempotencyValue(...) without guarding
the Idempotency-Key header, causing Redis errors when the key is null/blank; in
WaitingService's cancelWaiting method either replicate the null/blank check used
by registerWaiting (only call saveCancelIdempotencyValue when
httpServletRequest.getHeader("Idempotency-Key") is non-null and not blank) or
add a helper method (e.g., saveCancelIdempotencyResponse) that encapsulates the
same guard logic as saveIdempotencyResponse and use it from cancelWaiting to
ensure consistent idempotency-key handling.
Comment on lines
+156
to
175
| // 멱등키 검증 메서드 | ||
| private RegisterWaitingResponse validateIdempotency(HttpServletRequest httpServletRequest) { | ||
| String idempotentKey = httpServletRequest.getHeader("Idempotency-Key"); | ||
|
|
||
| // 멱등키 검증 - 이미 동일한 멱등키로 등록된 웨이팅이 있는지 확인 | ||
| // TODO 멱등성 검증 로직 점검 필요 | ||
| return waitingIdempotencyRepository.findByKey(idempotentKey) | ||
| .map(WaitingIdempotencyValue::getResponse) | ||
| .orElse(null); | ||
| } | ||
|
|
||
| private CancelWaitingResponse validateCancelIdempotency(HttpServletRequest httpServletRequest) { | ||
| String idempotentKey = httpServletRequest.getHeader("Idempotency-Key"); | ||
|
|
||
| // 멱등키 검증 - 이미 동일한 멱등키로 등록된 웨이팅이 있는지 확인 | ||
| // TODO 멱등성 검증 로직 점검 필요 | ||
| return waitingIdempotencyRepository.findByCancelKey(idempotentKey) | ||
| .map(WaitingCancelIdempotencyValue::getResponse) | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
Idempotency-Key 헤더가 없으면(null) NullPointerException 또는 IllegalArgumentException이 발생합니다.
validateIdempotency와 validateCancelIdempotency 모두 httpServletRequest.getHeader("Idempotency-Key")의 결과를 검증 없이 바로 findByKey(null) / findByCancelKey(null)에 전달합니다. Spring Data Redis의 opsForValue().get(null)은 IllegalArgumentException을 throw합니다.
멱등키가 선택적(optional)이라면, null/blank 체크 후 바로 null을 반환해야 합니다.
🐛 수정 제안
private RegisterWaitingResponse validateIdempotency(HttpServletRequest httpServletRequest) {
String idempotentKey = httpServletRequest.getHeader("Idempotency-Key");
+ if (idempotentKey == null || idempotentKey.isBlank()) {
+ return null;
+ }
return waitingIdempotencyRepository.findByKey(idempotentKey)
.map(WaitingIdempotencyValue::getResponse)
.orElse(null);
}
private CancelWaitingResponse validateCancelIdempotency(HttpServletRequest httpServletRequest) {
String idempotentKey = httpServletRequest.getHeader("Idempotency-Key");
+ if (idempotentKey == null || idempotentKey.isBlank()) {
+ return null;
+ }
return waitingIdempotencyRepository.findByCancelKey(idempotentKey)
.map(WaitingCancelIdempotencyValue::getResponse)
.orElse(null);
}🤖 Prompt for AI Agents
In
`@nowait-app-user-api/src/main/java/com/nowait/applicationuser/waiting/service/WaitingService.java`
around lines 156 - 175, In validateIdempotency and validateCancelIdempotency,
guard against a missing or blank Idempotency-Key by reading
httpServletRequest.getHeader("Idempotency-Key"), checking for null or blank
(trimmed) and returning null immediately if absent; only call
waitingIdempotencyRepository.findByKey(idempotentKey) and
waitingIdempotencyRepository.findByCancelKey(idempotentKey) when the key is
non-empty to avoid passing null to the repository (and prevent
IllegalArgumentException/NullPointerException).
Comment on lines
+144
to
+188
| @Test | ||
| @DisplayName("웨이팅 정상 등록 시 DB 저장, 이벤트 발생, 멱등 응답 저장 수행") | ||
| void registerWaiting_success() { | ||
| // given | ||
| CustomOAuth2User customOAuth2User = mock(CustomOAuth2User.class); | ||
| RegisterWaitingRequest request = new RegisterWaitingRequest(4); | ||
|
|
||
| when(httpServletRequest.getHeader("Idempotency-Key")).thenReturn(IDEMPOTENCY_KEY); | ||
| when(waitingIdempotencyRepository.findByKey(IDEMPOTENCY_KEY)) | ||
| .thenReturn(Optional.empty()); | ||
|
|
||
| Long userId = 10L; | ||
| String publicCode = "ZiVXAD1vVr5b"; | ||
|
|
||
| Store store = Store.builder().publicCode(publicCode).build(); | ||
| User user = User.builder().id(userId).build(); | ||
|
|
||
| when(storeRepository.findByPublicCodeAndDeletedFalse(publicCode)).thenReturn(java.util.Optional.of(store)); | ||
| when(customOAuth2User.getUserId()).thenReturn(userId); | ||
| when(customOAuth2User.getUserId()).thenReturn(10L); | ||
| when(userRepository.findById(userId)).thenReturn(java.util.Optional.of(user)); | ||
| RegisterWaitingResponse response = waitingService.registerWaiting(customOAuth2User, publicCode, request, httpServletRequest); | ||
|
|
||
| doNothing() | ||
| .when(waitingRedisRepository) | ||
| .incrementAndCheckWaitingLimit(userId, 3L); | ||
|
|
||
| when(waitingRedisRepository.incrementDailySequence(anyString())).thenReturn(1L); | ||
|
|
||
| // when | ||
| RegisterWaitingResponse response = waitingService.registerWaiting( | ||
| customOAuth2User, | ||
| publicCode, | ||
| request, | ||
| httpServletRequest | ||
| ); | ||
|
|
||
| // then | ||
| verify(waitingIdempotencyRepository).findByKey(anyString()); | ||
| assertThat(response).isNotNull(); | ||
| assertThat(response.getPartySize()).isEqualTo(4); | ||
| assertThat(response.getWaitingNumber()).isNotBlank(); | ||
|
|
||
| verify(waitingRedisRepository).incrementAndCheckWaitingLimit(userId, 3L); | ||
| verify(reservationRepository).save(any(Reservation.class)); | ||
| verify(eventPublisher).publishEvent(any(AddWaitingRegisterEvent.class)); | ||
| verify(waitingIdempotencyRepository).saveIdempotencyValue(anyString(), any(RegisterWaitingResponse.class)); | ||
| } |
There was a problem hiding this comment.
취소 대기(cancelWaiting) 멱등성에 대한 테스트가 누락되었습니다.
PR 목표가 "예약 취소 내 멱등성 구현"인데, cancelWaiting에 대한 테스트가 전혀 없습니다. 최소한 다음 시나리오에 대한 테스트를 추가해야 합니다:
- 멱등키가 이미 존재하여 캐시된
CancelWaitingResponse가 반환되는 경우 - 멱등키가 없거나 blank인 경우의 정상 취소 흐름
- 취소 성공 시 멱등 응답이 Redis에 저장되는지 검증
🤖 Prompt for AI Agents
In
`@nowait-app-user-api/src/test/java/com/nowait/applicationuser/waiting/service/WaitingServiceTest.java`
around lines 144 - 188, Add unit tests for cancelWaiting in WaitingServiceTest
covering: (1) when httpServletRequest.getHeader("Idempotency-Key") returns an
existing key and waitingIdempotencyRepository.findByKey(...) returns a cached
CancelWaitingResponse, assert waitingService.cancelWaiting(...) returns that
cached CancelWaitingResponse and no cancellation logic runs; (2) when
idempotency key is absent or blank, exercise waitingService.cancelWaiting(...)
normal cancel flow, stub repositories (reservationRepository,
waitingRedisRepository) and verify reservation state changes and
eventPublisher.publishEvent(...) is called; (3) on successful cancel verify
waitingIdempotencyRepository.saveIdempotencyValue(anyString(),
any(CancelWaitingResponse.class)) is invoked to persist the idempotent response.
Reference waitingService.cancelWaiting, CancelWaitingResponse,
waitingIdempotencyRepository.findByKey/saveIdempotencyValue,
httpServletRequest.getHeader("Idempotency-Key"), reservationRepository and
eventPublisher to locate code under test.
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.
작업 요약
Issue Link
#348
문제점 및 어려움
해결 방안
Reference
Summary by CodeRabbit
릴리스 노트