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

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

Merged
merged 9 commits into from
Nov 17, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,21 @@ public interface HighlightRepository extends Repository<Highlight, Long>, Highli

boolean existsById(long id);

@Modifying
@Query("""
DELETE FROM Highlight h
SELECT h FROM Highlight h
WHERE h.answerId IN :answerIds
ORDER BY h.lineIndex, h.highlightRange.startIndex ASC
""")
void deleteAllByAnswerIds(Collection<Long> answerIds);
List<Highlight> findAllByAnswerIdsOrderedAsc(Collection<Long> answerIds);

@Modifying
@Query("""
SELECT h
FROM Highlight h
WHERE h.answerId IN :answerIds
ORDER BY h.lineIndex, h.highlightRange.startIndex ASC
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
)
Copy link
Contributor

Choose a reason for hiding this comment

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

        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

요것은 안될까용

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<Highlight> findAllByAnswerIdsOrderedAsc(Collection<Long> answerIds);
void deleteByReviewGroupIdAndQuestionId(long reviewGroupId, long questionId);
}
Original file line number Diff line number Diff line change
@@ -1,35 +1,33 @@
package reviewme.highlight.service;

import java.util.List;
import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import reviewme.highlight.domain.Highlight;
import reviewme.highlight.repository.HighlightRepository;
import reviewme.highlight.service.dto.HighlightsRequest;
import reviewme.highlight.service.mapper.HighlightMapper;
import reviewme.highlight.service.validator.HighlightValidator;
import reviewme.review.repository.AnswerRepository;
import reviewme.review.service.validator.AnswerValidator;
import reviewme.reviewgroup.domain.ReviewGroup;

@Service
@RequiredArgsConstructor
public class HighlightService {

private final HighlightRepository highlightRepository;
private final AnswerRepository answerRepository;

private final HighlightValidator highlightValidator;
private final HighlightMapper highlightMapper;
private final AnswerValidator answerValidator;

@Transactional
public void editHighlight(HighlightsRequest highlightsRequest, ReviewGroup reviewGroup) {
highlightValidator.validate(highlightsRequest, reviewGroup);
List<Highlight> highlights = highlightMapper.mapToHighlights(highlightsRequest);
List<Long> requestedAnswerIds = highlightsRequest.getUniqueAnswerIds();
answerValidator.validateQuestionContainsAnswers(highlightsRequest.questionId(), requestedAnswerIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

오호....

answerValidator.validateReviewGroupContainsAnswers(reviewGroup, requestedAnswerIds);

Set<Long> answerIds = answerRepository.findIdsByQuestionId(highlightsRequest.questionId());
highlightRepository.deleteAllByAnswerIds(answerIds);
List<Highlight> highlights = highlightMapper.mapToHighlights(highlightsRequest);
highlightRepository.deleteByReviewGroupIdAndQuestionId(reviewGroup.getId(), highlightsRequest.questionId());
highlightRepository.saveAll(highlights);
}
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package reviewme.review.service.exception;

import java.util.Collection;
import lombok.extern.slf4j.Slf4j;
import reviewme.global.exception.ReviewMeException;

@Slf4j
public class QuestionNotContainingAnswersException extends ReviewMeException {

public QuestionNotContainingAnswersException(long questionId, Collection<Long> providedAnswerIds) {
super("질문에 속하지 않는 답변이예요.");
log.info("Question not containing provided answers - questionId: {}, providedAnswerIds: {}",
questionId, providedAnswerIds);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package reviewme.review.service.exception;

import java.util.Collection;
import lombok.extern.slf4j.Slf4j;
import reviewme.global.exception.ReviewMeException;

@Slf4j
public class ReviewGroupNotContainingAnswersException extends ReviewMeException {

public ReviewGroupNotContainingAnswersException(long reviewGroupId, Collection<Long> providedAnswerIds) {
super("리뷰 그룹에 속하지 않는 답변이예요.");
log.info("ReviewGroup not containing provided answers - reviewGroupId: {}, providedAnswerIds: {}",
reviewGroupId, providedAnswerIds);
}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,31 @@
package reviewme.review.service.validator;

import reviewme.review.domain.Answer;
import java.util.Collection;
import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import reviewme.review.repository.AnswerRepository;
import reviewme.review.service.exception.QuestionNotContainingAnswersException;
import reviewme.review.service.exception.ReviewGroupNotContainingAnswersException;
import reviewme.reviewgroup.domain.ReviewGroup;

public interface AnswerValidator {
@Component
@RequiredArgsConstructor
public class AnswerValidator {

boolean supports(Class<? extends Answer> answerClass);
private final AnswerRepository answerRepository;

void validate(Answer answer);
public void validateQuestionContainsAnswers(long questionId, Collection<Long> answerIds) {
Set<Long> receivedAnswerIds = answerRepository.findIdsByQuestionId(questionId);
if (!receivedAnswerIds.containsAll(answerIds)) {
throw new QuestionNotContainingAnswersException(questionId, answerIds);
}
}

public void validateReviewGroupContainsAnswers(ReviewGroup reviewGroup, Collection<Long> answerIds) {
Set<Long> receivedAnswerIds = answerRepository.findIdsByReviewGroupId(reviewGroup.getId());
if (!receivedAnswerIds.containsAll(answerIds)) {
throw new ReviewGroupNotContainingAnswersException(reviewGroup.getId(), answerIds);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

@Component
@RequiredArgsConstructor(access = AccessLevel.PROTECTED)
public class CheckboxAnswerValidator implements AnswerValidator {
public class CheckboxTypedAnswerValidator implements TypedAnswerValidator {

private final QuestionRepository questionRepository;
private final OptionGroupRepository optionGroupRepository;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@
import lombok.AccessLevel;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import reviewme.template.domain.Question;
import reviewme.template.repository.QuestionRepository;
import reviewme.review.domain.Answer;
import reviewme.review.domain.CheckboxAnswerSelectedOption;
import reviewme.review.domain.CheckboxAnswer;
import reviewme.review.domain.CheckboxAnswerSelectedOption;
import reviewme.review.domain.Review;
import reviewme.review.service.exception.MissingRequiredQuestionException;
import reviewme.review.service.exception.SubmittedQuestionAndProvidedQuestionMismatchException;
import reviewme.template.domain.Question;
import reviewme.template.domain.Section;
import reviewme.template.domain.SectionQuestion;
import reviewme.template.repository.QuestionRepository;
import reviewme.template.repository.SectionRepository;

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

private final AnswerValidatorFactory answerValidatorFactory;
private final TypedAnswerValidatorFactory typedAnswerValidatorFactory;

private final SectionRepository sectionRepository;
private final QuestionRepository questionRepository;
Expand All @@ -36,8 +36,8 @@ public void validate(Review review) {

private void validateAnswer(List<Answer> answers) {
for (Answer answer : answers) {
AnswerValidator validator = answerValidatorFactory.getAnswerValidator(answer.getClass());
validator.validate(answer);
typedAnswerValidatorFactory.getAnswerValidator(answer.getClass())
.validate(answer);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@

@Component
@RequiredArgsConstructor(access = AccessLevel.PROTECTED)
public class TextAnswerValidator implements AnswerValidator {
public class TextTypedAnswerValidator implements TypedAnswerValidator {

private static final int ZERO_LENGTH = 0;
private static final int MIN_LENGTH = 20;
private static final int MAX_LENGTH = 1_000;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package reviewme.review.service.validator;

import reviewme.review.domain.Answer;

public interface TypedAnswerValidator {

boolean supports(Class<? extends Answer> answerClass);

void validate(Answer answer);
}
Comment on lines +5 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@

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

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

public AnswerValidator getAnswerValidator(Class<? extends Answer> answerClass) {
return answerValidators.stream()
public TypedAnswerValidator getAnswerValidator(Class<? extends Answer> answerClass) {
return validators.stream()
.filter(validator -> validator.supports(answerClass))
.findFirst()
.orElseThrow(() -> new UnsupportedAnswerTypeException(answerClass));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,34 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertAll;
import static reviewme.fixture.ReviewGroupFixture.리뷰_그룹;

import java.util.ArrayList;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;
import reviewme.highlight.domain.Highlight;
import reviewme.highlight.domain.HighlightRange;
import reviewme.review.domain.Answer;
import reviewme.review.domain.Review;
import reviewme.review.domain.TextAnswer;
import reviewme.review.repository.ReviewRepository;
import reviewme.reviewgroup.domain.ReviewGroup;
import reviewme.reviewgroup.repository.ReviewGroupRepository;

@DataJpaTest
class HighlightRepositoryTest {

@Autowired
private HighlightRepository highlightRepository;

@Autowired
private ReviewRepository reviewRepository;

@Autowired
private ReviewGroupRepository reviewGroupRepository;

@Test
void 한_번에_여러_하이라이트를_벌크_삽입한다() {
// given
Expand Down Expand Up @@ -62,4 +76,43 @@ class HighlightRepositoryTest {
.containsExactly(1, 4, 2, 6, 3)
);
}

@Test
void 그룹_아이디와_질문_아이디로_하이라이트를_삭제한다() {
// given
ReviewGroup reviewGroup1 = reviewGroupRepository.save(리뷰_그룹());
ReviewGroup reviewGroup2 = reviewGroupRepository.save(리뷰_그룹());

List<Answer> answers1 = List.of(
new TextAnswer(1L, "A1"),
new TextAnswer(2L, "A2"),
new TextAnswer(3L, "A3")
);
List<Answer> answers2 = List.of(
new TextAnswer(1L, "B1"),
new TextAnswer(2L, "B2"),
new TextAnswer(3L, "B3")
);
reviewRepository.save(new Review(1L, reviewGroup1.getId(), answers1));
reviewRepository.save(new Review(2L, reviewGroup2.getId(), answers2));

List<Long> answerIds = new ArrayList<>();
answerIds.addAll(answers1.stream().map(Answer::getId).toList());
answerIds.addAll(answers2.stream().map(Answer::getId).toList());

HighlightRange range = new HighlightRange(0, 1);
answerIds.stream()
.map(answerId -> new Highlight(answerId, 0, range))
.forEach(highlightRepository::save);

// when
highlightRepository.deleteByReviewGroupIdAndQuestionId(reviewGroup1.getId(), 1L);

// then
List<Highlight> actual = highlightRepository.findAllByAnswerIdsOrderedAsc(answerIds);
assertAll(
() -> assertThat(actual).hasSize(5),
() -> assertThat(actual).extracting(Highlight::getAnswerId).doesNotContain(answers1.get(0).getId())
);
}
}
Loading