Conversation
k3vin7
commented
Mar 28, 2026
- PUT /banners/{bannerId}/images: 배너 이미지 교체 (기존 S3 이미지 삭제 후 새 이미지 업로드)
- DELETE /banners/{bannerId}: 배너 영구 삭제 (S3 이미지 포함)
- ADMIN 전용 권한 적용
There was a problem hiding this comment.
Code Review
This pull request implements the update and deletion of banner images, including the necessary security configurations, service methods for S3 interaction, and REST endpoints. Feedback was provided regarding potential data inconsistency issues in the service layer where S3 operations are performed before database transactions are finalized. Additionally, a potential runtime exception in the S3 image deletion logic was identified due to a lack of URL validation.
| Banner banner = bannerRepository.findById(bannerId) | ||
| .orElseThrow(() -> new BusinessException(BannerErrorStatus.BANNER_NOT_FOUND)); | ||
| s3ImageService.deleteImage(banner.getImageUrl()); | ||
| String newImageUrl = s3ImageService.uploadImage(image); | ||
| banner.update(newImageUrl, null, null, null); | ||
| return BannerResponseDto.from(banner); |
There was a problem hiding this comment.
현재 구현에서는 S3에서 기존 이미지를 삭제한 후 새 이미지 업로드를 시도하고 있습니다. 만약 새 이미지 업로드가 실패할 경우, 데이터베이스 트랜잭션은 롤백되지만 S3에서 삭제된 이미지는 복구되지 않습니다. 이로 인해 배너는 존재하지 않는 이미지를 가리키게 되어 데이터 불일치가 발생합니다.
이러한 문제를 방지하기 위해, 새 이미지 업로드를 먼저 완료한 후에 기존 이미지를 삭제하는 것이 안전합니다.
Banner banner = bannerRepository.findById(bannerId)
.orElseThrow(() -> new BusinessException(BannerErrorStatus.BANNER_NOT_FOUND));
String oldImageUrl = banner.getImageUrl();
String newImageUrl = s3ImageService.uploadImage(image);
banner.update(newImageUrl, null, null, null);
s3ImageService.deleteImage(oldImageUrl);
return BannerResponseDto.from(banner);| s3ImageService.deleteImage(banner.getImageUrl()); | ||
| bannerRepository.delete(banner); |
There was a problem hiding this comment.
updateBannerImage 메서드와 유사하게, deleteBanner 메서드에서도 S3 이미지를 먼저 삭제하고 데이터베이스에서 배너를 삭제하고 있습니다. 만약 데이터베이스 삭제가 실패하면 트랜잭션이 롤백되어 배너 레코드는 남지만, S3 이미지는 이미 삭제된 상태가 됩니다. 이 또한 데이터 불일치를 유발합니다.
데이터베이스에서 배너를 먼저 삭제한 후 S3 이미지를 삭제하는 것이 더 안전합니다.
| s3ImageService.deleteImage(banner.getImageUrl()); | |
| bannerRepository.delete(banner); | |
| bannerRepository.delete(banner); | |
| s3ImageService.deleteImage(banner.getImageUrl()); |
| try { | ||
| String key = imageUrl.substring(imageUrl.indexOf("images/")); | ||
| s3Client.deleteObject(DeleteObjectRequest.builder() | ||
| .bucket(bucketName) | ||
| .key(key) | ||
| .build()); | ||
| log.info("Image deleted successfully: {}", imageUrl); | ||
| } catch (S3Exception e) { | ||
| log.warn("Failed to delete image from S3: {}", imageUrl, e); | ||
| } |
There was a problem hiding this comment.
deleteImage 메서드에서 imageUrl로부터 S3 키를 추출하는 로직이 StringIndexOutOfBoundsException을 발생시킬 수 있습니다. imageUrl이 null이거나 예상치 못한 형식이라 "images/" 문자열을 포함하지 않는 경우, indexOf는 -1을 반환하고 substring 호출 시 예외가 발생합니다.
S3에서 객체를 삭제하기 전에 imageUrl의 유효성을 검사하고 키 추출이 안전한지 확인하는 방어적인 코드를 추가하는 것이 좋습니다.
if (imageUrl == null || !imageUrl.contains("images/")) {
log.warn("Invalid image URL for deletion: {}", imageUrl);
return;
}
try {
String key = imageUrl.substring(imageUrl.indexOf("images/"));
s3Client.deleteObject(DeleteObjectRequest.builder()
.bucket(bucketName)
.key(key)
.build());
log.info("Image deleted successfully: {}", imageUrl);
} catch (S3Exception e) {
log.warn("Failed to delete image from S3: {}", imageUrl, e);
}