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] feat: 하이라이트 추가 및 수정 API 구현 #813

Merged
merged 39 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
c62884c
feat: 하이라이트 기능을 위한 도메인, 테이블, 레포지토리 생성
skylar1220 Oct 9, 2024
5979829
refactor: reviewRequestCode도 서비스에 함께 넘기도록 수정
skylar1220 Oct 9, 2024
fa5aa38
feat: 하이라이트 수정을 위한 삭제, 저장 로직 구현
skylar1220 Oct 9, 2024
b55df2f
feat: 하이라이트 수정을 위한 검증 로직 구현
skylar1220 Oct 10, 2024
cbbc950
db: 하이라이트 조회 성능과 동시성을 위한 복합 인덱스 추가
skylar1220 Oct 10, 2024
84898cf
fix: 리뷰 그룹으로 질문 검증하는 로직 수정
skylar1220 Oct 10, 2024
2b74e5e
refactor: 레포지토리에 기존에 있던 메서드 활용하도록 수정
skylar1220 Oct 10, 2024
eb1459e
fix: 테이블명 오류 수정
skylar1220 Oct 10, 2024
f4418cc
refactor: 초기화시 필드 순서 수정
skylar1220 Oct 10, 2024
921cc6e
test: 하이라이트 레포지토리 테스트 추가
skylar1220 Oct 10, 2024
b3a58c3
test: 하이라이트 입력 검증 테스트 추가
skylar1220 Oct 10, 2024
a88d090
test: 하이라이트 수정 기능 서비스 테스트 추가
skylar1220 Oct 10, 2024
b8ce456
refactor: 생성자 파라미터 및 필드 순서 변경
skylar1220 Oct 10, 2024
ad59c37
fix: not null 설정 오타 수정
skylar1220 Oct 10, 2024
f87efad
refactor: 검증 순서 변경
skylar1220 Oct 10, 2024
d3abfd3
refactor: jpa 기본 메서드로 쿼리 여러개 나가는 부분을 @Query로 변경
skylar1220 Oct 10, 2024
95cf051
style: 개행 수정
skylar1220 Oct 10, 2024
bb9ebb1
refactor: 에러 메세지 수정
skylar1220 Oct 10, 2024
c700e45
db: 하이라이트 인덱스 관련 데이터 타입 int로 수정
skylar1220 Oct 10, 2024
834d900
refactor: 하이라이트 표시 위치 관련 속성을 객체로 분리
skylar1220 Oct 10, 2024
16f5d34
refactor: JPQL로 변경
skylar1220 Oct 10, 2024
f71d9de
refactor: 기존 레포지토리 메서드를 사용하도록 변경
skylar1220 Oct 10, 2024
4b28bd4
refactor: 메서드 네이밍 수정
skylar1220 Oct 10, 2024
cd6a16c
refactor: 하이라이트 위치 속성 객체에 추가 설정
skylar1220 Oct 10, 2024
fbd8fc4
refactor: 하이라이트 엔티티에서 reviewGroupId, questionId 필드 제거
skylar1220 Oct 10, 2024
1ce93e6
refactor: 기존 하이라이트 제거를 answerId를 통해 하도록 변경
skylar1220 Oct 10, 2024
ba72762
test: 책임을 이동한 테스트 제거 및 테스트 오류 수정
skylar1220 Oct 10, 2024
1c16507
db: 테이블에 questionId, review_group_id 컬럼 제거 반영
skylar1220 Oct 10, 2024
a7f25b8
refactor: 기존 중복 요청 메서드 제거
skylar1220 Oct 10, 2024
52c350b
refactor: 중복 요청의 경우 하나만 저장하도록 EqualsAndHashCode 재정의
skylar1220 Oct 10, 2024
7e546aa
refactor: 하이라이트 저장시 순서 보장 적용
skylar1220 Oct 10, 2024
55895db
refactor: HighlightPosition 검증, 객체 생성 후 하이라이트 객체 생성하도록 변경
skylar1220 Oct 10, 2024
aaf1c6c
refactor: 엔티티의 equals 조건 id로 복원 및 중복 제거 로직 삭제
skylar1220 Oct 11, 2024
560faec
refactor: HighlightPosition 객체 생성 위치 생성자 내로 이동
skylar1220 Oct 11, 2024
a3a968f
refactor: 예외명 수정
skylar1220 Oct 11, 2024
c226e21
refactor: 하이라이트 인덱스의 검증 추가
skylar1220 Oct 11, 2024
132ff57
refactor: 양수 검증 메서드명 수정
skylar1220 Oct 11, 2024
a9ceb36
test: 필드에 접근 제어자 추가
skylar1220 Oct 11, 2024
24b706b
fix: 하이라이트 줄 번호 계산 로직 및 변수명 수정
skylar1220 Oct 11, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class HighlightController {
@PostMapping("/v2/highlight")
public ResponseEntity<Void> highlight(@Valid @RequestBody HighlightsRequest request,
@SessionAttribute("reviewRequestCode") String reviewRequestCode) {
highlightService.highlight(request);
highlightService.highlight(request, reviewRequestCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

리뷰 컨트롤러처럼 개행을 맞추면 어떨까요?

    @GetMapping("/v2/reviews/{id}")
    public ResponseEntity<ReviewDetailResponse> findReceivedReviewDetail(
            @PathVariable long id,
            @SessionAttribute("reviewRequestCode") String reviewRequestCode
    ) {
        ReviewDetailResponse response = reviewDetailLookupService.getReviewDetail(id, reviewRequestCode);
        return ResponseEntity.ok(response);
    }

return ResponseEntity.ok().build();
Comment on lines +24 to 25
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.

아차차😂

}
}
52 changes: 52 additions & 0 deletions backend/src/main/java/reviewme/highlight/domain/HighLight.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package reviewme.highlight.domain;

import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import lombok.AccessLevel;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Entity
@Table(name = "highlight")
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@EqualsAndHashCode(of = "id")
@Getter
public class HighLight {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@Column(name = "review_group_id", nullable = false)
private long reviewGroupId;

Copy link
Contributor

Choose a reason for hiding this comment

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

굳이 questionId와 reviewId를 가지고 있지 않아도 되지 않을까요?
answerId로 소속은 충분히 알 수 있다고 생각해요,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

questionId와 reviewGroupId를 말하는 게 맞겠죠?
새로운 하이라이트가 들어왔을 때 question id = ? and review_group_id = ? 인 조건문으로 기존 데이터 삭제를 진행하기 떄문에 넣었어요~

Copy link
Contributor

Choose a reason for hiding this comment

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

JOIN해서 DELETE하는 게 좋지 않을까요? 데이터 삭제에만 필요한 것이 DB에 속성으로 등록될 필요가 있을지 고민해보아야겠네용,,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞네요 answerId로 처리하고 review group id와 question id는 뺐어요!

@Column(name = "question_id", nullable = false)
private long questionId;

@Column(name = "answer_id", nullable = false)
private long answerId;

@Column(name = "line_index", nullable = false)
private long lineIndex;

@Column(name = "start_index", nullable = false)
private long startIndex;

@Column(name = "end_index", nullable = false)
private long endIndex;

public HighLight(long reviewGroupId, long questionId, long answerId,
long lineIndex, long startIndex, long endIndex) {
this.reviewGroupId = reviewGroupId;
this.questionId = questionId;
this.answerId = answerId;
this.lineIndex = lineIndex;
this.startIndex = startIndex;
this.endIndex = endIndex;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package reviewme.highlight.repository;

import org.springframework.data.jpa.repository.JpaRepository;
import reviewme.highlight.domain.HighLight;

public interface HighlightRepository extends JpaRepository<HighLight, Long> {

void deleteByReviewGroupIdAndQuestionId(long reviewGroupId, long questionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드 이름으로 생성된 쿼리는 1건당 DELETE 쿼리가 날아가는 것으로 알고 있어요.

@Modifying, @Query로 한 번에 다 날아가게 하면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오오.. 기본 메서드는 건별로!! 반영완!

}
Original file line number Diff line number Diff line change
@@ -1,12 +1,51 @@
package reviewme.highlight.service;

import java.util.List;
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.validator.HighlightValidator;
import reviewme.review.service.exception.ReviewGroupNotFoundByReviewRequestCodeException;
import reviewme.reviewgroup.repository.ReviewGroupRepository;

@Service
@RequiredArgsConstructor
public class HighlightService {

public void highlight(HighlightsRequest request) {
// TODO: implement method
private final HighlightRepository highlightRepository;
private final ReviewGroupRepository reviewGroupRepository;

private final HighlightValidator highlightValidator;

@Transactional
public void highlight(HighlightsRequest request, String reviewRequestCode) {
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.

리팩토링 pr에서 editHighlight로 변경하였습니다~

long reviewGroupId = reviewGroupRepository.findByReviewRequestCode(reviewRequestCode)
.orElseThrow(() -> new ReviewGroupNotFoundByReviewRequestCodeException(reviewRequestCode))
.getId();

highlightValidator.validate(request, reviewGroupId);
deleteOldHighlight(request.questionId(), reviewGroupId);
saveNewHighlight(request, reviewGroupId);
}

private void deleteOldHighlight(long questionId, long reviewGroupId) {
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.

beforeHighlight로 수정!

highlightRepository.deleteByReviewGroupIdAndQuestionId(reviewGroupId, questionId);
}

private void saveNewHighlight(HighlightsRequest highlightsRequest, long reviewGroupId) {
List<HighLight> highLights = highlightsRequest.highlights()
.stream()
.flatMap(highlightRequest -> highlightRequest.lines().stream()
.flatMap(line -> line.ranges().stream()
.map(range -> new HighLight(
reviewGroupId, highlightsRequest.questionId(), highlightRequest.answerId(),
line.index(), range.startIndex(), range.endIndex()
))
))
.toList();
highlightRepository.saveAll(highLights);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package reviewme.highlight.service.exception;

import lombok.extern.slf4j.Slf4j;
import reviewme.global.exception.BadRequestException;

@Slf4j
public class HighlightDuplicatedException extends BadRequestException {

public HighlightDuplicatedException(long answerId, long lineIndex, long startIndex, long endIndex) {
super("중복된 하이라이트는 생성할 수 없어요.");
log.info("Highlight is duplicated - answerId: {}, lineIndex: {}, startIndex: {}, endIndex: {}",
answerId, lineIndex, startIndex, endIndex);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package reviewme.highlight.service.exception;

import lombok.extern.slf4j.Slf4j;
import reviewme.global.exception.BadRequestException;

@Slf4j
public class InvalidHighlightLineIndexException extends BadRequestException {

public InvalidHighlightLineIndexException(long submittedLineIndex, long maxLineIndex) {
super("줄 번호는 %d 이하여야해요.".formatted(maxLineIndex));
log.info("Line index is out of bound - maxIndex: {}, submittedLineIndex: {}", maxLineIndex, submittedLineIndex);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package reviewme.highlight.service.exception;

import lombok.extern.slf4j.Slf4j;
import reviewme.global.exception.BadRequestException;

@Slf4j
public class InvalidHighlightRangeException extends BadRequestException {

public InvalidHighlightRangeException(long startIndex, long endIndex) {
super("하이라이트 시작 글자 인덱스는 종료 글자 인덱스보다 작아야해요.");
Copy link
Contributor

@donghoony donghoony Oct 10, 2024

Choose a reason for hiding this comment

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

  • 라이언트에게 인덱스 대신 위치라는 말을 쓰면 어떨까요?
  • 작아야는 미만을 의미합니다. 같거나 작다라는 표현이 더 알맞겠습니다~!

하이라이트 끝 위치는 시작 위치보다 같거나 커야 해요.

log.info("Highlight index is out of bound - startIndex: {}, endIndex: {}", startIndex, endIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

out of bound는 주어진 배열보다 더 큰/작은 값에 대한 예외 사항이예요. Invalid highlight range: {} {}로 해도 충분하지 않을까요?

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package reviewme.highlight.service.exception;

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

@Slf4j
public class SubmittedAnswerAndProvidedAnswerMismatchException extends BadRequestException {

public SubmittedAnswerAndProvidedAnswerMismatchException(Collection<Long> providedAnswerIds,
Collection<Long> submittedAnswerIds) {
super("제출된 응답이 제공된 응답과 일치하지 않아요.");
log.info("SubmittedAnswer and providedAnswer mismatch - providedAnswerIds: {}, submittedAnswerIds: {}",
providedAnswerIds, submittedAnswerIds);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package reviewme.highlight.service.validator;

import java.util.Objects;
import lombok.Getter;

@Getter
public class HighlightKey {
Copy link
Contributor

Choose a reason for hiding this comment

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

객체 도출 좋네요! 이거 @Embedded로 하지 않은 이유가 있나요?

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.

highlightPosition 어떤가요? (사실 아루가 제안함)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영완!


private final Long answerId;
private final Long lineIndex;
private final Long startIndex;
private final Long endIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

long ?


public HighlightKey(Long answerId, Long lineIndex, Long startIndex, Long endIndex) {
this.answerId = answerId;
this.lineIndex = lineIndex;
this.startIndex = startIndex;
this.endIndex = endIndex;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

애초에 begin <= end는 불변식이니 여기에서 검증해도 되지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영완!


@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
HighlightKey that = (HighlightKey) o;
return answerId.equals(that.answerId) && lineIndex.equals(that.lineIndex) &&
startIndex.equals(that.startIndex) && endIndex.equals(that.endIndex);
}

@Override
public int hashCode() {
return Objects.hash(answerId, lineIndex, startIndex, endIndex);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@EqualsAndHashCode(of) 로 해결할 수 없었나용?

Copy link
Contributor Author

@skylar1220 skylar1220 Oct 10, 2024

Choose a reason for hiding this comment

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

@EqualsAndHashCode 만 붙이면 모든 필드에 데헤서 equals 처리해주는군요!

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
package reviewme.highlight.service.validator;

import java.util.HashSet;
import java.util.List;
import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import reviewme.highlight.service.dto.HighlightRequest;
import reviewme.highlight.service.dto.HighlightedLineRequest;
import reviewme.highlight.service.dto.HighlightsRequest;
import reviewme.highlight.service.exception.HighlightDuplicatedException;
import reviewme.highlight.service.exception.InvalidHighlightLineIndexException;
import reviewme.highlight.service.exception.InvalidHighlightRangeException;
import reviewme.highlight.service.exception.SubmittedAnswerAndProvidedAnswerMismatchException;
import reviewme.question.repository.QuestionRepository;
import reviewme.review.domain.TextAnswer;
import reviewme.review.repository.AnswerRepository;
import reviewme.review.repository.TextAnswerRepository;
import reviewme.review.service.exception.AnswerNotFoundByIdException;
import reviewme.review.service.exception.SubmittedQuestionAndProvidedQuestionMismatchException;

@Component
@RequiredArgsConstructor
public class HighlightValidator {

private final AnswerRepository answerRepository;
private final TextAnswerRepository textAnswerRepository;
private final QuestionRepository questionRepository;

public void validate(HighlightsRequest request, long reviewGroupId) {
validateAnswerByReviewGroup(request, reviewGroupId);
validateQuestionByReviewGroup(request, reviewGroupId);
Copy link
Contributor

Choose a reason for hiding this comment

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

요청의 뎁스 순서로 하면 validateQuestionByReviewGroup이 먼저 되어야 할 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 맞네요 질문 먼저 검증!

validateAnswerByQuestion(request);
validateLineIndex(request);
validateRange(request);
validateDuplicate(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

(개인 취향일 수도 있음)
무엇을 validate___By___ 함수들을 무엇을 검증하는지 더 명확하게 이름을 바꿀 수 있을 것 같아요!

예를 들면
validateAnswerByReviewGroup
-> validateHighlitedAnswerBelongToReviewGroup

validateQuestionByReviewGroup
-> validateHighlitedQuestionBelongToReviewGroup

validateAnswerByQuestion
-> validateHighlightedAnswerBelongToQuestion


(이건 정말정말 개인 취향일 수 있음)
혹은 이 세가지를 묶어서 이런 validation 함수를 만들수도 있겠네요!

validateHighlightedResourceHiarachyRelationship()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(아루 코멘트 차용) validateReviewGroupContainsQuestion처럼 validate + 긍정문의 검증 행위 형식으로 바꿔보았어요!

}
Copy link
Contributor

Choose a reason for hiding this comment

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

validateAnswerByReviewGroup, validateQuestionByReviewGroup, validateAnswerByQuestion

이 부분은 highlight가 연관된 것들을 id로 가지고 있어서 절차적으로 검증할 수 밖에 없는 부분인데요. 리뷰 그룹과 질문으로 답변을 가져오는 것으로 한번에 검증과 동시에 자원을 가져올 수 있지 않을까요? 로직과 쿼리 한 번으로 감당할 수 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리팩토링 pr에서 반영하겠습니다!


private void validateAnswerByReviewGroup(HighlightsRequest request, long reviewGroupId) {
Set<Long> providedAnswerIds = answerRepository.findIdsByReviewGroupId(reviewGroupId);
List<Long> submittedAnswerIds = request.highlights()
.stream()
.map(HighlightRequest::answerId)
.toList();

if (!providedAnswerIds.containsAll(submittedAnswerIds)) {
throw new SubmittedAnswerAndProvidedAnswerMismatchException(providedAnswerIds, submittedAnswerIds);
}
}

private void validateQuestionByReviewGroup(HighlightsRequest request, long reviewGroupId) {
Set<Long> providedQuestionIds = questionRepository.findIdsByReviewGroupId(reviewGroupId);
long submittedQuestionId = request.questionId();

if (!providedQuestionIds.contains(submittedQuestionId)) {
throw new SubmittedQuestionAndProvidedQuestionMismatchException(submittedQuestionId, providedQuestionIds);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

questionRepository.existByIdAndReviewGroupId() 어떠신가요? 성능도 더 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

직접적으로 연관이 없는 review group을 통해 question을 가져오도록 하는 것이 너무 특정적이고 비즈니스 로직적이지 않나해서
review group의 속성인 template을 가져오고, template을 통해 question을 가져오도록 변경하였습니다!


private void validateAnswerByQuestion(HighlightsRequest request) {
Set<Long> providedAnswerIds = answerRepository.findIdsByQuestionId(request.questionId());
List<Long> submittedAnswerIds = request.highlights()
.stream()
.map(HighlightRequest::answerId)
.toList();

if (!providedAnswerIds.containsAll(submittedAnswerIds)) {
throw new SubmittedAnswerAndProvidedAnswerMismatchException(providedAnswerIds, submittedAnswerIds);
}
}

private void validateLineIndex(HighlightsRequest request) {
Copy link
Contributor

@Kimprodp Kimprodp Oct 10, 2024

Choose a reason for hiding this comment

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

이 부분도 객체의 책임으로 할 수 있는 방법이 없을까요?

(개인적으로 답변을 함께 생성자에 넘겨서 의존하지 않고 answer를 사용하게 하거나 답변에 대한 index 길이 검증 책임을 가지는 클래스가 있어도 될 것 같아요)

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

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.

리팩토링 pr에서 반영하겠습니다!

for (HighlightRequest highlight : request.highlights()) {
TextAnswer textAnswer = textAnswerRepository.findById(highlight.answerId())
.orElseThrow(() -> new AnswerNotFoundByIdException(highlight.answerId()));
long maxLineIndex = textAnswer.getContent().lines().count();
Copy link
Contributor

Choose a reason for hiding this comment

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

오 이렇게 개행이 된 라인 수를 가져올 수 있군요! 야미~


for (HighlightedLineRequest line : highlight.lines()) {
long submittedLineIndex = line.index();
if (maxLineIndex < submittedLineIndex) {
throw new InvalidHighlightLineIndexException(submittedLineIndex, maxLineIndex);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이 if 문 수정이 필요해보여요.
지금 maxLineIndex 는 서술형 답변의 "라인 수(마지막 idx + 1)"을 나타내기 때문에 <= 여야 합니다.
그리고, maxLineIndex 라는 변수명을 answerLineCount 혹은 existingLineNumbers같은 걸로 바꾸는 것도 의미 전달이 더 잘 될 것 같아요.


그리고 테스트 코드에서도 아래 부분 수정해주세요!

long answerLineCount = textAnswer.getContent().lines().count();
HighlightedLineRequest highlightedLineRequest = new HighlightedLineRequest(answerLineCount + 1, List.of()); // ❗️이 부분을 answerLineCount로 바꿔야 경계값이 됩니다.
HighlightRequest highlightRequest = new HighlightRequest(textAnswer.getId(), List.of(highlightedLineRequest));
HighlightsRequest highlightsRequest = new HighlightsRequest(questionId, List.of(highlightRequest));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

비교대상 두개 모두 index 개념이어야 쉽게 이해갈 것 같아서 providedMaxLineIndex로 해봤는데 어떤가요?
저 경계값은 고쳐야겠네요! 반완!

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

Choose a reason for hiding this comment

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

레벨1, 2에서 어떻게 리팩터링했는지 돌아보면 실마리가 보일거예요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해볼게요!


private void validateRange(HighlightsRequest request) {
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

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.

서비스적인 속성이 아니네요 반영완!

request.highlights()
.stream()
.flatMap(highlightRequest -> highlightRequest.lines().stream()
.flatMap(highlightedLineRequest -> highlightedLineRequest.ranges().stream()
.filter(range -> range.startIndex() > range.endIndex())
)
)
.findFirst()
.ifPresent(range -> {
throw new InvalidHighlightRangeException(range.startIndex(), range.endIndex());
});
}

private void validateDuplicate(HighlightsRequest request) {
Set<HighlightKey> uniqueHighlights = new HashSet<>();

request.highlights().forEach(highlight ->
highlight.lines().forEach(line ->
line.ranges().forEach(range -> {
HighlightKey key = new HighlightKey(
highlight.answerId(), line.index(), range.startIndex(), range.endIndex()
);
if (!uniqueHighlights.add(key)) {
throw new HighlightDuplicatedException(
highlight.answerId(), line.index(), range.startIndex(), range.endIndex()
);
}
})
)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,16 @@ public interface QuestionRepository extends JpaRepository<Question, Long> {
WHERE ts.template_id = :templateId
""", nativeQuery = true)
List<Question> findAllByTemplatedId(long templateId);

@Query(value = """
SELECT q.id FROM question q
JOIN section_question sq
ON q.id = sq.question_id
JOIN template_section ts
ON sq.section_id = ts.section_id
JOIN review_group rg
ON ts.template_id = rg.template_id
WHERE rg.id = :reviewGroupId
""", nativeQuery = true)
Set<Long> findIdsByReviewGroupId(long reviewGroupId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package reviewme.review.repository;

import java.util.Set;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.stereotype.Repository;
import reviewme.review.domain.Answer;

@Repository
public interface AnswerRepository extends JpaRepository<Answer, Long> {

@Query(value = """
select a.id from answer a
join new_review r
on a.review_id = r.id
where r.review_group_id = :reviewGroupId
""", nativeQuery = true)
Set<Long> findIdsByReviewGroupId(long reviewGroupId);

@Query(value = """
select a.id from answer a
join question q
on a.question_id = q.id
where q.id = :questionId
""", nativeQuery = true)
Set<Long> findIdsByQuestionId(long questionId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

지금 아루의 PR이 올라와 있으니 미리 JPQL로 바꾸죠!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영완!

Loading