-
Notifications
You must be signed in to change notification settings - Fork 2
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: 이미지 업로드/수정/조회/삭제 기능 구현 #36
Conversation
…team2-BE into feature/30-image
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.
개발하느라 수고하셨습니다~! 아 그리고 aws accessKey, secretKey는 저희 application-secret.properties로 관리하고 있는데 OS 환경변수로 따로 분리하신 이유가 있으실까요?
ADD_MEMORY_TO_IMAGES_SUCCESS("IMAGE_2", "이미지에 memory를 추가하는데 성공하였습니다."), | ||
UPDATE_IMAGES_SUCCESS("IMAGE_3", "이미지 수정에 성공하였습니다"), | ||
GET_IMAGES_SUCCESS("IMAGE_4", "이미지 조회에 성공하였습니다"), | ||
DELETE_IMAGES_SUCCESS("IMAGE_5", "memory에 해당하는 이미지를 삭제하는데 성공하였습니다."); |
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개네용
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.
넴 수정하도록 하겠습니다
String imageName = originalImageName + "_" + contentType + "_" + fileSize; | ||
UUID imageUUID = UUID.nameUUIDFromBytes(imageName.getBytes()); | ||
|
||
return imageUUID.toString(); |
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.
특정 String으로 UUID를 만들면 해당 UUID에서 다시 String으로 만들 수 있을까요??
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.
없는걸로 알고 있습니다. 기존 이름을 보존시키고 싶으면 uuid변경 없이 이름 그대로 저장하거나 기존 이름을 따로 저장하는 방식으로 해야 할 것 같습니다. 일단.. 후자의 방식으로 수정하겠습니다
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.
아 굳이 비교해야 할 일이 있다면 파일명을 UUID로 바꿔서 비교하는 방식도 있겠네요 파일명(String에서 변환된 byte)이 같으면 UUID도 같을테니까요 실제 로직에서도 그렇게 하고는 있습니다.
|
||
@NotNull | ||
@Column(name = "image_url") | ||
private String imageUrl; |
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.
imageName과 imageUrl은 어차피 DB가 스네이크 케이스라 자동으로 image_name, image_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.
습관처럼 작성하게 되었습니다. 지우도록 하겠습니다
|
||
@Override | ||
public void deleteAllByIds(List<Long> ids) { | ||
imageJpaRepository.deleteAllById(ids); |
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.
지금 당장은 중요한건 아니지만 나중에 jpa repository의 deleteAllInBatch로 한 쿼리에 지우고 jpa 1차 캐시 비우게 하게 하는 것으로 하면 더 효율적일거 같습니다!
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.
넵 염두해두겠습니다.
private final ImageUploadService imageUploadService; | ||
private final ImageUpdateService imageUpdateService; | ||
private final ImageGetService imageGetService; | ||
private final ImageDeleteService imageDeleteService; |
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.
이렇게 4개로 나눠서 빈 주입하지 않고 ImageService라는 인터페이스를 만들어서 4개에 모두 상속시킨다음 사용하게 해도 좋을거 같습니다~그럼 ImageService 빈 주입 하나만으로 가능할거 같아요
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.
만약 하나의 인터페이스에 상속시키면 4개의 Image~ServiceImpl에서 ImageService의 모든 메소드의 구현이 강제되지 않나요? 그러면 4개로 나눈 의미가 없어진다고 생각합니다.
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.
아무 메소드가 없다면 컨트롤러에서 서비스 메서드 가져올 수 있는 방법이 있을까요? 예시가 있다면 보여주시면 좋을 것 같습니다.
Image image = | ||
imageRepository | ||
.findById(imageId) | ||
.orElseThrow(() -> new NotFoundException(NOT_FOUND)); |
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 클래스 내부의 Type을 static import 하면 어떤 도메인에서 NOT_FOUND가 직관적으로 보이지 않아서 그냥 보이게 하는게 낫지 않을까요??
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Service | ||
@Transactional |
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.
이건 readOnly = true 옵션 줘도 될거 같습니다
// 기존 이미지의 이미지 이름 리스트 | ||
List<String> existingImageNames = getExistingImageNames(existingImages); | ||
|
||
// 새로 들어온 이미지 이름 리스트 |
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.
알아보는데 문제 없으신가요? 없으시면 지우겠습니다
List<Image> images = new ArrayList<>(); | ||
try { | ||
for (MultipartFile multipartFile : memoryImages) { | ||
|
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.
수정하겠습니다
그냥 아예 어디 올리는거 자체에 거부감이 들어서 그렇게 했습니다. (예전에 access key 실수로 올렸다가 털린 경험이 있어서...)/.aws/credentials에 올리는 방법도 있기는 한데 그러면 개인 노트북에서 aws configure 쓰는 분들은 불편할거 같아 환경 변수로 설정을 했습니다 |
음 근데 application-secret.properties도 어디 올라가는게 아니라서 application-secret.properties혹은 OS 환경변수 둘중에 하나로 통일하는게 좋아보이네요 |
application-secret.properties가 application.yml이 참조하는 방식으로 이루어지는 건가요? |
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
aws accessKey, secretKey 같은 경우 OS 환경변수 설정에 추가하시면 되고 절대 어디 노출시키면 안됩니다