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

모임 이미지 수정 및 모임 삭제 시 버킷에서 이미지 삭제 #128

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lreowy
Copy link
Contributor

@lreowy lreowy commented Aug 22, 2023

모임 이미지 수정 기능 및 모임 삭제 시 버킷에서 이미지를 삭제하는 기능을 구현했습니다.

@lreowy lreowy linked an issue Aug 22, 2023 that may be closed by this pull request
@lreowy lreowy self-assigned this Aug 24, 2023
@lreowy
Copy link
Contributor Author

lreowy commented Sep 20, 2023

확 인 해 달 라 고

Comment on lines +129 to +132
//기본 배너 이미지일때
if(preImageFilePath.contains("partyBanner/")){
String imageFilePath = s3Uploader.fileUpload(file,"partyImage");
} else {
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
Contributor Author

Choose a reason for hiding this comment

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

yes.. 저거 버킷 내부 폴더 이름이라 없으면 안됩니당

Copy link
Member

@jaewonLeeKOR jaewonLeeKOR left a comment

Choose a reason for hiding this comment

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

너무 늦게 리뷰 해드리네요..
답변 부탁드립니다!

Comment on lines +125 to +143
try{
Party party = findPartyById(partyId);
if(file != null){
String preImageFilePath = party.getImageFilePath();
//기본 배너 이미지일때
if(preImageFilePath.contains("partyBanner/")){
String imageFilePath = s3Uploader.fileUpload(file,"partyImage");
} else {
//이전 이미지 버킷에서 삭제
String fileKey = s3Uploader.changeFileKeyPath(preImageFilePath);
s3Uploader.deleteFile(fileKey);
//새 이미지 업데이트
String imageFilePath = s3Uploader.fileUpload(file,"partyImage");
party.updateImageUrl(imageFilePath);
}
}
party.updateParty(patchPartyReq);
return new PatchPartyRes(true);
} catch (Exception e){
Copy link
Member

Choose a reason for hiding this comment

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

Exception을 발생시키는 부분이 어떤 부분인가요?
데이터베이스에 접근하는 곳에서 Exception이 발생하는 부분만 try catch문을 이용해서 Exception 처리를 바꾸는것은 어떻게 생각하나요
다른 예외상황이 생겼을때도 DATABASE_ERROR로 여겨질거 같아서요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

db에 접근해야 하는 findPartyById랑 updateParty 부분에서 Exception이 발생하는 것 같습니다
저 두개 부분만 그럼 따로 try-catch 적용 해보겠습니당

Comment on lines 148 to 156
public DeletePartyRes deleteParty(Long partyId) throws BaseException {
Party party = partyRepository.findById(partyId)
.orElseThrow(() -> new BaseException(INVALID_PARTY));
String imageFilePath = party.getImageFilePath();
String fileKey = s3Uploader.changeFileKeyPath(imageFilePath);
s3Uploader.deleteFile(fileKey);
deleteUserParty(party.getUsers());
partyRepository.delete(party);
return new DeletePartyRes(partyId);
Copy link
Member

Choose a reason for hiding this comment

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

데이터베이스에서 삭제릉 담당하는 메서드는 transaction 어노테이션을이용해서 삭제중의 참조가 발생하지 않도록하는것이 좋다고 생각하는데 어떻게 생각하시나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영하겠습니당

@lreowy lreowy added the 🛠️ 구현 개선 구현 개선 사항에 관련된 내용입니다. label Oct 7, 2023
@@ -171,7 +193,7 @@ public void joinValidation(Party party, User user) throws BaseException {
}


public Boolean isFullParty(Party party) {
public boolean isFullParty(Party party) {
Copy link
Member

Choose a reason for hiding this comment

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

궁금해서 물어보는건데 어떤 이유로 Wrapper 클래스에서 기본 데이터타입으로 바꾼건가요?
나중을 혹시나 나중응 위해서라도 wrapper 클래스를 사용하는게 낫지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이건.. 제가 구현한 부분이 아니라서 @kuk6933 형석쓰에게 답변을 요청하는게 나을 것 같습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

엇 원래 Boolean으로 되어있던 것을 boolean으로 바꾼 이유를 물어보신듯 합니다 !

Party party = findPartyById(partyId);
party.updateParty(patchPartyReq);
return new PatchPartyRes(true);
public PatchPartyRes updateParty(Long partyId, PatchPartyReq patchPartyReq, MultipartFile file) throws BaseException {
Copy link
Member

Choose a reason for hiding this comment

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

해당 이미지 제거 후 재 업로드를 진행하는 방식인가보네요
트랜잭션 어노테이션 사용하는게 낫지 않은가요?
따로 사용하지 않은 이유가 있을까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이부분도 기존 updateParty 구현된거에서 MultipartFile 부분만 제가 손 본거라 Party 담당자 @kuk6933 형석스에게 물어보는 것이 좋을 것 같습니당

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그리고 사실 이건 제가 CS 지식 부족 이슈로 여쭤보는거지만... 트랜잭션 어노테이션을 update 시 사용하지 않으면 어떤 문제가 발생할 여지가 있나요? 트랜잭션 어노테이션 사용을 중요하게 여기시는 것 같아 여쭤봅니당

Copy link
Member

Choose a reason for hiding this comment

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

면접 질문이 나왔네요.. ㅎㅎ
제가 S3에서 이미지 path가 없을때는 정확히 어떻게 동작하는지는 잘 모르지만, 여기서는 이미지를 제거한 사이에 이미지에 접근하면 해당 이미지를 찾을 수 없을것 같아요.
없는 이미지 주소를 참조한다는것은 프론트 측에서도 재접근하도록 하거나 기본 이미지를 보여주는 방식으로도 처리할 수 있는 문제이긴 하지만, 안정적인 서버의 모습이라고는 보여지지는 않는거 같아요.
해당 상황에 대한 예외처리가 어떤 형태로든 저희 서버에서 처리를 하고 있다면 다시 생각해볼 수도 있는 문제지만 저희는 그러한 예외처리도 없는것으로 알고 있기도 해서 Transaction 을 사용하는 것이 좋다고 생각했어요.
어떻게 생각하시나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ 구현 개선 구현 개선 사항에 관련된 내용입니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

모임 이미지 수정 및 삭제
3 participants