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

Feat/#25 Log를 저장할 떄 appKey를 검증하도록 수정 #84

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

miiiinju1
Copy link
Member

🚀 작업 내용

  • appKey가 존재하는 지 확인하는 쿼리 메서드 추가
  • Log를 저장할 때 appKey가 존재하는지 확인하도록 수정했습니다.

📸 이슈 번호

👀 Focus Commits [Optional]

  • 커밋해시: 내용

✍ 궁금한 점

  • 중점적으로 봐줄 내용
  • 변수명 괜찮나요
  • 로직이 좀 더럽나요
  • 가독성이 좀 그런가요?

- 존재하지 않는 Application Key로 Log를 저장할 수 없다.
- 잘못된 형태의 Application Key로 Log를 저장할 수 없다.
@miiiinju1 miiiinju1 self-assigned this Aug 19, 2024
Copy link

github-actions bot commented Aug 19, 2024

Risk Level 2 - /home/runner/work/Team5-Guys/Team5-Guys/logbat/src/main/java/info/logbat/domain/project/repository/AppJpaRepository.java

  1. The removal of the findByToken method may impact existing functionality if it is still being used elsewhere in the codebase. Ensure that this method is not referenced before removing it.

Risk Level 2 - /home/runner/work/Team5-Guys/Team5-Guys/logbat/src/test/java/info/logbat/domain/log/application/LogServiceTest.java

  1. The test method names are in Korean, which may reduce readability for non-Korean speakers. Consider using English for consistency and wider accessibility. Example:
    @DisplayName(\"Cannot save log with non-existent Application Key\")
    void saveLogWithNonExistentAppKey() {
        // test implementation
    }

Risk Level 3 - /home/runner/work/Team5-Guys/Team5-Guys/logbat/src/main/java/info/logbat/domain/log/application/LogService.java

  1. The method getAppKey throws a generic IllegalArgumentException. Consider creating a custom exception for better clarity and handling. Example:
    public class InvalidAppKeyException extends RuntimeException {
        public InvalidAppKeyException(String message) {
            super(message);
        }
    }
  2. The method getAppKey could benefit from clearer naming. Consider renaming it to validateAndGetAppKey to reflect its purpose more accurately.

🔍⚠️📜


Powered by Code Review GPT

Copy link
Member

@tidavid1 tidavid1 left a comment

Choose a reason for hiding this comment

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

👍🏻

Comment on lines 27 to 29
if (!appJpaRepository.existsByToken(appKeyUuid)) {
throw new IllegalArgumentException("존재하지 않는 Application Key 입니다.");
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분도 getAppKey() 부분에 들어가 있으면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

좋아요~~

Copy link
Member

Choose a reason for hiding this comment

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

린트 크아ㅏㅏㅏㄱ

Copy link
Member

@LuizyHub LuizyHub left a comment

Choose a reason for hiding this comment

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

잘 봤습니다!

@miiiinju1 miiiinju1 merged commit 8a1c1fc into dev Aug 19, 2024
1 check passed
@miiiinju1 miiiinju1 deleted the feat/#25 branch August 19, 2024 05:14
LuizyHub pushed a commit that referenced this pull request Aug 19, 2024
* feat: appKey가 존재하는 지 확인하는 쿼리 메서드 추가

- existsByToken메서드 추가

* feat: Log를 저장할 때 appKey가 존재하는 지 확인하도록 수정

* test: Log를 저장할 때 appKey가 존재하는 지 확인하도록 수정

- 존재하지 않는 Application Key로 Log를 저장할 수 없다.
- 잘못된 형태의 Application Key로 Log를 저장할 수 없다.

* refactor: Log를 저장할 때 appKey 검증 로직 리팩토링
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants