🚧 [STMT-146] 파일 업로드 기능 코드 리뷰 반영

[05AM]

💁 해결하려는 문제를 적어주세요

• 파일 업로드 기능 코드 리뷰 반영 및 리팩터링

🤔 어떤 방식으로 해결했는지 적어주세요

• BusinessException을 상속받는 NotImplementedException를 구현하여 구현되지 않은 메서드 사용 시 예외 던지기
• 파일 업로드 시 IOException 놓쳤던 로그 출력
• 유효한 이미지 확장자를 별도의 enum 타입으로 구현
• FileUtil 클래스 리팩터링: 확장자 검사를 문자열 추출에서 content type 검사로 변경
  • originalFileName에서 문자열을 추출하는 것보다는 contentType을 가져와 검사하는 것이 낫다고 판단하여 수정하였습니다.
  • null 검사 등의 로직은 동일하게 진행되고 있습니다. 혹시 추가 검증이 필요하다 생각되시면 편하게 리뷰 달아주세요!
• file upload 정상작동 테스트용 API 구현
  • 실제 서비스에서 사용되는 api는 아니지만 정상 작동을 테스트하기 위해 구현했습니다.
  • 이후 admin만 사용 가능한 api로 변경 가능하고, 사용할 일이 없을 시에는 삭제하겠습니다.

🙋 중점적으로 리뷰 했으면 하는 부분이 있다면 적어주세요

• GlobalExceptionController의 상수 WARN_LOG_MESSAGE를 가져와서 사용해도 되는지, 안되는 이유가 있다면 어떻게 구현하면 좋을지.
  • 개인적으로 생각해본 방법은 common의 Constant 클래스를 구현하는 방법을 생각했습니다.

public class GlobalExceptionHandler {

    public static final String ERROR_LOG_MESSAGE = "[ERROR] {} : {}";
    public static final String WARN_LOG_MESSAGE = "[WARN] {} : {}";

• 지금 구현한 방식으로 예외를 찍는다면 중복 코드가 많아질 것 같은데 더 좋은 방법은 없을지

catch (IOException e) {
log.warn(WARN_LOG_MESSAGE, e.getClass().getSimpleName(), e.getMessage());
log.debug(e.getMessage(), e);

throw new BusinessException(ErrorCode.UPLOAD_FILE_FAIL_ERROR);
}

• ValidImageContentType의 패키지 위치
  • 현재는 FileUtil과 관련된 파일이니 utils에 넣었는데 비즈니스 규칙과 연관된 클래스인듯도 하여 어디에 위치시켜야 할지 고민이 됩니다.

[zxcv9203]

• originalFileName에서 문자열을 추출하는 것보다는 contentType을 가져와 검사하는 것이 낫다고 판단하여 수정하였습니다.
• null 검사 등의 로직은 동일하게 진행되고 있습니다. 혹시 추가 검증이 필요하다 생각되시면 편하게 리뷰 달아주세요!

이전에도 들었던 생각이지만 contentType + 파일명의 .확장자 둘다 검증해야 할 것 같습니다. contentType을 image/jpeg인 상태더라도 .asd와 같이 임의로 확장자가 변경되어 넘어올 수 있기 때문에 둘다 검증하는게 좋을 것 같습니다 🤔

• GlobalExceptionHandler의 상수 WARN_LOG_MESSAGE를 가져와서 사용해도 되는지, 안되는 이유가 있다면 어떻게 구현하면 좋을지.

  • 개인적으로 생각해본 방법은 common의 Constant 클래스를 구현하는 방법을 생각했습니다.

public class GlobalExceptionHandler {

    public static final String ERROR_LOG_MESSAGE = "[ERROR] {} : {}";
    public static final String WARN_LOG_MESSAGE = "[WARN] {} : {}";

• 지금 구현한 방식으로 예외를 찍는다면 중복 코드가 많아질 것 같은데 더 좋은 방법은 없을지

catch (IOException e) {
log.warn(WARN_LOG_MESSAGE, e.getClass().getSimpleName(), e.getMessage());
log.debug(e.getMessage(), e);

throw new BusinessException(ErrorCode.UPLOAD_FILE_FAIL_ERROR);
}

글로벌 예외 핸들러에서 잡아서 출력해주어도 충분할 것 같습니다! 👍

• ValidImageContentType의 패키지 위치

패키지 위치는 괜찮은거 같습니다.
ValidImageContentType이라는 이름도 좋지만 Valid라는 키워드가 없어도 이해하기 충분한 것 같아서 제거해주셔도 좋을 것 같습니다 🤔

• 현재는 FileUtil과 관련된 파일이니 utils에 넣었는데 비즈니스 규칙과 연관된 클래스인듯도 하여 어디에 위치시켜야 할지 고민이 됩니다.

특정 도메인에 종속된 유틸 클래스라면 해당하는 도메인에 utils 패키지를 만들어서 관리해도 괜찮을 듯 합니다.
추가로, FileUtil의 경우 Image 관련 유틸 클래스인 것 같아 ImageUtil 이라는 이름으로 변경해도 괜찮을 것 같네요 👍

[05AM]

@zxcv9203

달아주신 피드백 확인했습니다!
마지막 항목의 FileUtil의 경우 추후 다른 유형의 파일을 처리하는 것을 염두에 두고 지은 이름이었는데 생각해보니 분리하는 쪽이 SRP에 더 부합하는 것 같습니다. 다만 ImageUtil은 이미지와 관련된 다른 처리도 연상될 가능성이 있어 다른 이름을 고안해 보겠습니다!

리뷰 반영 + 테스트용 코드를 삭제하고 다시 리뷰 요청 드리겠습니다.
파일 테스트 코드는 추후에 스터디 그룹 도메인 개발 후에 작성하겠습니다!