[be/fix] ai 피드백 생성, 조회 수정 #287
Conversation
Gwanghyeon-k
requested review from
lsh1215,
YunJuwon0825 and
dlwldnjs1009
as code owners
WalkthroughThis pull request introduces significant changes to the AI feedback and feedback modules. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AIFeedbackController
participant AIFeedbackService
participant AIFeedbackConverter
Client->>AIFeedbackController: Request createFeedbackFromS3(resumeId)
AIFeedbackController->>AIFeedbackService: generateAIFeedbackFromS3(resumeId)
AIFeedbackService-->>AIFeedbackController: AIFeedback entity
AIFeedbackController->>AIFeedbackConverter: toResponse(feedback)
AIFeedbackConverter-->>AIFeedbackController: AIFeedbackResponse
AIFeedbackController-->>Client: CommonResponse(AIFeedbackResponse)
sequenceDiagram
participant Client
participant FeedbackController
participant AIFeedbackService
Client->>FeedbackController: Request getFeedbackWithAIFeedback(resumeId, aifeedbackId)
FeedbackController->>AIFeedbackService: getFeedbackById(aifeedbackId)
AIFeedbackService-->>FeedbackController: AIFeedback entity
FeedbackController-->>Client: CommonResponse(AllFeedbackResponse)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/src/main/java/com/techeer/backend/api/aifeedback/service/OpenAIService.java (2)
43-60: Add try-with-resources for proper resource cleanup.The current implementation doesn't properly close the HTTP response resources. Use try-with-resources to ensure proper cleanup.
public String getAIFeedback(String resumeText) throws IOException { HttpPost httpPost = new HttpPost(apiUrl); httpPost.setHeader("Content-Type", "application/json"); httpPost.setHeader("Authorization", "Bearer " + apiKey); String jsonBody = createRequestBody(resumeText); StringEntity entity = new StringEntity(jsonBody, ContentType.APPLICATION_JSON); httpPost.setEntity(entity); - HttpResponse response = httpClient.execute(httpPost); - int statusCode = response.getStatusLine().getStatusCode(); - String responseBody = EntityUtils.toString(response.getEntity()); - - if (statusCode != 200) { - throw new IOException("OpenAI API 호출 실패: 상태 코드 " + statusCode + ", 응답: " + responseBody); - } - return responseBody; + try (CloseableHttpResponse response = httpClient.execute(httpPost)) { + int statusCode = response.getStatusLine().getStatusCode(); + String responseBody = EntityUtils.toString(response.getEntity()); + + if (statusCode != 200) { + throw new IOException("OpenAI API 호출 실패: 상태 코드 " + statusCode + ", 응답: " + responseBody); + } + return responseBody; + } }
78-80: Add actual debug logging as mentioned in the comment.Line 78 comments about logging for debugging, but no actual logging is implemented. Add proper debug logging using the existing logger.
// JSON 문자열 생성 후 로그 출력 (디버깅용) String jsonBody = objectMapper.writeValueAsString(requestBody); + // Add actual debug logging + if (log.isDebugEnabled()) { + log.debug("OpenAI API 요청 본문: {}", jsonBody); + } return jsonBody;backend/src/main/java/com/techeer/backend/api/aifeedback/controller/AIFeedbackController.java (1)
46-54: Consider Pagination for Large Feedback Lists
getFeedbacksByResumeIdcan potentially return a large list if a resume has extensive AI feedback records. Consider using pagination or filtering to optimize performance and reduce payload size.backend/src/main/java/com/techeer/backend/api/aifeedback/service/AIFeedbackService.java (1)
99-109: S3 I/O LoggingAdding logs to indicate success/failure of S3 object retrieval is a helpful touch for debugging. Consider also logging the object’s MIME type for clarity if relevant to your domain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/src/main/java/com/techeer/backend/api/aifeedback/controller/AIFeedbackController.java(1 hunks)backend/src/main/java/com/techeer/backend/api/aifeedback/converter/AIFeedbackConverter.java(1 hunks)backend/src/main/java/com/techeer/backend/api/aifeedback/repository/AIFeedbackRepository.java(1 hunks)backend/src/main/java/com/techeer/backend/api/aifeedback/service/AIFeedbackService.java(2 hunks)backend/src/main/java/com/techeer/backend/api/aifeedback/service/OpenAIService.java(1 hunks)backend/src/main/java/com/techeer/backend/api/feedback/controller/FeedbackController.java(3 hunks)backend/src/main/java/com/techeer/backend/api/feedback/converter/FeedbackConverter.java(0 hunks)backend/src/main/java/com/techeer/backend/api/feedback/dto/response/AllFeedbackResponse.java(0 hunks)backend/src/main/java/com/techeer/backend/api/feedback/service/FeedbackService.java(1 hunks)
💤 Files with no reviewable changes (2)
- backend/src/main/java/com/techeer/backend/api/feedback/dto/response/AllFeedbackResponse.java
- backend/src/main/java/com/techeer/backend/api/feedback/converter/FeedbackConverter.java
🔇 Additional comments (14)
backend/src/main/java/com/techeer/backend/api/aifeedback/converter/AIFeedbackConverter.java (1)
6-17: Clean and well-implemented converter class.The converter correctly handles null input and uses the builder pattern appropriately. The class has a clear single responsibility of converting entity objects to response DTOs.
backend/src/main/java/com/techeer/backend/api/aifeedback/repository/AIFeedbackRepository.java (1)
11-11: Good change to support multiple feedbacks per resume.Changing the return type from Optional to List properly reflects that a resume can have multiple AI feedbacks.
backend/src/main/java/com/techeer/backend/api/aifeedback/service/OpenAIService.java (1)
32-41: Good improvement with HTTP client timeout configuration.Adding timeout configuration for the HTTP client is a valuable improvement that prevents hanging connections or long wait times when the OpenAI API is slow to respond.
backend/src/main/java/com/techeer/backend/api/feedback/controller/FeedbackController.java (2)
4-4: Dependency Import Looks GoodNo issues. The import of
AIFeedbackServiceis straightforward and aligns with the usage in this controller.
38-38: Field Injection Looks GoodIntroducing
private final AIFeedbackService aifeedbackService;is consistent with the constructor-based dependency injection approach used in this class.backend/src/main/java/com/techeer/backend/api/aifeedback/controller/AIFeedbackController.java (4)
3-17: Imports & AnnotationsThe newly added imports and annotations (e.g.,
@Tag,@RestController) are well-structured and align with Spring and Swagger usage. No concerns here.
24-28: Constructor Injection Looks GoodThe explicit constructor for
AIFeedbackControlleris consistent with best practices for Spring dependency injection (setter or constructor-based).
30-36: AI Feedback Creation EndpointReturning
CommonResponse<AIFeedbackResponse>is clear and consistent with the project's standardized response approach. Logging and exception handling appear to be handled within the service.
38-44: Single Feedback RetrievalThe method
getFeedbackByIdis straightforward and well-documented. It aligns with the standard practice of fetching a single record by its numeric ID.backend/src/main/java/com/techeer/backend/api/aifeedback/service/AIFeedbackService.java (5)
31-31: Logger InitializationDefining a dedicated logger for this service is a good practice for structured logs, especially when diagnosing S3 file transactions and OpenAI responses.
38-39: Constructor InjectionThe constructor injection pattern here maintains consistency and testability across the service. No issues noted.
50-77: Validate Access Control for ResumeWhile the logic to generate AI feedback from an S3-stored PDF is solid, there's no direct check ensuring the requesting user has access to the resume. Confirm that relevant security/ownership checks are performed in higher layers or implement them here if needed.
79-86: AI Feedback Retrieval MethodsBoth
getFeedbackByIdandgetFeedbacksByResumeIdmethods perform as expected, throwing the correct exception if feedback is not found. Implementation is concise.
88-97: Key Extraction Method Works As IntendedThis utility method neatly addresses URL decoding and potential path issues. The fallback to
BusinessExceptionwhen decoding fails is suitable for error handling.
| Optional<AIFeedback> findByResumeId(Long resumeId); | ||
| List<AIFeedback> findByResumeId(Long resumeId); | ||
|
|
||
| Optional<AIFeedback> findById(Long feedbackId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant findById method.
This method is redundant since JpaRepository already provides a findById method with the same signature. The method can be safely removed to reduce code duplication.
- Optional<AIFeedback> findById(Long feedbackId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Optional<AIFeedback> findById(Long feedbackId); |
|
|
||
| public List<Feedback> getFeedbacksByResumeId(Long resumeId) { | ||
| return feedbackRepository.findAllByResumeId(resumeId); | ||
| } |
There was a problem hiding this comment.
Consolidate duplicate feedback retrieval methods.
There are now two nearly identical methods that retrieve feedbacks by resumeId:
getFeedbackByResumeId(singular) - includes resume existence validationgetFeedbacksByResumeId(plural) - skips validation
This creates confusion and a potential bug since the new method bypasses validation. Consider one of these approaches:
- Use the existing method and rename it to plural form, or
- Add validation to the new method and remove the old one
If keeping both methods, add resume validation to the new method:
public List<Feedback> getFeedbacksByResumeId(Long resumeId) {
+ // Validate resume exists
+ if (!resumeRepository.existsById(resumeId)) {
+ throw new BusinessException(ErrorCode.RESUME_NOT_FOUND);
+ }
return feedbackRepository.findAllByResumeId(resumeId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<Feedback> getFeedbacksByResumeId(Long resumeId) { | |
| return feedbackRepository.findAllByResumeId(resumeId); | |
| } | |
| public List<Feedback> getFeedbacksByResumeId(Long resumeId) { | |
| // Validate resume exists | |
| if (!resumeRepository.existsById(resumeId)) { | |
| throw new BusinessException(ErrorCode.RESUME_NOT_FOUND); | |
| } | |
| return feedbackRepository.findAllByResumeId(resumeId); | |
| } |
| @Operation(summary = "AI 피드백, 일반 피드백 조회", description = "해당 이력서에 대한 일반 피드백과 특정 AI 피드백(aifeedbackId를 이용)을 조회합니다.") | ||
| @GetMapping("/{resume_id}/feedbacks/{aifeedback_id}") | ||
| public CommonResponse<AllFeedbackResponse> getFeedbackWithAIFeedback( | ||
| @PathVariable("resume_id") Long resumeId, | ||
| @PathVariable("aifeedback_id") Long aifeedbackId) { | ||
|
|
||
| // 엔티티, 리스트 반환 | ||
| // 일반 피드백 조회 (이력서 ID 기준) | ||
| List<Feedback> feedbacks = feedbackService.getFeedbackByResumeId(resumeId); | ||
| AIFeedback aiFeedback = feedbackService.getAIFeedbackByResumeId(resumeId); | ||
| // 특정 AI 피드백 조회 (AI 피드백 ID 기준) | ||
| AIFeedback aiFeedback = aifeedbackService.getFeedbackById(aifeedbackId); | ||
|
|
||
| // 두 결과를 하나의 DTO로 변환 (여기서는 원시 도메인 객체를 전달) | ||
| AllFeedbackResponse response = FeedbackConverter.toAllFeedbackResponse(feedbacks, aiFeedback); | ||
|
|
||
| return CommonResponse.of(SuccessCode.FEEDBACK_FETCH_OK, response); | ||
| } |
There was a problem hiding this comment.
Validate AI Feedback Ownership
When fetching AI feedback by ID and general feedback by resume ID, there is no check to ensure that the retrieved AI feedback actually belongs to the specified resume. This could cause a user to retrieve AI feedback that doesn't match the given resume, potentially exposing data that shouldn't be accessible.
Below is a sample diff demonstrating how you might verify the resumeId alignment before returning the combined feedback:
public CommonResponse<AllFeedbackResponse> getFeedbackWithAIFeedback(
@PathVariable("resume_id") Long resumeId,
@PathVariable("aifeedback_id") Long aifeedbackId) {
List<Feedback> feedbacks = feedbackService.getFeedbackByResumeId(resumeId);
AIFeedback aiFeedback = aifeedbackService.getFeedbackById(aifeedbackId);
+ // Verify that aiFeedback belongs to the same resume
+ if (!aiFeedback.getResumeId().equals(resumeId)) {
+ throw new BusinessException(ErrorCode.FEEDBACK_NOT_FOUND);
+ }
AllFeedbackResponse response = FeedbackConverter.toAllFeedbackResponse(feedbacks, aiFeedback);
return CommonResponse.of(SuccessCode.FEEDBACK_FETCH_OK, response);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Operation(summary = "AI 피드백, 일반 피드백 조회", description = "해당 이력서에 대한 일반 피드백과 특정 AI 피드백(aifeedbackId를 이용)을 조회합니다.") | |
| @GetMapping("/{resume_id}/feedbacks/{aifeedback_id}") | |
| public CommonResponse<AllFeedbackResponse> getFeedbackWithAIFeedback( | |
| @PathVariable("resume_id") Long resumeId, | |
| @PathVariable("aifeedback_id") Long aifeedbackId) { | |
| // 엔티티, 리스트 반환 | |
| // 일반 피드백 조회 (이력서 ID 기준) | |
| List<Feedback> feedbacks = feedbackService.getFeedbackByResumeId(resumeId); | |
| AIFeedback aiFeedback = feedbackService.getAIFeedbackByResumeId(resumeId); | |
| // 특정 AI 피드백 조회 (AI 피드백 ID 기준) | |
| AIFeedback aiFeedback = aifeedbackService.getFeedbackById(aifeedbackId); | |
| // 두 결과를 하나의 DTO로 변환 (여기서는 원시 도메인 객체를 전달) | |
| AllFeedbackResponse response = FeedbackConverter.toAllFeedbackResponse(feedbacks, aiFeedback); | |
| return CommonResponse.of(SuccessCode.FEEDBACK_FETCH_OK, response); | |
| } | |
| @Operation(summary = "AI 피드백, 일반 피드백 조회", description = "해당 이력서에 대한 일반 피드백과 특정 AI 피드백(aifeedbackId를 이용)을 조회합니다.") | |
| @GetMapping("/{resume_id}/feedbacks/{aifeedback_id}") | |
| public CommonResponse<AllFeedbackResponse> getFeedbackWithAIFeedback( | |
| @PathVariable("resume_id") Long resumeId, | |
| @PathVariable("aifeedback_id") Long aifeedbackId) { | |
| // 일반 피드백 조회 (이력서 ID 기준) | |
| List<Feedback> feedbacks = feedbackService.getFeedbackByResumeId(resumeId); | |
| // 특정 AI 피드백 조회 (AI 피드백 ID 기준) | |
| AIFeedback aiFeedback = aifeedbackService.getFeedbackById(aifeedbackId); | |
| // Verify that aiFeedback belongs to the same resume | |
| if (!aiFeedback.getResumeId().equals(resumeId)) { | |
| throw new BusinessException(ErrorCode.FEEDBACK_NOT_FOUND); | |
| } | |
| // 두 결과를 하나의 DTO로 변환 (여기서는 원시 도메인 객체를 전달) | |
| AllFeedbackResponse response = FeedbackConverter.toAllFeedbackResponse(feedbacks, aiFeedback); | |
| return CommonResponse.of(SuccessCode.FEEDBACK_FETCH_OK, response); | |
| } |
| Map<String, String> message = new HashMap<>(); | ||
| message.put("role", "user"); | ||
| // 불필요한 공백이나 개행이 문제를 일으킬 수 있으므로 trim()으로 정리합니다. | ||
| message.put("content", ("이 이력서에서 잘 작성된 부분과 개선해야 할 부분을 구체적으로 지적해 주세요. " + |
There was a problem hiding this comment.
여기에 예를들어 gpt한테 너는 IT 빅테크 기업(구글, 페이스북, AWS)면접관이야. 이런식으로 role 부여해주면 대답이 더 좋아져. 이런거 찾아보고 role 추가해도 좋을거 같아.
| @Operation(summary = "AI 피드백, 일반 피드백 조회", description = "해당 이력서에 대한 일반 피드백과 특정 AI 피드백(aifeedbackId를 이용)을 조회합니다.") | ||
| @GetMapping("/{resume_id}/feedbacks/{aifeedback_id}") | ||
| public CommonResponse<AllFeedbackResponse> getFeedbackWithAIFeedback( | ||
| @PathVariable("resume_id") Long resumeId, | ||
| @PathVariable("aifeedback_id") Long aifeedbackId) { |
There was a problem hiding this comment.
url endpoint 이름이 지금 aifeedback_id인데 feedback_id로 가는게 맞을 듯
| // 일반 피드백 조회 (이력서 ID 기준) | ||
| List<Feedback> feedbacks = feedbackService.getFeedbackByResumeId(resumeId); | ||
| AIFeedback aiFeedback = feedbackService.getAIFeedbackByResumeId(resumeId); | ||
| // 특정 AI 피드백 조회 (AI 피드백 ID 기준) | ||
| AIFeedback aiFeedback = aifeedbackService.getFeedbackById(aifeedbackId); | ||
|
|
||
| // 두 결과를 하나의 DTO로 변환 (여기서는 원시 도메인 객체를 전달) | ||
| AllFeedbackResponse response = FeedbackConverter.toAllFeedbackResponse(feedbacks, aiFeedback); | ||
|
|
||
| return CommonResponse.of(SuccessCode.FEEDBACK_FETCH_OK, response); | ||
| } |
There was a problem hiding this comment.
특정 feedback 조회에 대한 api인데 toAllFeedbackResponse로 바꿔서 반환하는 로직은 뭔가 부자연스럽다고 느껴짐.
전체 feedback이 아니라 하나의 feedback에 대한 조회라면 좀 수정되어야 할 듯?
이전 로직에서 수정을 하면서 뭔가 이상해진 느낌이 있어
There was a problem hiding this comment.
그리고 기왕이면 unit test라도 짜줘. openai service를 mock으로 주입하면 충분히 가능할 듯
변경 사항
테스트 결과 (테스트를 했으면)
Summary by CodeRabbit
New Features
Refactor
Documentation