-
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/#289] 배너 이미지 pre-sigend url 조회 및 배너 생성 API 구현 #292
Conversation
|
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.
작업 고생해셨어요!
제가 미처 구현치 못했던 Exception Handler나 도메인 검증 로직 대신 구현까지 해주셔서 감사드립니다!!
우선 본 프로젝트에서는 와일드 카드에 대해서 지난 기수에는 지양하자고 구두로 합의하긴 했어서
지키면 좋겠지만 강하게 제약하진 않았습니다!!
저는 본 프로젝트에서 작업할 때도 물론 최대한 와일드카드를 지양하려고 노력합니다만 과거 구현체들에서 와일드카드가 적발되는 것도 사실이니
작업하시면서 편하신 방향으로 진행하거나 컨벤션을 이번에 명확하게 정의하는 것도 좋을 것 같아요~~
(의견 궁금합니다!! @sung-silver @hyunw9 )
operation-api/src/main/java/org/sopt/makers/operation/web/banner/api/BannerApiController.java
Outdated
Show resolved
Hide resolved
; | ||
|
||
private final String value; | ||
private final String location; |
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.
제가 Enum static 메서드를 무지성 복붙을 한 정황이 포착되었네요 ㅋㅋㅋㅋㅋㅋ 죄송합니다..
P3
s3 path가 ContentType
이라는 Enum 내부에location
이라는 필드로 관리되는 것에 대해서
외부적인 환경 측면의 의미를 값이 포함되어 있어 우려가 되긴합니다!!
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.
동규님 리뷰를 받고, contentType에 따라 폴더 경로를 결정할 수 있도록 로직을 분리하다보니 오히려 복잡성이 증가하는 느낌이 들었습니다!
현재 s3Service에서는 호출되는 클래스로부터 버킷이름과 파일 이름을 전달받고 있는데요, 이 과정에서 S3Service 내부에 contentType에 대한 enum을 검사하여 경로를 결정하게 하는 것이 도메인에 대한 의존성이 생긴다고 느껴졌습니다.
(content의 value를 이용하여 경로를 결정 -> 값을 결정하기 위해 물어보는 대상이 어쩌피 ContentType Enum 객체가 됨)
물론 지금 저희 구조에서 의존성을 완벽하게 모두 분리할수는 없겠지만요,, 이대로 가는건 어떨까요?!
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.
좋아요!!
충분히 고민하고 근거를 가지며 결정을 했으니 좋은 것 같습니다!!
operation-domain/src/main/java/org/sopt/makers/operation/banner/domain/PublishPeriod.java
Show resolved
Hide resolved
우하하!! 제가 놓친 부분들인 것 같은데 너무너무 감사합니다!!!!
확인 완료...! 근데 이렇게 되어있었으면 기존에도 작동을 안하고 있었던 건가...? |
저도.. 그건 모르겠네요 이벤트 브릿지에서도 이 부분을 사용하는 것 같았는데 url 발급이 안되서 에러를 확인하니까 저런 상황이었습니다,,, ㅋㅋㅋㅋㅋㅋ |
operation-api/src/main/java/org/sopt/makers/operation/web/banner/api/BannerApiController.java
Outdated
Show resolved
Hide resolved
; | ||
|
||
private final String value; | ||
private final String location; |
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.
동규님 리뷰를 받고, contentType에 따라 폴더 경로를 결정할 수 있도록 로직을 분리하다보니 오히려 복잡성이 증가하는 느낌이 들었습니다!
현재 s3Service에서는 호출되는 클래스로부터 버킷이름과 파일 이름을 전달받고 있는데요, 이 과정에서 S3Service 내부에 contentType에 대한 enum을 검사하여 경로를 결정하게 하는 것이 도메인에 대한 의존성이 생긴다고 느껴졌습니다.
(content의 value를 이용하여 경로를 결정 -> 값을 결정하기 위해 물어보는 대상이 어쩌피 ContentType Enum 객체가 됨)
물론 지금 저희 구조에서 의존성을 완벽하게 모두 분리할수는 없겠지만요,, 이대로 가는건 어떨까요?!
Related Issue 🚀
Work Description ✏️
pre signed url을 통한 이미지 업로드도 확인 완료했습니다
배너 생성 API도 확인 완료했습니다
배너 수정을 위해 S3에서 이미지를 삭제하는 로직을 미리 구현했습니다
동규오빠 작업 머지 이후의 변경사항
PR Point 📸