Merged
Conversation
Closed
📝 WalkthroughWalkthrough예약 도메인에 새로운 값 객체들(Money, ReservationNumber, ReservationTimeSlot, CancelReason, CancellationInfo), 열거형(CanceledBy, ReservationStatus), JPA 컨버터들 및 상태 머신을 포함한 JPA 엔티티 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Reservation as Reservation Entity
participant Database
Client->>Reservation: hold(modelId,..., holdExpiresAt)
activate Reservation
Reservation->>Reservation: validate inputs\ngenerate ReservationNumber\nset status = PENDING
deactivate Reservation
Reservation-->>Client: Reservation (PENDING)
Client->>Reservation: confirm(confirmedAt)
activate Reservation
Reservation->>Reservation: check status == PENDING\ncheck hold not expired\nset status = CONFIRMED\nset confirmedAt
deactivate Reservation
Reservation-->>Client: ✓ confirmed
alt Cancel Flow
Client->>Reservation: cancel(canceledBy, reason, canceledAt)
activate Reservation
Reservation->>Reservation: create CancellationInfo\nset status = CANCELED
deactivate Reservation
Reservation-->>Client: ✓ canceled
else Expire Flow
Client->>Reservation: expire(expiredAt)
activate Reservation
Reservation->>Reservation: check status == PENDING\nset status = EXPIRED
deactivate Reservation
Reservation-->>Client: ✓ expired
end
Reservation->>Database: persist state
Database-->>Reservation: saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60분 🚥 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)
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 |
Test Results (reservation)337 tests 337 ✅ 7s ⏱️ Results for commit d851334. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
MSA 전환 과정에서 결제/사가 연동 전에 **예약 코어 도메인(선점/확정/취소/만료)**의 상태 전이와 불변식을 엔티티/VO로 먼저 고정하기 위한 PR입니다.
Changes:
ReservationAggregate Root 및 상태 전이(hold/confirm/cancel/expire) 도메인 로직 추가- 예약 관련 VO/Enum(
ReservationNumber,ReservationTimeSlot,Money,CancelReason,CancellationInfo,ReservationStatus,CanceledBy) 추가 - VO-DB 매핑을 위한 JPA Converter 및 단위 테스트/Fixture 추가
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java | 예약 Aggregate Root, 상태 전이/검증 로직 및 JPA 매핑 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/ReservationStatus.java | 예약 상태 Enum 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/CanceledBy.java | 취소 주체 Enum 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumber.java | 외부 노출 예약번호(VO) 및 UUID 검증 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationTimeSlot.java | 예약 시간대(VO, Embeddable) 및 불변식 검증 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/Money.java | 금액(VO) 및 연산/검증 로직 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancelReason.java | 취소 사유(VO) 및 길이/empty 판정 로직 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancellationInfo.java | 취소 정보(VO, Embeddable) 및 검증 로직 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/ReservationNumberConverter.java | ReservationNumber ↔ String 변환 Converter 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/MoneyConverter.java | Money ↔ Long 변환 Converter 추가 |
| reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/CancelReasonConverter.java | CancelReason ↔ String 변환 Converter 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/ReservationTest.java | Reservation 상태 전이/예외/멱등성 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/fixture/ReservationFixture.java | Reservation 테스트 Fixture 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumberTest.java | ReservationNumber VO 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationTimeSlotTest.java | ReservationTimeSlot VO 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/MoneyTest.java | Money VO 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancelReasonTest.java | CancelReason VO 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancellationInfoTest.java | CancellationInfo VO 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/ReservationNumberConverterTest.java | ReservationNumberConverter 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/MoneyConverterTest.java | MoneyConverter 단위 테스트 추가 |
| reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/CancelReasonConverterTest.java | CancelReasonConverter 단위 테스트 추가 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
.../net/catsnap/CatsnapReservation/reservation/infrastructure/converter/MoneyConverterTest.java
Outdated
Show resolved
Hide resolved
reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/MoneyTest.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In
`@reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java`:
- Around line 314-326: The two ownership-check methods (isOwnedByModel and
isOwnedByPhotographer) lack explicit null-argument validation; update both
methods to explicitly fail fast when the incoming ID is null (e.g., call
Objects.requireNonNull(modelId, "modelId must not be null") in isOwnedByModel
and Objects.requireNonNull(photographerId, "photographerId must not be null") in
isOwnedByPhotographer) before performing the existing equality check so a caller
bug is surfaced as an exception rather than silently returning false.
- Around line 353-368: Current equals/hashCode in Reservation uses only id,
causing two new (id==null) instances to be equal; update equals and hashCode in
the Reservation class so that if id is non-null you use id for identity,
otherwise fall back to the business natural key reservationNumber (UUID). Modify
the equals(Object) method and hashCode() accordingly: in equals check id
equality when both non-null, otherwise compare reservationNumber; in hashCode
include reservationNumber when id is null (or combine both deterministically).
This fixes incorrect equality for transient entities while preserving id-based
identity after persistence.
- Around line 143-163: The Reservation constructor currently validates IDs and
holdExpiresAt but misses null checks for timeSlot and amount; add fail-fast
validation for these fields (e.g., call a new private method like
validateTimeSlotAndAmount(timeSlot, amount) or inline checks) inside the private
Reservation(...) before assigning fields, and throw an appropriate runtime
exception (IllegalArgumentException or NullPointerException) with a clear
message if timeSlot or amount is null so the domain invariant is enforced in the
Reservation class.
In
`@reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancellationInfo.java`:
- Line 14: Remove domain-to-infrastructure imports and explicit `@Convert` usages
for converters that are declared with `@Converter`(autoApply = true);
specifically, in classes like CancellationInfo and Reservation remove imports of
CancelReasonConverter, MoneyConverter, ReservationNumberConverter and delete
their `@Convert`(converter = ...) annotations so JPA will apply the autoApply
converters automatically; keep explicit `@Convert` references only for converters
that are NOT autoApply (e.g., AvailableStartTimesConverter,
WeekdayRulesConverter).
In
`@reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumber.java`:
- Around line 32-37: The current ReservationNumber value validation uses
UUID.fromString(value) which can accept non-standard forms; update
ReservationNumber to validate against a strict UUID regex (e.g., add a
UUID_PATTERN constant) in the constructor/validation logic instead of relying
solely on UUID.fromString, and ensure generate() produced values still pass that
pattern; replace the try/catch block around UUID.fromString(value) in
ReservationNumber with a pattern match and throw
DomainException(DomainErrorCode.DOMAIN_CONSTRAINT_VIOLATION, "예약 번호는 UUID 형식이어야
합니다. 현재: " + value) when the regex does not match.
In
`@reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/infrastructure/converter/CancelReasonConverter.java`:
- Around line 13-21: The converter currently returns null for a null DB value
causing CancelReason(null) → DB null → null on read; update
CancelReasonConverter.convertToEntityAttribute to return a new
CancelReason(null) when dbData is null (keeping convertToDatabaseColumn as-is),
so round‑trip preserves the VO; reference the
CancelReasonConverter.convertToEntityAttribute method and the
CancelReason(String) constructor/getValue to locate where to change the mapping.
In
`@reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/ReservationTest.java`:
- Around line 29-84: Add two missing null-validation tests for timeSlot and
amount in the ReservationTest Hold nested class: add one test that calls
Reservation.hold(..., null, DEFAULT_AMOUNT, ...) and asserts a DomainException
is thrown (message optional), and another that calls Reservation.hold(...,
DEFAULT_TIME_SLOT, null, ...) asserting DomainException; locate tests alongside
existing null_* tests in the Hold class that reference Reservation.hold and use
the same DEFAULT_* fixtures so they mirror null_모델ID... tests and validate
Reservation.hold's null checks for ReservationTimeSlot and Money.
- Around line 293-325: Add a boundary test to ReservationTest that asserts
isPayableAt returns false when called exactly at the hold expiry instant: create
a Reservation via ReservationFixture.createDefault() and assert
reservation.isPayableAt(DEFAULT_HOLD_EXPIRES_AT) isFalse(); place the test
alongside other IsPayableAt `@Test` methods so it verifies the exact-equality edge
case for isPayableAt against DEFAULT_HOLD_EXPIRES_AT.
In
`@reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/MoneyTest.java`:
- Around line 64-75: Add a unit test that verifies Money.add throws a
DomainException on overflow: create Money money1 = new Money(Long.MAX_VALUE) and
Money money2 = new Money(1L) and assert that calling money1.add(money2) results
in a DomainException; name the test something like add_오버플로우_시_예외가_발생한다 and rely
on Money.add and the constructor/validate() behavior to trigger the exception.
In
`@reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumberTest.java`:
- Around line 30-38: The test in ReservationNumberTest::generate로_새로운_예약번호를_생성한다
currently calls UUID.fromString(reservationNumber.getValue()) which will throw
an IllegalArgumentException on failure and produce a less clear failure message;
replace that raw call with an AssertJ assertion such as using assertThatCode(()
-> UUID.fromString(reservationNumber.getValue())).doesNotThrowAnyException() so
the assertion failure message is clearer and points to the UUID parsing; update
the assertion in the test after calling ReservationNumber.generate() and keep
the existing assertThat(reservationNumber.getValue()).isNotBlank() check.
In
`@reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/fixture/ReservationFixture.java`:
- Line 15: ReservationFixture is a pure utility class and should prevent
instantiation; add a private no-arg constructor (e.g., private
ReservationFixture() { throw new AssertionError("No instances."); }) inside the
ReservationFixture class to stop accidental construction, keeping existing
static methods and constants unchanged and optionally include an AssertionError
or comment for clarity.
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
...ion/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/CancellationInfo.java
Show resolved
Hide resolved
...on/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumber.java
Show resolved
Hide resolved
...rvation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/ReservationTest.java
Show resolved
Hide resolved
...rvation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/ReservationTest.java
Show resolved
Hide resolved
reservation/src/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/MoneyTest.java
Show resolved
Hide resolved
...rc/test/java/net/catsnap/CatsnapReservation/reservation/domain/vo/ReservationNumberTest.java
Show resolved
Hide resolved
...ion/src/test/java/net/catsnap/CatsnapReservation/reservation/fixture/ReservationFixture.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java`:
- Around line 353-363: The custom null-check method validateHoldExpiresAt
duplicates validateNotNull; replace calls to
validateHoldExpiresAt(holdExpiresAt) in the constructor (or wherever it's used)
with validateNotNull(holdExpiresAt, "홀드 만료 시각은 필수입니다.") and then remove the
redundant validateHoldExpiresAt method; keep validatePositive and
validateNotNull as the canonical validators and ensure any logic previously in
validateHoldExpiresAt is fully migrated before deletion.
- Around line 24-25: The Reservation domain currently imports MoneyConverter and
ReservationNumberConverter (and likely uses `@Convert` on fields) which makes the
domain depend on the infrastructure layer; instead annotate each converter class
(MoneyConverter and ReservationNumberConverter) with `@Converter`(autoApply =
true) in the infrastructure package so JPA will apply them automatically, then
remove the imports and any `@Convert`(...) annotations from the Reservation
aggregate root to eliminate the infra dependency from the domain.
- Around line 237-253: In cancel(), move null validation for canceledBy and
canceledAt to the start of the method (before any status logic) and throw the
same DomainException/DomainErrorCode used elsewhere when either is null so
callers fail fast instead of relying on CancellationInfo's constructor; keep the
existing status checks but add a brief comment that the final "if (status !=
PENDING && status != CONFIRMED)" is a defensive/forward-compatibility guard in
case new ReservationStatus values are added (so the unreachable-branch intent is
explicit).
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
reservation/src/main/java/net/catsnap/CatsnapReservation/reservation/domain/Reservation.java
Show resolved
Hide resolved
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.
📌 관련 이슈
✨ PR 세부 내용
배경
변경 내용
1) Reservation Aggregate Root 추가
Reservation엔티티 신규 추가PENDING,CONFIRMED,CANCELED,EXPIREDhold(...): 임시 예약 생성confirm(...): 결제 성공 후 확정cancel(...): 모델/작가/시스템 취소expire(...): 홀드 만료 처리isPayableAt(...),isHoldExpiredAt(...): 결제/만료 판단@Version기반 낙관적 락 적용으로 동시성 제어 기반 마련2) 예약 도메인 Value Object/Enum 추가
ReservationStatus,CanceledByReservationNumber,ReservationTimeSlot,Money,CancelReason,CancellationInfo3) JPA Converter 추가
ReservationNumberConverterMoneyConverterCancelReasonConverter4) 테스트 및 Fixture 추가
ReservationTest로 상태 전이/멱등성/예외 케이스 검증ReservationTimeSlot,Money,ReservationNumber,CancelReason,CancellationInfo)ReservationFixture추가로 테스트 데이터 생성 표준화변경 파일 요약
범위 외 (이번 PR 미포함)
Summary by CodeRabbit
새로운 기능
테스트