fix: 조회 API N+1 쿼리 및 트랜잭션 오버헤드 제거(eliminate N+1 queries and implicit transaction overhead in schedule inquiry API)#39
Conversation
…ository 인터페이스에 추가
…Concert> 조합 객체 반환하도록 수정
…> -> List<ConcertScheduleWithConcert> 로 변경
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis change adds a composite record Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Controller as "Controller\n(HTTP)"
participant App as "ConcertReservationApplication"
participant Service as "ConcertService"
participant Repo as "ConcertRepository"
participant DB as "Database / JPA"
Client->>Controller: GET /available-schedules
Controller->>App: requestAvailableSchedulesWithConcert()
App->>Service: getAvailableConcertScheduleWithConcert(now)
Service->>Repo: findAvailableSchedules(presentDateTime)
Repo->>DB: query schedules
DB-->>Repo: schedules
Repo-->>Service: List<ConcertSchedule>
Service->>Service: extract distinct concertIds
Service->>Repo: findAllById(concertIds)
Repo->>DB: query concerts by ids
DB-->>Repo: concerts
Repo-->>Service: List<Concert>
Service->>Service: map concerts by id & pair each schedule
Service-->>App: List<ConcertScheduleWithConcert>
App-->>Controller: composite list
Controller-->>Client: HTTP 200 with mapped responses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/slam/concertreservation/application/NonConcurrentConcertReservationIntegrationTest.java (1)
176-189:⚠️ Potential issue | 🟠 MajorTest registers a schedule with
concertId=1Lbut never creates a correspondingConcert.With the new batch-fetch logic,
ConcertService.getAvailableConcertScheduleWithConcert()now callsfindAllByIdto load theConcertfor each schedule. Since noConcertwithid=1Lis created in this test, the concert lookup will return an empty result, likely causing the schedule to be dropped from theConcertScheduleWithConcertlist (or paired withnull). This test may pass incidentally if other tests create that concert first, making it order-dependent and fragile.Consider creating a concert explicitly before registering the schedule:
Proposed fix
void shouldSuccessfullyGetAvailableConcertSchedule() { // given // Create an available schedule (start time <= now) + Concert concert = concertReservationApplication.registerConcert(concertName, artistName); ConcertSchedule expected = concertReservationApplication.registerConcertSchedule(1L, - concertDateTime.plusDays(1), LocalDateTime.now().minusMinutes(1), reservationEndAt.plusDays(1), + concert.getId(), + concertDateTime.plusDays(1), LocalDateTime.now().minusMinutes(1), reservationEndAt.plusDays(1), 1000);Note:
registerConcertScheduletakesconcertIdas the first parameter, so use the actual concert's ID instead of the hardcoded1L.
🤖 Fix all issues with AI agents
In
`@src/main/java/com/slam/concertreservation/domain/concert/service/ConcertService.java`:
- Around line 284-295: In the batch mapping inside ConcertService (the code
using concertRepository.findAllById(...), the schedules.stream().map(... -> new
ConcertScheduleWithConcert(...))), guard against missing concerts by checking
concerts.get(schedule.getConcertId()) and fail fast: if null throw the same
UnavailableRequestException used previously (include schedule.getConcertId() /
schedule id in the message for context). Update the mapping logic so each
schedule either resolves to a ConcertScheduleWithConcert with a non-null Concert
or immediately throws UnavailableRequestException when the concert is absent.
In
`@src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/impl/ConcertRepositoryImpl.java`:
- Around line 30-36: The current findAllById implementation
(ConcertRepositoryImpl.findAllById) can return fewer Concerts than requested
which leads to nulls when matching schedules; update the code that builds
schedule responses to either (a) validate that the returned list from
findAllById contains all requested IDs (compare size or missing IDs) and throw a
clear exception before calling ConcertScheduleResponse.from, or (b) filter out
ConcertSchedule entries that reference missing concerts by checking the map
built from
concertJpaRepository.findAllById(ids).stream().map(ConcertJpaEntity::toDomain)
for null/missing keys and skipping those schedules before invoking
ConcertScheduleResponse.from(schedule, concert); choose one approach and apply
it where ConcertScheduleResponse.from is called to prevent NullPointerException.
🧹 Nitpick comments (3)
src/test/java/com/slam/concertreservation/domain/concert/service/ConcertServiceUnitTest.java (1)
327-352: Good coverage for the happy path.The test correctly verifies that
getAvailableConcertScheduleWithConcertpairs schedules with their concerts. Consider adding a test for the edge case wherefindAllAvailablereturns schedules butfindAllByIdreturns fewer concerts (e.g., data inconsistency), to document the expected behavior — silent drop vs. exception.src/main/java/com/slam/concertreservation/domain/concert/service/ConcertService.java (2)
284-288: Use a static import or regular import forCollectorsinstead of inline FQN.
java.util.stream.Collectors.toMapis referenced with a fully-qualified name while the rest of the file uses standard imports. This is a minor inconsistency.✏️ Suggested fix
Add at the import section:
import java.util.stream.Collectors;Then simplify:
Map<Long, Concert> concerts = concertRepository.findAllById(concertIds) .stream() .collect( - java.util.stream.Collectors.toMap(Concert::getId, concert -> concert)); + Collectors.toMap(Concert::getId, concert -> concert));
270-296: Consider short-circuiting when there are no available schedules.If
schedulesis empty, the subsequentfindAllByIdcall with an empty list is a wasted round-trip (even though it's harmless). An early return avoids the unnecessary DB call and stream processing.✏️ Suggested early return
List<ConcertSchedule> schedules = concertScheduleRepository.findAllAvailable(presentDateTime); + + if (schedules.isEmpty()) { + return List.of(); + } // 예약 가능한 공연 ID 목록 추출
| @Override | ||
| public List<Concert> findAllById(List<Long> ids) { | ||
| return concertJpaRepository.findAllById(ids) | ||
| .stream() | ||
| .map(ConcertJpaEntity::toDomain) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 30 'getAvailableConcertScheduleWithConcert' --type=java -g '!*Test*'Repository: leonroars/slam
Length of output: 7769
🏁 Script executed:
rg -n 'class ConcertScheduleWithConcert' --type=java -A 15Repository: leonroars/slam
Length of output: 40
🏁 Script executed:
rg -n 'ConcertScheduleWithConcert' --type=java | head -20Repository: leonroars/slam
Length of output: 2909
🏁 Script executed:
cat -n src/main/java/com/slam/concertreservation/domain/concert/model/ConcertScheduleWithConcert.javaRepository: leonroars/slam
Length of output: 402
🏁 Script executed:
rg -n -B 5 -A 10 'getAvailableConcertSchedulesWithConcert' src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.javaRepository: leonroars/slam
Length of output: 694
🏁 Script executed:
rg -n -B 3 -A 15 'class ConcertScheduleResponse' --type=java -g '!*Test*' | head -50Repository: leonroars/slam
Length of output: 2441
findAllById doesn't guarantee all requested IDs are found — will cause NullPointerException.
Spring Data JPA's findAllById returns only entities that exist. If a ConcertSchedule references a concertId that doesn't exist in the DB, concerts.get(schedule.getConcertId()) returns null. This null Concert is then passed to ConcertScheduleResponse.from(schedule, concert), which calls concert.getName() and concert.getArtist() (lines 25-26 in ConcertScheduleResponse), causing a NullPointerException at runtime.
Either validate that all Concert IDs exist before creating results, or filter out schedules with missing concerts.
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/infrastructure/persistence/jpa/impl/ConcertRepositoryImpl.java`
around lines 30 - 36, The current findAllById implementation
(ConcertRepositoryImpl.findAllById) can return fewer Concerts than requested
which leads to nulls when matching schedules; update the code that builds
schedule responses to either (a) validate that the returned list from
findAllById contains all requested IDs (compare size or missing IDs) and throw a
clear exception before calling ConcertScheduleResponse.from, or (b) filter out
ConcertSchedule entries that reference missing concerts by checking the map
built from
concertJpaRepository.findAllById(ids).stream().map(ConcertJpaEntity::toDomain)
for null/missing keys and skipping those schedules before invoking
ConcertScheduleResponse.from(schedule, concert); choose one approach and apply
it where ConcertScheduleResponse.from is called to prevent NullPointerException.
1 participant
fix: 조회 API N+1 쿼리 및 트랜잭션 오버헤드 제거 (eliminate N+1 queries and implicit transaction overhead in schedule inquiry API)
🇰🇷 한글
배경
프론트엔드의 N+1 API 호출을 제거하기 위해 서버 측 DTO Aggregation을 도입한 이후, 동일 조건 부하 테스트에서 처리량이 약 30% 하락한 것을 확인했습니다.
(이전 약 900 RPS → 변경 후 약 630 RPS에서 포화)
원인
계층별 소거 진단(DB CloudWatch → 앱 서버 리소스 → HikariCP → MySQL QPS → General Log)을 통해 두 가지 복합 원인을 규명했습니다.
1. N+1 쿼리 패턴
핸들러 메서드에서 공연 일정 목록 조회 후, 각 일정마다
getConcert()를 루프 호출하여 건별 SELECT가 발생했습니다.2. Spring Data JPA 트랜잭션 오버헤드
getConcert()서비스 메서드에@Transactional이 없었으나,SimpleJpaRepository의 클래스 레벨@Transactional(readOnly = true)에 의해findById()호출마다 독립 트랜잭션이 생성되었습니다. 이로 인해 건별 조회마다SET SESSION TRANSACTION READ ONLY→COMMIT→SET SESSION TRANSACTION READ WRITE부수 명령이 반복 발생했습니다.두 문제가 곱해져 요청당 DB 커맨드가 SELECT 2개면 충분한 상황에서 **21개(N=5 기준)**까지 증가했습니다.
해결 방안 비교 및 결정
@ManyToOne매핑 후JOIN FETCHfindAllById()+@Transactional(readOnly = true)@BatchSize(size = N)@ManyToOne연관관계 필요 → 현재 적용 불가FROM A, B WHERE A.id = B.idObject[]반환으로 타입 안전성 낮음결정 근거:
변경 내용
getConcert()루프 호출을findAllById()IN 절 배치 조회로 변경@Transactional(readOnly = true)적용하여 트랜잭션 경계 통합ConcertScheduleWithConcert도메인 조합 객체 추가 (서비스가 DTO를 반환하지 않도록 유지)결과
위와 같이
SELECT쿼리 2개 + 트랜잭션 1 세트(SET SESSION..2개 +COMMIT) 으로 불필요한 트랜잭션 관련 부수 명령으로 인한 쿼리가 줄어들었음을 확인했습니다.🇺🇸 English
Background
After introducing server-side DTO aggregation to eliminate N+1 API calls from the frontend, load testing under identical conditions revealed a ~30% throughput drop (previous ~900 RPS → ~630 RPS saturation point).
Root Cause
Layered elimination diagnosis (DB CloudWatch → app server resources → HikariCP → MySQL QPS → General Log) identified two compounding issues:
1. N+1 Query Pattern
The handler method iterated over concert schedule results, invoking
getConcert()per schedule, causing individual SELECT queries per iteration.2. Spring Data JPA Implicit Transaction Overhead
Although
getConcert()had no explicit@Transactional,SimpleJpaRepository's class-level@Transactional(readOnly = true)caused eachfindById()call to open an independent transaction. This resulted in repeatedSET SESSION TRANSACTION READ ONLY→COMMIT→SET SESSION TRANSACTION READ WRITEauxiliary commands per call.Combined, these issues inflated DB commands from 2 SELECTs to 21 commands per request (N=5).
Solution Comparison and Decision
@ManyToOne, useJOIN FETCHfindAllById()+@Transactional(readOnly = true)@BatchSize(size = N)@ManyToOneassociation → not applicableFROM A, B WHERE A.id = B.idwithout associationsObject[]return type, low type safetyDecision rationale:
Changes
getConcert()loop withfindAllById()IN-clause batch fetch@Transactional(readOnly = true)to the service method to consolidate transaction boundariesConcertScheduleWithConcertdomain combination object (maintaining the principle that services don't return DTOs)Result
As shown above, the request now completes with 2 SELECT queries + 1 transaction set (2 SET SESSION.. + COMMIT), confirming the elimination of redundant transaction auxiliary commands.
Summary by CodeRabbit