-
Notifications
You must be signed in to change notification settings - Fork 0
코드 리뷰 Github Action 테스트를 위한 일부 코드 리팩토링 #11
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
Merged
Merged
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
d801e9d
refactor: Modify customException, memberService
comeevery-git c5d6486
fix: Modify code-review.yml
comeevery-git e5ab6e1
fix: Apply openai 1.x.x
comeevery-git d13f89c
fix: Modify openai response format
comeevery-git f21922f
fix: Modify openai response format
comeevery-git ead8754
fix: Create gpt_review.txt
comeevery-git 1330aeb
fix: Add log
comeevery-git a84d9d0
fix: Modify code-review.yml
comeevery-git 25ace9e
fix: Modify code-review.yml
comeevery-git 3dab92f
fix: Modify code-review.yml
comeevery-git 88c0f68
fix: Modify code-review.yml
comeevery-git af7eeb2
fix: Add log code-review.yml
comeevery-git ddeb9de
fix: Add log code-review.yml
comeevery-git 6f705f5
fix: Modify code-review.yml
comeevery-git 922112a
fix: Modify code-review.yml
comeevery-git 40bb9e9
fix: Modify code-review.yml
comeevery-git 47a60ff
fix: Modify code-review.yml
comeevery-git 231785a
fix: Modify code-review.yml
comeevery-git 70928d3
fix: Modify code-review.yml
comeevery-git 0d95077
fix: Modify code-review.yml
comeevery-git 3fe36db
fix: Modify code-review.yml
comeevery-git a14ff3d
fix: Modify code-review.yml
comeevery-git 80e2723
fix: Modify code-review.yml
comeevery-git 87e2c6e
fix: Modify code-review.yml
comeevery-git 1aa7e9b
fix: Modify code-review.yml
comeevery-git 63fd053
fix: Modify code-review.yml
comeevery-git 4b3d9b4
fix: Modify code-review.yml
comeevery-git 48610ee
fix: Modify code-review.yml
comeevery-git 42e7419
fix: Modify code-review.yml
comeevery-git 035b5b1
fix: Modify code-review.yml
comeevery-git 456a8cf
fix: Modify code-review.yml
comeevery-git 7acad3f
fix: Modify code-review.yml
comeevery-git d05eea0
fix: Modify code-review.yml
comeevery-git 6e45dda
fix: Modify code-review.yml
comeevery-git File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| name: Code Review with GPT | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| code_review: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '3.x' | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| pip install openai requests | ||
|
|
||
| - name: Generate diff for code review | ||
| id: diff | ||
| run: | | ||
| git fetch origin ${{ github.event.pull_request.base.ref }} --depth=1 | ||
| git diff origin/${{ github.event.pull_request.base.ref }} ${{ github.sha }} > pr_diff.txt | ||
| shell: bash | ||
|
|
||
| - name: Perform Code Review with GPT and Comment on Changed Lines | ||
| id: gpt_review | ||
| run: | | ||
| python <<EOF | ||
| import openai | ||
| import requests | ||
| import json | ||
|
|
||
| # OpenAI API Key 설정 | ||
| openai.api_key = "${{ secrets.OPENAI_API_KEY }}" | ||
|
|
||
| # 시스템 프롬프트 및 모델명 가져오기 | ||
| system_prompt = """${{ secrets.SYSTEM_PROMPT }}""" | ||
| model_name = "${{ secrets.OPENAI_MODEL }}" | ||
|
|
||
| # 처리된 파일과 라인 기록 (중복 방지) | ||
| processed_files = set() # 각 파일을 기록 | ||
| processed_lines = {} # 각 파일별로 라인을 기록하기 위한 딕셔너리로 수정 | ||
|
|
||
| # GitHub API 호출 로그 | ||
| print("Fetching changed files from GitHub API...") | ||
|
|
||
| # 변경된 파일과 라인 정보를 가져오기 위한 GitHub API 호출 | ||
| files_url = f"https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files" | ||
| headers = {"Authorization": f"token ${{ secrets.GITHUB_TOKEN }}"} | ||
| try: | ||
| response = requests.get(files_url, headers=headers, timeout=10) | ||
| print(f"GitHub API status code: {response.status_code}") # 상태 코드 로그 출력 | ||
| response.raise_for_status() | ||
| except requests.exceptions.RequestException as e: | ||
| print(f"Error fetching files: {e}") | ||
| exit(1) | ||
|
|
||
| if response.status_code == 200: | ||
| print("Files fetched successfully. Analyzing changes...") | ||
| files_changed = response.json() | ||
|
|
||
| # .java 파일만 필터링 | ||
| java_files = [file for file in files_changed if file['filename'].endswith('.java')] | ||
|
|
||
| if not java_files: | ||
| print("No .java files to review.") | ||
| exit(0) | ||
|
|
||
| for file in java_files: | ||
| file_path = file['filename'] | ||
|
|
||
| # 이미 처리한 파일은 건너뛰기 | ||
| if file_path in processed_files: | ||
| print(f"Skipping already processed file: {file_path}") | ||
| continue | ||
|
|
||
| print(f"Processing file: {file_path}") | ||
| patch = file.get('patch', '') | ||
| print(f"Analyzing patch for file: {file_path}") | ||
|
|
||
| # 각 파일마다 중복 라인 처리를 위한 집합 생성 | ||
| if file_path not in processed_lines: | ||
| processed_lines[file_path] = set() | ||
|
|
||
| # 전체 패치 내용을 GPT에게 전달 | ||
| if patch: # 패치가 존재할 경우에만 처리 | ||
| print(f"Calling GPT API for patch in file: {file_path}") | ||
| try: | ||
| gpt_response = openai.chat.completions.create( | ||
| model=model_name, | ||
| messages=[ | ||
| {"role": "system", "content": system_prompt}, | ||
| {"role": "user", "content": f"Here is the code diff for context:\n{patch}"} | ||
| ], | ||
| timeout=30 | ||
| ) | ||
| except Exception as e: | ||
| print(f"Error in GPT request: {e}") | ||
| continue | ||
|
|
||
| review_comment = gpt_response.choices[0].message.content | ||
| print(f"GPT response received for file: {file_path}") | ||
| print(f"Review comment: {review_comment}") | ||
|
|
||
| # 변경된 파일과 라인에 리뷰 코멘트를 추가 | ||
| commit_id = "${{ github.event.pull_request.head.sha }}" | ||
| line_number = file.get('patch').split('\n').index(next(line for line in file['patch'].split('\n') if line.startswith('+'))) + 1 | ||
| comment_body = { | ||
| "body": review_comment, | ||
| "path": file_path, | ||
| "line": line_number, | ||
| "side": "RIGHT", | ||
| "commit_id": commit_id | ||
| } | ||
|
|
||
| # 코멘트를 추가하기 전 파일 경로와 위치를 로그로 출력 | ||
| print(f"Commenting on file: {file_path}, line: {line_number}") | ||
|
|
||
| comment_url = f"https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/comments" | ||
| response = requests.post(comment_url, headers=headers, data=json.dumps(comment_body)) | ||
|
|
||
| # 응답 상태 확인 | ||
| if response.status_code == 201: | ||
| print(f"Comment posted successfully for file: {file_path}") | ||
| else: | ||
| print(f"Failed to post comment. Status code: {response.status_code}, Response: {response.text}") | ||
|
|
||
| # 각 파일을 처리한 후 파일 이름을 기록 | ||
| processed_files.add(file_path) | ||
|
|
||
| else: | ||
| print(f"Unexpected status code: {response.status_code}") | ||
| exit(1) | ||
|
|
||
| # 최종 리뷰 요약 코멘트 추가 | ||
| final_comment = "### 최종 리뷰 요약: .java 파일에 대한 모든 변경 사항을 검토 완료했습니다." | ||
| comment_url = f"https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/comments" | ||
| requests.post(comment_url, headers=headers, data=json.dumps({"body": final_comment})) | ||
| print("Final review comment posted.") | ||
| exit(0) | ||
| EOF |
30 changes: 27 additions & 3 deletions
30
src/main/java/app/common/infrastructure/exception/CustomException.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,35 @@ | ||
| package app.common.infrastructure.exception; | ||
|
|
||
| import app.common.domain.model.common.ResponseCode; | ||
| import lombok.AllArgsConstructor; | ||
| import lombok.Getter; | ||
|
|
||
| @AllArgsConstructor | ||
| import java.io.Serializable; | ||
|
|
||
| @Getter | ||
| public class CustomException extends RuntimeException { | ||
| public class CustomException extends RuntimeException implements Serializable { | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private final ResponseCode responseCode; | ||
| private final String additionalMessage; | ||
|
|
||
| public CustomException(ResponseCode responseCode) { | ||
| this(responseCode, null, null); | ||
| } | ||
|
|
||
| public CustomException(ResponseCode responseCode, String additionalMessage) { | ||
| this(responseCode, additionalMessage, null); | ||
| } | ||
|
|
||
| public CustomException(ResponseCode responseCode, String additionalMessage, Throwable cause) { | ||
| super(additionalMessage, cause); | ||
| this.responseCode = responseCode; | ||
| this.additionalMessage = additionalMessage; | ||
| } | ||
|
|
||
| @Override | ||
| public String getMessage() { | ||
| return (additionalMessage != null) | ||
| ? responseCode.getMessage() + ": " + additionalMessage | ||
| : responseCode.getMessage(); | ||
| } | ||
| } | ||
78 changes: 72 additions & 6 deletions
78
src/main/java/app/common/infrastructure/exception/CustomExceptionHandler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,86 @@ | ||
| package app.common.infrastructure.exception; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Review Summary]
[Issue Summary]
[Solutions]
결과적으로, 이 코드는 예외 처리를 잘 수행하고 있으며, 추가적인 개선 사항은 필요하지 않습니다. |
||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
| import org.springframework.web.servlet.NoHandlerFoundException; | ||
| import org.springframework.web.HttpRequestMethodNotSupportedException; | ||
| import org.springframework.web.bind.MethodArgumentNotValidException; | ||
| import org.springframework.dao.DataAccessException; | ||
| import org.springframework.security.access.AccessDeniedException; | ||
|
|
||
| import javax.validation.ConstraintViolationException; | ||
|
|
||
| import app.common.domain.model.common.BaseResponse; | ||
|
|
||
| @RestControllerAdvice | ||
| public class CustomExceptionHandler { | ||
| @ExceptionHandler(Exception.class) | ||
| protected BaseResponse handleException(Exception e) { | ||
| return BaseResponse.failResponse(e); | ||
| } | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(CustomExceptionHandler.class); | ||
|
|
||
| @ExceptionHandler(CustomException.class) | ||
| protected BaseResponse handleCustomException(CustomException e) { | ||
| return BaseResponse.failResponse(e.getResponseCode()); | ||
| protected ResponseEntity<BaseResponse> handleCustomException(CustomException e) { | ||
| logger.error("CustomException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse(e.getResponseCode()); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| protected ResponseEntity<BaseResponse> handleIllegalArgumentException(IllegalArgumentException e) { | ||
| logger.error("IllegalArgumentException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("INVALID_ARGUMENT"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| @ExceptionHandler(NoHandlerFoundException.class) | ||
| protected ResponseEntity<BaseResponse> handleNoHandlerFoundException(NoHandlerFoundException e) { | ||
| logger.error("NoHandlerFoundException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("RESOURCE_NOT_FOUND"); | ||
| return new ResponseEntity<>(response, HttpStatus.NOT_FOUND); | ||
| } | ||
|
|
||
| @ExceptionHandler(HttpRequestMethodNotSupportedException.class) | ||
| protected ResponseEntity<BaseResponse> handleHttpRequestMethodNotSupportedException(HttpRequestMethodNotSupportedException e) { | ||
| logger.error("HttpRequestMethodNotSupportedException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("METHOD_NOT_ALLOWED"); | ||
| return new ResponseEntity<>(response, HttpStatus.METHOD_NOT_ALLOWED); | ||
| } | ||
|
|
||
| @ExceptionHandler(MethodArgumentNotValidException.class) | ||
| protected ResponseEntity<BaseResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) { | ||
| logger.error("MethodArgumentNotValidException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("INVALID_INPUT"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| @ExceptionHandler(ConstraintViolationException.class) | ||
| protected ResponseEntity<BaseResponse> handleConstraintViolationException(ConstraintViolationException e) { | ||
| logger.error("ConstraintViolationException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("CONSTRAINT_VIOLATION"); | ||
| return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); | ||
| } | ||
|
|
||
| @ExceptionHandler(DataAccessException.class) | ||
| protected ResponseEntity<BaseResponse> handleDataAccessException(DataAccessException e) { | ||
| logger.error("DataAccessException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("DATABASE_ERROR"); | ||
| return new ResponseEntity<>(response, HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| @ExceptionHandler(AccessDeniedException.class) | ||
| protected ResponseEntity<BaseResponse> handleAccessDeniedException(AccessDeniedException e) { | ||
| logger.error("AccessDeniedException occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("ACCESS_DENIED"); | ||
| return new ResponseEntity<>(response, HttpStatus.FORBIDDEN); | ||
| } | ||
|
|
||
| @ExceptionHandler(Exception.class) | ||
| protected ResponseEntity<BaseResponse> handleException(Exception e) { | ||
| logger.error("Unexpected exception occurred: ", e); | ||
| BaseResponse response = BaseResponse.failResponse("INTERNAL_SERVER_ERROR"); | ||
| return new ResponseEntity<>(response, HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line-by-Line Review Comments
+import java.io.Serializable;Serializable인터페이스를 추가한 부분은 데이터 직렬화 및 역직렬화에 유용합니다. 하지만 추가적인 검토가 필요할 수 있는 부분입니다.public class CustomException extends RuntimeException implements Serializable {Serializable을 구현함으로써 이 예외 클래스가 직렬화 가능하다는 점은 좋은 접근입니다. 그러나 예외가 직렬화될 때 추가적인 필드가 포함되는 경우 적절한readObject및writeObject메서드를 구현하는 것이 필요할 수 있습니다.private final String additionalMessage;null인 경우가 있으므로 적절한 검증을 고려해야 합니다.public CustomException(ResponseCode responseCode) {null을 전달하는 것은 괜찮지만,responseCode가null인 경우를 체크해야 합니다.this(responseCode, null, null);null값이 의미하는 바에 대한 주석이 필요할 수 있습니다.public CustomException(ResponseCode responseCode, String additionalMessage) {responseCode가null일 경우 예외를 발생시키는 것이 좋습니다.public CustomException(ResponseCode responseCode, String additionalMessage, Throwable cause) {null일 경우를 염두에 두어야 합니다.super(additionalMessage, cause);additionalMessage가null일 경우RuntimeException의 메시지에 영향을 줄 수 있으니 더 다루어야 할 필요가 있습니다.@Override public String getMessage() {...}getMessage메서드의 구현은 유용하나, 여기서 추가 메시지가null일 경우를 신중하게 다루어야 합니다. 메시지가 올바르게 구성되어야만 더 깨끗한 출력이 가능합니다.return (additionalMessage != null) ? responseCode.getMessage() + ": " + additionalMessage : responseCode.getMessage();additionalMessage가 비어있지 않은 경우에 대해서도 추가적인 검증이 필요할 수 있습니다. 공백 메시지를 처리하기 위한 로직이 필요할 것입니다.Final Review Comment
[Review Summary]
전체적으로 예외 처리 클래스를 개선하는 좋은 방향으로 진행되었습니다. 그러나 다음과 같은 몇 가지 문제가 발견되었습니다.
[Issue Summary]
responseCode와additionalMessage에 대한 null 체크 부족[Solutions]
responseCode와additionalMessage에 대해 null이 아닌지 체크하는 로직을 추가하세요.getMessage메서드에서additionalMessage가 빈 문자열일 경우를 나타내는 추가 로직을 도입하여 더 명확한 반환값을 제공하는 것이 좋습니다.이러한 개선을 통해 코드의 안정성과 가독성을 높일 수 있습니다.