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.

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

[BUG] 좋아요 반 정규화로 인한 템플릿 좋아요 시 해당 템플릿의 ModifiedAt 변경 문제 #927

Merged
merged 11 commits into from
Dec 16, 2024

Conversation

kyum-q
Copy link
Contributor

@kyum-q kyum-q commented Nov 20, 2024

⚡️ 관련 이슈

close #925

📍주요 변경 사항

주요 변경 사항에 대해 작성해주세요.

  • 좋아요, 템플릿 공개여부 변경, 카테고리 변경, 태그 변경 시 ModifiedAt이 변경되지 않게 구현
  • @PreUpdate 이용해 ModifiedAt에 변경될 필요 없는 경우, 전 시간으로 돌린다.

🎸기타

고려해야 하는 내용을 작성해 주세요.

  • 방법 선택 이유
  • Category 테스트 중에 거짓 양성인 테스트가 있어서 수정했습니다. 확인 부탁!
    • 거짓 양성이었던 이유는 CategoryFixture 에서 get() 메서드를 쓰면 ID가 동일해서 였습니다. ㅎㅎ
  • 그리고 테스트 @Disabled인 것들 해결해 나갑시다 ~

🍗 PR 첫 리뷰 마감 기한

11/29 10:00

@kyum-q kyum-q added bug 개발자가 의도하지 않은 상황 BE 백엔드 labels Nov 20, 2024
@kyum-q kyum-q added this to the 7차 스프린트 💭 milestone Nov 20, 2024
@kyum-q kyum-q self-assigned this Nov 20, 2024
Copy link
Contributor Author

@kyum-q kyum-q left a comment

Choose a reason for hiding this comment

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

회의에서 말한 개선하고 싶은 부분은 이부분입니다.

Comment on lines +298 to +301
sut.update(member, template.getId(), updateRequest, category);
entityManager.flush();

assertThat(template.getModifiedAt()).isNotEqualTo(beforeModifiedAt);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flush 코드 없이 preUpdate가 불러지게 하고 싶은데 방법을 몰라서 EntitiyManager를 사용했습니다.

이걸 안사용하는 방안으로는 template.isModified 를 확인하는 방법도 있는데 정확한 테스트가 아니니 걱정됩니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

흠 여기는 어쩔 수 없이 flush를 해줘야 할 것 같네요.
저는 딱히 문제될 건 없을 거 같은데 다른 걱정되는 부분이 있나요?

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

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

켬미 수고했어요 👍🔥
수료 등 행사 때문에 리뷰가 많이 늦었네요 ㅠㅠ
리뷰 조금 남겨보았으니 확인 부탁해요!

Comment on lines 55 to 57
public void markModified() {
isModified = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

켬미~ 제안이 하나 있어요!
markModified() 말고 markUnModified() 는 어떨까요~?

변경 시간에 영향이 없도록 해야 하는 컬럼은 좋아요 컬럼 하나인데, 다른 update를 하는 컬럼에 전부 해당 메서드를 추가해야 해서 변경이 잡힌 것 같아요 🥲
또 다른 내부 update 문을 생성해야 할 때, markModified()를 해주어야 하는데 인지 비용도 있을 것 같아요!

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.

좋은 것 같아요 ~! 변경하겠습니다 ~

Copy link
Contributor Author

@kyum-q kyum-q Dec 2, 2024

Choose a reason for hiding this comment

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

markUnModified만 사용하려고 하였으나 생각해보니 다음과 같은 상황에서 템플릿의 modified가 변경되어야하는데 변경되지 않는 문제가 발생해서 isModified의 기본 값을 true로 하고 markUnModified()를 이용하되 상황에 따라 markModified()도 사용하는 식으로 변경했어요.. 근데 걱정이 좀 있네요 읽어주세요 ~

상황 :

소스 코드 변경 시 템플릿 modifedAt 변경되야 함, 소스코드 변경 && 템플릿 타이틀, 설명 변경하지 않음

이 때 로직 : 템플릿 먼저 변경 -> 소스코드 변경

  • 템플릿 변경 시 템플릿 타이틀, 설명 변경하지 않았기에 markUnModified()가 호출됨
    • isModified = false
  • 소스 코드 변경 template 의 isModified 를 다시 true로 변경해야하기에 template.markModifiedSourceCodes() 호출로 markModified() 호출
    • isModified = true

걱정 :

근데 이렇게 했을 때 또 다른 휴먼 에러가 발생할 것 같기도 하네요..
만약 소스코드 변경이 먼저 되고 템플릿이 변경된다면 또 문제가 생긴다. 그래서 추후 템플릿과 연관된 데이터 업데이트 시 template.markModifiedSourceCodes()을 꼭 호출해야한다. 그리고 무조건 템플릿 업데이트를 먼저해야 함을 알고 있어야합니다 ..!

저는 조금 이부분이 더 걱정되서 markModified()만 사용하는게 더 나을 것 같다고 생각이 들기도 하네요 ~
이거에 대해 어떻게 생각하시나요?

Copy link
Contributor Author

@kyum-q kyum-q Dec 3, 2024

Choose a reason for hiding this comment

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

근데 생각해보니 현재 코드일 경우 소스코드를 추가하거나 삭제할 때 템플릿의 modifiedAt이 변경되지 않고 있네요 ..!

어떻게 처리해야할지.. 모르겠네요..!

지금 생각해보니 처리 안 된 부분은 코멘트 에 ⚡️표시 해뒀어요 오늘 내에 수정해보겠습니다 확인해주세요 ~!

Copy link
Contributor

Choose a reason for hiding this comment

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

역시 로직이 복잡해지니 여러 문제가 생기네요...😭

코드가 이정도로 복잡해질 거라고는 생각 못했어요.

개인적으로는 두 가지 해결방안이 가장 좋다고 생각해요.

  1. 정책 변경: createdAt을 기준으로 정렬
  2. Like 테이블 정규화

1번은 투표로 결정되었으니 어쩔 수 없긴 하지만, 저는 수정된 날짜를 기준으로 정렬하는 기능은 사용자에게 좋지 않은 경험을 제공한다고 생각해요. 그런 서비스를 본 적도 없구요.

2번은 우리가 Like의 변경이나 추가가 Template에 영향을 주지 않길 원하는 건, 원래 그 둘이 다른 도메인이기 때문이라고 생각해요. 테이블을 원래대로 정규화하고 도메인을 분리하는 게 더 객체지향적이고 현재의 문제도 해결할 수 있는 방법일 거에요.

다만 켬미가 작업자이니만큼 켬미의 의견에 더 무게를 주고 싶어요. 제 의견에 대해 어떻게 생각하는지 알려주면 고맙겠습니당

Copy link
Contributor

@jminkkk jminkkk Dec 3, 2024

Choose a reason for hiding this comment

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

저도 1번이 적절하다고 생각해요.
실제 우리 데이터셋을 보면 변경한 데이터가 25.27% 정도예요. 아마 좋아요로 인해 변경에 잡힌 템플릿도 포함되어 있을 것 같아요.
따라서 실제 변경된 데이터는 25프로보다 이하라고 생각해요.

image

2번을 적용할 경우, 고려해야 하는 건 다음과 같아요.
좋아요 정렬 시에 인덱스 적용이 불가능해요. 이전처럼 매번 재조회를 해와야 하며 정렬 조건이 좋아요인 경우 filesort가 발생하여 비효율적이에요.

성능을 포기하고 1번을 유지할만큼 1번 조건이 사용자에게 유의미한가?를 고민해보면 좋을 것 같아요.
제우스 말처럼 결정된 사항이지만 결정 당시에 비해 비용을 고려해야 할 점이 추가로 생겼기 때문에 충분히 재논의해보아도 좋을 것 같아요. 다른 분들은 의견이 어떤지 의견 궁금해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

첫번째 의견인 정책 변경에 대해서는 한 번 다 함께 이야기해보면 좋을 것 같아요~ 저도 찬성입니다 ~

그리고 두번째 의견인 좋아요 정규화은 하지 않아도 됩니다 ! 좋아요 시에만 업데이트 안되게 하는 건 쉬운데 요구 중에 공개범위나 카테고리 변경 시에도 업데이트 되는게 싫다 라는 의견도 있었어서 그거 해결하다가 이렇게 복잡해진 것 같네요 ~!

@@ -38,10 +42,15 @@ void success() {
member,
categoryRepository.save(CategoryFixture.getFirstCategory())
));
LocalDateTime modifiedAtBeforeLike = template.getModifiedAt();
Copy link
Contributor

Choose a reason for hiding this comment

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

이 테스트 분리하거나, @DisplayName성공: 템플릿 수정 시간은 변경되지 않는다. 이렇게 추가해보면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DisplayName성공: 템플릿 수정 시간은 변경되지 않는다.로 변경하겠습니다~

Copy link
Contributor

@HoeSeong123 HoeSeong123 left a comment

Choose a reason for hiding this comment

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

그동안 바쁘다는 핑계로 너무 늦게 확인해버렸네요.
고생하셨습니다 켬미!!

Comment on lines +298 to +301
sut.update(member, template.getId(), updateRequest, category);
entityManager.flush();

assertThat(template.getModifiedAt()).isNotEqualTo(beforeModifiedAt);
Copy link
Contributor

Choose a reason for hiding this comment

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

흠 여기는 어쩔 수 없이 flush를 해줘야 할 것 같네요.
저는 딱히 문제될 건 없을 거 같은데 다른 걱정되는 부분이 있나요?

Copy link
Contributor

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

바쁜데 작업하느라 고생 많았어요~~

몰리가 남긴 코멘트에 동의해서 저도 approve 대신 comment 남겼습니다

Comment on lines 55 to 57
public void markModified() {
isModified = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 같은 의견이에요~!

@kyum-q
Copy link
Contributor Author

kyum-q commented Dec 5, 2024

논의 끝에 ModifiedAt 정렬을 지우고 CreatedAt 정렬하기로 했습니다 !

@kyum-q kyum-q closed this Dec 5, 2024
@kyum-q kyum-q reopened this Dec 11, 2024
@kyum-q
Copy link
Contributor Author

kyum-q commented Dec 11, 2024

이야기 끝에 다음과 같이 하기로 결정했습니다

요약

  • 좋아요/좋아요 취소 시에 수정 시간 변경되지 않는다
  • 그 외 변경 사항은 수정 시간이 변경된다.

Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

고생했어요 켬미! 🔥🔥


@Test
@DisplayName("템플릿 수정 성공 : 일부 데이터가 수정됬을 경우 modifiedAt 변경")
@DisplayName("템플릿 수정 성공 : 데이터가 수정됬을 경우 modifiedAt 변경")
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

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

새벽까지 작업하느라 고생했어요 켬미!

RC 남겼는데, 해당 내용으로 코드와 관련해 켬미와 의논을 좀더 하고 싶어요.

혹시 제 의견을 보고 동의한다면 문제가 없겠지만, 혹시 이견이 있거나 궁금한 게 있다면 코멘트 또는 게더로 회의하면 좋겠네용

Comment on lines 33 to 57
@Transient
private LocalDateTime lastKnownModifiedAt;

@Transient
private boolean isModified = true;

@PreUpdate
private void preUpdate() {
if (!isModified && lastKnownModifiedAt != null) {
modifiedAt = lastKnownModifiedAt;
return;
}
isModified = true;
}

@PostLoad
@PostPersist
@PostUpdate
private void postLoad() {
lastKnownModifiedAt = modifiedAt;
}

public void markUnModified() {
isModified = false;
}
Copy link
Contributor

@zeus6768 zeus6768 Dec 12, 2024

Choose a reason for hiding this comment

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

저는 이 코드를 가져가는 것에 회의적인 입장이에요.

로직이 복잡한데다, 자식 클래스의 메서드에서 markUnmodified를 호출해야할지 말아야할지 고민하면서 코드를 작성하는 것을 강제하게 되기 때문이에요. 그렇게되면 반드시 휴먼 에러가 발생할 것이라고 생각해요.

변경 시간에 영향이 없도록 해야 하는 컬럼은 좋아요 컬럼 하나인데, 다른 update를 하는 컬럼에 전부 해당 메서드를 추가해야 해서 변경이 잡힌 것 같아요 🥲

몰리가 처음에 언급했던 문제를 표면적으로 해결하긴 했지만, 근본적인 해결 방법은 아닌 것 같아요.

Like 테이블을 정규화하고 TemplateId와 LikeCount를 칼럼으로 하는 테이블을 만들어서 관리하는 게 더 좋다고 생각해요. 그렇게하면 좋아요순 정렬의 filsort 문제도 해결될 거에요.

Copy link
Contributor Author

@kyum-q kyum-q Dec 13, 2024

Choose a reason for hiding this comment

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

로직이 복잡한데다, 자식 클래스의 메서드에서 markUnmodified를 호출해야할지 말아야할지 고민하면서 코드를 작성하는 것을 강제하게 되기 때문이에요. 그렇게되면 반드시 휴먼 에러가 발생할 것이라고 생각해요.
몰리가 처음에 언급했던 문제를 표면적으로 해결하긴 했지만, 근본적인 해결 방법은 아닌 것 같아요.

저는 생각이 다른게 좋아요 관련해서는 몰리의 코멘트로 근본적인 휴먼에러가 날 수 있는 상황이 해결 됐다고 생각해요.
자식 클래스는 이전과 동일하게 수정시간의 변경이 되는 것이 정상이니 변경 사항이 없다면 markUnmodified를 호출하지 않으면 되는거라 제우스가 말하는 불편함이라던가 더 생각할 부분이 없다고 생각해요. 오히려 좋아요처럼 수정시간 업데이트가 불필요한 순간에 따로 테이블을 만들거나 복잡한 절차가 아닌 지금 만들어둔 markUnmodified를 호출하기만 하면 되니 더 편리하게 사용할 수 있을 것 같아요.

Like 테이블을 정규화하고 TemplateId와 LikeCount를 칼럼으로 하는 테이블을 만들어서 관리하는 게 더 좋다고 생각해요. 그렇게하면 좋아요순 정렬의 filsort 문제도 해결될 거에요.

제가 생각하기엔 이 방식은 정규화가 아닌 반정규화의 또 다른 방식인데 해당 테이블과 템플릿 테이블의 관계는 일대일이고 너무 종속된 데이터 같아요 그래서 굳이 연관테이블을 만드는 방식이 더 좋을지는 의문이에요.
그리고 이렇게 되면 템플릿을 조회할 때마다 해당 테이블을 join해서 값을 얻어와야하는 로직이 생기고 이는 반정규화의 장점(쿼리 성능과 단순화: 조인이 필요해짐 데이터가 분리됨)도 정규화의 장점(데이터 무결성과 독립성: 좋아요 개수를 좋아요 테이블에 있는 데이터로 하는게 아님 또 계산해서 저장)도 가져가지 않는 방식인 것 같다고 생각해요.

이러한 이유로 저는 현재처럼 템플릿에 개수를 둬서 반 정규화의 장점이라도 챙겨야한다고 생각하고 그래서 preupdate를 사용하는 것이 더 좋은 방식이라고 생각합니다

이에 대해 이야기가 필요하다면 이야기 참여를 원하는 사람들끼리 한 번 더 이야기해봐요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zangsu

제우스 말 대로 휴먼 에러 발생 가능성이 크다고 생각합니다.
다만, 켬미가 정규화를 선택하지 않은 이유가 "공개범위나 카테고리 변경 시에도 업데이트 되는게 싫다 라는 의견도 있었어서" 인 것 같아서 어렵네요,,

오해가 있는 것 같아서 짚어보자면 공개범위나 카테고리 변경 시 업데이트 되는 문제는 이 방식으로 해결되지 않고 고려하지 않기로 했습니다 그래서 그 이유는 아닙니다.

정규화를 선택하게 된다면 "본인이 수정하지 않는 값인 좋아요 수에 대해서만 수정 내역에서 제외" 하는 정책이 되겠네요. 저는 지금의 코드도 "개선이 필요하지만, 버그 픽스를 위해 필요한 코드" 정도로 수용 가능할 것 같아 우선 approve 남겨두겠습니다...!

개선이 필요한 부분이 있다면 일단 코멘트 주시면 감사할게요~ 제우스와 동일한 의견이라면 위에 코멘트를 읽어주시고 추후 회의를 해봐요~

Copy link
Contributor

Choose a reason for hiding this comment

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

(신나는 토론의 장이 열렸다는 얘기를 듣고 호다닥 달려왔습니다🏃‍➡️)

혹시 이제 기본 정렬 시간이 createdAt이 된 만큼 해당 코멘트에 변경사항이 생길까요??
에를 들어, 해당 코멘트에서 카테고리, 태그, 공개여부 등을 수정해도 수정 시간이 변하지 않게 한다고 되어 있습니다.
이렇게 했던 이유는 기본 정렬 시간이 modifiedAt 이므로 카테고리, 태그, 공개여부 등만 수정했는데 맨 위로 올라가는 것이 어색하게 느껴져서 그렇습니다.
하지만 이제는 createdAt이 기본 정렬이므로, 위 내용들은 빠져도 될 것 같습니다. 즉, 어떤 내용이더라도 본인이 수정한 내용이라면 수정 시간이 변경되는게 어색하지 않을 것 같아요.
다만 좋아요의 경우는 본인이 수정한게 아니므로 수정 시간이 변경되면 안된다고 생각해요.

위 내용을 물어본 이유는 다음과 같습니다.

  • 만약 좋아요에 대해서만 수정 시간이 변경되지 않는다면 제우스의 의견이 타당하다고 생각합니다.
    • 좋아요 하나에 대해서만 적용되는 이례적인 내용인데 BaseTimeEntity를 상속하는 모든 클래스들이 이를 알고 있어야 하는 것이 어색해요. 이는 SOLID 원칙 중 하나인 인터페이스 분리 원칙에 어긋나는 것 같다고 생각됩니다.
  • 좋아요 뿐 아니라, 카테고리, 태그, 공개여부 등도 수정 시간이 변경되지 않게 해야 한다면 켬미의 의견이 타당하다고 생각합니다.
    • 제우스가 제시한 의견으로는 다른 부분들에 대해 적용할 수 없기 때문입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

혹시 이제 기본 정렬 시간이 createdAt이 된 만큼 해당 코멘트에 변경사항이 생길까요??

현재는 고려하지 않고 있습니다.

  • 만약 좋아요에 대해서만 수정 시간이 변경되지 않는다면 제우스의 의견이 타당하다고 생각합니다.

    • 좋아요 하나에 대해서만 적용되는 이례적인 내용인데 BaseTimeEntity를 상속하는 모든 클래스들이 이를 알고 있어야 하는 것이 어색해요. 이는 SOLID 원칙 중 하나인 인터페이스 분리 원칙에 어긋나는 것 같다고 생각됩니다.

일단 BaseTimeEntity은 인터페이스가 아닙니다 그래서 인터페이스 분리 원칙에 어긋난다는 것을 이해하기 어렵네요. 개념상 어긋난다는 것이라면 제가 알기로는 ISP는 '클라이언트는 자신이 사용하지 않는 메서드에 의존하지 않아야 한다' 라는 내용으로 알고 있는데 수정시간 변경을 안하게 해주는 메서드를 추가했다고 사용하지 않는 메서드가 추가된게 아니라고 생각합니다. 그리고 인터페이스가 이러한 규칙을 따라야하는 이유는 인터페이스는 추상메서드만 제공하는 것이므로 상속하기 위해서는 무조건적으로 해당 메서드를 구현해야하기 때문에 사용하지 않는 메서드를 만들지 말라는 규칙이 생긴걸로 알고 있습니다.
제가 잘못 알고 있다면 알려주세요~

그리고 해당 클래스의 주요 역할은 수정 및 생성 시간을 관리하는 역할이라 생각하고, 수정시간 변경이 원치 않을 경우 이러한 메서드를 제공하는 것이라고 생각합니다. 그래서 이를 상속하는 클래스들이 필요에 따라 이를 사용할 수 있도록 시간을 관리하는 클래스에 이러한 로직이 있는게 어색하거나 오버프로그래밍이라고 생각하기 않습니다.

Copy link
Contributor Author

@kyum-q kyum-q Dec 13, 2024

Choose a reason for hiding this comment

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

말을 덧붙이자면 해당 코드가 좋아요 기능으로 인해 생겼다는 이유로만 코드를 살펴보면, “다른 클래스에서 사용되지 않는데 왜 필요한가?“라는 의문이 들 수 있을 것 같습니다.
하지만 저는 조회수나 상태 관리 등 다른 변경 사항에서도 수정 시간이 변경되지 않아야 하는 경우가 있을 거라 생각합니다. 그래서 이 기능을 유동적으로 사용할 수 있도록 구현한 현 상태가 좋다고 생각합니다.

메서드 명칭도 unMarkModifiedAt으로 설정한 이유는, 수정 시간을 변경하지 않는다는 의미를 명확히 전달하기 위함입니다. 이 로직을 BaseTimeEntity 클래스에 두어 시간 관리와 관련된 기능을 중앙화하고, 추후에 발생할 수 있는 다양한 요구사항에 대해 사용의 한계를 두지 않도록 설계한 것입니다.

제우스 의견에 단 제 답변에 덧붙여, 앞으로도 이 메서드를 사용하는 것이 좋아요에 한정되지 않을 것 같다는 의견입니다.

Copy link
Contributor

@zeus6768 zeus6768 Dec 13, 2024

Choose a reason for hiding this comment

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

저는 여전히 위 코드가 반영되는게 조심스러워서, 좀더 고민하고 자료를 찾아보며 근거를 추가하고 정리해봤어요. 내용이 좀 길어서 양해 바라요 🙏

내용이 너무 길어서 요약을 먼저 작성해요~!!

  1. 객체지향적 설계 훼손: 비즈니스 로직이 BaseTimeEntity에 들어오면서 역할이 모호해짐
  2. 코드 복잡성 증가: Lifecycle Hook와 Transient 필드를 통한 우회 로직으로 엔티티 이해도가 떨어짐
  3. 휴먼 에러 발생 가능성: markUnmodified() 호출 여부를 개발자가 항상 기억하고 판단해야 하는 부담이 있음
  4. 비즈니스 모델링 문제: 정규화 등을 통해 구조적으로 해결할 수 있는 문제를 복잡한 코드로 우회함
  5. 확장성 저하: 향후 유사한 요구사항이 추가되면 더 복잡해질 우려가 있음

1.

BaseTimeEntity의 원래 목적은 엔티티의 생성 및 수정 시점을 저장하는 것이었어요. 그런데 새로운 코드가 추가되면서 BaseTimeEntity가 “특정한 경우 수정시각을 갱신하지 않는” 비즈니스 규칙까지 알게 되었어요. 이렇게 되면 단순히 타임스탬프를 관리하는 역할을 넘어서, 특정 자식 엔티티의 변경 패턴까지 고려해야 하는 로직이 추가된 거예요.

그 결과 BaseTimeEntity는 더 이상 단순한 시간 기록용 클래스가 아니게 되고, 상태 변경 조건에 따른 예외 로직까지 처리해야 하는 복잡한 기능을 수행하게 돼요. 이렇게 역할이 추가되면 유지보수가 어려워지고, 향후 요구사항 변경 시 문제를 일으킬 수 있다고 생각해요.

2.

PreUpdate, PostLoad, PostPersist, PostUpdate 같은 JPA Entity Listener 메서드를 사용해서 modifiedAt 필드 갱신 시점을 유연하게 제어하려는 시도는 언뜻 보면 유용해 보여요. 하지만 이렇게 하면 코드 이해가 매우 어려워져요. 코드는 항상 확장에 열려 있어야 하는데, 여기에 점점 로직이 추가되더라도 괜찮을까요? 우리는 "이거 변경되지 않을 것 같아"라고 생각했지만 결국 정책 변경으로 구조를 바꿔야 하는 상황을 여러번 마주쳤어요.

  • 엔티티가 로드될 때 PostLoad가 호출되면서 lastKnownModifiedAt 필드가 설정되는데, 다른 개발자가 이 로직을 이해하려면 엔티티 라이프사이클 메서드 전체를 추적해야 해요.
  • markUnmodified를 언제 호출해야 하고, 호출하지 않으면 어떤 부작용이 생기는지 파악하려면 PreUpdate 로직, PostLoad 로직, 그리고 isModified 플래그 작동 원리를 전부 이해해야 해요.

이렇게 복잡한 동작 방식은 코드 가독성과 유지보수성을 떨어뜨리고, 새로운 팀원이나 미래에 우리리가 이 코드를 읽을 때 큰 진입 장벽이 될 거에요.

3.

켬미는 “필요할 때만 markUnmodified()를 호출하면 된다”고 했는데, 제가 곰곰이 생각해봤을때 이건 오히려 개발자에게 추가적인 인지 부담을 주는 것 같아요. “수정 시각을 갱신하지 말아야 하는 상황”이라는 비즈니스 조건을 항상 기억하고, 그 시점에 맞춰 markUnmodified()를 호출해야 하거든요.

비즈니스 로직이 복잡해질수록 이런 메서드 호출 규칙은 문서화하거나 코드 리뷰를 거치더라도 실수로 이어질 수 있어요. 호출을 누락하면 원하는 동작이 되지 않고, 잘못 호출하면 시간 갱신이 제대로 이루어지지 않을 수 있는 불안정한 패턴이라고 생각해요.

4.

likeCount 변화 시 modifiedAt을 갱신하지 않으려는 요구사항은 사실 Template 엔티티가 본질적으로 가지고 있는 수정시각 개념과는 동떨어진 요구사항이라고 생각해요. likeCount를 Template에서 분리하거나 별도 테이블(또는 엔티티)로 관리하면, 좋아요 변화가 발생하더라도 Template의 수정시각에 영향을 주지 않는 구조를 만드는 게 훨씬 명확하고 객체지향적이에요.

이렇게 DB 정규화를 통해 본질적으로 다른 변경 이벤트를 별도로 관리하면 비즈니스 로직을 더 명확하게 하고 유지보수성을 높일 수 있어요. 독립된 엔티티로 분리하면 BaseTimeEntity는 원래 목적(기본적인 생성/수정 시간 관리)에만 충실할 수 있어요.

5.

지금 markUnmodified() 메서드로 한 가지 특정 케이스(likeCount 변경 시 수정시각 변경 불필요)를 해결했어요. 그런데 비슷한 요구사항이 또 생기면 어떻게 될까요? 예를 들어, 다른 필드 변경에서도 수정 시간을 갱신하지 말거나 특정 조건 하에만 갱신을 막아야 하는 상황이 반복되면 BaseTimeEntity는 점점 복잡해질 거예요. 결국 markUnmodified() 방식이 남발되면서 유지보수가 힘들어지고, 비즈니스 로직과 Auditing 로직의 결합이 심화될 거예요.

반대로 정규화를 통한 구조적 개선은 이런 요구사항 변화에 훨씬 유연하게 대응할 수 있어요. 테이블 구조나 엔티티를 분리하면 데이터의 의미를 명확히 나눌 수 있어서, Transient 필드나 Lifecycle Hook을 사용해 우회하지 않아도 문제를 해결할 수 있어요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제우스 의견을 상세하게 알려줘서 고마워요~ 하지만 제 입장에서는 몇가지 주장이 아직 이해가 안되고 과연 더 나은 방향인지 모르겠어서 다시 제우스 의견을 토대로 이해 안되는 부분을 달아볼게요!

그렇지만 저는 제우스의 의견도 이해가 되어 다수결이 해당 방식을 원하고 해당 로직이 여전히 추후 어려움과 휴먼에러 가능성이 있다고 생각한다면 변경할 의향이 있습니다~!


1

객체지향적 설계 훼손: 비즈니스 로직이 BaseTimeEntity에 들어오면서 역할이 모호해짐

결론부터 말하면 해당 클래스를 단순하게 타임스탬프를 관리한다고 한다면 충분히 이해가 가는 주장인 것 같아요~!
저는 BaseTimeEntity의 역할을 다르게 생각하고 있었네요. 저는 해당 클래스가 시간을 관리하는 클래스라고 생각했고, 수정시간을 변경하지 않기 위해 시간과 관련된 비즈니스 규칙이 추가되는 것이 어색하지 않게 느껴졌어요.
이는 “특정 자식 엔티티의 변경 패턴까지 고려해야 하는 로직이 추가“한 것이 아닌 시간을 관리하는 또 다른 방법의 추가 로직이라고 생각했어요.

하지만 해당 클래스를 단순하게 타임스탬프를 관리한다고 한다면 충분히 이해가 가는 주장인 것 같아요~!

2

코드 복잡성 증가: Lifecycle Hook와 Transient 필드를 통한 우회 로직으로 엔티티 이해도가 떨어짐

주장의 내용이 ‘ Lifecycle Hook와 Transient 사용으로 인해 코드가 복잡하고 공부할 것이 생긴다 ’ 인 것 같은데 맞을까요?
그렇다면 이건 이해가 되지 않는 것 같아요. 저희가 JPA를 사용하여 개발 하고 있으니 기본적인 Lifecycle 동작 방식이 필요하다면 공부 하는 것이 맞다고 생각해요. ‘복잡하다’, ‘이해가 어렵다’라는 이유로 유용한 코드를 사용하지 않을 이유가 없다고 생각합니다.

markUnmodified를 언제 호출해야 하고, 호출하지 않으면 어떤 부작용이 생기는지 파악하려면 PreUpdate 로직, PostLoad 로직, 그리고 isModified 플래그 작동 원리를 전부 이해해야 해요.

이부분에 대해서는 문서를 명확하게 만들어놓는다면 이해를 도울 수 있을 것 같아요. 저희가 개발을 하면서 이해가 필요한 로직들을 문서화 하고 있는 것과 동일하다고 생각해요.

3

휴먼 에러 발생 가능성: markUnmodified() 호출 여부를 개발자가 항상 기억하고 판단해야 하는 부담이 있음

저는 여전히 동일한 입장입니다.. ㅎㅎ

“수정 시각을 갱신하지 말아야 하는 상황”이라는 비즈니스 조건을 항상 기억하고, 그 시점에 맞춰 markUnmodified()를 호출해야 하거든요.

저는 이 내용이 이해가 안되서 가장 비슷하다고 생각되는 사례로 말해볼게요. 저희는 flyway를 사용하면서 DB에 변경이 생기면 마이그레이션 파일을 생성해줘야합니다. 그러면 저희는 flyway도 “테이블이 변경되어야하는 상황”이라는 조건을 기억하고 그 시점에 마이그레이션 파일을 추가하는 것도 휴먼에러 가능성이 있는 거죠. 이정도의 휴먼에러 가능성은 동의합니다. 하지만 흔하지 않고 그 상황만 기억하고 익숙해진다면 해결되는 자연스러운 정도의 휴먼에러라고 생각합니다.

또한 수정 시간이 변경되지 않도록 해야 하는 로직이 드물것이라고 판단하기에 Flyway 마이그레이션처럼 리뷰와 문서로도 관리할 수 있다고 샹각해요.

4

비즈니스 모델링 문제: 정규화 등을 통해 구조적으로 해결할 수 있는 문제를 복잡한 코드로 우회함

likeCount 변화 시 modifiedAt을 갱신하지 않으려는 요구사항은 사실 Template 엔티티가 본질적으로 가지고 있는 수정시각 개념과는 동떨어진 요구사항이라고 생각해요.

해당 주장은 동의합니다~!

5

확장성 저하: 향후 유사한 요구사항이 추가되면 더 복잡해질 우려가 있음

다른 필드 변경에서도 수정 시간을 갱신하지 말거나 특정 조건 하에만 갱신을 막아야 하는 상황이 반복되면 BaseTimeEntity는 점점 복잡해질 거예요.

이부분도 이해가 잘되지 않네요 유사 요구사항이 생기다면 단지 markUnmodified() 호출로 이전 코드를 재사용할 수 있어 더 좋다고 생각합니다.
오히려 그럼 그때도 테이블을 분리한다면 너무 많은 테이블이 생기고 DB의 데이터 용량이 커질 거라고 생각해요.

@zangsu
Copy link
Contributor

zangsu commented Dec 12, 2024

제우스 말 대로 휴먼 에러 발생 가능성이 크다고 생각합니다.
다만, 켬미가 정규화를 선택하지 않은 이유가 "공개범위나 카테고리 변경 시에도 업데이트 되는게 싫다 라는 의견도 있었어서" 인 것 같아서 어렵네요,,

정규화를 선택하게 된다면 "본인이 수정하지 않는 값인 좋아요 수에 대해서만 수정 내역에서 제외" 하는 정책이 되겠네요. 저는 지금의 코드도 "개선이 필요하지만, 버그 픽스를 위해 필요한 코드" 정도로 수용 가능할 것 같아 우선 approve 남겨두겠습니다...!

Copy link
Contributor

@zangsu zangsu left a comment

Choose a reason for hiding this comment

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

생각보다 이 문제가 엄청 거대하군요,,,
고생 많으셨어요 켬미~~

@kyum-q
Copy link
Contributor Author

kyum-q commented Dec 14, 2024

회의 끝에 SkipModifiedAtUpdateBaseTimeEntity을 구현하였습니다~ 해당 commit을 확인해주세요!

Copy link
Contributor

@jminkkk jminkkk left a comment

Choose a reason for hiding this comment

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

불꽃 토론 👍
덕분에 BaseTime 역할이 명확해졌네요!

Copy link
Contributor

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

컨벤션과 네이밍 측면에서 개선할 점이 있다고 생각해 RC 할게요~!

반영하고 언급해주면 바로 approve 하겠습니다

@kyum-q
Copy link
Contributor Author

kyum-q commented Dec 15, 2024

@BE 또 다른 변경사항이 생겨서 이에 대해 코멘트로 남겨둘게요~! 확인 부탁드려요~ 다른 방식이 있다면 언제나 의견 환영합니다~

수정 사항:

updateTemplate() 메서드를 호출한 경우(크게 보면 템플릿 업데이트 API가 호출된 경우)에 템플릿의 modifiedAt이 업데이트 되도록 다음 커밋과 같이 수정했습니다.

이유:

현재는 변경사항이 없으면 updateTemplate이 호출되어도 템플릿의 modifiedAt이 업데이트 되지 않습니다. 그 예시로 소스코드만 업데이트했을 때 템플릿의 modifiedAt이 업데이트 되지 않습니다. 하지만 이 수정 시간이 템플릿 상세 페이지에서 보이는 데이터이다 보니 updateTemplate() 메서드를 호출한 경우에 템플릿의 modifiedAt이 업데이트되어야한다고 생각합니다!

Copy link
Contributor

@HoeSeong123 HoeSeong123 left a comment

Choose a reason for hiding this comment

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

오우 굉장히 치명적인 오류였네요.
여태 몰랐다니..
멋있습니다👍

@kyum-q kyum-q merged commit 4d4b164 into dev/be Dec 16, 2024
3 checks passed
@kyum-q kyum-q deleted the fix/925-not-change-modifiedAt branch December 16, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 bug 개발자가 의도하지 않은 상황
Projects
Status: Weekend Done
Development

Successfully merging this pull request may close these issues.

5 participants