-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor/#415 글 삭제, 수정시 게시글 이미지의 soft delete를 위해 update문이 중복해서 나지 않도록 수정함 #526
The head ref may contain hidden characters: "refactor/#415_\uAC8C\uC2DC\uAE00_\uC0AD\uC81C_\uC218\uC815\uC2DC_\uAC8C\uC2DC\uAE00_\uC774\uBBF8\uC9C0_\uC0AD\uC81C_\uCFFC\uB9AC_\uAC1C\uC120"
Conversation
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.
comment - commentImageInfo
post - postImageInfo
member - profileImageInfo
이렇게 같은 패키지로 놓고
각각의 패키지에서 image 패키지에 의존하는건 어떨까~ 생각해봅니다. 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.
빠르게 반영해주셨군요 감사합니다 께로
// todo: 이 부분 docs test에서만 쓰고 있어요 | ||
public void addPostImageInfo(final PostImageInfo postImageInfo) { | ||
this.postImageInfos.add(postImageInfo); | ||
} |
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에만 잘 저장해두면되지 Post 객체에는 저장해줄 필요가 없네요.
그래서 post 클래스에도 딱히 이미지를 메서드는 필요하지 않을 것 같아요.
하지만 docs test에 가볍게 mocking을 하려면 post에 postImageInfo를 넣어주는 작업이 필요한데, 어떻게 하는게 좋을까요 🧐
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.
생성자 주입으로 넣어주도록 수정하고, 위 메서드는 삭제했습니다 !!
postInfo의 post 정보는 null로 했어유 ㅎㅎ
먼가 postInfo의 post가 db에서 참조로 저장되기 위해서만 존재하는 늬낌,,,
@Query(value = "select * from post_image_info", nativeQuery = true) | ||
List<PostImageInfo> findAllImages(); |
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.
제가 못지운 부분을 지워주셨군요!! 감사드립니당
CommentInfoRepository, ProfileImageInfoRepository에 있는 부분도 부탁드려도 될까요(굽신굽신)
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.
수정했슴돠 !!
부모인 imageInfo는 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.
고생하셨습니다 케로상
바꾸기전이랑 바꾼 후 쿼리 비교해봤는데 잘되네염
코멘트 하나 달았는데 확인해주시면 감사하겠습니다
@@ -126,13 +124,18 @@ public PostIdResponse updatePost( | |||
final List<String> remainedImageNames = imageService.convertToStoreName(request.originalImages(), ImageType.POST); | |||
|
|||
if(isImagesEmpty(imageFilesToAdd)) { | |||
post.updateImages(remainedImageNames); | |||
final List<Long> imageIdsToDelete = post.getImageIdsToDeleteBy(remainedImageNames); | |||
postImageInfoRepository.deleteAllByIds(imageIdsToDelete); |
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.
이 부분 분기처리 안해도 될 거 같은데 어떻게 생각하나여? 케로상
지우고 테스트 해봤는데, delete할 게 없으면 어차피 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.
저도 빼보려고 지워보긴 했는데,,,
제가 지우고 테스트 했을때는 imageFilesToAdd가 비어있을 때 emptyList가 아니라서 그런지 터지더라고여???
분기가 있기는 해야할 것 같던데,,,, 호이쒸가 함 고쳐서 푸시 해주시졍!!!
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.
수고하셨어요!
soft delete를 위한 @Modifying 사용을 안전하게 하는 방법이 어떤 블로그에 적혀있어 가져왔어요. 이것만 반영해주시면 어프루브 하겠습니다 케로 !!
import org.springframework.data.jpa.repository.JpaRepository; | ||
import org.springframework.data.jpa.repository.Modifying; | ||
import org.springframework.data.jpa.repository.Query; | ||
import org.springframework.data.repository.query.Param; | ||
|
||
import java.util.List; | ||
|
||
public interface CommentImageInfoRepository extends JpaRepository<CommentImageInfo, Long> { | ||
|
||
@Modifying //todo: 옵션.. 이대로 괜찮은가? |
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.
이전 기수 선배님께서 같은 고민을 하고 남기신 글이 있어 공유드려요!
Modifying을 이용한 작업을 하면 1차 캐시와 데이터베이스 사이의 정보 불일치가 발생하기 때문에,
해당 메서드를 호출한 다음에는 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.
이 부분은 저도 고민을 했었는데여!! 👍🏻
같은 트랜잭션 내에서 다시 해당 엔티티를 조회할 일이 있다면 "JQPL로 변경된 값을 조회할 때는 1차 캐시를 비우거나 다이렉트 DB 조회를 해야합니다."
이 분도 이렇게 말씀하셧네용
현재 게시글의 수정과 삭제 이후에 같은 트랜잭션에서 게시글의 상세정보, 이미지나 댓글의 상세정보를 사용하고 있지 않아여!
return이 Void거나 id 정도만 사용되고 있습니당!! 따라서 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.
합리적이네요!
🔥 연관 이슈
📝 작업 요약
글 삭제, 수정시 게시글 이미지의 soft delete를 위해 update문이 중복해서 나지 않도록 수정함
🔎 작업 상세 설명
기존에 글의 사진을 삭제하거나 수정할 때 postImageInfo의 변경 감지를 이용하였기 때문에, update 쿼리가 중복해서 나가는 문제가 있었습니다.
post.delete() 메서드 안에서 수행해주던 작업을 서비스 레이어에서 postImageInfo레포를 이용하여 하도록 수정하였습니다 !
🌟 리뷰 요구 사항
현재 게시글 삭제를 위해 postService에서 postImageInfo레포를 사용하고 있고
postImageInfo는 post를 들고 잇기 때문에 패키지간 양방향 의존이 생겼습니다 !
(comment Image도 마찬가지)
어떻게 해결하면 좋을지 같이 생각해주십시오G