<feat> API Response DTO 도입 및 TSID 변경 관련 변경#37
Conversation
📝 WalkthroughWalkthroughAll public API endpoints in the ConcertReservationController have been refactored to return DTO-based responses instead of domain entities. Eight new DTO classes were introduced (UserResponse, ConcertResponse, ConcertScheduleResponse, SeatResponse, ReservationResponse, TokenResponse, UserPointBalanceResponse, PointHistoryResponse), each with factory methods for converting domain models. Related data fetching logic was added to endpoints where necessary to populate DTOs with associated entities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 3
🤖 Fix all issues with AI agents
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`:
- Around line 119-129: The controller method getAvailableConcertSchedules causes
an N+1 by calling reservationApp.getConcert(schedule.getConcertId()) per
schedule; fix by collecting unique concert IDs from
getAvailableConcertSchedules(), batch-fetching concerts via a new
reservationApp.getConcertsByIds(Set<Long>) method, build a Map<Long,Concert>,
and then call ConcertScheduleResponse.from(schedule,
concertMap.get(schedule.getConcertId())) when mapping schedules to responses so
each concert is fetched once.
- Around line 207-219: The current getUserReservations controller implementation
causes an N+1 query by calling reservationApp.getSeat(...) for each Reservation;
instead, after calling reservationApp.getUserReservations(...) collect all
distinct seatIds, call a new/appropriate batch method on reservationApp (e.g.,
getSeatsByIds) to fetch all Seat objects at once, build a Map<Long,Seat> keyed
by seatId, and then map reservations to ReservationResponse using
ReservationResponse.from(reservation, seatMap.get(reservation.getSeatId())) so
each seat is retrieved from the map rather than via repeated
reservationApp.getSeat calls.
- Around line 226-236: The cookie set in issueToken (method issueToken, class
ConcertReservationController) lacks secure and SameSite attributes; change the
cookie handling so the token cookie is marked Secure and SameSite (preferably
Strict or Lax) and keep HttpOnly and Path. Replace the current
response.addCookie(new Cookie(...)) usage with a construction that sets secure
(cookie.setSecure(true)) and ensures SameSite is emitted (either build the
Set-Cookie header manually on the HttpServletResponse or use Spring's
ResponseCookie/Servlet API to include SameSite), and make the Secure flag
configurable via a profile/property so non-HTTPS dev remains possible; return
TokenResponse.from(token) unchanged.
🧹 Nitpick comments (3)
src/test/java/com/slam/concertreservation/interfaces/dto/DtoSerializationTest.java (2)
13-37: Consider expanding test coverage for the remaining DTOs.The tests effectively verify that string IDs are preserved during JSON serialization. However, only 2 of the 8 new DTOs are covered. Given that the PR's primary goal is fixing JavaScript precision loss across all ID fields, consider adding similar tests for the other DTOs (especially
TokenResponse,SeatResponse, andReservationResponse) to ensure comprehensive coverage.Example test for TokenResponse
`@Test` `@DisplayName`("TokenResponse IDs should be serialized as Strings") void tokenResponseSerialization() throws Exception { TokenResponse dto = TokenResponse.builder() .id("token-uuid-string") .userId("1234567890123456789") .concertScheduleId("9876543210987654321") .status("ACTIVE") .expiredAt(LocalDateTime.now()) .build(); String json = objectMapper.writeValueAsString(dto); assertThat(json).contains("\"userId\":\"1234567890123456789\""); assertThat(json).contains("\"concertScheduleId\":\"9876543210987654321\""); }
28-32: Minor: ConcertResponse test is missing theartistfield.The
ConcertResponseDTO has anartistfield, but it's not set in the test. While not strictly required for the ID serialization test, setting all fields would make the test more representative and catch any serialization issues with the complete DTO.Suggested improvement
ConcertResponse dto = ConcertResponse.builder() .id("9876543210987654321") .name("Test Concert") + .artist("Test Artist") .build();src/main/java/com/slam/concertreservation/interfaces/dto/ConcertScheduleResponse.java (1)
21-32: Consider adding validation for mismatched Concert.The factory method assumes the caller passes the correct
Concertfor the givenConcertSchedule. A mismatch (e.g., schedule'sconcertId!=concert.getId()) would silently produce incorrect data.🛡️ Optional defensive validation
public static ConcertScheduleResponse from(ConcertSchedule schedule, Concert concert) { + if (!schedule.getConcertId().equals(concert.getId())) { + throw new IllegalArgumentException("Concert ID mismatch: schedule.concertId=" + + schedule.getConcertId() + ", concert.id=" + concert.getId()); + } return ConcertScheduleResponse.builder()
| @GetMapping("/concerts/schedules/available") | ||
| public ResponseEntity<List<ConcertSchedule>> getAvailableConcertSchedules() { | ||
| return ResponseEntity.ok(reservationApp.getAvailableConcertSchedules()); | ||
| public ResponseEntity<List<ConcertScheduleResponse>> getAvailableConcertSchedules() { | ||
| List<ConcertSchedule> schedules = reservationApp.getAvailableConcertSchedules(); | ||
| List<ConcertScheduleResponse> responses = schedules.stream() | ||
| .map(schedule -> { | ||
| Concert concert = reservationApp.getConcert(schedule.getConcertId()); | ||
| return ConcertScheduleResponse.from(schedule, concert); | ||
| }) | ||
| .toList(); | ||
| return ResponseEntity.ok(responses); | ||
| } |
There was a problem hiding this comment.
N+1 query issue: Consider batch-fetching concerts.
Each schedule triggers a separate getConcert() call, resulting in N+1 queries. For M schedules, this makes M additional database calls. Per the PR objectives, this is planned for a separate optimization PR, but flagging for visibility.
♻️ Suggested optimization approach for future PR
// Collect unique concertIds first
Set<Long> concertIds = schedules.stream()
.map(ConcertSchedule::getConcertId)
.collect(Collectors.toSet());
// Batch fetch all concerts
Map<Long, Concert> concertMap = reservationApp.getConcertsByIds(concertIds)
.stream()
.collect(Collectors.toMap(Concert::getId, Function.identity()));
// Map with cached lookups
List<ConcertScheduleResponse> responses = schedules.stream()
.map(schedule -> ConcertScheduleResponse.from(schedule, concertMap.get(schedule.getConcertId())))
.toList();🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`
around lines 119 - 129, The controller method getAvailableConcertSchedules
causes an N+1 by calling reservationApp.getConcert(schedule.getConcertId()) per
schedule; fix by collecting unique concert IDs from
getAvailableConcertSchedules(), batch-fetching concerts via a new
reservationApp.getConcertsByIds(Set<Long>) method, build a Map<Long,Concert>,
and then call ConcertScheduleResponse.from(schedule,
concertMap.get(schedule.getConcertId())) when mapping schedules to responses so
each concert is fetched once.
| @GetMapping("/users/{userId}/reservations") | ||
| public ResponseEntity<List<Reservation>> getUserReservations(@PathVariable String userId) { | ||
| return ResponseEntity.ok(reservationApp.getUserReservations(Long.valueOf(userId))); | ||
| public ResponseEntity<List<ReservationResponse>> getUserReservations(@PathVariable String userId) { | ||
| List<Reservation> reservations = reservationApp.getUserReservations(Long.valueOf(userId)); | ||
| List<ReservationResponse> responses = reservations.stream() | ||
| .map(reservation -> { | ||
| // Seat 정보 조회하여 seatNumber 포함 | ||
| Seat seat = reservationApp.getSeat(reservation.getSeatId()); | ||
| return ReservationResponse.from(reservation, seat); | ||
| }) | ||
| .toList(); | ||
|
|
||
| return ResponseEntity.ok(responses); | ||
| } |
There was a problem hiding this comment.
N+1 query issue: Batch-fetch seats for user reservations.
Same N+1 pattern as getAvailableConcertSchedules(). For M reservations, this makes M additional getSeat() calls. Consider batch-fetching seats by their IDs in the future optimization PR.
♻️ Suggested optimization approach for future PR
List<Reservation> reservations = reservationApp.getUserReservations(Long.valueOf(userId));
// Batch fetch all seats
Set<Long> seatIds = reservations.stream()
.map(Reservation::getSeatId)
.collect(Collectors.toSet());
Map<Long, Seat> seatMap = reservationApp.getSeatsByIds(seatIds)
.stream()
.collect(Collectors.toMap(Seat::getId, Function.identity()));
List<ReservationResponse> responses = reservations.stream()
.map(r -> ReservationResponse.from(r, seatMap.get(r.getSeatId())))
.toList();🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`
around lines 207 - 219, The current getUserReservations controller
implementation causes an N+1 query by calling reservationApp.getSeat(...) for
each Reservation; instead, after calling reservationApp.getUserReservations(...)
collect all distinct seatIds, call a new/appropriate batch method on
reservationApp (e.g., getSeatsByIds) to fetch all Seat objects at once, build a
Map<Long,Seat> keyed by seatId, and then map reservations to ReservationResponse
using ReservationResponse.from(reservation,
seatMap.get(reservation.getSeatId())) so each seat is retrieved from the map
rather than via repeated reservationApp.getSeat calls.
| @PostMapping("/queue/tokens") | ||
| public ResponseEntity<Token> issueToken(@RequestParam String userId, | ||
| public ResponseEntity<TokenResponse> issueToken(@RequestParam String userId, | ||
| @RequestParam Long scheduleId, | ||
| HttpServletResponse response) { | ||
| Token token = reservationApp.issueToken(Long.valueOf(userId), scheduleId); | ||
| Cookie cookie = new Cookie("tokenId", token.getId()); | ||
| cookie.setHttpOnly(true); | ||
| cookie.setPath("/"); | ||
| response.addCookie(cookie); | ||
| return ResponseEntity.ok(token); | ||
| return ResponseEntity.ok(TokenResponse.from(token)); | ||
| } |
There was a problem hiding this comment.
Cookie missing security attributes.
The token cookie lacks secure and SameSite attributes, as flagged by static analysis. This could expose the token over insecure channels and leave the endpoint vulnerable to CSRF.
🔒 Proposed fix for cookie security
Token token = reservationApp.issueToken(Long.valueOf(userId), scheduleId);
Cookie cookie = new Cookie("tokenId", token.getId());
cookie.setHttpOnly(true);
+ cookie.setSecure(true); // Only transmit over HTTPS
+ cookie.setAttribute("SameSite", "Strict"); // CSRF protection
cookie.setPath("/");
response.addCookie(cookie);Note: If the application needs to support non-HTTPS in development, consider making setSecure() configurable via a profile or property.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 233-233: A cookie was detected without setting the 'secure' flag. The 'secure' flag for cookies prevents the client from transmitting the cookie over insecure channels such as HTTP. Set the 'secure' flag by calling '.setSecure(true);'.
Context: response.addCookie(cookie);
Note: [CWE-614] Sensitive Cookie in HTTPS Session Without 'Secure' Attribute. [REFERENCES]
- https://owasp.org/www-community/controls/SecureCookieAttribute
(cookie-missing-secure-flag-java)
[warning] 233-233: The application does not appear to verify inbound requests which can lead to a Cross-site request forgery (CSRF) vulnerability. If the application uses cookie-based authentication, an attacker can trick users into sending authenticated HTTP requests without their knowledge from any arbitrary domain they visit. To prevent this vulnerability start by identifying if the framework or library leveraged has built-in features or offers plugins for CSRF protection. CSRF tokens should be unique and securely random. The Synchronizer Token or Double Submit Cookie patterns with defense-in-depth mechanisms such as the sameSite cookie flag can help prevent CSRF. For more information, see: [Cross-site request forgery prevention](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Req\ uest_Forgery_Prevention_Cheat_Sheet.html).
Context: response.addCookie(cookie);
Note: [CWE-352] Cross-Site Request Forgery (CSRF). [REFERENCES]
- https://stackoverflow.com/questions/42717210/samesite-cookie-in-java-application
(cookie-missing-samesite-java)
🤖 Prompt for AI Agents
In
`@src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java`
around lines 226 - 236, The cookie set in issueToken (method issueToken, class
ConcertReservationController) lacks secure and SameSite attributes; change the
cookie handling so the token cookie is marked Secure and SameSite (preferably
Strict or Lax) and keep HttpOnly and Path. Replace the current
response.addCookie(new Cookie(...)) usage with a construction that sets secure
(cookie.setSecure(true)) and ensures SameSite is emitted (either build the
Set-Cookie header manually on the HttpServletResponse or use Spring's
ResponseCookie/Servlet API to include SameSite), and make the Secure flag
configurable via a profile/property so non-HTTPS dev remains possible; return
TokenResponse.from(token) unchanged.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @leonroars. * #37 (comment) The following files were modified: * `src/main/java/com/slam/concertreservation/interfaces/ConcertReservationController.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/ConcertResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/ConcertScheduleResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/PointHistoryResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/ReservationResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/SeatResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/TokenResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/UserPointBalanceResponse.java` * `src/main/java/com/slam/concertreservation/interfaces/dto/UserResponse.java`
1 participant
PR: API Response DTO 도입 및 ID 정밀도 이슈 해결
배경 (Background)
Number타입으로 처리 시 정밀도 유실 발생.변경 사항 (Changes)
1. Response DTO 도입 (
interfaces/dto)UserResponse,ConcertResponse등 8개 DTO 생성.String으로 명시적 변환 -> 도메인 모델과 API Spec 간 독립성 확보2. Controller 리팩토링 및 Data Aggregation
ConcertReservationController의 반환 타입을 DTO로 전면 교체.ReservationResponse에seatNumber포함 등 UI 필요 데이터 조합.문제 해결 방안 의사결정
검증 (Verification)
./gradlew test --tests DtoSerializationTest: DTO의 ID 필드가 String으로 정확히 변환되는지 검증 Pass.UnavailableRequestException없이 정상 동작 확인 / 모든 테스트 통과 확인Summary by CodeRabbit
Release Notes
Refactor
Tests