[BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 #966

donghoony · 2024-11-15T07:38:23Z

resolves [BE] HighlightValidator의 역할을 Answer 쪽으로 이동한다. #965

🚀 어떤 기능을 구현했나요 ?

965에서 설명했습니다.
간단하게 이야기하면 HighlightValidator가 하는 일이 Highlight과 무관합니다. 따라서 리뷰 그룹/답변을 검증하는 게 Highlight 패키지 아래에 있는 게 어색합니다. 처음에는 아래와 같은 두 가지 TODO를 만들었어요.

        // TODO: request으로부터 아래 두 가지를 검증한다.
        // 1. answerId들이 각각 question에 포함되는지 확인한다.
        // 2. answerId들이 각각 reviewGroup에 포함되는지 확인한다.
        // validator 내부의 메서드도 재사용 가능하도록 작성되어야 한다.
        // 하지만 현재 각 validator들은 하나의 기능을 위해 존재하며, 리팩터링에 어려움이 있다.

🔥 어떻게 해결했나요 ?

우선 기존 AnswerValidator는 타입별로 검증하는 기능을 제공하므로 TypedAnswerValidator라고 이름을 수정했습니다.
AnswerValidator에서 정답에 대한 검증을 수행하도록 책임을 부여했습니다. HighlightValidator가 수행하던 두 가지 검증을 이곳에서 진행합니다.
하이라이트 삭제 로직에 중대한 오류가 있었습니다. 기존 코드는 아래와 같은데요, Question에 대한 모든 답안을 리뷰 그룹 상관없이 삭제합니다. 내가 하이라이트를 수정하면 다른 사람의 같은 질문 하이라이트가 사라집니다...

Set<Long> answerIds = answerRepository.findIdsByQuestionId(highlightsRequest.questionId());
highlightRepository.deleteAllByAnswerIds(answerIds);

아래와 같이 JOIN을 활용해 reviewGroup까지도 들어가 삭제하도록 합니다. 서브쿼리 있긴 한데, SELECT + DELETE와 같아서 추가 부하는 없을 것이라고 기대해요.

highlightRepository.deleteByReviewGroupIdAndQuestionId(reviewGroup.getId(), highlightsRequest.questionId());
//
    @Modifying
    @Query("""
            DELETE FROM Highlight highlight WHERE highlight in (
                SELECT h FROM Highlight h
                JOIN Answer a ON h.answerId = a.id
                JOIN Review r ON a.reviewId = r.id
                WHERE r.reviewGroupId = :reviewGroupId AND a.questionId = :questionId
            )
            """)
    void deleteByReviewGroupIdAndQuestionId(long reviewGroupId, long questionId);

📝 어떤 부분에 집중해서 리뷰해야 할까요?

급할 때일수록 천천히.. 꼼꼼히 확인해주시고 Validator 삭제에 대한 근거가 확실한지 봐주세요.
예외를 추가했는데 적절한지 봐주세요.

📚 참고 자료, 할 말

Validator가 하나의 서비스 메서드에 묶여서 확장에 어려움을 겪는 경우가 몇군데 있습니다. 이걸 잘 풀어내서 재사용성을 높이면 좋겠어요.

github-actions · 2024-11-15T07:39:57Z

Test Results

156 tests ±0 153 ✅ ±0 4s ⏱️ -1s
59 suites ±0 3 💤 ±0
59 files ±0 0 ❌ ±0

Results for commit 1ae78ed. ± Comparison against base commit 9b59f8d.

This pull request removes 15 and adds 15 tests. Note that renamed tests count towards both.

reviewme.highlight.service.validator.HighlightValidatorTest ‑ 하이라이트의_답변_id가_리뷰_그룹에_달린_답변이_아니면_예외를_발생한다()
reviewme.highlight.service.validator.HighlightValidatorTest ‑ 하이라이트의_답변_id가_하이라이트의_질문_id에_해당하는_답변이_아니면_예외를_발생한다()
reviewme.highlight.service.validator.HighlightValidatorTest ‑ 하이라이트의_질문_id가_리뷰_그룹의_템플릿에_속한_질문이_아니면_예외를_발생한다()
reviewme.review.service.validator.AnswerValidatorFactoryTest ‑ 지원하는_타입에_따른_밸리데이터를_가져온다()
reviewme.review.service.validator.AnswerValidatorFactoryTest ‑ 지원하지_않는_타입에_대한_밸리데이터_요청_시_예외가_발생한다()
reviewme.review.service.validator.CheckboxAnswerValidatorTest ‑ 옵션_그룹이_지정되지_않은_질문에_대한_답변이면_예외가_발생한다()
reviewme.review.service.validator.CheckboxAnswerValidatorTest ‑ 옵션그룹에서_정한_최대_선택_수_보다_많이_선택하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxAnswerValidatorTest ‑ 옵션그룹에서_정한_최소_선택_수_보다_적게_선택하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxAnswerValidatorTest ‑ 옵션그룹에서_제공하지_않은_옵션아이템을_응답하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxAnswerValidatorTest ‑ 저장되지_않은_질문에_대한_답변이면_예외가_발생한다()
…

reviewme.highlight.repository.HighlightRepositoryTest ‑ 그룹_아이디와_질문_아이디로_하이라이트를_삭제한다()
reviewme.review.service.validator.AnswerValidatorTest ‑ 답변이_리뷰그룹에_속하는지_검증한다()
reviewme.review.service.validator.AnswerValidatorTest ‑ 답변이_질문에_속하는지_검증한다()
reviewme.review.service.validator.CheckboxTypedAnswerValidatorTest ‑ 옵션_그룹이_지정되지_않은_질문에_대한_답변이면_예외가_발생한다()
reviewme.review.service.validator.CheckboxTypedAnswerValidatorTest ‑ 옵션그룹에서_정한_최대_선택_수_보다_많이_선택하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxTypedAnswerValidatorTest ‑ 옵션그룹에서_정한_최소_선택_수_보다_적게_선택하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxTypedAnswerValidatorTest ‑ 옵션그룹에서_제공하지_않은_옵션아이템을_응답하면_예외가_발생한다()
reviewme.review.service.validator.CheckboxTypedAnswerValidatorTest ‑ 저장되지_않은_질문에_대한_답변이면_예외가_발생한다()
reviewme.review.service.validator.TextTypedAnswerValidatorTest ‑ [1] length=19
reviewme.review.service.validator.TextTypedAnswerValidatorTest ‑ [2] length=10001
…

♻️ This comment has been updated with latest results.

Kimprodp

하나만 확인해줍쇼!

Kimprodp · 2024-11-16T03:31:01Z

backend/src/main/java/reviewme/highlight/repository/HighlightRepository.java

+                JOIN Answer a ON h.answerId = a.id
+                JOIN Review r ON a.reviewId = r.id
+                WHERE r.reviewGroupId = :reviewGroupId AND a.questionId = :questionId
+            )


DELETE FROM Highlight h WHERE h.answerId IN ( SELECT a.id FROM Answer a JOIN Review r ON a.reviewId = r.id WHERE r.reviewGroupId = :reviewGroupId AND a.questionId = :questionId

요것은 안될까용

조인 하나 줄여버린 테드... 👏🏻

Kimprodp · 2024-11-16T03:31:32Z

backend/src/main/java/reviewme/highlight/service/HighlightService.java

-        Set<Long> answerIds = answerRepository.findIdsByQuestionId(highlightsRequest.questionId());
-        highlightRepository.deleteAllByAnswerIds(answerIds);
+        List<Long> requestedAnswerIds = highlightsRequest.getUniqueAnswerIds();
+        answerValidator.validateQuestionContainsAnswers(highlightsRequest.questionId(), requestedAnswerIds);


nayonsoso

커밋이 잘 나눠져있어서 읽기 편했습니다!!😁
validator 삭제 & answer 로의 이동은 계속해서 말해왔던 것이기 때문에 쉽게 납득 & 이해할 수 있었어요!!
그런데 새로 만들어진 deleteByReviewGroupIdAndQuestionId 함수에 대해 테스트 코드 작성된게 없어 RC 드립니당~~

내가 하이라이트를 수정하면 다른 사람의 같은 질문 하이라이트가 사라집니다...

그리고 이건 정말로 그렇네요… 😱😱😱
아이고 이걸 왜 몰랐을까요 ㅠㅠㅠ

nayonsoso · 2024-11-16T04:06:40Z

backend/src/main/java/reviewme/highlight/repository/HighlightRepository.java

+    @Modifying
    @Query("""
-            SELECT h
-            FROM Highlight h
-            WHERE h.answerId IN :answerIds
-            ORDER BY h.lineIndex, h.highlightRange.startIndex ASC
+            DELETE FROM Highlight highlight WHERE highlight in (
+                SELECT h FROM Highlight h
+                JOIN Answer a ON h.answerId = a.id
+                JOIN Review r ON a.reviewId = r.id
+                WHERE r.reviewGroupId = :reviewGroupId AND a.questionId = :questionId
+            )


서브쿼리 vs exist 중에서 뭐가 더 성능이 좋을까 싶었는데
실행 계획을 보니 서브쿼리가 더 성능이 좋네요~ 굳굳👍

인덱스만 잘 태운다면..

바뀐 쿼리에 where 조건 컬럼들이 모두 인덱스가 적용되어있어서 괜찮겠네요👍👍

nayonsoso · 2024-11-16T04:09:57Z

backend/src/main/java/reviewme/review/service/validator/AnswerValidatorFactory.java

 @RequiredArgsConstructor(access = AccessLevel.PROTECTED)
 public class AnswerValidatorFactory {

-    private final List<AnswerValidator> answerValidators;
+    private final List<TypedAnswerValidator> validators;


이 클래스의 이름도 TypedAnswerValidatorFactory 로 바꿔줄 필요는 없을까요? 🤔

반영했습니다 🛫

nayonsoso · 2024-11-16T04:15:22Z

backend/src/main/java/reviewme/review/service/validator/TypedAnswerValidator.java

+public interface TypedAnswerValidator {
+
+    boolean supports(Class<? extends Answer> answerClass);
+
+    void validate(Answer answer);
+}


저는 지난 PR(#958) 에서 AnwerMapper 를 추상 클래스로만들었어요!
그 이유는 AnswerMapper 와 이를 상속하는 CheckboxAnswerMapper 등이 is 관계라고 생각했으며, CheckboxAnswerMapper 가 다른 클래스를 상속할 일도 없기 때문에 굳이 인터페이스로 만들 필요가 없다고 생각하기 때문이에요.

저는 이런 관점에서 TypedAnswerValidator 도 추상 클래스가 되어야 한다고 생각하는데, 아루는 어떻게 생각하나요?

is-a, has-a도 상속을 구분하는 하나의 방법이지만, 추상 클래스 또한 인터페이스에 비하면 구체화된 존재라고 할 수 있습니다. 구체 클래스보다는 인터페이스에 의존하는 게 더 유연할 것 같아서 이대로 두었어요.

skylar1220

HighlightValidator가 하는 일이 Highlight과 무관합니다. 따라서 리뷰 그룹/답변을 검증하는 게 Highlight 패키지 아래에 있는 게 어색합니다.

와우.. 하이라이트에 필요한 검증이니까 당연히 highlightValidator에서 한다고 생각했는데, 이렇게 생각해볼 수 있었네요. 생각 포인트 하나 더 알고 갑니다😂

내가 하이라이트를 수정하면 다른 사람의 같은 질문 하이라이트가 사라집니다...

😱죄송합니다 여러분....ㅎㅎㅎㅎ 왜 눈치 못챘지..

단순 제안 하나있습니다!

skylar1220 · 2024-11-16T12:17:53Z

backend/src/main/java/reviewme/review/service/validator/AnswerValidator.java

AnswerValidator라는 네이밍이 광범위하지 않나 싶어요.
추후에 어떤 역할이 여기에 추가될지 모른다는 걸 가정한다면 가능할수도 있겠지만,
현재는 question, reviewGroup에 해당하는 답변이 존재하는지 확인하는 역할의 응집성을 갖고 있으니 AnswerExistenceValidator는 어떨까요?

추후에 어떤 역할이 여기에 추가될지 모른다

저는 여기에 상당한 무게를 두었는데요, 개인적으로는 validator, mapper의 확장을 고려하고 있지 않습니다. 단지 매핑/검증용으로 나온 절차지향 중심 객체라고 생각해서요.

validator, mapper를 대체하는 것을 고려한다면 여기가 최적의 장소가 되겠군요! 공감합니다~

Kimprodp

👍 왕따봉

skylar1220

책임 책임 책임🥲🥲🥲 고민이 제일 어려웠는데 잘 바꿔줘서 배워갑니다...

donghoony · 2024-11-17T09:14:53Z

rebase & force push했어요

nayonsoso

확인했습니다😊👍

donghoony added ✨ BE 🐛 버그 🛠️ 리팩터링 labels Nov 15, 2024

donghoony added this to the 6차 스프린트: 최종장 milestone Nov 15, 2024

donghoony self-assigned this Nov 15, 2024

donghoony requested review from Kimprodp, nayonsoso and skylar1220 as code owners November 15, 2024 07:38

donghoony changed the title ~~[BE] 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정~~ [BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 Nov 15, 2024

donghoony added the 😱 긴급 label Nov 15, 2024

Kimprodp requested changes Nov 16, 2024

View reviewed changes

nayonsoso requested changes Nov 16, 2024

View reviewed changes

skylar1220 requested changes Nov 16, 2024

View reviewed changes

Kimprodp approved these changes Nov 17, 2024

View reviewed changes

donghoony mentioned this pull request Nov 17, 2024

[Release] 리뷰미 v2.0.5 배포 #968

Merged

skylar1220 approved these changes Nov 17, 2024

View reviewed changes

donghoony added 9 commits November 17, 2024 18:14

refactor: HighlightValidator 삭제

9920cb7

chore: add todos

c6a0b3b

refactor: 타입에 따라 검증하는 객체 이름에 Typed-를 접두사로 추가

35ec105

feat: AnswerValidator에서 정답과 관련된 검증 진행

70d4646

refactor: List -> Collection 받도록 수정

c11304d

fix: 리뷰 그룹에 해당하는 하이라이트만 삭제하도록 수정

23eac25

refactor: 사용하지 않는 상수 삭제

c7a6514

refactor: TypedAnswerValidatorFactory로 수정

59dcdbc

refactor: 쿼리 수정, 테스트 추가

1ae78ed

donghoony force-pushed the be/refactor/highlight-validation branch from 7a33c4d to 1ae78ed Compare November 17, 2024 09:14

nayonsoso approved these changes Nov 17, 2024

View reviewed changes

donghoony merged commit 9e017a1 into develop Nov 17, 2024
3 checks passed

donghoony deleted the be/refactor/highlight-validation branch November 17, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 #966

[BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 #966

donghoony commented Nov 15, 2024

github-actions bot commented Nov 15, 2024 •

edited

Loading

Kimprodp left a comment

Kimprodp Nov 16, 2024

donghoony Nov 16, 2024

Kimprodp Nov 16, 2024

nayonsoso left a comment

nayonsoso Nov 16, 2024

donghoony Nov 16, 2024

skylar1220 Nov 16, 2024

nayonsoso Nov 16, 2024

donghoony Nov 16, 2024

nayonsoso Nov 16, 2024

donghoony Nov 16, 2024

skylar1220 left a comment

skylar1220 Nov 16, 2024

donghoony Nov 17, 2024

skylar1220 Nov 17, 2024

Kimprodp left a comment

skylar1220 left a comment

donghoony commented Nov 17, 2024

nayonsoso left a comment

[BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 #966

[BE] refactor/fix: 하이라이트 검증 객체 책임 이동, 하이라이트 삭제 로직 수정 #966

Conversation

donghoony commented Nov 15, 2024

🚀 어떤 기능을 구현했나요 ?

🔥 어떻게 해결했나요 ?

📝 어떤 부분에 집중해서 리뷰해야 할까요?

📚 참고 자료, 할 말

github-actions bot commented Nov 15, 2024 • edited Loading

Test Results

Kimprodp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayonsoso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylar1220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimprodp left a comment

Choose a reason for hiding this comment

skylar1220 left a comment

Choose a reason for hiding this comment

donghoony commented Nov 17, 2024

nayonsoso left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2024 •

edited

Loading