refactor: MomentImage update 시 수동 삭제 적용 #591 #593
Conversation
Test Results 34 files 34 suites 6s ⏱️ Results for commit f7d10ce. ♻️ This comment has been updated with latest results. |
🌻Test Coverage Report
|
|
|
||
| private boolean contains(MomentImage momentImage) { | ||
| return this.images.stream() | ||
| .anyMatch(image -> image.getImageUrl().equals(momentImage.getImageUrl())); |
There was a problem hiding this comment.
| .anyMatch(image -> image.getImageUrl().equals(momentImage.getImageUrl())); | |
| .anyMatch(image -> image.isSame(momentImage)); |
| }); | ||
| } | ||
|
|
||
| public List<Long> findRemovalImageIds(MomentImages newMomentImages) { |
There was a problem hiding this comment.
MomentImages 내부에서 서비스 로직으로 삭제되어야 할 MomentImage 리스트를 내보내야 하는데,
이 과정(findRemovalImageIds 메서드)에서 내부적으로 List를 remove 한 후 내보내는 것이 나을지 아니면,
update가 이루어질때 내부적으로 remove가 이루어지는게 더 나을지 고민
-> 현재는 update 내부에서 이루어지게 구성했습니다.
전자로 진행하게 된다면 update 과정(remove, add)이 2개로 분리되어 진행이 되기 때문에 휴먼에러를 통해 remove를 안하고 새로운 이미지들만을 add하는 과정이 있을 수 있음
후자로 진행하면 하나의 update 메서드를 통해 remove되고 add되는거라 로직이 밖에서 봤을 때 깔끔한데, 삭제되어야 할 List를 찾는 과정을 update에서도 진행하고, findRemovalImageIds 에서도 진행하게 됨.
삭제되어야 할 List를 찾는 과정을 update에서도 진행하고, findRemovalImageIds 에서도 진행하게 됨.가 무엇을 말하는건지 잘 이해하지 못했어요. 다시 설명해주실 수 있을까요?
우선 현재 코드 기준으로, findRemovalImageIds()는 객체지향적이지 않은 것 같아요!
MemoryImages 입장에서는 포함하고 있지 않은 이미지들을 걸러주기 때문에 findImagesExcludedFrom와 같은 네이밍은 어떻게 생각하시나요? 반환형도 List 보다는 List가 더 객체지향적으로 다가와요!
There was a problem hiding this comment.
이미 내부적으로 그러한 메서드를 쓰고 있네요! 외부에서는 findImagesNotPresentIn을 사용하는게 더 객체지향적일 것 같아요!
There was a problem hiding this comment.
삭제되어야 할 List를 찾는 과정을 update에서도 진행하고, findRemovalImageIds 에서도 진행하게 됨.가 무엇을 말하는건지 잘 이해하지 못했어요. 다시 설명해주실 수 있을까요?
@Transactional
public void updateMomentById(
long momentId,
MomentRequest momentRequest,
Member member
) {
Moment moment = getMomentById(momentId);
validateMemoryOwner(moment.getMemory(), member);
Memory targetMemory = getMemoryById(momentRequest.memoryId());
validateMemoryOwner(targetMemory, member);
Moment newMoment = momentRequest.toMoment(targetMemory);
MomentImages oringMomentImages = moment.getMomentImages();
List<Long> removalImageIds = oringMomentImages.findRemovalImageIds(newMoment.getMomentImages());
// 기존 MomentImages에서 삭제되어야 할 이미지들의 id를 가지고 오는 과정에서 내부적으로 가지고 있는 List<MomentImage> images에서 삭제되어야 할 이미지들을 삭제하지 않고 무엇을 삭제해야하는지만 반환하고 있습니다.
momentImageRepository.deleteAllByIdInBulk(removalImageIds);
moment.update(newMoment);
}
// MomentImages
public List<Long> findRemovalImageIds(MomentImages newMomentImages) {
List<MomentImage> momentImages = findImagesNotPresentIn(newMomentImages); // 해당 과정에서 List.remove()와 같은 행동을 하지 않습니다.
return momentImages.stream()
.map(MomentImage::getId)
.toList();
}그렇기에 update를 진행할 때와 findRemovalImageIds 를 진행할 때 모두 stream으로 List를 순회하며 어떠한 것을 삭제할지 판단하는 연산이 수행되어 같은연산을 2번 수행하게 됩니다.
이와 같은 연산을 한번만 수행될 수 있게끔하기 위해 findRemovalImageIds을 진행할 때 내부적으로 가지고있는 List<MomentImage>에서 삭제되어야 할 것들을 remove를 통해 삭제하여 update 메서드가 호출될 때에는 내부적으로 삭제되어야할 것들을 찾는 메서드를 호출하지 않아도 되어 앞서진행했던 동일한 연산을 2번수행하는 것을 한번만 수행하는 것으로 바뀔 수 있습니다. 하지만 이 과정은 무조건 findRemovalImageIds를 호출한 뒤 update를 호출하여야 애플리케이션 딴에서 새롭게 갱신되는 이미지들만을 가질 수 있게 됩니다.
가장 이상적인 것은 삭제되어야할 MomentImage들을 반환하면서 update가 이루어지는 형태가 되어야 깔끔할 것 같은데, 메서드 명이 잘 생각이 나지 않네요 🥲
...
List<MomentImage> removalImageIds = oringMomentImages.updateAndfindRemovalImages(newMoment);
momentImageRepository.deleteAllByIdInBulk(removalImageIds);
}외부에서는 findImagesNotPresentIn을 사용하는게 더 객체지향적일 것 같아요!
Repository에서는 List<Long> 형태로 받아야 하기에 Domain -> Service로 넘겨줄 때 객체로 넘겨주게 된다면 Service에서 Long id로 다시 매핑해야하는 번거로움이 있을 것 같아 Ids를 넘겨주는 방식을 구성했었습니다. 사실 Domain은 Repository에 어떤 방식으로 넘겨지든 고려하지 않아야 하는 것이 더 맞는것 같아요! 수정했습니당
| Moment updatedMoment = momentRequest.toMoment(targetMemory); | ||
| moment.update(updatedMoment); | ||
| Moment newMoment = momentRequest.toMoment(targetMemory); | ||
| MomentImages oringMomentImages = moment.getMomentImages(); |
There was a problem hiding this comment.
오타가 있네요 !!
originMomentImages~~
There was a problem hiding this comment.
놓친 테스트가 없는지 한 번 확인 부탁드립니다! (필요없다고 판단하시면 추가하지 않으셔도 됩니다!)
There was a problem hiding this comment.
@Embedded
private MomentImages momentImages = new MomentImages();아~~~주 뒷북이지만 초기화해놓고 나중에 addAll()로 추가한 이유가 궁금해요!
addAll()이 protected이길래 타고 왔더니 초기화를 미리 하더군용!
There was a problem hiding this comment.
addAll 메서드를 사용하기 위해 미리 인스턴스화를 시켜놓은 다음에
image들을 추가하는 형태로 구성하기위해 했었던 것 같아요!
There was a problem hiding this comment.
MomentImageService를 만들어서 MomentService에서 참조하는 방식으로 구현할 수 있을 것 같아, 직접 구현하면서 시도하는 중입니다. 이렇게 하면 아래 문제들이 해결될 것 같은 느낌이 들어요. 한번 같이 고민해보면 좋을 것 같습니다.
1
MomentImages 내부에서 서비스 로직으로 삭제되어야 할 MomentImage 리스트를 내보내야 하는데,
이 과정(findRemovalImageIds 메서드)에서 내부적으로 List를 remove 한 후 내보내는 것이 나을지 아니면,
update가 이루어질때 내부적으로 remove가 이루어지는게 더 나을지 고민
-> 현재는 update 내부에서 이루어지게 구성했습니다.
전자로 진행하게 된다면 update 과정(remove, add)이 2개로 분리되어 진행이 되기 때문에 휴먼에러를 통해 remove를 안하고 새로운 이미지들만을 add하는 과정이 있을 수 있음
후자로 진행하면 하나의 update 메서드를 통해 remove되고 add되는거라 로직이 밖에서 봤을 때 깔끔한데, 삭제되어야 할 List를 찾는 과정을 update에서도 진행하고, findRemovalImageIds 에서도 진행하게 됨.
2
MomentImages 내부 메서드 findRemovalImageIds 테스트를 어떻게 해야할 지 고민입니다. 현재는 id로 비교해야하는 과정이라 DB의 저장이 이루어져야 id 비교검증이 가능한 상태입니다. 또한 해당 메서드 내부에서 사용되는 단일 메서드는 update 메서드를 통해 내부적으로 정상작동이 검증되었기에 테스트를 진행해야 할 지 고민이 되기도 합니다. 따라서 우선 테스트는 구성안했습니다.
해당 부분에 대해서 의견 부탁드려요!!
|
이렇게 // MomentImageService.java
// new
public class MomentImageService {
private final MomentImageRepository momentImageRepository;
public void updateImages(MomentImages currentMomentImages, Moment newMoment) {
MomentImages newMomentImages = newMoment.getMomentImages();
List<Long> removalImageIds = currentMomentImages.findRemovalImageIds(newMomentImages);
deleteImages(removalImageIds);
currentMomentImages.updateImages(newMomentImages, newMoment);
}
public void deleteImages(List<Long> momentIds) {
if (momentIds.isEmpty()) {
return;
}
momentImageRepository.deleteAllByMomentIdInBulk(momentIds);
}
}// MomentService.java
@Transactional
public void updateMomentById(
long momentId,
MomentRequest momentRequest,
Member member
) {
Moment moment = getMomentById(momentId);
validateMemoryOwner(moment.getMemory(), member);
Memory targetMemory = getMemoryById(momentRequest.memoryId());
validateMemoryOwner(targetMemory, member);
Moment newMoment = momentRequest.toMoment(targetMemory);
// change
momentImageService.updateImages(moment.getMomentImages(), newMoment);
moment.update(newMoment);
}
@Transactional
public void deleteMomentById(long momentId, Member member) {
momentRepository.findById(momentId).ifPresent(moment -> {
validateMemoryOwner(moment.getMemory(), member);
commentRepository.deleteAllByMomentIdInBulk(List.of(momentId));
// change
momentImageService.deleteImages(List.of(momentId));
momentRepository.deleteById(momentId);
});
}// MomentImages.java
// new
public void updateImages(MomentImages newMomentImages, Moment moment) {
List<MomentImage> toAdd = newMomentImages.findImagesNotPresentIn(this);
List<MomentImage> toRemove = newMomentImages.findImagesNotPresentIn(this);
toAdd.forEach(image -> {
images.add(image);
image.belongTo(moment);
});
images.removeAll(toRemove);
} |
|
이번에 호티가 수정한 부분이 아닐 수 있지만 코드를 살펴보다가 발견한건데, 받는 파라미터는 // MomentService.java
@Transactional
public void deleteMomentById(long momentId, Member member) {
momentRepository.findById(momentId).ifPresent(moment -> {
validateMemoryOwner(moment.getMemory(), member);
commentRepository.deleteAllByMomentIdInBulk(List.of(momentId));
momentImageRepository.deleteAllByMomentIdInBulk(List.of(momentId));
momentRepository.deleteById(momentId);
});
} |
이 부분은 |
좋은 의견 감사합니다 폭포! |
아 찾아보니 아래 메서드에서도 해당 메서드를 사용하네요! 여기서 // MemoryService.java
private void deleteAllRelatedMemory(long memoryId) {
List<Long> momentIds = momentRepository.findAllByMemoryId(memoryId)
.stream()
.map(Moment::getId)
.toList();
momentImageRepository.deleteAllByMomentIdInBulk(momentIds);
commentRepository.deleteAllByMomentIdInBulk(momentIds);
momentRepository.deleteAllByMemoryIdInBulk(memoryId);
memoryMemberRepository.deleteAllByMemoryIdInBulk(memoryId);
} |
가장 중요한걸 말씀을 안드렸었네요. 😓 |
There was a problem hiding this comment.
호티 남겨주신 코멘트 확인했습니다! 수고 많으셨어요!
다만,
Repository에서는 List 형태로 받아야 하기에 Domain -> Service로 넘겨줄 때 객체로 넘겨주게 된다면 Service에서 Long id로 다시 매핑해야하는 번거로움이 있을 것 같아 Ids를 넘겨주는 방식을 구성했었습니다. 사실 Domain은 Repository에 어떤 방식으로 넘겨지든 고려하지 않아야 하는 것이 더 맞는것 같아요! 수정했습니당
이 부분에 대한 커밋이 없는 것 같은데, 확인 부탁드려요! 첫 리뷰 요청 당시 커밋만 보입니다 ㅜㅜ 우선은 apporve 남겨놓겠습니다:)
⭐️ Issue Number
🚩 Summary
기존에 #569 해당 PR을 통해
양방향을 끊으면서 서비스로직이 복잡해졌고, BulkInsert를 도입하면서 JPA를 사용할 수 없고 JDBCTemplate으로 구성해야 하는데 비용을 고려해 보았을 때 BulkInsert를 통한 성능적 이점이 미비하여 적용하지 않기로 결정
따라서
MomentImage에 수동 삭제만 적용수정 사항
MomentImageRepository안에 테스트에서만 사용되는Optional<MomentImage> findFirstByMomentId(long momentId);메서드 삭제 진행🛠️ Technical Concerns
1MomentImages 내부에서 서비스 로직으로 삭제되어야 할
MomentImage리스트를 내보내야 하는데,이 과정(
findRemovalImageIds메서드)에서 내부적으로List를remove한 후 내보내는 것이 나을지 아니면,update가 이루어질때 내부적으로remove가 이루어지는게 더 나을지 고민-> 현재는 update 내부에서 이루어지게 구성했습니다.update과정(remove, add)이 2개로 분리되어 진행이 되기 때문에 휴먼에러를 통해remove를 안하고 새로운 이미지들만을add하는 과정이 있을 수 있음update메서드를 통해remove되고add되는거라 로직이 밖에서 봤을 때 깔끔한데, 삭제되어야 할List를 찾는 과정을update에서도 진행하고,findRemovalImageIds에서도 진행하게 됨.2MomentImages내부 메서드findRemovalImageIds테스트를 어떻게 해야할 지 고민입니다. 현재는id로 비교해야하는 과정이라DB의 저장이 이루어져야id비교검증이 가능한 상태입니다. 또한 해당 메서드 내부에서 사용되는 단일 메서드는update메서드를 통해 내부적으로 정상작동이 검증되었기에 테스트를 진행해야 할 지 고민이 되기도 합니다. 따라서 우선 테스트는 구성안했습니다.해당 부분에 대해서 의견 부탁드려요!!
🙂 To Reviewer
📋 To Do