Merge Master #1
Merged
Merge Master #1
Conversation
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
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR consolidates development work into master, featuring a major authentication refactoring and package reorganization. The authentication system now uses UUID-based JWT tokens (publicId as subject instead of username), implements pessimistic locking for concurrent reservation/order operations, and standardizes API responses with the ApiResponse wrapper. The codebase underwent structural improvements by moving DTOs and repositories into dedicated subpackages (dto, repository) within each domain (movie, theater, order, user, reservation), aligning with Spring Boot layered architecture conventions. Notable additions include CORS configuration for cross-origin requests, enhanced error handling with domain-specific error statuses, and performance optimizations that replaced N+1 query patterns with batch operations. This fits into the CEOS 22nd backend study curriculum (week 4 focuses on concurrency control with locks), building upon prior weeks covering Spring fundamentals, MVC patterns, and authentication.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/entity/OrderDetail.java | 1/5 | Added unitPrice field but introduced compilation-breaking duplicate @Builder annotations and conflicting constructor signatures |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtTokenProvider.java | 2/5 | Changed JWT subject from username to publicId (UUID), breaking backward compatibility; altered validate() to throw exceptions instead of returning boolean |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/ErrorStatus.java | 3/5 | Added token/seat/movie error statuses but incorrectly uses HTTP 404 for TOKEN_EXPIRED and INVALID_TOKEN instead of 401 |
| cgvclone/src/main/java/com/ceos22/spring_boot/config/SecurityConfig.java | 3/5 | Added CORS configuration with hardcoded placeholder Vercel URL marked as unknown, posing production security risk |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/enums/PaymentMethod.java | 3/5 | Changed enum values from Korean to English constants, requiring careful data migration for existing database records |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/dto/LoginRequestDto.java | 4/5 | Added login request DTO with Swagger annotations but lacks validation constraints and exposes password example |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/dto/LoginResponseDto.java | 4/5 | Created login response DTO with JWT token and user details; no null validation for user fields |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/properties/JwtProperties.java | 4/5 | Changed accessTokenValidityInSeconds (long) to accessTokenValidity (Duration), requiring application.yml update |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/service/FavoriteMovieService.java | 4/5 | Improved error handling with domain-specific exceptions; changed remove() to throw exception instead of silent failure |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/repository/ReservationSeatRepository.java | 4/5 | Replaced single-seat check with batch query method for performance, removing existsByScreeningSeat() |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/repository/ProductRepository.java | 4/5 | Added pessimistic write lock to prevent race conditions during concurrent product purchases |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/ReservationService.java | 4/5 | Optimized seat validation from N+1 queries to single batch query but potential race condition risk remains |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/user/UserRepository.java | 4/5 | Added findByPublicId(UUID) method to support UUID-based user lookups for JWT authentication |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/OrderController.java | 4/5 | Removed redundant null check, now relying on Spring Security for authentication enforcement |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/service/MovieService.java | 4/5 | Updated imports for package restructuring; getMovieById returns null instead of throwing exception |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/controller/ScreeningController.java | 4/5 | Updated MovieRepository import path; contains validation logic that should be in service layer |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/controller/AuthController.java | 4/5 | Migrated to standardized ApiResponse format; potential redundant DB query and double JWT parsing |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/repository/ReservationRepository.java | 4/5 | Added unused imports (ScreeningSeat, @Query, @Param) suggesting incomplete implementation |
| cgvclone/src/test/java/com/ceos22/spring_boot/service/MovieServiceTest.java | 4/5 | Updated imports for package restructure; uses fragile reflection-based test helper method |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/OrderService.java | 5/5 | Switched to Spring Transaction, added pessimistic locking for concurrency control, refactored to builder pattern |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtAuthenticationFilter.java | 5/5 | Added debug log guards and blank token validation to improve performance and security |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/exception/GlobalExceptionHandler.java | 5/5 | Moved generic Exception handler to bottom for proper handler precedence in Spring's resolution |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtAuthEntryPoint.java | 5/5 | Refactored to use dependency injection and improved variable naming for better readability |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/controller/FavoriteMovieController.java | 5/5 | Changed DELETE endpoint from 204 No Content to 200 OK for consistency across endpoints |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/repository/FavoriteMovieRepository.java | 5/5 | Relocated to repository subpackage following Spring Boot layered architecture conventions |
| cgvclone/src/main/resources/application.yml | 5/5 | Changed JWT token validity from 1800 seconds to 1h duration format, doubling expiration time |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/authentication/SaltAuthenticationProvider.java | 5/5 | Renamed variable from u to user for improved code readability |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/ApiResponse.java | 5/5 | Added Swagger/OpenAPI schema annotations for enhanced API documentation |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/user/UserService.java | 5/5 | Added findByUsername method with consistent error handling for authentication flows |
| README.md | 5/5 | Added week 4 documentation link covering concurrency problem resolution with locks |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/repository/MovieRepository.java | 5/5 | Relocated to repository subpackage to improve code organization |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/service/ScreeningService.java | 5/5 | Fixed import path for MovieRepository to match new repository package structure |
| cgvclone/build.gradle | 5/5 | Removed informal Korean comment above JWT dependencies |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/SuccessStatus.java | 5/5 | Added LOGIN_SUCCESS enum constant for specific login operation messaging |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/controller/MovieController.java | 5/5 | Updated MovieDto import path to reflect dto subdirectory restructuring |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/dto/MovieDto.java | 5/5 | Relocated to dto subpackage following Spring Boot package-by-feature layering |
Confidence score: 2/5
- This PR contains critical compilation-breaking issues and backward compatibility problems that will cause immediate failures if merged
- Score reflects three major blockers: (1) OrderDetail.java has duplicate
@Builderannotations causing compilation failure, (2) JwtTokenProvider.java breaks backward compatibility by changing JWT subject format without migration strategy and changes validate() method contract, (3) ErrorStatus uses incorrect HTTP status codes for authentication errors (404instead of 401) - Pay close attention to cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/entity/OrderDetail.java, cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtTokenProvider.java, and cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/ErrorStatus.java
Sequence Diagram
sequenceDiagram
participant User
participant AuthController
participant AuthenticationManager
participant SaltAuthenticationProvider
participant UserRepository
participant PasswordSaltService
participant JwtTokenProvider
participant UserService
User->>AuthController: "POST /auth/login (username, password)"
AuthController->>AuthenticationManager: "authenticate(UsernamePasswordAuthenticationToken)"
AuthenticationManager->>SaltAuthenticationProvider: "authenticate(authentication)"
SaltAuthenticationProvider->>UserRepository: "findByUsername(username)"
UserRepository-->>SaltAuthenticationProvider: "User entity"
SaltAuthenticationProvider->>PasswordSaltService: "matches(rawPassword, salt, hash)"
PasswordSaltService-->>SaltAuthenticationProvider: "boolean result"
alt Password matches
SaltAuthenticationProvider-->>AuthenticationManager: "UsernamePasswordAuthenticationToken with CustomUserPrincipal"
AuthenticationManager-->>AuthController: "Authentication"
AuthController->>JwtTokenProvider: "issueToken(authentication)"
JwtTokenProvider-->>AuthController: "JWT token"
AuthController->>UserService: "findByUsername(username)"
UserService->>UserRepository: "findByUsername(username)"
UserRepository-->>UserService: "User entity"
UserService-->>AuthController: "User"
AuthController->>JwtTokenProvider: "getExpiration(token)"
JwtTokenProvider-->>AuthController: "expiresIn"
AuthController-->>User: "LoginResponseDto (token, user info, expiresIn)"
else Password mismatch
SaltAuthenticationProvider-->>AuthenticationManager: "AuthFailureException"
AuthenticationManager-->>AuthController: "AuthFailureException"
AuthController-->>User: "Error response"
end
User->>AuthController: "POST /auth/register (username, password, name, email)"
AuthController->>UserService: "register(username, password, name, email)"
UserService->>UserRepository: "existsByUsername(username)"
UserRepository-->>UserService: "boolean"
UserService->>UserRepository: "existsByEmail(email)"
UserRepository-->>UserService: "boolean"
alt No duplicates
UserService->>PasswordSaltService: "generateSaltBase64()"
PasswordSaltService-->>UserService: "salt"
UserService->>PasswordSaltService: "hashBase64(rawPassword, salt)"
PasswordSaltService-->>UserService: "hash"
UserService->>UserRepository: "save(User)"
UserRepository-->>UserService: "User entity"
UserService-->>AuthController: "User"
AuthController-->>User: "RegisterResponse (userId)"
else Duplicate found
UserService-->>AuthController: "GeneralException"
AuthController-->>User: "Error response"
end
User->>OrderController: "POST /store/purchase (items, paymentMethod)"
OrderController->>OrderService: "purchase(userId, request)"
OrderService->>UserRepository: "findById(userId)"
UserRepository-->>OrderService: "User entity"
OrderService->>UserOrderRepository: "save(UserOrder PENDING)"
UserOrderRepository-->>OrderService: "UserOrder"
loop For each item
OrderService->>ProductRepository: "findByIdWithLock(productId)"
ProductRepository-->>OrderService: "Product entity"
OrderService->>Product: "decreaseStock(quantity)"
OrderService->>OrderDetailRepository: "save(OrderDetail)"
OrderDetailRepository-->>OrderService: "OrderDetail"
end
OrderService->>PaymentRepository: "save(Payment SUCCESS)"
PaymentRepository-->>OrderService: "Payment"
OrderService->>UserOrder: "markReserved(totalAmount)"
OrderService-->>OrderController: "PurchaseResponseDto"
OrderController-->>User: "ApiResponse<PurchaseResponseDto>"
User->>ReservationController: "POST /reservations (screeningId, seatIds)"
ReservationController->>ReservationService: "createReservation(userId, request)"
ReservationService->>UserRepository: "findById(userId)"
UserRepository-->>ReservationService: "User entity"
ReservationService->>ScreeningRepository: "findById(screeningId)"
ScreeningRepository-->>ReservationService: "Screening entity"
ReservationService->>ScreeningSeatRepository: "findAllById(seatIds)"
ScreeningSeatRepository-->>ReservationService: "List<ScreeningSeat>"
ReservationService->>ReservationSeatRepository: "findReservedSeatIds(seats)"
ReservationSeatRepository-->>ReservationService: "List<Long> reservedIds"
alt Seats available
ReservationService->>ReservationRepository: "save(Reservation PENDING)"
ReservationRepository-->>ReservationService: "Reservation"
loop For each seat
ReservationService->>ReservationSeatRepository: "save(ReservationSeat)"
ReservationSeatRepository-->>ReservationService: "ReservationSeat"
end
ReservationService-->>ReservationController: "ReservationResponse"
ReservationController-->>User: "ApiResponse<ReservationResponse>"
else Seats already reserved
ReservationService-->>ReservationController: "GeneralException"
ReservationController-->>User: "Error response"
end
User->>ScreeningController: "POST /screenings (movieId, screenId, startTime, endTime)"
ScreeningController->>ScreeningService: "createScreening(request)"
ScreeningService->>MovieRepository: "findById(movieId)"
MovieRepository-->>ScreeningService: "Movie entity"
ScreeningService->>ScreenRepository: "findById(screenId)"
ScreenRepository-->>ScreeningService: "Screen entity"
ScreeningService->>ScreeningRepository: "save(Screening)"
ScreeningRepository-->>ScreeningService: "Screening entity"
ScreeningService->>SeatRepository: "findByScreen(screen)"
SeatRepository-->>ScreeningService: "List<Seat>"
loop For each seat
ScreeningService->>ScreeningSeatRepository: "saveAll(ScreeningSeat)"
end
ScreeningSeatRepository-->>ScreeningService: "List<ScreeningSeat>"
ScreeningService-->>ScreeningController: "Screening"
ScreeningController-->>User: "ApiResponse<ScreeningResponseDto>"
36 files reviewed, 20 comments
cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/dto/LoginRequestDto.java
Show resolved
Hide resolved
cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtAuthEntryPoint.java
Outdated
Show resolved
Hide resolved
| if (me == null) { | ||
| return ApiResponse.onFailure(ErrorStatus._UNAUTHORIZED); | ||
| } | ||
| PurchaseResponseDto res = orderService.purchase(me.getUserId(), req); |
There was a problem hiding this comment.
logic: Potential NullPointerException if me is null. @AuthenticationPrincipal can return null when authentication fails or user is anonymous. The removed null check on lines 38-40 of the old code prevented this crash. Is there a global security filter that guarantees authentication for this endpoint?
Prompt To Fix With AI
This is a comment left during a code review.
Path: cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/OrderController.java
Line: 38:38
Comment:
**logic:** Potential NullPointerException if `me` is null. `@AuthenticationPrincipal` can return null when authentication fails or user is anonymous. The removed null check on lines 38-40 of the old code prevented this crash. Is there a global security filter that guarantees authentication for this endpoint?
How can I resolve this? If you propose a fix, please make it concise.
cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/repository/ProductRepository.java
Outdated
Show resolved
Hide resolved
cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/ErrorStatus.java
Outdated
Show resolved
Hide resolved
Comment on lines
30
to
37
| @Builder | ||
| public OrderDetail(UserOrder userOrder, Product product, int quantity, int unitPrice, int subtotal) { | ||
| this.userOrder = userOrder; | ||
| this.product = product; | ||
| this.quantity = quantity; | ||
| this.unitPrice = unitPrice; | ||
| this.subtotal = subtotal; | ||
| } |
There was a problem hiding this comment.
style: Constructor uses primitive int parameters while the fields are Integer wrapper types. Use Integer for consistency and to handle null values properly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/entity/OrderDetail.java
Line: 30:37
Comment:
**style:** Constructor uses primitive `int` parameters while the fields are `Integer` wrapper types. Use `Integer` for consistency and to handle null values properly.
How can I resolve this? If you propose a fix, please make it concise.
cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/entity/OrderDetail.java
Outdated
Show resolved
Hide resolved
cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtTokenProvider.java
Outdated
Show resolved
Hide resolved
Comment on lines
+70
to
+71
| User u = users.findByPublicId(publicId) | ||
| .orElseThrow(() -> new GeneralException(ErrorStatus._BAD_REQUEST, "사용자를 찾을 수 없습니다: " + publicId)); |
There was a problem hiding this comment.
style: Database lookup on every request for token validation can impact performance. Consider caching user data or only fetching when necessary.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtTokenProvider.java
Line: 70:71
Comment:
**style:** Database lookup on every request for token validation can impact performance. Consider caching user data or only fetching when necessary.
How can I resolve this? If you propose a fix, please make it concise.
cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/service/FavoriteMovieService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR merges the dev branch into master, consolidating week 4 work focused on concurrency control, JWT authentication refactoring, and architectural improvements. Key changes include:
-
Package Reorganization: DTOs, repositories, and entities are now separated into dedicated subpackages (
dto/,repository/,entity/) within each domain module, following Spring Boot conventions and improving codebase maintainability. -
JWT Authentication Overhaul: Token subjects changed from usernames to UUIDs (
publicId) for better security and privacy. JwtProperties now usesDurationinstead oflongfor type-safe time configuration. Error handling improved with specific status codes (TOKEN_EXPIRED,INVALID_TOKEN). -
Concurrency & Performance: Pessimistic locking added to
ProductRepository.findByIdWithLock()to prevent race conditions during purchases. Seat reservation validation refactored from N+1 queries to batch processing viafindReservedSeatIds(). -
API Standardization: Login endpoint now uses dedicated DTOs (
LoginRequestDto,LoginResponseDto) and the standardizedApiResponsewrapper. NewLOGIN_SUCCESSstatus added. CORS configuration added to support frontend clients on localhost:3000 and Vercel. -
Code Quality: Dependency injection improvements (ObjectMapper in JwtAuthEntryPoint), logging optimizations (debug level with conditional checks), enum constant renaming from Korean to English (
PaymentMethod), and exception handler ordering fixes.
The changes integrate authentication features from week 3 curriculum and implement database locking mechanisms for week 4's concurrency focus.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/entity/OrderDetail.java | 1/5 | Adds unitPrice field but introduces duplicate @Builder annotations causing compilation issues; price field never initialized |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/OrderController.java | 2/5 | Removes null check for authenticated principal, risking NullPointerException if SecurityConfig doesn't enforce authentication |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtTokenProvider.java | 3/5 | Refactors to UUID-based subjects and exception-throwing validation; lacks error handling for UUID parsing and database lookups |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/ErrorStatus.java | 3/5 | Adds new error statuses but uses incorrect HTTP 404 for token errors instead of 401 UNAUTHORIZED |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/repository/ReservationSeatRepository.java | 3/5 | Replaces single-seat existence check with batch query, shifting concurrency responsibility to service layer |
| cgvclone/src/main/java/com/ceos22/spring_boot/config/SecurityConfig.java | 4/5 | Adds CORS configuration with hardcoded production URL marked as uncertain ("아직 모름") |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/controller/AuthController.java | 4/5 | Refactors login with dedicated DTOs; fetches user twice (once for auth, once for response details) |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/dto/LoginResponseDto.java | 4/5 | New login response DTO with factory method; no null validation on User parameter could expose null fields |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/properties/JwtProperties.java | 4/5 | Breaking change from long to Duration for token validity; improves type safety but requires coordination across config/code |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/enums/PaymentMethod.java | 4/5 | Changes enum constants from Korean to English; breaking change requiring data migration for persisted values |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/service/FavoriteMovieService.java | 4/5 | Switches to Spring transactions and domain exceptions; remove() now throws error instead of silent failure (breaking) |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/OrderService.java | 4/5 | Adds pessimistic locking for products; requires verification that findByIdWithLock() properly implements database locking |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/repository/ProductRepository.java | 4/5 | Adds PESSIMISTIC_WRITE locking for concurrency control; no explicit timeout configuration risks deadlocks |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/ReservationService.java | 4/5 | Optimizes seat validation from N+1 to batch query; uses more specific error status for reserved seats |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/controller/ScreeningController.java | 4/5 | Updates import for repository restructuring; controller directly uses repositories violating layered architecture |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtAuthEntryPoint.java | 4/5 | Refactors to use DI for ObjectMapper; unused message variable remains on line 26 |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/repository/ReservationRepository.java | 4.5/5 | Adds unused imports (ScreeningSeat, @Query, @Param) with no corresponding query methods |
| cgvclone/src/test/java/com/ceos22/spring_boot/service/MovieServiceTest.java | 4/5 | Updates imports for package restructuring; uses brittle reflection to set private fields in tests |
| cgvclone/src/main/resources/application.yml | 4/5 | Changes JWT validity from integer seconds to Duration string format; requires JwtProperties update |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/dto/LoginRequestDto.java | 5/5 | New login request DTO using record syntax with Swagger annotations; previous reviews noted missing validation |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/authentication/SaltAuthenticationProvider.java | 5/5 | Renames variable from u to user for better readability; no functional changes |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/jwt/JwtAuthenticationFilter.java | 5/5 | Optimizes logging to debug level with conditional check; adds blank token validation for early rejection |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/exception/GlobalExceptionHandler.java | 5/5 | Moves generic Exception handler to end ensuring specific handlers evaluated first; follows Spring best practices |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/ApiResponse.java | 5/5 | Adds comprehensive Swagger annotations for API documentation; purely additive with no logic changes |
| cgvclone/src/main/java/com/ceos22/spring_boot/common/response/status/SuccessStatus.java | 5/5 | Adds LOGIN_SUCCESS enum constant for dedicated login success status; aligns with auth work |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/controller/FavoriteMovieController.java | 5/5 | Changes DELETE endpoint from 204 No Content to 200 OK for consistency across favorite operations |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/controller/MovieController.java | 5/5 | Updates import statement for MovieDto package restructuring; no functional changes |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/dto/MovieDto.java | 5/5 | Relocates from domain root to dto subpackage; improves package organization following Spring conventions |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/repository/FavoriteMovieRepository.java | 5/5 | Moves repository to dedicated repository subpackage; no functional changes, improves architecture |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/repository/MovieRepository.java | 5/5 | Relocates repository to dedicated subpackage following Spring Boot conventions; safe structural change |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/movie/service/MovieService.java | 5/5 | Updates imports for package restructuring (MovieDto, MovieRepository); no service logic changes |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/service/ScreeningService.java | 5/5 | Updates import for MovieRepository package restructuring; logic unchanged, hardcoded prices remain |
| cgvclone/src/main/java/com/ceos22/spring_boot/domain/user/UserRepository.java | 5/5 | Adds findByPublicId(UUID) query method for JWT authentication with UUIDs; follows Spring Data conventions |
| README.md | 5/5 | Adds week 4 learning materials link about concurrency control; documentation-only change |
| cgvclone/build.gradle | 5/5 | Removes Korean comment questioning JWT dependency versions; documentation cleanup |
Confidence score: 2/5
- This PR contains critical issues that will likely cause immediate compilation failures and runtime crashes in production.
- Score reflects multiple blocking issues: (1) OrderDetail.java has duplicate
@Builderannotations causing compilation errors, uninitialized price field risking NPEs; (2) OrderController.java removes null check exposing NPE risk if SecurityConfig authentication not properly configured; (3) ErrorStatus uses wrong HTTP status for auth errors (404 instead of 401); (4) JwtTokenProvider lacks error handling for UUID parsing and user lookups; (5) ReservationSeatRepository API change requires careful transaction/locking coordination. Additionally, SecurityConfig has hardcoded production URL marked as uncertain, PaymentMethod enum rename is breaking without migration, and JwtProperties Duration change requires coordinated updates. - Pay extremely close attention to OrderDetail.java (duplicate builders, uninitialized fields), OrderController.java (removed null check), JwtTokenProvider.java (missing error handlers), and SecurityConfig.java (incorrect production URL). Verify all authentication endpoints are protected in SecurityConfig and that database locking is properly tested for concurrency scenarios.
Sequence Diagram
sequenceDiagram
participant User
participant AuthController
participant AuthenticationManager
participant SaltAuthenticationProvider
participant UserRepository
participant PasswordSaltService
participant JwtTokenProvider
participant UserService
User->>AuthController: "POST /auth/login {username, password}"
AuthController->>AuthenticationManager: "authenticate(UsernamePasswordAuthenticationToken)"
AuthenticationManager->>SaltAuthenticationProvider: "authenticate(authentication)"
SaltAuthenticationProvider->>UserRepository: "findByUsername(username)"
UserRepository-->>SaltAuthenticationProvider: "User entity"
SaltAuthenticationProvider->>PasswordSaltService: "matches(rawPassword, salt, passwordHash)"
PasswordSaltService-->>SaltAuthenticationProvider: "boolean (password valid)"
SaltAuthenticationProvider-->>AuthenticationManager: "UsernamePasswordAuthenticationToken (authenticated)"
AuthenticationManager-->>AuthController: "Authentication object"
AuthController->>JwtTokenProvider: "issueToken(auth)"
JwtTokenProvider-->>AuthController: "JWT token string"
AuthController->>UserService: "findByUsername(username)"
UserService->>UserRepository: "findByUsername(username)"
UserRepository-->>UserService: "User entity"
UserService-->>AuthController: "User object"
AuthController-->>User: "LoginResponseDto {accessToken, userId, username, email, expiresIn}"
Additional Comments (6)
-
cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/security/authentication/SaltAuthenticationProvider.java, line 23 (link)style: Constructor parameters on same line - split across lines for better readability
-
cgvclone/src/test/java/com/ceos22/spring_boot/service/MovieServiceTest.java, line 96-114 (link)style: Reflection to set private fields in tests is brittle and breaks encapsulation. Use a test builder or expose a package-private constructor/builder in the Movie entity for testing
-
cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/service/ScreeningService.java, line 57 (link)style: Hardcoded ticket prices (15000, 13000) should be extracted to configuration or constants to simplify future price changes and support internationalization
-
cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/controller/ScreeningController.java, line 32-34 (link)style: Controller directly injects repositories (MovieRepository, ScreeningRepository, ScreenRepository) but only uses MovieRepository and ScreenRepository. Business logic (entity fetching) should be in ScreeningService, not the controller. ScreeningRepository injection is unused - remove it.
-
cgvclone/src/main/java/com/ceos22/spring_boot/domain/theater/controller/ScreeningController.java, line 40-48 (link)style: Repository access and validation logic belongs in the service layer, not the controller. Move movie/screen existence checks to ScreeningService.createScreening() to maintain separation of concerns and enable proper transaction management.
-
cgvclone/src/main/java/com/ceos22/spring_boot/common/auth/controller/AuthController.java, line 53-56 (link)style: Register endpoint doesn't use the new unified
ApiResponsewrapper while login does. Consider wrapping this response inApiResponse.onSuccess()for consistency
35 files reviewed, 8 comments
| throw new GeneralException(ErrorStatus.SEAT_ALREADY_RESERVED, | ||
| "이미 예약된 좌석이 포함되어 있습니다: " + reservedSeatIds); | ||
| } | ||
|
|
There was a problem hiding this comment.
style: extra blank line
Prompt To Fix With AI
This is a comment left during a code review.
Path: cgvclone/src/main/java/com/ceos22/spring_boot/domain/reservation/ReservationService.java
Line: 71:71
Comment:
**style:** extra blank line
How can I resolve this? If you propose a fix, please make it concise.| public interface ProductRepository extends JpaRepository<Product, Long> { | ||
| @Lock(LockModeType.PESSIMISTIC_WRITE) | ||
| @Query("SELECT p FROM Product p WHERE p.productId = :id") | ||
| Optional<Product> findByIdWithLock(@Param("id") Long id); |
There was a problem hiding this comment.
style: The custom JPQL query duplicates Spring Data JPA's built-in findById() functionality. Unless there's a specific need for the custom query, consider using the derived method name pattern: rename to findWithLockById(Long id) and remove the @Query annotation - Spring Data JPA will automatically apply the lock. Is there a specific reason to use JPQL here rather than relying on Spring Data JPA's query derivation with the @lock annotation?
Prompt To Fix With AI
This is a comment left during a code review.
Path: cgvclone/src/main/java/com/ceos22/spring_boot/domain/order/repository/ProductRepository.java
Line: 15:15
Comment:
**style:** The custom JPQL query duplicates Spring Data JPA's built-in `findById()` functionality. Unless there's a specific need for the custom query, consider using the derived method name pattern: rename to `findWithLockById(Long id)` and remove the `@Query` annotation - Spring Data JPA will automatically apply the lock. Is there a specific reason to use JPQL here rather than relying on Spring Data JPA's query derivation with the @Lock annotation?
How can I resolve this? If you propose a fix, please make it concise.
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.
No description provided.