feat: 전체 참여자 입금 승인 시 정산 자동 완료 처리 및 캐릭터 자동 지급 기능 추가#34
Open
feat: 전체 참여자 입금 승인 시 정산 자동 완료 처리 및 캐릭터 자동 지급 기능 추가#34
Conversation
Walkthrough정산 완료 처리와 아웃박스 패턴을 기반으로 한 이벤트 기반 아키텍처를 도입합니다. 정산 완료 시 리워드 및 알림 작업을 이벤트 큐에 등록하고, 스케줄러가 주기적으로 이를 처리합니다. 관리자용 컨트롤러와 보안 정책을 추가합니다. 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 EventTaskScheduler as EventTaskScheduler
participant CommandEventTaskService as CommandEventTaskService
participant RewardService as RewardService
Client->>CommandPaymentRequest: 결제 승인
CommandPaymentRequest->>SettlementCompletionProcessor: completeIfAllPaid(settlementId)
SettlementCompletionProcessor->>MemberReader: existsUnpaidMember(settlementId)
alt 미지급 멤버 없음
MemberReader-->>SettlementCompletionProcessor: false
SettlementCompletionProcessor->>SettlementUpdater: complete(settlementId)
SettlementUpdater-->>SettlementCompletionProcessor: true
SettlementCompletionProcessor->>CommandOutboxEventService: create(SETTLEMENT_COMPLETED, ...)
CommandOutboxEventService-->>SettlementCompletionProcessor: OutboxEvent
SettlementCompletionProcessor-->>CommandPaymentRequest: true
else 미지급 멤버 존재
MemberReader-->>SettlementCompletionProcessor: true
SettlementCompletionProcessor-->>CommandPaymentRequest: false
end
rect rgba(135, 206, 250, 0.5)
Note over OutboxEventPublisher,RewardService: 백그라운드 처리
OutboxEventPublisher->>OutboxEventPublisher: publish(outboxEventId)
OutboxEventPublisher->>CommandEventTaskService: 이벤트 태스크 생성
EventTaskScheduler->>EventTaskScheduler: processPendingTasks()
EventTaskScheduler->>CommandEventTaskService: process(eventTaskId)
CommandEventTaskService->>RewardService: grant(settlementId, userId)
RewardService-->>CommandEventTaskService: 완료
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)
📝 Coding Plan
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
Comment on lines
+30
to
+39
| Long userId = claims.get(JwtConstants.AUTH_ID.message, Long.class); | ||
| String role = claims.get(JwtConstants.ROLE.message, String.class); | ||
|
|
||
| if (Authority.ADMIN.name().equals(role)) { | ||
| UserDetails adminDetails = new AuthDetails(userId, "admin", role); | ||
| return new UsernamePasswordAuthenticationToken(adminDetails, "", adminDetails.getAuthorities()); | ||
| } | ||
|
|
||
| UserDetails userDetails = authDetailsService.loadUserByUsername( | ||
| claims.get(JwtConstants.AUTH_ID.message).toString()); | ||
| userId.toString()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# JWT 생성 시 AUTH_ID/ROLE 클레임이 항상 들어가는지 확인
rg -n --type=java 'AUTH_ID\.message|ROLE\.message|Jwts\.builder|setClaims|claim\('
rg -n --type=java 'get\(JwtConstants\.AUTH_ID\.message, Long\.class\)|get\(JwtConstants\.ROLE\.message, String\.class\)'Repository: moddo-kr/moddo-backend
Length of output: 1181
🏁 Script executed:
cat -n src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.javaRepository: moddo-kr/moddo-backend
Length of output: 2353
🏁 Script executed:
cat -n src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtProvider.javaRepository: moddo-kr/moddo-backend
Length of output: 4937
🏁 Script executed:
rg -n --type=java "TokenInvalidException" | head -20Repository: moddo-kr/moddo-backend
Length of output: 1985
🏁 Script executed:
rg -n --type=java "class JwtAuth|void|JWT|throws" src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.java -A 5 -B 2Repository: moddo-kr/moddo-backend
Length of output: 369
필수 클레임 누락 시 인증 로직에서 NPE 발생 가능
userId 또는 role 클레임이 누락되거나 파싱 실패인 경우, 라인 34와 39에서 NPE가 발생하여 HTTP 500 서버 에러가 반환됩니다. JWT 생성 시 항상 포함되지만, 토큰 변조나 예상치 못한 상황에서 클레임이 없을 수 있으므로 방어 처리가 필요합니다.
라인 31 이후에 필수값 검증을 추가하고 TokenInvalidException으로 처리해서 인증 오류를 명확히 구분하세요.
수정 예시
Long userId = claims.get(JwtConstants.AUTH_ID.message, Long.class);
String role = claims.get(JwtConstants.ROLE.message, String.class);
+
+ if (userId == null || role == null) {
+ throw new TokenInvalidException();
+ }
if (Authority.ADMIN.name().equals(role)) {
UserDetails adminDetails = new AuthDetails(userId, "admin", role);
return new UsernamePasswordAuthenticationToken(adminDetails, "", adminDetails.getAuthorities());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/dnd/moddo/auth/infrastructure/security/JwtAuth.java` around
lines 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.
src/main/java/com/dnd/moddo/common/logging/EventTaskFailureNotifier.java
Show resolved
Hide resolved
Comment on lines
+40
to
+44
| if (eventTask.getTaskType() == EventTaskType.REWARD_GRANT) { | ||
| rewardService.grant(eventTask.getOutboxEvent().getAggregateId(), eventTask.getTargetUserId()); | ||
| } else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
미지원 태스크 타입이 PROCESSING 상태로 고착됩니다.
Line 42~44에서 return하면 완료/실패 전이가 일어나지 않습니다. 선점된 태스크가 PROCESSING에 남아 재시도 대상(PENDING/FAILED)에서 영구 제외될 수 있습니다.
제안 수정
try {
if (eventTask.getTaskType() == EventTaskType.REWARD_GRANT) {
rewardService.grant(eventTask.getOutboxEvent().getAggregateId(), eventTask.getTargetUserId());
} else {
- return;
+ throw new IllegalStateException("Unsupported task type: " + eventTask.getTaskType());
}
eventTask.markCompleted();
} catch (Exception 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/outbox/application/command/CommandEventTaskService.java`
around lines 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.
Comment on lines
+36
to
+41
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public void publish(Long outboxEventId) { | ||
| OutboxEvent outboxEvent = outboxReader.findById(outboxEventId); | ||
| if (outboxEvent.getStatus() != OutboxEventStatus.PENDING) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
동일 outboxEventId 중복 발행 경쟁 조건이 있습니다.
Line 38-40의 상태 확인이 원자적이지 않아 동시 호출 시 둘 다 PENDING을 통과할 수 있고, Line 63-67 태스크가 중복 생성될 수 있습니다. PENDING -> PROCESSING 선점(원자적 update) 또는 optimistic lock으로 단일 처리 보장을 추가해주세요.
Also applies to: 63-67
🤖 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 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).
Comment on lines
+43
to
+53
| try { | ||
| appendTasks(outboxEvent); | ||
| outboxEvent.markPublished(); | ||
| } catch (Exception exception) { | ||
| log.error("Failed to publish outbox event. outboxEventId={}, eventType={}, aggregateId={}", | ||
| outboxEvent.getId(), | ||
| outboxEvent.getEventType(), | ||
| outboxEvent.getAggregateId(), | ||
| exception); | ||
| outboxEvent.markFailed(); | ||
| } |
There was a problem hiding this comment.
예외를 삼키면 부분 생성된 태스크가 커밋될 수 있습니다.
현재 구조는 appendTasks 중간 실패 시 일부 태스크가 저장된 채 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/application/impl/OutboxEventPublisher.java`
around lines 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.
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
Release Notes
New Features
Chores