Conversation
Contributor
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Caller
participant Svc as CourseGenerationAiService
participant VS as VectorStore
participant AI as ChatClient
Client->>Svc: generateCourse(request: 기간, 위치들)
par 위치별 병렬 검색
Svc->>VS: similaritySearch(location_i, topK=30)
VS-->>Svc: Documents_i
and 부족 시 결합 검색
Svc->>VS: similaritySearch(combined, topK=80)
VS-->>Svc: Documents_combined
end
Svc->>Svc: parseAndAddDocuments → SimpleSpotDTO Map(중복 제거)
Svc->>Svc: spotsForOpenAI(앵커/위성 후보 선별, fastDist2)
Svc->>AI: prompt(start/end, locations, candidates)
AI-->>Svc: entity(CourseDetailDTO)
Svc->>Svc: postFix(aiResult, originalSpotMap) → 일자별 코스/타이틀 구성
Svc-->>Client: CourseResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit d266497. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java (1)
65-76: NPE 위험: request null 검사 순서 및 필드 검증 누락request를 사용한 뒤에 requireNonNull을 호출하고 있어 NPE 가능성이 있습니다. startDate/endDate/locations에 대한 null/empty 검증도 함께 선반영해 주세요.
다음 패치를 적용해 주세요:
public CourseResponse.CourseDetailDTO generateAiCourse( CourseRequest.CreateCourseRequest request) { - LocalDate today = LocalDate.now(); - if (request.getStartDate().isBefore(today) || request.getEndDate().isBefore(today)) { - throw new CustomException(CourseErrorStatus.PAST_DATE_NOT_ALLOWED); - } - Objects.requireNonNull(request, "request must not be null"); - LocalDate start = request.getStartDate(); - LocalDate end = request.getEndDate(); - int expectedDays = (int) ChronoUnit.DAYS.between(start, end) + 1; - List<String> locations = request.getLocations(); + Objects.requireNonNull(request, "request must not be null"); + LocalDate start = + Objects.requireNonNull(request.getStartDate(), "startDate must not be null"); + LocalDate end = + Objects.requireNonNull(request.getEndDate(), "endDate must not be null"); + List<String> locations = + Objects.requireNonNull(request.getLocations(), "locations must not be null"); + if (locations.isEmpty()) { + throw new IllegalArgumentException("locations must not be empty"); + } + LocalDate today = LocalDate.now(); + if (start.isBefore(today) || end.isBefore(today)) { + throw new CustomException(CourseErrorStatus.PAST_DATE_NOT_ALLOWED); + } + int expectedDays = (int) ChronoUnit.DAYS.between(start, end) + 1; if (expectedDays <= 0) { throw new IllegalArgumentException("endDate must be on/after startDate"); }
🧹 Nitpick comments (8)
src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java (8)
85-111: 스레드 풀 전략 개선 및 예외 처리 보강 제안newCachedThreadPool은 부하 시 스레드가 무제한 증가할 수 있습니다. 위치 수는 제한적이므로 고정 크기 풀을 권장하며, 각 future의 예외를 개별 로깅/격리하세요.
다음과 같이 한정된 풀을 사용하고, allOf + handle로 예외를 흡수하는 방안을 고려해 주세요:
- ExecutorService executor = Executors.newCachedThreadPool(); + ExecutorService executor = + Executors.newFixedThreadPool(Math.min(locations.size(), + Math.max(2, Runtime.getRuntime().availableProcessors()))); try { - List<CompletableFuture<List<Document>>> futures = + List<CompletableFuture<List<Document>>> futures = locations.stream() .map( location -> CompletableFuture.supplyAsync( () -> { log.info( "Executing parallel search for '{}' with topK={}", location, topKPerLocation); SearchRequest searchRequest = SearchRequest.builder() .query(location) .topK(topKPerLocation) .build(); return vectorStore.similaritySearch(searchRequest); }, executor)) .toList(); - parallelDocuments.addAll( - futures.stream().map(CompletableFuture::join).flatMap(List::stream).toList()); + CompletableFuture.allOf(futures.toArray(CompletableFuture[]::new)).join(); + parallelDocuments.addAll( + futures.stream() + .map(f -> f.exceptionally(ex -> { + log.warn("Vector search failed: {}", ex.toString()); + return List.<Document>of(); + })) + .map(CompletableFuture::join) + .flatMap(List::stream) + .toList()); } finally { executor.shutdown(); }
214-219: 예외 래핑 시 원인 스택이 손실됩니다e.getMessage()만 포함하면 디버깅이 어려워집니다. cause를 함께 전달하세요.
- } catch (JsonProcessingException e) { - throw new RuntimeException("AI 프롬프트 준비 실패: " + e.getMessage()); - } + } catch (JsonProcessingException e) { + throw new RuntimeException("AI 프롬프트 준비 실패", e); + }
221-237: 프롬프트 규칙(최소 4개) ↔ postFix(최소 3개) 불일치프롬프트는 “하루 최소 4개”를 요구하지만 후처리는 3개 미만을 드롭하고 있습니다. 일관성을 위해 후처리 기준을 4로 맞추세요.
315-324: 빈 locations 대비 기본값 선택 시 IndexOutOfBounds 위험유효성 검증이 우회될 경우 requestedLocations가 비어 있을 수 있습니다. 안전 가드를 추가하세요.
private String findLocationForSpot(String addr1, List<String> requestedLocations) { - if (addr1 == null) return requestedLocations.get(0); + if (requestedLocations == null || requestedLocations.isEmpty()) return ""; + if (addr1 == null) return requestedLocations.get(0);
361-362: 후처리 최소 방문지 기준 3 → 4로 상향프롬프트와 일치하도록 최소 방문지 수를 4로 조정하세요.
- if (restoredSpots.size() < 3) continue; + if (restoredSpots.size() < 4) continue;
380-386: 제목의 ‘N일’ 계산을 실제 확정 일수에 맞추기AI 결과 후처리로 일부 일이 드롭되면 제목의 일수(ChronoUnit 기반)와 불일치할 수 있습니다. 확정 일수 기반으로 계산하세요.
- long finalDays = ChronoUnit.DAYS.between(start, end) + 1; + long finalDays = + validatedDailyCourses.isEmpty() + ? ChronoUnit.DAYS.between(start, end) + 1 + : validatedDailyCourses.size();
421-428: 좌표 0도를 에러로 간주하는 로직은 일반성 낮음대한민국 데이터에서는 영향이 거의 없지만, (0,0)을 정합한 좌표로 처리해야 하는 데이터셋에서는 부자연스러운 결과를 유발합니다. null/결측만 상단에서 필터링하고, fastDist2는 순수 거리 계산으로 두는 것을 권장합니다.
238-249: OpenAiChatOptions — 토큰 옵션 확인 및 JSON 출력 강제화 권장
- 토큰 제한 옵션 확인: OpenAiChatOptions.Builder에 maxTokens(Integer) — 비‑reasoning 모델용, maxCompletionTokens(Integer) — reasoning 모델용이 있으며 상호배타적입니다(둘 다 설정하면 마지막 설정이 우선). (docs.spring.io)
- DTO 직접 매핑 시 responseFormat 강제 권장: OpenAiChatOptions.builder().responseFormat(...)로 ResponseFormat.Type.JSON_OBJECT를 사용해 JSON 모드 강제하거나, 구조화 응답이 필요하면 JSON_SCHEMA + schema(+ strict) 사용하세요. JSON 모드 사용 시 모델에게 JSON 생성 지시(system/user 메시지)를 함께 전달해야 합니다(무한 공백 출력/부분 잘림 위험). (docs.spring.io)
- 파일: src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java (238-249) — chatClient 호출부에 responseFormat 설정 추가 및 사용 중인 모델 유형에 맞게 maxTokens / maxCompletionTokens 중 하나만 적용하세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/yfive/gbjs/domain/course/dto/response/CourseResponse.java(2 hunks)src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/yfive/gbjs/domain/course/dto/response/CourseResponse.java (2)
src/main/java/com/yfive/gbjs/domain/course/dto/request/CourseRequest.java (1)
Builder(16-35)src/main/java/com/yfive/gbjs/domain/seal/dto/response/SealResponse.java (2)
Builder(19-51)Builder(53-64)
src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java (2)
src/main/java/com/yfive/gbjs/domain/course/dto/request/CourseRequest.java (1)
CourseRequest(14-133)src/main/java/com/yfive/gbjs/domain/course/dto/response/CourseResponse.java (1)
CourseResponse(12-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (2)
src/main/java/com/yfive/gbjs/domain/course/dto/response/CourseResponse.java (2)
36-38: @DaTa 도입으로 인한 가변성/동치성 영향 확인 필요@Getter → @DaTa 전환으로 세터/equals/hashCode/toString이 생성됩니다. 응답 DTO를 셋/맵 키로 쓰거나 캐시에 넣는 코드가 있다면 동치성/가변성으로 인한 회귀가 없는지 확인해 주세요. 필요 시 equals/hashCode 범위를 식별자 위주로 제한(@EqualsAndHashCode(of = "..."))하는 방안을 고려해 주세요.
83-85: Boolean 필드(getIsSealSpot) null 안전성 점검partitioningBy(SimpleSpotDTO::getIsSealSpot)에서 값이 null이면 NPE가 납니다. 현재 생성 경로(parseAndAddDocuments)에서는 항상 true/false로 세팅되지만, 외부 주입/역직렬화 경로가 추가되면 null이 들어올 수 있습니다. 필드에 @Builder.Default로 false를 기본값으로 주는 것도 방법입니다.
Comment on lines
+155
to
+172
| if (sealSpots.size() >= 2) { | ||
| Collections.shuffle(sealSpots, rand); | ||
| List<CourseResponse.SimpleSpotDTO> anchors = sealSpots.stream().limit(2).toList(); | ||
| double midLat = (anchors.get(0).getLatitude() + anchors.get(1).getLatitude()) / 2; | ||
| double midLon = (anchors.get(0).getLongitude() + anchors.get(1).getLongitude()) / 2; | ||
| List<CourseResponse.SimpleSpotDTO> satelliteCandidates = | ||
| regularSpots.stream() | ||
| .sorted( | ||
| Comparator.comparingDouble( | ||
| spot -> fastDist2(midLat, midLon, spot.getLatitude(), spot.getLongitude()))) | ||
| .limit(7) | ||
| .collect(Collectors.toCollection(ArrayList::new)); | ||
| Collections.shuffle(satelliteCandidates, rand); | ||
| List<CourseResponse.SimpleSpotDTO> satellites = | ||
| satelliteCandidates.stream().limit(3).toList(); | ||
| finalCandidates.addAll(anchors); | ||
| finalCandidates.addAll(satellites); | ||
| } else if (sealSpots.size() == 1) { |
Contributor
There was a problem hiding this comment.
자동 언박싱 NPE 가능: 위경도 null 처리 없이 거리 계산
anchor/regular spot에서 latitude/longitude가 null일 수 있어 Double→double 언박싱 시 NPE가 납니다. 비교/정렬 전에 좌표가 있는 데이터만 사용하도록 필터링하고, 좌표 부족 시 안전한 폴백을 두세요.
다음 패치를 적용해 주세요:
- if (sealSpots.size() >= 2) {
- Collections.shuffle(sealSpots, rand);
- List<CourseResponse.SimpleSpotDTO> anchors = sealSpots.stream().limit(2).toList();
- double midLat = (anchors.get(0).getLatitude() + anchors.get(1).getLatitude()) / 2;
- double midLon = (anchors.get(0).getLongitude() + anchors.get(1).getLongitude()) / 2;
+ if (sealSpots.size() >= 2) {
+ List<CourseResponse.SimpleSpotDTO> anchorCandidates =
+ sealSpots.stream()
+ .filter(s -> s.getLatitude() != null && s.getLongitude() != null)
+ .collect(Collectors.toCollection(ArrayList::new));
+ if (anchorCandidates.size() < 2 && !regularSpots.isEmpty()) {
+ // 좌표가 부족하면 레귤러 중 좌표 있는 후보로 보강
+ anchorCandidates.addAll(
+ regularSpots.stream()
+ .filter(s -> s.getLatitude() != null && s.getLongitude() != null)
+ .limit(2)
+ .toList());
+ }
+ if (anchorCandidates.size() < 2) {
+ continue; // 폴백 불가 시 스킵
+ }
+ Collections.shuffle(anchorCandidates, rand);
+ List<CourseResponse.SimpleSpotDTO> anchors = anchorCandidates.stream().limit(2).toList();
+ double midLat = (anchors.get(0).getLatitude() + anchors.get(1).getLatitude()) / 2.0;
+ double midLon = (anchors.get(0).getLongitude() + anchors.get(1).getLongitude()) / 2.0;
List<CourseResponse.SimpleSpotDTO> satelliteCandidates =
- regularSpots.stream()
+ regularSpots.stream()
+ .filter(s -> s.getLatitude() != null && s.getLongitude() != null)
.sorted(
Comparator.comparingDouble(
spot -> fastDist2(midLat, midLon, spot.getLatitude(), spot.getLongitude())))
.limit(7)
.collect(Collectors.toCollection(ArrayList::new));
Collections.shuffle(satelliteCandidates, rand);
List<CourseResponse.SimpleSpotDTO> satellites =
satelliteCandidates.stream().limit(3).toList();
finalCandidates.addAll(anchors);
finalCandidates.addAll(satellites);
- } else if (sealSpots.size() == 1) {
- CourseResponse.SimpleSpotDTO anchor = sealSpots.get(0);
+ } else if (sealSpots.size() == 1) {
+ CourseResponse.SimpleSpotDTO anchor = sealSpots.get(0);
+ if (anchor.getLatitude() == null || anchor.getLongitude() == null) {
+ // 앵커에 좌표가 없으면 스킵
+ continue;
+ }
List<CourseResponse.SimpleSpotDTO> satellites =
- regularSpots.stream()
+ regularSpots.stream()
+ .filter(s -> s.getLatitude() != null && s.getLongitude() != null)
.sorted(
Comparator.comparingDouble(
spot ->
fastDist2(
anchor.getLatitude(),
anchor.getLongitude(),
spot.getLatitude(),
spot.getLongitude())))
.limit(4)
.toList();
finalCandidates.add(anchor);
finalCandidates.addAll(satellites);
} else {
if (regularSpots.isEmpty()) continue;
double avgLat =
regularSpots.stream()
.filter(s -> s.getLatitude() != null)
.mapToDouble(CourseResponse.SimpleSpotDTO::getLatitude)
.average()
.orElse(0.0);
double avgLon =
regularSpots.stream()
.filter(s -> s.getLongitude() != null)
.mapToDouble(CourseResponse.SimpleSpotDTO::getLongitude)
.average()
.orElse(0.0);
List<CourseResponse.SimpleSpotDTO> centeredSpots =
- regularSpots.stream()
+ regularSpots.stream()
+ .filter(s -> s.getLatitude() != null && s.getLongitude() != null)
.sorted(
Comparator.comparingDouble(
spot -> fastDist2(avgLat, avgLon, spot.getLatitude(), spot.getLongitude())))
.limit(5)
.toList();
finalCandidates.addAll(centeredSpots);
}Also applies to: 173-188, 191-209
🤖 Prompt for AI Agents
In
src/main/java/com/yfive/gbjs/domain/course/service/CourseGenerationAiService.java
around lines 155-172 (also apply same fix for 173-188 and 191-209): the code
does distance calculations using spot.getLatitude()/getLongitude() which can be
null and will cause a Double→double unboxing NPE; filter out spots that have
null latitude or longitude before any comparator/fastDist2 calls, and when there
are not enough coordinate-bearing spots provide a safe fallback path (e.g., skip
distance-based sorting, choose only spots with non-null coords, or handle the
empty case) so no unboxing of null occurs and logic still handles insufficient
coordinate data gracefully.
Merged
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.
Summary by CodeRabbit