-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
Conversation
확 인 해 달 라 고 |
//기본 배너 이미지일때 | ||
if(preImageFilePath.contains("partyBanner/")){ | ||
String imageFilePath = s3Uploader.fileUpload(file,"partyImage"); | ||
} else { |
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.
yes.. 저거 버킷 내부 폴더 이름이라 없으면 안됩니당
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.
너무 늦게 리뷰 해드리네요..
답변 부탁드립니다!
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){ |
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을 발생시키는 부분이 어떤 부분인가요?
데이터베이스에 접근하는 곳에서 Exception이 발생하는 부분만 try catch문을 이용해서 Exception 처리를 바꾸는것은 어떻게 생각하나요
다른 예외상황이 생겼을때도 DATABASE_ERROR로 여겨질거 같아서요
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.
db에 접근해야 하는 findPartyById랑 updateParty 부분에서 Exception이 발생하는 것 같습니다
저 두개 부분만 그럼 따로 try-catch 적용 해보겠습니당
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); |
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.
데이터베이스에서 삭제릉 담당하는 메서드는 transaction 어노테이션을이용해서 삭제중의 참조가 발생하지 않도록하는것이 좋다고 생각하는데 어떻게 생각하시나요?
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.
반영하겠습니당
@@ -171,7 +193,7 @@ public void joinValidation(Party party, User user) throws BaseException { | |||
} | |||
|
|||
|
|||
public Boolean isFullParty(Party party) { | |||
public boolean isFullParty(Party party) { |
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.
궁금해서 물어보는건데 어떤 이유로 Wrapper 클래스에서 기본 데이터타입으로 바꾼건가요?
나중을 혹시나 나중응 위해서라도 wrapper 클래스를 사용하는게 낫지 않을까요?
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.
이건.. 제가 구현한 부분이 아니라서 @kuk6933 형석쓰에게 답변을 요청하는게 나을 것 같습니다
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.
엇 원래 Boolean으로 되어있던 것을 boolean으로 바꾼 이유를 물어보신듯 합니다 !
Party party = findPartyById(partyId); | ||
party.updateParty(patchPartyReq); | ||
return new PatchPartyRes(true); | ||
public PatchPartyRes updateParty(Long partyId, PatchPartyReq patchPartyReq, MultipartFile file) throws BaseException { |
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.
이부분도 기존 updateParty 구현된거에서 MultipartFile 부분만 제가 손 본거라 Party 담당자 @kuk6933 형석스에게 물어보는 것이 좋을 것 같습니당
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.
그리고 사실 이건 제가 CS 지식 부족 이슈로 여쭤보는거지만... 트랜잭션 어노테이션을 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.
면접 질문이 나왔네요.. ㅎㅎ
제가 S3에서 이미지 path가 없을때는 정확히 어떻게 동작하는지는 잘 모르지만, 여기서는 이미지를 제거한 사이에 이미지에 접근하면 해당 이미지를 찾을 수 없을것 같아요.
없는 이미지 주소를 참조한다는것은 프론트 측에서도 재접근하도록 하거나 기본 이미지를 보여주는 방식으로도 처리할 수 있는 문제이긴 하지만, 안정적인 서버의 모습이라고는 보여지지는 않는거 같아요.
해당 상황에 대한 예외처리가 어떤 형태로든 저희 서버에서 처리를 하고 있다면 다시 생각해볼 수도 있는 문제지만 저희는 그러한 예외처리도 없는것으로 알고 있기도 해서 Transaction 을 사용하는 것이 좋다고 생각했어요.
어떻게 생각하시나요?
모임 이미지 수정 기능 및 모임 삭제 시 버킷에서 이미지를 삭제하는 기능을 구현했습니다.