[Feature] 상품 이미지 API 추가 #5

Cass07 · 2024-12-08T19:53:27Z

작업 내역 개요

이미지 등록/수정/삭제, 로컬 서버의 이미지 조회 추가

ImageStore 인터페이스를 구현하는 로컬 서버에 이미지 파일을 저장하는 LocalImageStore 클래스 구현
존재하는 상품을 대상으로 이미지를 등록/수정/삭제하는 메소드 추가
- 조회 API의 경우, 상품 조회 API를 통해서 조회 가능하다고 판단하여 추가하지 않았음
- imageUrl 접두사의 경우, property로 지정하였고, 차후 CD진행 시에는 property파일의 값을 CD 과정에서 변경하여 배포 URL 접두사로 변경할 예정
로컬 서버의 이미지 파일을 조회해서 URL resource 로 리턴하는 메소드 추가
- /images/{imageFileName}
  - ex) http://localhost:8080/api/v1/images/test.png
- 서버의 Resource를 조회하기 위한 용도로 추가한 것이라, Rest API로 만들지 않았음

이미지 등록/수정/삭제, 로컬 서버의 이미지 조회 추가 - ImageStore 인터페이스를 구현하는 로컬 서버에 이미지 파일을 저장하는 LocalImageStore 클래스 구현 - 존재하는 상품을 대상으로 이미지를 등록/수정/삭제하는 메소드 추가 - 로컬 서버의 이미지 파일을 조회해서 URL resource 로 리턴하는 메소드 추가

github-actions · 2024-12-08T19:54:29Z

Jacoco Test Coverage Report

Overall Project	95.57% `-1.65%`	🍏
Files changed	94.03%	🍏

File	Coverage
ImageController.java	100%	🍏
ImageFacade.java	100%	🍏
ImageStore.java	100%	🍏
Product.java	100%	🍏
LocalImageStore.java	85.86% `-14.14%`	🍏
MultipartToBytesConverter.java	54.55% `-45.45%`	❌

Cass07 · 2024-12-08T19:56:04Z

src/test/java/dev/cass/knorda/api/product/controller/ImageControllerTest.java

+		// given
+		String imageName = "image.png";
+		String imageDir = "./image/";
+		URL fileDir = getClass().getClassLoader().getResource(imageName);
+		assertNotNull(fileDir);
+
+		File file = new File(fileDir.getFile());
+		File newDir = new File(imageDir);
+		if (!newDir.exists()) {
+			assertTrue(newDir.mkdir());
+		}
+		File newFile = new File(imageDir, imageName);
+		FileOutputStream fos = new FileOutputStream(newFile);
+		fos.write(Files.readAllBytes(file.toPath()));
+		fos.close();
+


이미지 resource 조회의 test를 위해서, test/resources 에 테스트용 파일을 추가하여, 실제 테스트 환경에 파일을 추가하여 테스트하고 삭제하도록 구현하였는데, 이 방법이 최선인지는 잘 모르겠습니다.

변수 명명규칙에 맞도록 변경, SSRF 취약점을 보완하기 위해 resource 조회 메소드 수정

github-actions · 2024-12-08T20:15:51Z

Jacoco Test Coverage Report

Overall Project	95.18% `-2.06%`	🍏
Files changed	92.71%	🍏

File	Coverage
ImageController.java	100%	🍏
ImageFacade.java	100%	🍏
ImageStore.java	100%	🍏
Product.java	100%	🍏
LocalImageStore.java	82.08% `-17.92%`	🍏
MultipartToBytesConverter.java	54.55% `-45.45%`	❌

contentType을 정상적으로 리턴하도록 수정하고, 존재하지 않는 이미지를 조회 시도할 때, Http Status Not Found를 리턴하도록 수정

github-actions · 2024-12-08T20:36:54Z

Jacoco Test Coverage Report

Overall Project	94.57% `-2.75%`	🍏
Files changed	90.93%	🍏

File	Coverage
ImageFacade.java	100%	🍏
ImageStore.java	100%	🍏
Product.java	100%	🍏
ImageController.java	91.51% `-8.49%`	🍏
LocalImageStore.java	82.08% `-17.92%`	🍏
MultipartToBytesConverter.java	54.55% `-45.45%`	❌

f-lab-jd

고생하셨습니다! 👍 리뷰드린 내용 한번 확인&답변 부탁드리겠습니다!

f-lab-jd · 2024-12-16T15:13:19Z

src/main/java/dev/cass/knorda/api/product/controller/ImageController.java

+	/**
+	 * 애플리케이션 서버에 저장된 이미지를 리턴해 주는 메소드
+	 */
+	@GetMapping(value = "/images/{imageName}")


작성해주신 API 관련해서 궁금한점이 있습니다!

해당 API 의 목적은 무엇인가요?

imageId 가 없는 리소스인가요?

해당 API 는 Service 가 아니라 Controller 에 비즈니스 로직이 있는데, 따로 이유가 있으신가요~?

웹 애플리케이션이 실행되는 서버에 저장된 이미지 파일을 리턴하기 위한 메소드입니다 (MIME Type을 image/png, jpg... 등으로 리턴하기 위해서)
이미지 저장 비즈니스 로직을 보면 이미지 파일을 받으면 이를 무작위 문자열의 이름으로 파일을 저장하여 실제 이미지를 호출할 수 있는 URL을 리턴해서, product 테이블에서 이 URL을 저장하고 있습니다.
그래서 실제 저장된 파일의 이름을 imageName 변수로 받아 호출하도록 일단 구현하였습니다.

비즈니스 로직은 별도의 ~~controller~~ service를 추가하여 분리하는 것이 맞는 것 같습니다.

f-lab-jd · 2024-12-16T15:13:32Z

src/main/java/dev/cass/knorda/api/product/controller/ImageController.java

+	@GetMapping(value = "/images/{imageName}")
+	public ResponseEntity<Resource> getImage(@PathVariable String imageName) {
+		String encodedImageName = URLEncoder.encode(imageName, StandardCharsets.UTF_8);
+		String imageDir = "./image/";


매직넘버와 스트링은 상수로 빼는것이 좋습니다.

해당 부분의 로직은 컨트롤러 레이어에서 서비스 레이어로 분리한 후, 스트링 또한 상수로 분리하여 수정하겠습니다.

f-lab-jd · 2024-12-16T15:14:11Z

src/main/java/dev/cass/knorda/api/product/controller/ImageController.java

+	}
+
+	@PostMapping("/products/{productId}/images")
+	public ResponseEntity<ImageDto.ImageResponse> registerImage(@PathVariable int productId, MultipartFile image,


ProductImage 와 Image 가 구분되는게 맞다면, ProductImageController 라는 곳에서 처리를 해도 좋을 것 같습니다.

ProductImage, Image가 명확하게 구분될 수 있도록 클래스 명을 수정했습니다.

f-lab-jd · 2024-12-16T15:15:36Z

src/main/java/dev/cass/knorda/api/product/controller/ImageController.java

+	@PostMapping("/products/{productId}/images")
+	public ResponseEntity<ImageDto.ImageResponse> registerImage(@PathVariable int productId, MultipartFile image,
+		HttpSession session) {
+		String loggedInMember = SessionManageUtils.getMemberName(session);


loggedInMember 라는 변수는 String 보다는 Object에 어울리는 이름입니다.
별도의 Object로 관리하는 것이 어떤가요~? (그리고 보통은 로직 안에서 id 로 처리를 하기 때문에 id 를 넘기는게 더 좋아보입니다)

해당 부분은 Session을 관리하는 로직을 분리하는 과정에서 객체명을 변경하고, 사용자 정보를 다루는 방법을 통일하도록 하겠습니다.

f-lab-jd · 2024-12-16T15:22:36Z

src/main/java/dev/cass/knorda/api/product/facade/ImageFacade.java

+
+@Component
+@RequiredArgsConstructor
+public class ImageFacade {


ProductImageFacade 가 조금 더 적당한 네이밍인 것 같은데 어떻게 생각하시나요~?

Image, ProductImage를 명확히 구분할 수 있도록 전체적으로 클래스명을 수정하였고, facade 또한 ProductImageFacade로 수정했습니다.

f-lab-jd · 2024-12-16T15:32:11Z

src/main/java/dev/cass/knorda/api/product/image/LocalImageStore.java

+			File file = new File(newDir, newImageName);
+			log.info(file.getAbsolutePath());
+
+			FileOutputStream fos = new FileOutputStream(file);


try with resource 문법을 사용하시면 가독성을 더 높일 수 있습니다.

try with resource 문법을 사용하도록 수정했습니다.

f-lab-jd · 2024-12-16T15:35:16Z

src/main/java/dev/cass/knorda/api/product/image/LocalImageStore.java

+				Files.delete(file.toPath());
+			}
+		} catch (Exception e) {
+			log.error(e.getMessage());


stacktrace 는 남기지 않아도 될까요?

Exception 발생 시 stack trace를 로거를 통해 남기도록 수정하겠습니다.

f-lab-jd · 2024-12-16T15:36:37Z

src/main/java/dev/cass/knorda/api/product/facade/ImageFacade.java

+	private final ProductService productService;
+	private final ImageStore localImageStore;
+
+	@Transactional


이미지 저장 실패 시 DB 트랜잭션은 롤백되어도, 파일은 롤백이 불가능할텐데요.
어떠한 방식으로 Atomic 하게 해결할 수 있을까요?
(당장 현재 PR에 반영해야하는 의도는 아닙니다)

TransactionalEventListner를 사용하면 될 것 같습니다.
이미지를 등록하는 경우에는 트랜잭션이 실패하였을 때, 이미 등록한 이미지 파일을 삭제하는 로직을 실행시키고,
이미지를 삭제하는 경우라면, 트랜잭션을 커밋하기 전까지는 실제 파일을 삭제하지 않고, 성공하였을 때 이미지 파일을 삭제하는 로직을 실행하도록 처리하면 될 것 같습니다.

f-lab-jd · 2024-12-16T15:39:01Z

src/main/java/dev/cass/knorda/api/product/facade/ImageFacade.java

+	}
+
+	@Transactional
+	public ImageDto.ImageResponse updateImage(int productId, ImageDto.ImageRequest imageRequest,


(그럴일이 거의 없겠지만) 여러명의 상품관리자가 동시에 registerImage 혹은 updateImage 를 호출하면, 이미지는 여러개 생성이 될 수 있을텐데, 동시성제어를 위해서는 어떤 방식을 적용할 수 있을까요?

이미지 정보를 crud하는 데 락을 지정하여, 이미지의 등록/수정/삭제 시 작업이 완료되기 전까지 다른 스레드가 접근하지 못하도록 처리해야 합니다.
product 테이블의 특성 상, 쓰기 락을 지정하기보다는 MySQL의 네임드 락을 적용하는 것이 제일 적합할 것으로 생각됩니다. (실제 image 관련 로직 처리시에만 락을 확인하고, 기타 product 객체를 다룰 때에는 영향을 주지 않도록)

f-lab-jd · 2024-12-16T15:40:28Z

src/main/java/dev/cass/knorda/api/product/image/LocalImageStore.java

+	@Override
+	public String storeImage(byte[] imageBytes, String imageName) {
+		// 이미지를 저장하고 저장된 이미지의 URL을 반환한다.
+		if (imageBytes == null) {


유효하지 않은 확장자, 큰 용량 등을 막는 유효성검사도 추가되면 좋을 것 같습니다.

등록 가능한 이미지 파일의 확장자 풀을 추가하고, 파일의 헤더와 용량의 검사도 추가하면 될 것 같습니다.

PR 리뷰사항 수정 - ProductImage, Image 개념이 명확하게 구분되도록 객체명을 수정 - ImageController의 비즈니스 로직을 Service 레이어에 분리 - LocalImageStore에 파일 헤더, 확장자, 용량, 파일명 검증 과정을 추가 - Converter를 구현한 커스텀 converter를 component로 등록하여 사용하지 않고, 스프링의 conversionService에 등록하여 사용하도록 변경 - Closable한 객체를 사용하는 부분은 try with resource 문법으로 변경 - error를 처리할 시, logger에 추가적으로 stack trace를 기록하도록 변경

Update 비즈니스 로직과 컨트롤러를 삭제 - 별도의 Update 로직을 사용하지 않고, 사용자가 Delete 후 Post 하도록 변경

ProductImageFacade에서 동시성 제어 추가 - NamedLock을 사용할 수 있게 해 주는 NamedLockManager 추가 - ProductImageFacade의 비즈니스 로직을 NamedLockManager의 로직으로 감싸 네임드 락이 적용되도록 함

github-actions · 2024-12-24T17:16:09Z

Jacoco Test Coverage Report

Overall Project	92.47% `-5.59%`	🍏
Files changed	88.67%	🍏

File	Coverage
ProductImageController.java	100%	🍏
ImageController.java	100%	🍏
WebConfig.java	100%	🍏
ProductImageFacade.java	100%	🍏
ImageStore.java	100%	🍏
Product.java	100%	🍏
ImageService.java	100%	🍏
ImageCheckUtils.java	98.25% `-1.75%`	🍏
LocalImageStore.java	87.14% `-12.86%`	🍏
MultipartToBytesConverter.java	54.55% `-45.45%`	❌
NamedLockManager.java	6.45% `-93.55%`	❌
NamedLockFailedException.java	0%	❌

누락된 Private 생성자를 추가하고, long 변수의 값을 초기화하는 계산식의 결과가 long 타입이 되도록 값을 변경

sonarqubecloud · 2024-12-24T17:23:34Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2024-12-24T17:23:38Z

Jacoco Test Coverage Report

Overall Project	92.47% `-5.59%`	🍏
Files changed	88.67%	🍏

File	Coverage
ProductImageController.java	100%	🍏
ImageController.java	100%	🍏
WebConfig.java	100%	🍏
ProductImageFacade.java	100%	🍏
ImageStore.java	100%	🍏
Product.java	100%	🍏
ImageService.java	100%	🍏
ImageCheckUtils.java	98.25% `-1.75%`	🍏
LocalImageStore.java	87.14% `-12.86%`	🍏
MultipartToBytesConverter.java	54.55% `-45.45%`	❌
NamedLockManager.java	6.45% `-93.55%`	❌
NamedLockFailedException.java	0%	❌

Cass07 commented Dec 8, 2024

View reviewed changes

[Refactor] code smell 수정

22d73c5

변수 명명규칙에 맞도록 변경, SSRF 취약점을 보완하기 위해 resource 조회 메소드 수정

[Refactor] Resource 조회 시 Error 핸들링 추가

4b49fde

contentType을 정상적으로 리턴하도록 수정하고, 존재하지 않는 이미지를 조회 시도할 때, Http Status Not Found를 리턴하도록 수정

f-lab-jd reviewed Dec 16, 2024

View reviewed changes

Cass07 added 3 commits December 23, 2024 23:39

[Refactor] Product Image Update를 삭제

622390b

Update 비즈니스 로직과 컨트롤러를 삭제 - 별도의 Update 로직을 사용하지 않고, 사용자가 Delete 후 Post 하도록 변경

[Refactor] Code Smell 수정

8ddf9ed

누락된 Private 생성자를 추가하고, long 변수의 값을 초기화하는 계산식의 결과가 long 타입이 되도록 값을 변경

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] 상품 이미지 API 추가 #5

[Feature] 상품 이미지 API 추가 #5

Cass07 commented Dec 8, 2024

github-actions bot commented Dec 8, 2024

Cass07 Dec 8, 2024

github-actions bot commented Dec 8, 2024

github-actions bot commented Dec 8, 2024

f-lab-jd left a comment

f-lab-jd Dec 16, 2024

Cass07 Dec 17, 2024 •

edited

Loading

f-lab-jd Dec 16, 2024

Cass07 Dec 17, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 23, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 23, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 22, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 22, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 18, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 18, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 17, 2024

f-lab-jd Dec 16, 2024

Cass07 Dec 18, 2024

github-actions bot commented Dec 24, 2024

sonarqubecloud bot commented Dec 24, 2024

github-actions bot commented Dec 24, 2024

[Feature] 상품 이미지 API 추가 #5

Are you sure you want to change the base?

[Feature] 상품 이미지 API 추가 #5

Conversation

Cass07 commented Dec 8, 2024

작업 내역 개요

github-actions bot commented Dec 8, 2024

Jacoco Test Coverage Report

Choose a reason for hiding this comment

github-actions bot commented Dec 8, 2024

Jacoco Test Coverage Report

github-actions bot commented Dec 8, 2024

Jacoco Test Coverage Report

f-lab-jd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cass07 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 24, 2024

Jacoco Test Coverage Report

sonarqubecloud bot commented Dec 24, 2024

Quality Gate passed

github-actions bot commented Dec 24, 2024

Jacoco Test Coverage Report

Cass07 Dec 17, 2024 •

edited

Loading