[FEAT] 찜 등록·해제·내 찜 목록 API - #24
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
CheatIsKey
left a comment
There was a problem hiding this comment.
리뷰 요약
6b12b3c 기준, base는 #19의 head 8e9a1af입니다.
동시성 부분이 이 PR의 핵심인데, 처음 구현이 틀렸다는 걸 테스트로 잡아내고 그 경위를 본문에 남긴 것이 가장 좋았습니다. UnexpectedRollbackException의 원인을 "예외를 잡아도 세션이 이미 rollback-only로 표시돼 커밋에서 터진다"까지 정확히 짚었고, 해법(판정과 변경을 한 구문으로)도 맞습니다. CLIENT_FOUND_ROWS 때문에 반환값을 두지 않겠다고 명시한 것도 나중에 그 값을 믿고 분기하는 코드를 미리 막는 좋은 결정입니다.
블로킹은 없습니다. 다만 1·2번은 머지 전에 정리하는 게 싸다고 봅니다.
머지 전
1. 비공개 매물 거부에 CommonErrorCode.CONFLICT(C004)를 씁니다 — 409를 고른 이유가 코드에서 사라집니다
본문의 근거는 이렇습니다.
404를 주면 ... 클라이언트가 "이미 팔린 상품입니다" 안내를 만들 수 없습니다.
그런데 실제로 나가는 건 C004입니다.
throw new BusinessException(
CommonErrorCode.CONFLICT, "판매 중인 매물만 찜할 수 있습니다.");C004는 "요청이 현재 상태와 충돌합니다"라는 범용 코드라, 클라이언트는 이 응답을 다른 모든 409와 구분할 수 없습니다. 안내 문구를 만들려면 결국 한국어 메시지를 문자열 비교해야 하고, 그러면 404 대신 409를 고른 이유가 없어집니다.
같은 판단의 선례가 이미 두 군데 있습니다.
ListingErrorCode가 이 도메인에 이미 있습니다 (LST001 PRICE_DROP_LIMIT_EXCEEDED, #19에서 신설)CommonErrorCode안에 주석으로 남아 있습니다 — "CONFLICT(C004)와 상태 코드는 같지만 코드를 분리한다" 며C009 CONCURRENT_MODIFICATION을 나눈 바로 그 자리입니다
LST0xx FAVORITE_TARGET_NOT_LISTABLE(409) 하나 추가하는 정도면 됩니다. CLAUDE.md의 "한 번 공개한 코드는 변경·재사용 금지"를 감안하면 클라이언트가 붙기 전인 지금이 가장 쌉니다.
2. 엔티티의 생성 경로를 프로덕션이 타지 않습니다
src/main 전체에서 호출자가 0인 것들입니다.
| 심볼 | main 호출자 | test 호출자 |
|---|---|---|
ListingFavorite.of(...) |
없음 | 5곳 |
lowerBasePriceIfDropped(...) |
없음 | 4곳 |
markNotified(...) |
없음 | 1곳 |
last_notified_at 컬럼 |
쓰는 코드 없음 | 1곳 |
findByUserIdAndListing(...) |
없음 | 3곳 |
두 가지가 섞여 있습니다.
(a) of()와 프로덕션 INSERT가 갈라져 있습니다. 실제 등록은 네이티브 insertIfAbsent로만 들어가므로 of()의 null 검증은 운영에서 한 번도 실행되지 않고, "기준가 = 찜 시점 가격" 규칙이 of()와 FavoriteCommandService 두 곳에 각각 있습니다. ListingFavoriteTest 4건과 ListingFavoriteRepositoryTest의 favorite() 헬퍼는 전부 of() 경로를 지키고 있어서, 누가 of()의 기준가 규칙을 바꿔도 운영 동작은 안 바뀌고 테스트는 계속 통과합니다. 지금은 두 곳이 같은 값을 쓰니 문제가 없지만, 규칙이 하나뿐인데 정의가 둘인 상태입니다. of()를 지우고 테스트도 insertIfAbsent로 넣거나, 서비스가 기준가를 엔티티에서 얻어오게 하는 쪽 중 하나로 모아주세요.
(b) 알림 쪽(lowerBasePriceIfDropped·markNotified·last_notified_at)은 이 PR에 소비자가 없습니다. 본문에도 "발송 자체는 알림 도메인 몫"이라고 적으셨습니다. 설계 의도(판정과 발송 기록 분리)는 좋은데, CLAUDE.md 2번(요청받은 것만 최소로)에 걸립니다. 알림 도메인 PR로 옮기거나, 남긴다면 deferred 이슈(사유·담당·검토일·영향)를 걸어주세요. notify_base_price는 실제로 쓰이니 컬럼 자체는 이 PR 소관이 맞습니다.
확인 부탁드립니다
3. 해제 후 재찜하면 기준가가 올라갑니다 — 가드가 절반만 막습니다
두 주석이 서로 반대 이야기를 합니다.
ListingFavoriteRepository.insertIfAbsent:
재찜 때 기준가를 다시 쓰면, 판매자가 값을 올린 뒤 사용자가 하트를 다시 누르는 것만으로 기준가가 올라가 "지금까지의 최저가 대비 인하" 정책이 무너진다.
ListingFavorite 클래스:
해제는 하드 삭제다. ... 다시 찜하면 그 시점 가격으로 기준가가 새로 시작한다.
해제가 하드 삭제이므로 해제 → 재찜 경로에서는 정확히 첫 번째 주석이 막겠다는 일이 일어납니다. 950,000에 찜 → 판매자가 1,200,000으로 인상 → 사용자가 하트를 껐다 켬 → 기준가 1,200,000. ON DUPLICATE KEY UPDATE 가드는 행이 살아 있을 때만 작동합니다.
정책이 어느 쪽인지 정해야 합니다.
- 행 단위 최저가(찜을 다시 시작하면 리셋)라면 →
insertIfAbsent주석에서 "정책이 무너진다"는 표현을 빼고, 가드의 목적을 "연타 시 기준가 흔들림 방지"로 좁혀 적어주세요 - 사용자·매물 쌍의 역대 최저가라면 → 하드 삭제로는 지킬 수 없습니다. 알림 도메인이 이 위에 얹히기 전에 정하는 게 맞습니다
4. keepsFirstBasePriceUnderConcurrency가 이름이 약속한 것을 검증하지 않습니다
listingRepository.saveAndFlush(Listing.register(..., 950_000, ...)); // @BeforeEach
...
favoriteCommandService.add(USER, PUBLIC_ID);
runConcurrently(() -> favoriteCommandService.add(USER, PUBLIC_ID));
assertThat(...getNotifyBasePrice()).isEqualTo(950_000);매물 가격이 950,000으로 고정이고 add()는 listing.getPrice()를 그대로 넘기므로, 모든 스레드가 950,000을 씁니다. ON DUPLICATE KEY UPDATE를 notify_base_price = VALUES(notify_base_price)로 바꿔도 이 테스트는 통과합니다.
다행히 가드 자체는 ListingFavoriteRepositoryTest.insertIfAbsentIsIdempotent가 950,000 → 1,200,000으로 제대로 지키고 있습니다. 그래서 실제 회귀 위험은 없고, 이 테스트만 비어 있는 상태입니다. 동시성 경로에서 보고 싶으셨다면 첫 찜 뒤 매물 가격을 올리고 나서 동시 재찜을 걸어야 합니다.
5. /api/users/me/favorites를 매물 도메인이 소유합니다
/api/users를 매핑하는 유일한 클래스가 domain/listing/controller/FavoriteController입니다. 유저 도메인이 들어오면 같은 루트를 두 도메인이 나눠 갖게 됩니다.
인증 담당으로서 붙이자면, 인증 도메인(#18, 아직 미머지)은 /api/v1/auth/me라 "내 것" 경로가 /api/v1/auth/me와 /api/users/me로 갈립니다. v1 접두어 문제는 제 PR에서 /api/auth/*로 내리는 것으로 정리하겠습니다. 여기서는 찜 목록을 /api/users/me/favorites로 둘지 /api/listings/favorites 쪽으로 옮길지만 정해주시면 됩니다 — 유저 도메인 담당과 합의가 필요하면 deferred로 남겨도 좋겠습니다.
사소한 것
컨트롤러 분리 사유가 정확하지 않습니다. javadoc은 이렇게 적혀 있습니다.
ListingController에 끼워 넣으면 클래스 레벨@RequestMapping과 어긋나므로
그런데 ListingController는 @RequestMapping("/api/listings") 아래에 이미 @PatchMapping("/{publicId}/status")를 갖고 있어서, /{publicId}/favorite 두 개는 그대로 들어갑니다. 어긋나는 건 목록 하나뿐입니다. 분리 자체는 괜찮은 판단이지만, 이유는 "찜은 별도 기능 단위"라고 쓰는 게 맞습니다.
FavoriteConcurrencyTest가 listings를 비트랜잭션으로 전역 삭제합니다. @BeforeEach·@AfterEach의 listingRepository.deleteAllInBatch()는 커밋됩니다 — 이 PR에서 CategorySeederTest를 고친 것과 같은 종류의 함정입니다. 지금은 매물을 전제하는 비트랜잭션 클래스가 없어 안 터지지만, 다음 사람이 하나 추가하면 순서에 따라 깨집니다. 방금 만드신 "지운 뒤 되돌린다" 패턴을 여기도 적용하거나, 최소한 왜 복구가 필요 없는지 한 줄 남겨주세요.
barrier.await()에 타임아웃이 없습니다. 스레드 하나가 배리어 전에 죽으면 나머지 7개가 무기한 대기하고 invokeAll도 안 끝나 CI가 그대로 멈춥니다. await(5, TimeUnit.SECONDS) 권장합니다.
filterFingerprint를 또 null로 넘깁니다. CursorPayload가 "커서 재사용 시 필터가 달라졌으면 거부하기 위한 값"으로 정의한 필드인데, #16에 이어 두 번째로 비어서 나갑니다. 찜 목록은 필터가 없어 위험이 낮으니 이 PR에서 굳이 채울 필요는 없지만, 공용 계약 쪽을 정리하거나 deferred로 묶어주세요.
@Modifying(clearAutomatically = true)가 호출자의 영속성 컨텍스트를 통째로 비웁니다. 지금 add/remove는 이후에 엔티티를 쓰지 않아 무해합니다. 다만 나중에 이 서비스를 더 큰 트랜잭션 안에서 부르면 앞서 로딩한 엔티티가 조용히 detach됩니다. 네이티브 INSERT는 읽지 않으니 clearAutomatically가 꼭 필요한지도 한 번 보시면 좋겠습니다.
findByUserIdAndListing은 테스트에서만 쓰입니다. 프로덕션 미사용 리포지토리 메서드라, 테스트 전용이면 그렇게 적어두거나 테스트 헬퍼로 내리는 게 낫겠습니다.
본문의 "ApiContractTest +2건"은 실제로 3건입니다 (목록·등록·해제).
좋았던 부분
- 틀린 구현을 테스트로 잡고 그 경위를 본문과 주석에 남긴 것.
UnexpectedRollbackException의 원인을 "세션이 rollback-only로 표시돼 예외를 잡아도 커밋에서 터진다"까지 정확히 짚었습니다. 이런 건 다음 사람이 같은 함정을 다시 파는 걸 막습니다 - 판정과 변경을 한 구문으로 합쳐 예외 자체를 만들지 않은 해법. 해제도 단일 DELETE로 간 것
CLIENT_FOUND_ROWS때문에 반환값을 두지 않겠다고 명시한 것 — "구분되는 것처럼 보이는 값을 돌려주면 나중에 그걸 믿고 분기하는 코드가 생긴다"는 판단이 정확합니다- 인하 판정(
lowerBasePriceIfDropped)과 발송 기록(markNotified) 분리 — 발송 실패 시 알림이 영구 유실되는 걸 막는 설계입니다 ApiContractTest에/api/listings/{publicId}/favorite가 상세 공개 화이트리스트에 안 걸리는지 POST·DELETE 둘 다 고정한 것./api/listings/{publicId}가 세그먼트 하나만 매칭한다는 걸 정확히 이해하고 그 경계를 테스트로 박아둔 겁니다pagesAcrossTieAtBoundary— 동일created_at이 페이지 경계에 걸리는 케이스는 실제로 자주 나는데 보통 빠지는 테스트입니다- 팔린·삭제 매물이 목록에 남는 걸 조인 조건 회귀 테스트로 고정한 것
CategorySeederTest복구 — 범위 밖이지만 맞는 수정입니다. 이 PR에 두는 데 이견 없습니다
한 줄 요약: 동시성 처리는 맞게 갔고 테스트도 촘촘합니다. 정리할 건 ① 찜 거부 에러코드를 LST0xx로 분리(409를 고른 이유를 코드에 반영), ② 엔티티 of()와 프로덕션 INSERT 경로 일원화 + 알림용 미사용 코드 정리, ③ 해제→재찜 시 기준가 리셋이 정책인지 확정. 나머지는 사소합니다.
작업 내용
POST /api/listings/{publicId}/favorite(201)DELETE /api/listings/{publicId}/favorite(200)GET /api/users/me/favorites(커서 페이징)동시성 — 처음 구현이 틀렸고 테스트로 잡았습니다
처음에는 "있는지 조회 → 없으면 저장, UNIQUE 위반은 잡아서 성공 처리"로 짰습니다. 8스레드 테스트를 붙이니 그대로 깨졌습니다.
UnexpectedRollbackException: ... marked as rollback-only→ 500. Hibernate가 제약 위반을 변환하면서 세션을 rollback-only로 표시해, 예외를 잡고 성공을 리턴해도 커밋에서 터집니다.ObjectOptimisticLockingFailureException: expected row count 1 but was 0→ 409. 조회한 엔티티를 지우는 사이 다른 요청이 먼저 지웠습니다.둘 다 판정과 변경을 한 구문으로 합쳐 해결했습니다.
INSERT ... ON DUPLICATE KEY UPDATE user_id = user_id— 중복이어도 예외가 안 나고, 기준가를 덮어쓰지 않는 것이 핵심입니다. 덮어쓰면 판매자가 값을 올린 뒤 사용자가 하트를 다시 누르는 것만으로 기준가가 올라가 "최저가 대비 인하" 정책이 무너집니다.DELETE— 없으면 0행이고 그게 정상입니다.FavoriteConcurrencyTest는@Transactional을 붙이지 않았습니다. 별도 스레드는 테스트 트랜잭션 밖이라 롤백 안에 갇히면 경쟁 자체가 재현되지 않습니다.그 밖의 결정
lowerBasePriceIfDropped/markNotified). 판정과 함께last_notified_at을 미리 찍으면, 발송이 실패해도 기준가는 이미 내려가 있어 같은 가격으로는 두 번 다시 알림이 나가지 않습니다.insertIfAbsent는 반환값을 두지 않았습니다. MySQL Connector/J가CLIENT_FOUND_ROWS를 기본으로 켜서 중복이어도 영향 행 수가 1로 올라와, 신규/중복 구분에 쓸 수 없습니다.listing_favorites입니다. ERD는FAVORITES지만 정책서·요구사항서가 이 이름이고 ERD 주석에 통일 필요 표시가 있습니다.MutableEntity상속 — ERD엔created_at만 있으나notify_base_price가 실제로 갱신됩니다./api/listings와/api/users/me로 갈려 컨트롤러를 분리했습니다.범위 밖 수정 1건 (양해 부탁드립니다)
CategorySeederTest의@AfterEach가 카테고리를 지우고 복구하지 않아, 뒤에 도는 비트랜잭션 테스트 클래스가 빈 테이블을 보게 됩니다. 지금까지는 그런 클래스가 전부 앞 순서(repository패키지)라 우연히 안 터졌고, 이번에 추가한service.FavoriteConcurrencyTest가 뒤에 붙으면서 드러났습니다. 시더를 다시 돌리도록 한 줄 고쳤습니다. 이 PR에서 빼는 게 낫다면 말씀해 주세요.테스트
FavoriteConcurrencyTest3건: 8스레드 동시 찜/해제, 재찜 몰릴 때 기준가 유지ListingFavoriteRepositoryTest10건: UNIQUE, 멱등 INSERT, 커서 연속성, 동일 시각 페이지 경계, 사용자 격리, 판매완료·삭제 행 보존FavoriteCommandServiceTest7건,FavoriteQueryServiceTest6건,FavoriteControllerTest3건,ListingFavoriteTest4건,ApiContractTest+2건참고
ddl-auto로 생성됩니다(기존 매물 PR과 동일).ON DUPLICATE KEY UPDATE는 MySQL 전용 구문입니다. DB를 바꿀 계획은 없지만 기록해 둡니다.