-
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?
Changes from all commits
c5203df
95d5acc
071883e
0c93b85
9b21975
9ae3cea
d877181
c5c73ab
4453a14
5a3a140
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Random; | ||
import java.util.stream.Collectors; | ||
|
||
import static org.rf.rfserver.config.BaseResponseStatus.*; | ||
|
@@ -120,15 +121,36 @@ public GetPartyRes getParty(Long partyId) throws BaseException { | |
.build(); | ||
} | ||
|
||
public PatchPartyRes updateParty(Long partyId, PatchPartyReq patchPartyReq) throws BaseException { | ||
Party party = findPartyById(partyId); | ||
party.updateParty(patchPartyReq); | ||
return new PatchPartyRes(true); | ||
public PatchPartyRes updateParty(Long partyId, PatchPartyReq patchPartyReq, MultipartFile file) throws BaseException { | ||
try{ | ||
Party party = findPartyById(partyId); | ||
if(file != null){ | ||
String preImageFilePath = party.getImageFilePath(); | ||
//기본 배너 이미지일때 | ||
if(preImageFilePath.contains("partyBanner/")){ | ||
String imageFilePath = s3Uploader.fileUpload(file,"partyImage"); | ||
} else { | ||
Comment on lines
+129
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. yes.. 저거 버킷 내부 폴더 이름이라 없으면 안됩니당 |
||
//이전 이미지 버킷에서 삭제 | ||
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){ | ||
Comment on lines
+125
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception을 발생시키는 부분이 어떤 부분인가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. db에 접근해야 하는 findPartyById랑 updateParty 부분에서 Exception이 발생하는 것 같습니다 |
||
throw new BaseException(DATABASE_ERROR); | ||
} | ||
} | ||
|
||
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); | ||
Comment on lines
148
to
156
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 궁금해서 물어보는건데 어떤 이유로 Wrapper 클래스에서 기본 데이터타입으로 바꾼건가요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 엇 원래 Boolean으로 되어있던 것을 boolean으로 바꾼 이유를 물어보신듯 합니다 ! |
||
if (party.getUsers().size() >= party.getMemberCount()) { | ||
return true; | ||
} | ||
|
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 을 사용하는 것이 좋다고 생각했어요.
어떻게 생각하시나요?