-
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]사진첨부 기능이 추가된 게시글 작성 api구현 #203
Conversation
@@ -35,6 +36,10 @@ public class Content extends BaseTimeEntity { | |||
@NotNull | |||
private String contentText; | |||
|
|||
@Setter |
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.
이 부분에 대해서 @Setter를 사용하지 않고,
pulic void updateContentImage(String contentImageUrl) {
this.contentImage = contentImageUrl ;}
과 같은 함수로 처리를 했을 때 잘 처리되지 않는다고 말씀하신 것이 맞을까요??
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.
@Setter를 사용하는 것보다는 명시적인 메소드를 통해서 처리하는 것이 더 바람직하다고 생각됩니다. 작동하는 로직은 동일하지만 가독성 측면과, 추후 유지 보수성 측면에서 메소드로 구현할 경우 추가 로직을 넣을 수 있기 때문에 그렇게 생각하고 있습니다.
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.
메소드로 구현할 수 있다면 도메인단에서 setter어노테이션은 최대한 지양하는 것이 어떨까하는 의견입니다!
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.
저도 그렇게 생각했는데 결국 동작이 같아서 유지보수랑 가독성이 측면에선 @Setter가 더 편하다고 않으신가여??
필드가 하나라서 그렇게 생각했습니다! 참고했던 주소 남겨보겠습니당
https://velog.io/@baekgom/Entity%EC%97%90%EC%84%9C-Setter-%EC%82%AC%EC%9A%A9%EC%9D%84-%EC%A7%80%EC%96%91%ED%95%98%EC%9E%90
근데 메소드말고는 다른방법은 없겠지오..
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.
만약 메소드로 구현한다면 updateImage가 아닌 setImage로 이름을 설정하려고 했었어서 더 동작이 같다고 생각이 들었었습니다!
하나의 요청이 왔을 때 설정하는거라 update보단 set에 가깝다고 생각했는데 update가 더 가깝다고 생각하시나여?
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.
유지 보수 측면의 경우 추후 한 게시물에 2장 이상의 사진이 올라간다고 할 경우 setter를 사용하는 것보다는 메소드를 사용하는 것이 적절하지 않을까 생각했습니다. 물론 세부적인 로직을 생각해보지는 않았으나 setter보다는 메소드가 활용도가 높아서요! (물론 여러 장의 사진을 올리게 될 경우 contentImage테이블을 따로 만들어야하는 생각도 있습니다.추후 기능 요구사항이 나오면 그때 의논해봐요) 가독성 측면에서는 updateContentImgae보다는 setContentImage가 더 가독성이 좋은 것 같긴 하네요:)
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.
결국 똑같은 작업을 하는 것이라면 도메인 파일 안에서 import lombok.Setter;
를 사용하는 것을 저는 지양하는 게 좋지 않을까 싶은 생각입니다. 그럼 메서드 명을 setContentImage로 하는 것은 어떤가요? 혹시 유지보수 측면에서 @Setter가 더 편할 것 같다고 말씀하셨는데 어느 부분에서 그렇게 생각하셨는지 여쭤봐도 될까요?
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.
말씀해주신대로 이미지의 다중 첨부가 가능하다면 제1정규화를 고려해서 테이블을 따로 만드는 쪽으로 생각했습니다!
추가적인 내용은 카톡으로 말씀드렸습니다~~
|
||
RequestBody requestBody = RequestBody.fromBytes(image.getBytes()); | ||
s3Client.putObject(request, requestBody); | ||
return S3_URL + key; |
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.
여기서 사용되는 S3_URL이 위에서 변수로 선언되어있는데 ( private static final String S3_URL = "https://dontbe-s3.s3.ap-northeast-2.amazonaws.com/";
) 저 변수는 Prod용 S3버킷 주소이기 때문에 현재 dev DB에 저장된 url에 접근할 수가 없습니다. S3-dev버킷에 들어가서 저장된 사진의 객체 URL과 dev DB에 저장된 URL의 차이점 확인 부탁드립니다!
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.
오우 큰일날뻔했네여! 수정해서 push했습니다 aa99e2a 확인부탁드립니다!
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.
고생 많으셨습니다~~!!
📣 Related Issue
📝 Summary
사진 첨부 기능을 추가했습니다.
🙏 Question & PR point
s3폴더링 시 contentId로 폴더를 구분였습니다. 이렇게 하게되면 추후 사진을 여러장 업로드 할 수 있게 되었을 때의 폴더를 깔끔하게 관리가 가능하다고 생각했습니다. 하지만contentImage컬럼에 @Setter를 달아야했습니다. 최대한 setter를 지양하고 싶어서 다른 방법을 생각해보려고 했지만 떠오르지 않았는데 다른 방법이 있을까요??
buillder로 Image부분만 업데이트하려고 했는데 제대로 저장이 안되더라구요..!!
그리고 추후 S3service의 upload메소드를 하나로 합치면 좋을 것 같습니다! 기존 메소드는 경로를 s3 서비스에서 만들어주고있는데 서비스단(memberService, contentService 등등)에서 경로를 건네주면 한 메소드로 재사용이 가능할 것 같습니다! 추후 리팩토링요소로 가져가면 좋을 듯 합니다 ^-^
📬 Postman