[Feature] ProductImage 도메인 구현#180
Conversation
| public void update(String imageUrl, boolean isDefault) { | ||
| this.imageUrl = imageUrl; | ||
| this.isDefault = isDefault; | ||
| } |
There was a problem hiding this comment.
P1: 현재 업데이트는 대표 사진만 변경하고자 할 때 (null, true)로 업데이트 되어 이미지가 사라질 수 있습니다.
if (imageUrl != null) { this.imageUrl = imageUrl; } if (isDefault != null) { this.isDefault = isDefault; }
를 사용하는게 어떨까요?
There was a problem hiding this comment.
좋은 포인트 감사합니다 🙂
기존 update()는 두 필드를 한 번에 수정하다 보니 대표 이미지 변경 시 null 값이 들어올 위험이 있어서 updateImageUrl(), markAsDefault(), unmarkAsDefault()로 역할을 명확히 분리했습니다.
| @PostMapping | ||
| public void addProductImage( | ||
| @PathVariable UUID productId, @Valid @RequestBody ProductImageCreateRequest request) { | ||
| log.info("상품 이미지 추가 요청: productId={}, isDefault={}", productId, request.isDefault()); | ||
| productImageService.addProductImage(productId, request); | ||
| } |
There was a problem hiding this comment.
P1: 다른 PR에서 리뷰했었는데 저희는 이미지 주소가 이미지는 파일 자체를 받아야 하기 때문에 ProductImageCreateRequest가 아닌 MultipartFile를 통해 받아야 해요.
파일 업로드 시에는 @RequestBody로 받지 않기 때문에 공부해보시면 좋을 거 같습니다.
P3: 추가로 현재는 한 번에 한 장씩 등록하고 있는데, 여러 이미지를 한 번에 업로드할 수 있도록 변경하는 방향으로도 고려해보시면 좋을 거 같습니다.
There was a problem hiding this comment.
앗 해당 부분 미처 반영을 못했네요 다시 한번 확인부탁드립니다!
@RequestBody 대신 @RequestPart("files") List로 변경하여 실제 파일 업로드가 가능하도록 했고,
consumes = MediaType.MULTIPART_FORM_DATA_VALUE를 추가해 multipart 요청을 처리하도록 수정했습니다.
Bal1oon
left a comment
Bal1oon
left a comment
There was a problem hiding this comment.
고생하셨습니다 👏
Multipart 적용이 잘 되어 있고, 전체적인 구조가 괜찮은 것 같습니다!
몇 가지 코멘트 남겼으니 한 번 확인 부탁드립니다 :)
| if (image.isDefault()) { | ||
| productImageRepository | ||
| .findTopByProductIdOrderByCreatedAtDesc(productId) | ||
| productImageRepository.findTopByProductIdOrderByCreatedAtDesc(productId) | ||
| .ifPresent(ProductImage::markAsDefault); | ||
| } | ||
|
|
||
| fileStorageService.delete(image.getImageUrl()); | ||
| productImageRepository.delete(image); |
There was a problem hiding this comment.
P3: 이미지 삭제 시, 대표 이미지를 삭제하면 최신 이미지를 대표 이미지로 지정하도록 되어 있는데, findTopByProductIdOrderByCreatedAtDesc()가
삭제되는 이미지 자신을 포함해 조회될 수 있는 타이밍 이슈가 있습니다.
productImageRepository.delete(image) 이후에 조회하도록 순서를 바꾸는 것이 더 안전할 듯 합니다.
아니면 flush()로 DB 반영 후 조회하는 방법이 있습니다
| validateFile(file); | ||
|
|
||
| // 실제 파일 저장 (로컬 또는 S3) | ||
| String storedUrl = fileStorageService.save(file); |
There was a problem hiding this comment.
P1: 이 부분(fileStorageService.save(file);) I/O작업이라 DB 트랜잭션과 분리해야 하는 부분인데요, GPT로 생성한 코드입니다. 참고하시면 좋을 것 같습니다 :)
/**
* 상품 이미지 업로드
* - 파일 저장은 트랜잭션 밖에서 선행
* - DB insert는 트랜잭션 안에서 수행
*/
public void uploadProductImages(UUID productId, List<MultipartFile> files, Boolean isDefault) {
// 1️⃣ 파일 저장 (트랜잭션 X)
List<String> storedUrls = files.stream()
.map(file -> {
validateFile(file);
return fileStorageService.save(file);
})
.toList();
// 2️⃣ DB 저장 (트랜잭션 O)
saveProductImages(productId, storedUrls, isDefault);
}
@Transactional
protected void saveProductImages(UUID productId, List<String> storedUrls, Boolean isDefault) {
Member member = memberUtil.getCurrentMember();
Product product = findValidProduct(productId);
memberUtil.assertMemberResourceAccess(product.getStore().getMember());
if (Boolean.TRUE.equals(isDefault)) {
productImageRepository.findByProductId(productId).stream()
.filter(ProductImage::isDefault)
.findFirst()
.ifPresent(ProductImage::unmarkAsDefault);
}
for (String storedUrl : storedUrls) {
ProductImage productImage = ProductImage.create(product, storedUrl, isDefault);
productImageRepository.save(productImage);
log.info("상품 이미지 등록 완료: productId={}, storedUrl={}, isDefault={}",
productId, storedUrl, isDefault);
}
}| @Slf4j | ||
| public class FileStorageService { | ||
|
|
||
| private static final String UPLOAD_DIR = "uploads/"; |
There was a problem hiding this comment.
Postman으로 테스트해봤는데 디렉토리 경로가 Tomcat 임시 디렉토리 기준 상대경로로 인식되기 때문에 파일 저장이 안되네요. 해결 방법에 대해서는 여러 방법이 있는데 로컬 개발 환경에서는 프로젝트 루트 기준으로 경로를 잡는 것이 안전하다고 하네요.
private static final String UPLOAD_DIR =
System.getProperty("user.dir") + "/uploads/";제 로컬에서도 한번 해보겠습니다
|
현재 Postman으로 테스트 했을 때에 제가 겪는 문제들 남겨놓습니다:
|
| @Slf4j | ||
| public class FileStorageService { | ||
|
|
||
| private static final String UPLOAD_DIR = "uploads/"; |
There was a problem hiding this comment.
P2: 파일 경로와 같은 상수 값들은 직접 변수로 선언하기 보다는, Constants.class와 같은 불변 클래스나 레코드를 통해 관리하는게 유지보수 및 안정성 측면에서 좋아보입니다! 참고해주세요!
| String uniqueName = UUID.randomUUID() + "_" + file.getOriginalFilename(); | ||
| Path path = Paths.get(UPLOAD_DIR + uniqueName); | ||
|
|
There was a problem hiding this comment.
P4: 현재 사용자 입력을 그대로 받아 파일경로로 사용하고 있는데, "../../etc"와 같이 상대 경로가 포함된 악의적인 경로를 통해 입력되면, 의도치 않은 폴더로 사용자의 파일이 이동할 수 있을 것 같습니다.
Spring에서 지원하는 StringUtils를 사용하면 경로를 안전하게 처리할 수 있다고 하네요!
String cleanFilename = StringUtils.cleanPath(file.getOriginalFilename());
또한 문자열을 "+" 로 연결해 최종 경로로 사용하는 것보다, Path 타입에서 지원하는 .resolve() 를 사용해서 경로를 처리하는게 좋아보입니다.
다만 S3에서는 파일 경로를 기반으로 처리하진 않으니, 참고만 해보셔도 좋을 듯 합니다!
| public void uploadProductImages( | ||
| @PathVariable UUID productId, | ||
| @RequestPart("files") List<MultipartFile> files, | ||
| @RequestPart("isDefault") Boolean isDefault) { |
There was a problem hiding this comment.
P3: isDefault 없이 구현하는 건 어떠신가요? 들어오는 이미지들에 대해서는 모두 isDefault false를 기본으로 하고 상품에 이미지가 없는 경우 제일 처음에 들어온 이미지를 true로 바꿔 저장하는 로직으로 구현하면 좀 더 편할 것 같습니다.
역할 분리도 명확해지고요.
| if (Boolean.TRUE.equals(isDefault)) { | ||
| boolean alreadyHasDefault = | ||
| productImageRepository.findByProductId(productId).stream() | ||
| .anyMatch(ProductImage::isDefault); | ||
| if (alreadyHasDefault) { | ||
| throw new CommonException(ProductImageErrorCode.DUPLICATE_DEFAULT_IMAGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2: 여러 이미지 업로드 시 isDefault=true의 경우 해당 이미지들이 모두 true로 처리되어 for문을 돌고 있습니다.
해당 로직에 경우 마지막 이미지만 대표 이미지로 남고, 앞서 들어온 이미지들은 true -> false로 바뀌어 DB에 저장이 되거나 flush 타이밍에 따라 무시되는데 지금 실행해본 결과 후자인 것 같습니다. 추가로 계속 DB 조회를 하는 것이 문제가 될 수 있을 것 같네요.
isDefault=true일 경우
- 첫 번째 이미지만 대표로 지정하도록 명시해서 for문을 돌게하거나
isDefault를 파라미터에서 제외시켜 무조건 false로만 생성 + 상품에 기존 이미지가 아무것도 없으면 첫장만true로 하는게 구현하시기 편하실 거 같습니다.
| public void uploadProductImages(UUID productId, List<MultipartFile> files, Boolean isDefault) { | ||
| // 파일 저장 | ||
| List<String> storedUrls = | ||
| files.stream() | ||
| .map( | ||
| file -> { | ||
| validateFile(file); | ||
| return fileStorageService.save(file); | ||
| }) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| // DB 저장 | ||
| saveProductImages(productId, storedUrls, isDefault); | ||
| } |
There was a problem hiding this comment.
P2: 트랜잭션 분리
saveProductImages()에 @Transactional이 붙어 있긴 하지만, 같은 클래스 내부에서 호출하고 있기 때문에 Spring AOP가 적용되지 않습니다.
Spring의 @Transactional은 “프록시 기반 AOP”로 동작하는데, 프록시가 적용되려면 외부 Bean이 해당 메서드를 호출해야 합니다.
지금 구조에서는 uploadProductImages() → this.saveProductImages()로 직접 호출하므로 프록시를 거치지 않고 실행되어, 트랜잭션이 전혀 시작되지 않습니다.
P2: 스토리지와 DB 불일치 발생
예외 복구 로직이 없어 파일 저장은 되었지만 DB에는 저장이 되지 않는 상황이 생길 수 있습니다.
예로 fileStorageService.save(file)이 수행되어 파일 저장이 이뤄졌는데 saveProductImage()에서 MemberUtil의 사용자 인증을 통과하지 못하는 경우가 있을 수 있네요.
| @Getter | ||
| @Table( | ||
| name = "p_product_image", | ||
| uniqueConstraints = {@UniqueConstraint(columnNames = {"product_id", "is_default"})}) |
There was a problem hiding this comment.
P1: 이 조건이 is_default가 false로 중복되어도 안된다고 하네요. 제가 제안드렸던 건데 없애는 게 맞는 것 같습니다. 만약 제약을 추가하고 싶다면 DB 레벨에서 직접 추가하는 방식을 사용해야 한대요
|
@willjsw 님이 코멘트 주신 부분들 해결해주시면 될 것 같습니다. |
- 대표 이미지 추가 시 기존 대표 자동 해제 로직 추가 - changeDefaultImage 로직에서 기존 대표 해제 후 지정 이미지 대표로 설정 - deleteProductImage 시 대표 이미지 삭제 시 최신 이미지 자동 대표 지정 - findValidProduct, findValidImage 유효성 검증 메서드 추가
- @RequestBody → @RequestPart("files")로 수정하여 다중 파일 업로드 지원 - consumes = MediaType.MULTIPART_FORM_DATA_VALUE 적용 - 서비스 로직에서 MultipartFile 직접 처리 가능하도록 구조 변경 - 불필요한 ProductImageCreateRequest DTO 제거
- FileStorageService 추가하여 파일 I/O 로직 분리 (S3 확장 대비 구조) - 이미지가 없을 경우 예외 대신 빈 리스트 반환하도록 수정 - 파일명 충돌 방지를 위해 UUID prefix 적용 - 파일 확장자 및 크기 검증 로직 추가 (.jpg, .jpeg, .png / 10MB 이하) - ProductImageErrorCode 정리 및 FILE_SAVE_FAILED 추가
- FileStorageService: 정규식 "(?i).*(\\.jpg|\\.jpeg|\\.png)$" 적용으로 대소문자 구분 제거 - 파일 확장자 및 크기 검증 로직 강화 - ProductImageService: 대표 이미지 삭제 시 flush() 후 최신 이미지 조회하도록 순서 수정
- I/O 작업(fileStorageService.save) 트랜잭션 분리 - DB insert는 별도 @transactional 메서드에서 수행 - 롤백 시 파일 I/O 영향 최소화
- Tomcat 임시 폴더 문제 해결 (System.getProperty("user.dir") 사용)
- isDefault=true 중복 등록 시 예외 처리 추가
- 파일 저장 트랜잭션 외부 분리로 안정성 향상
- StringUtils.cleanPath(), Path.resolve() 적용 - 파일 저장 상수 FileStorageConstants로 분리 - ProductImageService에서 상수 직접 선언 제거 - 파일 크기/확장자 검증 로직 정리
- FileStorageService 리팩터링 - 경로 하드코딩 제거 및 FileProperties 연동 (환경별 설정 분리) - FileStorageConstants 추가로 정책 상수(확장자, 최대 용량 등) 분리 - StringUtils.cleanPath 및 Path.resolve() 적용으로 경로 조작 방지 - Objects.requireNonNull 적용으로 null-safe 파일명 처리 - 파일 확장자/용량 검증 로직 강화 - delete(String) 메서드 추가로 안전한 파일 삭제 지원 - S3 업로드 분기 구조 설계 (후속 구현 예정) - ProductImageService 개선 - 트랜잭션-IO 로직 분리 (파일 저장과 DB 저장 분리) - @lazy self 주입을 통한 트랜잭션 프록시 적용 - 업로드 중 예외 발생 시 스토리지 롤백 복구 처리 - 순환참조(BeanCurrentlyInCreationException) 문제 해결 - 대표 이미지 변경 및 삭제 로직 안정화
3b15f9a to
6f16170
Compare
|
@growingpsy Approved 이후 변경이 있어, 임시로 Draft 전환했습니다. |
971ae9d to
121c3ad
Compare
- build.gradle에 QueryDSL 의존성 버전 명시 (:5.0.0:jakarta) - Q클래스 생성 경로(build/generated/querydsl) 인식 추가 - Lombok annotationProcessor 설정 보완 - PropertiesConfig 중복 @EnableConfigurationProperties 제거
4 participants
🛠️ Issue Number
closes #177
📌 작업 내용 및 특이사항
상품 이미지 CRUD 구현 (대표 이미지 지정 포함)
공통
상품 이미지 등록
상품 이미지 조회
상품 이미지 수정
상품 이미지 삭제
📚 참고사항
product-image-*문서 추가)