[WTH-146] penalty 도메인 코틀린 마이그레이션#10
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughJava 기반의 패널티 도메인(엔티티, DTO, 예외, 매퍼, 서비스, 컨트롤러, 레포지토리 등) 전체가 제거되고, 동일한 도메인이 Kotlin으로 재구현되었습니다. CQRS 패턴으로 커맨드/쿼리 유스케이스가 분리되며 DTO, 매퍼, 예외, 레포지토리, 컨트롤러 및 관련 로직이 Kotlin으로 추가되었습니다. 또한 빌드 설정에 Lombok Gradle 플러그인이 추가되고 UserRepository에 락 조회 메서드가 추가되었습니다. 변경사항
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as PenaltyAdminController
participant UseCase as SavePenaltyUseCase
participant UserRepo as UserRepository
participant CardinalSvc as UserCardinalGetService
participant Mapper as PenaltyMapper
participant PenaltyRepo as PenaltyRepository
Client->>Controller: POST /api/v1/admin/penalties (SavePenaltyRequest)
Controller->>UseCase: save(request)
UseCase->>UserRepo: findByIdWithLock(userId)
Note right of UserRepo: PESSIMISTIC_WRITE lock (2s timeout)
UserRepo-->>UseCase: User
UseCase->>CardinalSvc: getCurrentCardinal(user)
CardinalSvc-->>UseCase: Cardinal
UseCase->>Mapper: toEntity(request, user, cardinal)
Mapper-->>UseCase: Penalty(entity)
UseCase->>PenaltyRepo: save(penalty)
PenaltyRepo-->>UseCase: savedPenalty
alt request.penaltyType == WARNING and warningCount becomes even
UseCase->>PenaltyRepo: save(autoPenalty)
PenaltyRepo-->>UseCase: savedAutoPenalty
end
UseCase-->>Controller: void
Controller-->>Client: 200 PENALTY_ASSIGN_SUCCESS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50분 제안 검토자
시
🚥 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/kotlin/com/weeth/domain/penalty/application/dto/request/SavePenaltyRequest.kt (1)
6-13: 입력 유효성 검증 어노테이션 추가를 권장합니다.
userId에 대한@field:NotNull또는@field:Positive같은 Bean Validation 어노테이션이 없습니다. 컨트롤러에서@Valid와 함께 사용할 경우 유효성 검증이 필요할 수 있습니다.💡 유효성 검증 어노테이션 추가 제안
package com.weeth.domain.penalty.application.dto.request import com.weeth.domain.penalty.domain.enums.PenaltyType import io.swagger.v3.oas.annotations.media.Schema +import jakarta.validation.constraints.NotNull +import jakarta.validation.constraints.Positive data class SavePenaltyRequest( `@field`:Schema(description = "패널티 대상 사용자 ID", example = "1") + `@field`:NotNull + `@field`:Positive val userId: Long, `@field`:Schema(description = "패널티 유형", example = "WARNING") + `@field`:NotNull val penaltyType: PenaltyType, `@field`:Schema(description = "패널티 사유", example = "정기모임 무단 불참") val penaltyDescription: String?, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/com/weeth/domain/penalty/application/dto/request/SavePenaltyRequest.kt` around lines 6 - 13, The SavePenaltyRequest DTO is missing Bean Validation annotations; update the data class SavePenaltyRequest to add appropriate validation on fields (e.g., annotate userId with `@field`:NotNull and `@field`:Positive, annotate penaltyType with `@field`:NotNull, and optionally add `@field`:Size or `@field`:NotBlank on penaltyDescription as desired) so that controller-level `@Valid` checks work; ensure you import the javax.validation.constraints annotations and keep the annotations on the field target (using `@field`:...) for Kotlin data class properties.src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt (2)
26-29: Java Optional 패턴 대신 Kotlin 관용구 사용 권장
orElseThrow는 Java Optional 패턴입니다. 코딩 가이드라인에 따라 Kotlin의 Elvis 연산자(?:)나requireNotNull()을 사용하는 것이 좋습니다.♻️ Kotlin 관용구로 변경
- val user = - userRepository - .findByIdWithLock(request.userId) - .orElseThrow { UserNotFoundException() } + val user = + userRepository.findByIdWithLock(request.userId) + ?: throw UserNotFoundException()단,
findByIdWithLock의 반환 타입이Optional<User>인 경우.orElse(null)을 먼저 호출하거나, 반환 타입을User?로 변경해야 합니다.As per coding guidelines: "Use safe navigation operators (?.), Elvis operator (?:), and requireNotNull() — avoid !! in Kotlin code"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt` around lines 26 - 29, The code uses Java Optional.orElseThrow when retrieving the user in SavePenaltyUseCase; replace this with Kotlin idioms by converting the Optional to a nullable User (e.g., call .orElse(null) on the Optional returned by userRepository.findByIdWithLock) and then use the Elvis operator or requireNotNull to throw UserNotFoundException (e.g., val user = requireNotNull(optionalUser ?: throw UserNotFoundException()) or val user = optionalUser ?: throw UserNotFoundException()), so you avoid Java Optional.orElseThrow and follow Kotlin conventions.
40-50: 비즈니스 로직을 Entity로 이동 고려경고 카운트에 따른 자동 패널티 생성 규칙(
warningCount % 2 == 0)은 비즈니스 로직입니다. Rich Domain Model 패턴에 따르면, 이 로직을User엔티티로 이동하는 것이 좋습니다.♻️ User 엔티티에 메서드 추가 예시
User 엔티티에 추가:
fun shouldCreateAutoPenalty(): Boolean = warningCount % 2 == 0UseCase에서 사용:
PenaltyType.WARNING -> { user.incrementWarningCount() - val warningCount = user.warningCount - if (warningCount % 2 == 0) { - val description = AUTO_PENALTY_DESCRIPTION.format(warningCount) + if (user.shouldCreateAutoPenalty()) { + val description = AUTO_PENALTY_DESCRIPTION.format(user.warningCount) val autoPenalty = mapper.toAutoPenalty(description, user, cardinal) penaltyRepository.save(autoPenalty) user.incrementPenaltyCount() } }Based on learnings: "UseCase classes orchestrate only; business logic lives in Entities following Rich Domain Model pattern"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt` around lines 40 - 50, Move the auto-penalty decision out of the use case into the User entity: add a method on User (e.g., fun shouldCreateAutoPenalty(): Boolean = warningCount % 2 == 0) and replace the inline check in SavePenaltyUseCase when handling PenaltyType.WARNING so that after calling user.incrementWarningCount() the use case calls user.shouldCreateAutoPenalty(); if true, build the description with AUTO_PENALTY_DESCRIPTION, call mapper.toAutoPenalty(description, user, cardinal) and penaltyRepository.save(autoPenalty) and then call user.incrementPenaltyCount() — this keeps orchestration in SavePenaltyUseCase and the rule in the User entity.src/main/kotlin/com/weeth/domain/penalty/presentation/PenaltyAdminController.kt (1)
60-67: DELETE 엔드포인트의 RESTful 설계 고려현재
@RequestParam으로penaltyId를 받고 있지만, RESTful 컨벤션에서는 리소스 ID를 path variable로 사용하는 것이 일반적입니다:DELETE /api/v1/admin/penalties/{penaltyId}현재 구현도 동작에는 문제없으므로 선택적 개선 사항입니다.
♻️ Path variable 사용 예시
- `@DeleteMapping` + `@DeleteMapping`("/{penaltyId}") `@Operation`(summary = "패널티 삭제") fun delete( - `@RequestParam` penaltyId: Long, + `@PathVariable` penaltyId: Long, ): CommonResponse<Void?> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/com/weeth/domain/penalty/presentation/PenaltyAdminController.kt` around lines 60 - 67, Change the DELETE handler in PenaltyAdminController to follow RESTful conventions by moving penaltyId from a request parameter to a path variable: update the mapping annotation on the delete method to include the path segment for the ID (e.g., "/{penaltyId}"), change the method parameter annotation from `@RequestParam` to `@PathVariable` for penaltyId, and leave the invocation of deletePenaltyUseCase.delete(penaltyId) and the CommonResponse return unchanged so behavior is preserved.src/main/kotlin/com/weeth/domain/penalty/domain/repository/PenaltyRepository.kt (1)
10-15:findByUserIdAndCardinalIdOrderByIdDesc의 JOIN FETCH 전략 확인 필요이 쿼리는
p.cardinal만 JOIN FETCH하고p.user는 포함하지 않습니다.GetPenaltyQueryService.findByUser()에서 이 메서드를 호출한 후penalties.map(::toDetailResponse)를 통해 penalty를 매핑할 때penalty.cardinal에 접근하므로 cardinal은 문제없지만, 만약 user 속성에 접근하는 경우가 있다면 N+1 문제가 발생할 수 있습니다.현재 사용처에서는 user를 별도로 조회하므로 문제없어 보이지만, 일관성을 위해
findByCardinalIdOrderByIdDesc처럼p.user도 함께 fetch하는 것을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/com/weeth/domain/penalty/domain/repository/PenaltyRepository.kt` around lines 10 - 15, The current JPQL in PenaltyRepository.findByUserIdAndCardinalIdOrderByIdDesc only JOIN FETCHes p.cardinal which can cause N+1 if callers later access p.user; update the query to also FETCH the user (e.g., add "JOIN FETCH p.user") so both associations are eagerly loaded, or alternatively add a separate repository method (like findByCardinalIdOrderByIdDesc) that JOIN FETCHes both p.cardinal and p.user; update any usages such as GetPenaltyQueryService.findByUser() to call the new/updated method if needed.
🤖 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/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt`:
- Line 52: The when expression in SavePenaltyUseCase silently ignores
PenaltyType.AUTO_PENALTY via an empty else branch; update the when over
PenaltyType (in SavePenaltyUseCase) to explicitly handle AUTO_PENALTY (e.g.,
implement the intended logic or, if unsupported, throw a clear exception like
IllegalArgumentException/UnsupportedOperationException mentioning AUTO_PENALTY)
instead of leaving else -> {} so the case is not silently skipped.
---
Nitpick comments:
In
`@src/main/kotlin/com/weeth/domain/penalty/application/dto/request/SavePenaltyRequest.kt`:
- Around line 6-13: The SavePenaltyRequest DTO is missing Bean Validation
annotations; update the data class SavePenaltyRequest to add appropriate
validation on fields (e.g., annotate userId with `@field`:NotNull and
`@field`:Positive, annotate penaltyType with `@field`:NotNull, and optionally add
`@field`:Size or `@field`:NotBlank on penaltyDescription as desired) so that
controller-level `@Valid` checks work; ensure you import the
javax.validation.constraints annotations and keep the annotations on the field
target (using `@field`:...) for Kotlin data class properties.
In
`@src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt`:
- Around line 26-29: The code uses Java Optional.orElseThrow when retrieving the
user in SavePenaltyUseCase; replace this with Kotlin idioms by converting the
Optional to a nullable User (e.g., call .orElse(null) on the Optional returned
by userRepository.findByIdWithLock) and then use the Elvis operator or
requireNotNull to throw UserNotFoundException (e.g., val user =
requireNotNull(optionalUser ?: throw UserNotFoundException()) or val user =
optionalUser ?: throw UserNotFoundException()), so you avoid Java
Optional.orElseThrow and follow Kotlin conventions.
- Around line 40-50: Move the auto-penalty decision out of the use case into the
User entity: add a method on User (e.g., fun shouldCreateAutoPenalty(): Boolean
= warningCount % 2 == 0) and replace the inline check in SavePenaltyUseCase when
handling PenaltyType.WARNING so that after calling user.incrementWarningCount()
the use case calls user.shouldCreateAutoPenalty(); if true, build the
description with AUTO_PENALTY_DESCRIPTION, call
mapper.toAutoPenalty(description, user, cardinal) and
penaltyRepository.save(autoPenalty) and then call user.incrementPenaltyCount() —
this keeps orchestration in SavePenaltyUseCase and the rule in the User entity.
In
`@src/main/kotlin/com/weeth/domain/penalty/domain/repository/PenaltyRepository.kt`:
- Around line 10-15: The current JPQL in
PenaltyRepository.findByUserIdAndCardinalIdOrderByIdDesc only JOIN FETCHes
p.cardinal which can cause N+1 if callers later access p.user; update the query
to also FETCH the user (e.g., add "JOIN FETCH p.user") so both associations are
eagerly loaded, or alternatively add a separate repository method (like
findByCardinalIdOrderByIdDesc) that JOIN FETCHes both p.cardinal and p.user;
update any usages such as GetPenaltyQueryService.findByUser() to call the
new/updated method if needed.
In
`@src/main/kotlin/com/weeth/domain/penalty/presentation/PenaltyAdminController.kt`:
- Around line 60-67: Change the DELETE handler in PenaltyAdminController to
follow RESTful conventions by moving penaltyId from a request parameter to a
path variable: update the mapping annotation on the delete method to include the
path segment for the ID (e.g., "/{penaltyId}"), change the method parameter
annotation from `@RequestParam` to `@PathVariable` for penaltyId, and leave the
invocation of deletePenaltyUseCase.delete(penaltyId) and the CommonResponse
return unchanged so behavior is preserved.
| } | ||
| } | ||
|
|
||
| else -> {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find PenaltyType enum definition
fd "PenaltyType" --type f \( -name "*.kt" -o -name "*.java" \)Repository: Team-Weeth/weeth-server
Length of output: 297
🏁 Script executed:
# Find PenaltyType enum definition with correct syntax
fd "PenaltyType"Repository: Team-Weeth/weeth-server
Length of output: 135
🏁 Script executed:
# Read PenaltyType enum
cat -n src/main/kotlin/com/weeth/domain/penalty/domain/enums/PenaltyType.ktRepository: Team-Weeth/weeth-server
Length of output: 233
🏁 Script executed:
# Read SavePenaltyUseCase.kt around line 52
cat -n src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt | head -70 | tail -40Repository: Team-Weeth/weeth-server
Length of output: 1653
AUTO_PENALTY 타입이 처리되지 않는 문제 해결 필요
PenaltyType 열거형에 PENALTY, WARNING 외에 AUTO_PENALTY가 정의되어 있으나, 현재 when 표현식에서 AUTO_PENALTY는 빈 else 분기로 조용히 무시됩니다. AUTO_PENALTY가 내부적으로 생성되지만 직접 처리되지 않는 이상, 예외를 던지거나 명시적으로 처리해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/kotlin/com/weeth/domain/penalty/application/usecase/command/SavePenaltyUseCase.kt`
at line 52, The when expression in SavePenaltyUseCase silently ignores
PenaltyType.AUTO_PENALTY via an empty else branch; update the when over
PenaltyType (in SavePenaltyUseCase) to explicitly handle AUTO_PENALTY (e.g.,
implement the intended logic or, if unsupported, throw a clear exception like
IllegalArgumentException/UnsupportedOperationException mentioning AUTO_PENALTY)
instead of leaving else -> {} so the case is not silently skipped.
hyxklee
left a comment
There was a problem hiding this comment.
고생하셨습니다!
혹시 테스트는 추가로 필요하지 않을까용??
| private val cardinalGetService: CardinalGetService, | ||
| private val mapper: PenaltyMapper, | ||
| ) { | ||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
해당 어노테이션은 클래스 단에 달아줘도 될 것 같아요! 어차피 해당 클래스의 책임 자체가 조회만 수행하는 거니까
There was a problem hiding this comment.
넵! 클래스단에 달아두겟습니당
공통 컨벤션에 따라 이번 마이그레이션은 별도 테스트 코드가 불필요하다고 판단하여 추가하지 않았습니다!
📌 Summary
Penalty 도메인 Java → Kotlin 마이그레이션 및 아키텍처 구조 변경
📝 Changes
What
Why
How
📸 Screenshots / Logs
💡 Reviewer 참고사항
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores