feat: 사용하지 않는 이미지 제거 기능 #193

kevstevie · 2023-08-13T03:09:58Z

issue: #192

as-is

사용되지 않는 이미지가 s3에서 삭제되지 않음

to-be

이미지를 업로드했을 때 UnusedImage 테이블에 저장
리뷰가 업로드 됐을 때 UnusedImage 테이블에서 제거
리뷰가 제거됐을 때 s3에서 이미지 제거
스케쥴링을 통해 오전 4시마다 24시간이 지난 이미지 제거

jinyoungchoi95

수고하셨습니다 :)

코맨트 확인부탁드리고, 기능들 테스트도 함께 추가되면 좋을 것 같네요 👍

jinyoungchoi95 · 2023-08-15T15:31:52Z

...ip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageEventListener.java

+import org.springframework.transaction.event.TransactionalEventListener;
+
+@Component
+@Async(value = "asyncTaskExecutor")


생각해볼만한점이 하나 있을 것 같은데요. (이전에 저희가 만들때는 고려하지 않았던 내용들도 있지만 고민해보면 좋을 것 같아서 코맨트 남겨두어요.)

asyncTaskExecutor를 사용하면 다른 컴포넌트랑 동일한 스레드를 사용하게 될 텐데 괜찮을까요? 분리하지 않고 사용하는 이유가 있을까요?

@Transactional을 호출하고 있는데, async하게 돌아가는 다른 스레드들이 메인 스레드의 connection을 가져가서 사용해도 괜찮은가요?

jinyoungchoi95 · 2023-08-15T15:32:20Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

    }

    @Transactional
    public ImageUploadResponse uploadImage(final MultipartFile file) {
        String extension = validateExtension(file);
        String key = createKey(extension);
-        PutObjectRequest request = createRequest(file, key);
+        PutObjectRequest request = createPutRequest(file, key);


jinyoungchoi95 · 2023-08-15T15:37:46Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+        unusedImageRepository.save(
+                UnusedImage.builder()
+                           .key(key)
+                           .build()
+        );


s3Client.putObject를 넘어서 save 동작 시 문제가 발생한다면, s3에 저장된 object는 어떻게 되나요?

jinyoungchoi95 · 2023-08-15T15:45:37Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+
+    @Transactional
+    public void deleteUsingImage(final String imageUrl) {
+        if (imageUrl != null) {


null 체크를 하시는 이유가 있을까요?

jinyoungchoi95 · 2023-08-15T15:47:22Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+
+    @Scheduled(cron = "0 0 4 * * *")
+    @Transactional
+    public void deleteUnusedImages() {


요정도 사이즈가 되니 ImageService가 담기에는 책임이 너무 많아보이기는 하네요.

jinyoungchoi95 · 2023-08-15T15:50:25Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+    public void deleteUnusedImages() {
+        LocalDateTime deleteBoundary = LocalDateTime.now().minusDays(1L);
+        List<UnusedImage> unusedImages = unusedImageRepository.findAllByCreatedAtBefore(deleteBoundary);
+        unusedImageRepository.deleteAllByCreatedAtBefore(deleteBoundary);


deleteAll은 쿼리가 어떻게 날아가나요?

개인적으로는 deleteBoundary를 LocalDateTime을 받는 것보단 LocalDate만 받는건 어떨까싶긴하네요. 스케쥴 돌아가는 시점에 따라서 시분초까지 계산하기에는 누락건이 발생할 수 있을 것 같아서요.

sonarqubecloud · 2023-12-14T08:16:07Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
30.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Cyma-s

안녕하세요 주드
잊어버리신 줄 알았는데 해오셨네요 감사합니다
개인적으로 궁금한 부분 질문 남겼는데 확인해주세요 😄

Cyma-s · 2023-08-13T03:22:01Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java


-    public ImageService(final S3Client s3Client, @Value("${cloud.aws.s3.bucket}") final String bucketName) {
+    public ImageService(final S3Client s3Client, @Value("${cloud.aws.s3.bucket}") final String bucketName, unusedImageRepository unusedImageRepository) {


final 이 빠진 것 같아여 👍

Cyma-s · 2023-08-13T03:23:37Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+    }
+
+    @Transactional
+    public void deleteImageWhenReviewDeleted(final String imageUrl) {


메서드 이름 굿 👍

Cyma-s · 2023-08-13T03:24:56Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+                                  .build();
+    }
+
+    @Scheduled(cron = "0 0 4 * * *")


신기하네요 이거

Cyma-s · 2023-12-14T15:09:25Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageUploader.java

+
+import java.util.List;
+
+public interface ImageUploader {


의존성 분리 굿 👍🏻

Cyma-s · 2023-12-14T15:11:19Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/S3ImageUploader.java

+import static com.woowacourse.matzip.environment.ProfileUtil.PROD;
+
+@Profile({LOCAL, PROD})
+@Component


@Primary 같이 빈의 우선순위를 표현해줄 필요는 없나요? 아직은 없지만 여러 ImageUploader 가 생긴다면 빈 우선순위가 필요할 것 같아서요

Cyma-s · 2023-12-14T15:19:51Z

matzip-core/src/main/java/com/woowacourse/matzip/domain/image/ImageExtension.java

-                );
+    private static final String DELIMITER = ".";
+
+    public static void validateExtension(final String fileName) {


extension 이 아닌 fileName 을 매개변수로 받고, 전체적인 fileName 에 대한 검증을 수행하고 있으니 validateFileName 이 더 괜찮은 네이밍 같다는 생각이 듭니당 어떻게 생각하세요?

Cyma-s · 2023-12-14T15:20:40Z

matzip-app-external-api/src/main/java/com/woowacourse/support/config/S3Config.java

+import static com.woowacourse.matzip.environment.ProfileUtil.LOCAL;
+import static com.woowacourse.matzip.environment.ProfileUtil.PROD;
+
+@Profile({LOCAL, PROD})


LOCAL 도 포함하신 이유가 있나요?

Cyma-s · 2023-12-14T15:22:02Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageUploader.java

+
+    void deleteImage(final String imageUrl);
+
+    void deleteImages(final List<UnusedImage> unusedImages);


매개변수의 타입 자체가 UnusedImage 이기 때문에 메서드 이름도 그와 동일하게 deleteUnusedImages 로 하는 게 더 좋을 것 같아요. 매개변수 타입이 추상화된 클래스였다면 deleteImages 로 추상화해서 표현해도 될 것 같은데, 현재는 구체 클래스이다 보니 명확하게 명시해주는 건 어떨까요?

Cyma-s · 2023-12-14T15:22:45Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageUploader.java

+
+    String uploadImage(final MultipartFile file);
+
+    void deleteImage(final String imageUrl);


deleteByImageUrl 이라는 네이밍은 어떠신가요 매개변수가 Image 나 파일이 아니라서 네이밍이 조금 어색할 수도 있을 것 같아요

Cyma-s · 2023-12-14T15:24:46Z

matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/ImageService.java

+    private final ImageUploader imageUploader;
+    private final UnusedImageRepository unusedImageRepository;


의존성이 훨씬 깔끔하네요

sonarqubecloud · 2024-07-20T06:17:02Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

kevstevie · 2024-07-20T06:51:05Z

변경사항

스케쥴링에 따른 삭제 역할을 다른 클래스로 분리
async로 작동하기 때문에 transaction이 필요없다고 판단하여 eventlistner로 전환
s3 api 사용 시간동안 db 커넥션을 점유하지 않도록 @Transactional 제거
실패 가능성이 높은 s3 api 사용을 db로직보다 위에 둠으로써 파일처리와 db작업이 성공/실패를 동시에 보장할 수 있도록 순서 수정

todo
테스트..

Cyma-s

바쁜 일정 중에 기능 구현을 또 해주셨네요 👍 트랜잭션 관련해서 궁금한 게 있어 리뷰 남겼습니다. 시간 될 때 확인해주세요 감사합니다

Cyma-s · 2024-07-22T13:46:33Z

matzip-app-external-api/src/test/java/com/woowacourse/support/TestImageManager.java

+
+@Profile(TEST)
+@Component
+public class TestImageManager implements ImageManager {


Cyma-s · 2024-07-22T13:47:37Z

...-app-external-api/src/main/java/com/woowacourse/matzip/application/image/S3ImageManager.java

+    }
+
+    @Override
+    public String uploadImage(final MultipartFile file) {


만약 이미지 업로드 실패하면 어떻게 되나요?

실패하는 경우 s3에서 어떤 응답을 주는지 못찾아서 그 로직이 없는데 추가하겠습니다 감사합니다

Cyma-s · 2024-07-22T13:48:09Z

...-app-external-api/src/main/java/com/woowacourse/matzip/application/image/S3ImageManager.java

+
+    @Override
+    public void deleteImages(final List<UnusedImage> unusedImages) {
+


불필요한 공백 🫡

Cyma-s · 2024-07-22T13:49:17Z

...-app-external-api/src/main/java/com/woowacourse/matzip/application/image/S3ImageManager.java

+    }
+
+    @Override
+    public void deleteImages(final List<UnusedImage> unusedImages) {


이 메서드도 실패하는 경우에는 어떻게 처리되나요?

Cyma-s · 2024-07-22T13:50:39Z

...ip-app-external-api/src/main/java/com/woowacourse/matzip/application/image/ImageDeleter.java

+        this.unusedImageRepository = unusedImageRepository;
+    }
+
+    @Scheduled(cron = "0 0 4 * * *")


@Transactional 은 없어도 되나요?

넵 select와 delete가 한 트랜잭션으로 묶일 필요가 없다고 생각했고 시간이 오래걸리는 s3 api 사용 시간동안 db 커넥션을 물고 있지 않는게 나아서 제거 했습니다.

kevstevie added the feature label Aug 13, 2023

kevstevie requested review from Cyma-s, Ohzzi, jinyoungchoi95, jayjaehunchoi and beer-2000 August 13, 2023 03:09

kevstevie self-assigned this Aug 13, 2023

jinyoungchoi95 requested changes Aug 15, 2023

View reviewed changes

Cyma-s requested changes Dec 14, 2023

View reviewed changes

kevstevie added 14 commits July 14, 2024 21:10

feat: 사용하지 않는 이미지 삭제 기능

7f10372

refactor: 리뷰의 이미지가 없을 경우 분기 처리

4eb8e0e

refactor: S3 이미지 업로드 의존성 분리

d7cafd9

chore: spring 버전업에 따른 import 수정

e8c2e1a

feat: 이미지 삭제 역할 분리

0fcf438

refactor: image 패키지 분리

84955f0

feat: 삭제 실패시 queue로 추가

a538406

refactor: 이미지 단일삭제기능 제거

54ab58c

refactor: 삭제 조건 추가

bbb0d40

refactor: 이미지 삭제 기능 제거

58ad1c7

feat: 스케쥴 크론 추가

7c30366

refactor: event listen 구조 수정

f31471e

refactor: 이미지 업로드 트랜잭션 제거

0edeb5f

refactor: db에 저장하도록 수정

96f2673

kevstevie force-pushed the issue192 branch from 27c82c4 to 96f2673 Compare July 20, 2024 05:52

kevstevie added 3 commits July 20, 2024 14:55

refactor: profile 제거

c2a63be

refactor: test 제거

b35b92f

fix: test 의존성 주입 해소

e85f77f

Cyma-s requested changes Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 사용하지 않는 이미지 제거 기능 #193

feat: 사용하지 않는 이미지 제거 기능 #193

kevstevie commented Aug 13, 2023

jinyoungchoi95 left a comment

jinyoungchoi95 Aug 15, 2023

jinyoungchoi95 Aug 15, 2023

jinyoungchoi95 Aug 15, 2023

jinyoungchoi95 Aug 15, 2023

jinyoungchoi95 Aug 15, 2023

jinyoungchoi95 Aug 15, 2023

sonarqubecloud bot commented Dec 14, 2023

Cyma-s left a comment

Cyma-s Aug 13, 2023

Cyma-s Aug 13, 2023

Cyma-s Aug 13, 2023

Cyma-s Dec 14, 2023

Cyma-s Dec 14, 2023

Cyma-s Dec 14, 2023

Cyma-s Dec 14, 2023

Cyma-s Dec 14, 2023 •

edited

Loading

Cyma-s Dec 14, 2023

Cyma-s Dec 14, 2023

sonarqubecloud bot commented Jul 20, 2024

kevstevie commented Jul 20, 2024 •

edited

Loading

Cyma-s left a comment

Cyma-s Jul 22, 2024

Cyma-s Jul 22, 2024

kevstevie Jul 22, 2024

Cyma-s Jul 22, 2024

Cyma-s Jul 22, 2024

Cyma-s Jul 22, 2024

kevstevie Jul 22, 2024


		public ImageService(final S3Client s3Client, @Value("${cloud.aws.s3.bucket}") final String bucketName) {
		public ImageService(final S3Client s3Client, @Value("${cloud.aws.s3.bucket}") final String bucketName, unusedImageRepository unusedImageRepository) {


		void deleteImage(final String imageUrl);

		void deleteImages(final List<UnusedImage> unusedImages);


		String uploadImage(final MultipartFile file);

		void deleteImage(final String imageUrl);

		private final ImageUploader imageUploader;
		private final UnusedImageRepository unusedImageRepository;


		@Override
		public void deleteImages(final List<UnusedImage> unusedImages) {

feat: 사용하지 않는 이미지 제거 기능 #193

Are you sure you want to change the base?

feat: 사용하지 않는 이미지 제거 기능 #193

Conversation

kevstevie commented Aug 13, 2023

issue: #192

as-is

to-be

jinyoungchoi95 left a 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

sonarqubecloud bot commented Dec 14, 2023

Quality Gate passed

Cyma-s left a 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

Cyma-s Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 20, 2024

Quality Gate passed

kevstevie commented Jul 20, 2024 • edited Loading

Cyma-s left a 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

Cyma-s Dec 14, 2023 •

edited

Loading

kevstevie commented Jul 20, 2024 •

edited

Loading