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

Open
wants to merge 5 commits into
base: dev/be
Choose a base branch
from

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()만 사용하는게 더 나을 것 같다고 생각이 들기도 하네요 ~
이거에 대해 어떻게 생각하시나요?

@@ -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.

저도 같은 의견이에요~!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 bug 개발자가 의도하지 않은 상황
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants