Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements an admin API for updating detailed spot information, allowing administrators to modify various aspects of a spot including basic details, features, operating hours, menus, and images.
- Adds comprehensive spot detail update functionality with validation
- Implements domain model updates for mutable spot and opening hour fields
- Enhances database constraints to support spot updates while preventing duplicates
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| SpotRepository.java | Adds method to check for duplicate spots excluding current spot ID |
| SpotEntity.java | Removes unique constraint on name+address to allow updates |
| Spot.java | Converts immutable fields to mutable with update methods |
| OpeningHour.java | Converts immutable fields to mutable with update methods |
| AdminService.java | Implements comprehensive updateSpotDetail method with validation |
| UpdateSpotDetailRequest.java | Defines request structure with validation for spot updates |
| AdminController.java | Adds PATCH endpoint for spot detail updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| SpotType originalSpotType = spot.getSpotType(); | ||
|
|
||
| // 2. 동일한 장소명과 주소를 가진 활성화된 장소가 있는지 확인 (자기 자신 제외) | ||
| if ((request.spotName() != null || request.address() != null) && spot.getSpotStatus() == SpotStatus.ACTIVE) { |
There was a problem hiding this comment.
The condition is checking for ACTIVE spots, but the variable spot.getSpotStatus() might not reflect the current status if the spot entity was modified earlier. Consider using spotEntity.getSpotStatus() instead to ensure you're checking the actual database state.
| if ((request.spotName() != null || request.address() != null) && spot.getSpotStatus() == SpotStatus.ACTIVE) { | |
| if ((request.spotName() != null || request.address() != null) && spotEntity.getSpotStatus() == SpotStatus.ACTIVE) { |
| List<SpotOptionEntity> existingFeatures = spotOptionRepository.findAllBySpotId(spotId).stream() | ||
| .filter(so -> { | ||
| OptionEntity option = optionRepository.findById(so.getOptionId()) | ||
| .orElse(null); | ||
|
|
There was a problem hiding this comment.
This code performs N+1 queries by calling optionRepository.findById() for each spot option. Consider fetching all options in a single query and creating a lookup map, or add a repository method to find spot options by spot ID and category ID directly.
| List<SpotOptionEntity> existingFeatures = spotOptionRepository.findAllBySpotId(spotId).stream() | |
| .filter(so -> { | |
| OptionEntity option = optionRepository.findById(so.getOptionId()) | |
| .orElse(null); | |
| List<SpotOptionEntity> spotOptions = spotOptionRepository.findAllBySpotId(spotId); | |
| List<Long> optionIds = spotOptions.stream() | |
| .map(SpotOptionEntity::getOptionId) | |
| .toList(); | |
| List<OptionEntity> optionEntities = optionRepository.findAllById(optionIds); | |
| // Build a lookup map from optionId to OptionEntity | |
| java.util.Map<Long, OptionEntity> optionEntityMap = new java.util.HashMap<>(); | |
| for (OptionEntity optionEntity : optionEntities) { | |
| optionEntityMap.put(optionEntity.getId(), optionEntity); | |
| } | |
| List<SpotOptionEntity> existingFeatures = spotOptions.stream() | |
| .filter(so -> { | |
| OptionEntity option = optionEntityMap.get(so.getOptionId()); |
| List<SpotOptionEntity> existingPrice = spotOptionRepository.findAllBySpotId(spotId).stream() | ||
| .filter(so -> { | ||
| OptionEntity option = optionRepository.findById(so.getOptionId()) | ||
| .orElse(null); | ||
|
|
||
| return option != null && option.getCategoryId().equals(priceCategoryId); | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
This code also performs N+1 queries similar to the feature filtering above. The same optimization recommendation applies here - consider batching the option queries or creating a more efficient repository method.
| finalImageUrls.add(imageUrl); | ||
| } else { | ||
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | ||
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); |
There was a problem hiding this comment.
Using substring() with lastIndexOf() could throw an exception if the URL doesn't contain a '/' character. Consider adding validation or using a more robust URL parsing approach, such as checking if lastIndexOf() returns -1.
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| int lastSlashIdx = imageUrl.lastIndexOf("/"); | |
| String fileName; | |
| if (lastSlashIdx == -1) { | |
| fileName = imageUrl; // fallback: use whole string | |
| // Optionally, log a warning here if desired | |
| } else { | |
| fileName = imageUrl.substring(lastSlashIdx + 1); | |
| } |
| finalImageUrls.add(imageUrl); | ||
| } else { | ||
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | ||
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); |
There was a problem hiding this comment.
Same issue as above - using substring() with lastIndexOf() without checking if the '/' character exists could cause an exception. Consider adding validation or using a more robust URL parsing method.
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| String fileName; | |
| if (imageUrl != null && !imageUrl.isEmpty()) { | |
| int lastSlash = imageUrl.lastIndexOf("/"); | |
| if (lastSlash != -1 && lastSlash < imageUrl.length() - 1) { | |
| fileName = imageUrl.substring(lastSlash + 1); | |
| } else { | |
| fileName = imageUrl; | |
| } | |
| } else { | |
| fileName = ""; | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for (String imageUrl : request.menuboardImageList()) { | ||
| s3Adapter.validateImageExists(imageUrl); | ||
| } | ||
| } | ||
|
|
||
| if (request.spotImageList() != null) { | ||
| for (String imageUrl : request.spotImageList()) { | ||
| s3Adapter.validateImageExists(imageUrl); | ||
| } |
There was a problem hiding this comment.
The nested loops for image validation could be optimized by using parallel streams or batch validation methods to reduce the number of sequential S3 calls, especially when dealing with multiple images.
| for (String imageUrl : request.menuboardImageList()) { | |
| s3Adapter.validateImageExists(imageUrl); | |
| } | |
| } | |
| if (request.spotImageList() != null) { | |
| for (String imageUrl : request.spotImageList()) { | |
| s3Adapter.validateImageExists(imageUrl); | |
| } | |
| request.menuboardImageList().parallelStream() | |
| .forEach(s3Adapter::validateImageExists); | |
| } | |
| if (request.spotImageList() != null) { | |
| request.spotImageList().parallelStream() | |
| .forEach(s3Adapter::validateImageExists); |
| List<SpotOptionEntity> existingOptions = spotOptionRepository.findAllBySpotId(spotId); | ||
| spotOptionRepository.deleteAll(existingOptions); |
There was a problem hiding this comment.
This approach fetches all existing options just to delete them. Consider using a direct delete query like deleteAllBySpotId(spotId) to avoid the unnecessary SELECT operation.
| List<SpotOptionEntity> existingOptions = spotOptionRepository.findAllBySpotId(spotId); | |
| spotOptionRepository.deleteAll(existingOptions); | |
| spotOptionRepository.deleteAllBySpotId(spotId); |
| List<MenuEntity> existingMenus = menuRepository.findAllBySpotId(spotId); | ||
| menuRepository.deleteAll(existingMenus); |
There was a problem hiding this comment.
Similar to the spot options deletion, this fetches entities before deletion. Consider implementing deleteAllBySpotId(spotId) method in MenuRepository for better performance.
| List<MenuEntity> existingMenus = menuRepository.findAllBySpotId(spotId); | |
| menuRepository.deleteAll(existingMenus); | |
| menuRepository.deleteAllBySpotId(spotId); |
| if (!Boolean.TRUE.equals(closed)) { | ||
| // 휴무일이 아닌 경우 startTime과 endTime 필수 검증 | ||
| if (startTime == null || endTime == null) { | ||
| throw new IllegalArgumentException("휴무일이 아닌 경우 영업 시작/종료 시간은 필수입니다."); |
There was a problem hiding this comment.
The error message is in Korean while the codebase appears to use English for error messages. Consider using English for consistency: 'Start time and end time are required when not closed.'
| throw new IllegalArgumentException("휴무일이 아닌 경우 영업 시작/종료 시간은 필수입니다."); | |
| throw new IllegalArgumentException("Start time and end time are required when not closed."); |
| // break time 검증 (둘 다 있거나 둘 다 없어야 함) | ||
| if ((breakStartTime == null && breakEndTime != null) || | ||
| (breakStartTime != null && breakEndTime == null)) { | ||
| throw new IllegalArgumentException("브레이크 타임은 시작과 종료 시간이 모두 있거나 모두 없어야 합니다."); |
There was a problem hiding this comment.
The error message is in Korean while other validation messages appear to be in English. Consider using English for consistency: 'Break time must have both start and end time, or neither.'
| throw new IllegalArgumentException("브레이크 타임은 시작과 종료 시간이 모두 있거나 모두 없어야 합니다."); | |
| throw new IllegalArgumentException("Break time must have both start and end time, or neither."); |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @Modifying | ||
| @Query(value = """ | ||
| DELETE FROM spot_option | ||
| WHERE spot_id = :spotId | ||
| AND option_id IN ( | ||
| SELECT o.id | ||
| FROM "option" o | ||
| WHERE o.category_id = :categoryId | ||
| ) | ||
| """, nativeQuery = true) | ||
| void deleteBySpotIdAndCategoryId(@Param("spotId") Long spotId, @Param("categoryId") Long categoryId); |
There was a problem hiding this comment.
The @Modifying annotation should include clearAutomatically = true to clear the persistence context after execution, preventing stale entity issues.
| // 3. spotType 변경 처리 | ||
| if (request.spotType() != null && originalSpotType != request.spotType()) { | ||
| // 기존 spotOption 삭제 (features와 price) | ||
| spotOptionRepository.deleteAllBySpotId(spotId); |
There was a problem hiding this comment.
This unconditionally deletes all spot options when spot type changes, but lines 445-446 and 475-476 already handle selective deletion of features and price options. This creates redundant delete operations that could impact performance.
| spotOptionRepository.deleteAllBySpotId(spotId); | |
| // 삭제는 아래에서 feature와 price별로 개별적으로 처리됨 |
| // 기존 DB 레코드 모두 삭제 (순서 재정렬을 위해) | ||
| menuboardImageRepository.deleteAll(existingMenuboardImages); | ||
|
|
||
| // 새로운 메뉴판 이미지 추가 | ||
| if (!request.menuboardImageList().isEmpty()) { | ||
| List<String> finalImageUrls = new ArrayList<>(); | ||
|
|
||
| for (String imageUrl : request.menuboardImageList()) { | ||
| // 기존 이미지인지 확인 (이미 spot 폴더에 있음) | ||
| if (existingImageUrls.contains(imageUrl)) { | ||
| finalImageUrls.add(imageUrl); | ||
| } else { | ||
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | ||
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | ||
| String destinationKey = String.format("spots/%d/menuboard/%s", spotId, fileName); | ||
| String newImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | ||
| finalImageUrls.add(newImageUrl); | ||
| } | ||
| } | ||
|
|
||
| // DB에 저장 | ||
| List<MenuboardImageEntity> newMenuboardImages = finalImageUrls.stream() | ||
| .map(image -> MenuboardImageEntity.builder() | ||
| .spotId(spotId) | ||
| .image(image) | ||
| .build()) | ||
| .toList(); | ||
|
|
||
| menuboardImageRepository.saveAll(newMenuboardImages); | ||
| } |
There was a problem hiding this comment.
Deleting and recreating all records just for reordering is inefficient. Consider updating existing records in place or using a position/order field to maintain sequence without full deletion.
| // 기존 DB 레코드 모두 삭제 (순서 재정렬을 위해) | |
| menuboardImageRepository.deleteAll(existingMenuboardImages); | |
| // 새로운 메뉴판 이미지 추가 | |
| if (!request.menuboardImageList().isEmpty()) { | |
| List<String> finalImageUrls = new ArrayList<>(); | |
| for (String imageUrl : request.menuboardImageList()) { | |
| // 기존 이미지인지 확인 (이미 spot 폴더에 있음) | |
| if (existingImageUrls.contains(imageUrl)) { | |
| finalImageUrls.add(imageUrl); | |
| } else { | |
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | |
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| String destinationKey = String.format("spots/%d/menuboard/%s", spotId, fileName); | |
| String newImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | |
| finalImageUrls.add(newImageUrl); | |
| } | |
| } | |
| // DB에 저장 | |
| List<MenuboardImageEntity> newMenuboardImages = finalImageUrls.stream() | |
| .map(image -> MenuboardImageEntity.builder() | |
| .spotId(spotId) | |
| .image(image) | |
| .build()) | |
| .toList(); | |
| menuboardImageRepository.saveAll(newMenuboardImages); | |
| } | |
| // 기존 DB 레코드 중 삭제된 이미지는 삭제 | |
| List<MenuboardImageEntity> imagesToDelete = existingMenuboardImages.stream() | |
| .filter(entity -> !request.menuboardImageList().contains(entity.getImage())) | |
| .toList(); | |
| if (!imagesToDelete.isEmpty()) { | |
| menuboardImageRepository.deleteAll(imagesToDelete); | |
| } | |
| // 순서 및 신규 이미지 처리 | |
| List<MenuboardImageEntity> imagesToSaveOrUpdate = new ArrayList<>(); | |
| for (int i = 0; i < request.menuboardImageList().size(); i++) { | |
| String imageUrl = request.menuboardImageList().get(i); | |
| MenuboardImageEntity existingEntity = existingMenuboardImages.stream() | |
| .filter(entity -> entity.getImage().equals(imageUrl)) | |
| .findFirst() | |
| .orElse(null); | |
| String finalImageUrl = imageUrl; | |
| if (existingEntity == null) { | |
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | |
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| String destinationKey = String.format("spots/%d/menuboard/%s", spotId, fileName); | |
| finalImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | |
| MenuboardImageEntity newEntity = MenuboardImageEntity.builder() | |
| .spotId(spotId) | |
| .image(finalImageUrl) | |
| .order(i) | |
| .build(); | |
| imagesToSaveOrUpdate.add(newEntity); | |
| } else { | |
| // 기존 이미지의 순서만 업데이트 | |
| existingEntity.setOrder(i); | |
| imagesToSaveOrUpdate.add(existingEntity); | |
| } | |
| } | |
| if (!imagesToSaveOrUpdate.isEmpty()) { | |
| menuboardImageRepository.saveAll(imagesToSaveOrUpdate); | |
| } |
| spotImageRepository.deleteAll(existingSpotImages); | ||
|
|
||
| // 새로운 장소 이미지 추가 | ||
| if (!request.spotImageList().isEmpty()) { | ||
| List<String> finalImageUrls = new ArrayList<>(); | ||
|
|
||
| for (String imageUrl : request.spotImageList()) { | ||
| // 기존 이미지인지 확인 (이미 spot 폴더에 있음) | ||
| if (existingImageUrls.contains(imageUrl)) { | ||
| finalImageUrls.add(imageUrl); | ||
| } else { | ||
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | ||
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | ||
| String destinationKey = String.format("spots/%d/spot/%s", spotId, fileName); | ||
| String newImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | ||
| finalImageUrls.add(newImageUrl); | ||
| } | ||
| } | ||
|
|
||
| // DB에 저장 | ||
| List<SpotImageEntity> newSpotImages = finalImageUrls.stream() | ||
| .map(image -> SpotImageEntity.builder() | ||
| .spotId(spotId) | ||
| .image(image) | ||
| .build()) | ||
| .toList(); | ||
|
|
||
| spotImageRepository.saveAll(newSpotImages); |
There was a problem hiding this comment.
Deleting and recreating all records just for reordering is inefficient. Consider updating existing records in place or using a position/order field to maintain sequence without full deletion.
| spotImageRepository.deleteAll(existingSpotImages); | |
| // 새로운 장소 이미지 추가 | |
| if (!request.spotImageList().isEmpty()) { | |
| List<String> finalImageUrls = new ArrayList<>(); | |
| for (String imageUrl : request.spotImageList()) { | |
| // 기존 이미지인지 확인 (이미 spot 폴더에 있음) | |
| if (existingImageUrls.contains(imageUrl)) { | |
| finalImageUrls.add(imageUrl); | |
| } else { | |
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | |
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| String destinationKey = String.format("spots/%d/spot/%s", spotId, fileName); | |
| String newImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | |
| finalImageUrls.add(newImageUrl); | |
| } | |
| } | |
| // DB에 저장 | |
| List<SpotImageEntity> newSpotImages = finalImageUrls.stream() | |
| .map(image -> SpotImageEntity.builder() | |
| .spotId(spotId) | |
| .image(image) | |
| .build()) | |
| .toList(); | |
| spotImageRepository.saveAll(newSpotImages); | |
| // Instead of deleting all, update order field in place, add new images, and delete removed ones. | |
| List<String> newImageList = request.spotImageList(); | |
| if (newImageList != null && !newImageList.isEmpty()) { | |
| // Map existing images by URL for quick lookup | |
| java.util.Map<String, SpotImageEntity> existingImageMap = new java.util.HashMap<>(); | |
| for (SpotImageEntity entity : existingSpotImages) { | |
| existingImageMap.put(entity.getImage(), entity); | |
| } | |
| List<SpotImageEntity> imagesToSaveOrUpdate = new ArrayList<>(); | |
| int order = 0; | |
| for (String imageUrl : newImageList) { | |
| SpotImageEntity entity = existingImageMap.get(imageUrl); | |
| if (entity != null) { | |
| // Update order if changed | |
| entity.setOrder(order); | |
| imagesToSaveOrUpdate.add(entity); | |
| } else { | |
| // 새 이미지인 경우 temp -> spot 폴더로 이동 | |
| String fileName = imageUrl.substring(imageUrl.lastIndexOf("/") + 1); | |
| String destinationKey = String.format("spots/%d/spot/%s", spotId, fileName); | |
| String newImageUrl = s3Adapter.moveFile(imageUrl, destinationKey); | |
| SpotImageEntity newEntity = SpotImageEntity.builder() | |
| .spotId(spotId) | |
| .image(newImageUrl) | |
| .order(order) | |
| .build(); | |
| imagesToSaveOrUpdate.add(newEntity); | |
| } | |
| order++; | |
| } | |
| // Save all updated/new entities | |
| spotImageRepository.saveAll(imagesToSaveOrUpdate); | |
| // Delete entities for images that were removed | |
| List<SpotImageEntity> imagesToDelete = existingSpotImages.stream() | |
| .filter(entity -> !newImageList.contains(entity.getImage())) | |
| .toList(); | |
| if (!imagesToDelete.isEmpty()) { | |
| spotImageRepository.deleteAll(imagesToDelete); | |
| } |
💡 Issue
📄 Description