[FEATURE] Shipment 배송 이벤트 발행/구독 기능 구현 및 배송담당자 삭제 내부 API 구현 - #47
Merged
Merged
Conversation
- ShipmentRabbitConfig: order.created 큐 및 DLQ 설정 - OrderCreatedEvent, ShipmentCreatedEvent, ShipmentCreationFailedEvent 정의 - ProcessedSagaEventJpaEntity, Repository: DB 기반 멱등성 저장 - IdempotentSagaExecutor, IdempotentSagaHandler: Redis+DB 이중 멱등성 처리 - ProcessedSagaEventCleanupScheduler: 24시간 경과 이벤트 주기적 정리
- HubClient 인터페이스 및 HubFeignClient/HubClientImpl 구현 - CacheClient 인터페이스 및 RedisClient 구현 - @EnableFeignClients, @EnableScheduling 활성화 - ShipmentManagerRepository: findFirstAvailableByType, findAllByType 추가
- UserClient로 수신자 조회, Redis 커서 기반 허브 담당자 순번 배정 - 이벤트 수신 후 createShipment 호출
- domain/event에 ShipmentCreatedEvent, ShipmentCreationFailedEvent 레코드 정의 - ShipmentEventPublisher 인터페이스를 application 레이어에 추가 - ShipmentEventPublisherImpl: domain 레코드를 SagaEvent로 변환 후 발행
- POST /api/shipments/{shipmentId}/complete api 추가
- ShipmentCompletedEvent 발행
- OrderCanceledEvent 구독
- Hub 삭제 시 업체 배송 담당자 일괄삭제 - User 삭제시 배송 담당자 삭제 - 매일 자정에 일괄 삭제 처리
- findFirstAvailableByType, findAllByType 쿼리에 pendingDeletion = false 조건 추가
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough주요 변경사항은 shipment-service에 배송 생성/완료/실패 이벤트 발행 및 주문 생성/취소 이벤트 구독을 포함한 대규모 기능 추가와 연동입니다. Hub 경로 조회(Feign), Redis 기반 캐시/커서(CacheClient/RedisClient), 배송담당자 할당 로직, Shipment 도메인 상태 전이(markCompleted/markCanceled), idempotent 처리(ProcessedSagaEventJpaEntity, IdempotentSagaExecutor, IdempotentSagaHandler), 스케줄러(ProcessedSagaEventCleanupScheduler, ShipmentManagerCleanupScheduler), 내부 삭제 API(/internal/shipment-managers/hubs/{hubId}, /internal/shipment-managers/users/{userId}), 컨트롤러의 UserContext 인자 해석기 및 관련 DTO/리포지터리/테스트가 추가되었습니다. Sequence Diagram(s)sequenceDiagram
participant OrderService as Order Service
participant RabbitMQ as RabbitMQ
participant OrderCreatedHandler as OrderCreatedHandler
participant IdempotentExecutor as IdempotentSagaExecutor
participant ShipmentService as Shipment Service
participant HubFeign as HubFeignClient
participant Cache as CacheClient/RedisClient
participant EventPub as ShipmentEventPublisher
participant DB as ProcessedSagaEventRepository
OrderService->>RabbitMQ: Publish order.created
RabbitMQ->>OrderCreatedHandler: Deliver OrderCreatedEvent
OrderCreatedHandler->>IdempotentExecutor: handle(event)
IdempotentExecutor->>Cache: hasProcessed(redisKey)
alt not processed
IdempotentExecutor->>ShipmentService: createShipment(command)
ShipmentService->>HubFeign: getHubRoutes(depId, arrId)
HubFeign-->>ShipmentService: List<HubRouteResult>
ShipmentService->>Cache: increment(HUB_MANAGER_CURSOR_KEY)
Cache-->>ShipmentService: cursorIndex
ShipmentService->>DB: persist Shipment (with routes)
ShipmentService->>EventPub: publishCreated(ShipmentCreatedEvent)
EventPub->>RabbitMQ: Publish shipment.created
IdempotentExecutor->>DB: save ProcessedSagaEventJpaEntity
IdempotentExecutor->>Cache: set(redisKey, 24h)
else already processed
IdempotentExecutor-->>OrderCreatedHandler: skip processing
end
sequenceDiagram
participant Client as Client
participant Controller as ShipmentController
participant ShipmentService as Shipment Service
participant Repo as ShipmentRepository/ShipmentJpaRepository
participant EventPub as ShipmentEventPublisher
participant RabbitMQ as RabbitMQ
Client->>Controller: POST /api/shipments/{id}/complete
Controller->>ShipmentService: completeShipment(shipmentId)
ShipmentService->>Repo: findByOrderIdWithRoutes(orderId)
Repo-->>ShipmentService: Shipment + routes
ShipmentService->>ShipmentService: validate routes all ARRIVED_AT_HUB
ShipmentService->>ShipmentService: markCompleted()
ShipmentService->>Repo: save(shipment)
ShipmentService->>EventPub: publishCompleted(ShipmentCompletedEvent)
EventPub->>RabbitMQ: Publish shipment.completed
ShipmentService-->>Controller: ShipmentCompleteResult
Controller-->>Client: ShipmentCompleteResDto
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
soo96
requested review from
250ghghghgh,
Jin4041,
afterrest,
kim-jun-won and
zlonce
April 7, 2026 01:52
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (8)
shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/command/CreateShipmentCommand.java (1)
6-16: 커맨드 레코드에 최소 불변조건 검증을 추가하는 것을 권장합니다.이 레코드가 외부 이벤트 입력을 받는 경로라면
quantity > 0및 필수 UUID/null 체크를 compact constructor에서 선제 검증하면 실패 지점이 명확해집니다.예시 diff
public record CreateShipmentCommand( UUID orderId, UUID ordererId, UUID productId, int quantity, UUID departureHubId, UUID arrivalHubId, LocalDateTime requestDeadline, String requestNote, String shipmentAddress ) { + public CreateShipmentCommand { + if (orderId == null || ordererId == null || productId == null + || departureHubId == null || arrivalHubId == null) { + throw new IllegalArgumentException("필수 식별자는 null일 수 없습니다."); + } + if (quantity <= 0) { + throw new IllegalArgumentException("quantity는 1 이상이어야 합니다."); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/command/CreateShipmentCommand.java` around lines 6 - 16, Add defensive validation to the CreateShipmentCommand record by implementing a compact constructor that checks non-null for all UUID/string/LocalDateTime fields (orderId, ordererId, productId, departureHubId, arrivalHubId, requestDeadline, shipmentAddress) and ensures quantity > 0; throw IllegalArgumentException or NullPointerException with clear messages when validation fails so creation fails fast and gives a clear error location.shipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentRoute.java (1)
121-123: 상태 전환 검증 누락 고려
markCanceled()는 현재 상태 검증 없이 바로CANCELED로 변경합니다. 다른 상태 전환 메서드들(markMovingToHub,markArrivedAtHub)은 유효한 상태 전환인지 검증하는 패턴을 따르고 있습니다.이미
CANCELED또는ARRIVED_AT_HUB상태인 경로를 다시 취소하는 것이 비즈니스적으로 허용되는지 확인이 필요합니다. 의도적인 설계라면 무시해도 됩니다.♻️ 상태 검증 추가 제안
public void markCanceled() { + if (this.status == ShipmentRouteStatus.CANCELED) { + return; // 이미 취소됨 - 멱등성 보장 + } + if (this.status == ShipmentRouteStatus.ARRIVED_AT_HUB) { + throw new BusinessException(ShipmentErrorCode.INVALID_SHIPMENT_ROUTE_STATUS); + } this.status = ShipmentRouteStatus.CANCELED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentRoute.java` around lines 121 - 123, The markCanceled() method currently sets status unconditionally; follow the same state-transition validation pattern as markMovingToHub/markArrivedAtHub by checking the current status (ShipmentRoute#status) before assigning ShipmentRouteStatus.CANCELED and throw an IllegalStateException for invalid transitions (e.g., when status is ARRIVED_AT_HUB or already CANCELED) or make it idempotent by returning early if status == CANCELED—update the markCanceled() method accordingly to perform this validation instead of unconditional assignment.shipment-service/src/main/java/com/shipflow/shipmentservice/application/client/dto/HubRouteResult.java (1)
9-17: 테스트 편의성을 위해@AllArgsConstructor추가 고려AI 요약에 따르면 테스트에서 리플렉션을 통해 인스턴스를 생성하고 있습니다.
@AllArgsConstructor를 추가하면 테스트 코드가 간결해지고, Jackson 역직렬화도 명시적으로 지원됩니다.♻️ 제안하는 수정
import lombok.Getter; import lombok.NoArgsConstructor; +import lombok.AllArgsConstructor; `@Getter` `@NoArgsConstructor` +@AllArgsConstructor public class HubRouteResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/application/client/dto/HubRouteResult.java` around lines 9 - 17, Add a Lombok all-args constructor to the HubRouteResult DTO by annotating the class with `@AllArgsConstructor` and importing lombok.AllArgsConstructor so tests and Jackson can construct instances without reflection; update the class declaration that currently has `@Getter` and `@NoArgsConstructor` to include `@AllArgsConstructor` (retain existing annotations and fields: sequence, departureHubId, arrivalHubId, estimatedDistance, estimatedDuration).shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCanceledResult.java (1)
21-28:canceledAt매핑 의미 확인 필요
canceledAt에shipment.getUpdatedAt()을 사용하고 있습니다. 취소 시점의updatedAt이 취소 시간을 나타내는 것이 맞다면 괜찮지만,Shipment도메인에 명시적인canceledAt필드가 있다면 해당 필드를 사용하는 것이 더 명확합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCanceledResult.java` around lines 21 - 28, The mapping in ShipmentCanceledResult.fromEntity uses shipment.getUpdatedAt() for canceledAt which may be incorrect; check the Shipment domain for an explicit canceledAt accessor (e.g., getCanceledAt()) and if it exists, change ShipmentCanceledResult.fromEntity to call shipment.getCanceledAt() instead of shipment.getUpdatedAt(); if Shipment lacks a canceledAt field, either add a proper canceledAt property to the Shipment aggregate and expose getCanceledAt(), or add a clear comment/rename to indicate that updatedAt is intentionally used as the cancellation timestamp so the mapping is explicit and correct.shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ProcessedSagaEventRepository.java (1)
7-9: 대량 삭제 시 성능 이슈 가능성Spring Data JPA의 파생 쿼리
deleteBy*메서드는 먼저 엔티티를 조회한 후 개별적으로 삭제합니다. 정리 스케줄러에서 많은 레코드를 삭제할 경우 N+1 문제가 발생할 수 있습니다.♻️ 벌크 삭제를 위한 JPQL 쿼리 사용 제안
+import org.springframework.data.jpa.repository.Modifying; +import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param; + public interface ProcessedSagaEventRepository extends JpaRepository<ProcessedSagaEventJpaEntity, String> { - void deleteByProcessedAtBefore(LocalDateTime cutoff); + `@Modifying` + `@Query`("DELETE FROM ProcessedSagaEventJpaEntity e WHERE e.processedAt < :cutoff") + int deleteByProcessedAtBefore(`@Param`("cutoff") LocalDateTime cutoff); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ProcessedSagaEventRepository.java` around lines 7 - 9, The derived deleteByProcessedAtBefore method causes N+1 deletes because Spring Data loads entities before removing; replace it with a bulk JPQL delete using a `@Modifying` `@Query` on the ProcessedSagaEventRepository (e.g., a method like deleteBefore or deleteByProcessedAtBefore using "delete from ProcessedSagaEventJpaEntity e where e.processedAt < :cutoff") and ensure the method is executed in a `@Transactional` context and marked `@Modifying` (consider setting clearAutomatically=true) so deletion is performed in one SQL bulk operation rather than row-by-row.shipment-service/src/test/java/com/shipflow/shipmentservice/application/ShipmentServiceTest.java (2)
72-80:HubRouteResult생성에 Reflection 사용은 적절하지만, Builder 또는 테스트 픽스처 고려
ReflectionTestUtils는 DTO에 setter가 없을 때 유용하지만,HubRouteResult에 테스트용 생성자나 Builder를 추가하거나 별도의 테스트 픽스처 클래스를 만드는 것이 유지보수에 더 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/test/java/com/shipflow/shipmentservice/application/ShipmentServiceTest.java` around lines 72 - 80, The test currently uses ReflectionTestUtils inside createHubRouteResult to set HubRouteResult internals; instead add a test-friendly way to construct HubRouteResult (either a package-private constructor, a static test Builder on HubRouteResult, or a separate test fixture/factory) and update createHubRouteResult to call that constructor/builder/factory instead of using ReflectionTestUtils.setField for sequence, departureHubId, arrivalHubId, estimatedDistance and estimatedDuration; ensure the new construction is accessible to ShipmentServiceTest and remove all ReflectionTestUtils usage for HubRouteResult.
120-123:createShipment_success테스트에서 더 상세한 검증 권장현재 테스트는 예외가 발생하지 않는 것과 이벤트 발행만 검증합니다.
ArgumentCaptor를 사용하여 저장된Shipment의 필드 값이나 발행된 이벤트의 내용을 검증하면 테스트 신뢰도가 높아집니다.💡 예시
// given ArgumentCaptor<Shipment> shipmentCaptor = ArgumentCaptor.forClass(Shipment.class); // when shipmentService.createShipment(command); // then then(shipmentRepository).should().save(shipmentCaptor.capture()); Shipment captured = shipmentCaptor.getValue(); assertThat(captured.getOrderId()).isEqualTo(orderId); assertThat(captured.getDepartureHubId()).isEqualTo(departureHubId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/test/java/com/shipflow/shipmentservice/application/ShipmentServiceTest.java` around lines 120 - 123, Enhance the createShipment_success test to capture and assert the actual Shipment saved and the published event contents: use ArgumentCaptor.forClass(Shipment.class) to capture the argument passed to shipmentRepository.save (and/or capture the event via eventPublisher.publishCreated), call shipmentService.createShipment(command) as before, then retrieve the captured Shipment and assert key fields like getOrderId(), getDepartureHubId(), and any status/metadata; also assert the published event's payload fields to match expected values so the test verifies state and event content, not just no-exception and publish invocation.shipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentManagerController.java (1)
37-44:userContext파라미터가 현재 사용되지 않습니다.
createShipmentManager에서userContext가 주입되지만 사용되지 않습니다. TODO 주석이나 권한 처리 계획이 있다면 주석으로 명시하는 것이 좋습니다. 동일한 패턴이getShipmentManager,searchShipmentManager에도 적용됩니다.API Gateway 도입 계획에 따른 의도적인 구조임을 이해하지만, 향후 구현 예정임을 나타내는 TODO 주석 추가를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentManagerController.java` around lines 37 - 44, The UserContext parameter is injected into controller methods but unused; add a clear TODO comment in ShipmentManagerController on the methods createShipmentManager, getShipmentManager, and searchShipmentManager stating that UserContext is intentionally kept for future API Gateway / authorization integration and will be used for permission checks and user info propagation, so retain the parameter for now; ensure the comment briefly indicates expected future behavior (e.g., "TODO: use UserContext for auth/claims from API Gateway") and keep the method signatures unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shipment-service/build.gradle`:
- Around line 51-52: Remove the duplicate Feign dependency declaration: delete
the second implementation entry for
'org.springframework.cloud:spring-cloud-starter-openfeign' in build.gradle so
only the single declaration remains (the earlier one on Line 33). Ensure no
other identical implementation lines exist to avoid redundant dependency
declarations.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCompleteResult.java`:
- Line 26: The DTO mapping uses shipment.getUpdatedAt() for completedAt, which
can be overwritten by subsequent updates; update the domain to expose a
dedicated completion timestamp or change the mapping to use that dedicated field
instead of getUpdatedAt() — add a completedAt field on the Shipment entity (and
set it when status transitions in updateStatus()), expose a getter (e.g.,
getCompletedAt()), and update ShipmentCompleteResult.completedAt(...) to use
shipment.getCompletedAt(); apply the same fix for
ShipmentCanceledResult.canceledAt using a dedicated canceledAt field and getter.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/ShipmentManagerService.java`:
- Around line 120-123: markPendingDeletionByUserId currently throws
BusinessException(SHIPMENT_MANAGER_NOT_FOUND) when
shipmentManagerRepository.findByUserId(userId) is empty, making the operation
non-idempotent; change it to treat "not found" as a no-op success (e.g., return
immediately or log and return) instead of throwing, while preserving the
existing behavior of calling manager.markPendingDeletion() when a
ShipmentManager is present; update the code referencing
shipmentManagerRepository.findByUserId, ShipmentManager.markPendingDeletion, and
avoid throwing BusinessException with SHIPMENT_MANAGER_NOT_FOUND for missing
users.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/ShipmentService.java`:
- Around line 56-104: The createShipment method currently publishes domain
events inside the `@Transactional` boundary causing possible DB/event
inconsistency; change createShipment to only persist and then publish an
internal Spring event (e.g., create and publish ShipmentCreatedInternalEvent and
ShipmentCreationFailedInternalEvent) via ApplicationEventPublisher from within
ShipmentService (replace eventPublisher.publishCreated(...) and
publishCreationFailed(...) calls with
applicationEventPublisher.publishEvent(...)), and implement a new
ShipmentEventListener with methods annotated `@TransactionalEventListener`(phase =
TransactionPhase.AFTER_COMMIT) that receive ShipmentCreatedInternalEvent and
ShipmentCreationFailedInternalEvent and call the external
eventPublisher.publishCreated(...) / publishCreationFailed(...); keep existing
symbols ShipmentService, eventPublisher,
eventPublisher.publishCreated/publishCreationFailed, and add
ShipmentCreatedInternalEvent/ShipmentCreationFailedInternalEvent and
ShipmentEventListener to ensure events are only sent after successful commit.
- Around line 155-168: completeShipment currently publishes the
ShipmentCompletedEvent inside the `@Transactional` method (via
eventPublisher.publishCompleted after shipment.markCompleted), which risks
transactional-event inconsistency if the transaction later fails; change the
flow so the domain change (shipment.markCompleted) is committed within the
transaction and the event is emitted only after commit—either by removing
publishCompleted from the `@Transactional` completeShipment and calling it from a
non-transactional caller, or by using a TransactionalEventListener or an outbox
pattern to defer publishing until after commit (refer to completeShipment,
shipment.markCompleted, eventPublisher.publishCompleted and the `@Transactional`
annotation to locate the code).
- Around line 99-103: The catch block in ShipmentService currently always calls
eventPublisher.publishCreationFailed and may mask the original exception if
publishing fails or incorrectly signal failure when shipmentRepository.save
already committed; modify the method so you track whether
shipmentRepository.save(...) completed successfully (e.g., a boolean persisted
or by checking the returned entity) and only call
eventPublisher.publishCreationFailed(command.orderId()) when the save did NOT
complete, and wrap the publishCreationFailed call in its own try/catch that logs
any publish error but rethrows the original exception (do not replace the
original exception if publish fails); reference shipmentRepository.save and
eventPublisher.publishCreationFailed in ShipmentService's catch handling and
ensure the original exception is always the one propagated.
- Around line 193-211: The startIndex calculation can overflow when cursor is
near Long.MAX_VALUE because it does (cursor - 1) before casting; instead take
cursor modulo the hubManagers size first. In buildRoutes, get int n =
hubManagers.size(), compute an int cursorMod = (int)(cursor % n) and then derive
startIndex safely (e.g. (cursorMod + n - 1) % n) so the subtraction never
happens on the full long; keep the rest of the mapping logic (managerIndex
calculation using route.getSequence()) unchanged.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/domain/Shipment.java`:
- Around line 118-154: Public updateStatus(ShipmentStatus) allows bypassing
transition rules; prevent this by making updateStatus private (or
package-private) and ensure all external status changes go through
markCompleted()/markCanceled() which call
validateCompletable()/validateCancelable() and then set the status;
alternatively, if updateStatus must stay public, have it enforce the same
validation logic (call validateCompletable() when setting COMPLETED and
validateCancelable() when setting CANCELLED, and throw BusinessException for
illegal transitions) so the aggregate invariants in
markCompleted()/markCanceled() cannot be bypassed.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubClientImpl.java`:
- Around line 28-33: In HubClientImpl (around the hubFeignClient.getHubRoutes
call) add null checks for the ApiResponse itself before accessing getData(): if
response == null or response.getData() == null or empty, throw the domain
BusinessException using ShipmentErrorCode.HUB_ROUTE_NOT_FOUND so external null
responses are converted to the same domain error; update the logic around
ApiResponse<List<HubRouteResult>> response =
hubFeignClient.getHubRoutes(departureHubId, arrivalHubId) to explicitly handle a
null response and then the existing routes null/empty case.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubFeignClient.java`:
- Around line 17-18: The Feign client method in HubFeignClient uses implicit
parameter names for `@RequestParam` (departureHubId, arrivalHubId) which fails at
runtime on Spring Boot 3.5 + Java 21; fix it by either adding explicit names to
the annotations (e.g., change `@RequestParam` UUID departureHubId to
`@RequestParam`("departureHubId") UUID departureHubId and similarly for
arrivalHubId) or enable the compiler -parameters flag in your build (add the
JavaCompile tasks.withType configuration to build.gradle) so parameter name
metadata is retained.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/RedisClient.java`:
- Around line 19-22: The increment method in RedisClient currently masks a null
result from redisTemplate.opsForValue().increment(key) by returning 0L, which
can produce a negative/incorrect round-robin cursor in callers; instead detect a
null response and fail fast: change RedisClient.increment to throw a clear
unchecked exception (e.g., IllegalStateException) including the key and a short
context message when value == null so Redis connection/operation failures
surface immediately to callers performing (cursor - 1) % hubManagers.size();
keep the successful path returning the non-null Long value.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/consume/OrderCanceledEvent.java`:
- Around line 12-18: OrderCanceledEvent's orderId remains null after Jackson
deserialization because the class only has `@NoArgsConstructor` and `@Getter`;
update OrderCanceledEvent to allow Jackson to set orderId by either adding an
explicit constructor public OrderCanceledEvent(UUID orderId) that assigns
this.orderId or by annotating the class with `@Setter` (or adding a setter for
orderId), so that OrderCanceledHandler's event.getOrderId() returns the expected
value.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/handler/IdempotentSagaExecutor.java`:
- Line 22: 현재 IdempotentSagaExecutor의 REDIS_TTL(현재 Duration.ofHours(24))이
processed-event 정리 작업과 동일한 24시간으로 설정되어 있어, 브로커의 최대 재시도/재전달 창보다 짧을 경우 지연 재전달된 동일
eventId가 다시 처리될 수 있습니다; 변경 방법은 REDIS_TTL 상수를 하드코딩된 24시간에서 분리하고(예: 새 구성 키 또는
프로퍼티로) broker의 최대 재시도 창보다 길게 기본값을 설정하거나 완전히 구성 가능하게 만들어야 합니다 — 즉 클래스
IdempotentSagaExecutor에서 REDIS_TTL을 구성값으로 치환하고 processed-event 정리(job) 타임아웃과는
별도의 설정(예: idempotency.ttl)으로 관리하도록 수정하세요.
- Around line 35-48: The idempotency marker is written after doProcess in
executeWithIdempotency, allowing concurrent deliveries to both run side effects;
change the flow to atomically acquire a pre-check/lock before invoking doProcess
(e.g., attempt a Redis SETNX / cacheClient.setIfAbsent(redisKey, "1", REDIS_TTL)
or perform a unique insert into processedSagaEventRepository to reserve the
event) and return early if the acquire fails; if using DB unique insert, catch
the duplicate-key exception and skip processing, and still register
TransactionSynchronizationManager.registerSynchronization to set the cache
afterCommit and persist ProcessedSagaEventJpaEntity only once (or rely on the
successful unique insert as the persistent marker).
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/scheduler/ProcessedSagaEventCleanupScheduler.java`:
- Around line 24-25: 현재 ProcessedSagaEventCleanupScheduler에서 LocalDateTime
cutoff = LocalDateTime.now().minusHours(24)로 TTL이 하드코딩되어 있어 재전달 지연이 24시간을 초과할 경우
중복 처리 위험이 있습니다; 변경 방법은 ProcessedSagaEventCleanupScheduler에 환경설정으로부터 읽는 TTL 값을
주입(예: Duration 또는 long hours 프로퍼티)하고 LocalDateTime.now().minus(주입된 TTL)으로
cutoff를 계산한 뒤 processedSagaEventRepository.deleteByProcessedAtBefore(...)를 호출하도록
수정하세요 — 관련 심볼: ProcessedSagaEventCleanupScheduler, cutoff,
processedSagaEventRepository.deleteByProcessedAtBefore.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentJpaRepository.java`:
- Around line 31-37: The repository method findByOrderIdWithRoutes currently
returns Optional<Shipment> but orderId is not guaranteed unique, risking
NonUniqueResultException; fix by either (A) enforcing 1:1 semantics: add a
unique constraint on the orderId field in the Shipment entity (e.g.,
`@Column`(unique = true) and add a DB migration/constraint) and keep
findByOrderIdWithRoutes as Optional<Shipment>, or (B) supporting 1:many
semantics: change the repository method signature
findByOrderIdWithRoutes(`@Param`("orderId") UUID orderId) to return List<Shipment>
(and update all callers to handle a list) so multiple shipments for the same
order are handled safely.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentManagerJpaRepository.java`:
- Around line 38-46: The custom JPQL using "limit 1" is non-standard; remove the
`@Query` block and replace the repository method with a Spring Data derived query
to get the first matching row. Rename/replace findFirstAvailableByType(...) with
a derived signature such as Optional<ShipmentManager>
findFirstByTypeAndDeletedAtIsNullAndPendingDeletionFalseOrderByShipmentSequenceAsc(ShipmentManagerType
type) (remove `@Param`), which yields the same semantics in a portable way.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/presentation/UserContextArgumentResolver.java`:
- Line 43: Wrap the UUID parsing in UserContextArgumentResolver so that
UUID.fromString(userId) parsing failures are caught and converted into a
controlled 4xx error: catch IllegalArgumentException thrown by UUID.fromString
in the method that constructs the UserContext and throw a BusinessException (or
another existing client error type) with a clear message about invalid X-User-Id
format; alternatively, add an exception handler for IllegalArgumentException
that returns 400 Bad Request. Update the code paths that construct new
UserContext(UUID.fromString(userId), role) to use this guarded parsing to ensure
consistent 4xx responses.
In `@shipment-service/src/main/resources/application.yaml`:
- Line 27: Replace the hardcoded spring.rabbitmq.port value (currently set to
5672) in application.yaml with an environment-variable-backed placeholder so the
port can be overridden at deploy time; use the SPRING_RABBITMQ_PORT env var with
a default of 5672 (i.e., change the port entry for spring.rabbitmq.port to
reference SPRING_RABBITMQ_PORT with default 5672) to match other infra patterns
and enable flexibility across environments.
- Around line 28-29: Remove the fallback "guest" defaults so RabbitMQ
credentials must be injected: replace username: ${RABBITMQ_USERNAME:guest} and
password: ${RABBITMQ_PASSWORD:guest} with username: ${RABBITMQ_USERNAME} and
password: ${RABBITMQ_PASSWORD} in application.yaml to force runtime failure when
env vars are missing (or add explicit startup validation that
RABBITMQ_USERNAME/RABBITMQ_PASSWORD are present); update any startup/config
validation logic if needed to fail fast and document the required env vars.
---
Nitpick comments:
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/client/dto/HubRouteResult.java`:
- Around line 9-17: Add a Lombok all-args constructor to the HubRouteResult DTO
by annotating the class with `@AllArgsConstructor` and importing
lombok.AllArgsConstructor so tests and Jackson can construct instances without
reflection; update the class declaration that currently has `@Getter` and
`@NoArgsConstructor` to include `@AllArgsConstructor` (retain existing annotations
and fields: sequence, departureHubId, arrivalHubId, estimatedDistance,
estimatedDuration).
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/command/CreateShipmentCommand.java`:
- Around line 6-16: Add defensive validation to the CreateShipmentCommand record
by implementing a compact constructor that checks non-null for all
UUID/string/LocalDateTime fields (orderId, ordererId, productId, departureHubId,
arrivalHubId, requestDeadline, shipmentAddress) and ensures quantity > 0; throw
IllegalArgumentException or NullPointerException with clear messages when
validation fails so creation fails fast and gives a clear error location.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCanceledResult.java`:
- Around line 21-28: The mapping in ShipmentCanceledResult.fromEntity uses
shipment.getUpdatedAt() for canceledAt which may be incorrect; check the
Shipment domain for an explicit canceledAt accessor (e.g., getCanceledAt()) and
if it exists, change ShipmentCanceledResult.fromEntity to call
shipment.getCanceledAt() instead of shipment.getUpdatedAt(); if Shipment lacks a
canceledAt field, either add a proper canceledAt property to the Shipment
aggregate and expose getCanceledAt(), or add a clear comment/rename to indicate
that updatedAt is intentionally used as the cancellation timestamp so the
mapping is explicit and correct.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentRoute.java`:
- Around line 121-123: The markCanceled() method currently sets status
unconditionally; follow the same state-transition validation pattern as
markMovingToHub/markArrivedAtHub by checking the current status
(ShipmentRoute#status) before assigning ShipmentRouteStatus.CANCELED and throw
an IllegalStateException for invalid transitions (e.g., when status is
ARRIVED_AT_HUB or already CANCELED) or make it idempotent by returning early if
status == CANCELED—update the markCanceled() method accordingly to perform this
validation instead of unconditional assignment.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ProcessedSagaEventRepository.java`:
- Around line 7-9: The derived deleteByProcessedAtBefore method causes N+1
deletes because Spring Data loads entities before removing; replace it with a
bulk JPQL delete using a `@Modifying` `@Query` on the ProcessedSagaEventRepository
(e.g., a method like deleteBefore or deleteByProcessedAtBefore using "delete
from ProcessedSagaEventJpaEntity e where e.processedAt < :cutoff") and ensure
the method is executed in a `@Transactional` context and marked `@Modifying`
(consider setting clearAutomatically=true) so deletion is performed in one SQL
bulk operation rather than row-by-row.
In
`@shipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentManagerController.java`:
- Around line 37-44: The UserContext parameter is injected into controller
methods but unused; add a clear TODO comment in ShipmentManagerController on the
methods createShipmentManager, getShipmentManager, and searchShipmentManager
stating that UserContext is intentionally kept for future API Gateway /
authorization integration and will be used for permission checks and user info
propagation, so retain the parameter for now; ensure the comment briefly
indicates expected future behavior (e.g., "TODO: use UserContext for auth/claims
from API Gateway") and keep the method signatures unchanged.
In
`@shipment-service/src/test/java/com/shipflow/shipmentservice/application/ShipmentServiceTest.java`:
- Around line 72-80: The test currently uses ReflectionTestUtils inside
createHubRouteResult to set HubRouteResult internals; instead add a
test-friendly way to construct HubRouteResult (either a package-private
constructor, a static test Builder on HubRouteResult, or a separate test
fixture/factory) and update createHubRouteResult to call that
constructor/builder/factory instead of using ReflectionTestUtils.setField for
sequence, departureHubId, arrivalHubId, estimatedDistance and estimatedDuration;
ensure the new construction is accessible to ShipmentServiceTest and remove all
ReflectionTestUtils usage for HubRouteResult.
- Around line 120-123: Enhance the createShipment_success test to capture and
assert the actual Shipment saved and the published event contents: use
ArgumentCaptor.forClass(Shipment.class) to capture the argument passed to
shipmentRepository.save (and/or capture the event via
eventPublisher.publishCreated), call shipmentService.createShipment(command) as
before, then retrieve the captured Shipment and assert key fields like
getOrderId(), getDepartureHubId(), and any status/metadata; also assert the
published event's payload fields to match expected values so the test verifies
state and event content, not just no-exception and publish invocation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3422109e-ce89-4ca6-be61-d6e89dcbd557
📒 Files selected for processing (57)
hub-service/src/main/java/com/shipflow/hubservice/infrastructure/client/DeliveryClient.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/messaging/event/consume/ShipmentCompletedEvent.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/messaging/event/consume/ShipmentCreatedEvent.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/messaging/event/publish/OrderCanceledEvent.javashipment-service/build.gradleshipment-service/src/main/java/com/shipflow/shipmentservice/ShipmentserviceApplication.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/ShipmentEventPublisher.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/ShipmentManagerService.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/ShipmentService.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/client/CacheClient.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/client/HubClient.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/client/dto/HubRouteResult.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/command/CreateShipmentCommand.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCanceledResult.javashipment-service/src/main/java/com/shipflow/shipmentservice/application/dto/result/ShipmentCompleteResult.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/Shipment.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentManager.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentRoute.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/ShipmentRouteStatus.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/event/ShipmentCompletedEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/event/ShipmentCreatedEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/event/ShipmentCreationFailedEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/exception/ShipmentErrorCode.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/repository/ShipmentManagerRepository.javashipment-service/src/main/java/com/shipflow/shipmentservice/domain/repository/ShipmentRepository.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubClient.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubClientImpl.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubFeignClient.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/RedisClient.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/ShipmentEventPublisherImpl.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/config/ShipmentRabbitConfig.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/consume/OrderCanceledEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/consume/OrderCreatedEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/publish/ShipmentCompletedSagaEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/publish/ShipmentCreatedSagaEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/event/publish/ShipmentCreationFailedSagaEvent.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/handler/IdempotentSagaExecutor.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/handler/IdempotentSagaHandler.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/handler/OrderCanceledHandler.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/handler/OrderCreatedHandler.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/scheduler/ProcessedSagaEventCleanupScheduler.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/messaging/scheduler/ShipmentManagerCleanupScheduler.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ProcessedSagaEventJpaEntity.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ProcessedSagaEventRepository.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentJpaRepository.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentManagerJpaRepository.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentManagerRepositoryImpl.javashipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/persistence/ShipmentRepositoryImpl.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentController.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentManagerController.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/ShipmentManagerInternalController.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/UserContext.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/UserContextArgumentResolver.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/WebMvcConfig.javashipment-service/src/main/java/com/shipflow/shipmentservice/presentation/dto/response/ShipmentCompleteResDto.javashipment-service/src/main/resources/application.yamlshipment-service/src/test/java/com/shipflow/shipmentservice/application/ShipmentServiceTest.java
💤 Files with no reviewable changes (1)
- shipment-service/src/main/java/com/shipflow/shipmentservice/infrastructure/client/HubClient.java
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
3 participants
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 제목
✨ 작업 내용
이벤트 처리
shipment.createdshipment.creation.failedshipment.completedorder.created-> 배송 생성 구현order.canceled-> 배송 취소 구현내부 API
DELETE /internal/shipment-managers/hubs/{hubId}DELETE /internal/shipment-managers/users/{hubId}UserContext로 사용자 정보 매핑 추가🔍 상세 내용
배송 생성 완료이벤트 발행배송 생성 실패이벤트 발행배송 완료이벤트 발행주문 생성 성공이벤트 구독 -> 배송 생성 처리주문 취소이벤트 구독 -> 배송 취소 처리🔗 관련 이슈
Closes #38
리뷰 받고 싶은 포인트가 있으면 작성해주세요.
✅ 체크리스트
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
인프라 개선