[SRLT-97] 쿼리 개선 #65
[SRLT-97] 쿼리 개선 #65
Conversation
- 서브섹션 한번에 조회하도록 수정
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
쿼리 인터페이스 src/main/java/starlight/application/businessplan/required/BusinessPlanQuery.java |
getOrThrowWithAllSubSections(Long id) 메서드 추가: 모든 서브섹션을 포함한 사업계획서 조회 계약 추가 |
영속성 레이어 — 리포지토리 src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanRepository.java |
findByIdWithAllSubSections(@param("id") Long id) 메서드 추가: 다중 LEFT JOIN FETCH JPQL로 사업계획서 및 모든 연관 서브엔티티를 한 번에 로드 |
영속성 레이어 — JPA 어댑터 src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanJpa.java |
getOrThrowWithAllSubSections(Long id) 메서드 추가: 리포지토리 호출 후 미존재 시 BusinessPlanException(BUSINESS_PLAN_NOT_FOUND) 발생 |
애플리케이션 서비스 src/main/java/starlight/application/businessplan/BusinessPlanServiceImpl.java |
getBusinessPlanDetail() 변경: 기존의 소유권 검사 결합 로직을 제거하고 getOrThrowWithAllSubSections()로 로드한 뒤 plan.isOwnedBy(memberId)로 별도 검증 수행 |
단위 테스트 src/test/java/starlight/application/businessplan/BusinessPlanServiceImplUnitTest.java |
테스트에서 getOrThrow(...) 호출을 getOrThrowWithAllSubSections(...)로 대체: 전체 서브섹션 반환 경로에 맞춘 목 설정 변경 |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- 검토 포인트:
findByIdWithAllSubSectionsJPQL이 모든 예상 연관관계를 정확히 fetch 하는지(중복/누락/카르티션 곱 여부) 검증- JPA 쿼리 성능 및 쿼리 결과가 엔티티 그래프와 매핑되는지 확인
- 서비스에서 소유권 검증 분리로 인한 권한 예외 처리 흐름(동일한 접근 경로에 누락된 검사 없는지) 점검
- 단위 테스트가 변경된 호출을 적절히 목킹하고 있으며 실제 시나리오를 충분히 커버하는지 확인
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | 제목은 '쿼리 개선'으로 PR의 핵심 변화인 서브섹션 조회 쿼리 최적화(단일 쿼리로 통합)를 정확하게 반영하고 있습니다. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
SRLT-97-QA-사항-반영
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 help to get the list of available commands and usage tips.
Test Results252 tests 252 ✅ 10s ⏱️ Results for commit c3df1b7. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/starlight/application/businessplan/BusinessPlanServiceImpl.java (1)
75-78: 상세 조회에서만 전체 서브섹션 로드하도록 분리한 점 좋습니다
getBusinessPlanDetail에서만getOrThrowWithAllSubSections를 써서, 무거운 fetch 그래프를 필요한 곳에만 제한한 점이 도메인/성능 관점에서 깔끔합니다.- 이후
plan.isOwnedBy(memberId)체크로 기존getOwnedBusinessPlanOrThrow와 동일한 예외 전략(존재하지 않으면BUSINESS_PLAN_NOT_FOUND, 소유 아니면UNAUTHORIZED_ACCESS)도 유지되고 있습니다.추가로, 중복되는 소유권 체크를 줄이고 싶다면 아래와 같이 별도 헬퍼로 빼는 것도 선택지입니다 (필수는 아닙니다):
private BusinessPlan getOwnedBusinessPlanWithAllSubSectionsOrThrow(Long planId, Long memberId) { BusinessPlan plan = businessPlanQuery.getOrThrowWithAllSubSections(planId); if (!plan.isOwnedBy(memberId)) { throw new BusinessPlanException(BusinessPlanErrorType.UNAUTHORIZED_ACCESS); } return plan; }그 후
getBusinessPlanDetail에서 이 메서드를 사용하는 식으로 공통화할 수 있습니다.src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanRepository.java (1)
26-47: 단일 쿼리로 전체 섹션/서브섹션을 가져오는 설계는 의도에 잘 부합합니다 (JPA 매핑만 한 번 확인 권장)
SELECT DISTINCT bp+LEFT JOIN FETCH조합으로 루트BusinessPlan한 건과 연관된 섹션/서브섹션을 한 번에 로딩하는 구조가, 이번 PR 목적(서브섹션 N+1 제거)에 잘 맞습니다.findById의@EntityGraph는 상위 섹션까지만 로딩하고, 이 메서드는 더 깊은 서브엔티티까지 가져오는 역할로 자연스럽게 분리된 것도 좋습니다.다만 JPA/Hibernate 특성상 한 쿼리에서 여러 개의 컬렉션(@onetomany, 특히 List/Bag) 를
JOIN FETCH하면 “multiple bag fetch” 예외가 발생할 수 있습니다. 지금 쿼리에 등장하는 필드들 중에서:
tc.teamMembers외에overviewBasic,problemBackground,problemPurpose,problemMarket,feasibilityStrategy,feasibilityMarket,growthModel,growthFunding,growthEntry등이 컬렉션(특히 List) 로 매핑되어 있다면 문제가 될 수 있습니다.- 전부
@OneToOne/@ManyToOne또는Set기반이라면 현재 설계는 문제 없습니다.한 번 엔티티 매핑을 보시고, 만약 두 개 이상이
@OneToMany List라면:
- 일부 연관을
@BatchSize+ 지연 로딩으로 빼거나,- DTO 전용 쿼리로 분리하는 등의 전략을 고려해 보시면 좋겠습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanJpa.java(1 hunks)src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanRepository.java(1 hunks)src/main/java/starlight/application/businessplan/BusinessPlanServiceImpl.java(1 hunks)src/main/java/starlight/application/businessplan/required/BusinessPlanQuery.java(1 hunks)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: build-and-push
🔇 Additional comments (3)
src/main/java/starlight/application/businessplan/required/BusinessPlanQuery.java (1)
11-12: 모든 서브섹션까지 로드하는 포트 메서드 네이밍/역할 적절해 보입니다
getOrThrowWithAllSubSections로 용도가 명확하게 드러나고, 기존getOrThrow와 책임이 잘 분리된 것 같습니다. 후속 어댑터/서비스 구현과도 일관적이라 이대로 가져가도 무방해 보입니다.src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanJpa.java (1)
25-30: 포트 구현이 기존getOrThrow패턴과 잘 맞습니다
findByIdWithAllSubSections->BusinessPlanException(BUSINESS_PLAN_NOT_FOUND)로 매핑하는 로직이 기존getOrThrow와 동일해 일관성이 좋습니다. 서비스단의@Transactional(readOnly = true)컨텍스트에서도 무리 없이 동작할 구조입니다.src/main/java/starlight/adapter/businessplan/persistence/BusinessPlanRepository.java (1)
19-23: 미리보기 쿼리 가독성 개선 👍텍스트 블록으로 정렬/정렬 기준이 한눈에 들어와서,
COALESCE(modifiedAt, createdAt) DESC, id DESC정렬 의도가 더 명확합니다. 동작 변경 없이 리팩터링된 것으로 보입니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/starlight/application/businessplan/BusinessPlanServiceImplUnitTest.java (1)
329-362: 쿼리 최적화를 위한 테스트 변경이 완료되었습니다.
getBusinessPlanDetail메서드와 관련된 두 테스트 모두 새로운getOrThrowWithAllSubSections메서드를 사용하도록 올바르게 업데이트되었습니다.참고로 다른 테스트들(예:
upsertSubSection,deleteSubSection등)은 여전히getOrThrow를 사용하는데, 이는 올바른 선택입니다. 해당 메서드들은 개별 서브섹션만 필요로 하므로 lazy loading이 더 효율적입니다.선택사항: 통합 테스트에서 실제 쿼리 성능 검증 고려
단위 테스트는 mock을 사용하므로 실제 쿼리 최적화 효과를 검증하지 못합니다. 여유가 되신다면 통합 테스트나 쿼리 실행 계획 검증을 통해 N+1 문제가 실제로 해결되었는지 확인하시면 좋을 것 같습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/starlight/application/businessplan/BusinessPlanServiceImplUnitTest.java(2 hunks)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: build-and-push
🔇 Additional comments (2)
src/test/java/starlight/application/businessplan/BusinessPlanServiceImplUnitTest.java (2)
340-340: 쿼리 최적화 변경이 올바르게 반영되었습니다.
getOrThrowWithAllSubSections를 사용하여 모든 서브섹션을 한 번에 조회하도록 mock을 설정한 것이 적절합니다. 이는 N+1 쿼리 문제를 해결하기 위한 PR 목표와 일치합니다.
358-358: 권한 검증 테스트도 새로운 조회 메서드와 일관성 있게 업데이트되었습니다.
getBusinessPlanDetail메서드가 사용하는 조회 방식 변경에 맞춰 테스트 mock도 일관성 있게 수정되었습니다. 권한 검증 로직은 조회 방식과 무관하게 올바르게 동작합니다.
📝작업 내용
🔎코드 설명
💬고민사항 및 리뷰 요구사항
비고 (Optional)
Summary by CodeRabbit
개선 사항
신규 기능
리팩토링
테스트
✏️ Tip: You can customize this high-level summary in your review settings.