[ refactor ] : DTO 파일 분리 및 Usecase 레이어 도입으로 도메인 간 의존 제거#30
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughShared DTOs are split from monolithic files into per-domain request/response packages across curriculum, session, user, gamification, and notification modules. New usecase classes (SessionLifeCycleUsecase, SessionLessonUsecase, UserHomeUsecase) and a SessionQueryService coordinate session and user-home flows, replacing prior direct service calls in controllers, with corresponding import and test updates. ChangesCurriculum, session, and user flow split
Notification DTO package split
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SessionController
participant SessionLessonUsecase
participant SessionQueryService
participant CurriculumService
SessionController->>SessionLessonUsecase: getToday/getLesson/getReaction/getStatus
SessionLessonUsecase->>SessionQueryService: getStartedSessionWithSnapshot / getStatus / findActiveSession
SessionQueryService-->>SessionLessonUsecase: session state and snapshot
SessionLessonUsecase->>CurriculumService: getTodayTopics(...) or getLessonContent(...)
CurriculumService-->>SessionLessonUsecase: TodayTopicResponse list or LessonContent
SessionLessonUsecase-->>SessionController: SessionTodayResponse / SessionLessonResponse / SessionStatusResponse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SessionService가 curriculum 도메인의 repository 3개(LessonTopic, LessonQuestion, HintNote)를 직접 주입하던 구조를 CurriculumService 위임으로 교체. # Conflicts: # src/main/kotlin/org/prography/samsung/backend/session/service/SessionService.kt
2975aa9 to
18748c1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/kotlin/org/prography/samsung/backend/session/service/SessionService.kt (2)
125-134: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
advancePhasereturn type mismatch feeding intoSessionLessonUsecase.Pipeline logs show
advancePhase(...)'s actual return type resolves to the oldorg.prography.samsung.backend.session.dto.SessionPhaseResponse, whileSessionLessonUsecase.advancePhaseexpectsdto.response.SessionPhaseResponse. Update the import/type used here to the newdto.response.SessionPhaseResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/session/service/SessionService.kt` around lines 125 - 134, The advancePhase flow in SessionService is returning the old SessionPhaseResponse type, which does not match what SessionLessonUsecase expects. Update SessionService.advancePhase to use the dto.response.SessionPhaseResponse import and ensure the returned object resolves to that new response type, keeping the method signature and return construction aligned with the updated DTO package.Source: Pipeline failures
32-39: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftRoot cause of build failures:
start()was never migrated to the new usecase contract.
start()still declaresrequest: org.prography.samsung.backend.session.dto.SessionStartRequest?(old package, fully-qualified) and only accepts 2 params, whileSessionStartUsecase.start()callssessionService.start(userId, request, topics)(3 args) with the newdto.request.SessionStartRequest/dto.response.SessionStartResponsetypes. This is exactly what the pipeline reports as compile errors atSessionStartUsecase.kt:27("Too many arguments", type mismatches on both the parameter and return type).Additionally, this method still owns
lessonTopicRepositoryandresolveSelectedLessonTopic(curriculum-domain concerns) in the constructor/body — contradicting this PR's stated goal of removing cross-domain repository access fromSessionService(now thatSessionStartUsecaseresolvestopicsviaCurriculumServiceupstream).Needs: update
start()to accept the pre-resolvedtopicsfrom the usecase, use the new DTO packages, and drop the now-redundantlessonTopicRepository/resolveSelectedLessonTopicif no longer needed elsewhere.SessionServiceTest.kt's constructor wiring andsut.start(...)calls will also need updating once this signature changes.🛠️ Illustrative direction (adapt to actual `CurriculumService.getLessonTopics` return type)
- fun start( - userId: Long, - request: org.prography.samsung.backend.session.dto.SessionStartRequest?, - ): SessionStartResponse { + fun start( + userId: Long, + request: SessionStartRequest?, + topics: List<LessonTopic>, + ): SessionStartResponse { val existing = tutoringSessionRepository.findByUserIdAndStatus(userId, SessionStatus.STARTED) if (existing != null) { return SessionStartResponse( sessionId = existing.id, startedAt = KstDateTimeUtils.toOffsetDateTimeString(existing.startedAt), resumed = true, ) } val user = userRepository.findById(userId).orElseThrow { CustomException(DomainErrorCode.USER_NOT_FOUND) } val userCurriculum = userCurriculumRepository.findById(userId).orElseThrow { CustomException(DomainErrorCode.CURRICULUM_NOT_SELECTED) } - val selectedTopic = resolveSelectedLessonTopic(request?.lessonTopicId, userCurriculum.curriculum.id) - val targetCurriculumId = selectedTopic?.curriculum?.id ?: request?.curriculumId ?: userCurriculum.curriculum.id - if (userCurriculum.curriculum.id != targetCurriculumId) { - throw CustomException(DomainErrorCode.CURRICULUM_MISMATCH) - } - - val topic = - selectedTopic - ?: lessonTopicRepository.findAllByCurriculumIdOrderBySequenceAsc(targetCurriculumId).firstOrNull() - ?: throw CustomException(DomainErrorCode.LESSON_TOPIC_NOT_FOUND) + val topic = topics.firstOrNull() ?: throw CustomException(DomainErrorCode.LESSON_TOPIC_NOT_FOUND)Also applies to: 60-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/session/service/SessionService.kt` around lines 32 - 39, Update SessionService.start to match the new usecase contract: it should accept the pre-resolved topics argument from SessionStartUsecase and use the new dto.request.SessionStartRequest and dto.response.SessionStartResponse types instead of the old fully qualified request type. Also remove the now-redundant curriculum-domain dependency by dropping lessonTopicRepository and resolveSelectedLessonTopic from SessionService if they are no longer used. After changing the start signature, adjust any callers and tests such as SessionServiceTest wiring and sut.start invocations to pass the new arguments and expect the new response type.Source: Pipeline failures
src/main/kotlin/org/prography/samsung/backend/user/dto/request/UserScheduleRequest.kt (1)
7-29: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFormat/range constraints are documentation-only, not enforced.
frequency(allowed 2/3),days(allowed weekday codes), andtime(HH:00pattern) only carry@Schema/@ArraySchemametadata for OpenAPI docs. None of these are backed by actual Bean Validation constraints (e.g.@Pattern,@Min/@Max, or a custom validator), so invalid values liketime = "25:30"or an arbitraryfrequencywill pass@Validand reach the service layer.🛡️ Proposed fix to add runtime validation
package org.prography.samsung.backend.user.dto.request import io.swagger.v3.oas.annotations.media.ArraySchema import io.swagger.v3.oas.annotations.media.Schema +import jakarta.validation.constraints.Max +import jakarta.validation.constraints.Min +import jakarta.validation.constraints.NotEmpty +import jakarta.validation.constraints.NotNull +import jakarta.validation.constraints.Pattern data class UserScheduleRequest( `@field`:NotNull + `@field`:Min(2) + `@field`:Max(3) `@field`:Schema(description = "주당 수업 횟수", allowableValues = ["2", "3"], example = "3") val frequency: Int, `@field`:NotNull + `@field`:NotEmpty `@field`:ArraySchema( schema = Schema( description = "수업 요일", allowableValues = ["MON", "TUE", "WED", "THU", "FRI", "SAT", "SUN"], example = "MON", ), ) val days: List<String>, `@field`:NotNull + `@field`:Pattern(regexp = "^([01]\\d|2[0-3]):00$") `@field`:Schema( description = "수업 시간 (KST, HH:mm 형식)", pattern = "^([01]\\d|2[0-3]):00$", example = "18:00", ) val time: String, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/user/dto/request/UserScheduleRequest.kt` around lines 7 - 29, Add real Bean Validation to UserScheduleRequest so the constraints are enforced at runtime, not just documented in OpenAPI. Keep the existing `@Schema/`@ArraySchema metadata, but back `frequency`, `days`, and `time` with validation on the request DTO (for example via range/allowed-value validation and a pattern check, or a custom validator for the weekday list). Make sure the fix is applied in UserScheduleRequest so `@Valid` rejects invalid values before they reach the service layer.
🧹 Nitpick comments (5)
src/main/kotlin/org/prography/samsung/backend/user/service/UserProfileService.kt (1)
20-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared curriculum lookup to avoid duplication.
The
userCurriculumlookup ingetProfile(Lines 25-28) duplicates the exact samefindById/orElseThrow(CustomException(DomainErrorCode.CURRICULUM_NOT_SELECTED))logic in the newgetUserCurriculum(Lines 45-47). HavegetProfilecallgetUserCurriculum(userId)to keep the lookup in one place.♻️ Proposed refactor
- val userCurriculum = - userCurriculumRepository.findById(userId).orElseThrow { - CustomException(DomainErrorCode.CURRICULUM_NOT_SELECTED) - } + val userCurriculum = getUserCurriculum(userId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/user/service/UserProfileService.kt` around lines 20 - 47, `UserProfileService.getProfile` duplicates the curriculum fetch and `CURRICULUM_NOT_SELECTED` exception handling already implemented in `getUserCurriculum`; update `getProfile` to reuse `getUserCurriculum(userId)` instead of calling `userCurriculumRepository.findById(...).orElseThrow(...)` directly, so the curriculum lookup logic stays centralized in `UserProfileService`.src/main/kotlin/org/prography/samsung/backend/user/usecase/UserHomeUsecase.kt (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstructor formatting inconsistent with codebase convention.
Other multi-dependency constructors in this PR (e.g.,
UserController) are formatted across multiple lines. This single-line constructor is a minor style deviation; consider matching the existing convention for consistency.As per coding guidelines, "Follow existing ktlint style conventions in Kotlin code."💅 Suggested formatting
-class UserHomeUsecase(private val userProfileService: UserProfileService, private val sessionService: SessionService) { +class UserHomeUsecase( + private val userProfileService: UserProfileService, + private val sessionService: SessionService, +) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/user/usecase/UserHomeUsecase.kt` at line 9, The primary issue is that UserHomeUsecase uses a single-line multi-dependency constructor, which is inconsistent with the Kotlin formatting convention used elsewhere in this PR. Update the UserHomeUsecase class declaration so the constructor parameters are split across multiple lines, matching the style already used by UserController and other multi-argument constructors, and keep the formatting ktlint-compliant.Source: Coding guidelines
src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserScheduleResponse.kt (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
DayOfWeek-like enum instead ofList<String>.The
allowableValuesconstraint documents a fixed set of weekday codes (MON..SUN), but the field type isList<String>, so invalid values are only caught at doc/validation time, not compile time.♻️ Optional refactor
- val days: List<String>, + val days: List<WeekDay>,Where
WeekDayis an enum with valuesMON, TUE, WED, THU, FRI, SAT, SUN.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserScheduleResponse.kt` around lines 10 - 17, The `UserScheduleResponse` DTO currently models weekday codes as `List<String>` even though the allowed values are fixed to MON..SUN. Update the `days` field to use the existing `WeekDay` enum (or a DayOfWeek-like enum) instead of raw strings, and adjust the OpenAPI schema annotations in `UserScheduleResponse` so the type and documented values stay aligned and enforce valid weekday values at compile time.src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserSettingsResponse.kt (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant same-package import.
UserScheduleResponseis imported fromorg.prography.samsung.backend.user.dto.response, the same package this file belongs to — unnecessary.🧹 Remove redundant import
import org.prography.samsung.backend.curriculum.dto.response.CurriculumChipResponse -import org.prography.samsung.backend.user.dto.response.UserScheduleResponse🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserSettingsResponse.kt` around lines 1 - 6, Remove the redundant same-package import in UserSettingsResponse; since UserSettingsResponse and UserScheduleResponse are in the same package, the explicit import is unnecessary. Keep the data class definition intact and only adjust the imports at the top of UserSettingsResponse.src/main/kotlin/org/prography/samsung/backend/session/dto/response/RewardAckResponse.kt (1)
1-3: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the duplicate
RewardAckResponseDTO
This file duplicatesorg.prography.samsung.backend.session.dto.RewardAckResponsefromSessionDtos.kt, and nothing points to the newsession.dto.responsecopy yet. Keep the session response DTOs in one package or migrate the callers together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/session/dto/response/RewardAckResponse.kt` around lines 1 - 3, The new RewardAckResponse DTO duplicates the existing RewardAckResponse in SessionDtos.kt, so keep only one source of truth for this response model. Remove the redundant session.dto.response copy or migrate all callers to it together, and update any references so the session response types remain centralized under the existing DTO package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/LessonContent.kt`:
- Around line 1-3: Rename the `LessonContent` DTO to use the service-layer
`*Command` suffix, since it is an internal transfer object rather than an
outbound response. Update the `LessonContent` data class name and adjust any
references to it across the curriculum flow, including any constructors,
property types, and call sites that currently use `LessonContent`, while keeping
related response DTOs like `LessonQuestionResponse` and `HintNoteResponse`
unchanged.
In
`@src/main/kotlin/org/prography/samsung/backend/session/service/SessionCompletionService.kt`:
- Around line 36-38: The idempotent-return path in SessionCompletionService’s
session completion flow is using a raw user lookup fallback via
userProfileRepository.findById(userId).orElseThrow(), which can raise
NoSuchElementException instead of the project’s standard CustomException. Update
that lookup to follow the same error-handling pattern already used later in the
method by supplying a CustomException with the appropriate domain error code
(for missing users), so this branch stays consistent with the rest of the
service and the Kotlin error-handling guideline.
In
`@src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLessonUsecase.kt`:
- Around line 37-42: `SessionLessonUsecase.getLesson` and `getReaction`
reference missing `SessionConstants.INTRO_TOPIC_SEQUENCE` and
`REACTION_TOPIC_SEQUENCE`, so add those members to `SessionConstants` or update
these calls to use the existing sequence constant used elsewhere such as
`SNAPSHOT_SEQUENCE`. Make sure the chosen identifiers are defined on
`SessionConstants` and that `buildPhaseResponse` still receives the correct
topic sequence for each phase.
- Around line 44-46: `SessionLessonUsecase.advancePhase` is still expecting the
new `SessionPhaseResponse`, but `SessionService.advancePhase` is returning the
old-package DTO, causing the type mismatch. Update `SessionService.advancePhase`
to return the same `SessionPhaseResponse` type used by `SessionLessonUsecase`,
and make sure any mapping/imports inside `SessionService` use the new DTO
consistently so the wrapper method compiles cleanly.
In
`@src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionStartUsecase.kt`:
- Around line 20-27: The compile failure comes from SessionStartUsecase.start
calling sessionService.start with an extra topics argument and mismatched
request/response types. Update the call site to match the actual
SessionService.start signature, and align the imports/types in
SessionStartUsecase with the session service’s current DTOs; if topics are
needed, pass them only after updating SessionService.start to accept them
consistently across the interface and implementation.
In
`@src/main/kotlin/org/prography/samsung/backend/user/dto/request/UserSettingsRequest.kt`:
- Around line 22-27: The `UserSettingsRequest.time` field in
`UserSettingsRequest` has mismatched docs and validation: the `@Schema`
description says only 15:00–20:00 should be allowed, but the current pattern
implies any full hour, and `@Schema` alone does not enforce anything at runtime.
Update the `time` property annotations so the documented range matches the
actual accepted values, and add a real validation constraint on `time` (not just
`@Schema`) so `UserSettingsRequest` rejects invalid times consistently.
In
`@src/test/kotlin/org/prography/samsung/backend/session/service/SessionServiceTest.kt`:
- Around line 62-65: The multi-line `sut.start(...)` call in
`SessionServiceTest` is missing the trailing comma style used elsewhere, which
can break ktlint. Update the `start` invocation so the final argument in the
multi-line call follows the project’s Kotlin formatting convention, matching
other call sites like `TutoringSession` construction and `RewardResponse` for
consistency.
---
Outside diff comments:
In
`@src/main/kotlin/org/prography/samsung/backend/session/service/SessionService.kt`:
- Around line 125-134: The advancePhase flow in SessionService is returning the
old SessionPhaseResponse type, which does not match what SessionLessonUsecase
expects. Update SessionService.advancePhase to use the
dto.response.SessionPhaseResponse import and ensure the returned object resolves
to that new response type, keeping the method signature and return construction
aligned with the updated DTO package.
- Around line 32-39: Update SessionService.start to match the new usecase
contract: it should accept the pre-resolved topics argument from
SessionStartUsecase and use the new dto.request.SessionStartRequest and
dto.response.SessionStartResponse types instead of the old fully qualified
request type. Also remove the now-redundant curriculum-domain dependency by
dropping lessonTopicRepository and resolveSelectedLessonTopic from
SessionService if they are no longer used. After changing the start signature,
adjust any callers and tests such as SessionServiceTest wiring and sut.start
invocations to pass the new arguments and expect the new response type.
In
`@src/main/kotlin/org/prography/samsung/backend/user/dto/request/UserScheduleRequest.kt`:
- Around line 7-29: Add real Bean Validation to UserScheduleRequest so the
constraints are enforced at runtime, not just documented in OpenAPI. Keep the
existing `@Schema/`@ArraySchema metadata, but back `frequency`, `days`, and `time`
with validation on the request DTO (for example via range/allowed-value
validation and a pattern check, or a custom validator for the weekday list).
Make sure the fix is applied in UserScheduleRequest so `@Valid` rejects invalid
values before they reach the service layer.
---
Nitpick comments:
In
`@src/main/kotlin/org/prography/samsung/backend/session/dto/response/RewardAckResponse.kt`:
- Around line 1-3: The new RewardAckResponse DTO duplicates the existing
RewardAckResponse in SessionDtos.kt, so keep only one source of truth for this
response model. Remove the redundant session.dto.response copy or migrate all
callers to it together, and update any references so the session response types
remain centralized under the existing DTO package.
In
`@src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserScheduleResponse.kt`:
- Around line 10-17: The `UserScheduleResponse` DTO currently models weekday
codes as `List<String>` even though the allowed values are fixed to MON..SUN.
Update the `days` field to use the existing `WeekDay` enum (or a DayOfWeek-like
enum) instead of raw strings, and adjust the OpenAPI schema annotations in
`UserScheduleResponse` so the type and documented values stay aligned and
enforce valid weekday values at compile time.
In
`@src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserSettingsResponse.kt`:
- Around line 1-6: Remove the redundant same-package import in
UserSettingsResponse; since UserSettingsResponse and UserScheduleResponse are in
the same package, the explicit import is unnecessary. Keep the data class
definition intact and only adjust the imports at the top of
UserSettingsResponse.
In
`@src/main/kotlin/org/prography/samsung/backend/user/service/UserProfileService.kt`:
- Around line 20-47: `UserProfileService.getProfile` duplicates the curriculum
fetch and `CURRICULUM_NOT_SELECTED` exception handling already implemented in
`getUserCurriculum`; update `getProfile` to reuse `getUserCurriculum(userId)`
instead of calling `userCurriculumRepository.findById(...).orElseThrow(...)`
directly, so the curriculum lookup logic stays centralized in
`UserProfileService`.
In
`@src/main/kotlin/org/prography/samsung/backend/user/usecase/UserHomeUsecase.kt`:
- Line 9: The primary issue is that UserHomeUsecase uses a single-line
multi-dependency constructor, which is inconsistent with the Kotlin formatting
convention used elsewhere in this PR. Update the UserHomeUsecase class
declaration so the constructor parameters are split across multiple lines,
matching the style already used by UserController and other multi-argument
constructors, and keep the formatting ktlint-compliant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff60a22d-875b-4043-8cbb-cb75fdf45108
📒 Files selected for processing (56)
src/main/kotlin/org/prography/samsung/backend/common/dto/ActiveSessionResponse.ktsrc/main/kotlin/org/prography/samsung/backend/common/dto/CurriculumSummaryResponse.ktsrc/main/kotlin/org/prography/samsung/backend/common/dto/LevelResponse.ktsrc/main/kotlin/org/prography/samsung/backend/common/dto/SharedDtos.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/CurriculumChipResponse.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/HintNoteResponse.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/LessonContent.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/LessonQuestionResponse.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/TodayTopicResponse.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/service/CurriculumService.ktsrc/main/kotlin/org/prography/samsung/backend/curriculum/service/HintNoteMapper.ktsrc/main/kotlin/org/prography/samsung/backend/gamification/dto/response/RewardResponse.ktsrc/main/kotlin/org/prography/samsung/backend/notification/controller/NotificationController.ktsrc/main/kotlin/org/prography/samsung/backend/notification/dto/request/NotificationRegisterRequest.ktsrc/main/kotlin/org/prography/samsung/backend/notification/dto/response/NotificationRegisterResponse.ktsrc/main/kotlin/org/prography/samsung/backend/notification/dto/response/NotificationRescheduleResponse.ktsrc/main/kotlin/org/prography/samsung/backend/notification/service/NotificationService.ktsrc/main/kotlin/org/prography/samsung/backend/session/controller/SessionController.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/request/SessionCompleteRequest.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/request/SessionStartRequest.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/RewardAckResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionAbortResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionLessonResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionPhaseResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionStartResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionStatusResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionTodayResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/service/SessionCompletionService.ktsrc/main/kotlin/org/prography/samsung/backend/session/service/SessionService.ktsrc/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLessonUsecase.ktsrc/main/kotlin/org/prography/samsung/backend/session/usecase/SessionStartUsecase.ktsrc/main/kotlin/org/prography/samsung/backend/user/controller/OnboardingController.ktsrc/main/kotlin/org/prography/samsung/backend/user/controller/UserController.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/UserDtos.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/request/OnboardingRequest.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/request/UserScheduleRequest.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/request/UserSettingsRequest.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/OnboardingCompleteResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/OnboardingResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/OnboardingStatusResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserHomeResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserProfileResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserScheduleResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserSettingsResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/service/OnboardingService.ktsrc/main/kotlin/org/prography/samsung/backend/user/service/UserProfileService.ktsrc/main/kotlin/org/prography/samsung/backend/user/service/UserSettingsService.ktsrc/main/kotlin/org/prography/samsung/backend/user/usecase/UserHomeUsecase.ktsrc/test/kotlin/org/prography/samsung/backend/curriculum/service/CurriculumServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/notification/service/NotificationServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/session/service/SessionServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/user/UserApiIntegrationTest.ktsrc/test/kotlin/org/prography/samsung/backend/user/service/OnboardingServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/user/service/UserProfileServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/user/service/UserSettingsServiceTest.ktsrc/test/kotlin/org/prography/samsung/backend/user/usecase/UserHomeUsecaseTest.kt
💤 Files with no reviewable changes (2)
- src/main/kotlin/org/prography/samsung/backend/user/dto/UserDtos.kt
- src/main/kotlin/org/prography/samsung/backend/common/dto/SharedDtos.kt
- SessionService를 SessionQueryService, SessionLifecycleService, SessionRewardService로 분리해 단일 책임 원칙 적용 - SessionCompletionUsecase, SessionLifeCycleUsecase, SessionHistoryUsecase 추가로 Controller가 모든 엔드포인트에서 Usecase를 경유하도록 통일 - HistoryDtos를 dto/response 하위로 분리 - CurriculumService에 getLessonTopicById, getFirstLessonTopic 추가로 session 도메인의 LessonTopicRepository 직접 의존 제거
tlarbals824
left a comment
tlarbals824
left a comment
There was a problem hiding this comment.
적절한 usecase 정의, god service 정리 좋네요! 다만 Usecase를 serivce의 인터페이스로 정의할 것인지, 아니면 facade로서 오케스트레이터의 역할로 정의할지 생각하시면 좋을 것 같아요!
| @Component | ||
| class SessionCompletionUsecase( | ||
| private val sessionCompletionService: SessionCompletionService, | ||
| private val sessionRewardService: SessionRewardService, | ||
| ) { | ||
| @Transactional | ||
| fun complete(userId: Long, sessionId: String): RewardResponse = sessionCompletionService.complete(userId, sessionId) | ||
|
|
||
| @Transactional(readOnly = true) | ||
| fun getReward(userId: Long, sessionId: String): RewardResponse = sessionRewardService.getReward(userId, sessionId) | ||
|
|
||
| @Transactional | ||
| fun acknowledgeReward(userId: Long, sessionId: String): RewardAckResponse = | ||
| sessionRewardService.acknowledgeReward(userId, sessionId) | ||
| } |
There was a problem hiding this comment.
Usecase의 역할을 어떻게 정의해야 할까요? 현재 구조처럼 Service를 그대로 호출하기만 한다면, Usecase ~= Service로 보여져 레이어 분리의 의미가 희미해집니다.
오히려 Usecase를 인터페이스(포트)로 두고 Service가 구현(어댑터)하는 구조는 어떨까요? 핵사고날 아키텍처의 "외부 인터페이스로만 소통" 원칙과 같은 맥락으로, 의존 방향을 Controller → Usecase(추상)로 고정하고 Service가 그 계약을 채우도록 하면 Usecase가 단순 패스스루가 아니라 진짜 추상화 경계로 기능할 수 있을 것 같습니다.
There was a problem hiding this comment.
저도 해당 메서드들을 usecase로 옮기는 부분에서 usecase가 단순 service를 호출하는 문제에서 고민했던 포인트가
Session도메인에 오케스트레이션을 위해usecase를 도입하였는데, 모든 엔드포인트들에 대해 일관성을 유지하기 위해 단순 호출도usecase를 거치게 한다.- 단순 호출의 경우는
Session도메인에서 아키텍처 일관성이 깨지더라도 바로Controller에서 바로Service를 호출하는 방향으로 하여usecase는 오케스트레이션의 역할만 담당하도록 한다.
이 두가지에 대해 고민하다 일관성을 위해 현재 방식을 적용했습니다!
리뷰해주신 내용에 대해 찾아보고 적용시켜 보겠습니다!
There was a problem hiding this comment.
아래 트랜잭션 관련 리뷰 방향대로라면 usecase가 트랜잭션을 관리하고 오케스트레이션의 역할을 담당하게 되어 추상화의 역할을 담당하기엔 어려움이 있을 것 같습니다.
이에 대해 usecase가 @Transactional을 담당하도록 하게 될 경우, 패스스루에 해당하는 Service 메서드들에 대해 usecase를 거치지 않고, Service를 직접 호출하도록 수정하면 Service 메서드에 Transactional을 적용해야하는 불일치가 발생할 수 있을 것 같습니다.
(규칙은 usecase가 transaction을 담당하는 것인데, 패스스루 메서드에 대해선 service가 transaction을 담당하는 불일치 발생)
규칙을 준수하기 위해선 패스스루 방식에도 usecase 계층을 중간에 넣어 해당 계층은 트랜잭션을 담당하는 역할로만 사용해야 하는 트레이드오프가 있을 것 같습니다.
저는 usecase를 오케스트레이션의 역할로 사용하면서 해당 패스스루에 대해서도 usecase 계층을 넣어 트랜잭션을 담당하도록 하는 방향이 지금으로썬 좋은 절충안으로 생각합니다!
There was a problem hiding this comment.
다시 보니 이 부분은 정답이 있다기보다 팀 컨벤션 선택에 가까운 것 같아요.
제용님 방향(일관성을 위해 Controller 진입점은 Usecase 경유, Usecase가 오케스트레이션 + 트랜잭션 경계)도 충분히 좋다고 생각합니다.
경로가 하나로 고정되면 “이 엔드포인트는 Usecase / Service 중 어디?”를 매번 고민하지 않아도 되어서, 팀 단위 인지 부하는 오히려 줄 수 있을 것 같아요.
제가 처음에 말한 포트/어댑터 쪽은 지금 단계에선 과한 것 같아 철회할게요.
패스스루 Usecase가 조금 어색해 보일 수는 있지만, “Controller 진입점은 항상 Usecase”라는 규칙의 트레이드오프로 이해하고 가겠습니다.
그 규칙을 skill/문서에 한 줄로만 남겨두면 이후 리뷰도 편할 것 같아요.
(예: Controller → Usecase → Service, Controller는 Usecase만 호출, Usecase가 @transactional 소유)
| startedAt = KstDateTimeUtils.toOffsetDateTimeString(existing.startedAt), | ||
| resumed = true, | ||
| ) | ||
| } |
There was a problem hiding this comment.
해당 메서드는 tutoringSessionRepository.save() + sessionTopicSnapshotRepository.save() 두 쓰기를 원자적으로 묶어야 하니 트랜잭션이 필요한 건 맞아요. 다만 SessionLifeCycleUsecase.start()에도 @transactional이 있어서 REQUIRED 전파로 참여만 하고 있는 상태예요.
제가 고민인 건, 쓰기 조율은 Usecase(Facade)가, Service는 트랜잭션 없이 Usecase 경계에 참여만 하는 구조가 좀 더 깔끔해보여요. 지금 구조면 Service 단독 호출 시 트랜잭션이 없는 것처럼 보일 수 있거든요.
왜 고민하냐면, 트랜잭션이 중첩된 상태에서 하위(Service)에서 예외가 발생하면 상위(Usecase)가 rollback-only 마킹되면서 "왜 커밋이 안 되지?"를 디버깅해야 하는 상황이 생기더라고요. REQUIRED 기본 전파라면 결국 한 트랜잭션이긴 한데, 양쪽에 경계가 선언되어 있으면 어디서 마킹했는지 추적하면서 읽어야 해서 인지 부하가 커지는 것 같아요. 어떻게 생각하시나요?
There was a problem hiding this comment.
하나의 트랜잭션으로 묶는 부분에 대해서만 고려하고, 예외 발생 부분에 대해서 인지하는 것에는 미처 생각해보지 못했습니다!
음 우선 제가 usecase와 service 모두 @Transactional을 적용한 이유는 우선 해당 메서드와 오케스트레이션에 트랜잭션이 적용되었다는 가독성 측면과 혹시 모를 Service 메서드 단독 호출시 트랜잭션이 적용되도록 하기 위한 부분이었습니다!
저도 디버깅적인 측면에서 Usecase가 단독으로 트랜잭션을 관리하며 Service가 해당 Usecase에 참여하도록 하는 부분에는 찬성합니다. 대신, 팀 단위에서 항상 Service 메서드 호출은 Usecase를 통해 호출되도록 하여 트랜잭션을 적용해야 한다는 규칙을 인지하고 리뷰에서도 이 부분에 대해 서로 검증하면 좋을 것 같습니다! 이를 이 다음 작업인 skill로 만들 때 명시하도록 하겠습니다!
| data class ActiveSessionResponse( | ||
| val sessionId: String, | ||
| @field:Schema(description = "세션 상태", allowableValues = ["STARTED", "COMPLETED", "ABORTED"], example = "STARTED") | ||
| val status: SessionStatus, | ||
| @field:Schema(description = "현재 수업 페이즈", allowableValues = ["INTRO", "REACTION"], example = "INTRO") | ||
| val currentPhase: SessionPhase, | ||
| val startedAt: String, | ||
| ) |
There was a problem hiding this comment.
ActiveSessionResponse 필드를 보면 sessionId, status, currentPhase, startedAt 전부 session 도메인 개념인데 common/dto에 위치하고 있어요. 공유 응답이라는 의도는 이해하긴 하는데, 이러면 user와 session이 모두 common을 거쳐서 ActiveSessionResponse를 참조하게 되니까 의존 방향이 common으로 수렴하더라고요.
session 소유 DTO(session/dto/response)로 옮기고, user 쪽에서 필요하면 그걸 직접 참조하거나 자체 매핑을 두는 쪽이 도메인 경계가 좀 더 명확해보여요.
그리고 common에 두는 기준을 한번 정하면 좋을 것 같은데, 저는 common에는 도메인 맥락이 아예 없는 포맷 유틸(예: Cursor, PagedResponse 같은)만 넣고, LevelResponse나 CurriculumSummaryResponse처럼 도메인 개념이 담긴 DTO는 각 도메인으로 내리는 게 일관성 있어 보이거든요. 어떨까요?
There was a problem hiding this comment.
해당 방향성이 더 일관성있고 좋은 것 같습니다!
여러 도메인에서 호출되는 DTO라고 해서 common에 두지않고, 주체가 되는 도메인에 DTO에 두고, 다른 도메인에서 참조하는 방향으로 가고, 실제 common에는 전역적으로 사용할 수 있는 도메인과 분리되거나 말씀하신 포맷 유틸들에 대한 부분만 적용하도록 하는 방향성으로 가는게 맞다고 생각합니다!
…sson -> SessionLifeCycle)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLifeCycleUsecase.kt (1)
20-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the redundant curriculum pre-check
SessionLifecycleService.startalready validates curriculum mismatch withlessonTopicIdprecedence. This check only looks atrequest.curriculumId, so it can reject a valid request whenlessonTopicIdmatches the current curriculum butcurriculumIdis stale. Let the service handle this as the single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLifeCycleUsecase.kt` around lines 20 - 28, Remove the redundant curriculum validation from SessionLifeCycleUsecase.start and let SessionLifecycleService.start remain the single source of truth. The pre-check using request?.curriculumId and userCurriculum.curriculum.id can incorrectly reject valid requests when lessonTopicId drives curriculum resolution, so just fetch the user/userCurriculum and delegate directly to sessionLifecycleService.start without comparing curriculum IDs here.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLifeCycleUsecase.kt`:
- Around line 20-28: Remove the redundant curriculum validation from
SessionLifeCycleUsecase.start and let SessionLifecycleService.start remain the
single source of truth. The pre-check using request?.curriculumId and
userCurriculum.curriculum.id can incorrectly reject valid requests when
lessonTopicId drives curriculum resolution, so just fetch the
user/userCurriculum and delegate directly to sessionLifecycleService.start
without comparing curriculum IDs here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf315987-2060-45df-b423-63f393599a9f
📒 Files selected for processing (13)
src/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/CurriculumSummaryResponse.ktsrc/main/kotlin/org/prography/samsung/backend/gamification/dto/response/LevelResponse.ktsrc/main/kotlin/org/prography/samsung/backend/gamification/dto/response/RewardResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/controller/SessionController.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/ActiveSessionResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionStatusResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionTodayResponse.ktsrc/main/kotlin/org/prography/samsung/backend/session/service/SessionQueryService.ktsrc/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLessonUsecase.ktsrc/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLifeCycleUsecase.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserHomeResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/dto/response/UserProfileResponse.ktsrc/main/kotlin/org/prography/samsung/backend/user/service/UserProfileService.kt
💤 Files with no reviewable changes (4)
- src/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionTodayResponse.kt
- src/main/kotlin/org/prography/samsung/backend/session/dto/response/SessionStatusResponse.kt
- src/main/kotlin/org/prography/samsung/backend/session/usecase/SessionLessonUsecase.kt
- src/main/kotlin/org/prography/samsung/backend/gamification/dto/response/RewardResponse.kt
✅ Files skipped from review due to trivial changes (2)
- src/main/kotlin/org/prography/samsung/backend/gamification/dto/response/LevelResponse.kt
- src/main/kotlin/org/prography/samsung/backend/curriculum/dto/response/CurriculumSummaryResponse.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserProfileResponse.kt
- src/main/kotlin/org/prography/samsung/backend/user/dto/response/UserHomeResponse.kt
2 participants
PR
PR 요약
아키텍처 구조를 수정하였습니다. 계층적 구조 (Controller -> Serivce -> Repository, Entity) 를 사용하였으며, 도메인 간의 참조가 필요한 경우에는 Serivce Layer에서 다른 Service를 의존하는 방식이 아닌 Usecase 계층을 중간에 두어 오케스트레이션 역할을 담당하도록 하였습니다.
우선은 제가 이전에 사용했던 아키텍처 구조를 가져와 사용하였는데, 많은 피드백 주시면 감사하겠습니다!
**
conversation도메인의 경우 앞서 작업 중이신 도메인이라 완료되면 따로 수정 후 PR 올리겠습니다!단일 파일에 몰려 있던 DTO를 도메인별
dto/request,dto/response로 분리하고,서비스 간 크로스 도메인 의존을 끊기 위해 Usecase 레이어를 도입했습니다.
이로써 각 Service는 자신의 도메인 Repository만 의존하며, 도메인 간 조합은 Usecase가 전담합니다.
변경된 점
1. DTO 파일 분리 (
SharedDtos.kt,SessionDtos.kt,UserDtos.kt)common/dto/ActiveSessionResponse,LevelResponse,CurriculumSummaryResponse{domain}/dto/request/SessionStartRequest,UserSettingsRequest등{domain}/dto/response/SessionLessonResponse,UserHomeResponse등2. Usecase 레이어 도입 (
SessionStartUsecase,SessionLessonUsecase,UserHomeUsecase)SessionService가CurriculumService와UserCurriculumRepository를 직접 의존하고,UserProfileService가SessionService를 의존하는 등 도메인 간 결합이 존재했습니다.Before — 도메인 간 직접 의존
After — Usecase가 조합, Service는 자기 도메인만
Usecase는
@Service대신@Component로 선언하여 비즈니스 서비스 레이어와 의미적으로 구분했습니다.Summary by CodeRabbit