Repository navigation
fix: AI 추천 서비스 누락 파일 추가 - #47
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AI course recommendation services and supporting persistence. The flow searches Naver places, resolves candidates, obtains missing prices from Gemini, filters combinations by budget, and returns up to two recommendations with unit tests. ChangesAI Course Recommendation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/main/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java (2)
101-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the course-type literals into constants.
The strings "식당", "카페", and "놀거리" appear in
COURSE_TYPESat line 36 and again here.combinerelies on the two lists holding identical values. If someone editsCOURSE_TYPESalone,byTypereturns empty lists andrecommendsilently returns no courses with a success code.Define named constants and build
COURSE_TYPESfrom them.♻️ Proposed change
- private static final List<String> COURSE_TYPES = List.of("식당", "카페", "놀거리"); + private static final String TYPE_RESTAURANT = "식당"; + private static final String TYPE_CAFE = "카페"; + private static final String TYPE_ACTIVITY = "놀거리"; + private static final List<String> COURSE_TYPES = List.of(TYPE_RESTAURANT, TYPE_CAFE, TYPE_ACTIVITY); @@ - List<Candidate> restaurants = byType(candidates, "식당"); - List<Candidate> cafes = byType(candidates, "카페"); - List<Candidate> activities = byType(candidates, "놀거리"); + List<Candidate> restaurants = byType(candidates, TYPE_RESTAURANT); + List<Candidate> cafes = byType(candidates, TYPE_CAFE); + List<Candidate> activities = byType(candidates, TYPE_ACTIVITY);🤖 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/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java` around lines 101 - 103, In CourseAiRecommendationService, define named constants for the restaurant, cafe, and activity course types, build COURSE_TYPES from those constants, and replace the literals passed to byType in recommend with the same constants so both paths remain synchronized.
57-76: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBatch the place and estimate lookups.
toCandidateruns onefindByNaverPlaceIdquery and onefindByPlacequery for every search result item, andsearchCandidatescalls it for all items of all three course types. The query count grows linearly with the Naverdisplaycount times three.Load the places with a single
findByNaverPlaceIdInquery and the estimates with a singlefindByPlaceInquery, then build the candidates from the resulting maps.🤖 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/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java` around lines 57 - 76, Refactor searchCandidates and toCandidate to batch persistence lookups: collect all external IDs, load existing places with one findByNaverPlaceIdIn call, create and save missing places as needed, then load estimates once via findByPlaceIn. Build candidates using maps keyed by external ID and place, and remove the per-item findByNaverPlaceId and findByPlace calls from toCandidate.src/test/java/com/daytodo/domain/course/service/CourseAiRecommendationServiceTest.java (2)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the stub by argument instead of by call order.
The consecutive-return stub binds
restaurant,cafe, andactivityto the first, second, and thirdfindByNaverPlaceIdinvocations. That order depends on howsearchCandidatesiteratesCOURSE_TYPESand the response items. A refactor that changes the iteration order makes the test assign the wrongPlaceto each course type, and the assertions still pass because they check only the recommendation order.Stub by the external id argument, and assert the place ids in the resulting course.
♻️ Proposed change
- when(placeRepository.findByNaverPlaceId(anyString())) - .thenReturn(Optional.of(restaurant), Optional.of(cafe), Optional.of(activity)); + when(placeRepository.findByNaverPlaceId("restaurant-link")).thenReturn(Optional.of(restaurant)); + when(placeRepository.findByNaverPlaceId("cafe-link")).thenReturn(Optional.of(cafe)); + when(placeRepository.findByNaverPlaceId("activity-link")).thenReturn(Optional.of(activity));Then assert the identity of each place:
assertThat(result.result().get(0).places()).extracting(CourseResponse.AiRecommendationPlace::recommendationOrder) .containsExactly(1, 2, 3); + assertThat(result.result().get(0).places()).extracting(CourseResponse.AiRecommendationPlace::placeId) + .containsExactly(1L, 2L, 3L);🤖 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/test/java/com/daytodo/domain/course/service/CourseAiRecommendationServiceTest.java` around lines 59 - 60, Update the findByNaverPlaceId stub in CourseAiRecommendationServiceTest to return restaurant, cafe, or activity based on each place’s external Naver id rather than invocation order. Then assert the resulting course contains the expected place ids for each recommendation, preserving order-independent repository behavior while verifying place identity.
81-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test and add a real budget-rejection case.
The test name states that no combination matches the budget. The setup returns an empty item list, so no candidates exist and the budget filter never runs. The test verifies the "no places found" path only.
Rename it to reflect the empty search result. Then add a separate test that supplies one place per course type with estimates outside the requested budget, and assert that
result()is empty. That case covers the strict containment rule incombine, where a course is rejected unlesstotalMin >= minBudgetandtotalMax <= maxBudget.A third case is also uncovered:
saveMissingPriceEstimatesdrops a candidate when the inference client returns no estimate for its key.🤖 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/test/java/com/daytodo/domain/course/service/CourseAiRecommendationServiceTest.java` around lines 81 - 93, Rename returnsEmptyListWhenNoCombinationMatchesBudget to describe the empty search-result scenario, then add a separate test with one candidate for each course type whose estimates fall outside the requested bounds and assert result() is empty, covering combine’s totalMin/totalMax containment checks. Add another test where saveMissingPriceEstimates receives no estimate for a candidate key and verify that candidate is excluded.src/main/java/com/daytodo/domain/course/service/GeminiPriceInferenceClient.java (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject a blank
GEMINI_API_KEYat startup.The default value for
GEMINI_API_KEYis an empty string. With a missing key the bean starts normally, and every call fails later as a genericCOURSE_RECOMMENDATION_FAILED. This hides a configuration error behind a runtime feature failure.Validate the key in the constructor, or remove the default so Spring fails to start when the property is absent.
♻️ Proposed change
- `@Value`("${GEMINI_API_KEY:}") String apiKey, + `@Value`("${GEMINI_API_KEY}") String apiKey,🤖 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/java/com/daytodo/domain/course/service/GeminiPriceInferenceClient.java` around lines 28 - 32, Update the GeminiPriceInferenceClient constructor to reject a missing or blank apiKey during bean creation, or remove the empty `@Value` default so Spring fails startup when GEMINI_API_KEY is absent. Preserve valid non-blank keys and ensure configuration errors surface at startup rather than during inference calls.
🤖 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/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java`:
- Around line 92-97: Ensure only one PlacePriceEstimate is created per Place:
update saveMissingPriceEstimates to group candidates by Place, invoke
saveEstimatedCandidate once per group, and reuse the saved estimate for other
candidates sharing that Place. In
src/main/java/com/daytodo/domain/place/repository/PlacePriceEstimateRepository.java:10,
confirm a unique database constraint exists on PlacePriceEstimate.place and add
it if missing.
- Around line 147-154: Update clean to decode HTML entities after removing
markup, so values such as & and " are persisted as their corresponding
characters. Update coordinate to return a nullable result for missing or
unparsable input, and adjust its callers to reject the item or persist null
rather than using 0 as a fallback.
- Around line 44-55: Restructure recommend so it no longer keeps a read-write
transaction open during remote I/O: perform searchCandidates and
existing-estimate loading in a read-only step, invoke the Gemini price inference
in saveMissingPriceEstimates with no transaction active, then persist new places
and estimates in a short write transaction. Preserve the existing response
assembly and filtering behavior while separating the read, remote-call, and
write phases.
In
`@src/main/java/com/daytodo/domain/course/service/GeminiPriceInferenceClient.java`:
- Around line 35-38: Update the RestClient construction in
GeminiPriceInferenceClient to use a request factory configured with explicit
connect and read timeouts before building the client. Preserve the existing
GEMINI_BASE_URL and x-goog-api-key configuration while ensuring estimate()
cannot leave the transactional recommendation flow waiting indefinitely.
---
Nitpick comments:
In
`@src/main/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java`:
- Around line 101-103: In CourseAiRecommendationService, define named constants
for the restaurant, cafe, and activity course types, build COURSE_TYPES from
those constants, and replace the literals passed to byType in recommend with the
same constants so both paths remain synchronized.
- Around line 57-76: Refactor searchCandidates and toCandidate to batch
persistence lookups: collect all external IDs, load existing places with one
findByNaverPlaceIdIn call, create and save missing places as needed, then load
estimates once via findByPlaceIn. Build candidates using maps keyed by external
ID and place, and remove the per-item findByNaverPlaceId and findByPlace calls
from toCandidate.
In
`@src/main/java/com/daytodo/domain/course/service/GeminiPriceInferenceClient.java`:
- Around line 28-32: Update the GeminiPriceInferenceClient constructor to reject
a missing or blank apiKey during bean creation, or remove the empty `@Value`
default so Spring fails startup when GEMINI_API_KEY is absent. Preserve valid
non-blank keys and ensure configuration errors surface at startup rather than
during inference calls.
In
`@src/test/java/com/daytodo/domain/course/service/CourseAiRecommendationServiceTest.java`:
- Around line 59-60: Update the findByNaverPlaceId stub in
CourseAiRecommendationServiceTest to return restaurant, cafe, or activity based
on each place’s external Naver id rather than invocation order. Then assert the
resulting course contains the expected place ids for each recommendation,
preserving order-independent repository behavior while verifying place identity.
- Around line 81-93: Rename returnsEmptyListWhenNoCombinationMatchesBudget to
describe the empty search-result scenario, then add a separate test with one
candidate for each course type whose estimates fall outside the requested bounds
and assert result() is empty, covering combine’s totalMin/totalMax containment
checks. Add another test where saveMissingPriceEstimates receives no estimate
for a candidate key and verify that candidate is excluded.
🪄 Autofix
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 Plus
Run ID: 2451826e-c0a7-4963-ab25-26eb55497efc
📒 Files selected for processing (6)
src/main/java/com/daytodo/domain/course/service/AiPriceInferenceClient.javasrc/main/java/com/daytodo/domain/course/service/CourseAiRecommendationService.javasrc/main/java/com/daytodo/domain/course/service/GeminiPriceInferenceClient.javasrc/main/java/com/daytodo/domain/place/repository/PlacePriceEstimateRepository.javasrc/test/java/com/daytodo/domain/course/service/CourseAiRecommendationServiceTest.javasrc/test/java/com/daytodo/domain/place/entity/PlacePriceEstimateTest.java
| private Candidate saveEstimatedCandidate(Candidate candidate, AiPriceInferenceClient.PriceEstimate estimate) { | ||
| if (estimate == null) return null; | ||
| PlacePriceEstimate saved = placePriceEstimateRepository.save(new PlacePriceEstimate(candidate.place(), estimate.minPrice(), | ||
| estimate.maxPrice(), estimate.confidence(), estimate.reason())); | ||
| return candidate.withPriceEstimate(saved); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Nothing enforces one PlacePriceEstimate per Place. The write path inserts an estimate row for every candidate without checking for an existing row, while the read path assumes at most one row exists. Two candidates in a single request can resolve to the same Place, because searchCandidates runs three separate Naver queries and one venue can appear in more than one of them. The second insert then makes the Optional query fail permanently for that place.
src/main/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java#L92-L97: group candidates byPlaceinsaveMissingPriceEstimatesand callsaveEstimatedCandidateat most once perPlace. Reuse the saved estimate for the remaining candidates that share thatPlace.src/main/java/com/daytodo/domain/place/repository/PlacePriceEstimateRepository.java#L10-L10: confirm that a unique constraint exists on theplacecolumn ofPlacePriceEstimate. If it does not exist, add it so the database rejects a duplicate insert instead of corrupting thefindByPlacecontract.
📍 Affects 2 files
src/main/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java#L92-L97(this comment)src/main/java/com/daytodo/domain/place/repository/PlacePriceEstimateRepository.java#L10-L10
🤖 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/java/com/daytodo/domain/course/service/CourseAiRecommendationService.java`
around lines 92 - 97, Ensure only one PlacePriceEstimate is created per Place:
update saveMissingPriceEstimates to group candidates by Place, invoke
saveEstimatedCandidate once per group, and reuse the saved estimate for other
candidates sharing that Place. In
src/main/java/com/daytodo/domain/place/repository/PlacePriceEstimateRepository.java:10,
confirm a unique database constraint exists on PlacePriceEstimate.place and add
it if missing.
|
수고하셨습니다 !! CodeRabbit이 남긴 🟠 Major 이슈 2개는 머지 전에 반영해주시면 좋을 것 같아요! recommend()가 @transactional인데 그 안에서 Naver API 3번 + Gemini API 1번을 호출하고 있어서, 외부 API가 느려지면 DB 커넥션을 계속 붙잡고 있게 될 것 같아요. Gemini 쪽에 타임아웃도 따로 없어서 최악의 경우 커넥션 풀 고갈까지 갈 수 있을 것 같습니다. 둘 다 CodeRabbit 코멘트에 구체적인 수정 방향까지 나와있어서 참고하시면 편하실 것 같습니다! |
2 participants
📌 관련 이슈
🔎 What is this PR?
✨ Changes
이전 PR에서 누락된 AI 추천 서비스, Gemini 가격 추론 클라이언트, 가격 추정 Repository를 추가했습니다.
관련 단위 테스트를 함께 추가했습니다.
📷 Result
💬 To. Reviewer
✅ 체크 리스트
Summary by CodeRabbit
New Features
Tests