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

[BE] refactor: TemplateMapper에서 ReviewGroup 의존성 제거 #1011

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[BE] refactor: TemplateMapper에서 ReviewGroup 의존성 제거 #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion backend/src/main/java/reviewme/template/service/TemplateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package reviewme.template.service;

import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import reviewme.reviewgroup.domain.ReviewGroup;
import reviewme.reviewgroup.service.ReviewGroupService;
import reviewme.template.service.dto.response.SectionResponse;
import reviewme.template.service.dto.response.TemplateResponse;
import reviewme.template.service.mapper.TemplateMapper;

Expand All @@ -18,6 +20,11 @@ public class TemplateService {
@Transactional(readOnly = true)
public TemplateResponse generateReviewForm(String reviewRequestCode) {
ReviewGroup reviewGroup = reviewGroupService.getReviewGroupByReviewRequestCode(reviewRequestCode);
return templateMapper.mapToTemplateResponse(reviewGroup);
List<SectionResponse> sectionResponses = templateMapper.mapSectionResponses(
reviewGroup.getId(), reviewGroup.getTemplateId()
);
return new TemplateResponse(
reviewGroup.getTemplateId(), reviewGroup.getReviewee(), reviewGroup.getProjectName(), sectionResponses
);
}
}
19 changes: 4 additions & 15 deletions backend/src/main/java/reviewme/template/service/mapper/TemplateMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import reviewme.reviewgroup.domain.ReviewGroup;
import reviewme.template.domain.OptionGroup;
import reviewme.template.domain.OptionItem;
import reviewme.template.domain.Question;
Expand All @@ -20,7 +19,6 @@
import reviewme.template.service.dto.response.OptionItemResponse;
import reviewme.template.service.dto.response.QuestionResponse;
import reviewme.template.service.dto.response.SectionResponse;
import reviewme.template.service.dto.response.TemplateResponse;
import reviewme.template.service.exception.MissingOptionItemsInOptionGroupException;
import reviewme.template.service.exception.QuestionInSectionNotFoundException;
import reviewme.template.service.exception.SectionInTemplateNotFoundException;
Expand All @@ -38,23 +36,14 @@ public class TemplateMapper {
private final OptionGroupRepository optionGroupRepository;
private final OptionItemRepository optionItemRepository;

public TemplateResponse mapToTemplateResponse(ReviewGroup reviewGroup) {
Template template = templateRepository.findById(reviewGroup.getTemplateId())
.orElseThrow(() -> new TemplateNotFoundByReviewGroupException(
reviewGroup.getId(), reviewGroup.getTemplateId()
));
public List<SectionResponse> mapSectionResponses(long templateId, long reviewGroupId) {
Template template = templateRepository.findById(templateId)
.orElseThrow(() -> new TemplateNotFoundByReviewGroupException(reviewGroupId, templateId));

List<SectionResponse> sectionResponses = template.getSectionIds()
return template.getSectionIds()
Comment on lines +39 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

reviewGroupId가 예외 로그에만 사용되고 있네요. 그럼에도 이 함수의 인자로 전달해준 이유는 "특정 리뷰 그룹의 템플릿 아이디에 대한 템플릿이 없다"는 구체적인 예외 발생 맥락을 전달하기 위함이 맞나요?

(티키타카 오래걸릴까봐 미뤄 짐작하고 이어 답변하자면 ... )
제가 레포지토리에서 예외를 발생하는 것을 맡았는데, 그럼 이 경우에는

  • template 레포지토리에서 특정 값 조회했을 때 없었다는 것
  • reviewGroupId 에 해당하는 template 이었다는 것

을 담기 위해서 아래와 같이 try-catch로 구현하려는데 이런 의도가 맞나요?

try {
    Template template = templateRepository.findById(templateId);
     // ... 블라블라
} catch (NotFoundException ex) {
    throw new TemplateNotFoundByReviewGroupException(reviewGroupId, templateId);
}

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. Repository로 예외를 옮김으로써 서비스에서 반복적인 코드를 줄일 수 있다.
  2. 이때, 서비스단에서 남기고 있던 추가 맥락을 로깅할 수 없게 된다 (트레이드 오프)
  3. 해당 부분은 AOP 로깅 과정에서 파라미터를 로깅하는 방식으로 추적할 수 있다.

디코 대화도 남깁니다~

하지만 레포로 넘기면 엔티티단에서의 예외 처리, 서비스단에서의 코드 중복이 없는 대신 맥락이 조금 모자라지겠네요
해당 맥락은 AOP로 충당하는 방식으로 갈 건지~ 아니면 이대로 서비스에 둘 건지~가 되겠군요
아마 전자가 조금 더 합리적일 것이라는 생각입니당
AOP 로깅할 때 예외 발생했을 때에만 스택트레이스의 파라미터를 로깅할 수 있을지 확인해봐야겠어요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

결론: try-catch를 사용하지 말고, 예외를 최소화한 뒤 맥락은 AOP를 통해 제공하는 것을 고려해 보자

.stream()
.map(this::mapToSectionResponse)
.toList();

return new TemplateResponse(
template.getId(),
reviewGroup.getReviewee(),
reviewGroup.getProjectName(),
sectionResponses
);
}

private SectionResponse mapToSectionResponse(TemplateSection templateSection) {
Expand Down
65 changes: 35 additions & 30 deletions backend/src/test/java/reviewme/template/service/mapper/TemplateMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,22 @@
import java.util.List;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import reviewme.reviewgroup.domain.ReviewGroup;
import reviewme.reviewgroup.repository.ReviewGroupRepository;
import reviewme.support.ServiceTest;
import reviewme.template.domain.OptionGroup;
import reviewme.template.domain.Question;
import reviewme.template.domain.Section;
import reviewme.template.domain.Template;
import reviewme.template.repository.OptionGroupRepository;
import reviewme.template.repository.OptionItemRepository;
import reviewme.template.repository.QuestionRepository;
import reviewme.reviewgroup.domain.ReviewGroup;
import reviewme.reviewgroup.repository.ReviewGroupRepository;
import reviewme.support.ServiceTest;
import reviewme.template.domain.Section;
import reviewme.template.service.exception.MissingOptionItemsInOptionGroupException;
import reviewme.template.service.exception.SectionInTemplateNotFoundException;
import reviewme.template.repository.SectionRepository;
import reviewme.template.repository.TemplateRepository;
import reviewme.template.service.dto.response.QuestionResponse;
import reviewme.template.service.dto.response.SectionResponse;
import reviewme.template.service.dto.response.TemplateResponse;
import reviewme.template.service.exception.MissingOptionItemsInOptionGroupException;
import reviewme.template.service.exception.SectionInTemplateNotFoundException;

@ServiceTest
class TemplateMapperTest {
Expand Down Expand Up @@ -67,22 +67,22 @@ class TemplateMapperTest {
Section section1 = sectionRepository.save(항상_보이는_섹션(List.of(question1.getId())));
Section section2 = sectionRepository.save(항상_보이는_섹션(List.of(question2.getId())));

templateRepository.save(템플릿(List.of(section1.getId(), section2.getId())));
Template template = templateRepository.save(템플릿(List.of(section1.getId(), section2.getId())));

ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when
TemplateResponse templateResponse = templateMapper.mapToTemplateResponse(reviewGroup);
List<SectionResponse> sectionResponses = templateMapper.mapSectionResponses(
template.getId(), reviewGroup.getId()
);

// then
assertAll(
() -> assertThat(templateResponse.revieweeName()).isEqualTo(reviewGroup.getReviewee()),
() -> assertThat(templateResponse.projectName()).isEqualTo(reviewGroup.getProjectName()),
() -> assertThat(templateResponse.sections()).hasSize(2),
() -> assertThat(templateResponse.sections().get(0).header()).isEqualTo(section1.getHeader()),
() -> assertThat(templateResponse.sections().get(0).questions()).hasSize(1),
() -> assertThat(templateResponse.sections().get(1).header()).isEqualTo(section2.getHeader()),
() -> assertThat(templateResponse.sections().get(1).questions()).hasSize(1)
() -> assertThat(sectionResponses).hasSize(2),
() -> assertThat(sectionResponses.get(0).header()).isEqualTo(section1.getHeader()),
() -> assertThat(sectionResponses.get(0).questions()).hasSize(1),
() -> assertThat(sectionResponses.get(1).header()).isEqualTo(section2.getHeader()),
() -> assertThat(sectionResponses.get(1).questions()).hasSize(1)
);
}

Expand All @@ -91,32 +91,35 @@ class TemplateMapperTest {
// given
Question question = questionRepository.save(서술형_필수_질문());
Section section = sectionRepository.save(항상_보이는_섹션(List.of(question.getId())));
templateRepository.save(템플릿(List.of(section.getId())));
Template template = templateRepository.save(템플릿(List.of(section.getId())));

ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when
TemplateResponse templateResponse = templateMapper.mapToTemplateResponse(reviewGroup);
List<SectionResponse> sectionResponses = templateMapper.mapSectionResponses(
template.getId(), reviewGroup.getId()
);

// then
SectionResponse sectionResponse = templateResponse.sections().get(0);
assertThat(sectionResponse.onSelectedOptionId()).isNull();
assertThat(sectionResponses.get(0).onSelectedOptionId()).isNull();
}

@Test
void 가이드라인이_없는_경우_가이드_라인을_제공하지_않는다() {
// given
Question question = questionRepository.save(서술형_필수_질문());
Section section = sectionRepository.save(항상_보이는_섹션(List.of(question.getId())));
templateRepository.save(템플릿(List.of(section.getId())));
Template template = templateRepository.save(템플릿(List.of(section.getId())));

ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when
TemplateResponse templateResponse = templateMapper.mapToTemplateResponse(reviewGroup);
List<SectionResponse> sectionResponses = templateMapper.mapSectionResponses(
template.getId(), reviewGroup.getId()
);

// then
QuestionResponse questionResponse = templateResponse.sections().get(0).questions().get(0);
QuestionResponse questionResponse = sectionResponses.get(0).questions().get(0);
assertAll(
() -> assertThat(questionResponse.hasGuideline()).isFalse(),
() -> assertThat(questionResponse.guideline()).isNull()
Expand All @@ -128,26 +131,28 @@ class TemplateMapperTest {
// given
Question question = questionRepository.save(서술형_필수_질문());
Section section = sectionRepository.save(항상_보이는_섹션(List.of(question.getId())));
templateRepository.save(템플릿(List.of(section.getId())));
Template template = templateRepository.save(템플릿(List.of(section.getId())));

ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when
TemplateResponse templateResponse = templateMapper.mapToTemplateResponse(reviewGroup);
List<SectionResponse> sectionResponses = templateMapper.mapSectionResponses(
template.getId(), reviewGroup.getId()
);

// then
QuestionResponse questionResponse = templateResponse.sections().get(0).questions().get(0);
QuestionResponse questionResponse = sectionResponses.get(0).questions().get(0);
assertThat(questionResponse.optionGroup()).isNull();
}

@Test
void 템플릿_매핑_시_템플릿에_제공할_섹션이_없을_경우_예외가_발생한다() {
// given
templateRepository.save(템플릿(List.of(1L)));
Template template = templateRepository.save(템플릿(List.of(1L)));
ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when, then
assertThatThrownBy(() -> templateMapper.mapToTemplateResponse(reviewGroup))
assertThatThrownBy(() -> templateMapper.mapSectionResponses(template.getId(), reviewGroup.getId()))
.isInstanceOf(SectionInTemplateNotFoundException.class);
}

Expand All @@ -158,12 +163,12 @@ class TemplateMapperTest {
optionGroupRepository.save(선택지_그룹(question.getId()));

Section section = sectionRepository.save(항상_보이는_섹션(List.of(question.getId())));
templateRepository.save(템플릿(List.of(section.getId())));
Template template = templateRepository.save(템플릿(List.of(section.getId())));

ReviewGroup reviewGroup = reviewGroupRepository.save(리뷰_그룹());

// when, then
assertThatThrownBy(() -> templateMapper.mapToTemplateResponse(reviewGroup))
assertThatThrownBy(() -> templateMapper.mapSectionResponses(template.getId(), reviewGroup.getId()))
.isInstanceOf(MissingOptionItemsInOptionGroupException.class);
}
}
Loading