Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions src/main/java/com/blaybus/backend/controller/QuizController.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.springframework.http.ResponseEntity;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.web.bind.annotation.PatchMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
Expand Down Expand Up @@ -56,4 +57,16 @@ public ResponseEntity<QuizDto.GradeResponse> grade(
QuizDto.GradeResponse response = gradingService.grade(quizId, request.answer(), user);
return ResponseEntity.ok(response);
}

@PatchMapping("/progress")
public ResponseEntity<Void> syncProgress(
@AuthenticationPrincipal CustomUserDetails userDetails,
@PathVariable Long sceneId,
@Valid @RequestBody QuizDto.SyncProgressRequest request) {
User user = userRepository.findByUsername(userDetails.getUsername())
.orElseThrow(() -> new BusinessException(CommonErrorCode.USER_NOT_FOUND));
Comment on lines +66 to +67
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

컨트롤러 내 여러 메소드에서 사용자 정보를 조회하는 로직이 중복되고 있습니다.

User user = userRepository.findByUsername(userDetails.getUsername())
        .orElseThrow(() -> new BusinessException(CommonErrorCode.USER_NOT_FOUND));

이러한 중복은 코드 유지보수성을 저하시킬 수 있습니다.

개선 방안:
HandlerMethodArgumentResolver를 구현하여 사용자 조회 로직을 중앙에서 관리하고, 컨트롤러 메소드에서는 @AuthUser User user 와 같이 어노테이션을 통해 바로 User 객체를 주입받는 방식을 고려해볼 수 있습니다. 이렇게 하면 코드 중복을 제거하고 컨트롤러를 더 깔끔하게 유지할 수 있습니다. 이는 장기적인 관점에서 프로젝트의 유지보수성을 높이는 데 도움이 될 것입니다.

References
  1. 단순한 코드 수정 요청을 넘어, 개발자의 성장을 돕는 통찰을 제공하십시오. 코드 패턴을 분석하여 유지보수성 및 성능 측면에서의 잘한 점(Pros)과 개선할 점(Cons)을 명확히 구분해 설명하십시오. 장기적으로 나아가야 할 기술적 방향성을 제시하여 자기계발에 도움을 주십시오. (link)


quizService.syncProgress(sceneId, request, user);
return ResponseEntity.ok().build();
}
}
18 changes: 13 additions & 5 deletions src/main/java/com/blaybus/backend/dto/QuizDto.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,21 @@

public class QuizDto {

public record GradeRequest(@NotBlank
String answer) {
public record GradeRequest(@NotBlank String answer) {
}

public record GradeResponse(
boolean correct,
double score,
String correctAnswer) {
boolean correct,
double score,
String correctAnswer) {
}

public record SyncProgressRequest(
Long lastQuizId,
Integer totalQuestions,
Integer success,
Integer failure,
Integer solveTime,
boolean isComplete) {
}
Comment on lines +16 to 23
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

SyncProgressRequest DTO의 필드에 대한 유효성 검증이 누락되었습니다. 컨트롤러에서 @Valid 어노테이션을 사용하고 있으므로, DTO 필드에 제약 조건을 추가하여 예상치 못한 값(예: 음수)이 서비스 레이어로 전달되는 것을 막아야 합니다. 이는 데이터 정합성을 보장하고 잠재적인 버그를 예방하는 데 중요합니다. null 값을 허용하지 않으려면 @NotNull도 함께 사용하는 것이 좋습니다.

Suggested change
public record SyncProgressRequest(
Long lastQuizId,
Integer totalQuestions,
Integer success,
Integer failure,
Integer solveTime,
boolean isComplete) {
}
public record SyncProgressRequest(
Long lastQuizId,
@jakarta.validation.constraints.PositiveOrZero Integer totalQuestions,
@jakarta.validation.constraints.PositiveOrZero Integer success,
@jakarta.validation.constraints.PositiveOrZero Integer failure,
@jakarta.validation.constraints.PositiveOrZero Integer solveTime,
boolean isComplete) {
}
References
  1. 코드 변경 시 발생할 수 있는 보안 위협(XSS, SQL Injection, IDOR, 민감 정보 노출 등)을 반드시 검토하십시오. 입력 값 검증 및 인가 로직의 누락 여부를 확인하십시오. (link)
  2. 이 프로젝트는 CAP 이론 중 Consistency인 데이터 정합성을 우선시합니다. (link)

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public enum CommonErrorCode implements ErrorCode {
SCENE_NOT_FOUND(HttpStatus.NOT_FOUND, "씬 정보를 찾을 수 없습니다."),

QUIZ_NOT_FOUND(HttpStatus.NOT_FOUND, "퀴즈를 찾을 수 없습니다."),
QUIZ_PROGRESS_NOT_FOUND(HttpStatus.NOT_FOUND, "퀴즈 진행 상황을 찾을 수 없습니다."),

EMBEDDING_API_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, "임베딩 API 호출에 실패했습니다."),
OPENAI_API_ERROR(HttpStatus.SERVICE_UNAVAILABLE, "AI 서비스 연결에 실패했습니다."),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,21 @@ public QuizDto.GradeResponse grade(Long quizId, String userAnswer, User user) {

boolean correct;
double score;
String correctAnswer;

if (quiz.getType() == QuizType.SELECT) {
correct = quiz.getAnswer().split("|")[0].equalsIgnoreCase(userAnswer.trim());
correctAnswer = quiz.getAnswer().split(",")[0];
correct = correctAnswer.equalsIgnoreCase(userAnswer.trim());
score = correct ? 1.0 : 0.0;
} else {
score = embeddingService.calculateSimilarity(userAnswer, quiz.getAnswer());
correctAnswer = quiz.getAnswer();
score = embeddingService.calculateSimilarity(userAnswer, correctAnswer);
correct = score >= SIMILARITY_THRESHOLD;
}

updateProgress(user, quiz, correct);

return new QuizDto.GradeResponse(correct, score, quiz.getAnswer());
return new QuizDto.GradeResponse(correct, score, correctAnswer);
}

private void updateProgress(User user, Quiz quiz, boolean correct) {
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/com/blaybus/backend/service/QuizService.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.blaybus.backend.domain.quiz.QuizUserProgress;
import com.blaybus.backend.domain.scene.SceneInformation;
import com.blaybus.backend.domain.user.User;
import com.blaybus.backend.dto.QuizDto;
import com.blaybus.backend.dto.QuizResponse;
import com.blaybus.backend.exception.BusinessException;
import com.blaybus.backend.exception.CommonErrorCode;
Expand Down Expand Up @@ -52,6 +53,26 @@ public QuizResponse getSceneQuizzes(Long sceneId, User user) {
return mapToResponse(sceneId, progress, quizzes);
}

@Transactional
public void syncProgress(Long sceneId, QuizDto.SyncProgressRequest request, User user) {
QuizUserProgress progress = progressRepository.findByUserIdAndSceneId(user.getId(), sceneId)
.orElseThrow(() -> new BusinessException(CommonErrorCode.QUIZ_PROGRESS_NOT_FOUND));

QuizUserProgress updated = QuizUserProgress.builder()
.id(progress.getId())
.user(progress.getUser())
.scene(progress.getScene())
.lastQuizId(request.lastQuizId())
.totalQuestions(request.totalQuestions())
.success(request.success())
.failure(request.failure())
.solveTime(request.solveTime())
.isComplete(request.isComplete())
.build();

progressRepository.save(updated);
}
Comment on lines +57 to +74
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

JPA 엔티티를 업데이트하는 현재 방식은 비효율적이며, 더 나은 대안이 있습니다.

현재 방식의 문제점 (Cons):

  • builder()를 사용해 새 인스턴스를 생성하고 save()를 호출하는 것은 JPA의 merge 동작을 유발합니다.
  • merge는 영속성 컨텍스트에 해당 엔티티가 없는 경우 데이터베이스에서 다시 조회하는 추가적인 SELECT 쿼리를 발생시킬 수 있어 비효율적입니다.
  • 코드를 읽는 개발자에게 새로운 엔티티를 생성하는 것처럼 보여 혼란을 줄 수 있습니다.

개선 방안 (Pros):
JPA의 변경 감지(dirty checking) 기능을 활용하는 것이 좋습니다. @Transactional 메소드 내에서 영속 상태의 엔티티를 수정한 후 명시적으로 save()를 호출하지 않아도 트랜잭션이 커밋될 때 변경 사항이 자동으로 데이터베이스에 반영됩니다. 이를 통해 성능을 개선하고 코드를 더 명확하게 만들 수 있습니다.

대안 1: 엔티티에 업데이트 메소드 추가 (권장)
QuizUserProgress.java에 업데이트 로직을 캡슐화하는 메소드를 추가합니다.

// QuizUserProgress.java
public void syncProgress(QuizDto.SyncProgressRequest request) {
    this.lastQuizId = request.lastQuizId();
    this.totalQuestions = request.totalQuestions();
    this.success = request.success();
    this.failure = request.failure();
    this.solveTime = request.solveTime();
    this.isComplete = request.isComplete();
}

그 후 QuizServicesyncProgress 메소드를 다음과 같이 수정합니다.

// QuizService.java
@Transactional
public void syncProgress(Long sceneId, QuizDto.SyncProgressRequest request, User user) {
    QuizUserProgress progress = progressRepository.findByUserIdAndSceneId(user.getId(), sceneId)
            .orElseThrow(() -> new BusinessException(CommonErrorCode.QUIZ_PROGRESS_NOT_FOUND));

    progress.syncProgress(request);
    // progressRepository.save() 호출이 더 이상 필요하지 않습니다.
}

대안 2: Setter 사용
엔티티에 Setter를 열어두고 서비스 레이어에서 직접 값을 변경할 수도 있지만, 도메인 객체의 캡슐화를 해칠 수 있어 첫 번째 방안을 더 권장합니다.

References
  1. 단순한 코드 수정 요청을 넘어, 개발자의 성장을 돕는 통찰을 제공하십시오. 코드 패턴을 분석하여 유지보수성 및 성능 측면에서의 잘한 점(Pros)과 개선할 점(Cons)을 명확히 구분해 설명하십시오. (link)
  2. 단일 해법만 제시하지 말고, 트레이드오프(Trade-off)를 고려한 여러 대안을 제안하십시오. 각 대안의 장단점(성능, 가독성, 유지보수성 등)을 비교 분석하여 설명하십시오. (link)


private QuizResponse mapToResponse(Long sceneId, QuizUserProgress progress, List<Quiz> quizzes) {
QuizResponse.UserProgressDto progressDto = new QuizResponse.UserProgressDto(
progress.getId(),
Expand Down
10 changes: 5 additions & 5 deletions src/main/resources/data/initial_quiz_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"target_purpose": "명칭 및 구조 파악",
"type": "SELECT",
"question": "로봇집게에서 물건이 닿는 안쪽 부분에 마찰력을 높이기 위해 덧대는 부품은 무엇인가요?",
"answer": "그립 패드|링크 암|기어 스크루|실린더"
"answer": "그립 패드,링크 암,기어 스크루,실린더"
},
{
"scene_info_title": "로봇집게",
Expand All @@ -18,7 +18,7 @@
"target_purpose": "명칭 및 구조 파악",
"type": "SELECT",
"question": "서스펜션 구조에서 나선형으로 말려 있으며 충격을 받으면 길이가 변하는 부품은 무엇인가요?",
"answer": "코일 스프링|쇼크 업소버|컨트롤 암|너클"
"answer": "코일 스프링,쇼크 업소버,컨트롤 암,너클"
},
{
"scene_info_title": "서스펜션",
Expand All @@ -32,7 +32,7 @@
"target_purpose": "명칭 및 구조 파악",
"type": "SELECT",
"question": "판스프링 끝부분에서 차체와 연결되어 스프링의 길이 변화를 수용하는 부품은 무엇인가요?",
"answer": "셔클(Shackle)|센터 볼트|유볼트|리프 클립"
"answer": "셔클(Shackle),센터 볼트,유볼트,리프 클립"
},
{
"scene_info_title": "판스프링",
Expand All @@ -46,7 +46,7 @@
"target_purpose": "명칭 및 구조 파악",
"type": "SELECT",
"question": "드론의 몸체 중심에서 프로펠러가 달린 모터까지 뻗어 있는 뼈대 부분을 무엇이라 하나요?",
"answer": "암(Arm)|프레임|랜딩 기어|모터 마운트"
"answer": "암(Arm),프레임,랜딩 기어,모터 마운트"
},
{
"scene_info_title": "드론",
Expand All @@ -60,7 +60,7 @@
"target_purpose": "명칭 및 구조 파악",
"type": "SELECT",
"question": "바이스에서 물건을 직접 압착하는 두 개의 벽면 중 움직이지 않는 부분의 명칭은?",
"answer": "고정 조(Fixed Jaw)|가동 조|핸들|슬라이드 베드"
"answer": "고정 조(Fixed Jaw),가동 조,핸들,슬라이드 베드"
},
{
"scene_info_title": "공작기계바이스",
Expand Down
85 changes: 85 additions & 0 deletions src/test/java/com/blaybus/backend/service/QuizServiceTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package com.blaybus.backend.service;

import static org.assertj.core.api.Assertions.*;
import static org.mockito.BDDMockito.*;

import java.util.Optional;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import com.blaybus.backend.domain.quiz.QuizUserProgress;
import com.blaybus.backend.domain.scene.SceneInformation;
import com.blaybus.backend.domain.user.User;
import com.blaybus.backend.dto.QuizDto;
import com.blaybus.backend.exception.BusinessException;
import com.blaybus.backend.exception.CommonErrorCode;
import com.blaybus.backend.repository.QuizUserProgressRepository;

@ExtendWith(MockitoExtension.class)
class QuizServiceTest {

@Mock
private QuizUserProgressRepository progressRepository;

@InjectMocks
private QuizService quizService;

@Test
@DisplayName("퀴즈 진행 상황을 정상적으로 동기화한다.")
void syncProgressSuccess() {
// given
Long sceneId = 1L;
User user = mock(User.class);
given(user.getId()).willReturn(1L);
SceneInformation scene = SceneInformation.builder().id(sceneId).build();
QuizUserProgress progress = QuizUserProgress.builder()
.id(1L)
.user(user)
.scene(scene)
.totalQuestions(5)
.success(2)
.failure(1)
.solveTime(100)
.isComplete(false)
.build();

QuizDto.SyncProgressRequest request = new QuizDto.SyncProgressRequest(
3L, 5, 4, 1, 250, true);

given(progressRepository.findByUserIdAndSceneId(user.getId(), sceneId))
.willReturn(Optional.of(progress));

// when
quizService.syncProgress(sceneId, request, user);

// then
then(progressRepository).should().save(argThat(updated -> updated.getLastQuizId().equals(3L) &&
updated.getSuccess().equals(4) &&
updated.getSolveTime().equals(250) &&
updated.isComplete()));
}

@Test
@DisplayName("진행 상황이 존재하지 않으면 예외를 던진다.")
void syncProgressNotFound() {
// given
Long sceneId = 1L;
User user = mock(User.class);
given(user.getId()).willReturn(1L);
QuizDto.SyncProgressRequest request = new QuizDto.SyncProgressRequest(
3L, 5, 4, 1, 250, true);

given(progressRepository.findByUserIdAndSceneId(user.getId(), sceneId))
.willReturn(Optional.empty());

// when & then
assertThatThrownBy(() -> quizService.syncProgress(sceneId, request, user))
.isInstanceOf(BusinessException.class)
.hasFieldOrPropertyWithValue("errorCode", CommonErrorCode.QUIZ_PROGRESS_NOT_FOUND);
}
}