Skip to content
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.

Sign up for GitHub

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: 스타카토 양방향 연관관계 끊기 #547 #569

Closed
wants to merge 4 commits into from
Closed

refactor: 스타카토 양방향 연관관계 끊기 #547 #569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2974d8b
refactor: MomentImage와의 양방향 연관관계 삭제
Ho-Tea Dec 12, 2024
23b25b3
refactor: MomentImage 일급 컬렉션 비즈니스 로직 수정
Ho-Tea Dec 12, 2024
252d68f
refactor: MomentImageRepository 배치 삭제와 Moment 별 조회 로직 추가
Ho-Tea Dec 12, 2024
fe68126
refactor: 스타카토 수정시 이미지 수동 삭제 로직 구현
Ho-Tea Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions backend/src/main/java/com/staccato/memory/service/MemoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.staccato.memory.service.dto.response.MemoryResponses;
import com.staccato.memory.service.dto.response.MomentResponse;
import com.staccato.moment.domain.Moment;
import com.staccato.moment.domain.MomentImage;
import com.staccato.moment.repository.MomentImageRepository;
import com.staccato.moment.repository.MomentRepository;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -85,10 +86,9 @@ private List<MomentResponse> getMomentResponses(List<Moment> moments) {
}

private String getMomentThumbnail(Moment moment) {
if (moment.hasImage()) {
return moment.getThumbnailUrl();
}
return null;
return momentImageRepository.findFirstByMomentIdOrderByIdAsc(moment.getId())
.map(MomentImage::getImageUrl)
.orElse(null);
}

@Transactional
Expand Down
18 changes: 0 additions & 18 deletions backend/src/main/java/com/staccato/moment/domain/Moment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ public class Moment extends BaseEntity {
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "memory_id", nullable = false)
private Memory memory;
@Embedded
private MomentImages momentImages = new MomentImages();

@Builder
public Moment(
Expand All @@ -54,14 +52,12 @@ public Moment(
@NonNull String address,
@NonNull BigDecimal latitude,
@NonNull BigDecimal longitude,
@NonNull MomentImages momentImages,
@NonNull Memory memory
) {
validateIsWithinMemoryDuration(visitedAt, memory);
this.visitedAt = visitedAt.truncatedTo(ChronoUnit.SECONDS);
this.title = title.trim();
this.spot = new Spot(placeName, address, latitude, longitude);
this.momentImages.addAll(momentImages, this);
this.memory = memory;
}

Expand All @@ -71,27 +67,13 @@ private void validateIsWithinMemoryDuration(LocalDateTime visitedAt, Memory memo
}
}

public void update(String title, MomentImages newMomentImages) {
this.title = title;
this.momentImages.update(newMomentImages, this);
}

public void update(Moment updatedMoment) {
this.visitedAt = updatedMoment.getVisitedAt();
this.title = updatedMoment.getTitle();
this.spot = updatedMoment.getSpot();
this.momentImages.update(updatedMoment.momentImages, this);
this.memory = updatedMoment.getMemory();
}

public String getThumbnailUrl() {
return momentImages.getImages().get(0).getImageUrl();
}

public boolean hasImage() {
return momentImages.isNotEmpty();
}

public void changeFeeling(Feeling feeling) {
this.feeling = feeling;
}
Expand Down
7 changes: 2 additions & 5 deletions backend/src/main/java/com/staccato/moment/domain/MomentImage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;

import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
Expand All @@ -28,12 +27,10 @@ public class MomentImage {
@JoinColumn(name = "moment_id", nullable = false)
private Moment moment;


@Builder
public MomentImage(@Nonnull String imageUrl) {
public MomentImage(@Nonnull String imageUrl, Moment moment) {
this.imageUrl = imageUrl;
}

protected void belongTo(Moment moment) {
this.moment = moment;
}
}
59 changes: 24 additions & 35 deletions backend/src/main/java/com/staccato/moment/domain/MomentImages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,42 @@
package com.staccato.moment.domain;

import java.util.ArrayList;
import java.util.List;
import jakarta.persistence.CascadeType;
import jakarta.persistence.Embeddable;
import jakarta.persistence.OneToMany;
import com.staccato.exception.StaccatoException;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Embeddable
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class MomentImages {

public record MomentImages(List<MomentImage> images) {
private static final int MAX_COUNT = 5;
@OneToMany(mappedBy = "moment", cascade = CascadeType.ALL, orphanRemoval = true)
private List<MomentImage> images = new ArrayList<>();

public MomentImages(List<String> addedImages) {
validateNumberOfImages(addedImages);
this.images.addAll(addedImages.stream()
.map(MomentImage::new)
.toList());

public MomentImages {
validateNumberOfImages(images);
}

public static MomentImages of(List<String> imageUrls, Moment moment) {
List<MomentImage> images = imageUrls.stream()
.map(imageUrl -> new MomentImage(imageUrl, moment))
.toList();
return new MomentImages(images);
}

private void validateNumberOfImages(List<String> addedImages) {
private <T> void validateNumberOfImages(List<T> addedImages) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제네릭으로 한 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List<String>, List<MomentImage> 형식 모두를 호환하고자 제네릭으로 표현했었는데, 사용되는 곳은 압축생성자 한 곳이네요! 필요없어요!

if (addedImages.size() > MAX_COUNT) {
throw new StaccatoException("사진은 5장을 초과할 수 없습니다.");
}
}

protected void addAll(MomentImages newMomentImages, Moment moment) {
newMomentImages.images.forEach(image -> {
this.images.add(image);
image.belongTo(moment);
});
}

protected void update(MomentImages momentImages, Moment moment) {
removeExistsImages(new ArrayList<>(images));
addAll(momentImages, moment);
public List<MomentImage> findValuesNotPresentIn(MomentImages targetMomentImages) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findImages가 아닌 findValues로 네이밍한 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

특별한 이유가 없습니다!
findImages가 더 나은 네이밍 같네요!

return this.images.stream()
.filter(image -> !targetMomentImages.contains(image))
.toList();
}

private void removeExistsImages(List<MomentImage> originalImages) {
originalImages.forEach(this.images::remove);
private boolean contains(MomentImage momentImage) {
return this.images.stream()
.anyMatch(image -> image.getImageUrl().equals(momentImage.getImageUrl()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image에 같은지 물어보는 메서드를 분리하면 어떨까요?
코드 한 줄에 .이 너무 많은 것 같아요!

}

public boolean isNotEmpty() {
return !images.isEmpty();
public List<String> getUrls() {
return images.stream()
.map(MomentImage::getImageUrl)
.toList();
}
}
10 changes: 8 additions & 2 deletions backend/src/main/java/com/staccato/moment/repository/MomentImageRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@
import com.staccato.moment.domain.MomentImage;

public interface MomentImageRepository extends JpaRepository<MomentImage, Long> {
Optional<MomentImage> findFirstByMomentId(long momentId);

@Modifying
@Query("DELETE FROM MomentImage mi WHERE mi.moment.id In :momentIds")
void deleteAllByMomentIdInBatch(@Param("momentIds") List<Long> momentIds);

@Modifying
@Query("DELETE FROM MomentImage mi WHERE mi.id In :ids")
void deleteAllByIdInBatch(@Param("ids") List<Long> ids);

List<MomentImage> findAllByMomentId(long momentId);

Optional<MomentImage> findFirstByMomentIdOrderByIdAsc(long momentId);
}
40 changes: 35 additions & 5 deletions backend/src/main/java/com/staccato/moment/service/MomentService.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스에 메서드가 굉장히 많아졌네요!
책임 분리가 조금 필요해보이는데, 생각한 방향이 있을까요?
호티의 의견이 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 기존에 MomentImages 가 가지고 있던 비즈니스 로직이 밖으로 나오게 되어 책임이 모호해 졌습니다.

  • 양방향 연관관계를 유지한다면 고려하지 않아도 될 것 같아요
  • 양방향 연관관계를 유지하지 않는다면 퍼사드 패턴과 같이 새로운 계층 구조를 통해 해결할 수 있을 것 같아요

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import com.staccato.memory.repository.MemoryRepository;
import com.staccato.moment.domain.Feeling;
import com.staccato.moment.domain.Moment;
import com.staccato.moment.domain.MomentImage;
import com.staccato.moment.domain.MomentImages;
import com.staccato.moment.repository.MomentImageRepository;
import com.staccato.moment.repository.MomentRepository;
import com.staccato.moment.service.dto.request.FeelingRequest;
Expand All @@ -37,8 +39,10 @@ public MomentIdResponse createMoment(MomentRequest momentRequest, Member member)
Memory memory = getMemoryById(momentRequest.memoryId());
validateMemoryOwner(memory, member);
Moment moment = momentRequest.toMoment(memory);
MomentImages newMomentImages = momentRequest.toMomentImages(moment);

momentRepository.save(moment);
momentImageRepository.saveAll(newMomentImages.images());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

momentImages 양방향 관계를 끊어보니 어떤가요?
사실 저는 스타카토 관련 작업을 할 때 Image 없이 작업을 하는 일이 잘 없다보니 양방향이 필요한 부분이라고 생각했었는데요! 실제로 서비스 모든 로직에서 Images 관련 작업이 별개로 필요해지니, 양방향을 사용할 때보다 휴먼 에러가 나오기 쉽겠다는 생각이 들었어요!
호티는 그 2가지를 다 경험해보았으니 어땠는지 궁금합니다.
(추가로 궁금한 건 삭제가 잘 안되던 버그를 해결하기 위해 양방향 참조를 끊은걸까요? BatchInsert를 위해 양방향 참조를 끊은걸까요?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제 관련된 전이를 끊으니(orphanRemoval) 부모엔티티가 자식 엔티티의 생성 생명주기만을 관리하게 되었고,
양방향 연관관계는 추후에 언제든 예기치 못한 예외상황을 만들 수 있다고 생각되었고,
영속성전이 속성으로 인해 Insert가 항상 생성되는 엔티티마다 나가는 문제도 있어서 양방향 연관관계를 끊는 방식으로 구성하게 되었습니다.

양방향 연관관계를 끊으면서 Insert가 생성되는 엔티티마다 나가는 것을 하나의 BulkInsert를 통해 성능적 이점을 도모하려고 시도하였습니다. 하지만 PR에 적어놓은 것 처럼 성능적 이점은 미미했고, 더 나아가 양방향 연관관계를 끊는 것으로 인해 수정된 곳이 많아졌고, 서비스 로직에 굳이 없어도 될 비즈니스 로직이
명시되게 되었습니다.

굳이 없어도 될 비즈니스 로직

// MomentService.java 의 updateMomentById 메서드

MomentImages momentImages = getMomentImagesBy(moment);
MomentImages newMomentImages = momentRequest.toMomentImages(moment);
removeExistImages(momentImages, newMomentImages);
saveNewImages(momentImages, newMomentImages);
  • BulkInsert를 통해 얻는 성능적 이점이 미미
  • 양방향 연관관계를 끊음으로 인해 Serivce에 굳이 없어도 될 비즈니스 로직이 존재
  • 휴먼에러 가능성이 증가

이와 같은 이유로 양방향 연관관계로 다시 유지하는게 나을 것 같다는 생각이 들었습니다.
직접 양방향과 단방향 연관관계의 이점과 단점을 비교하여 설정한 이 과정 자체가 의미있었다 생각합니다 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 공감합니다!! 직접 코드로 구현해보고 장단점을 비교해봤을 때 더 잘 와닿는다고 생각해요! 호티한테도 의미있는 과정이었다니 다행입니다👍👍


return new MomentIdResponse(moment.getId());
}
Expand All @@ -57,7 +61,8 @@ public MomentLocationResponses readAllMoment(Member member) {
public MomentDetailResponse readMomentById(long momentId, Member member) {
Moment moment = getMomentById(momentId);
validateMemoryOwner(moment.getMemory(), member);
return new MomentDetailResponse(moment);
MomentImages momentImages = getMomentImagesBy(moment);
return new MomentDetailResponse(moment, momentImages);
}

@Transactional
Expand All @@ -69,18 +74,43 @@ public void updateMomentById(
Moment moment = getMomentById(momentId);
validateMemoryOwner(moment.getMemory(), member);

Memory targetMemory = getMemoryById(momentRequest.memoryId());
validateMemoryOwner(targetMemory, member);
Memory memory = getMemoryById(momentRequest.memoryId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 순서 확인해주세요! (getMemoryById)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 이해가 잘 가지않았습니다!
보충 설명 부탁드려용

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨벤션을 의미한 거였습니다! getMemoryById가 사용되는 가장 마지막 메서드 하위에 위치해주세요:)

validateMemoryOwner(memory, member);

Moment newMoment = momentRequest.toMoment(memory);
moment.update(newMoment);

Moment updatedMoment = momentRequest.toMoment(targetMemory);
moment.update(updatedMoment);
MomentImages momentImages = getMomentImagesBy(moment);
MomentImages newMomentImages = momentRequest.toMomentImages(moment);
removeExistImages(momentImages, newMomentImages);
saveNewImages(momentImages, newMomentImages);
}

private Moment getMomentById(long momentId) {
return momentRepository.findById(momentId)
.orElseThrow(() -> new StaccatoException("요청하신 스타카토를 찾을 수 없어요."));
}

private MomentImages getMomentImagesBy(Moment moment) {
List<MomentImage> momentImages = momentImageRepository.findAllByMomentId(moment.getId())
.stream()
.toList();
return new MomentImages(momentImages);
}

private void removeExistImages(MomentImages momentImages, MomentImages newMomentImages) {
List<MomentImage> removeList = momentImages.findValuesNotPresentIn(newMomentImages);
List<Long> ids = removeList.stream()
.map(MomentImage::getId)
.toList();
momentImageRepository.deleteAllByIdInBatch(ids);
}

private void saveNewImages(MomentImages momentImages, MomentImages newMomentImages) {
List<MomentImage> saveList = newMomentImages.findValuesNotPresentIn(momentImages);
momentImageRepository.saveAll(saveList);
}

@Transactional
public void deleteMomentById(long momentId, Member member) {
momentRepository.findById(momentId).ifPresent(moment -> {
Expand Down
9 changes: 4 additions & 5 deletions backend/src/main/java/com/staccato/moment/service/dto/request/MomentRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,14 @@
import java.time.LocalDateTime;
import java.util.List;
import java.util.Objects;

import jakarta.validation.constraints.Min;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;

import org.springframework.format.annotation.DateTimeFormat;

import com.staccato.memory.domain.Memory;
import com.staccato.moment.domain.Moment;
import com.staccato.moment.domain.MomentImages;

import io.swagger.v3.oas.annotations.media.ArraySchema;
import io.swagger.v3.oas.annotations.media.Schema;

Expand Down Expand Up @@ -65,7 +61,10 @@ public Moment toMoment(Memory memory) {
.longitude(longitude)
.address(address)
.memory(memory)
.momentImages(new MomentImages(momentImageUrls))
.build();
}

public MomentImages toMomentImages(Moment moment) {
return MomentImages.of(momentImageUrls, moment);
}
}
6 changes: 3 additions & 3 deletions backend/src/main/java/com/staccato/moment/service/dto/response/MomentDetailResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import java.util.List;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.staccato.moment.domain.Moment;
import com.staccato.moment.domain.MomentImage;
import com.staccato.moment.domain.MomentImages;
import io.swagger.v3.oas.annotations.media.ArraySchema;
import io.swagger.v3.oas.annotations.media.Schema;

Expand Down Expand Up @@ -41,15 +41,15 @@ public record MomentDetailResponse(
@Schema(example = "-0.12712788587027796")
BigDecimal longitude
) {
public MomentDetailResponse(Moment moment) {
public MomentDetailResponse(Moment moment, MomentImages momentImages) {
this(
moment.getId(),
moment.getMemory().getId(),
moment.getMemory().getTitle(),
moment.getMemory().getTerm().getStartAt(),
moment.getMemory().getTerm().getEndAt(),
moment.getTitle(),
moment.getMomentImages().getImages().stream().map(MomentImage::getImageUrl).toList(),
momentImages.getUrls(),
moment.getVisitedAt(),
moment.getFeeling().getValue(),
moment.getSpot().getPlaceName(),
Expand Down
17 changes: 0 additions & 17 deletions backend/src/test/java/com/staccato/fixture/moment/MomentFixture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@

import java.math.BigDecimal;
import java.time.LocalDateTime;
import java.util.List;
import com.staccato.memory.domain.Memory;
import com.staccato.moment.domain.Moment;
import com.staccato.moment.domain.MomentImages;

public class MomentFixture {
private static final BigDecimal latitude = new BigDecimal("37.77490000000000");
Expand All @@ -20,7 +18,6 @@ public static Moment create(Memory memory) {
.address("address")
.placeName("placeName")
.memory(memory)
.momentImages(new MomentImages(List.of()))
.build();
}

Expand All @@ -33,20 +30,6 @@ public static Moment create(Memory memory, LocalDateTime visitedAt) {
.placeName("placeName")
.address("address")
.memory(memory)
.momentImages(new MomentImages(List.of()))
.build();
}

public static Moment createWithImages(Memory memory, LocalDateTime visitedAt, MomentImages momentImages) {
return Moment.builder()
.visitedAt(visitedAt)
.title("staccatoTitle")
.latitude(latitude)
.longitude(longitude)
.placeName("placeName")
.address("address")
.memory(memory)
.momentImages(momentImages)
.build();
}
}
Loading
Loading