feat: 전체 참여자 입금 승인 시 정산 자동 완료 처리 및 캐릭터 자동 지급 기능 추가#34
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough정산 완료 검사와 아웃박스 기반 비동기 퍼블리시/태스크 파이프라인을 추가했습니다. 정산이 모두 지급되면 SettlementCompletionProcessor가 완료 처리 후 아웃박스 이벤트를 생성하고, 아웃박스 퍼블리셔·실행기·태스크 생성·스케줄러·태스크 처리기로 리워드·알림 작업을 비동기 처리합니다. Changes
Sequence DiagramsequenceDiagram
participant Client as 결제 승인 요청자
participant CommandPaymentRequest as CommandPaymentRequest
participant SettlementCompletionProcessor as SettlementCompletionProcessor
participant MemberReader as MemberReader
participant SettlementUpdater as SettlementUpdater
participant CommandOutboxEventService as CommandOutboxEventService
participant OutboxEventPublisher as OutboxEventPublisher
participant OutboxEventPublishExecutor as OutboxEventPublishExecutor
participant EventTaskCreator as EventTaskCreator
participant EventTaskScheduler as EventTaskScheduler
participant CommandEventTaskService as CommandEventTaskService
participant RewardService as RewardService
Client->>CommandPaymentRequest: approvePaymentRequest(settlementId)
CommandPaymentRequest->>SettlementCompletionProcessor: completeIfAllPaid(settlementId)
SettlementCompletionProcessor->>MemberReader: existsUnpaidMember(settlementId)
alt unpaid member exists
MemberReader-->>SettlementCompletionProcessor: true
SettlementCompletionProcessor-->>CommandPaymentRequest: false
else all paid
MemberReader-->>SettlementCompletionProcessor: false
SettlementCompletionProcessor->>SettlementUpdater: complete(settlementId)
SettlementUpdater-->>SettlementCompletionProcessor: true
SettlementCompletionProcessor->>CommandOutboxEventService: create(SETTLEMENT_COMPLETED, SETTLEMENT, settlementId)
CommandOutboxEventService-->>SettlementCompletionProcessor: OutboxEvent(id)
SettlementCompletionProcessor-->>CommandPaymentRequest: true
Note over OutboxEventPublisher,OutboxEventPublishExecutor: 비동기 퍼블리시·태스크 생성 흐름
OutboxEventPublisher->>OutboxEventPublishExecutor: publish(outboxEventId)
OutboxEventPublishExecutor->>MemberReader: findAssignedMembersBySettlementId(settlementId)
OutboxEventPublishExecutor->>EventTaskCreator: create(...) [per member -> REWARD_GRANT/NOTIFICATION_SEND]
EventTaskScheduler->>CommandEventTaskService: process(eventTaskId)
CommandEventTaskService->>RewardService: grant(settlementId, userId)
RewardService-->>CommandEventTaskService: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📝 테스트 커버리지 리포트입니다!
|
📝 테스트 커버리지 리포트입니다!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
src/main/java/com/dnd/moddo/event/application/impl/MemberReader.java (1)
48-51: 할당 멤버 필터링은 DB 레벨로 내리는 것을 권장합니다.
현재는 전체 멤버를 읽은 뒤 메모리에서 거르므로, 멤버 수가 늘면 불필요한 로딩 비용이 커집니다.♻️ 제안 수정안
public List<Member> findAssignedMembersBySettlementId(Long settlementId) { - return findAllBySettlementId(settlementId).stream() - .filter(Member::isAssigned) - .toList(); + return memberRepository.findAllBySettlementIdAndIsAssignedTrue(settlementId); }// MemberRepository 측(별도 파일) 예시 List<Member> findAllBySettlementIdAndIsAssignedTrue(Long settlementId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/event/application/impl/MemberReader.java` around lines 48 - 51, The current MemberReader.findAssignedMembersBySettlementId loads all members then filters by isAssigned in memory; change it to push the filter to the DB by adding a repository query (e.g., add List<Member> findAllBySettlementIdAndIsAssignedTrue(Long settlementId) to MemberRepository) and have MemberReader call that repository method instead of findAllBySettlementId(...). Ensure the repository method name or a `@Query` equivalent is used so only assigned members are fetched from the database.src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventTaskAppenderTest.java (1)
54-57: 저장 순서 고정 단정은 테스트를 불안정하게 만들 수 있습니다.현재 검증은 인덱스 순서에 강하게 결합되어 있어, 구현 리팩터링(저장 순서 변경)만으로도 실패할 수 있습니다. 의미 기반(타입+대상 유저)으로 순서 비의존 검증을 권장합니다.
수정 예시
- List<EventTask> savedTasks = captor.getAllValues(); - verifySavedTask(savedTasks.get(0), outboxEvent, EventTaskType.REWARD_GRANT, 10L); - verifySavedTask(savedTasks.get(1), outboxEvent, EventTaskType.NOTIFICATION_SEND, 10L); - verifySavedTask(savedTasks.get(2), outboxEvent, EventTaskType.REWARD_GRANT, 20L); - verifySavedTask(savedTasks.get(3), outboxEvent, EventTaskType.NOTIFICATION_SEND, 20L); + List<EventTask> savedTasks = captor.getAllValues(); + org.assertj.core.api.Assertions.assertThat(savedTasks) + .hasSize(4) + .allSatisfy(task -> org.assertj.core.api.Assertions.assertThat(task.getOutboxEvent()).isEqualTo(outboxEvent)) + .extracting(EventTask::getTaskType, EventTask::getTargetUserId) + .containsExactlyInAnyOrder( + org.assertj.core.groups.Tuple.tuple(EventTaskType.REWARD_GRANT, 10L), + org.assertj.core.groups.Tuple.tuple(EventTaskType.NOTIFICATION_SEND, 10L), + org.assertj.core.groups.Tuple.tuple(EventTaskType.REWARD_GRANT, 20L), + org.assertj.core.groups.Tuple.tuple(EventTaskType.NOTIFICATION_SEND, 20L) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventTaskAppenderTest.java` around lines 54 - 57, The test in OutboxEventTaskAppenderTest currently assumes a fixed save order by asserting savedTasks[0..3], which makes it fragile; modify the assertions to be order-independent: build the set/list of expected task descriptors (EventTaskType and target user id and delay values) and for each expected descriptor assert that savedTasks contains a matching task (use verifySavedTask-like checks but search savedTasks with stream/anyMatch or use assertions like containsExactlyInAnyOrder), referencing savedTasks, verifySavedTask, and EventTaskType to locate and change the verification logic so it matches by task type + target user (and delay) instead of index positions.src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskRetryPolicy.java (1)
3-4: 재시도 한도는 설정값으로 외부화해두는 편이 운영에 유리합니다.지금처럼
5로 고정하면 장애 대응 중 환경별로 재시도 횟수를 조정하려면 다시 배포해야 합니다.application.yml이나@ConfigurationProperties로 빼두면 운영 중 완급 조절이 훨씬 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskRetryPolicy.java` around lines 3 - 4, The hardcoded retry limit in EventTaskRetryPolicy (public static final int MAX_RETRY_COUNT = 5) should be externalized to configuration; create a configuration property (e.g., EventTaskProperties or RetryProperties) bound via `@ConfigurationProperties` to application.yml (e.g., event.task.max-retry) and replace the static constant with an injectable value (constructor or setter) in EventTaskRetryPolicy so the policy reads the max retry from that bean; update usages of EventTaskRetryPolicy.MAX_RETRY_COUNT to reference the injected property or a getter on the policy bean.src/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.java (1)
47-56: 비관리자 케이스에서 조회 서비스 미호출도 같이 고정해두면 좋겠습니다.지금 검증은
commandRewardService만 막고 있어서, 이후 순서가 바뀌어querySettlementService.findIdByCode(...)가 먼저 실행돼도 테스트가 통과합니다. 인가 실패 시 리소스 조회 자체가 일어나지 않는 것도 같이 검증해두면 회귀를 더 잘 막을 수 있습니다.✅ 제안 diff
verify(commandRewardService, never()).manualGrant(anyLong(), anyLong()); + verify(querySettlementService, never()).findIdByCode(anyString()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.java` around lines 47 - 56, Update the non-admin test (manualGrantForbiddenWhenNotAdmin) to also assert that no resource lookup occurs: after calling rewardAdminController.manualGrant with a non-admin LoginUserInfo, verify that querySettlementService.findIdByCode(...) is never invoked in addition to the existing verify(commandRewardService, never()). This will ensure the controller blocks permission checks before any settlement lookup; reference the test method manualGrantForbiddenWhenNotAdmin, the mocked querySettlementService.findIdByCode, and commandRewardService.manualGrant when adding the extra verify.src/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.java (1)
13-16: 배치 크기 30이 하드코딩되어 있습니다.현재는 문제없지만, 향후 설정 가능하도록
@Query와Pageable을 사용하는 방식을 고려해볼 수 있습니다. 현재 구현도 충분히 동작합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.java` around lines 13 - 16, The repository method findTop30ByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc hardcodes the batch size (30); change the API to accept a Pageable or use an `@Query` with a limit parameter so batch size becomes configurable. Update EventTaskRepository by replacing findTop30ByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc(List<EventTaskStatus>, int) with a method signature that accepts a Pageable (e.g., findByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc(List<EventTaskStatus>, int, Pageable)) or add a custom `@Query` that takes a native limit parameter, and adjust calling code to pass PageRequest.of(0, batchSize) so the batch size is configurable at runtime.src/main/java/com/dnd/moddo/outbox/presentation/EventTaskAdminController.java (1)
33-37: 권한 검증 로직이 적절합니다.현재 구현이 잘 동작하지만, Spring Security의
@PreAuthorize어노테이션 사용을 고려해볼 수 있습니다. 이렇게 하면 권한 검증 로직이 더 선언적이고 일관성 있게 관리됩니다.`@PreAuthorize`("hasRole('ADMIN')")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/presentation/EventTaskAdminController.java` around lines 33 - 37, Replace the manual role check in validateAdmin(LoginUserInfo) with Spring Security method-level authorization: annotate the controller method(s) (or the controller class) that call validateAdmin with `@PreAuthorize`("hasRole('ADMIN')") and remove or stop calling validateAdmin; ensure method signatures accept the Spring Security principal (or keep LoginUserInfo if mapped from the principal) and that global method security is enabled (prePostEnabled = true) in your security configuration so `@PreAuthorize` is enforced; keep UserPermissionException only if you still need custom handling for non-auth scenarios.src/test/java/com/dnd/moddo/domain/Member/service/CommandMemberServiceTest.java (1)
132-132: 검증 인자는 명시적 ID로 고정하는 편이 안전합니다.현재 검증은 테스트 팩토리의 기본
SettlementID 값에 의존할 여지가 있습니다. 테스트 내에서 ID를 명시적으로 세팅해 의도를 고정해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/Member/service/CommandMemberServiceTest.java` at line 132, 테스트가 팩토리 기본 ID에 의존하고 있으므로 verify 호출의 인자를 명시적 ID로 고정하세요: in CommandMemberServiceTest set the mockSettlement's ID explicitly (e.g., via setter or builder) before invoking the tested method and then call verify(settlementCompletionProcessor).completeIfAllPaid(<that-explicit-id>) so the assertion uses the same literal ID instead of mockSettlement.getId(); ensure you update both the setup of mockSettlement and the verify line to reference the explicit ID constant.src/main/java/com/dnd/moddo/outbox/domain/event/OutboxEvent.java (1)
68-75: 상태 전이에 가드를 추가해 상태 역행을 막아 주세요.현재는
PUBLISHED이후에도markFailed()가 호출되면 상태가 덮어써질 수 있습니다. 터미널 상태 보호를 두는 편이 안전합니다.💡 제안 수정
public void markPublished() { + if (this.status == OutboxEventStatus.PUBLISHED) { + return; + } this.status = OutboxEventStatus.PUBLISHED; this.publishedAt = LocalDateTime.now(); } public void markFailed() { + if (this.status == OutboxEventStatus.PUBLISHED) { + return; + } this.status = OutboxEventStatus.FAILED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/domain/event/OutboxEvent.java` around lines 68 - 75, OutboxEvent currently allows state regression (markFailed can overwrite PUBLISHED); update the methods in class OutboxEvent (markPublished() and markFailed()) to guard terminal states by checking the current status field before mutating: in markPublished() set status = OutboxEventStatus.PUBLISHED and publishedAt = LocalDateTime.now() only if status is not already a terminal state, and in markFailed() set status = OutboxEventStatus.FAILED only if status is not already PUBLISHED (or another terminal state); if the current status is terminal, return early (or no-op) to prevent overwriting; reference the status field and enum values OutboxEventStatus.PUBLISHED / OutboxEventStatus.FAILED when implementing the checks.src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskProcessorTest.java (1)
81-81: 재시도 한도값 하드코딩을 정책 상수로 치환해 주세요.
5가 테스트에 박혀 있어 운영 정책 변경 시 테스트와 구현이 쉽게 어긋납니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskProcessorTest.java` at line 81, The test hardcodes the retry limit (when(eventTask.getAttemptCount()).thenReturn(5)); replace the literal 5 with the canonical policy constant used by the implementation (for example the retry limit constant on EventTaskProcessor or the shared OutboxRetryPolicy/OutboxProperties constant) so the test reads when(eventTask.getAttemptCount()).thenReturn(<IMPLEMENTATION_RETRY_CONSTANT>); import and reference that constant in EventTaskProcessorTest to keep test aligned with runtime policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/dnd/moddo/event/application/impl/SettlementUpdater.java`:
- Around line 24-32: The complete method in SettlementUpdater uses
settlementRepository.getById followed by settlement.complete which is not atomic
and can race; replace this read-then-write with a conditional atomic update on
the repository (e.g., add a method like markCompletedIfNotCompleted that sets
completedAt only when null) and change SettlementUpdater.complete to call that
repository method and return true only if the update count > 0; remove the
non-atomic settlement.complete path and ensure Outbox/event emission is
triggered only when the atomic update succeeded.
In
`@src/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.java`:
- Around line 16-17: The retry path calls
eventTaskProcessor.process(eventTaskId) but EventTaskProcessor.process currently
only guards against COMPLETED; add EventTaskStatus.PROCESSING to the
early-return guard in EventTaskProcessor.process so that if
eventTask.getStatus() == EventTaskStatus.COMPLETED || eventTask.getStatus() ==
EventTaskStatus.PROCESSING the method returns immediately, preventing concurrent
duplicate processing when CommandEventTaskService.retry triggers a manual retry.
In
`@src/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEvent.java`:
- Line 3: OutboxEventCreatedEvent currently accepts a nullable Long which allows
null IDs to propagate; prevent this by enforcing non-null at construction:
either change the component type to primitive long (outboxEventId) or add a
null-check in the record canonical constructor (e.g.,
Objects.requireNonNull(outboxEventId, ...)) so construction fails fast; update
the record OutboxEventCreatedEvent and its canonical constructor to validate
outboxEventId and include a clear null-error message.
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskFailureNotifier.java`:
- Around line 18-36: The notifyRetryExhausted method in EventTaskFailureNotifier
must not let notification failures abort the transaction started in
EventTaskProcessor.process; wrap the call to errorNotifier.notifyError(...)
inside a try-catch in notifyRetryExhausted, catching Exception (or Throwable if
you must) and only logging the failure (via a logger) without rethrowing so that
eventTask.markFailed() persists; ensure you reference the same DiscordMessage
creation logic but isolate it in the try block and log the caught exception with
context (e.g., taskId) instead of propagating it.
In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java`:
- Around line 24-28: The current check-then-act in EventTaskProcessor (checking
eventTask.getStatus() == EventTaskStatus.COMPLETED then calling
eventTask.markProcessing()) is racy and can allow duplicate processing; modify
the processing flow to enforce a database-level concurrency control: either add
optimistic locking to the EventTask entity (annotate a version field with
`@Version` and handle OptimisticLockException around markProcessing() and save) or
change the repository call that loads the task in EventTaskProcessor to acquire
a pessimistic lock (e.g., use a SELECT ... FOR UPDATE / repository method with
LockModeType.PESSIMISTIC_WRITE) so the status check and markProcessing() happen
inside a single locked transaction. Ensure failure handling reverts or skips if
the lock/optimistic version indicates concurrent modification and keep
references to EventTask, EventTaskProcessor, markProcessing(), and
EventTaskStatus.COMPLETED when making the change.
In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskScheduler.java`:
- Around line 21-27: processPendingTasks currently calls
eventTaskProcessor.process(...) inside the loop without error handling so one
task exception aborts the batch; wrap the per-task call in a try-catch around
eventTaskProcessor.process(eventTask.getId()) inside processPendingTasks, catch
Exception (or a more specific runtime exception), log the failure including the
eventTask.getId() and exception details, and continue with the next task; ensure
you reference the same repository call
(eventTaskRepository.findTop30ByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc)
and constants (EventTaskStatus, EventTaskRetryPolicy.MAX_RETRY_COUNT) when
locating the loop to modify.
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 23-30: publishPendingEvents currently loads all PENDING events via
outboxEventRepository.findAllByStatus into memory which can OOM; modify
publishPendingEvents to fetch and process events in bounded batches (use
pageable or a limit) and loop until none remain, e.g., call
repository.findAllByStatus with a PageRequest or repository method that accepts
limit/offset; keep calling publish(outboxEvent.getId()) for each event in the
batch. Note that publish is annotated REQUIRES_NEW, so ensure
publishPendingEvents does not rely on a single large transaction (remove
`@Transactional` or scope it to batch iteration) so each publish call still opens
its own transaction as intended. Ensure to reference publishPendingEvents,
findAllByStatus/OutboxEventRepository, and publish when making changes.
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventTaskAppender.java`:
- Around line 23-36: 현재 appendSettlementCompletedTasks에서
memberReader.findAssignedMembersBySettlementId로 순회하면서 member별로
eventTaskRepository.saveAndFlush를 반복해 I/O가 크고 원자성이 보장되지 않습니다;
appendSettlementCompletedTasks(OutboxEvent) 안에서 각 Member에 대해
EventTask.pending(outboxEvent, EventTaskType..., targetUserId)를 생성해
List<EventTask>로 모은 뒤 한 번에 eventTaskRepository.saveAll(list)로 배치 저장하도록 변경하고,
appendTasks 또는 appendSettlementCompletedTasks 메서드에 트랜잭션을 적용(예: `@Transactional`)해
단일 트랜잭션으로 묶어 중간 실패 시 전체 롤백되도록 하세요; 참조 심볼: appendTasks,
appendSettlementCompletedTasks, memberReader.findAssignedMembersBySettlementId,
eventTaskRepository.saveAndFlush -> saveAll, EventTask.pending,
EventTaskType.REWARD_GRANT/NOTIFICATION_SEND.
In `@src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java`:
- Around line 27-47: Add a uniqueness constraint to prevent duplicate EventTask
rows for the same (outbox_id, task_type, target_user_id): update the EventTask
entity declaration to include a `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = {"outbox_id","task_type","target_user_id"})) and
ensure the fields referenced are the entity fields outboxEvent (maps to
outbox_id), taskType, and targetUserId; also add a corresponding database
migration (unique index on outbox_id, task_type, target_user_id) so the DB
enforces it at persistence time.
In
`@src/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.java`:
- Line 13: The repository method findAllByStatus currently returns all matching
OutboxEvent rows which can OOM when events accumulate; change it to a paginated
query (e.g., replace List<OutboxEvent> findAllByStatus(OutboxEventStatus status)
with a pageable variant such as Page<OutboxEvent> findByStatus(OutboxEventStatus
status, Pageable pageable) or Slice<OutboxEvent> findByStatus(..., Pageable
pageable)), and update any callers (batch publish/retry code) to iterate pages
via Pageable/nextPage until empty instead of loading all results at once; keep
the OutboxEventRepository interface and OutboxEventStatus type names to locate
and modify implementations.
In `@src/main/java/com/dnd/moddo/reward/application/impl/RewardGrantHandler.java`:
- Around line 30-34: The current blanket swallowing of
DataIntegrityViolationException around
collectionRepository.save(Collection.acquire(targetUserId, character.getId()))
may hide non-duplicate constraint failures; update the handler to detect true
unique-key/duplicate grants (e.g., inspect exception.getCause() for
ConstraintViolationException or SQLState/code such as Postgres 23505, or catch a
duplicate-specific exception like DuplicateKeyException) and only suppress
those, while rethrowing or logging other DataIntegrityViolationException cases
so NOT NULL, FK, or other constraint violations are not silently ignored.
In `@src/main/java/com/dnd/moddo/reward/domain/character/Collection.java`:
- Around line 20-25: 엔티티 Collection에 선언한
`@UniqueConstraint`(uk_collections_user_character, columns user_id, character_id)가
실제 DB에 반영되지 않았으니, 새로운 마이그레이션 파일을 추가하여 collections 테이블에 (user_id, character_id)
유니크 제약을 생성하는 DDL을 추가하고(예: ALTER TABLE collections ADD CONSTRAINT
uk_collections_user_character UNIQUE(user_id, character_id)), 마이그레이션의 롤백
스크립트(ALTER TABLE ... DROP CONSTRAINT)를 함께 포함해 테스트/프로덕션 마이그레이션에 적용되도록 PR에 포함시켜
주세요.
In
`@src/main/java/com/dnd/moddo/reward/domain/character/exception/SettlementCharacterNotFoundException.java`:
- Around line 8-9: The exception SettlementCharacterNotFoundException currently
calls super with HttpStatus.INTERNAL_SERVER_ERROR; change the status to a more
appropriate business/data-level code (e.g., HttpStatus.NOT_FOUND) in the
SettlementCharacterNotFoundException(Long settlementId) constructor so the
exception reflects a "not found" condition rather than a 500 and avoids
triggering retry logic.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.java`:
- Around line 47-48: The test EventTaskSchedulerTest currently verifies calls to
eventTaskProcessor.process(1L) and process(2L) separately which only asserts
existence, not order; replace the two independent verify(...) calls with an
ordered verification using Mockito's InOrder (create InOrder inOrder =
inOrder(eventTaskProcessor)) and then assert
inOrder.verify(eventTaskProcessor).process(1L) followed by
inOrder.verify(eventTaskProcessor).process(2L) to guarantee the sequence; keep
the same mock and test setup but use InOrder for the two process(...)
assertions.
In
`@src/test/java/com/dnd/moddo/domain/reward/service/implementation/RewardGrantHandlerTest.java`:
- Around line 77-79: 현재 테스트(RewardGrantHandlerTest)에서
rewardGrantHandler.handle(1L, 2L)가 예외를 던지지 않는지만 확인하고 있어 저장 로직 호출 여부를 검증하지 못합니다;
handle 호출 후 Mockito.verify를 사용해 모의 저장소의 save 호출을 검증(예: verify(rewardRepository,
times(1)).save(any(Reward.class)) 또는 구체적 인자 매처)하도록 수정해 주세요 — 대상 메서드는
rewardGrantHandler.handle(...)이고 검증 대상은 rewardRepository.save(...)입니다.
---
Nitpick comments:
In `@src/main/java/com/dnd/moddo/event/application/impl/MemberReader.java`:
- Around line 48-51: The current MemberReader.findAssignedMembersBySettlementId
loads all members then filters by isAssigned in memory; change it to push the
filter to the DB by adding a repository query (e.g., add List<Member>
findAllBySettlementIdAndIsAssignedTrue(Long settlementId) to MemberRepository)
and have MemberReader call that repository method instead of
findAllBySettlementId(...). Ensure the repository method name or a `@Query`
equivalent is used so only assigned members are fetched from the database.
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskRetryPolicy.java`:
- Around line 3-4: The hardcoded retry limit in EventTaskRetryPolicy (public
static final int MAX_RETRY_COUNT = 5) should be externalized to configuration;
create a configuration property (e.g., EventTaskProperties or RetryProperties)
bound via `@ConfigurationProperties` to application.yml (e.g.,
event.task.max-retry) and replace the static constant with an injectable value
(constructor or setter) in EventTaskRetryPolicy so the policy reads the max
retry from that bean; update usages of EventTaskRetryPolicy.MAX_RETRY_COUNT to
reference the injected property or a getter on the policy bean.
In `@src/main/java/com/dnd/moddo/outbox/domain/event/OutboxEvent.java`:
- Around line 68-75: OutboxEvent currently allows state regression (markFailed
can overwrite PUBLISHED); update the methods in class OutboxEvent
(markPublished() and markFailed()) to guard terminal states by checking the
current status field before mutating: in markPublished() set status =
OutboxEventStatus.PUBLISHED and publishedAt = LocalDateTime.now() only if status
is not already a terminal state, and in markFailed() set status =
OutboxEventStatus.FAILED only if status is not already PUBLISHED (or another
terminal state); if the current status is terminal, return early (or no-op) to
prevent overwriting; reference the status field and enum values
OutboxEventStatus.PUBLISHED / OutboxEventStatus.FAILED when implementing the
checks.
In `@src/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.java`:
- Around line 13-16: The repository method
findTop30ByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc hardcodes the
batch size (30); change the API to accept a Pageable or use an `@Query` with a
limit parameter so batch size becomes configurable. Update EventTaskRepository
by replacing
findTop30ByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc(List<EventTaskStatus>,
int) with a method signature that accepts a Pageable (e.g.,
findByStatusInAndAttemptCountLessThanOrderByCreatedAtAsc(List<EventTaskStatus>,
int, Pageable)) or add a custom `@Query` that takes a native limit parameter, and
adjust calling code to pass PageRequest.of(0, batchSize) so the batch size is
configurable at runtime.
In
`@src/main/java/com/dnd/moddo/outbox/presentation/EventTaskAdminController.java`:
- Around line 33-37: Replace the manual role check in
validateAdmin(LoginUserInfo) with Spring Security method-level authorization:
annotate the controller method(s) (or the controller class) that call
validateAdmin with `@PreAuthorize`("hasRole('ADMIN')") and remove or stop calling
validateAdmin; ensure method signatures accept the Spring Security principal (or
keep LoginUserInfo if mapped from the principal) and that global method security
is enabled (prePostEnabled = true) in your security configuration so
`@PreAuthorize` is enforced; keep UserPermissionException only if you still need
custom handling for non-auth scenarios.
In
`@src/test/java/com/dnd/moddo/domain/Member/service/CommandMemberServiceTest.java`:
- Line 132: 테스트가 팩토리 기본 ID에 의존하고 있으므로 verify 호출의 인자를 명시적 ID로 고정하세요: in
CommandMemberServiceTest set the mockSettlement's ID explicitly (e.g., via
setter or builder) before invoking the tested method and then call
verify(settlementCompletionProcessor).completeIfAllPaid(<that-explicit-id>) so
the assertion uses the same literal ID instead of mockSettlement.getId(); ensure
you update both the setup of mockSettlement and the verify line to reference the
explicit ID constant.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskProcessorTest.java`:
- Line 81: The test hardcodes the retry limit
(when(eventTask.getAttemptCount()).thenReturn(5)); replace the literal 5 with
the canonical policy constant used by the implementation (for example the retry
limit constant on EventTaskProcessor or the shared
OutboxRetryPolicy/OutboxProperties constant) so the test reads
when(eventTask.getAttemptCount()).thenReturn(<IMPLEMENTATION_RETRY_CONSTANT>);
import and reference that constant in EventTaskProcessorTest to keep test
aligned with runtime policy.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventTaskAppenderTest.java`:
- Around line 54-57: The test in OutboxEventTaskAppenderTest currently assumes a
fixed save order by asserting savedTasks[0..3], which makes it fragile; modify
the assertions to be order-independent: build the set/list of expected task
descriptors (EventTaskType and target user id and delay values) and for each
expected descriptor assert that savedTasks contains a matching task (use
verifySavedTask-like checks but search savedTasks with stream/anyMatch or use
assertions like containsExactlyInAnyOrder), referencing savedTasks,
verifySavedTask, and EventTaskType to locate and change the verification logic
so it matches by task type + target user (and delay) instead of index positions.
In
`@src/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.java`:
- Around line 47-56: Update the non-admin test
(manualGrantForbiddenWhenNotAdmin) to also assert that no resource lookup
occurs: after calling rewardAdminController.manualGrant with a non-admin
LoginUserInfo, verify that querySettlementService.findIdByCode(...) is never
invoked in addition to the existing verify(commandRewardService, never()). This
will ensure the controller blocks permission checks before any settlement
lookup; reference the test method manualGrantForbiddenWhenNotAdmin, the mocked
querySettlementService.findIdByCode, and commandRewardService.manualGrant when
adding the extra verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 876dce3e-ce75-4b5a-ae6e-77392674b47c
📒 Files selected for processing (56)
src/main/java/com/dnd/moddo/ModdoApplication.javasrc/main/java/com/dnd/moddo/event/application/command/CommandMemberService.javasrc/main/java/com/dnd/moddo/event/application/command/CommandPaymentRequest.javasrc/main/java/com/dnd/moddo/event/application/command/CommandSettlementService.javasrc/main/java/com/dnd/moddo/event/application/impl/MemberReader.javasrc/main/java/com/dnd/moddo/event/application/impl/SettlementCompletionProcessor.javasrc/main/java/com/dnd/moddo/event/application/impl/SettlementUpdater.javasrc/main/java/com/dnd/moddo/event/infrastructure/MemberRepository.javasrc/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.javasrc/main/java/com/dnd/moddo/outbox/application/command/CommandOutboxEventService.javasrc/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEvent.javasrc/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEventListener.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskFailureNotifier.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskRetryPolicy.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskScheduler.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventCreator.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventTaskAppender.javasrc/main/java/com/dnd/moddo/outbox/domain/event/OutboxEvent.javasrc/main/java/com/dnd/moddo/outbox/domain/event/type/AggregateType.javasrc/main/java/com/dnd/moddo/outbox/domain/event/type/OutboxEventStatus.javasrc/main/java/com/dnd/moddo/outbox/domain/event/type/OutboxEventType.javasrc/main/java/com/dnd/moddo/outbox/domain/task/EventTask.javasrc/main/java/com/dnd/moddo/outbox/domain/task/type/EventTaskStatus.javasrc/main/java/com/dnd/moddo/outbox/domain/task/type/EventTaskType.javasrc/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.javasrc/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.javasrc/main/java/com/dnd/moddo/outbox/presentation/EventTaskAdminController.javasrc/main/java/com/dnd/moddo/reward/application/CommandRewardService.javasrc/main/java/com/dnd/moddo/reward/application/impl/RewardGrantHandler.javasrc/main/java/com/dnd/moddo/reward/domain/character/Collection.javasrc/main/java/com/dnd/moddo/reward/domain/character/exception/SettlementCharacterNotFoundException.javasrc/main/java/com/dnd/moddo/reward/infrastructure/CollectionRepository.javasrc/main/java/com/dnd/moddo/reward/infrastructure/RewardQueryRepositoryImpl.javasrc/main/java/com/dnd/moddo/reward/presentation/RewardAdminController.javasrc/main/resources/configsrc/test/java/com/dnd/moddo/domain/Member/service/CommandMemberServiceTest.javasrc/test/java/com/dnd/moddo/domain/outbox/controller/EventTaskAdminControllerTest.javasrc/test/java/com/dnd/moddo/domain/outbox/entity/EventTaskTest.javasrc/test/java/com/dnd/moddo/domain/outbox/entity/OutboxEventTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandOutboxEventServiceTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskProcessorTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventCreatedEventListenerTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventCreatorTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventTaskAppenderTest.javasrc/test/java/com/dnd/moddo/domain/paymentRequest/service/CommandPaymentRequestTest.javasrc/test/java/com/dnd/moddo/domain/paymentRequest/service/implementation/PaymentRequestUpdaterTest.javasrc/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.javasrc/test/java/com/dnd/moddo/domain/reward/service/CommandRewardServiceTest.javasrc/test/java/com/dnd/moddo/domain/reward/service/implementation/RewardGrantHandlerTest.javasrc/test/java/com/dnd/moddo/domain/settlement/service/CommandSettlementServiceTest.javasrc/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementCompletionProcessorTest.java
src/main/java/com/dnd/moddo/event/application/impl/SettlementUpdater.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEvent.java
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskFailureNotifier.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java
Outdated
Show resolved
Hide resolved
...va/com/dnd/moddo/reward/domain/character/exception/SettlementCharacterNotFoundException.java
Outdated
Show resolved
Hide resolved
src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/dnd/moddo/domain/reward/service/implementation/RewardGrantHandlerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java (1)
24-29:⚠️ Potential issue | 🔴 Critical같은 태스크가 중복 처리될 수 있습니다.
getById()이후 상태 확인과markProcessing()사이에 동시성 제어가 없어서, 두 워커가 같은PENDING태스크를 동시에 집으면 둘 다 보상 지급까지 진행할 수 있습니다. 조회-상태 확인-전이 구간을 하나의 점유 경계로 묶어야 합니다(@Version기반 충돌 감지나 조회 시 쓰기 락 등).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java` around lines 24 - 29, The code checks status after eventTaskRepository.getById(...) and then calls eventTask.markProcessing(), which allows two workers to race; wrap the read-and-transition in a concurrency-safe boundary by either using optimistic locking on EventTask (add `@Version` to the entity and persist the status change, handling OptimisticLockException to abort/skip duplicate workers) or by fetching the row with a write/pessimistic lock (implement a repository method like findByIdForUpdate(...) that uses SELECT ... FOR UPDATE) and performing the status check + eventTask.markProcessing() inside that locked/transactional scope; use the EventTask, eventTaskRepository.getById, eventTask.markProcessing, and EventTaskStatus.COMPLETED symbols to locate and update the logic accordingly.
🧹 Nitpick comments (3)
src/test/java/com/dnd/moddo/domain/Member/service/implementation/MemberReaderTest.java (1)
187-197: 테스트 커버리지 개선 제안:false케이스 추가 고려현재 테스트는 미납 멤버가 존재하는 경우(
true반환)만 검증합니다. 모든 멤버가 입금 완료한 경우(false반환)에 대한 테스트도 추가하면 커버리지가 더 완전해집니다.💡 추가 테스트 케이스 예시
`@DisplayName`("정산에 미납 멤버가 없으면 false를 반환한다.") `@Test` void existsUnpaidMemberReturnsFalseWhenAllPaid() { Long settlementId = 1L; when(memberRepository.existsBySettlementIdAndIsPaidFalse(settlementId)).thenReturn(false); boolean result = memberReader.existsUnpaidMember(settlementId); assertThat(result).isFalse(); verify(memberRepository).existsBySettlementIdAndIsPaidFalse(settlementId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/Member/service/implementation/MemberReaderTest.java` around lines 187 - 197, Add a complementary unit test in MemberReaderTest that verifies existsUnpaidMember returns false when no unpaid members exist: mock memberRepository.existsBySettlementIdAndIsPaidFalse(settlementId) to return false, call memberReader.existsUnpaidMember(settlementId), assertThat(result).isFalse(), and verify the repository method was invoked; this mirrors the existing true-case test and improves coverage.src/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java (2)
72-84: 테스트 로직 LGTM - 멱등성 검증이 잘 되어 있습니다.이미 완료된 정산에 대해 재호출 시
false를 반환하고complete()가 다시 호출되지 않음을 검증하여 멱등성을 보장합니다.다만 한 가지 스타일 개선 사항: Line 78에서
java.time.LocalDateTime.now()를 풀 패키지 경로로 사용하고 있습니다. 파일 상단에 import를 추가하면 코드 일관성이 향상됩니다.
,♻️ import 추가 제안
파일 상단 imports에 추가:
import java.time.LocalDateTime;테스트 코드 수정:
- when(settlement.getCompletedAt()).thenReturn(java.time.LocalDateTime.now()); + when(settlement.getCompletedAt()).thenReturn(LocalDateTime.now());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java` around lines 72 - 84, In SettlementUpdaterTest (method completeAlreadyCompletedSettlement) replace the fully-qualified java.time.LocalDateTime.now() with an imported type: add "import java.time.LocalDateTime;" to the file imports and change the call to LocalDateTime.now() where settlement.getCompletedAt() is stubbed; this keeps the test consistent with the rest of the file and avoids using the full package name inside completeAlreadyCompletedSettlement.
84-84: 정산을 찾을 수 없는 경우에 대한 테스트 케이스 추가 고려기존
updateAccountNotFoundGroup테스트처럼complete()메서드에서도 정산을 찾을 수 없는 경우에 대한 테스트가 있으면 테스트 커버리지가 더 완전해집니다.💡 추가 테스트 케이스 예시
`@DisplayName`("정산이 존재하지 않으면 완료 처리할 때 예외가 발생한다.") `@Test` void completeNotFoundSettlement() { // given Long settlementId = 1L; doThrow(new GroupNotFoundException(settlementId)).when(settlementRepository).getById(settlementId); // when & then assertThatThrownBy(() -> settlementUpdater.complete(settlementId)) .isInstanceOf(GroupNotFoundException.class); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java` at line 84, Add a new unit test in SettlementUpdaterTest to mirror the existing updateAccountNotFoundGroup case but for complete(): mock settlementRepository.getById to throw new GroupNotFoundException(settlementId) for a given settlementId, then call settlementUpdater.complete(settlementId) and assert that it throws GroupNotFoundException; reference the existing test pattern and use settlementRepository.getById, settlementUpdater.complete, and GroupNotFoundException to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java`:
- Around line 31-36: The code in EventTaskProcessor currently only handles
EventTaskType.REWARD_GRANT and then always calls eventTask.markCompleted(),
which causes other task types to be marked completed without handling; update
the processing logic in EventTaskProcessor so that you explicitly handle each
supported EventTaskType (e.g., via a switch or if/else) and call
rewardGrantHandler.handle(...) followed by eventTask.markCompleted() only when
handling succeeds, and for any unsupported/unknown EventTaskType throw an
exception or mark the task as failed instead of calling
eventTask.markCompleted(); reference the existing symbols
EventTaskType.REWARD_GRANT, rewardGrantHandler.handle(...), and
eventTask.markCompleted() when implementing this change.
- Around line 37-40: The call to
eventTaskFailureNotifier.notifyRetryExhausted(...) inside the transactional
process() can throw (via errorNotifier.notifyError() -> Discord API) and cause
the transaction to rollback, undoing eventTask.markFailed(...) and attemptCount
increments; to fix, ensure notification is executed outside or isolated from the
transaction by either (a) moving the notifyRetryExhausted invocation so it runs
after the transactional block completes successfully, or (b) wrapping the
notifyRetryExhausted call in its own try-catch that swallows/logs notification
exceptions (and does not rethrow) so notifyRetryExhausted failures cannot roll
back changes to eventTask; locate references to EventTaskProcessor.process(),
eventTask.markFailed(), eventTask.getAttemptCount(),
EventTaskRetryPolicy.MAX_RETRY_COUNT, and
eventTaskFailureNotifier.notifyRetryExhausted() when applying the change.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java`:
- Around line 61-77: The test publishPendingEvents() currently verifies only
that outboxEventTaskAppender.appendTasks was called for both mocks but not the
order; update OutboxEventPublisherTest to use Mockito's InOrder to assert
appendTasks(first) was called before appendTasks(second) (create InOrder inOrder
= inOrder(outboxEventTaskAppender) and use inOrder.verify(...) for first then
second) so the test enforces the intended call sequence from
publishPendingEvents().
---
Duplicate comments:
In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java`:
- Around line 24-29: The code checks status after
eventTaskRepository.getById(...) and then calls eventTask.markProcessing(),
which allows two workers to race; wrap the read-and-transition in a
concurrency-safe boundary by either using optimistic locking on EventTask (add
`@Version` to the entity and persist the status change, handling
OptimisticLockException to abort/skip duplicate workers) or by fetching the row
with a write/pessimistic lock (implement a repository method like
findByIdForUpdate(...) that uses SELECT ... FOR UPDATE) and performing the
status check + eventTask.markProcessing() inside that locked/transactional
scope; use the EventTask, eventTaskRepository.getById, eventTask.markProcessing,
and EventTaskStatus.COMPLETED symbols to locate and update the logic
accordingly.
---
Nitpick comments:
In
`@src/test/java/com/dnd/moddo/domain/Member/service/implementation/MemberReaderTest.java`:
- Around line 187-197: Add a complementary unit test in MemberReaderTest that
verifies existsUnpaidMember returns false when no unpaid members exist: mock
memberRepository.existsBySettlementIdAndIsPaidFalse(settlementId) to return
false, call memberReader.existsUnpaidMember(settlementId),
assertThat(result).isFalse(), and verify the repository method was invoked; this
mirrors the existing true-case test and improves coverage.
In
`@src/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java`:
- Around line 72-84: In SettlementUpdaterTest (method
completeAlreadyCompletedSettlement) replace the fully-qualified
java.time.LocalDateTime.now() with an imported type: add "import
java.time.LocalDateTime;" to the file imports and change the call to
LocalDateTime.now() where settlement.getCompletedAt() is stubbed; this keeps the
test consistent with the rest of the file and avoids using the full package name
inside completeAlreadyCompletedSettlement.
- Line 84: Add a new unit test in SettlementUpdaterTest to mirror the existing
updateAccountNotFoundGroup case but for complete(): mock
settlementRepository.getById to throw new GroupNotFoundException(settlementId)
for a given settlementId, then call settlementUpdater.complete(settlementId) and
assert that it throws GroupNotFoundException; reference the existing test
pattern and use settlementRepository.getById, settlementUpdater.complete, and
GroupNotFoundException to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41724690-deea-455f-a3ce-13bcfa58ca25
📒 Files selected for processing (8)
src/main/java/com/dnd/moddo/common/logging/EventTaskFailureNotifier.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.javasrc/test/java/com/dnd/moddo/domain/Member/service/implementation/MemberReaderTest.javasrc/test/java/com/dnd/moddo/domain/common/logging/EventTaskFailureNotifierTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskProcessorTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.javasrc/test/java/com/dnd/moddo/domain/reward/service/implementation/RewardGrantHandlerTest.javasrc/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/dnd/moddo/domain/reward/service/implementation/RewardGrantHandlerTest.java
src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskProcessor.java
Outdated
Show resolved
Hide resolved
src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java
Show resolved
Hide resolved
📝 테스트 커버리지 리포트입니다!
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java (1)
77-99:⚠️ Potential issue | 🟡 Minor“순서대로” 동작에 대한 순서 단언이 빠져 있습니다.
Line 97, Line 98은 각 호출의 발생만 확인합니다. 테스트명에 맞게 호출 순서도 검증하는 편이 안전합니다.
제안 수정
+import org.mockito.InOrder; ... - verify(first).markPublished(); - verify(second).markPublished(); + InOrder inOrder = inOrder(first, second); + inOrder.verify(first).markPublished(); + inOrder.verify(second).markPublished();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java` around lines 77 - 99, The test publishPendingEvents currently only verifies that first.markPublished() and second.markPublished() were called, but not their order; update the test to assert call order by creating an InOrder for the mocks (e.g., InOrder inOrder = inOrder(first, second)) after calling outboxEventPublisher.publishPendingEvents() and use inOrder.verify(first).markPublished() followed by inOrder.verify(second).markPublished(), replacing or supplementing the existing verify(...) calls to ensure ordered verification.src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.java (1)
47-48:⚠️ Potential issue | 🟡 Minor호출 순서 검증이 누락되어 테스트 의도와 불일치합니다.
Line 47, Line 48의 개별
verify(...)는 호출 “존재”만 확인합니다. DisplayName의 “순서대로”를 보장하려면 순서 단언이 필요합니다.제안 수정
+import org.mockito.InOrder; ... - verify(commandEventTaskService).process(1L); - verify(commandEventTaskService).process(2L); + InOrder inOrder = inOrder(commandEventTaskService); + inOrder.verify(commandEventTaskService).process(1L); + inOrder.verify(commandEventTaskService).process(2L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.java` around lines 47 - 48, The test EventTaskSchedulerTest currently verifies only that commandEventTaskService.process(1L) and process(2L) were called, but not their order; replace the two separate verify(...) calls with an InOrder verification using Mockito.inOrder(commandEventTaskService) and assert that process(1L) was called before process(2L) to match the test DisplayName about order; keep using the same commandEventTaskService and the process(...) method names when constructing the InOrder assertions.src/main/java/com/dnd/moddo/reward/application/RewardService.java (1)
34-38:⚠️ Potential issue | 🟠 Major무결성 예외를 전부 성공 처리하면 실제 데이터 오류가 숨겨집니다.
Line 36에서
DataIntegrityViolationException전체를 삼키면, 중복 지급 외 오류까지 정상 처리로 오인될 수 있습니다. 중복 키만 멱등 성공 처리하고 나머지는 전파하는 분기가 필요합니다.제안 수정
import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.dao.DuplicateKeyException; ... - } catch (DataIntegrityViolationException exception) { + } catch (DuplicateKeyException exception) { // Concurrent/manual duplicate grants are treated as idempotent success. + } catch (DataIntegrityViolationException exception) { + throw exception; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/reward/application/RewardService.java` around lines 34 - 38, In RewardService, don't swallow every DataIntegrityViolationException around collectionRepository.save(Collection.acquire(...)); instead detect whether the exception is a duplicate-key/unique-constraint violation (e.g., root cause instanceof ConstraintViolationException or SQL state/SQLException error code for duplicate key, or convert to Spring's DuplicateKeyException) and only treat that case as idempotent success; for any other DataIntegrityViolationException rethrow it so real data errors are propagated. Locate the try/catch around collectionRepository.save(...) and replace the broad catch with a conditional check on the exception's root cause (or map to DuplicateKeyException) to decide between swallowing and rethrowing.src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java (1)
27-33:⚠️ Potential issue | 🟠 MajorPENDING 전체 조회는 배치 제한이 필요합니다.
Line 29에서 전체 PENDING 이벤트를 한 번에 로드하면 적체 시 메모리/지연 리스크가 큽니다. 배치 단위 조회로 반복 처리하세요.
배치 처리 예시
`@Transactional` public void publishPendingEvents() { - List<OutboxEvent> pendingEvents = outboxReader.findAllByStatus(OutboxEventStatus.PENDING); - - for (OutboxEvent outboxEvent : pendingEvents) { - publish(outboxEvent.getId()); - } + while (true) { + List<OutboxEvent> pendingEvents = outboxReader.findPendingBatch(100); // createdAt asc + if (pendingEvents.isEmpty()) { + break; + } + for (OutboxEvent outboxEvent : pendingEvents) { + publish(outboxEvent.getId()); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java` around lines 27 - 33, The publishPendingEvents method currently loads all PENDING events at once via outboxReader.findAllByStatus(OutboxEventStatus.PENDING); change it to process in repeated bounded-size batches: implement/read using a paginated method (e.g. outboxReader.findPendingBatch(limit, lastId/offset) or findAllByStatusWithLimit) and loop fetching batches until empty, calling publish(outboxEvent.getId()) for each item; ensure each batch is processed and committed before fetching the next batch to avoid memory spikes and long transactions in publishPendingEvents.
🧹 Nitpick comments (5)
src/main/java/com/dnd/moddo/common/config/SecurityConfig.java (1)
54-60: 관리자 인증 실패 시 리다이렉트 로직 개선이 필요합니다.현재
request.getRequestURI().startsWith("/admin/")는/admin정확한 경로를 놓치고, 컨텍스트 경로가 추가될 경우 요청 경로 비교가 실패할 수 있습니다.request.getServletPath()로 판별하고 리다이렉트 URL에request.getContextPath()를 포함하는 것이 더 견고합니다.참고: 현재 코드베이스는 context-path가 설정되지 않아 즉시 문제는 아니지만, 배포 환경에 따라 문제가 될 수 있으므로 사전에 개선하면 좋습니다.
수정 예시
.exceptionHandling(exception -> exception .authenticationEntryPoint((request, response, authException) -> { - if (request.getRequestURI().startsWith("/admin/")) { - response.sendRedirect("/admin/login"); + String path = request.getServletPath(); + if ("/admin".equals(path) || path.startsWith("/admin/")) { + response.sendRedirect(request.getContextPath() + "/admin/login"); return; } response.sendError(HttpStatus.UNAUTHORIZED.value()); }) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/common/config/SecurityConfig.java` around lines 54 - 60, In SecurityConfig inside the authenticationEntryPoint lambda, replace the use of request.getRequestURI() with request.getServletPath() for admin-path detection and construct the redirect by prepending request.getContextPath() to the "/admin/login" path before calling response.sendRedirect; specifically update the condition that currently uses request.getRequestURI().startsWith("/admin/") and the response.sendRedirect("/admin/login") call so it checks servlet path (e.g., request.getServletPath().startsWith("/admin")) and redirects to request.getContextPath() + "/admin/login".src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskCreatorTest.java (1)
40-43: 생성 시 초기 상태(PENDING) 검증도 추가하면 테스트가 더 견고해집니다.Line 40-43 검증에
status == PENDING를 포함하면EventTask.pending(...)의 핵심 계약 회귀를 바로 잡을 수 있습니다.✅ 제안 수정안
import com.dnd.moddo.outbox.domain.task.EventTask; +import com.dnd.moddo.outbox.domain.task.type.EventTaskStatus; import com.dnd.moddo.outbox.domain.task.type.EventTaskType; @@ assertThat(captor.getValue().getOutboxEvent()).isEqualTo(outboxEvent); assertThat(captor.getValue().getTaskType()).isEqualTo(EventTaskType.REWARD_GRANT); assertThat(captor.getValue().getTargetUserId()).isEqualTo(20L); + assertThat(captor.getValue().getStatus()).isEqualTo(EventTaskStatus.PENDING); assertThat(result).isEqualTo(savedTask);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskCreatorTest.java` around lines 40 - 43, Add assertions that the created EventTask is initialized with status PENDING: after capturing the created task (captor.getValue()) and after the returned result (result), assert their getStatus() equals EventTaskStatus.PENDING (or use the enum/constant your code uses), ensuring EventTask.pending(...) contract is validated alongside the existing checks of getOutboxEvent(), getTaskType(), getTargetUserId(), and result equality.src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskCreator.java (1)
20-20:saveAndFlush는 꼭 필요할 때만 사용을 권장합니다.Line 20은 태스크 생성 건마다 flush를 강제해 처리량이 높은 구간에서 비용이 커질 수 있습니다. 즉시 DB 반영이 반드시 필요하지 않다면
save로 바꾸는 편이 안전합니다.⚡ 제안 수정안
- return eventTaskRepository.saveAndFlush(EventTask.pending(outboxEvent, taskType, targetUserId)); + return eventTaskRepository.save(EventTask.pending(outboxEvent, taskType, targetUserId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskCreator.java` at line 20, The code in EventTaskCreator uses eventTaskRepository.saveAndFlush(EventTask.pending(outboxEvent, taskType, targetUserId)) which forces a DB flush per created task and can hurt throughput; change the call to eventTaskRepository.save(...) to avoid immediate flush unless synchronous DB visibility is required. Update the invocation in the method that constructs EventTask via EventTask.pending(...) to call save instead of saveAndFlush, keeping parameters outboxEvent, taskType, and targetUserId the same; only switch out saveAndFlush to save where immediate persistence is not required.src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceTest.java (1)
57-57: 성공 경로에서 실패 알림 미발행 검증을 추가하면 더 명확합니다.Line 57 검증 뒤에
eventTaskFailureNotifier미호출 검증을 추가하면 정상 재처리 시 오탐 알림이 없는 계약까지 고정할 수 있습니다.🧪 제안 수정안
verify(rewardService).grant(10L, 20L); + verifyNoInteractions(eventTaskFailureNotifier);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceTest.java` at line 57, After verifying rewardService.grant(10L, 20L), add a negative verification that eventTaskFailureNotifier was not invoked to ensure no failure notification is emitted on the successful path; locate the test method in CommandEventTaskServiceTest where verify(rewardService).grant(10L, 20L) appears and append a Mockito no-interaction/never verification against the eventTaskFailureNotifier (e.g., verifyNoInteractions(eventTaskFailureNotifier) or verify(eventTaskFailureNotifier, never()).<notifyFailure/...>) to assert it did not get called.src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.java (1)
41-46: 재시도 횟수 하드코딩(5)을 정책 상수로 통일해 주세요.테스트가 정책 값 변경에 자동 추종하지 못합니다. 프로덕션과 동일하게
EventTaskRetryPolicy.MAX_RETRY_COUNT를 사용하면 유지보수가 쉬워집니다.제안 수정
+import com.dnd.moddo.outbox.application.impl.EventTaskRetryPolicy; ... - 5 + EventTaskRetryPolicy.MAX_RETRY_COUNT ... - 5 + EventTaskRetryPolicy.MAX_RETRY_COUNT ... - 5 + EventTaskRetryPolicy.MAX_RETRY_COUNT ... - 5 + EventTaskRetryPolicy.MAX_RETRY_COUNT ... - when(eventTask.getAttemptCount()).thenReturn(5); + when(eventTask.getAttemptCount()).thenReturn(EventTaskRetryPolicy.MAX_RETRY_COUNT);Also applies to: 59-64, 89-94, 114-119, 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.java` around lines 41 - 46, Replace hardcoded retry count literals (e.g., the 5 passed to eventTaskRepository.claimProcessing in CommandEventTaskServiceProcessTest) with the policy constant EventTaskRetryPolicy.MAX_RETRY_COUNT so tests follow production policy; update every occurrence noted (the blocks around lines with claimProcessing calls and any other assertions using the literal) to reference EventTaskRetryPolicy.MAX_RETRY_COUNT instead of 5.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.java`:
- Around line 30-39: The authentication code in JwtAuth reads claims directly
and can NPE when required claims are missing; add defensive validation after
extracting claims (claims.get(JwtConstants.AUTH_ID.message, Long.class) and
claims.get(JwtConstants.ROLE.message, String.class)) to check that userId and
role are non-null (and userId is a valid Long), and if not throw
TokenInvalidException; apply this before the Authority.ADMIN branch and before
calling authDetailsService.loadUserByUsername(userId.toString()) so you fail
fast with TokenInvalidException rather than causing an NPE in AuthDetails or
loadUserByUsername.
In `@src/main/java/com/dnd/moddo/common/logging/EventTaskFailureNotifier.java`:
- Around line 27-33: The notification currently sends
eventTask.getLastErrorMessage() raw from EventTaskFailureNotifier when building
the message; replace that with a sanitized, truncated version to avoid leaking
tokens/PII: create or call a sanitizer utility (e.g., maskSensitiveInfo(String
message)) that removes or masks common secrets (tokens, emails, UUIDs, keys) and
then truncate the result to a safe length (e.g., 200 chars) before including it
in the message instead of eventTask.getLastErrorMessage(); update the message
assembly in EventTaskFailureNotifier to use the sanitized/truncated value for
lastErrorMessage.
In
`@src/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.java`:
- Around line 40-44: The current early return in CommandEventTaskService leaves
non-supported eventTask types stuck in PROCESSING; replace the `return` with an
explicit state transition and persistence so the task moves to a terminal or
retryable state (e.g., mark as FAILED or COMPLETED) instead of staying
PROCESSING. Locate the branch that checks `eventTask.getTaskType() ==
EventTaskType.REWARD_GRANT` and after the else branch invoke the
service/repository method that updates the task status (use your existing
state-transition method such as markFailed/complete/updateStatus on the task or
repository save), include a reason/message about unsupported task type, and
persist the change so the task can be requeued or cleaned up. Ensure
rewardService.grant(...) remains unchanged for REWARD_GRANT and that
non-supported types always get their status transitioned and saved.
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 43-53: The current try/catch in OutboxEventPublisher around
appendTasks + outboxEvent.markPublished() can commit partially-created tasks if
appendTasks fails; refactor so task-creation (appendTasks) runs in its own
transaction and any exception causes that transaction to roll back and the
exception to propagate, and perform state transitions
(outboxEvent.markPublished() or outboxEvent.markFailed()) in a separate
transaction/handler after the appendTasks transaction completes or fails; update
methods/transactional annotations around appendTasks and the code paths that
call outboxEvent.markPublished()/markFailed() to ensure task persistence and
event state transitions are in distinct transactional boundaries so a failed
append does not leave partial tasks committed while the event is marked FAILED.
- Around line 36-41: There is a race where publish(Long outboxEventId) reads
OutboxEvent and checks getStatus() != OutboxEventStatus.PENDING non-atomically,
allowing duplicate processing; change the flow to perform an atomic state
transition (e.g., execute a single UPDATE that sets status = PROCESSING where id
= :outboxEventId and status = PENDING and check affectedRows == 1) or add
optimistic locking on OutboxEvent (version field + version check) so only one
caller proceeds to create the task; update the publish method to use that atomic
update/lock before creating the task (references: publish(Long outboxEventId),
outboxReader.findById, OutboxEvent, OutboxEventStatus.PENDING, and the
task-creation block around lines 63-67).
---
Duplicate comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 27-33: The publishPendingEvents method currently loads all PENDING
events at once via outboxReader.findAllByStatus(OutboxEventStatus.PENDING);
change it to process in repeated bounded-size batches: implement/read using a
paginated method (e.g. outboxReader.findPendingBatch(limit, lastId/offset) or
findAllByStatusWithLimit) and loop fetching batches until empty, calling
publish(outboxEvent.getId()) for each item; ensure each batch is processed and
committed before fetching the next batch to avoid memory spikes and long
transactions in publishPendingEvents.
In `@src/main/java/com/dnd/moddo/reward/application/RewardService.java`:
- Around line 34-38: In RewardService, don't swallow every
DataIntegrityViolationException around
collectionRepository.save(Collection.acquire(...)); instead detect whether the
exception is a duplicate-key/unique-constraint violation (e.g., root cause
instanceof ConstraintViolationException or SQL state/SQLException error code for
duplicate key, or convert to Spring's DuplicateKeyException) and only treat that
case as idempotent success; for any other DataIntegrityViolationException
rethrow it so real data errors are propagated. Locate the try/catch around
collectionRepository.save(...) and replace the broad catch with a conditional
check on the exception's root cause (or map to DuplicateKeyException) to decide
between swallowing and rethrowing.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.java`:
- Around line 47-48: The test EventTaskSchedulerTest currently verifies only
that commandEventTaskService.process(1L) and process(2L) were called, but not
their order; replace the two separate verify(...) calls with an InOrder
verification using Mockito.inOrder(commandEventTaskService) and assert that
process(1L) was called before process(2L) to match the test DisplayName about
order; keep using the same commandEventTaskService and the process(...) method
names when constructing the InOrder assertions.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java`:
- Around line 77-99: The test publishPendingEvents currently only verifies that
first.markPublished() and second.markPublished() were called, but not their
order; update the test to assert call order by creating an InOrder for the mocks
(e.g., InOrder inOrder = inOrder(first, second)) after calling
outboxEventPublisher.publishPendingEvents() and use
inOrder.verify(first).markPublished() followed by
inOrder.verify(second).markPublished(), replacing or supplementing the existing
verify(...) calls to ensure ordered verification.
---
Nitpick comments:
In `@src/main/java/com/dnd/moddo/common/config/SecurityConfig.java`:
- Around line 54-60: In SecurityConfig inside the authenticationEntryPoint
lambda, replace the use of request.getRequestURI() with request.getServletPath()
for admin-path detection and construct the redirect by prepending
request.getContextPath() to the "/admin/login" path before calling
response.sendRedirect; specifically update the condition that currently uses
request.getRequestURI().startsWith("/admin/") and the
response.sendRedirect("/admin/login") call so it checks servlet path (e.g.,
request.getServletPath().startsWith("/admin")) and redirects to
request.getContextPath() + "/admin/login".
In `@src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskCreator.java`:
- Line 20: The code in EventTaskCreator uses
eventTaskRepository.saveAndFlush(EventTask.pending(outboxEvent, taskType,
targetUserId)) which forces a DB flush per created task and can hurt throughput;
change the call to eventTaskRepository.save(...) to avoid immediate flush unless
synchronous DB visibility is required. Update the invocation in the method that
constructs EventTask via EventTask.pending(...) to call save instead of
saveAndFlush, keeping parameters outboxEvent, taskType, and targetUserId the
same; only switch out saveAndFlush to save where immediate persistence is not
required.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.java`:
- Around line 41-46: Replace hardcoded retry count literals (e.g., the 5 passed
to eventTaskRepository.claimProcessing in CommandEventTaskServiceProcessTest)
with the policy constant EventTaskRetryPolicy.MAX_RETRY_COUNT so tests follow
production policy; update every occurrence noted (the blocks around lines with
claimProcessing calls and any other assertions using the literal) to reference
EventTaskRetryPolicy.MAX_RETRY_COUNT instead of 5.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceTest.java`:
- Line 57: After verifying rewardService.grant(10L, 20L), add a negative
verification that eventTaskFailureNotifier was not invoked to ensure no failure
notification is emitted on the successful path; locate the test method in
CommandEventTaskServiceTest where verify(rewardService).grant(10L, 20L) appears
and append a Mockito no-interaction/never verification against the
eventTaskFailureNotifier (e.g., verifyNoInteractions(eventTaskFailureNotifier)
or verify(eventTaskFailureNotifier, never()).<notifyFailure/...>) to assert it
did not get called.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskCreatorTest.java`:
- Around line 40-43: Add assertions that the created EventTask is initialized
with status PENDING: after capturing the created task (captor.getValue()) and
after the returned result (result), assert their getStatus() equals
EventTaskStatus.PENDING (or use the enum/constant your code uses), ensuring
EventTask.pending(...) contract is validated alongside the existing checks of
getOutboxEvent(), getTaskType(), getTargetUserId(), and result equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7279785-554c-4e05-9e6a-67ff9dd320fe
📒 Files selected for processing (28)
src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.javasrc/main/java/com/dnd/moddo/common/config/SecurityConfig.javasrc/main/java/com/dnd/moddo/common/logging/EventTaskFailureNotifier.javasrc/main/java/com/dnd/moddo/event/application/impl/SettlementUpdater.javasrc/main/java/com/dnd/moddo/event/infrastructure/SettlementRepository.javasrc/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.javasrc/main/java/com/dnd/moddo/outbox/application/command/CommandOutboxEventService.javasrc/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEvent.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskCreator.javasrc/main/java/com/dnd/moddo/outbox/application/impl/EventTaskScheduler.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxReader.javasrc/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.javasrc/main/java/com/dnd/moddo/reward/application/RewardService.javasrc/main/java/com/dnd/moddo/reward/domain/character/exception/SettlementCharacterNotFoundException.javasrc/main/java/com/dnd/moddo/reward/infrastructure/CharacterRepository.javasrc/main/java/com/dnd/moddo/reward/presentation/RewardAdminController.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandOutboxEventServiceTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskCreatorTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/EventTaskSchedulerTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxReaderTest.javasrc/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.javasrc/test/java/com/dnd/moddo/domain/reward/service/RewardServiceGrantTest.javasrc/test/java/com/dnd/moddo/domain/reward/service/RewardServiceTest.javasrc/test/java/com/dnd/moddo/domain/settlement/service/implementation/SettlementUpdaterTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/com/dnd/moddo/domain/reward/controller/RewardAdminControllerTest.java
- src/main/java/com/dnd/moddo/outbox/application/impl/EventTaskScheduler.java
- src/main/java/com/dnd/moddo/reward/presentation/RewardAdminController.java
- src/main/java/com/dnd/moddo/outbox/application/event/OutboxEventCreatedEvent.java
- src/main/java/com/dnd/moddo/outbox/infrastructure/EventTaskRepository.java
src/main/java/com/dnd/moddo/common/logging/EventTaskFailureNotifier.java
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.java
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java
Outdated
Show resolved
Hide resolved
src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java
Outdated
Show resolved
Hide resolved
📝 테스트 커버리지 리포트입니다!
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java (1)
27-36:⚠️ Potential issue | 🟠 Major동일 이벤트 태스크 중복 저장을 막는 유니크 제약이 필요합니다.
현재는
(outbox_id, task_type, target_user_id)중복 row가 들어갈 수 있어 중복 지급 위험이 남아있습니다.제안 수정
import jakarta.persistence.JoinColumn; import jakarta.persistence.ManyToOne; import jakarta.persistence.Table; +import jakarta.persistence.UniqueConstraint; @@ -@Table(name = "event_task") +@Table( + name = "event_task", + uniqueConstraints = `@UniqueConstraint`( + name = "uk_event_task_outbox_type_target", + columnNames = {"outbox_id", "task_type", "target_user_id"} + ) +) `@Entity` public class EventTask {또한 DB 마이그레이션에도 동일 유니크 인덱스를 반드시 추가해 주세요.
#!/bin/bash set -euo pipefail # 1) 엔티티 유니크 제약 선언 확인 sed -n '1,220p' src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java # 2) 마이그레이션/DDL에서 event_task 유니크 인덱스 확인 rg -n -i -C2 'event_task|outbox_id|task_type|target_user_id|unique( index| constraint)?' -g '*.{sql,xml,yml,yaml,java}'Also applies to: 38-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java` around lines 27 - 36, Add a unique constraint to the EventTask entity and corresponding DB migration to prevent duplicate rows for the same event: update the EventTask class to declare a unique index/constraint over (outbox_id, task_type, target_user_id) referencing the fields outboxEvent (outbox_id), taskType, and targetUserId, and then add a migration/DDL change that creates the same UNIQUE INDEX/CONSTRAINT on the event_task table so the database enforces uniqueness as well.src/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.java (1)
16-16:⚠️ Potential issue | 🟠 Major상태별 전체 조회는 적체 시 메모리/처리시간 리스크가 큽니다.
Line 16은 PENDING/FAILED 이벤트가 누적될 때 한 번에 모두 로딩되어 배치 발행 경로에서 병목/OOM 위험이 있습니다. 페이지 기반 조회로 제한하는 게 안전합니다.
변경 예시
-import java.util.List; +import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Slice; @@ - List<OutboxEvent> findAllByStatus(OutboxEventStatus status); + Slice<OutboxEvent> findByStatusOrderByIdAsc(OutboxEventStatus status, Pageable pageable);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.java` at line 16, The repository method findAllByStatus(OutboxEventStatus status) loads all matching rows and risks OOM/batching bottlenecks; change OutboxEventRepository to a paginated API (e.g., replace with a method accepting a Pageable and returning Page<OutboxEvent> or Slice<OutboxEvent>) and update callers (batch publisher/processor) to iterate pages (or slices) and process each page before fetching the next so only a bounded number of events are loaded at a time.src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java (1)
21-24:⚠️ Potential issue | 🟠 MajorPENDING 전체 로딩은 배치 안정성 리스크가 큽니다.
Line 23은 이벤트 적체 시 메모리 사용량과 트랜잭션 체류 시간이 커집니다. 제한된 배치 조회(페이지/슬라이스)로 반복 처리하는 방식이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java` around lines 21 - 24, publishPendingEvents() currently loads all OutboxEvent objects via outboxReader.findAllByStatus(OutboxEventStatus.PENDING), which risks high memory and long transaction time; change the implementation to process events in bounded batches (e.g., pageable or slice queries) and iterate until no more pending items, committing after each batch (remove or limit `@Transactional` scope to per-batch) so each iteration queries a page of PENDING events, processes/publishes them, updates statuses, and proceeds to the next page to avoid loading all events at once.
🧹 Nitpick comments (2)
src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java (1)
93-95: DEAD 전이 시에도 처리 완료 시각을 남기는 것이 좋습니다.현재
COMPLETED와 달리DEAD는processedAt이 비어 운영 추적성이 떨어집니다.제안 수정
public void markDead(String errorMessage) { this.status = EventTaskStatus.DEAD; + this.processedAt = LocalDateTime.now(); this.lastErrorMessage = errorMessage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java` around lines 93 - 95, The markDead method in EventTask should also record the processing completion timestamp like markCompleted does; update EventTask.markDead to set this.processedAt to the current time (same time source/format used by markCompleted) in addition to setting this.status = EventTaskStatus.DEAD and this.lastErrorMessage = errorMessage so DEAD transitions retain processedAt for operational tracing.src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublishExecutor.java (1)
55-60: 참여자 수가 많을 때 태스크 생성 flush 비용이 커질 수 있습니다.Line 56-59는 멤버별 2건 생성 루프인데, 현재
EventTaskCreator.create가saveAndFlush를 사용하므로 대량 참여자에서 DB 부하가 급격히 증가할 수 있습니다. 배치 저장(또는 flush 주기 제어) 방식으로 바꾸는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublishExecutor.java` around lines 55 - 60, appendSettlementCompletedTasks is creating two tasks per member in a tight loop using eventTaskCreator.create (which currently does saveAndFlush), causing DB flush overhead for large member lists; modify this method to collect EventTask instances for each member (use memberReader.findAssignedMembersBySettlementId and build tasks of types EventTaskType.REWARD_GRANT and EventTaskType.NOTIFICATION_SEND) and then persist them in bulk with a single batch save/flush call (e.g., add a new eventTaskCreator.createAll or use eventTaskRepository.saveAll and one flush at the end), or implement a controlled flush interval if full batching is not possible. Ensure you only change appendSettlementCompletedTasks and related eventTaskCreator persistence behavior so the loop no longer invokes saveAndFlush per task.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 39-45: The current failure path calls outboxReader.findById(...)
for logging before calling outboxEventPublishExecutor.markFailed(...), which can
throw and prevent marking the event failed; change the order in
OutboxEventPublisher so you call
outboxEventPublishExecutor.markFailed(outboxEventId) first to guarantee state
transition, then attempt to load OutboxEvent via
outboxReader.findById(outboxEventId) only for richer logging (wrap this
lookup/log in its own try/catch and fall back to a minimal log if lookup fails),
and keep the original log.error call using
outboxEvent.getId()/getEventType()/getAggregateId() only when the find succeeds.
- Around line 21-28: publishPendingEvents in OutboxEventPublisher is never
scheduled so PENDING outbox rows can be left unprocessed; either add a periodic
trigger or wire it into the existing scheduler. Modify
OutboxEventPublisher.publishPendingEvents to be invoked regularly (for example
annotate the method with `@Scheduled` with a sensible fixedDelay/fixedRate or call
it from EventTaskScheduler.processPendingTasks), and ensure the loop over
outboxReader.findAllByStatus(OutboxEventStatus.PENDING) handles each record
independently (catch/log exceptions around publish(outboxEvent.getId()) so one
failure doesn't stop the batch) so retrying works after transaction rollbacks.
---
Duplicate comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 21-24: publishPendingEvents() currently loads all OutboxEvent
objects via outboxReader.findAllByStatus(OutboxEventStatus.PENDING), which risks
high memory and long transaction time; change the implementation to process
events in bounded batches (e.g., pageable or slice queries) and iterate until no
more pending items, committing after each batch (remove or limit `@Transactional`
scope to per-batch) so each iteration queries a page of PENDING events,
processes/publishes them, updates statuses, and proceeds to the next page to
avoid loading all events at once.
In `@src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java`:
- Around line 27-36: Add a unique constraint to the EventTask entity and
corresponding DB migration to prevent duplicate rows for the same event: update
the EventTask class to declare a unique index/constraint over (outbox_id,
task_type, target_user_id) referencing the fields outboxEvent (outbox_id),
taskType, and targetUserId, and then add a migration/DDL change that creates the
same UNIQUE INDEX/CONSTRAINT on the event_task table so the database enforces
uniqueness as well.
In
`@src/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.java`:
- Line 16: The repository method findAllByStatus(OutboxEventStatus status) loads
all matching rows and risks OOM/batching bottlenecks; change
OutboxEventRepository to a paginated API (e.g., replace with a method accepting
a Pageable and returning Page<OutboxEvent> or Slice<OutboxEvent>) and update
callers (batch publisher/processor) to iterate pages (or slices) and process
each page before fetching the next so only a bounded number of events are loaded
at a time.
---
Nitpick comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublishExecutor.java`:
- Around line 55-60: appendSettlementCompletedTasks is creating two tasks per
member in a tight loop using eventTaskCreator.create (which currently does
saveAndFlush), causing DB flush overhead for large member lists; modify this
method to collect EventTask instances for each member (use
memberReader.findAssignedMembersBySettlementId and build tasks of types
EventTaskType.REWARD_GRANT and EventTaskType.NOTIFICATION_SEND) and then persist
them in bulk with a single batch save/flush call (e.g., add a new
eventTaskCreator.createAll or use eventTaskRepository.saveAll and one flush at
the end), or implement a controlled flush interval if full batching is not
possible. Ensure you only change appendSettlementCompletedTasks and related
eventTaskCreator persistence behavior so the loop no longer invokes saveAndFlush
per task.
In `@src/main/java/com/dnd/moddo/outbox/domain/task/EventTask.java`:
- Around line 93-95: The markDead method in EventTask should also record the
processing completion timestamp like markCompleted does; update
EventTask.markDead to set this.processedAt to the current time (same time
source/format used by markCompleted) in addition to setting this.status =
EventTaskStatus.DEAD and this.lastErrorMessage = errorMessage so DEAD
transitions retain processedAt for operational tracing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 11867baa-bd8d-4014-94b9-697232986b1d
📒 Files selected for processing (11)
src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.javasrc/main/java/com/dnd/moddo/common/config/SecurityConfig.javasrc/main/java/com/dnd/moddo/outbox/application/command/CommandEventTaskService.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublishExecutor.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.javasrc/main/java/com/dnd/moddo/outbox/domain/event/type/OutboxEventStatus.javasrc/main/java/com/dnd/moddo/outbox/domain/task/EventTask.javasrc/main/java/com/dnd/moddo/outbox/domain/task/type/EventTaskStatus.javasrc/main/java/com/dnd/moddo/outbox/infrastructure/OutboxEventRepository.javasrc/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/com/dnd/moddo/common/config/SecurityConfig.java
- src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/dnd/moddo/outbox/domain/task/type/EventTaskStatus.java
- src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.java
- src/test/java/com/dnd/moddo/domain/outbox/service/CommandEventTaskServiceProcessTest.java
- src/main/java/com/dnd/moddo/outbox/domain/event/type/OutboxEventStatus.java
src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java
Outdated
Show resolved
Hide resolved
📝 테스트 커버리지 리포트입니다!
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.java (1)
1-1: 테스트 패키지 경로가 프로덕션 코드 패키지와 일치하지 않습니다.테스트 클래스는
com.dnd.moddo.domain.outbox.service패키지에 있지만, 테스트 대상 클래스OutboxEventCreatedEvent는com.dnd.moddo.outbox.application.event패키지에 위치합니다. 일반적인 관례상 테스트 패키지 경로를 프로덕션 코드와 동일하게 맞추는 것이 좋습니다.📦 패키지 경로 수정 제안
-package com.dnd.moddo.domain.outbox.service; +package com.dnd.moddo.outbox.application.event;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.java` at line 1, The test package declaration is incorrect: OutboxEventCreatedEventTest currently uses package com.dnd.moddo.domain.outbox.service while the production class OutboxEventCreatedEvent lives in com.dnd.moddo.outbox.application.event; update the test to use the identical package com.dnd.moddo.outbox.application.event (and move the test file to the matching directory if your project layout enforces it), then adjust any imports or visibility issues in OutboxEventCreatedEventTest to reference OutboxEventCreatedEvent and its collaborators correctly.src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventScheduler.java (1)
13-16: 스케줄러 메서드에 예외 처리 추가 권장
publishPendingEvents()호출 중 예상치 못한 예외(예: DB 연결 실패)가 발생하면 스케줄러가 중단될 수 있습니다. 스케줄러의 안정적인 동작을 위해 예외 처리를 추가하는 것을 권장드립니다.🛡️ 예외 처리 추가 예시
+import lombok.extern.slf4j.Slf4j; + `@Component` `@RequiredArgsConstructor` +@Slf4j public class OutboxEventScheduler { private final OutboxEventPublisher outboxEventPublisher; `@Scheduled`(fixedDelay = 5000) public void publishPendingEvents() { - outboxEventPublisher.publishPendingEvents(); + try { + outboxEventPublisher.publishPendingEvents(); + } catch (Exception e) { + log.error("Failed to publish pending outbox events", e); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventScheduler.java` around lines 13 - 16, OutboxEventScheduler.publishPendingEvents currently calls outboxEventPublisher.publishPendingEvents() without handling runtime errors, so wrap that call in a try-catch that catches Throwable (or at least Exception) inside the publishPendingEvents method to prevent the scheduler from stopping; on catch, log the error with context (include exception message/stack) using your logger and ensure the method returns normally so the `@Scheduled` loop continues.src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublishExecutorTest.java (2)
78-90:SETTLEMENT_COMPLETED외 이벤트 타입에 대한 테스트 추가 권장현재 테스트는
SETTLEMENT_COMPLETED이벤트 타입만 커버하고 있습니다.appendTasks()메서드는SETTLEMENT_COMPLETED가 아닌 경우 아무 작업도 하지 않고 리턴하는데(silent no-op), 이 동작을 명시적으로 검증하는 테스트가 있으면 향후 새로운 이벤트 타입 추가 시 의도치 않은 동작 변경을 방지할 수 있습니다.💡 테스트 케이스 예시
`@Test` `@DisplayName`("SETTLEMENT_COMPLETED 외 이벤트 타입은 태스크를 생성하지 않는다.") void skipAppendTasksForOtherEventTypes() { // 현재 enum에 SETTLEMENT_COMPLETED만 있지만, // 향후 다른 타입 추가 시 이 테스트가 의도된 동작을 보장합니다. OutboxEvent outboxEvent = mock(OutboxEvent.class); when(outboxReader.findById(1L)).thenReturn(outboxEvent); // 가상의 다른 이벤트 타입이 있다고 가정 // when(outboxEvent.getEventType()).thenReturn(OutboxEventType.OTHER_TYPE); outboxEventPublishExecutor.appendTasks(1L); verifyNoInteractions(eventTaskCreator); verifyNoInteractions(memberReader); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublishExecutorTest.java` around lines 78 - 90, Add a unit test that verifies OutboxEventPublishExecutor.appendTasks does nothing for event types other than SETTLEMENT_COMPLETED: mock an OutboxEvent and have outboxReader.findById return it, stub outboxEvent.getEventType() to a non-SETTLEMENT_COMPLETED value, call outboxEventPublishExecutor.appendTasks(1L), and then assert no interactions with eventTaskCreator and memberReader (e.g., verifyNoInteractions(eventTaskCreator); verifyNoInteractions(memberReader);) so future additions of OutboxEventType cannot change the silent no-op behavior.
53-54: AssertJ 정적 임포트 사용 권장다른 Mockito 메서드들은 정적 임포트(
import static org.mockito.Mockito.*)를 사용하고 있는데, AssertJ는 전체 경로를 사용하고 있습니다. 일관성을 위해 정적 임포트를 추천드립니다.♻️ 정적 임포트로 개선
+import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.*; // ... - org.assertj.core.api.Assertions.assertThat(claimed).isTrue(); - org.assertj.core.api.Assertions.assertThat(notClaimed).isFalse(); + assertThat(claimed).isTrue(); + assertThat(notClaimed).isFalse();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublishExecutorTest.java` around lines 53 - 54, Replace fully-qualified AssertJ calls in OutboxEventPublishExecutorTest with static imports for consistency: change usages of org.assertj.core.api.Assertions.assertThat(claimed) and org.assertj.core.api.Assertions.assertThat(notClaimed) to assertThat(claimed) and assertThat(notClaimed), and add the static import import static org.assertj.core.api.Assertions.assertThat; at the top of the test class so all AssertJ assertions use the static form like the other Mockito static imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java`:
- Around line 21-28: publishPendingEvents() currently runs under a single
`@Transactional` and iterates pendingEvents so any exception from publish()
(including failures in markFailed) aborts the loop and may roll back the whole
batch; change to fetch pendingEvents outside a transaction, remove or narrow the
`@Transactional` on publishPendingEvents(), and handle each OutboxEvent
individually by invoking publish(eventId) inside a try/catch that logs the error
and attempts to markFailed(eventId) (or calls the existing markFailed method)
without rethrowing so the loop continues; use
outboxReader.findAllByStatus(OutboxEventStatus.PENDING) to get the batch and
ensure publish(), markFailed(), and any per-event transactional annotations
handle per-event commit/rollback.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.java`:
- Around line 20-30: Update the error message in the OutboxEventCreatedEvent
constructor to match the validation (outboxEventId must be positive) and update
the test expectations accordingly: change the thrown IllegalArgumentException
message produced by the OutboxEventCreatedEvent(Long outboxEventId) guard
(currently triggered when outboxEventId <= 0) to a message that says the id must
be positive (e.g., "outboxEventId가 양수여야 합니다.") and adjust the assertions in
OutboxEventCreatedEventTest.assertThatThrownBy(...) to expect that new message
for both 0L and -1L cases.
---
Nitpick comments:
In
`@src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventScheduler.java`:
- Around line 13-16: OutboxEventScheduler.publishPendingEvents currently calls
outboxEventPublisher.publishPendingEvents() without handling runtime errors, so
wrap that call in a try-catch that catches Throwable (or at least Exception)
inside the publishPendingEvents method to prevent the scheduler from stopping;
on catch, log the error with context (include exception message/stack) using
your logger and ensure the method returns normally so the `@Scheduled` loop
continues.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublishExecutorTest.java`:
- Around line 78-90: Add a unit test that verifies
OutboxEventPublishExecutor.appendTasks does nothing for event types other than
SETTLEMENT_COMPLETED: mock an OutboxEvent and have outboxReader.findById return
it, stub outboxEvent.getEventType() to a non-SETTLEMENT_COMPLETED value, call
outboxEventPublishExecutor.appendTasks(1L), and then assert no interactions with
eventTaskCreator and memberReader (e.g., verifyNoInteractions(eventTaskCreator);
verifyNoInteractions(memberReader);) so future additions of OutboxEventType
cannot change the silent no-op behavior.
- Around line 53-54: Replace fully-qualified AssertJ calls in
OutboxEventPublishExecutorTest with static imports for consistency: change
usages of org.assertj.core.api.Assertions.assertThat(claimed) and
org.assertj.core.api.Assertions.assertThat(notClaimed) to assertThat(claimed)
and assertThat(notClaimed), and add the static import import static
org.assertj.core.api.Assertions.assertThat; at the top of the test class so all
AssertJ assertions use the static form like the other Mockito static imports.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.java`:
- Line 1: The test package declaration is incorrect: OutboxEventCreatedEventTest
currently uses package com.dnd.moddo.domain.outbox.service while the production
class OutboxEventCreatedEvent lives in com.dnd.moddo.outbox.application.event;
update the test to use the identical package
com.dnd.moddo.outbox.application.event (and move the test file to the matching
directory if your project layout enforces it), then adjust any imports or
visibility issues in OutboxEventCreatedEventTest to reference
OutboxEventCreatedEvent and its collaborators correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffe1909f-9277-44cd-a0c3-43f4899ae6cd
📒 Files selected for processing (6)
src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.javasrc/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventScheduler.javasrc/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublishExecutorTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.javasrc/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventSchedulerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventSchedulerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/dnd/moddo/domain/outbox/service/implementation/OutboxEventPublisherTest.java
src/main/java/com/dnd/moddo/outbox/application/impl/OutboxEventPublisher.java
Outdated
Show resolved
Hide resolved
Comment on lines
+20
to
+30
| @Test | ||
| @DisplayName("0 이하 outboxEventId로 이벤트를 생성하면 예외가 발생한다.") | ||
| void throwExceptionWhenOutboxEventIdIsNotPositive() { | ||
| assertThatThrownBy(() -> new OutboxEventCreatedEvent(0L)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("outboxEventId가 0이상이어야 합니다."); | ||
|
|
||
| assertThatThrownBy(() -> new OutboxEventCreatedEvent(-1L)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("outboxEventId가 0이상이어야 합니다."); | ||
| } |
There was a problem hiding this comment.
프로덕션 코드의 에러 메시지가 실제 검증 로직과 불일치합니다.
테스트는 현재 프로덕션 코드의 동작을 정확하게 검증하고 있지만, 프로덕션 코드 자체에 의미적 불일치가 있습니다:
- 검증 조건:
if (outboxEventId <= 0)→ 0 이하를 거부 (양수만 허용) - 에러 메시지:
"outboxEventId가 0이상이어야 합니다."→ 0 이상이어야 한다고 명시
조건은 "양수여야 한다"인데, 메시지는 "0 이상이어야 한다"라고 하여 서로 모순됩니다.
프로덕션 코드(OutboxEventCreatedEvent.java)의 에러 메시지를 실제 검증 로직에 맞게 수정하고, 테스트의 기대 메시지도 함께 업데이트해야 합니다.
🐛 프로덕션 코드 및 테스트 수정 제안
프로덕션 코드 (OutboxEventCreatedEvent.java) 수정:
public record OutboxEventCreatedEvent(Long outboxEventId) {
public OutboxEventCreatedEvent {
if (outboxEventId <= 0) {
throw new IllegalArgumentException("outboxEventId가 양수여야 합니다.");
}
}
}테스트 코드 수정:
`@Test`
`@DisplayName`("0 이하 outboxEventId로 이벤트를 생성하면 예외가 발생한다.")
void throwExceptionWhenOutboxEventIdIsNotPositive() {
assertThatThrownBy(() -> new OutboxEventCreatedEvent(0L))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("outboxEventId가 0이상이어야 합니다.");
+ .hasMessage("outboxEventId가 양수여야 합니다.");
assertThatThrownBy(() -> new OutboxEventCreatedEvent(-1L))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("outboxEventId가 0이상이어야 합니다.");
+ .hasMessage("outboxEventId가 양수여야 합니다.");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/dnd/moddo/domain/outbox/service/OutboxEventCreatedEventTest.java`
around lines 20 - 30, Update the error message in the OutboxEventCreatedEvent
constructor to match the validation (outboxEventId must be positive) and update
the test expectations accordingly: change the thrown IllegalArgumentException
message produced by the OutboxEventCreatedEvent(Long outboxEventId) guard
(currently triggered when outboxEventId <= 0) to a message that says the id must
be positive (e.g., "outboxEventId가 양수여야 합니다.") and adjust the assertions in
OutboxEventCreatedEventTest.assertThatThrownBy(...) to expect that new message
for both 0L and -1L cases.
📝 테스트 커버리지 리포트입니다!
|
📝 테스트 커버리지 리포트입니다!
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#️⃣연관된 이슈
X
🔀반영 브랜치
feature/payment-request -> develop
🔧변경 사항
💬리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Chores