-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Feat] 메뉴 폴더 저장(S3) #18
Conversation
- AWS SDK for Java 2.x 사용
…OurMenu-BE-V2 into KAN-20-feat/menu-folder
- redis, mysql password 환경변수로 관리
- S3 configuration 설정 - FileUtil으로 UUID.random() 이아닌 시간값으로 식별자 분류 - S3 로직 AwsS3Service으로 분리
- userId 컬럼추가 - index -> customIndex 변경
- JsonInclude - isSuccess JsonProperty 추가
- s3 업로드 로직 변경 (비동기 -> 비동기 + block) - request(form-data)에 대해 하나의 DTO로 병합 - repository 추가 - service 메서드 주석 추가
Test Results1 tests 1 ✅ 0s ⏱️ Results for commit d5e8015. ♻️ This comment has been updated with latest results. |
- s3 환경변수 추가 - redis 환경변수 추가
6173b56
to
5eb7f8d
Compare
- s3 join 호출 위치 수정 - url response 반환 추가
- ApiUtil successOnly 추가(response가 없는 성공 메세지)
- dto 정적 클래스 추가 - patch로 구현
- UserDetailImpl -> CusteomUserDetails
- 벌크 연산 - menuFolder update 함수reformat - response 메뉴판 수정과 동일한 객체 사용
- 인텍스 기준 내림차순 정렬
- ErrorCode(글로벌) + 메세지 조합
return s3AsyncClient.putObject(objectRequest, asyncRequestBody) | ||
.thenApply(resp -> getS3Url(name)) | ||
.exceptionally(ex -> { | ||
throw new RuntimeException("이미지 업로드중 문제가 발생하였습니다", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 CustomException이 아닌 RuntimeException을 사용한건 의도하신 부분인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처리 못한 부분입니다 수정하겠습니다!
@@ -15,4 +15,8 @@ public static <T> ApiResponse<T> success(T response) { | |||
public static ApiResponse<?> error(ErrorResponse errorResponse) { | |||
return new ApiResponse<>(false, null, errorResponse); | |||
} | |||
|
|||
public static ApiResponse<Void> successOnly() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
응답 객체가 없는 경우에 해당 메소드 호출로 통일하면 좋을 거 같습니다!
} | ||
|
||
private MenuFolder findOneByOwn(Long userId, Long menuFolderId) { | ||
MenuFolder menuFolder = findOne(menuFolderId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 findOne, findOneByOwn 메소드 두 개로 분리되어 있는데, 서로 다른 예외를 다루기 위해서인가요?? 두 개의 메소드로 분리하신 이유가 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비즈니스 로직측면에서 생각해보면 findOne 메소드를 단독으로 호출하는 경우가 없네요.. 하나로 합치는게 더 좋겠어요
import lombok.Builder; | ||
import lombok.Getter; | ||
|
||
@AllArgsConstructor(access = AccessLevel.PRIVATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
int fileExtensionIndex = originalFileName.lastIndexOf(FILE_EXTENSION_DELIMITER); | ||
String fileExtension = originalFileName.substring(fileExtensionIndex); | ||
String fileName = originalFileName.substring(0, fileExtensionIndex); | ||
String now = String.valueOf(System.currentTimeMillis()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 방식으로 파일 이름 중복을 막을 수 있군요.. 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예전에 UUID으로 만들었는데 파일이름 + 시간이 더 좋아보였어요.
menuFolderRepository.delete(menuFolder); | ||
} | ||
|
||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transactional 어노테이션을 사용함으로써 Repository에서의 save메소드와 같은 별도의 처리를 하지 않는 방식으로 이해했는데 맞을까요?? 이에 대해 간략히 설명해주시면 감사하겠습니다 :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요 서비스단에서 하나의 트랜잭션으로 묶었기 때문에 레포 단에서는 트랜잭션을 고려하지 않았어요
} | ||
|
||
@Transactional | ||
public void deleteMenuFolder(Long userId, Long menuFolderId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메뉴판을 삭제하면 기존의 메뉴판들의 index가 조정되어야 할 것 같아요. 그리고 해당 메뉴판의 이미지는 S3에 계속 존재하는 건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- index 조정은 생각하지 못했네요..
- S3 삭제는 아직 구현하지 않았어요
여러 부분에서 제가 이해가 부족하여 여쭤보는게 많은 점 양해 부탁드립니다.. 그리고 이후 CustomException 이름 짓는 컨벤션이나 ErrorCode에 대해 얘기해봐도 좋을 것 같습니당 |
ErrorCode 관련해서 애매한 부분이 있었는데, 회의때 이야기 해봐요. 저도 리뷰 보면서 배우는게 많아서 서로 계속 물어보죠 ~ 🚗 |
- ErrorCode 등록 - 리뷰 반영
- 리뷰 반영
- findOne 메소드 findOneOwn 메소드와 통합 = 리뷰 반영
- utf-8 인코딩 추가(특수문자 처리) - 일부 메소드 이름 변경 - 리뷰 반영
4878ce6
to
d5e8015
Compare
✏️ 작업 개요
⛳ 작업 분류
🔨 작업 상세 내용
💡 생각해볼 문제
객체 생성에 대해서of 와 from 정적 메소드를 추가했습니다.new
builder
of 메소드
from 메소드
등등 어떤것이 적절한 방법인지 고민하고 있습니다. 토의해보면 좋겠습니다. (builder는 캡슐화를 위반하는거 아닌가라는 생각이 드네요..)