Conversation
…um을 toString으로 직렬화해 status를 name()으로 고정함
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
Walkthrough운세 도메인, 사용자별 운세 갱신 로직, 운세 카드·단어 영속화, 앱 API와 오류 응답 및 테스트가 추가되었습니다. Soptamp 사용자 모델과 포트 계약도 함께 추가되었습니다. Changes운세 기능
Soptamp 포트 이관
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FortuneController
participant FortuneFacade
participant FortuneService
participant FortuneCardRepositoryAdapter
Client->>FortuneController: GET /api/v2/fortune/word or /card/today
FortuneController->>FortuneFacade: getTodayFortuneWord(userId, todayDate)
FortuneFacade->>FortuneService: getTodayFortuneWord(userId, todayDate)
FortuneService->>FortuneCardRepositoryAdapter: getTodayFortuneCard(userId)
FortuneCardRepositoryAdapter-->>FortuneService: FortuneCard
FortuneService-->>FortuneController: fortune result
FortuneController-->>Client: 성공 응답
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the fortune (운세) feature from the legacy app server, introducing new database schemas, domain entities, services, and a dedicated exception handler to preserve the legacy API contract. The review feedback highlights critical issues, including a potential application crash in FortuneWordIdGenerator if the database is unseeded, a logical bug in FortuneService that could return stale fortune cards, missing foreign key constraints in the migration script, and a missing exception handler for missing request parameters that could break the legacy response format.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| public Long generate() { | ||
| List<Long> ids = fortuneWordRepositoryPort.findAllIds(); | ||
| return ids.get(random.nextInt(ids.size())); | ||
| } |
There was a problem hiding this comment.
If the fortune_word table is empty (e.g., during initial setup or if seeding fails), fortuneWordRepositoryPort.findAllIds() will return an empty list. This will cause random.nextInt(0) to throw an IllegalArgumentException and crash the application. Additionally, calling findAllIds() on every generation loads all IDs into memory. Since fortune words are static master data, consider caching these IDs or using a more efficient query (e.g., count + offset) if the dataset grows large. For now, let's add a defensive check to handle the empty list case gracefully.
public Long generate() {
List<Long> ids = fortuneWordRepositoryPort.findAllIds();
if (ids.isEmpty()) {
throw new IllegalStateException("No fortune words found in the database. Please ensure the database is seeded.");
}
return ids.get(random.nextInt(ids.size()));
}| public FortuneCard getTodayFortuneCard(Long userId) { | ||
| return fortuneCardRepositoryPort | ||
| .findTodayCardByUserId(userId) | ||
| .orElseThrow(() -> new FortuneException(FortuneFailure.NOT_FOUND_FORTUNE_FROM_USER)); | ||
| } |
There was a problem hiding this comment.
In getTodayFortuneCard, the repository query findTodayCardByUserId fetches the card associated with the user's current user_fortune record. However, if the user has not checked their fortune today (i.e., checked_at is in the past), this will return yesterday's (or an older) card as today's card. To enforce the 'today' contract, we should verify that the user has actually checked their fortune today before returning the card.
public FortuneCard getTodayFortuneCard(Long userId) {
if (!isExistTodayFortune(userId)) {
throw new FortuneException(FortuneFailure.NOT_FOUND_FORTUNE_FROM_USER);
}
return fortuneCardRepositoryPort
.findTodayCardByUserId(userId)
.orElseThrow(() -> new FortuneException(FortuneFailure.NOT_FOUND_FORTUNE_FROM_USER));
}| CREATE TABLE user_fortune | ||
| ( | ||
| id BIGINT NOT NULL GENERATED BY DEFAULT AS IDENTITY, | ||
| user_id BIGINT NOT NULL, | ||
| fortune_word_id BIGINT NOT NULL, | ||
| checked_at DATE NOT NULL, | ||
| PRIMARY KEY (id), | ||
| CONSTRAINT uq_user_fortune_user_id UNIQUE (user_id) | ||
| ); |
There was a problem hiding this comment.
The newly added tables (fortune_word and user_fortune) lack foreign key constraints. To ensure referential integrity and prevent orphaned records, consider adding foreign key constraints:
fortune_word.fortune_card_idreferencingfortune_card(id)user_fortune.user_idreferencingusers(id)user_fortune.fortune_word_idreferencingfortune_word(id)
| @ExceptionHandler(MethodArgumentTypeMismatchException.class) | ||
| public ResponseEntity<AppFailureResponse> handleTypeMismatch( | ||
| MethodArgumentTypeMismatchException e) { | ||
| String value = e.getValue() == null ? "" : e.getValue().toString(); | ||
| List<FieldError> errors = List.of(new FieldError(e.getName(), value, e.getErrorCode())); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST) | ||
| .body(AppFailureResponse.of(INVALID_PARAMETER_MESSAGE, HttpStatus.BAD_REQUEST, errors)); | ||
| } |
There was a problem hiding this comment.
Since todayDate in FortuneController.getTodayFortuneWord is a required request parameter, if the client fails to provide it, Spring will throw a MissingServletRequestParameterException. Currently, AppExceptionHandler does not handle this exception, meaning it will fall back to the global exception handler and return the new BaseResponse format instead of the legacy AppFailureResponse format. To preserve the legacy contract and prevent client-side parsing errors, consider adding a handler for MissingServletRequestParameterException.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/src/main/resources/db/migration/V1__create_initial_schema.sql (1)
303-332: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value스키마 제약 조건 및 식별자 생성 전략 강화 (선택 사항)
안정적인 데이터 모델링을 위해 다음 사항들을 선택적으로 고려해 볼 수 있습니다:
- 외래 키(Foreign Key) 추가:
fortune_word.fortune_card_id,user_fortune.user_id,user_fortune.fortune_word_id는 논리적으로 다른 테이블을 참조하고 있습니다. 데이터 정합성을 강제하기 위해FOREIGN KEY제약 조건을 추가하는 것을 권장합니다 (단, 프로젝트의 데이터베이스 설계 컨벤션 상 외래 키를 생략하는 정책이라면 무시하셔도 됩니다).user_fortune식별자 전략: 추가된 DDL 주석에 명시된 대로user_fortune은 시드 데이터 없이 빈 상태로 시작합니다. 명시적인 ID 삽입을 통한 시드 주입이 필요하지 않다면,GENERATED BY DEFAULT대신 명시적 할당을 막는 더 엄격한GENERATED ALWAYS AS IDENTITY를 사용하는 것이 더 안전할 수 있습니다.🤖 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 `@api/src/main/resources/db/migration/V1__create_initial_schema.sql` around lines 303 - 332, Strengthen the fortune schema by adding foreign-key constraints for fortune_word.fortune_card_id, user_fortune.user_id, and user_fortune.fortune_word_id, following the project’s existing database convention. Since user_fortune starts empty and does not require seeded explicit IDs, change its id identity definition to GENERATED ALWAYS AS IDENTITY while leaving the seeded master-table strategies unchanged.storage/src/main/java/org/sopt/makers/storage/db/app/fortune/querydsl/FortuneCardQuerydslRepositoryImpl.java (1)
27-32: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win명시적인 Inner Join을 사용하는 것을 권장합니다.
.from(A, B, C)형태로 다중 엔티티를 선언하고where절로 조건을 주면, JPQL 생성 시 Cross Join(카테시안 곱)이 발생할 수 있습니다. DBMS 옵티마이저가 이를 최적화할 수는 있지만, 명시적으로innerJoin을 사용하는 것이 쿼리 성능을 보장하고 가독성 측면에서도 안전한 권장 방식입니다.♻️ 명시적 Join 적용 예시
- .from(userFortune, fortuneWord, fortuneCard) - .where( - userFortune.userId.eq(userId), - userFortune.fortuneWordId.eq(fortuneWord.id), - fortuneWord.fortuneCardId.eq(fortuneCard.id)) + .from(userFortune) + .innerJoin(fortuneWord).on(userFortune.fortuneWordId.eq(fortuneWord.id)) + .innerJoin(fortuneCard).on(fortuneWord.fortuneCardId.eq(fortuneCard.id)) + .where(userFortune.userId.eq(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 `@storage/src/main/java/org/sopt/makers/storage/db/app/fortune/querydsl/FortuneCardQuerydslRepositoryImpl.java` around lines 27 - 32, Update the QueryDSL query in FortuneCardQuerydslRepositoryImpl to replace the comma-separated `.from(userFortune, fortuneWord, fortuneCard)` with explicit inner joins, preserving the existing relationships among userFortune, fortuneWord, and fortuneCard and the userId filter. Keep the query’s returned result and fetchFirst behavior unchanged.domain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneService.java (2)
32-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value시간 기준(Clock vs 파라미터)의 일관성을 맞추는 것을 고려해 보세요.
getTodayFortuneWord는 외부에서todayDate를 파라미터로 주입받아 사용하지만,isExistTodayFortune은 내부적으로 주입받은clock을 사용하여 날짜를 구하고 있습니다. 시간 기준을 한 가지 방식으로 통일하면 유지보수와 테스트 관점에서의 일관성을 더욱 높일 수 있습니다.🤖 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 `@domain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneService.java` around lines 32 - 50, Unify the date source used by getTodayFortuneWord and isExistTodayFortune in FortuneService: either consistently derive today from the injected clock or consistently accept it as a parameter. Update the affected method signatures and callers as needed while preserving each method’s current behavior and testability.
52-60: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value신규 사용자 동시성 처리(TOCTOU) 관련 스키마 제약조건 확인
resolveTodayUserFortune메서드는 조회 후 데이터가 없으면 새로 생성하는 로직(orElseGet)을 사용하고 있습니다. 동일한 사용자에 대해 동시에 여러 번의 API 요청이 발생할 경우 DB에 중복된UserFortune데이터가 적재될 수 있는 레이스 컨디션이 발생합니다.데이터 무결성을 위해 DB 스키마 레벨에서
user_id컬럼에 Unique 제약 조건이 설정되어 있는지 확인해 주시기 바랍니다. 제약 조건이 설정되어 있다면 중복 적재 시 발생하는DataIntegrityViolationException을 통해 자연스럽게 방어할 수 있습니다.🤖 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 `@domain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneService.java` around lines 52 - 60, Verify the database schema and migration for the UserFortune entity used by resolveTodayUserFortune and ensure the user_id column has a unique constraint. Preserve the existing findByUserId/orElseGet flow, relying on the database constraint to prevent duplicate records during concurrent creation and surface DataIntegrityViolationException on conflicts.
🤖 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
`@domain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneWordIdGenerator.java`:
- Around line 14-19: Update FortuneWordIdGenerator.generate to use the
thread-safe ThreadLocalRandom instead of the shared RandomGenerator instance,
removing the singleton random field if no longer needed. Before selecting an ID,
explicitly handle an empty result from fortuneWordRepositoryPort.findAllIds() by
applying the project’s established exception behavior rather than calling
nextInt with a zero bound.
---
Nitpick comments:
In `@api/src/main/resources/db/migration/V1__create_initial_schema.sql`:
- Around line 303-332: Strengthen the fortune schema by adding foreign-key
constraints for fortune_word.fortune_card_id, user_fortune.user_id, and
user_fortune.fortune_word_id, following the project’s existing database
convention. Since user_fortune starts empty and does not require seeded explicit
IDs, change its id identity definition to GENERATED ALWAYS AS IDENTITY while
leaving the seeded master-table strategies unchanged.
In
`@domain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneService.java`:
- Around line 32-50: Unify the date source used by getTodayFortuneWord and
isExistTodayFortune in FortuneService: either consistently derive today from the
injected clock or consistently accept it as a parameter. Update the affected
method signatures and callers as needed while preserving each method’s current
behavior and testability.
- Around line 52-60: Verify the database schema and migration for the
UserFortune entity used by resolveTodayUserFortune and ensure the user_id column
has a unique constraint. Preserve the existing findByUserId/orElseGet flow,
relying on the database constraint to prevent duplicate records during
concurrent creation and surface DataIntegrityViolationException on conflicts.
In
`@storage/src/main/java/org/sopt/makers/storage/db/app/fortune/querydsl/FortuneCardQuerydslRepositoryImpl.java`:
- Around line 27-32: Update the QueryDSL query in
FortuneCardQuerydslRepositoryImpl to replace the comma-separated
`.from(userFortune, fortuneWord, fortuneCard)` with explicit inner joins,
preserving the existing relationships among userFortune, fortuneWord, and
fortuneCard and the userId filter. Keep the query’s returned result and
fetchFirst behavior unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f75983-730c-434d-94a9-729b8d15694f
📒 Files selected for processing (37)
api/src/main/java/org/sopt/makers/api/common/exception/AppExceptionHandler.javaapi/src/main/java/org/sopt/makers/api/common/response/AppFailureResponse.javaapi/src/main/java/org/sopt/makers/api/controller/app/fortune/FortuneApi.javaapi/src/main/java/org/sopt/makers/api/controller/app/fortune/FortuneController.javaapi/src/main/java/org/sopt/makers/api/controller/app/fortune/dto/FortuneCardResponse.javaapi/src/main/java/org/sopt/makers/api/controller/app/fortune/dto/FortuneResponse.javaapi/src/main/resources/db/migration/V1__create_initial_schema.sqlapi/src/test/java/org/sopt/makers/api/controller/app/AppChannelMockMvc.javaapi/src/test/java/org/sopt/makers/api/controller/app/fortune/FortuneControllerTest.javaapi/src/test/resources/mockito-extensions/org.mockito.plugins.MockMakerdomain/domain-app/build.gradle.ktsdomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/FortuneCard.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/FortuneWord.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/UserFortune.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/exception/FortuneException.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/exception/FortuneFailure.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/facade/FortuneFacade.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/port/FortuneCardRepositoryPort.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/port/FortuneWordRepositoryPort.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/port/UserFortuneRepositoryPort.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneService.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/fortune/service/FortuneWordIdGenerator.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/soptamp/SoptampUser.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/soptamp/port/SoptampPointUpdaterPort.javadomain/domain-app/src/main/java/org/sopt/makers/domain/app/soptamp/port/SoptampUserQueryPort.javadomain/domain-app/src/test/java/org/sopt/makers/domain/app/fortune/service/FortuneServiceTest.javastorage/build.gradle.ktsstorage/src/main/java/org/sopt/makers/storage/db/app/fortune/adapter/FortuneCardRepositoryAdapter.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/adapter/FortuneWordRepositoryAdapter.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/adapter/UserFortuneRepositoryAdapter.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/entity/FortuneCardEntity.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/entity/FortuneWordEntity.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/entity/UserFortuneEntity.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/querydsl/FortuneCardQuerydslRepository.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/querydsl/FortuneCardQuerydslRepositoryImpl.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/repository/FortuneWordJpaRepository.javastorage/src/main/java/org/sopt/makers/storage/db/app/fortune/repository/UserFortuneJpaRepository.java
…r 기본 구현은 스레드 안전하지 않음
1 participant
Related Issue 🚀
Work Description ✏️
fortune-api-controller
GET/api/v2/fortune/wordGET/api/v2/fortune/card/today관련 작업:
db/app/<도메인>경로에 둠 (storage 패키지가 도메인 모듈 구조를 미러링하도록){message, status, errors}) 에러 포맷으로 응답하게 함PR Point 📸