[FEATURE] [Notification] AI-Slack 이벤트 연동 및 Slack API 개선 #30 - #48
Merged
Merged
Conversation
…rvice/infrastructure/messaging/dto/ShipmentCreatedEvent.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ID 정규식 확장, 페이지 정렬 추가
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough이 변경은 shipment.created 이벤트 수신부터 AI 발송 시한 생성, Slack 자동 발송 및 상태 기록까지의 이벤트 기반 오케스트레이션을 구현합니다. Redis 기반 멱등성 검사와 RabbitMQ 리스너 연동, Order 서비스 조회(Feign), NotificationOrchestratorService를 통한 AiAppService/SlackAppService 연동, 헤더 기반 userId/userRole 권한 검증 추가 및 AI/Slack 로그의 페이징 검색(QueryDSL)·검증·리포지토리 변경이 포함됩니다. 또한 Redis/RedisTemplate, Feign, Swagger/OpenAPI, Eureka 설정 및 docker-compose·application.yaml 관련 인프라/구성 변경이 포함됩니다. Sequence Diagram(s)sequenceDiagram
participant RabbitMQ as RabbitMQ
participant Handler as ShipmentCreatedHandler
participant Redis as Redis (idempotency)
participant Orchestrator as NotificationOrchestratorService
participant OrderClient as OrderInternalClient (Feign)
participant AiService as AiAppService
participant AiRepo as AiLogRepository
participant SlackService as SlackAppService
participant SlackRepo as SlackMessageRepository
RabbitMQ->>Handler: ShipmentCreatedEvent
Handler->>Redis: setIfAbsent(saga:processed:{eventId})
alt already processed
Redis-->>Handler: false
Handler->>Handler: log warning & return
else first time
Redis-->>Handler: true
Handler->>Orchestrator: handleShipmentCreated(event)
Orchestrator->>OrderClient: getOrderReadModel(orderId)
OrderClient-->>Orchestrator: OrderReadModelResponse / (error -> null)
Orchestrator->>AiService: generateAiLog(GenerateDeadlineCommand)
AiService->>AiRepo: save(aiLog)
AiRepo-->>AiService: saved AiLog (id)
Orchestrator->>SlackService: sendSlackMessage(SendSlackMessageCommand)
SlackService->>SlackRepo: save(slackMessage)
SlackRepo-->>SlackService: saved SlackMessage
alt send success
SlackService-->>Orchestrator: success
Orchestrator->>AiRepo: markSlackSendSuccess(aiLogId)
else send failure
SlackService-->>Orchestrator: exception
Orchestrator->>AiRepo: markSlackSendFail(aiLogId)
Orchestrator-->>Handler: rethrow
Handler->>Redis: delete(saga:processed:{eventId})
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Notification 서비스에서 shipment.created 이벤트를 수신해 AI 발송 시한을 계산하고 Slack 알림까지 자동 발송하는 오케스트레이션 흐름을 추가하며, Slack/AI 조회 API의 페이징·검색 및 설정(Feign/Eureka/Swagger/Auditing/Redis 등)을 보강합니다.
Changes:
- RabbitMQ
ShipmentCreatedEvent소비 → AI 생성 → Slack 발송까지 이어지는NotificationOrchestratorService및 소비 핸들러(멱등성 처리) 추가 - Slack/AI 외부 API에 검색/페이징, 요청 검증(@Valid) 및 역할 기반 접근 제어(헤더 기반) 적용
- Feign/Eureka/Swagger/Gemini WebClient/JPA Auditing/Redis 등 인프라 설정 및 의존성 보강
Reviewed changes
Copilot reviewed 44 out of 47 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| notification-service/src/test/java/com/shipflow/notificationservice/SlackAppServiceTest.java | SlackAppService API 시그니처 변경(유저/역할, 페이징 검색) 반영 테스트 수정 |
| notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java | shipment.created 처리 오케스트레이션 단위 테스트 추가 |
| notification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.java | AiAppService 조회 시그니처 변경(유저/역할) 반영 |
| notification-service/src/main/resources/application.yaml | 서비스명/DB/RabbitMQ/Redis/Eureka/Swagger 등 런타임 설정 조정 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/external/SlackController.java | Slack API에 헤더 기반 사용자정보, 검색/페이징 파라미터, 검증 적용 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/response/SlackMessageResponse.java | 목록 변환 유틸 제거(페이지 기반 응답으로 전환) |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/UpdateSlackMessageRequest.java | 수정 요청 DTO 검증 및 command 변환 시 user/role 포함 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/SendSlackMessageRequest.java | 발송 요청 DTO 검증 및 command 변환 시 user/role 포함 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/common/NotificationExceptionHandler.java | IllegalArgumentException → 공통 API 응답으로 변환하는 예외 처리 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/common/BasePageRequest.java | page/size 기본값 처리 및 기본 정렬 pageable 생성 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/external/AiController.java | AI 조회 검색/페이징 파라미터 및 헤더 기반 사용자정보 처리 추가, debug 엔드포인트 분리 |
| notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/dto/request/GenerateDeadlineRequest.java | AI 생성 요청 스키마 확장(orderId 등) 및 command 변환 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/NotificationserviceApplication.java | Feign 활성화(@EnableFeignClients) |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageRepositoryImpl.java | QueryDSL 기반 Slack 메시지 검색/페이징 구현 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageJpaRepository.java | 목록 조회 메서드 제거(검색/페이징 구현으로 대체) |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/ai/AiLogRepositoryImpl.java | QueryDSL 기반 AI 로그 검색/페이징 구현 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/ai/AiLogJpaRepository.java | 목록 조회 메서드 제거(검색/페이징 구현으로 대체) |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/dto/ShipmentCreatedEvent.java | shipment.created 이벤트 DTO 확장 및 SagaEvent 상속 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedListener.java | 이벤트 핸들러 위임용 컴포넌트 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedHandler.java | RabbitMQ 리스너 + Redis 멱등성 처리 + 오케스트레이터 호출 구현 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedConsumer.java | 기존 단순 로깅 Consumer 제거 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderReadModelResponse.java | Order read-model 응답 DTO 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderInternalClient.java | orderservice FeignClient 추가(내부 read-model 조회) |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/ai/GeminiApiClient.java | WebClient 빈 충돌 방지 위해 Qualifier 기반 주입으로 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/ai/config/GeminiApiConfig.java | gemini 전용 WebClient 빈 명시 및 타임아웃 설정 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/SlackMessage.java | 메시지 업데이트 메서드 위치/주석 정리 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/repository/SlackMessageRepository.java | Slack 메시지 검색/페이징 repository API로 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/exception/SlackErrorCode.java | Slack 접근 거부 에러 코드 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/repository/AiLogRepository.java | AI 로그 검색/페이징 repository API로 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/exception/AiErrorCode.java | AI 접근 거부 에러 코드 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/AiLog.java | 공백/포맷팅 정리 |
| notification-service/src/main/java/com/shipflow/notificationservice/config/SwaggerConfig.java | OpenAPI(Swagger) 설정 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/config/RedisConfig.java | RedisTemplate(String,String) 설정 추가(멱등성 키 저장용) |
| notification-service/src/main/java/com/shipflow/notificationservice/config/JPAConfig.java | AuditorAware를 X-User-Id 헤더 기반으로 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/slack/SlackAppService.java | Slack CRUD에 역할 검증 추가 및 목록 조회를 검색/페이징으로 변경 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/UpdateSlackMessageCommand.java | update command에 userId/userRole 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SendSlackMessageCommand.java | send command에 userId/userRole 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SearchSlackMessageCommand.java | Slack 메시지 검색 command 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java | 이벤트 기반 AI+Slack 오케스트레이션 서비스 추가 및 발송 상태 업데이트 로직 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/ai/dto/command/SearchAiLogCommand.java | AI 로그 검색 command 추가 |
| notification-service/src/main/java/com/shipflow/notificationservice/application/ai/dto/command/GenerateDeadlineCommand.java | AI deadline 생성 command 스키마 확장(orderId, quantity 등) |
| notification-service/src/main/java/com/shipflow/notificationservice/application/ai/AiAppService.java | AI 조회/검색에 역할 검증 및 repository search 기반 페이징 적용 |
| notification-service/build.gradle | Feign/Eureka/Swagger/Redis 의존성 추가 |
| keycloak/shipflow-export.json | Keycloak realm export 업데이트(포맷 포함) |
| docker-compose.yml | notificationservice RabbitMQ 환경변수 추가 및 포트/헬스체크 포맷 정리 |
| common/src/main/java/com/shipflow/common/domain/BaseEntity.java | 공백/포맷팅 정리 |
| .gitignore | 파일 끝 공백 라인 추가 |
Comments suppressed due to low confidence (1)
notification-service/src/main/java/com/shipflow/notificationservice/application/slack/SlackAppService.java:68
- getSlackMessage() 시그니처에 userId가 포함되어 있지만 메서드 본문에서 전혀 사용되지 않습니다. 호출부/Command와의 일관성을 위해 제거하거나, 추후 감사/권한 체크에 사용할 계획이면 TODO와 함께 실제 사용(예: 접근 범위 제한, 로깅 등)을 추가해 주세요.
// 단건 조회
public SlackMessageResult getSlackMessage(UUID userId, String userRole, UUID slackId) {
//권한 확인
validateMasterRole(userRole);
SlackMessage slackMessage = slackMessageRepository.findByIdAndDeletedAtIsNull(slackId)
.orElseThrow(() -> new BusinessException(SlackErrorCode.SLACK_MESSAGE_NOT_FOUND));
return SlackMessageResult.from(slackMessage);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/repository/AiLogRepository.java (1)
9-18: 🛠️ Refactor suggestion | 🟠 Major저장소 인터페이스가 application 계층 DTO에 의존하고 있습니다.
SearchAiLogCommand에는userId/userRole까지 포함돼 있어서, 현재 시그니처는 조회 필터뿐 아니라 권한 문맥도 domain 저장소 API로 끌고 내려옵니다. 검색 조건 전용 타입을 domain 쪽으로 분리하고 역할 검사는 서비스에서 끝내는 편이 계층 경계를 지키기 좋습니다.리팩터링 방향 예시
-import com.shipflow.notificationservice.application.ai.dto.command.SearchAiLogCommand; +import com.shipflow.notificationservice.domain.ai.repository.criteria.AiLogSearchCriteria; ... - Page<AiLog> search(SearchAiLogCommand command, Pageable pageable); + Page<AiLog> search(AiLogSearchCriteria criteria, Pageable pageable);
AiLogSearchCriteria에는 실제 검색 조건만 두고,userId/userRole기반 권한 검사는 application service에서 끝내는 구성이 더 자연스럽습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/repository/AiLogRepository.java` around lines 9 - 18, AiLogRepository currently depends on the application DTO SearchAiLogCommand, which leaks authorization context into the domain layer; create a domain-specific search type (e.g., AiLogSearchCriteria) containing only query/filter fields and change the repository signature Page<AiLog> search(SearchAiLogCriteria criteria, Pageable pageable) in AiLogRepository, then update the application service to map SearchAiLogCommand → AiLogSearchCriteria and perform userId/userRole authorization checks there before calling the repository.notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/repository/SlackMessageRepository.java (1)
9-18: 🛠️ Refactor suggestion | 🟠 Major도메인 레이어의 애플리케이션 DTO 의존을 분리해 주세요.
Line 9, Line 18에서
domain의 Repository가applicationCommand를 참조하고 있어 계층 의존 방향이 역전됩니다. 검색 조건 타입은domain(또는infrastructure전용 포트)으로 이동하는 편이 안전합니다.리팩터링 예시
- import com.shipflow.notificationservice.application.slack.dto.command.SearchSlackMessageCommand; + import com.shipflow.notificationservice.domain.slack.repository.criteria.SlackMessageSearchCriteria; - Page<SlackMessage> search(SearchSlackMessageCommand command, Pageable pageable); + Page<SlackMessage> search(SlackMessageSearchCriteria criteria, Pageable pageable);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/repository/SlackMessageRepository.java` around lines 9 - 18, The repository SlackMessageRepository currently depends on application DTO SearchSlackMessageCommand which inverts layer dependencies; introduce a domain-level search criteria (e.g., SearchSlackMessageCriteria or SlackMessageSearchSpec) inside the domain (or a domain-facing port) and change the repository method signature Page<SlackMessage> search(...) to accept that new type instead of SearchSlackMessageCommand; update any mappers or adapters in the application layer to convert SearchSlackMessageCommand → SearchSlackMessageCriteria and adjust implementations of SlackMessageRepository to consume the domain criteria so the domain layer no longer imports application DTOs.notification-service/src/main/java/com/shipflow/notificationservice/application/ai/AiAppService.java (1)
33-64:⚠️ Potential issue | 🟠 Major트랜잭션 내에서 외부 API 호출로 인한 실패 상태 미저장 및 연결 점유 문제
Gemini 호출이
@Transactional경계 내에 있어 예외 발생 시 전체 트랜잭션이 롤백됩니다.aiLog.markFail()로 상태를 변경해도 DB 플러시 없이 예외가 던져지면 저장되지 않으므로 실패 로그가 기록되지 않습니다. 또한 외부 API 호출 중 DB 커넥션이 점유된 상태로 유지되어 커넥션 풀 효율이 저하됩니다.pending 상태 저장 → 외부 호출 → 상태 업데이트를 분리된 트랜잭션으로 나누고, 외부 호출을
@Transactional외부로 빼주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/application/ai/AiAppService.java` around lines 33 - 64, The generateAiLog method currently performs the external aiGenerator.generate(prompt) call inside a single `@Transactional` method which causes rollbacks to erase aiLog.markFail() and holds DB connections during the external call; fix by persisting the initial AiLog in pending state first (use aiLogRepository.save(aiLog)), then perform the external call outside the transaction (move the aiGenerator.generate(prompt) call into a non-transactional context or a separate method without `@Transactional`), and finally update the AiLog status in a separate transaction (e.g., a method annotated with `@Transactional` or `@Transactional`(propagation = REQUIRES_NEW) that calls aiLog.markSuccess(...) or aiLog.markFail() and saves via aiLogRepository.save(aiLog)); ensure methods referenced (generateAiLog, aiGenerator.generate, aiLogRepository.save, aiLog.markSuccess, aiLog.markFail) are reorganized accordingly so pending gets flushed before the external call and status updates run in their own transactions to release DB connections during the external API request.notification-service/src/main/java/com/shipflow/notificationservice/application/slack/SlackAppService.java (1)
33-58:⚠️ Potential issue | 🔴 CriticalSlack API 호출 실패가 성공으로 반환되고 있습니다.
SlackMessage를 먼저 저장한 뒤 외부 Slack API를 호출하는데,BusinessException은 삼켜서 반환값의sendStatus필드가 FAIL인데도 호출부(SlackController)에서 확인하지 않아 HTTP 200으로 응답됩니다. 그 외 예외는 전체 트랜잭션이 롤백되어 저장된 기록마저 사라집니다. 또한markFail()은sentAt타임스탐프를 설정하지 않아markSuccess()와 불일치합니다.pending 저장 → 외부 호출 → 성공/실패 마킹을 분리하고, 실패는 호출자에게 명시적으로 반환하거나 예외를 발생시켜 실패 감지를 강제해주세요.
🧹 Nitpick comments (9)
notification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.java (1)
156-172: 권한 실패 케이스 테스트를 추가해 주세요.현재는
MASTER성공 경로만 검증해서 권한 회귀를 잡기 어렵습니다.userRole != MASTER일 때BusinessException이 발생하는 테스트 1개를 추가하는 것을 권장합니다.테스트 추가 예시
+ `@Test` + void get_fail_non_master_role() { + UUID id = UUID.randomUUID(); + UUID userId = UUID.randomUUID(); + String userRole = "HUB_MANAGER"; + + assertThatThrownBy(() -> aiAppService.getAiLog(userId, userRole, id)) + .isInstanceOf(BusinessException.class); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.java` around lines 156 - 172, Add a unit test in AiAppServiceTest that verifies getAiLog throws BusinessException when the caller role is not MASTER: mock aiLogRepository.findByIdAndDeletedAtIsNull(id) to return the existing AiLog (as in the successful test), call aiAppService.getAiLog(userId, "SOME_OTHER_ROLE", id) and assert that a BusinessException is thrown; ensure the test targets the getAiLog method and uses the same id/AiLog setup but with userRole != "MASTER" to validate the permission failure path.notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SearchSlackMessageCommand.java (1)
15-16: 검색 기간 역전 입력을 조기 차단하는 검증을 권장합니다.Line 15~16은
createdAtFrom > createdAtTo케이스를 허용하므로, Command 단계에서 불변식을 잡아두면 하위 쿼리/응답 해석이 더 안정적입니다.개선 예시
public record SearchSlackMessageCommand( UUID userId, String userRole, String receiverSlackId, SlackSendStatus sendStatus, SlackMessageType messageType, LocalDateTime createdAtFrom, LocalDateTime createdAtTo ) { + public SearchSlackMessageCommand { + if (createdAtFrom != null && createdAtTo != null && createdAtFrom.isAfter(createdAtTo)) { + throw new IllegalArgumentException("createdAtFrom은 createdAtTo보다 이후일 수 없습니다."); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SearchSlackMessageCommand.java` around lines 15 - 16, SearchSlackMessageCommand currently allows createdAtFrom > createdAtTo which can lead to invalid queries; add a validation in the command (e.g., in the constructor or a `@PostConstruct/validate` method of SearchSlackMessageCommand) that checks if createdAtFrom and createdAtTo are both non-null and throws IllegalArgumentException (or a custom ValidationException) when createdAtFrom.isAfter(createdAtTo), ensuring the invariant is enforced at the command level before any query/handler logic runs; reference the createdAtFrom and createdAtTo fields and the SearchSlackMessageCommand constructor/factory to locate where to add this check.notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderInternalClient.java (1)
9-13: Feign 클라이언트에 resilience 패턴 적용 권장외부 서비스(
orderservice) 호출 시 장애 전파를 방지하기 위해 fallback 또는 circuit breaker 설정을 고려해 주세요. 현재 구현은 order-service 장애 시 notification-service로 장애가 전파될 수 있습니다.♻️ Fallback 적용 예시
-@FeignClient(name = "orderservice") +@FeignClient(name = "orderservice", fallback = OrderInternalClientFallback.class) public interface OrderInternalClient { `@GetMapping`("/internal/orders/{orderId}/read-model") OrderReadModelResponse getOrderReadModel(`@PathVariable` UUID orderId); }별도의
OrderInternalClientFallback클래스를 생성하여 fallback 로직을 구현하거나,application.yml에 Feign timeout 및 retry 설정을 추가하세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderInternalClient.java` around lines 9 - 13, Add a resilience layer to the Feign client to avoid cascading failures: create a fallback implementation class (e.g., OrderInternalClientFallback) that implements OrderInternalClient and provides a safe default for getOrderReadModel(UUID orderId), then register it with the FeignClient annotation (or with a Feign builder) and wire any needed bean; alternatively, configure circuit breaker/timeout/retry policies (e.g., Resilience4j or Spring Cloud Circuit Breaker) around the OrderInternalClient bean and tune Feign timeouts/retries in application.yml so orderservice failures don’t propagate to notification-service.notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java (2)
75-81: 구체적인 검증 추가 권장현재 테스트는
assertThatNoException()만 사용하여 예외가 발생하지 않는지만 검증합니다. 더 구체적인 assertion을 추가하면 테스트 품질이 향상됩니다.♻️ 구체적인 검증 예시
// when & then assertThatNoException() .isThrownBy(() -> notificationOrchestratorService.handleShipmentCreated(event)); verify(orderInternalClient).getOrderReadModel(event.getOrderId()); verify(aiAppService).generateAiLog(any()); - verify(slackAppService).sendSlackMessage(any()); + verify(slackAppService).sendSlackMessage(argThat(cmd -> + cmd.receiverSlackId().equals("U123SLACK") && + cmd.messageType() == SlackMessageType.DEADLINE_ALERT + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java` around lines 75 - 81, The test currently only asserts no exception; enhance it by adding specific verifications and assertions: assert the expected interactions on NotificationOrchestratorServiceTest by verifying notificationOrchestratorService.handleShipmentCreated invoked the expected collaborators (retain verify(orderInternalClient).getOrderReadModel(event.getOrderId()), verify(aiAppService).generateAiLog(any()), verify(slackAppService).sendSlackMessage(any())) and add stricter checks—e.g., verify generateAiLog and sendSlackMessage are called with payloads derived from event/order (use argument captors to assert fields on the captured objects) and optionally verify invocation counts or order if important (verify(..., times(1)) or inOrder on aiAppService and slackAppService).
45-104: AI 생성 실패 시나리오 테스트 추가 고려현재 테스트는 order 조회 실패 시나리오는 있지만,
aiAppService.generateAiLog()호출 실패 시 동작을 검증하는 테스트가 없습니다. 오케스트레이션 서비스의 resilience를 보장하려면 AI 실패 시나리오도 테스트하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java` around lines 45 - 104, Add a unit test for notificationOrchestratorService.handleShipmentCreated that simulates aiAppService.generateAiLog throwing an exception: mock orderInternalClient.getOrderReadModel(...) to return a valid OrderReadModelResponse, mock aiAppService.generateAiLog(any()) to throw(new RuntimeException("AI failure")), keep aiLogRepository.findByIdAndDeletedAtIsNull(...) behavior as needed, then assertThatNoException().isThrownBy(() -> notificationOrchestratorService.handleShipmentCreated(event)) and verify orderInternalClient.getOrderReadModel(...) was called and that slackAppService.sendSlackMessage(...) was still invoked (ensuring the orchestrator falls back and continues despite AI failure).notification-service/src/main/java/com/shipflow/notificationservice/config/JPAConfig.java (1)
43-58: 불필요한 예외 throw-catch 패턴 개선 권장Line 53에서
IllegalStateException을 던지고 Line 56에서 바로 catch하여SYSTEM_UUID를 반환하는 패턴은 비효율적입니다. 또한 catch-allException으로 UUID 파싱 오류 등 모든 예외가 무시되어 디버깅이 어려워질 수 있습니다.♻️ 개선된 구현
`@Bean` public AuditorAware<UUID> auditorAware() { return () -> { - try { - ServletRequestAttributes attrs = - (ServletRequestHolder.getRequestAttributes(); - - // 요청 컨텍스트 없는 경우만 SYSTEM_UUID (RabbitMQ 등) - if (attrs == null) - return Optional.of(SYSTEM_UUID); - - String userId = attrs.getRequest().getHeader("X-User-Id"); - if (userId == null || userId.isBlank()) - throw new IllegalStateException("X-User-Id 헤더가 없습니다."); - - return Optional.of(UUID.fromString(userId)); - } catch (Exception e) { - return Optional.of(SYSTEM_UUID); - } + ServletRequestAttributes attrs = + (ServletRequestAttributes)RequestContextHolder.getRequestAttributes(); + + if (attrs == null) { + return Optional.of(SYSTEM_UUID); + } + + String userId = attrs.getRequest().getHeader("X-User-Id"); + if (userId == null || userId.isBlank()) { + return Optional.of(SYSTEM_UUID); + } + + try { + return Optional.of(UUID.fromString(userId)); + } catch (IllegalArgumentException e) { + // 잘못된 UUID 형식 로깅 권장 + return Optional.of(SYSTEM_UUID); + } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/config/JPAConfig.java` around lines 43 - 58, The current code in JPAConfig uses an unnecessary throw-then-catch and a broad catch(Exception); instead, remove the IllegalStateException and handle cases explicitly: after obtaining ServletRequestAttributes from RequestContextHolder, if attrs is null return Optional.of(SYSTEM_UUID); read the "X-User-Id" header and if it is null or blank return Optional.of(SYSTEM_UUID) (do not throw); attempt to parse UUID.fromString(userId) inside a small try-catch that only catches IllegalArgumentException (or DateTimeParseException if used) and on parse failure return Optional.of(SYSTEM_UUID); avoid catching Exception broadly and keep references to ServletRequestAttributes, RequestContextHolder, UUID.fromString, and SYSTEM_UUID so the change is localized.notification-service/src/test/java/com/shipflow/notificationservice/SlackAppServiceTest.java (1)
59-61: 다양한 역할에 대한 테스트 케이스 추가 고려현재 모든 테스트가
"MASTER"역할만 사용합니다. 서비스 레이어에validateMasterRole검증이 있다면, 권한이 없는 역할로 호출 시 예외가 발생하는 케이스도 테스트하면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/test/java/com/shipflow/notificationservice/SlackAppServiceTest.java` around lines 59 - 61, Tests in SlackAppServiceTest currently only use the "MASTER" role; add cases to assert behavior for non-MASTER roles and the authorization check in validateMasterRole. Add at least one test that sets userRole to a non-master value (e.g., "USER" or "GUEST") and verifies the service method under test (the same method(s) currently exercised in SlackAppServiceTest) throws the expected exception or returns the expected error, and another test that ensures validateMasterRole enforces access by calling it directly or via the public service method and asserting the exception type/message; reference SlackAppServiceTest and the validateMasterRole logic when locating where to add these tests.notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/dto/request/GenerateDeadlineRequest.java (1)
20-20:quantity는 DTO에서@Positive로 막는 편이 낫습니다.지금은
0이나 음수가 컨트롤러 validation을 통과한 뒤 서비스에서AI_EVENT_INVALID로 떨어집니다. 요청 스키마에서 바로 거르는 쪽이 API 계약이 더 명확합니다.🔧 수정 예시
import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; @@ - `@NotNull` Integer quantity, + `@NotNull` `@Positive` Integer quantity,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/dto/request/GenerateDeadlineRequest.java` at line 20, 요약: GenerateDeadlineRequest의 quantity 필드는 컨트롤러 단에서 0 또는 음수를 차단하도록 `@Positive` 검증을 추가해야 합니다 — 수정: GenerateDeadlineRequest 클래스에서 quantity 선언에 `@Positive` 애노테이션을 추가하거나 `@NotNull` 대신 `@NotNull` `@Positive` 조합을 사용하고 javax.validation.constraints.Positive를 임포트하여 요청 스키마 레벨에서 1 이상의 값만 허용되게 만드세요.notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageRepositoryImpl.java (1)
53-58:Pageable의sort가 지금은 전혀 반영되지 않습니다.현재는
offset/limit만 쓰고orderBy(createdAt.desc())를 고정해서 호출자의 정렬 조건이 모두 버려집니다. 정렬을 지원할 계획이면pageable.getSort()를 QueryDSLorderBy로 풀고, 고정 정렬만 허용할 거면 API에서sort를 막는 편이 계약이 명확합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageRepositoryImpl.java` around lines 53 - 58, The current query always uses orderBy(slackMessage.createdAt.desc()) and ignores Pageable.getSort(); update SlackMessageRepositoryImpl so the QueryDSL orderBy honors pageable.getSort() by converting Pageable.getSort() into QueryDSL OrderSpecifier(s) (mapping property names to slackMessage fields, e.g., createdAt) and pass them to queryFactory.selectFrom(slackMessage).where(builder).orderBy(...).offset(...).limit(...); if implementing sort mapping is not desired, explicitly reject or strip incoming Sort in the API/Service layer (or validate Pageable) so callers cannot provide custom sorts—choose one approach and implement it around the query construction that currently references slackMessage, builder, pageable and createdAt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java`:
- Around line 43-57: The code currently uses resolveSlackId(event) and will send
a message even when slackId is missing by substituting "확인 필요"; instead, check
the slackId returned by resolveSlackId (and similarly where used around lines
116-119) and if it is null/empty, do not call slackAppService.sendSlackMessage
or construct a SendSlackMessageCommand — instead skip sending and update/leave
the notification state to FAIL or PENDING as appropriate (invoke whatever status
update path you use in NotificationOrchestratorService), ensuring no placeholder
string like "확인 필요" is used as a Slack identifier.
- Around line 63-67: The calls to markSlackSendSuccess and markSlackSendFail
from handleShipmentCreated are self-invocations so their `@Transactional` (e.g.,
`@Transactional`(propagation = Propagation.REQUIRES_NEW)) is not applied; fix by
moving those methods into a separate Spring bean (e.g., SlackSendAuditService)
and inject that bean into NotificationOrchestratorService and call
slackSendAuditService.markSlackSendSuccess(aiId)/markSlackSendFail(aiId), or
alternatively wrap the calls with TransactionTemplate inside
handleShipmentCreated or obtain the proxied instance
(applicationContext.getBean(NotificationOrchestratorService.class)) and call the
methods via that proxy so the REQUIRES_NEW transaction semantics on
markSlackSendSuccess/Fail are actually honored.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedListener.java`:
- Around line 15-17: ShipmentCreatedListener.onShipmentCreated currently lacks
the `@RabbitListener` annotation so it never receives messages; either add
`@RabbitListener`(queues =
NotificationRabbitConfig.QUEUE_NOTIFICATION_SHIPMENT_CREATED) to the
onShipmentCreated(ShipmentCreatedEvent event) method so this class actually
consumes messages and delegates to shipmentCreatedHandler.handle(event), or
remove the dead ShipmentCreatedListener class and keep the existing
`@RabbitListener` on ShipmentCreatedHandler (ensure only one listener remains and
tests/configs reference the retained class).
In
`@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/dto/ShipmentCreatedEvent.java`:
- Around line 33-36: The publisher (OrderMessageFlowTest / the OrderMessage
publisher) is not populating the new ShipmentCreatedEvent fields
shipmentManagerSlackId and routes, so notification-service's auto-alert routing
cannot work; update the code that constructs ShipmentCreatedEvent (the
constructor call in OrderMessageFlowTest and any publisher factory methods) to
pass a valid shipmentManagerSlackId string and a non-empty List<RouteInfo> (or
realistic test RouteInfo instances), and if the ShipmentCreatedEvent
constructors/signatures do not accept these new params update the
constructor/overloads accordingly so the publisher actually sets
shipmentManagerSlackId and routes rather than relying on notification-service
defaults.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/external/AiController.java`:
- Around line 45-56: Change the X-User-Id handler parameters in AiController
methods (e.g., getAiLog, getAiLogs, and the shown method using
aiAppService.generateAiLog) from String to UUID by declaring
`@RequestHeader`("X-User-Id") UUID userId so Spring will validate/convert headers
and return 400 for bad UUIDs; remove manual UUID.fromString(userId) calls and
pass userId directly to request.toCommand(...) (or other methods) and adjust any
imports/signatures accordingly.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/presentation/common/NotificationExceptionHandler.java`:
- Around line 19-26: The current handler method handleIllegalArgument that
catches IllegalArgumentException is too broad and can mask internal bugs; change
it to handle a dedicated validation exception (e.g., create and throw
ValidationException) or narrow the handler to only controller-layer validation
errors so genuine programming errors are not mapped to
CommonErrorCode.VALIDATION_ERROR; update the `@ExceptionHandler` to reference
ValidationException (or add an additional handler for ValidationException and
remove/limit the IllegalArgumentException handler) and ensure
controllers/services throw ValidationException for input validation failures
rather than IllegalArgumentException.
- Around line 22-25: NotificationExceptionHandler currently returns
e.getMessage() to the client and only logs a short warn line; change the
IllegalArgumentException handler to return a generic client-facing message
(e.g., "Invalid request parameters") via
ApiResponse.fail(CommonErrorCode.VALIDATION_ERROR, "<generic message>",
request.getRequestURI()) instead of e.getMessage(), and log the full exception
(including stack trace) on the server side using the logger (e.g.,
logger.warn("IllegalArgumentException while handling request {}",
request.getRequestURI(), e)) so debugging retains details but sensitive
internals are not exposed to clients.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/SendSlackMessageRequest.java`:
- Around line 14-16: Update the Slack ID validation in SendSlackMessageRequest
for the receiverSlackId field to accept Enterprise user IDs by adding the 'W'
prefix to the pattern; specifically modify the `@Pattern` regexp from
"^[UCDG][A-Z0-9]+$" to include W (e.g., "^[UCDGW][A-Z0-9]+$") so Enterprise IDs
starting with 'W' are validated correctly.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/external/SlackController.java`:
- Around line 44-50: Replace manual UUID parsing in SlackController with
Spring's native conversion by changing method parameters from
`@RequestHeader`("X-User-Id") String userId to `@RequestHeader`("X-User-Id") UUID
userId and remove all UUID.fromString(userId) usages (e.g., in the
sendSlackMessage call where request.toCommand(UUID.fromString(userId), userRole)
is used); apply the same change to the other endpoints in SlackController (the
handlers at the locations corresponding to lines 64, 87, 112, 125) so each
handler accepts UUID userId directly and passes that UUID into toCommand /
service methods (keep X-User-Role as String).
In `@notification-service/src/main/resources/application.yaml`:
- Around line 24-27: Replace the hard-coded Redis host/port in application.yaml
with environment-variable placeholders so the container can connect to an
external Redis; specifically change data.redis.host and data.redis.port to read
from e.g. REDIS_HOST and REDIS_PORT using Spring-style placeholders (with
sensible defaults if desired) so runtime environment controls the Redis endpoint
used by the idempotency logic.
In
`@notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java`:
- Around line 182-197: The comment above createEvent currently says
"reflection으로 필드 세팅" but the method actually instantiates a ShipmentCreatedEvent
via its constructor; update or remove the misleading comment so it matches the
implementation (modify the comment above the createEvent method or delete it),
referencing the createEvent method and the ShipmentCreatedEvent constructor
usage to keep the code and comment consistent.
---
Outside diff comments:
In
`@notification-service/src/main/java/com/shipflow/notificationservice/application/ai/AiAppService.java`:
- Around line 33-64: The generateAiLog method currently performs the external
aiGenerator.generate(prompt) call inside a single `@Transactional` method which
causes rollbacks to erase aiLog.markFail() and holds DB connections during the
external call; fix by persisting the initial AiLog in pending state first (use
aiLogRepository.save(aiLog)), then perform the external call outside the
transaction (move the aiGenerator.generate(prompt) call into a non-transactional
context or a separate method without `@Transactional`), and finally update the
AiLog status in a separate transaction (e.g., a method annotated with
`@Transactional` or `@Transactional`(propagation = REQUIRES_NEW) that calls
aiLog.markSuccess(...) or aiLog.markFail() and saves via
aiLogRepository.save(aiLog)); ensure methods referenced (generateAiLog,
aiGenerator.generate, aiLogRepository.save, aiLog.markSuccess, aiLog.markFail)
are reorganized accordingly so pending gets flushed before the external call and
status updates run in their own transactions to release DB connections during
the external API request.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/domain/ai/repository/AiLogRepository.java`:
- Around line 9-18: AiLogRepository currently depends on the application DTO
SearchAiLogCommand, which leaks authorization context into the domain layer;
create a domain-specific search type (e.g., AiLogSearchCriteria) containing only
query/filter fields and change the repository signature Page<AiLog>
search(SearchAiLogCriteria criteria, Pageable pageable) in AiLogRepository, then
update the application service to map SearchAiLogCommand → AiLogSearchCriteria
and perform userId/userRole authorization checks there before calling the
repository.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/domain/slack/repository/SlackMessageRepository.java`:
- Around line 9-18: The repository SlackMessageRepository currently depends on
application DTO SearchSlackMessageCommand which inverts layer dependencies;
introduce a domain-level search criteria (e.g., SearchSlackMessageCriteria or
SlackMessageSearchSpec) inside the domain (or a domain-facing port) and change
the repository method signature Page<SlackMessage> search(...) to accept that
new type instead of SearchSlackMessageCommand; update any mappers or adapters in
the application layer to convert SearchSlackMessageCommand →
SearchSlackMessageCriteria and adjust implementations of SlackMessageRepository
to consume the domain criteria so the domain layer no longer imports application
DTOs.
---
Nitpick comments:
In
`@notification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SearchSlackMessageCommand.java`:
- Around line 15-16: SearchSlackMessageCommand currently allows createdAtFrom >
createdAtTo which can lead to invalid queries; add a validation in the command
(e.g., in the constructor or a `@PostConstruct/validate` method of
SearchSlackMessageCommand) that checks if createdAtFrom and createdAtTo are both
non-null and throws IllegalArgumentException (or a custom ValidationException)
when createdAtFrom.isAfter(createdAtTo), ensuring the invariant is enforced at
the command level before any query/handler logic runs; reference the
createdAtFrom and createdAtTo fields and the SearchSlackMessageCommand
constructor/factory to locate where to add this check.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/config/JPAConfig.java`:
- Around line 43-58: The current code in JPAConfig uses an unnecessary
throw-then-catch and a broad catch(Exception); instead, remove the
IllegalStateException and handle cases explicitly: after obtaining
ServletRequestAttributes from RequestContextHolder, if attrs is null return
Optional.of(SYSTEM_UUID); read the "X-User-Id" header and if it is null or blank
return Optional.of(SYSTEM_UUID) (do not throw); attempt to parse
UUID.fromString(userId) inside a small try-catch that only catches
IllegalArgumentException (or DateTimeParseException if used) and on parse
failure return Optional.of(SYSTEM_UUID); avoid catching Exception broadly and
keep references to ServletRequestAttributes, RequestContextHolder,
UUID.fromString, and SYSTEM_UUID so the change is localized.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderInternalClient.java`:
- Around line 9-13: Add a resilience layer to the Feign client to avoid
cascading failures: create a fallback implementation class (e.g.,
OrderInternalClientFallback) that implements OrderInternalClient and provides a
safe default for getOrderReadModel(UUID orderId), then register it with the
FeignClient annotation (or with a Feign builder) and wire any needed bean;
alternatively, configure circuit breaker/timeout/retry policies (e.g.,
Resilience4j or Spring Cloud Circuit Breaker) around the OrderInternalClient
bean and tune Feign timeouts/retries in application.yml so orderservice failures
don’t propagate to notification-service.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageRepositoryImpl.java`:
- Around line 53-58: The current query always uses
orderBy(slackMessage.createdAt.desc()) and ignores Pageable.getSort(); update
SlackMessageRepositoryImpl so the QueryDSL orderBy honors pageable.getSort() by
converting Pageable.getSort() into QueryDSL OrderSpecifier(s) (mapping property
names to slackMessage fields, e.g., createdAt) and pass them to
queryFactory.selectFrom(slackMessage).where(builder).orderBy(...).offset(...).limit(...);
if implementing sort mapping is not desired, explicitly reject or strip incoming
Sort in the API/Service layer (or validate Pageable) so callers cannot provide
custom sorts—choose one approach and implement it around the query construction
that currently references slackMessage, builder, pageable and createdAt.
In
`@notification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/dto/request/GenerateDeadlineRequest.java`:
- Line 20: 요약: GenerateDeadlineRequest의 quantity 필드는 컨트롤러 단에서 0 또는 음수를 차단하도록
`@Positive` 검증을 추가해야 합니다 — 수정: GenerateDeadlineRequest 클래스에서 quantity 선언에
`@Positive` 애노테이션을 추가하거나 `@NotNull` 대신 `@NotNull` `@Positive` 조합을 사용하고
javax.validation.constraints.Positive를 임포트하여 요청 스키마 레벨에서 1 이상의 값만 허용되게 만드세요.
In
`@notification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.java`:
- Around line 156-172: Add a unit test in AiAppServiceTest that verifies
getAiLog throws BusinessException when the caller role is not MASTER: mock
aiLogRepository.findByIdAndDeletedAtIsNull(id) to return the existing AiLog (as
in the successful test), call aiAppService.getAiLog(userId, "SOME_OTHER_ROLE",
id) and assert that a BusinessException is thrown; ensure the test targets the
getAiLog method and uses the same id/AiLog setup but with userRole != "MASTER"
to validate the permission failure path.
In
`@notification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.java`:
- Around line 75-81: The test currently only asserts no exception; enhance it by
adding specific verifications and assertions: assert the expected interactions
on NotificationOrchestratorServiceTest by verifying
notificationOrchestratorService.handleShipmentCreated invoked the expected
collaborators (retain
verify(orderInternalClient).getOrderReadModel(event.getOrderId()),
verify(aiAppService).generateAiLog(any()),
verify(slackAppService).sendSlackMessage(any())) and add stricter checks—e.g.,
verify generateAiLog and sendSlackMessage are called with payloads derived from
event/order (use argument captors to assert fields on the captured objects) and
optionally verify invocation counts or order if important (verify(..., times(1))
or inOrder on aiAppService and slackAppService).
- Around line 45-104: Add a unit test for
notificationOrchestratorService.handleShipmentCreated that simulates
aiAppService.generateAiLog throwing an exception: mock
orderInternalClient.getOrderReadModel(...) to return a valid
OrderReadModelResponse, mock aiAppService.generateAiLog(any()) to throw(new
RuntimeException("AI failure")), keep
aiLogRepository.findByIdAndDeletedAtIsNull(...) behavior as needed, then
assertThatNoException().isThrownBy(() ->
notificationOrchestratorService.handleShipmentCreated(event)) and verify
orderInternalClient.getOrderReadModel(...) was called and that
slackAppService.sendSlackMessage(...) was still invoked (ensuring the
orchestrator falls back and continues despite AI failure).
In
`@notification-service/src/test/java/com/shipflow/notificationservice/SlackAppServiceTest.java`:
- Around line 59-61: Tests in SlackAppServiceTest currently only use the
"MASTER" role; add cases to assert behavior for non-MASTER roles and the
authorization check in validateMasterRole. Add at least one test that sets
userRole to a non-master value (e.g., "USER" or "GUEST") and verifies the
service method under test (the same method(s) currently exercised in
SlackAppServiceTest) throws the expected exception or returns the expected
error, and another test that ensures validateMasterRole enforces access by
calling it directly or via the public service method and asserting the exception
type/message; reference SlackAppServiceTest and the validateMasterRole logic
when locating where to add these tests.
🪄 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: 1b6f5086-12a1-4309-8a06-908f4ddce377
📒 Files selected for processing (47)
.gitignorecommon/src/main/java/com/shipflow/common/domain/BaseEntity.javadocker-compose.ymlkeycloak/shipflow-export.jsonnotification-service/build.gradlenotification-service/src/main/java/com/shipflow/notificationservice/NotificationserviceApplication.javanotification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.javanotification-service/src/main/java/com/shipflow/notificationservice/application/ai/AiAppService.javanotification-service/src/main/java/com/shipflow/notificationservice/application/ai/dto/command/GenerateDeadlineCommand.javanotification-service/src/main/java/com/shipflow/notificationservice/application/ai/dto/command/SearchAiLogCommand.javanotification-service/src/main/java/com/shipflow/notificationservice/application/slack/SlackAppService.javanotification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SearchSlackMessageCommand.javanotification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/SendSlackMessageCommand.javanotification-service/src/main/java/com/shipflow/notificationservice/application/slack/dto/command/UpdateSlackMessageCommand.javanotification-service/src/main/java/com/shipflow/notificationservice/config/JPAConfig.javanotification-service/src/main/java/com/shipflow/notificationservice/config/RedisConfig.javanotification-service/src/main/java/com/shipflow/notificationservice/config/SwaggerConfig.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/ai/AiLog.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/ai/exception/AiErrorCode.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/ai/repository/AiLogRepository.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/slack/SlackMessage.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/slack/exception/SlackErrorCode.javanotification-service/src/main/java/com/shipflow/notificationservice/domain/slack/repository/SlackMessageRepository.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/ai/GeminiApiClient.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/ai/config/GeminiApiConfig.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderInternalClient.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/client/order/OrderReadModelResponse.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedConsumer.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedHandler.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedListener.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/dto/ShipmentCreatedEvent.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/ai/AiLogJpaRepository.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/ai/AiLogRepositoryImpl.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageJpaRepository.javanotification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/slack/SlackMessageRepositoryImpl.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/dto/request/GenerateDeadlineRequest.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/ai/external/AiController.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/common/BasePageRequest.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/common/NotificationExceptionHandler.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/SendSlackMessageRequest.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/UpdateSlackMessageRequest.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/response/SlackMessageResponse.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/external/SlackController.javanotification-service/src/main/resources/application.yamlnotification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.javanotification-service/src/test/java/com/shipflow/notificationservice/NotificationOrchestratorServiceTest.javanotification-service/src/test/java/com/shipflow/notificationservice/SlackAppServiceTest.java
💤 Files with no reviewable changes (3)
- notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/persistence/ai/AiLogJpaRepository.java
- notification-service/src/main/java/com/shipflow/notificationservice/infrastructure/messaging/consumer/ShipmentCreatedConsumer.java
- notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/response/SlackMessageResponse.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java (1)
50-50:⚠️ Potential issue | 🔴 Critical
@Transactional이 self-invocation으로 적용되지 않습니다.Line 50/73/76의 동일 클래스 내부 호출은 프록시를 우회합니다. 그래서 Line 208/215의 트랜잭션 경계가 보장되지 않아
AiLog상태 변경이 의도대로 커밋되지 않을 수 있습니다. 상태 업데이트 메서드를 별도 빈으로 분리해 호출하세요.수정 예시 (별도 트랜잭션 빈 분리)
+@Service +@RequiredArgsConstructor +public class AiLogStatusService { + private final AiLogRepository aiLogRepository; + + `@Transactional` + public void markSlackSendSuccess(UUID aiLogId) { + AiLog aiLog = aiLogRepository.findByIdAndDeletedAtIsNull(aiLogId) + .orElseThrow(() -> new BusinessException(AiErrorCode.AI_LOG_NOT_FOUND)); + aiLog.markSendSuccess(); + } + + `@Transactional` + public void markSlackSendFail(UUID aiLogId) { + AiLog aiLog = aiLogRepository.findByIdAndDeletedAtIsNull(aiLogId) + .orElseThrow(() -> new BusinessException(AiErrorCode.AI_LOG_NOT_FOUND)); + aiLog.markSendFail(); + } +}public class NotificationOrchestratorService { + private final AiLogStatusService aiLogStatusService; ... - markSlackSendFail(aiResult.aiId()); + aiLogStatusService.markSlackSendFail(aiResult.aiId()); ... - markSlackSendSuccess(aiResult.aiId()); + aiLogStatusService.markSlackSendSuccess(aiResult.aiId()); ... - markSlackSendFail(aiResult.aiId()); + aiLogStatusService.markSlackSendFail(aiResult.aiId());#!/bin/bash set -euo pipefail FILE=$(fd -i "NotificationOrchestratorService.java" | head -n1) echo "Target file: $FILE" echo "== Calls to status update methods inside orchestrator ==" rg -n "markSlackSend(Success|Fail)\(" "$FILE" echo echo "== Transactional annotations on those methods ==" rg -n "@Transactional|void markSlackSend(Success|Fail)\(" "$FILE" echo echo "== Relevant code window ==" nl -ba "$FILE" | sed -n '40,225p'Also applies to: 73-77, 208-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java` at line 50, NotificationOrchestratorService is self-invoking transactional methods (e.g., markSlackSendFail and markSlackSendSuccess) so `@Transactional` is bypassed; extract these state-update routines into a separate Spring bean (e.g., NotificationStatusUpdater) with `@Component/`@Service and `@Transactional` on its methods (retain method names like markSlackSendFail(AiId) / markSlackSendSuccess(...)), inject that bean into NotificationOrchestratorService and replace internal calls (the current self-calls to markSlackSendFail/markSlackSendSuccess) with calls to the injected updater to ensure transaction boundaries are applied and AiLog state changes commit as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java`:
- Around line 118-122: In NotificationOrchestratorService, do not swallow all
exceptions around orderInternalClient.getOrderReadModel(orderId); replace the
broad try-catch that returns null with a clear policy: catch only expected
exceptions (e.g., transient network errors) and perform a bounded retry with
exponential backoff, log a WARN or ERROR with context (orderId) and the
exception stack for every failure, and for non-recoverable errors rethrow or
return an explicit failure type (e.g., Optional/Result) instead of null so
callers can distinguish UNKNOWN vs hard failure; update the method surrounding
getOrderReadModel to use these specific exception types, retry logic, and
detailed logging.
---
Duplicate comments:
In
`@notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.java`:
- Line 50: NotificationOrchestratorService is self-invoking transactional
methods (e.g., markSlackSendFail and markSlackSendSuccess) so `@Transactional` is
bypassed; extract these state-update routines into a separate Spring bean (e.g.,
NotificationStatusUpdater) with `@Component/`@Service and `@Transactional` on its
methods (retain method names like markSlackSendFail(AiId) /
markSlackSendSuccess(...)), inject that bean into
NotificationOrchestratorService and replace internal calls (the current
self-calls to markSlackSendFail/markSlackSendSuccess) with calls to the injected
updater to ensure transaction boundaries are applied and AiLog state changes
commit as intended.
🪄 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: 5c95bcc9-882f-4714-9c35-4fbb71475228
📒 Files selected for processing (5)
notification-service/src/main/java/com/shipflow/notificationservice/application/NotificationOrchestratorService.javanotification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/SendSlackMessageRequest.javanotification-service/src/main/resources/application.yamlnotification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.javanotification-service/src/test/java/com/shipflow/notificationservice/NotificationserviceApplicationTests.java
💤 Files with no reviewable changes (1)
- notification-service/src/test/java/com/shipflow/notificationservice/NotificationserviceApplicationTests.java
✅ Files skipped from review due to trivial changes (1)
- notification-service/src/test/java/com/shipflow/notificationservice/AiAppServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- notification-service/src/main/java/com/shipflow/notificationservice/presentation/slack/dto/request/SendSlackMessageRequest.java
- notification-service/src/main/resources/application.yaml
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
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 제목
[FEATURE] [Notification] AI-Slack 이벤트 연동 및 Slack API 개선 #30
✨ 작업 내용
Notification 서비스의 AI/Slack 핵심 로직 구현 및 Gateway 연동, 트랜잭션 경계 개선
🔍 상세 내용
ShipmentCreatedEvent) 수신 후 AI 발송 시한 계산 및 Slack 알림 발송 플로우 구현NotificationOrchestratorService클래스 레벨@Transactional제거 → 외부 API 호출(Gemini, Slack) 중 DB 커넥션 점유 방지GeminiApiConfigWebClient 빈 이름geminiWebClient로 명시 → 빈 충돌 방지JPAConfigAuditorAware를X-User-Id헤더 기반으로 수정 →createdBy,updatedBy자동 처리OrderInternalClientFeignClient nameorderservice로 수정 (Eureka 등록명 일치)🔗 관련 이슈
Closes #30
docker-compose.ymlnotificationservice에 RabbitMQ 환경변수(RABBITMQ_HOST,RABBITMQ_PORT,RABBITMQ_USERNAME,RABBITMQ_PASSWORD) 명시적 추가로 수정 했습니다. 확인 부탁드립니다.✅ 체크리스트
Summary by CodeRabbit
릴리스 노트
New Features
Refactor
Chores
Tests