Conversation
✅ 테스트 결과 for PRBuild: success 🧪 테스트 실행 with Gradle |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive ranking API system for auction items, along with CRUD operations for item options and code formatting improvements across the codebase.
Changes:
- Implemented a ranking API with 12 endpoints covering price rankings, volume rankings, price/volume change rankings, category-specific rankings, and all-time records
- Added full CRUD operations (create, update, delete) for ItemOptionInfo entity
- Standardized import statement ordering across 100+ Java files to follow Java conventions (static imports first, then java.*, then third-party libraries)
Reviewed changes
Copilot reviewed 156 out of 156 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| RankingRepository.java | New repository with 12 native SQL queries for various ranking calculations using parameterized queries |
| RankingMapper.java | Mapper component to convert Object[] results from native queries to typed response DTOs with null-safe type conversions |
| Price/Volume/Category/AllTimeRankingService.java | Service layer classes delegating to repository and mapper with read-only transactions |
| Price/Volume/Category/AllTimeRankingController.java | REST controllers exposing ranking endpoints with proper validation |
| RankingConstants.java | Constants class defining default, min, and max limit values for ranking queries |
| RankingSearchRequest.java | Request DTO with validation constraints for limit parameter |
| CategoryRankingSearchRequest.java | Request DTO with validation for category-specific ranking queries |
| *RankingResponse.java | Response DTOs for different ranking types with Swagger documentation |
| ItemOptionInfoService.java | Added create, update, and delete methods to existing service |
| ItemOptionInfoController.java | Added POST, PUT, and DELETE endpoints for CRUD operations |
| ItemOptionInfoRequest.java | New request DTO for create/update/delete operations |
| ItemOptionInfo.java | Added constructor and update method |
| ItemOptionInfoRepositoryPort/Impl | Added repository methods for findById, save, delete, existsById |
| ItemOptionInfoMapper.java | Added mapping methods to convert between request DTO and entity |
| 100+ other files | Import statement reordering following Java conventions (static imports first, java.*, then others) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public ItemOptionInfo create(ItemOptionInfo itemOptionInfo) { | ||
| if (itemOptionInfoRepositoryPort.existsById(itemOptionInfo.getId())) { | ||
| throw new IllegalArgumentException("이미 존재하는 아이템 옵션 정보입니다."); | ||
| } | ||
| return itemOptionInfoRepositoryPort.save(itemOptionInfo); |
There was a problem hiding this comment.
The create method has a race condition vulnerability. Between checking existence with existsById and calling save, another thread could insert the same record, leading to a duplicate key constraint violation. Consider using a try-catch block to handle DataIntegrityViolationException instead, or use a unique constraint with proper exception handling.
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional(readOnly = true) | ||
| public class PriceRankingService { | ||
|
|
||
| private final RankingRepository rankingRepository; | ||
| private final RankingMapper rankingMapper; | ||
|
|
||
| /** 오늘의 최고가 거래 TOP N (API 1) */ | ||
| public List<PriceRankingResponse> getTodayHighestPrice(int limit) { | ||
| List<Object[]> results = rankingRepository.findTodayHighestPrice(limit); | ||
| return rankingMapper.toPriceRankingResponses(results); | ||
| } | ||
|
|
||
| /** 이번 주 최고가 아이템 TOP N (API 2) */ | ||
| public List<PriceRankingResponse> getWeekHighestPrice(int limit) { | ||
| List<Object[]> results = rankingRepository.findWeekHighestPrice(limit); | ||
| return rankingMapper.toPriceRankingResponses(results); | ||
| } | ||
|
|
||
| /** 오늘의 최대 거래액 TOP N (API 3) */ | ||
| public List<PriceRankingResponse> getTodayLargestVolume(int limit) { | ||
| List<Object[]> results = rankingRepository.findTodayLargestVolume(limit); | ||
| return rankingMapper.toPriceRankingResponses(results); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ranking feature lacks test coverage. The codebase has comprehensive test coverage for similar services (e.g., ItemInfoServiceTest). Consider adding tests for RankingRepository queries, service methods, and controller endpoints to maintain consistent test coverage across the codebase.
| @Transactional | ||
| public ItemOptionInfo create(ItemOptionInfo itemOptionInfo) { | ||
| if (itemOptionInfoRepositoryPort.existsById(itemOptionInfo.getId())) { | ||
| throw new IllegalArgumentException("이미 존재하는 아이템 옵션 정보입니다."); | ||
| } | ||
| return itemOptionInfoRepositoryPort.save(itemOptionInfo); | ||
| } | ||
|
|
||
| @Transactional | ||
| public ItemOptionInfo update(ItemOptionInfoId id, String optionDesc) { | ||
| ItemOptionInfo itemOptionInfo = findById(id); | ||
| itemOptionInfo.updateOptionDesc(optionDesc); | ||
| return itemOptionInfoRepositoryPort.save(itemOptionInfo); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void delete(ItemOptionInfoId id) { | ||
| ItemOptionInfo itemOptionInfo = findById(id); | ||
| itemOptionInfoRepositoryPort.delete(itemOptionInfo); | ||
| } |
There was a problem hiding this comment.
The new CRUD operations for ItemOptionInfo lack test coverage. The codebase has comprehensive test coverage for similar services. Consider adding tests for create, update, and delete operations, especially edge cases like duplicate creation attempts, updating non-existent records, and deleting non-existent records.
| @DeleteMapping | ||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| @Operation( | ||
| summary = "아이템 옵션 정보 삭제", | ||
| description = | ||
| "아이템 옵션 정보를 삭제합니다. 복합 키(optionType, optionSubType, optionValue, optionValue2)로 식별합니다.") | ||
| public void delete(@Valid @RequestBody ItemOptionInfoRequest request) { | ||
| ItemOptionInfoId id = itemOptionInfoMapper.toId(request); | ||
| itemOptionInfoService.delete(id); | ||
| } |
There was a problem hiding this comment.
Using @RequestBody with @DeleteMapping is technically allowed but not RESTful best practice. DELETE requests typically should not have a request body. Consider using path variables or query parameters for the composite key instead. For example: @DeleteMapping("/{optionType}/{optionSubType}/{optionValue}") or use POST with a descriptive path like /delete if you must use a request body.
| if (value instanceof Number) { | ||
| return ((Number) value).longValue(); | ||
| } | ||
| return Long.parseLong(value.toString()); |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
1 participant
📋 상세 설명
📊 체크리스트
추후에 업데이트라벨만 등록