Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(file): 파일 도메인 리팩토링 #98

Merged
merged 5 commits into from
May 2, 2024
Merged

Conversation

NameIsUser06
Copy link
Member

@NameIsUser06 NameIsUser06 commented Apr 29, 2024

image 도메인과 S3 도메인을 file 도메인으로 변경 / 병합 하였습니다.

  • 로직상 변경은 없으며 구조적인 리팩토링만 진행하였습니다

고민한 부분


R2에서 파일을 가져오는 것과 로컬에서 가져오는 것을 R2로 묶지 않았습니다.

  • 기존 로컬에서 파일을 가져오는 코드만 필요하며 작동에 문제가 없고 코드 양이 적습니다.
  • 현재 R2로 파일을 모두 묶어 처리해야할 문제가 없기 때문에 상황이 생겼을 때 처리하는 것이 좋을 것 같습니다.

commandFileService에 대한 테스트코드를 작성하지 않았습니다.

  • uploadFile 함수는 R2에 파일을 저장하고 url을 만들어 반환하는데 테스트를 위한 롤백 처리가 어렵기때문에 테스트를 작성하지 않았습니다.
    • 실제 R2에 저장하지 않고 진행하는 테스트는 Mock 어노테이션을 통한 객체를 이용해야합니다
    • 하지만 Mock을 이용한다면 특정 함수가 반환하는 값을 테스트 코드에서 지정하기 때문에 테스트를 해야할 근거가 사라진다고 생각했습니다.
  • MultipartFile 형태의 테스트 데이터를 fixtureMonkey를 통해 생성하기가 매우 까다롭습니다
    • 테스트용 MultipartFile은 MockMultipartFile 클래스를 이용합니다.
    • file.jpg와 같은 originalFilename 형식을 fixtureMonkey를 통해 만드는 것이 어렵습니다
    • originalFilename에 맞는 contentType을 fixtureMonkey를 통해 만드는 것이 어렵습니다

위와 같은 문제로 fixtureMonkey를 통한 다양한 경우의 테스트 데이터 생성과 데이터 롤백에 대한 어려움이 있는 도메인이라 판단되었습니다


@NameIsUser06 NameIsUser06 added the 🔨 Refactor There is something to refactor label Apr 29, 2024
@NameIsUser06 NameIsUser06 self-assigned this Apr 29, 2024
@NameIsUser06 NameIsUser06 linked an issue Apr 29, 2024 that may be closed by this pull request
import com.project.bumawiki.global.s3.controller.dto.ImageResponse;
import com.project.bumawiki.global.s3.service.CommandImageService;
import com.project.bumawiki.domain.file.presentation.dto.R2FileResponseDto;
import com.project.bumawiki.domain.file.service.CommandFileService;

import lombok.RequiredArgsConstructor;

@RestController
@RequiredArgsConstructor
@RequestMapping("/api/s3")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엔드포인트도 r2로 통일하는게 좋지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 수정하겠습니다

Comment on lines 38 to 44
auth:
jwt:
header: ${HEADER}
secret: ${SECRET}
accessExp: ${ACCESS_EXP}
refreshExp: ${REFRESH_EXP}
prefix: ${PREFIXES}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 빠져도 되나요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix가 변경되면서 자동으로 안쓰는 부분이 지워지고 올라간거같네요.
롤백처리하겠습니당

qlido
qlido previously approved these changes Apr 30, 2024
Copy link
Member

@qlido qlido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다.
전체적으로 S3 -> R2로 네이밍을 변경하셨는데
현재 R2는 S3 API에 대해서 R2 엔드포인트를 연결하여 사용 중입니다.
그래서 AmazonS3가 좀 더 상위 타입에 가까운 느낌이에요.

저는 S3 네이밍을 유지하는편이 더 좋다고 생각해요. R2자체가 S3에 의존하고 있고 또 서로 빠르게 교체할 수 있어서, 추상화처럼 S3 인터페이스를 쓰고 R2 구현체를 사용하는 느낌이 되었으면합니다

Comment on lines 33 to 35
return R2FileResponseDto.from(
commandFileService.uploadFile(file)
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new 대신 from을 사용하신 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new를 바로 사용하는게 더 좋을 것 같아 수정하겠습니다

@NameIsUser06
Copy link
Member Author

좋은 코멘트 감사합니다.
말씀하신 것처럼 실제 코드에서도 S3 API를 통해서 엔드포인트에게 연결중입니다.
S3 인터페이스를 통한 R2 구현체를 사용하는 것도 좋은 방법인 것 같아요.
하지만 S3 -> R2로의 네이밍 변경은 개발자에게 현재 코드가 어떤 서비스를 사용하고 있는지 더 명확하게 전달해준다고 생각해요.

Copy link
Member

@jacobhboy jacobhboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흐하하 수고하셨습니다! 정리하시는 실력이 정말 많이 느신 것 같아요. 읽으면서 왜 이렇게 하셨는지 바로바로 공감할 수 있었습니다! 앞으로도 열심히 고민하면서 좋은 결과를 내셨으면 좋겠습니다!!

@NameIsUser06 NameIsUser06 enabled auto-merge May 2, 2024 00:42
@NameIsUser06 NameIsUser06 disabled auto-merge May 2, 2024 00:42
@NameIsUser06 NameIsUser06 merged commit a6de4c0 into master May 2, 2024
0 of 2 checks passed
@NameIsUser06 NameIsUser06 deleted the refactor/#96 branch May 2, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 Refactor There is something to refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(file): file 도메인 리팩토링
4 participants