[BE] 이벤트당 이미지 업로드 수량 제한

[Arachneee]

issue

• close [BE] 이벤트당 업로드 가능한 이미지 수량 제한 #814

구현 사항

이벤트 1개에 업로드할 수 있는 이미지 수량을 10개로 제한합니다.

flow : validateImageCount -> uploadImages-> saveImages

비지니스 플로우가 Controller에 노출되어 파사드 패턴을 도입했습니다.

추가 논의 사항

현재 flow가 validateImageCount -> uploadImages-> saveImages 인데
eventService에서 validateImageCount와 saveImages로 별도의 메소드로 분리되어
외부에서 인지했을 때 saveImages전에 validate가 필요한지 인지하기 어려울 것 같습니다.

그래서 다른 방법으로

1. validateAndsaveImages -> uploadImages를 실시하고 업로드 실패시 데이터베이스만 deleteImages를 하는 방법
2. uploadImages -> validateAndsaveImages를 실시하고 업로드 실패시 deleteImages API를 날리는 방법
   이 있는데 어떻게 생각하시나요?

[github-actions]

Test Results

 24 files   24 suites   3s ⏱️
142 tests 142 ✅ 0 💤 0 ❌
146 runs  146 ✅ 0 💤 0 ❌

Results for commit 784a400.

♻️ This comment has been updated with latest results.

[kunsanglee]

안녕하세요 백호~ 작업하신 것들이 제 생각보다 더 어려운 내용이라 리뷰하는데 시간이 좀 걸렸네요 🫠
작업하시느라 고생 많았습니다.
같이 얘기해보면 좋을만 한 것들 코멘트로 남겼으니 참고해주세요.
더 얘기해봅시다 👍

[kunsanglee]

현재 flow가 validateImageCount -> uploadImages-> saveImages 인데
eventService에서 validateImageCount와 saveImages로 별도의 메소드로 분리되어
외부에서 인지했을 때 saveImages전에 validate가 필요한지 인지하기 어려울 것 같습니다.

저는 EventImageFacadeService의 필요성을 못 느끼겠어요.
EventService가 ImageService를 가지고있고, saveImages 할 때 내부에서 validateImageCount 메서드와 같은 검증 로직을 수행해주면 되는거 아닌가요?

이렇게 하면 백호가 말씀해주신 것 처럼 별도 메서드로 분리해서 인지하기 어려워지는 문제도 해결되는 것으로 보입니다.

[Arachneee]

EventImageFacadeService 를 사용한 이유는 트랜잭션 분리를 명확하게 하기 위함입니다.
imageService.uploadImages(images)은 긴 시간이 소모되는 작업으로 트랜잭션 내부에서 동작하면 안됩니다. 그래서 트랜잭션을 분리해야합니다.
EventService가 ImageService를 가지고 있는 방법을 사용하게 된다면 아래와 비슷한 구조가 될 것같습니다.

public class EventService {
    private final ImageService imageService;
    ...
    public void saveImagesAndUpload(token, imageNames) {
        validateImageCount(token, images.size());
        imageService.uploadImages(images);
        saveImages(token, imageNames);
    }

    private void validateImageCount(String token, int uploadImageCount) {
        ...
    }

    private void saveImages(String token, List<String> imageNames) {
        ...
    }
}

이런 구조가 되면 EventService의 saveImagesAndUpload 메소드에 트랜잭션이 걸려서는 안됩니다.(imageService.uploadImages(images); 가 트랜잭션 범위 안에 있으면 안되기 때문)
그런데 validateImageCount와 saveImages는 트랜잭션이 필요하죠.
하지만 프록시로 인해 내부 메소드 호출에 트랜잭션을 적용할 수 없습니다. 셀프 참조 등의 방법이 있긴하지만 복잡도가 많이 올라갑니다.

결론은 트랜잭션 분리를 확실히 하기 위해 Facade 패턴을 도입했습니다.

제가 제시한 다른 방법 중에

1. validateAndsaveImages -> uploadImages를 실시하고 업로드 실패시 데이터베이스만 deleteImages를 하는 방법
2. uploadImages -> validateAndsaveImages를 실시하고 업로드 실패시 deleteImages API를 날리는 방법

1, 2 중에서는 1번 방법이 더 좋지 않을까 생각합니다. 1번은 업로드 전에 검증을 먼저 할 수 있고 업로드에 실패해도 데이터베이스에 보상을 해줄 수 있어서 리소스가 적을 것 같습니다.

[kunsanglee]

EventService가 ImageService를 가지고 사용하는 예시에서 ImageService.uploadImages를 비동기로 처리하면 EventService.saveImagesAndUpload 메서드에서 시작한 트랜잭션이 기다리지 않고 메서드를 종료할 수 있긴 합니다. 일단은 지금대로 진행 하시죠

[Arachneee]

맞습니다. 비동기로 처리하면 바로 종료할 수 있죠. 그런데 업로드에 실패했을 때 에러 응답을 내려줄 수 없어 더 고민을 해봐야겠네요.

[kunsanglee]

이 메서드가 왜 public이어야 하는지 모르겠습니다🙃

[Arachneee]

위 답변 참고해주세요.

[kunsanglee]

먼저 현재 코드 기반에서 CompletableFuture로 이미지 업로드 하는 로직을 비동기로 10개 쓰레드가 처리하는 것으로 파악했습니다. uploadImages가 이미지 개수만큼 호출하는 uploadImage 메서드 수행중 발생하는 IOException을 catch 하고 HaengdongException으로 감싼 후에 다시 던져서 GlobalExceptionHandler에서 처리하도록 하려고 한 것 으로 이해했습니다. 하지만 CompletableFuture는 비동기 작업을 수행할 때 발생하는 예외를 CompletionException으로 래핑하여 전달합니다. 그래서 래핑된 CompletionException 안에 원인으로 HaengdongException이 들어갈 것 이고 결국 GlobalExcpetionHandler가 처리하지 못하게 됩니다.

그리고 정책적인 측면에서 나머지 팀원들과 더 얘기해봐야 하겠지만 백호가 생각하기에 이미지 10개를 업로드 하는 중에 1개 이상 실패한 경우 업로드에 성공한 이미지들은 어떻게 처리할 생각인지 궁금합니다. 제가 파악한 의도가 맞다면 현재 코드 기반으로는
어느 하나라도 업로드에 실패하면 eventImageRepository에 저장하지 않도록 한 것 같은데 맞나요?

만약 특정 이미지 업로드 실패가 전체 프로세스를 중단시키지 않도록 하고 싶다면, exceptionally를 사용하여 각 CompletableFuture에서 예외를 개별적으로 처리할 수 있습니다. 그리고 exceptionally를 사용하여 예외를 처리했기 때문에 아래에 있는 getFuture 메서드도 사용하지 않게 됩니다.

List<CompletableFuture<String>> futures = images.stream()
                .map(image -> CompletableFuture.supplyAsync(() -> uploadImage(image), executorService)
                        .exceptionally(ex -> {
                            log.error("Failed to upload image: {}", image.getOriginalFilename(), ex);
                            return null;
                        }))
                .toList();

        CompletableFuture<List<String>> result = CompletableFuture.allOf(futures.toArray(new CompletableFuture[0]))
                .thenApply(v -> futures.stream()
                        .map(CompletableFuture::join)
                        .filter(Objects::nonNull)
                        .toList());

        return result.join();

이렇게 하더라도 s3에 업로드 성공했으나, DB에 저장시 실패한 경우 양쪽의 저장소가 정합성이 맞지 않는 문제가 발생할 수 있기 때문에 오늘 회의에서 백호가 언급해주셨던 보상 로직이 필요할 것 같네요. 아니면 s3 업로드에 실패한 이미지들을 재시도 처리 하는 방법이 될 수도 있을 것 같아요. 그렇게 하면 MultipartFile 기반으로 InputStream을 사용하여 메모리를 차지하지 않을 수 있던 장점이 사라지겠네요 🤔

[Arachneee]

정확히 이해하셨습니다. 👍

래핑된 CompletionException 안에 원인으로 HaengdongException이 들어갈 것 이고 결국 GlobalExcpetionHandler가 처리하지 못하게 됩니다.

네 맞습니다. 이 부분은 try-catch를 통해 GlobalExcpetionHandler에서 처리할 수 있도록 변경해보겠습니다.

백호가 생각하기에 이미지 10개를 업로드 하는 중에 1개 이상 실패한 경우 업로드에 성공한 이미지들은 어떻게 처리할 생각인지 궁금합니다. 제가 파악한 의도가 맞다면 현재 코드 기반으로는
어느 하나라도 업로드에 실패하면 eventImageRepository에 저장하지 않도록 한 것 같은데 맞나요?

네 맞습니다. 현재는 하나가 실패하면 저장하지 않도록 했습니다. 이 부분은 1개가 실패하면 성공한 이미지는 삭제하는 로직을 비동기로 호출하도록 하는 방안 등을 고려해야할 것 같습니다.
이부분은 다음 PR 이미지 예외 처리 부분에서 적용해보겠습니다.

[kunsanglee]

고정으로 10개 스레드를 사용하는 근거는 한 행사의 이미지 업로드 제한이 10장이라 그런건가요?? 😀

[Arachneee]

넵 10개 제한이라 10개로 지정했습니다.

[Arachneee]

이미지 업로드 예외시에 HaengdongException 을 반환 할 수 있도록 수정했습니다.

데이터 정합성에 대한 부분은 로직 순서를 변경하거나 재시도 로직, 배치 처리를 활용해서 맞춰봐야할 것 같아요.

꼼꼼하게 리뷰해주셔서 감사합니다! 😀