[REFACTOR] 주문 내부 API 사용 수정 및 페이징 기능 추가 - #43
Open
kim-jun-won wants to merge 11 commits into
Open
kim-jun-won wants to merge 11 commits into
kim-jun-won wants to merge 11 commits into
Conversation
kim-jun-won
requested review from
250ghghghgh,
Jin4041,
afterrest,
soo96 and
zlonce
April 6, 2026 10:09
Contributor
📝 WalkthroughWalkthroughorder-service에 OpenFeign 클라이언트(spring-cloud-starter-openfeign)와 Spring Retry/AOP를 도입하고, Product/User/Company 내부 API용 Feign 인터페이스 및 어댑터를 추가했습니다. 비동기 병렬 호출로 CreateOrderCommand를 조합하는 OrderFetchService가 추가되었고, 주문 도메인·영속성·읽기 모델에 deliveryAddress가 확장되었습니다. 역할 기반 UserRole과 조건 기반 검색(OrderSearchCondition) 및 페이징/정렬 서포트가 추가되었으며 여러 도메인 예외 타입과 컨트롤러/요청·응답 DTO 시그니처가 변경되었습니다. Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as OrderController
participant CmdService as OrderCommandService
participant FetchSvc as OrderFetchService
participant ProductAdapter as ProductClient(Adapter)
participant UserAdapter as UserClient(Adapter)
participant CompanyAdapter as CompanyClient(Adapter)
participant Domain as OrderDomain
participant DB as Database
participant EventPub as EventPublisher
Client->>Controller: POST /api/orders (CreateOrderRequest, X-User-Id)
Controller->>CmdService: createOrder(request, ordererId)
CmdService->>FetchSvc: fetchAndBuild(ordererId, productId, quantity, deadline, note)
par parallel
FetchSvc->>ProductAdapter: fetch(ordererId, productId, quantity)
ProductAdapter-->>FetchSvc: ProductInfo
and
FetchSvc->>UserAdapter: fetch(ordererId)
UserAdapter-->>FetchSvc: UserInfo
end
FetchSvc->>CompanyAdapter: fetch(receiverCompanyId)
CompanyAdapter-->>FetchSvc: ReceiverCompanyInfo
FetchSvc-->>CmdService: CreateOrderCommand (with names, hub, address)
CmdService->>Domain: Order.create(cmd)
Domain-->>CmdService: Order
CmdService->>DB: save(order)
DB-->>CmdService: saved
CmdService->>EventPub: publish(OrderCreatingEvent)
CmdService-->>Controller: OrderResult
Controller-->>Client: 201 Created (OrderResponse)
sequenceDiagram
participant Client as Client
participant Controller as OrderController
participant UserCtx as UserContext
participant QuerySvc as OrderQueryService
participant ReadRepo as OrderReadModelQueryRepository
participant DB as Database
Client->>Controller: GET /api/orders?... (X-User-Id, X-User-Role)
Controller->>UserCtx: getUserRole(httpRequest)
UserCtx-->>Controller: UserRole
alt restricted role
Controller->>QuerySvc: searchOrders(condition.withOrdererId(requesterId), pageable)
else
Controller->>QuerySvc: searchOrders(condition, pageable)
end
QuerySvc->>ReadRepo: search(condition, normalizedPageable)
ReadRepo->>DB: QueryDSL query (filters, sort, limit+1)
DB-->>ReadRepo: results
ReadRepo-->>QuerySvc: Slice<OrderReadModel>
QuerySvc-->>Controller: Slice<OrderReadModel>
Controller-->>Client: 200 OK (Slice<OrderReadModelResponse>)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![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: 9
🧹 Nitpick comments (8)
order-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.java (1)
86-102: 코드 중복 개선 고려
orderReadModelWithStatus와orderReadModel메서드 간에 빌더 설정 코드가 중복됩니다. 향후 필드 추가 시 두 곳을 모두 수정해야 하는 유지보수 부담이 있습니다.♻️ 중복 제거를 위한 리팩토링 제안
+ private static OrderReadModel.OrderReadModelBuilder baseOrderReadModelBuilder(UUID orderId) { + return OrderReadModel.builder() + .orderId(orderId) + .ordererId(USER_ID) + .productId(PRODUCT_ID) + .supplierCompanyId(SUPPLIER_ID) + .receiverCompanyId(RECEIVER_ID) + .departureHubId(DEP_HUB_ID) + .arrivalHubId(ARR_HUB_ID) + .quantity(10) + .requestDeadline(DEADLINE) + .requestNote("테스트 메모") + .createdBy(USER_ID) + .createdAt(LocalDateTime.of(2026, 4, 1, 9, 0)); + } + public static OrderReadModel orderReadModel(UUID orderId) { - return OrderReadModel.builder() - .orderId(orderId) - .orderStatus(OrderStatus.CREATING) - // ... other fields - .build(); + return baseOrderReadModelBuilder(orderId) + .orderStatus(OrderStatus.CREATING) + .build(); } public static OrderReadModel orderReadModelWithStatus(UUID orderId, OrderStatus status) { - return OrderReadModel.builder() - .orderId(orderId) - .orderStatus(status) - // ... other fields - .build(); + return baseOrderReadModelBuilder(orderId) + .orderStatus(status) + .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.java` around lines 86 - 102, The two factory methods orderReadModelWithStatus and orderReadModel duplicate builder setup; refactor by extracting the common builder configuration into a single helper (e.g., a private method like baseOrderReadModelBuilder or buildDefaultOrderReadModel) that returns an OrderReadModel.OrderReadModelBuilder pre-populated with shared fields, then have orderReadModelWithStatus and orderReadModel call that helper and only set the differing fields (orderId and orderStatus) before build(); update references to OrderReadModel.builder() in both methods to use the helper to eliminate duplication.order-service/src/main/java/com/shipflow/orderservice/domain/repository/OrderReadModelRepository.java (1)
3-3: 도메인 레이어에서 애플리케이션 레이어 DTO 의존성
OrderReadModelRepository는 도메인 레이어에 위치하지만application.dto.OrderSearchCondition을 import하고 있습니다. 이는 클린 아키텍처의 의존성 방향 원칙에 어긋납니다.이상적으로는
OrderSearchCondition을 도메인 레이어로 이동하거나, 도메인 레이어 전용 검색 조건 인터페이스를 정의하는 것이 좋습니다. 다만 실용적인 트레이드오프로 현재 구조도 작동하므로 향후 리팩토링 시 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/domain/repository/OrderReadModelRepository.java` at line 3, OrderReadModelRepository currently depends on application-layer DTO OrderSearchCondition which violates dependency direction; replace that dependency by introducing a domain-level search type (e.g., OrderSearchCriteria or an interface) and update OrderReadModelRepository method signatures to use that domain type instead of com.shipflow.orderservice.application.dto.OrderSearchCondition. Concretely: create the new type in the domain package, move or map fields from OrderSearchCondition if needed, change imports/usages in OrderReadModelRepository to reference the new domain symbol, and remove the import of the application DTO so the domain layer no longer depends on the application layer.order-service/src/main/resources/application.yaml (1)
53-53:defaultZone의 localhost 기본값은 운영 환경 안전성이 낮습니다.환경변수 누락 시 비로컬 환경에서 Eureka 등록/조회가 실패할 수 있습니다. 프로파일별 기본값 분리(예: local만 localhost) 또는 필수 환경변수 강제 구성을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/resources/application.yaml` at line 53, The defaultZone property currently falls back to localhost via defaultZone: ${EUREKA_URL:http://localhost:8761/eureka/}, which is unsafe for non-local deployments; remove the hardcoded localhost fallback and either make EUREKA_URL mandatory (e.g., change to use ${EUREKA_URL} with deployment ensuring the env var is set) or move the localhost default into a profile-specific file (create application-local.yaml that sets defaultZone to http://localhost:8761/eureka/ while leaving the main application.yaml to reference ${EUREKA_URL} without a default). Update any docs or deployment manifests to ensure EUREKA_URL is provided in CI/CD or production environments.order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/ProductFeignClient.java (1)
18-21: 헤더 키 문자열은 상수로 모아두는 편이 안전합니다.Line 18~Line 21의
"X-Internal-Request","X-User-Id"는 공용 상수로 추출하면 클라이언트 간 오타/불일치 리스크를 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/ProductFeignClient.java` around lines 18 - 21, ProductFeignClient currently uses literal header names "X-Internal-Request" and "X-User-Id" in the `@RequestHeader` annotations; extract these into shared public constants (e.g., HEADER_INTERNAL_REQUEST and HEADER_USER_ID) in a central place (either a new HttpHeadersConstants class or a common constants interface) and replace the string literals in ProductFeignClient's method signature with references to those constants so all clients use the same header keys and avoid typos/inconsistencies.order-service/src/test/java/com/shipflow/orderservice/application/OrderCommandServiceTest.java (1)
61-61: 입력 매핑 검증을 위해 any() 남용은 줄이는 게 좋습니다.Line 61은 너무 느슨해서
CreateOrderRequest -> fetchAndBuild인자 매핑 버그를 잡기 어렵습니다.eq(...)기반으로 주요 인자를 검증하면 회귀 탐지력이 올라갑니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/test/java/com/shipflow/orderservice/application/OrderCommandServiceTest.java` at line 61, The test in OrderCommandServiceTest uses loose any() matchers for orderFetchService.fetchAndBuild which hides mapping bugs; update the when(...) invocation to assert key inputs explicitly (use eq(...) or argumentCaptor assertions) for the CreateOrderRequest and any other important fields passed into orderFetchService.fetchAndBuild so the test verifies the actual request mapping into fetchAndBuild (e.g., replace any() for the CreateOrderRequest parameter with eq(expectedRequest) or capture/verify specific fields, keep other nonessential params as any() if needed).order-service/src/test/java/com/shipflow/orderservice/presentation/OrderControllerTest.java (1)
190-203: 페이지네이션 파라미터 검증 누락
getOrders_페이지네이션_파라미터전달됨테스트가page=1,size=30파라미터를 전달하지만, 실제로Pageable객체에 해당 값이 올바르게 전달되었는지 검증하지 않습니다.♻️ Pageable 검증 추가 제안
`@Test` void getOrders_페이지네이션_파라미터전달됨() throws Exception { when(userContext.getUserId(any())).thenReturn(userId); when(userContext.getUserRole(any())).thenReturn(UserRole.MASTER); Slice<OrderReadModel> slice = new SliceImpl<>(List.of(), PageRequest.of(1, 30, Sort.by(Sort.Direction.DESC, "createdAt")), false); - when(orderQueryService.searchOrders(any(), any())).thenReturn(slice); + ArgumentCaptor<Pageable> pageableCaptor = ArgumentCaptor.forClass(Pageable.class); + when(orderQueryService.searchOrders(any(), pageableCaptor.capture())).thenReturn(slice); mockMvc.perform(get("/api/orders") .header("X-User-Id", userId.toString()) .header("X-User-Role", "MASTER") .param("page", "1").param("size", "30")) .andExpect(status().isOk()); + + assertThat(pageableCaptor.getValue().getPageNumber()).isEqualTo(1); + assertThat(pageableCaptor.getValue().getPageSize()).isEqualTo(30); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/test/java/com/shipflow/orderservice/presentation/OrderControllerTest.java` around lines 190 - 203, The test getOrders_페이지네이션_파라미터전달됨 currently does not assert that the Pageable passed to orderQueryService.searchOrders contains page=1, size=30 and the expected Sort; add verification using an ArgumentCaptor<Pageable> or Mockito.argThat when(orderQueryService.searchOrders(...)) to capture/assert that the Pageable.getPageNumber() == 1, getPageSize() == 30 and getSort() has Sort.Direction.DESC on "createdAt" (reference: the test method getOrders_페이지네이션_파라미터전달됨 and the mock orderQueryService.searchOrders call).order-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelQueryRepositoryImpl.java (1)
78-92: 알 수 없는 정렬 필드 처리 방식 개선 권장현재
default케이스에서 유효하지 않은 정렬 속성에 대해createdAt.desc()를 추가하고 있습니다. 이로 인해 사용자가 잘못된 정렬 필드(예:"invalidField")를 전달해도 오류 없이 예상치 못한 정렬이 적용됩니다.클라이언트의 실수를 조기에 파악할 수 있도록 유효하지 않은 속성은 무시하거나 예외를 발생시키는 것이 좋습니다.
♻️ 유효하지 않은 정렬 속성 무시 제안
private OrderSpecifier<?>[] toOrderSpecifiers(Sort sort, QOrderReadModelJpaEntity q) { List<OrderSpecifier<?>> specifiers = new ArrayList<>(); for (Sort.Order order : sort) { boolean asc = order.isAscending(); OrderSpecifier<?> spec = switch (order.getProperty()) { case "createdAt" -> asc ? q.createdAt.asc() : q.createdAt.desc(); case "updatedAt" -> asc ? q.updatedAt.asc() : q.updatedAt.desc(); - default -> q.createdAt.desc(); + default -> null; }; - specifiers.add(spec); + if (spec != null) { + specifiers.add(spec); + } } if (specifiers.isEmpty()) { specifiers.add(q.createdAt.desc()); } return specifiers.toArray(new OrderSpecifier[0]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelQueryRepositoryImpl.java` around lines 78 - 92, toOrderSpecifiers currently maps unknown sort properties to q.createdAt.desc(), hiding client mistakes; change it to validate order.getProperty() against allowed fields (e.g., "createdAt", "updatedAt") inside toOrderSpecifiers and if a property is unrecognized throw an IllegalArgumentException (or alternatively skip that Sort.Order) rather than defaulting to createdAt; reference the toOrderSpecifiers method, the Sort.Order instances you iterate, and the QOrderReadModelJpaEntity fields (q.createdAt, q.updatedAt) so the validation logic can locate and handle invalid properties consistently.order-service/src/main/java/com/shipflow/orderservice/presentation/controller/OrderController.java (1)
62-64:toCondition()중복 호출 방지
searchRequest.toCondition()이 두 분기 모두에서 호출됩니다. 한 번만 호출하여 결과를 재사용하는 것이 좋습니다.♻️ 중복 호출 제거 제안
UUID requesterId = userContext.getUserId(httpRequest); UserRole role = userContext.getUserRole(httpRequest); - OrderSearchCondition condition = role.isRestrictedToOwnOrders() - ? searchRequest.toCondition().withOrdererId(requesterId) - : searchRequest.toCondition(); + OrderSearchCondition condition = searchRequest.toCondition(); + if (role.isRestrictedToOwnOrders()) { + condition = condition.withOrdererId(requesterId); + } Slice<OrderReadModel> result = orderQueryService.searchOrders(condition, pageable);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/presentation/controller/OrderController.java` around lines 62 - 64, Compute searchRequest.toCondition() once into a local variable (e.g., baseCondition) and reuse it when building the final OrderSearchCondition: call searchRequest.toCondition() only once, then set condition = role.isRestrictedToOwnOrders() ? baseCondition.withOrdererId(requesterId) : baseCondition; reference the existing searchRequest.toCondition(), role.isRestrictedToOwnOrders(), withOrdererId(requesterId) and the condition variable in OrderController to locate and apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java`:
- Around line 35-39: 현재 클래스 레벨 `@Transactional` 때문에 createOrder() 전체가 트랜잭션 안에서
실행되어 orderFetchService.fetchAndBuild(...)의 외부 Feign 호출들이 트랜잭션을 장시간 점유하고 있습니다; 수정
방법은 외부 조회(orderFetchService.fetchAndBuild)를 트랜잭션 밖으로 이동시켜 먼저
Product/User/Company를 조회하고 CreateOrderCommand를 구성한 뒤, 실제 DB 저장과 이벤트 발행만 별도의 짧은
트랜잭션(예: 메서드 레벨 `@Transactional` 또는 TransactionTemplate을 사용하는 saveAndPublish 같은 새로운
메서드)으로 감싸서 처리하도록 변경하세요; 관련 심볼: createOrder, orderFetchService.fetchAndBuild, 클래스
레벨 `@Transactional`, 저장/이벤트 발행 로직(예: saveOrder, publishEvents)을 찾아 분리 구현하세요.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java`:
- Around line 28-36: Exceptions thrown by productAdapter.fetch(...) or
userAdapter.fetch(...) become wrapped in
java.util.concurrent.CompletionException when using CompletableFuture.join(),
breaking domain-exception mapping; update OrderFetchService to unwrap
CompletionException after the futures complete: wrap the
CompletableFuture.allOf(...).join() / productFuture.join()/userFuture.join()
calls in a try-catch that catches CompletionException, inspect ex.getCause(),
and rethrow the cause if it's an instance of your domain exceptions (or wrap
appropriately), and add the import for java.util.concurrent.CompletionException;
alternatively, use productFuture.handle(...) / exceptionally(...) to propagate
the original cause instead of letting join() produce CompletionException.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderProjectionService.java`:
- Around line 27-33: OrderProjectionService currently saves productName,
supplierCompanyName, receiverCompanyName only at creation while
OrderUpdatedEvent updates only IDs, causing stale names; update the
OrderUpdatedEvent payload to include productName, supplierCompanyName,
receiverCompanyName and modify the projection handler in OrderProjectionService
(the OrderUpdatedEvent handling method) to set the corresponding fields
(productName, supplierCompanyName, receiverCompanyName) whenever
productId/supplierCompanyId/receiverCompanyId are updated so the read model
stays consistent with IDs.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderQueryService.java`:
- Around line 43-47: The current getOrder(UUID orderId, UUID requesterId,
UserRole role) first calls getOrder(orderId) which reveals whether an orderId
exists before authorization; for restricted roles this leaks existence. Change
the logic so that for roles where role.isRestrictedToOwnOrders() you perform an
ownership-aware lookup (e.g. use repository.findByIdAndOrdererId(orderId,
requesterId) or add a getOrder(orderId, requesterId) code path) and return
OrderResult only if found; otherwise throw OrderNotFoundException (do not throw
UnauthorizedException separately). For non-restricted roles keep the existing
getOrder(orderId) flow. Ensure the referenced method names
OrderQueryService.getOrder(UUID, UUID, UserRole), getOrder(UUID), OrderResult,
UnauthorizedException and OrderNotFoundException are updated accordingly.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/CompanyClientAdapter.java`:
- Around line 21-33: In CompanyClientAdapter::fetch, the current catch order
hides feign.RetryableException because catch(feign.FeignException e) wraps it in
ExternalServiceException; change the exception handling to first catch
feign.RetryableException and rethrow it (do not wrap) so Spring Retry can detect
and retry, then catch feign.FeignException for other errors and wrap those in
ExternalServiceException; preserve the existing catch for
feign.FeignException.NotFound to throw CompanyNotFoundException.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/ProductClientAdapter.java`:
- Around line 22-38: The feign.FeignException catch block is swallowing
feign.RetryableException and preventing `@Retryable`(retryFor =
RetryableException.class) from triggering; update fetch(...) so that you either
add a dedicated catch for feign.RetryableException before the generic catch or
detect and rethrow the feign.RetryableException (do not wrap it in
ExternalServiceException) so Spring Retry can see the RetryableException; keep
existing handling for feign.FeignException.NotFound -> ProductNotFoundException
and for other feign.FeignException wrap into ExternalServiceException.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/UserClientAdapter.java`:
- Around line 30-33: The current catch order in UserClientAdapter catches
feign.FeignException before feign.RetryableException which prevents Spring Retry
from seeing RetryableException; modify the exception handling in the method
annotated with `@Retryable` to add a specific catch (feign.RetryableException re)
before the generic catch(feign.FeignException e) and rethrow re (do not wrap) so
RetryableException propagates, keep the existing
catch(feign.FeignException.NotFound) to throw UserNotFoundException and the
final catch(feign.FeignException) to wrap others into ExternalServiceException.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ProductInfo.java`:
- Line 11: ProductInfo currently declares stock as Integer which allows null and
causes NPE when auto-unboxed in ProductClientAdapter (at the comparison
info.stock() < quantity); either change ProductInfo.stock to a primitive int to
guarantee non-null, or add a null-check in ProductClientAdapter before comparing
(e.g., if info.stock() == null) and convert that case to a clear domain
exception/error; locate the ProductInfo.stock declaration and the comparison in
ProductClientAdapter (info.stock() < quantity) and implement one of these fixes
consistently.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/web/UserContext.java`:
- Around line 24-29: The missing-role branch in getUserRole currently throws
IllegalArgumentException which causes inconsistency with the
UnauthorizedException thrown for invalid roles; change the missing-header branch
to throw the same domain auth/authorization exception (e.g.,
UnauthorizedException or the project's AuthenticationException) instead of
IllegalArgumentException so both failure paths from
getUserRole(HttpServletRequest) — including the UserRole.from(role) invalid-role
path — produce a consistent domain exception and HTTP error response.
---
Nitpick comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/domain/repository/OrderReadModelRepository.java`:
- Line 3: OrderReadModelRepository currently depends on application-layer DTO
OrderSearchCondition which violates dependency direction; replace that
dependency by introducing a domain-level search type (e.g., OrderSearchCriteria
or an interface) and update OrderReadModelRepository method signatures to use
that domain type instead of
com.shipflow.orderservice.application.dto.OrderSearchCondition. Concretely:
create the new type in the domain package, move or map fields from
OrderSearchCondition if needed, change imports/usages in
OrderReadModelRepository to reference the new domain symbol, and remove the
import of the application DTO so the domain layer no longer depends on the
application layer.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/ProductFeignClient.java`:
- Around line 18-21: ProductFeignClient currently uses literal header names
"X-Internal-Request" and "X-User-Id" in the `@RequestHeader` annotations; extract
these into shared public constants (e.g., HEADER_INTERNAL_REQUEST and
HEADER_USER_ID) in a central place (either a new HttpHeadersConstants class or a
common constants interface) and replace the string literals in
ProductFeignClient's method signature with references to those constants so all
clients use the same header keys and avoid typos/inconsistencies.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelQueryRepositoryImpl.java`:
- Around line 78-92: toOrderSpecifiers currently maps unknown sort properties to
q.createdAt.desc(), hiding client mistakes; change it to validate
order.getProperty() against allowed fields (e.g., "createdAt", "updatedAt")
inside toOrderSpecifiers and if a property is unrecognized throw an
IllegalArgumentException (or alternatively skip that Sort.Order) rather than
defaulting to createdAt; reference the toOrderSpecifiers method, the Sort.Order
instances you iterate, and the QOrderReadModelJpaEntity fields (q.createdAt,
q.updatedAt) so the validation logic can locate and handle invalid properties
consistently.
In
`@order-service/src/main/java/com/shipflow/orderservice/presentation/controller/OrderController.java`:
- Around line 62-64: Compute searchRequest.toCondition() once into a local
variable (e.g., baseCondition) and reuse it when building the final
OrderSearchCondition: call searchRequest.toCondition() only once, then set
condition = role.isRestrictedToOwnOrders() ?
baseCondition.withOrdererId(requesterId) : baseCondition; reference the existing
searchRequest.toCondition(), role.isRestrictedToOwnOrders(),
withOrdererId(requesterId) and the condition variable in OrderController to
locate and apply the change.
In `@order-service/src/main/resources/application.yaml`:
- Line 53: The defaultZone property currently falls back to localhost via
defaultZone: ${EUREKA_URL:http://localhost:8761/eureka/}, which is unsafe for
non-local deployments; remove the hardcoded localhost fallback and either make
EUREKA_URL mandatory (e.g., change to use ${EUREKA_URL} with deployment ensuring
the env var is set) or move the localhost default into a profile-specific file
(create application-local.yaml that sets defaultZone to
http://localhost:8761/eureka/ while leaving the main application.yaml to
reference ${EUREKA_URL} without a default). Update any docs or deployment
manifests to ensure EUREKA_URL is provided in CI/CD or production environments.
In
`@order-service/src/test/java/com/shipflow/orderservice/application/OrderCommandServiceTest.java`:
- Line 61: The test in OrderCommandServiceTest uses loose any() matchers for
orderFetchService.fetchAndBuild which hides mapping bugs; update the when(...)
invocation to assert key inputs explicitly (use eq(...) or argumentCaptor
assertions) for the CreateOrderRequest and any other important fields passed
into orderFetchService.fetchAndBuild so the test verifies the actual request
mapping into fetchAndBuild (e.g., replace any() for the CreateOrderRequest
parameter with eq(expectedRequest) or capture/verify specific fields, keep other
nonessential params as any() if needed).
In
`@order-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.java`:
- Around line 86-102: The two factory methods orderReadModelWithStatus and
orderReadModel duplicate builder setup; refactor by extracting the common
builder configuration into a single helper (e.g., a private method like
baseOrderReadModelBuilder or buildDefaultOrderReadModel) that returns an
OrderReadModel.OrderReadModelBuilder pre-populated with shared fields, then have
orderReadModelWithStatus and orderReadModel call that helper and only set the
differing fields (orderId and orderStatus) before build(); update references to
OrderReadModel.builder() in both methods to use the helper to eliminate
duplication.
In
`@order-service/src/test/java/com/shipflow/orderservice/presentation/OrderControllerTest.java`:
- Around line 190-203: The test getOrders_페이지네이션_파라미터전달됨 currently does not
assert that the Pageable passed to orderQueryService.searchOrders contains
page=1, size=30 and the expected Sort; add verification using an
ArgumentCaptor<Pageable> or Mockito.argThat
when(orderQueryService.searchOrders(...)) to capture/assert that the
Pageable.getPageNumber() == 1, getPageSize() == 30 and getSort() has
Sort.Direction.DESC on "createdAt" (reference: the test method
getOrders_페이지네이션_파라미터전달됨 and the mock orderQueryService.searchOrders call).
🪄 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: 8658951e-072f-46b0-a7d8-c05fbeb43466
📒 Files selected for processing (46)
keycloak/shipflow-export.jsonorder-service/build.gradleorder-service/src/main/java/com/shipflow/orderservice/OrderserviceApplication.javaorder-service/src/main/java/com/shipflow/orderservice/application/dto/CreateOrderCommand.javaorder-service/src/main/java/com/shipflow/orderservice/application/dto/OrderSearchCondition.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderProjectionService.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderQueryService.javaorder-service/src/main/java/com/shipflow/orderservice/domain/event/OrderCreatingEvent.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/CompanyNotFoundException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/ExternalServiceException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/InsufficientStockException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/OrderErrorCode.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/ProductNotFoundException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/UnauthorizedException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/exception/UserNotFoundException.javaorder-service/src/main/java/com/shipflow/orderservice/domain/model/UserRole.javaorder-service/src/main/java/com/shipflow/orderservice/domain/repository/OrderReadModelRepository.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/CompanyFeignClient.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/ProductFeignClient.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/UserFeignClient.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/CompanyClientAdapter.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/ProductClientAdapter.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/UserClientAdapter.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ProductInfo.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ReceiverCompanyInfo.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/UserInfo.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/config/QuerydslConfig.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/messaging/event/publish/OrderCreatedEvent.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelQueryRepository.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelQueryRepositoryImpl.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderReadModelRepositoryImpl.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/web/UserContext.javaorder-service/src/main/java/com/shipflow/orderservice/presentation/controller/OrderController.javaorder-service/src/main/java/com/shipflow/orderservice/presentation/controller/OrderInternalController.javaorder-service/src/main/java/com/shipflow/orderservice/presentation/dto/CreateOrderRequest.javaorder-service/src/main/java/com/shipflow/orderservice/presentation/dto/OrderReadModelResponse.javaorder-service/src/main/java/com/shipflow/orderservice/presentation/dto/OrderSearchRequest.javaorder-service/src/main/resources/application.yamlorder-service/src/test/java/com/shipflow/orderservice/application/OrderCommandServiceTest.javaorder-service/src/test/java/com/shipflow/orderservice/application/OrderQueryServiceTest.javaorder-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.javaorder-service/src/test/java/com/shipflow/orderservice/integration/OrderIntegrationTest.javaorder-service/src/test/java/com/shipflow/orderservice/presentation/OrderControllerTest.javaorder-service/src/test/resources/test-schema.sql
💤 Files with no reviewable changes (1)
- order-service/src/main/java/com/shipflow/orderservice/presentation/dto/CreateOrderRequest.java
Comment on lines
+35
to
+39
| public OrderResult createOrder(CreateOrderRequest request, UUID ordererId) { | ||
| CreateOrderCommand cmd = orderFetchService.fetchAndBuild( | ||
| ordererId, request.productId(), request.quantity(), | ||
| request.requestDeadline(), request.requestNote() | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Spring, does class-level @transactional wrap the entire public method body, including Feign/HTTP calls made before the repository save, and are external network calls inside a transaction considered bad practice?
💡 Result:
Yes—if the call enters through the Spring transactional proxy, a class-level @Transactional applies to each eligible method (typically public methods) and the transaction advice runs around the entire method invocation (begin before the method body executes, commit/rollback after it returns/throws). In Spring’s default proxy mode, only external calls through the proxy are intercepted (self-invocation won’t trigger the interceptor), but once you’re inside the proxied method, everything you do in that method (including Feign/HTTP calls made before repository.save(...)) executes while the transaction is active. [1]
That said, the Feign/HTTP call is not part of the database transaction (it can’t be atomically committed/rolled back with your DB work), and Spring explicitly notes it does not propagate transaction contexts across remote calls. [2]
Putting external network calls inside a DB transaction is generally considered bad practice because it can make transactions long-running (holding connections/locks longer, increasing contention/timeouts/deadlock risk). Common guidance is to keep transactions short and avoid I/O/remote calls inside them. [3]
Sources:
[1] Spring Framework Reference (Transaction Management) – proxy mode / external calls intercepted
[2] Spring Framework Data Access reference – no transaction context propagation across remote calls
[3] Best-practices guidance (keep transactions short; avoid I/O/remote calls inside transactions)
🏁 Script executed:
find . -type f -name "OrderCommandService.java" | head -5Repository: Ship-Flow/ship-flow
Length of output: 163
🏁 Script executed:
cat -n ./order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java | head -60Repository: Ship-Flow/ship-flow
Length of output: 3318
🏁 Script executed:
grep -n "class OrderFetchService" ./order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java | head -5Repository: Ship-Flow/ship-flow
Length of output: 98
🏁 Script executed:
cat -n ./order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java | head -100Repository: Ship-Flow/ship-flow
Length of output: 2809
외부 조회를 트랜잭션 범위 밖으로 이동하세요.
클래스 레벨 @Transactional으로 인해 createOrder() 메서드 전체가 하나의 데이터베이스 트랜잭션으로 실행됩니다. 현재 코드는 orderFetchService.fetchAndBuild() 메서드에서 Product, User, Company 서비스로의 Feign 호출 3회를 모두 트랜잭션 내에서 수행한 후 저장하므로, 네트워크 대기 시간이 트랜잭션을 장시간 점유합니다. 이로 인해 데이터베이스 연결이 오래 유지되어 잠금 경합, 타임아웃, 데드락 위험이 증가합니다.
외부 조회는 트랜잭션 밖에서 완료하고, 데이터베이스 저장과 이벤트 발행만 별도 트랜잭션으로 감싸도록 구조를 개선하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java`
around lines 35 - 39, 현재 클래스 레벨 `@Transactional` 때문에 createOrder() 전체가 트랜잭션 안에서
실행되어 orderFetchService.fetchAndBuild(...)의 외부 Feign 호출들이 트랜잭션을 장시간 점유하고 있습니다; 수정
방법은 외부 조회(orderFetchService.fetchAndBuild)를 트랜잭션 밖으로 이동시켜 먼저
Product/User/Company를 조회하고 CreateOrderCommand를 구성한 뒤, 실제 DB 저장과 이벤트 발행만 별도의 짧은
트랜잭션(예: 메서드 레벨 `@Transactional` 또는 TransactionTemplate을 사용하는 saveAndPublish 같은 새로운
메서드)으로 감싸서 처리하도록 변경하세요; 관련 심볼: createOrder, orderFetchService.fetchAndBuild, 클래스
레벨 `@Transactional`, 저장/이벤트 발행 로직(예: saveOrder, publishEvents)을 찾아 분리 구현하세요.
| UUID supplierCompanyId, | ||
| String supplierCompanyName, | ||
| UUID departureHubId, | ||
| Integer stock |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] ProductInfo.stock() 사용 지점 확인"
rg -nP --type=java -C2 '\.stock\(\)'
echo
echo "[2] stock 비교/언박싱 경로 확인"
rg -nP --type=java -C2 'stock\(\)\s*(<|>|<=|>=|==|!=)'
echo
echo "[3] null 가드 존재 여부 확인"
rg -nP --type=java -C2 'stock\(\)\s*==\s*null|Objects\.requireNonNull\([^)]*stock\(\)'Repository: Ship-Flow/ship-flow
Length of output: 3276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] ProductInfo.java file - check stock field declaration"
find . -name "ProductInfo.java" -type f | head -5
echo
echo "[2] Read ProductInfo.java to verify Integer type"
find . -name "ProductInfo.java" -type f | xargs cat -n | head -50
echo
echo "[3] ProductClientAdapter.java full context around stock() comparison (lines 20-40)"
rg -nP --type=java -A20 "getProductInfo.*quantity" order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/ProductClientAdapter.java
echo
echo "[4] Check if ProductUpdateStockRequest also uses Integer"
rg -nP --type=java "class ProductUpdateStockRequest" -A15Repository: Ship-Flow/ship-flow
Length of output: 1288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] Check ProductFeignClient interface for null-handling annotations"
rg -nP --type=java "interface ProductFeignClient" -A30 order-service/
echo
echo "[2] Check response wrapper class (getProductInfo return type)"
rg -nP --type=java "class.*Response" order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ | head -20Repository: Ship-Flow/ship-flow
Length of output: 1601
stock를 Integer로 두면 런타임 NPE 경로가 열립니다.
ProductClientAdapter 라인 31에서 info.stock() < quantity 비교 시 auto-unboxing이 발생하므로, API가 null을 반환하면 즉시 NullPointerException이 발생합니다. 현재 try-catch는 FeignException만 처리하고 NPE는 처리하지 않습니다.
다음 중 하나로 수정하세요:
Integer를int(primitive)로 변경- 어댑터에서 stock 비교 전 null 검증 추가 및 도메인 예외로 변환
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ProductInfo.java`
at line 11, ProductInfo currently declares stock as Integer which allows null
and causes NPE when auto-unboxed in ProductClientAdapter (at the comparison
info.stock() < quantity); either change ProductInfo.stock to a primitive int to
guarantee non-null, or add a null-check in ProductClientAdapter before comparing
(e.g., if info.stock() == null) and convert that case to a clear domain
exception/error; locate the ProductInfo.stock declaration and the comparison in
ProductClientAdapter (info.stock() < quantity) and implement one of these fixes
consistently.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
order-service/src/main/java/com/shipflow/orderservice/domain/model/Order.java (1)
45-57:⚠️ Potential issue | 🟡 Minor
deliveryAddress유효성 검증이 필요합니다Line [45]~Line [57]에서 주소가
null/blank여도 주문 생성이 가능해 downstream 처리에서 실패할 수 있습니다. 생성 시점에 방어 검증을 추가하는 편이 안전합니다.제안 수정
public static Order create( UUID ordererId, UUID productId, CompanyInfo companyInfo, HubInfo hubInfo, Quantity quantity, LocalDateTime requestDeadline, String requestNote, String deliveryAddress, UUID createdBy ) { + if (deliveryAddress == null || deliveryAddress.isBlank()) { + throw new IllegalArgumentException("deliveryAddress는 필수입니다."); + } Order order = new Order(); order.id = UUID.randomUUID(); ... order.requestNote = requestNote; order.deliveryAddress = deliveryAddress;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/domain/model/Order.java` around lines 45 - 57, Add defensive validation for the deliveryAddress parameter in the Order creation code: before instantiating and populating Order (the block that sets order.id, order.ordererId, order.productId, etc.), check that deliveryAddress is not null and not blank (e.g., after trimming) and if it is invalid throw an IllegalArgumentException with a clear message; update the Order creation/factory logic in the Order class so all callers fail fast on bad addresses.
♻️ Duplicate comments (2)
order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java (1)
27-39:⚠️ Potential issue | 🟠 Major외부 조회가 트랜잭션 내부에서 실행되고 있습니다
Line [27], Line [35]~Line [39] 기준으로
createOrder()전체가 트랜잭션 안에서 수행되어 Feign 조회 지연이 DB 트랜잭션 점유 시간으로 직결됩니다. 외부 조회(orderFetchService.fetchAndBuild)와 DB 저장/이벤트 발행의 트랜잭션 경계를 분리해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java` around lines 27 - 39, The createOrder method in OrderCommandService currently runs inside the `@Transactional` class scope causing orderFetchService.fetchAndBuild (an external/Feign call) to execute within the DB transaction; refactor so the external fetch happens outside the transaction boundary (call orderFetchService.fetchAndBuild before entering the transactional block) and confine DB write and event publish logic to a separate transactional method (e.g., a private or package-visible method annotated with `@Transactional` that performs orderRepository.save(...) and publishes via rabbitPublisher and domainEventPublisher). Ensure createOrder orchestrates: 1) call orderFetchService.fetchAndBuild(...) first, 2) then invoke the transactional save/publish method to persist and emit events.order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java (1)
33-36:⚠️ Potential issue | 🟠 Major
join()예외 언랩이 없어 도메인 예외 매핑이 깨질 수 있습니다Line [33]~Line [36]에서
join()이CompletionException으로 감싸서 던지기 때문에, 어댑터에서 던진 도메인 예외 타입이 상위 계층에 그대로 전달되지 않습니다.join()구간에서 원인 예외를 언랩해 재던지세요.제안 수정
+import java.util.concurrent.CompletionException; ... - CompletableFuture.allOf(productFuture, userFuture).join(); - - ProductInfo product = productFuture.join(); - UserInfo user = userFuture.join(); + final ProductInfo product; + final UserInfo user; + try { + CompletableFuture.allOf(productFuture, userFuture).join(); + product = productFuture.join(); + user = userFuture.join(); + } catch (CompletionException e) { + Throwable cause = e.getCause(); + if (cause instanceof RuntimeException re) throw re; + if (cause instanceof Error er) throw er; + throw e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java` around lines 33 - 36, The current use of CompletableFuture.allOf(productFuture, userFuture).join() followed by productFuture.join() and userFuture.join() will wrap adapter-thrown domain exceptions in CompletionException and break exception mapping; modify the code around these calls (references: productFuture, userFuture, ProductInfo product = productFuture.join(), UserInfo user = userFuture.join()) to catch CompletionException (and/or ExecutionException if you prefer) after the joins, unwrap the root cause via getCause(), and rethrow that cause (or map it back to the correct domain exception) so the original domain exception types from the adapters are propagated to upper layers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderJpaEntity.java`:
- Around line 59-60: OrderJpaEntity now declares a new field deliveryAddress but
the production schema lacks p_orders.delivery_address; add an explicit
production migration to add this column rather than relying on Hibernate
ddl-auto: create a new SQL migration (or Liquibase/Flyway changelog) that ALTERs
table p_orders ADD COLUMN delivery_address with the same type and nullability as
used in OrderJpaEntity, place it alongside other production init scripts (e.g.,
docker/postgres/init/) or register it with your migration tool, and ensure
deployment runs this migration before services start; also verify
test-schema.sql and any schema-generation scripts remain consistent.
---
Outside diff comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/domain/model/Order.java`:
- Around line 45-57: Add defensive validation for the deliveryAddress parameter
in the Order creation code: before instantiating and populating Order (the block
that sets order.id, order.ordererId, order.productId, etc.), check that
deliveryAddress is not null and not blank (e.g., after trimming) and if it is
invalid throw an IllegalArgumentException with a clear message; update the Order
creation/factory logic in the Order class so all callers fail fast on bad
addresses.
---
Duplicate comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.java`:
- Around line 27-39: The createOrder method in OrderCommandService currently
runs inside the `@Transactional` class scope causing
orderFetchService.fetchAndBuild (an external/Feign call) to execute within the
DB transaction; refactor so the external fetch happens outside the transaction
boundary (call orderFetchService.fetchAndBuild before entering the transactional
block) and confine DB write and event publish logic to a separate transactional
method (e.g., a private or package-visible method annotated with `@Transactional`
that performs orderRepository.save(...) and publishes via rabbitPublisher and
domainEventPublisher). Ensure createOrder orchestrates: 1) call
orderFetchService.fetchAndBuild(...) first, 2) then invoke the transactional
save/publish method to persist and emit events.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java`:
- Around line 33-36: The current use of CompletableFuture.allOf(productFuture,
userFuture).join() followed by productFuture.join() and userFuture.join() will
wrap adapter-thrown domain exceptions in CompletionException and break exception
mapping; modify the code around these calls (references: productFuture,
userFuture, ProductInfo product = productFuture.join(), UserInfo user =
userFuture.join()) to catch CompletionException (and/or ExecutionException if
you prefer) after the joins, unwrap the root cause via getCause(), and rethrow
that cause (or map it back to the correct domain exception) so the original
domain exception types from the adapters are propagated to upper layers.
🪄 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: da8bf69c-44ef-4287-9d6a-21850a9eb512
📒 Files selected for processing (12)
order-service/src/main/java/com/shipflow/orderservice/application/dto/CreateOrderCommand.javaorder-service/src/main/java/com/shipflow/orderservice/application/dto/OrderResult.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderCommandService.javaorder-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.javaorder-service/src/main/java/com/shipflow/orderservice/domain/model/Order.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ReceiverCompanyInfo.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/messaging/event/publish/OrderCreatedEvent.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/persistence/OrderJpaEntity.javaorder-service/src/test/java/com/shipflow/orderservice/application/OrderCommandServiceTest.javaorder-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.javaorder-service/src/test/java/com/shipflow/orderservice/integration/OrderIntegrationTest.javaorder-service/src/test/resources/test-schema.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/dto/ReceiverCompanyInfo.java
- order-service/src/test/resources/test-schema.sql
- order-service/src/main/java/com/shipflow/orderservice/application/dto/CreateOrderCommand.java
- order-service/src/test/java/com/shipflow/orderservice/fixture/OrderFixture.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java (1)
29-32: 블로킹 HTTP 호출에ForkJoinPool.commonPool()사용은 위험합니다.
supplyAsync()를 executor 없이 호출하면 기본적으로ForkJoinPool.commonPool()을 사용합니다. 이 풀은 CPU 바운드 작업용으로 설계되었으며, Feign 클라이언트의 블로킹 HTTP 호출을 실행하면 풀이 고갈되어 애플리케이션의 다른 병렬 스트림 및 CompletableFuture 작업에 영향을 줄 수 있습니다.I/O 바운드 작업을 위한 전용
Executor를 주입하여 사용하는 것을 권장합니다.♻️ 전용 Executor 사용 제안
+import java.util.concurrent.Executor; + `@Service` `@RequiredArgsConstructor` public class OrderFetchService { private final ProductClientAdapter productAdapter; private final UserClientAdapter userAdapter; private final CompanyClientAdapter companyAdapter; + private final Executor ioExecutor; // `@Bean으로` 등록된 I/O용 Executor 주입 public CreateOrderCommand fetchAndBuild(UUID ordererId, UUID productId, int quantity, LocalDateTime deadline, String note) { // Step 1: product, user 병렬 호출 CompletableFuture<ProductInfo> productFuture = CompletableFuture.supplyAsync( - () -> productAdapter.fetch(ordererId.toString(), productId, quantity)); + () -> productAdapter.fetch(ordererId.toString(), productId, quantity), ioExecutor); CompletableFuture<UserInfo> userFuture = CompletableFuture.supplyAsync( - () -> userAdapter.fetch(ordererId)); + () -> userAdapter.fetch(ordererId), ioExecutor);별도의 Configuration 클래스에서 Executor Bean을 정의합니다:
`@Configuration` public class AsyncConfig { `@Bean` public Executor ioExecutor() { return Executors.newCachedThreadPool(); // 또는 ThreadPoolTaskExecutor로 상세 설정 가능 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java` around lines 29 - 32, The CompletableFuture.supplyAsync calls in OrderFetchService currently use the ForkJoinPool.commonPool() for blocking Feign HTTP calls (see CompletableFuture.supplyAsync and productAdapter.fetch / userAdapter.fetch); inject a dedicated I/O Executor bean (e.g., create AsyncConfig that defines an ioExecutor bean) and change the two calls to supplyAsync(..., ioExecutor). Modify OrderFetchService to accept the Executor (constructor injection or `@Autowired/`@Qualifier("ioExecutor")) and use that executor for both productFuture and userFuture to avoid exhausting the common pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java`:
- Line 51: OrderFetchService calls
companyAdapter.fetch(user.receiverCompanyId()) without guarding against a null
receiverCompanyId; add a null-check on UserInfo.receiverCompanyId() before
calling companyAdapter.fetch (in the method that performs the fetch in
OrderFetchService), and handle the null case by either throwing a clear domain
exception (e.g., InvalidRequestException / MissingReceiverCompanyId) or
returning a safe default/result and logging the situation; ensure the check
references UserInfo.receiverCompanyId() and companyAdapter.fetch(...) so the fix
is easy to locate and unit-test.
---
Nitpick comments:
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java`:
- Around line 29-32: The CompletableFuture.supplyAsync calls in
OrderFetchService currently use the ForkJoinPool.commonPool() for blocking Feign
HTTP calls (see CompletableFuture.supplyAsync and productAdapter.fetch /
userAdapter.fetch); inject a dedicated I/O Executor bean (e.g., create
AsyncConfig that defines an ioExecutor bean) and change the two calls to
supplyAsync(..., ioExecutor). Modify OrderFetchService to accept the Executor
(constructor injection or `@Autowired/`@Qualifier("ioExecutor")) and use that
executor for both productFuture and userFuture to avoid exhausting the common
pool.
🪄 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: 0d957a40-ee0a-43ee-8435-cca7bf68abc8
📒 Files selected for processing (4)
order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/CompanyClientAdapter.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/ProductClientAdapter.javaorder-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/UserClientAdapter.java
🚧 Files skipped from review as they are similar to previous changes (3)
- order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/UserClientAdapter.java
- order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/CompanyClientAdapter.java
- order-service/src/main/java/com/shipflow/orderservice/infrastructure/client/adapter/ProductClientAdapter.java
| UserInfo user = userFuture.join(); | ||
|
|
||
| // Step 2: receiverCompanyId 확보 후 company 호출 | ||
| ReceiverCompanyInfo company = companyAdapter.fetch(user.receiverCompanyId()); |
Contributor
There was a problem hiding this comment.
user.receiverCompanyId()가 null일 경우에 대한 방어 로직이 없습니다.
UserInfo.receiverCompanyId()가 외부 User 서비스에서 null로 반환될 경우, companyAdapter.fetch()에 null이 전달되어 NPE 또는 잘못된 HTTP 요청이 발생할 수 있습니다.
🛡️ null 검증 추가 제안
ProductInfo product = productFuture.join();
UserInfo user = userFuture.join();
// Step 2: receiverCompanyId 확보 후 company 호출
+ UUID receiverCompanyId = user.receiverCompanyId();
+ if (receiverCompanyId == null) {
+ throw new IllegalStateException("User의 receiverCompanyId가 설정되지 않았습니다: " + ordererId);
+ }
- ReceiverCompanyInfo company = companyAdapter.fetch(user.receiverCompanyId());
+ ReceiverCompanyInfo company = companyAdapter.fetch(receiverCompanyId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@order-service/src/main/java/com/shipflow/orderservice/application/service/OrderFetchService.java`
at line 51, OrderFetchService calls
companyAdapter.fetch(user.receiverCompanyId()) without guarding against a null
receiverCompanyId; add a null-check on UserInfo.receiverCompanyId() before
calling companyAdapter.fetch (in the method that performs the fetch in
OrderFetchService), and handle the null case by either throwing a clear domain
exception (e.g., InvalidRequestException / MissingReceiverCompanyId) or
returning a safe default/result and logging the situation; ensure the check
references UserInfo.receiverCompanyId() and companyAdapter.fetch(...) so the fix
is easy to locate and unit-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
4 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 제목
[Refactor] #41 [REFACTOR] 주문 내부 API 사용 수정 및 페이징 기능 추가
✨ 작업 내용
주문 최초 생성시 다음의 작업을 수행하게 합니다.
🔍 상세 내용
구체적인 작업 내용을 작성해주세요.
-FeignClient 설정
-Self-invocation 을 막기위한 Adaptor패턴 적용
-Inner api 에 맞는 dto 작성
🔗 관련 이슈
Closes #41
✅ 체크리스트
Summary by CodeRabbit
새로운 기능
개선사항
테스트