Skip to content

[FEAT] 찜 등록·해제·내 찜 목록 API - #24

Open
RootToApex wants to merge 1 commit into
feat/listing-mutationfrom
feat/listing-favorite
Open

[FEAT] 찜 등록·해제·내 찜 목록 API#24
RootToApex wants to merge 1 commit into
feat/listing-mutationfrom
feat/listing-favorite

Conversation

@RootToApex

Copy link
Copy Markdown
Member

작업 내용

  • 찜 등록 POST /api/listings/{publicId}/favorite (201)
  • 찜 해제 DELETE /api/listings/{publicId}/favorite (200)
  • 내 찜 목록 GET /api/users/me/favorites (커서 페이징)

동시성 — 처음 구현이 틀렸고 테스트로 잡았습니다

처음에는 "있는지 조회 → 없으면 저장, UNIQUE 위반은 잡아서 성공 처리"로 짰습니다. 8스레드 테스트를 붙이니 그대로 깨졌습니다.

  • 동시 찜: UnexpectedRollbackException: ... marked as rollback-only500. Hibernate가 제약 위반을 변환하면서 세션을 rollback-only로 표시해, 예외를 잡고 성공을 리턴해도 커밋에서 터집니다.
  • 동시 해제: ObjectOptimisticLockingFailureException: expected row count 1 but was 0409. 조회한 엔티티를 지우는 사이 다른 요청이 먼저 지웠습니다.

둘 다 판정과 변경을 한 구문으로 합쳐 해결했습니다.

  • 등록: INSERT ... ON DUPLICATE KEY UPDATE user_id = user_id — 중복이어도 예외가 안 나고, 기준가를 덮어쓰지 않는 것이 핵심입니다. 덮어쓰면 판매자가 값을 올린 뒤 사용자가 하트를 다시 누르는 것만으로 기준가가 올라가 "최저가 대비 인하" 정책이 무너집니다.
  • 해제: 단일 DELETE — 없으면 0행이고 그게 정상입니다.

FavoriteConcurrencyTest@Transactional을 붙이지 않았습니다. 별도 스레드는 테스트 트랜잭션 밖이라 롤백 안에 갇히면 경쟁 자체가 재현되지 않습니다.

그 밖의 결정

  • 비공개 매물 찜은 404가 아니라 409입니다. 상세 조회는 팔린 매물도 200으로 응답하므로, 404를 주면 방금 화면에 띄운 매물이 없다는 뜻이 되어 클라이언트가 "이미 팔린 상품입니다" 안내를 만들 수 없습니다. 없는 publicId(진짜 404)와도 구분됩니다.
  • 찜한 매물이 팔리거나 삭제돼도 목록에서 빼지 않습니다(정책). 조인 조건에 상태 필터를 넣으면 조용히 깨지는 부분이라 통합 테스트로 고정했습니다.
  • 기준가 인하와 발송 기록을 분리했습니다(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에서 빼는 게 낫다면 말씀해 주세요.

테스트

  • 전체 150건 통과 (0 실패) — 찜 관련 33건
  • FavoriteConcurrencyTest 3건: 8스레드 동시 찜/해제, 재찜 몰릴 때 기준가 유지
  • ListingFavoriteRepositoryTest 10건: UNIQUE, 멱등 INSERT, 커서 연속성, 동일 시각 페이지 경계, 사용자 격리, 판매완료·삭제 행 보존
  • FavoriteCommandServiceTest 7건, FavoriteQueryServiceTest 6건, FavoriteControllerTest 3건, ListingFavoriteTest 4건, ApiContractTest +2건

참고

  • 스키마는 아직 Flyway 파일이 없어 ddl-auto로 생성됩니다(기존 매물 PR과 동일).
  • ON DUPLICATE KEY UPDATE는 MySQL 전용 구문입니다. DB를 바꿀 계획은 없지만 기록해 둡니다.
  • 가격 인하 알림 발송 자체는 알림 도메인 몫이라 이 PR에는 없습니다.

@RootToApex RootToApex added the enhancement New feature or request label Aug 31, 2026
@RootToApex RootToApex self-assigned this Aug 31, 2026
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 35733441-34a4-46c5-b0c7-080253ad8bba

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@CheatIsKey CheatIsKey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약

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건과 ListingFavoriteRepositoryTestfavorite() 헬퍼는 전부 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 UPDATEnotify_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 두 개는 그대로 들어갑니다. 어긋나는 건 목록 하나뿐입니다. 분리 자체는 괜찮은 판단이지만, 이유는 "찜은 별도 기능 단위"라고 쓰는 게 맞습니다.

FavoriteConcurrencyTestlistings를 비트랜잭션으로 전역 삭제합니다. @BeforeEach·@AfterEachlistingRepository.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 경로 일원화 + 알림용 미사용 코드 정리, ③ 해제→재찜 시 기준가 리셋이 정책인지 확정. 나머지는 사소합니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants