Feat: 반려동물 프로필 사진 수정 by bokyung95 · Pull Request #17 · Gaethering/Gaethering-Server2

bokyung95 · 2022-12-04T12:11:46Z

s3 연동
기본 이미지 지정
반려동물 프로필 이미지 수정 API 작성
s3 업로드 테스트 코드 작성

Pet Entity 수정 Pet Controller, Service 생성

FailedUploadImageException : 이미지 업로드 실패시 예외 ImageNotFoundException : 이미지 파일이 존재하지 않을 시 예외 InvalidImageTypeException : 이미지 확장가 아닐 때 예외 PetNotFoundException : 반려동물 프로필이 존재하지 않을 때의 예외

naekang

주말동안 고생 많으셨습니다~ 🙏

naekang · 2022-12-04T13:24:55Z

module-member/src/test/resources/application.yml

+
+dir: test-dir
+default:
+  image-url: http://127.0.0.1:8001/test-bucket/test-dir/4a95753d-1827-43c5-b9a0-2359186aedec.png


테스트용 이미지 url임을 명시하기 위해 이름에 test를 붙이는 것도 좋을거같습니다!

naekang · 2022-12-04T13:25:44Z

module-member/src/test/java/com/gaethering/modulemember/service/ImageUploadServiceTest.java

+        String urlPath = imageUploadService.uploadPetImage(file);
+
+        // then
+        assertThat(urlPath.substring(0, urlPath.lastIndexOf("/") + 1)).isEqualTo("http://127.0.0.1:8001/test-bucket/test-dir/");


isEqualTo 메소드 내부를 path변수로 대체할 수 있지 않을까요?? 🤟

naekang · 2022-12-04T13:56:06Z

module-member/src/test/java/com/gaethering/modulemember/service/ImageUploadServiceTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+
+@Import(AwsS3MockConfig.class)
+@SpringBootTest


@SpringBootTest랑 @ExtendWith(MockitoExtension.class) 이 두가지 방식이 있는데 Mock을 이용하는 방식을 택하는 것도 좋을거같습니다!
phantasmicmeans/develop-study#12

현재는 Mock으로 테스트하는 방법이 없어 추후에 더 찾아보고 수정하도록 하겠습니다:)

naekang · 2022-12-04T14:09:58Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadService.java

+
+import org.springframework.web.multipart.MultipartFile;
+
+public interface ImageUploadService {


Pet 뿐만 아니라 Member에서도 사용할 수 있기 때문에 메소드 이름에 Pet을 빼는게 어떨까요?

PetService에서 ImageUploadService에 의존하여 image를 업로드하는 것을 따로 뺀 것은 좋은 것 같습니다. 하지만 둘 다 service 계층으로 정의해둘 경우 PetService에서 다른 service에 의존하는게 조금 어색해보입니다. service말고 그냥 component처럼 빈 등록을 하면 어떨까요?

naekang · 2022-12-04T14:33:11Z

module-member/src/main/java/com/gaethering/modulemember/controller/PetController.java

+    @PatchMapping("/mypage/pets/{petId}/image")
+    public ResponseEntity<String> updatePetImage(@PathVariable("petId") Long id,
+                                                 @RequestPart("file") MultipartFile multipartFile) {
+        if (multipartFile.isEmpty()) {


예외 처리를 Service 단에서 진행하는게 어떨까....ㅎ

naekang · 2022-12-04T14:34:23Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+    private String dir;
+
+    @Override
+    public String uploadPetImage(MultipartFile multipartFile) {


@Transactional이 있어야 할 거 같습니다!

naekang · 2022-12-04T14:39:10Z

module-member/src/test/java/com/gaethering/modulemember/config/AwsS3MockConfig.java

+public class AwsS3MockConfig {
+
+    @Value("${cloud.aws.s3.bucket}")
+    public String bucket;


혹시 public인 이유가 따로 있을까요?

bokyung95

리뷰사항 참고하여 수정하겠습니다:)

bokyung95 · 2022-12-04T14:52:33Z

module-member/src/test/java/com/gaethering/modulemember/service/ImageUploadServiceTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+
+@Import(AwsS3MockConfig.class)
+@SpringBootTest


현재는 Mock으로 테스트하는 방법이 없어 추후에 더 찾아보고 수정하도록 하겠습니다:)

ImageUploadService, ImageUploadServiceImpl, PetServiceImpl ImageUploadServiceTest

younghoondoodoom

주말에도 고생 많으셨습니다! 제 생각 코멘트 남겼으니 참고 부탁드립니다!

younghoondoodoom · 2022-12-04T20:37:00Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+
+    private final AmazonS3 amazonS3;
+
+    @Value("${cloud.aws.s3.bucket}")


@value가 필드에 직접 들어가게 되어있고 Setter도 없으면 테스트하기 어려운 코드가 될 것 같습니다. 테스트 코드에서도 spring에 무조건 의존하게 될 것 같습니다. 혹시 생성자의 파라미터에 @value를 통해 받게 하도록 해서 테스트하기 쉽게 만드는 것이 어떨까요??

younghoondoodoom · 2022-12-04T20:37:16Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+    @Value("${cloud.aws.s3.bucket}")
+    private String bucket;
+
+    @Value("${dir}")


위에 코멘트랑 똑같은 생각입니다!

younghoondoodoom · 2022-12-04T20:39:30Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+    @Override
+    public String createFileName(String filename) {
+        return UUID.randomUUID().toString().concat(getFileExtension(filename));
+    }


내부적으로만 사용하는 메소드 같은데 인터페이스를 통해 public으로 열어놓은 이유가 있으실까요??

younghoondoodoom · 2022-12-04T20:39:43Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+        validateFileExtension(filename);
+
+        return filename.substring(filename.lastIndexOf("."));
+    }


위에 코멘트랑 똑같은 의견입니다!

younghoondoodoom · 2022-12-04T20:41:16Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+        fileValidate.add(".png");
+        fileValidate.add(".JPG");
+        fileValidate.add(".JPEG");
+        fileValidate.add(".PNG");


이미지 파일 확장자를 저장하는 리스트를 따로 상수로 빼던가 클래스를 분리 시키는게 좋을 것 같습니다! 확장자가 추가된다면 코드를 직접 수정해야하는 번거로움이 있을 것 같습니다!

younghoondoodoom · 2022-12-04T20:47:05Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadService.java

+
+import org.springframework.web.multipart.MultipartFile;
+
+public interface ImageUploadService {


PetService에서 ImageUploadService에 의존하여 image를 업로드하는 것을 따로 뺀 것은 좋은 것 같습니다. 하지만 둘 다 service 계층으로 정의해둘 경우 PetService에서 다른 service에 의존하는게 조금 어색해보입니다. service말고 그냥 component처럼 빈 등록을 하면 어떨까요?

younghoondoodoom · 2022-12-04T20:48:53Z

module-member/src/main/java/com/gaethering/modulemember/service/ImageUploadServiceImpl.java

+@Transactional
+public class ImageUploadServiceImpl implements ImageUploadService {
+
+    private final AmazonS3 amazonS3;


해당 클래스가 AmazonS3에 직접적으로 의존하고 있는 것 같습니다. 만약 이미지 업로드를 다른 방식으로 하게되면 해당 클래스를 직접 수정해야하는 상황이 발생할 것 같습니다. 중간에 인터페이스를 둔 클래스 분리를 통해 의존성을 숨기는게 어떨까요?

younghoondoodoom · 2022-12-04T20:49:46Z

module-member/src/main/java/com/gaethering/modulemember/service/pet/PetServiceImpl.java

+    private final ImageUploadService imageUploadService;
+    private final PetRepository petRepository;
+
+    @Value("${default.image-url}")


위에 @value에 대한 의견과 같습니다!

bkDana added 6 commits December 4, 2022 18:07

feat: Aws s3 설정

ea4ab94

feat: Aws s3 이미지 업로드 및 삭제 기능 구현

8000673

refactor: Aws s3 이미지 업로드 불필요한 코드 제거

c1ef5ce

refactor: 펫 프로필 이미지 수정 기능 구현

5fd0fc0

Pet Entity 수정 Pet Controller, Service 생성

feat: Aws S3 이미지 업로드 테스트 코드 작성

200dc14

bokyung95 added the feature New feature or request label Dec 4, 2022

bokyung95 self-assigned this Dec 4, 2022

bokyung95 linked an issue Dec 4, 2022 that may be closed by this pull request

반려동물 프로필 사진 수정 #5

Open

4 tasks

bokyung95 changed the title ~~Feat 5~~ Feat: 반려동물 프로필 사진 수정 Dec 4, 2022

bokyung95 requested review from EunjungOh123, naekang and younghoondoodoom December 4, 2022 12:19

refactor: Aws S3 이미지 업로드 테스트 코드 수정

d7fe548

naekang reviewed Dec 4, 2022

View reviewed changes

bokyung95 commented Dec 4, 2022

View reviewed changes

bkDana added 4 commits December 4, 2022 23:55

refactor: 이미지파일 여부 예외처리 Service 단으로 이동

1df9e65

refactor: 이미지 업로드, 삭제 메소드명 변경

7f21ec7

ImageUploadService, ImageUploadServiceImpl, PetServiceImpl ImageUploadServiceTest

refactor: 테스트 파일 수정

34eaa4e

refactor: 테스트 yml 파일 수정

7d92e08

younghoondoodoom reviewed Dec 4, 2022

View reviewed changes


		import org.springframework.web.multipart.MultipartFile;

		public interface ImageUploadService {


		private final AmazonS3 amazonS3;

		@Value("${cloud.aws.s3.bucket}")

Conversation

bokyung95 commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naekang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bokyung95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

younghoondoodoom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bokyung95 commented Dec 4, 2022 •

edited

Loading