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/#51 로그 관련 도메인와 로그 API 요청의 필드명 변경 #53

Merged
merged 11 commits into from
Aug 15, 2024

Conversation

miiiinju1
Copy link
Member

@miiiinju1 miiiinju1 commented Aug 14, 2024

🚀 작업 내용

  • 로그 관련 엔티티와 DTO의 필드명 변경
  • UUID 변환 유틸리티 클래스 추가, 그리고 application_id를 appKey로 변경하는 작업 수행

📸 이슈 번호

👀 Focus Commits [Optional]

  • ac1b255, cd4a25e : UUID를 DB예 binary를 저장할 때 이용할 util클래스를 만들었습니다.

✍ 궁금한 점

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

- CreateLogRequest
  - logLevel -> level
  - logData -> data
- CreateLogServiceRequest
  - logLevel -> level
  - logData -> data
- Log
  - logData -> data
- 필드 변경에 따른 테스트 코드 수정
UUID 문자열과 byte 배열 간의 변환을 위한 UUIDUtil 클래스를 구현했습니다.
- uuidStringToBytes: UUID 문자열을 byte 배열로 변환
- bytesToUuidString: byte 배열을 UUID 문자열로 변환
UUID 변환을 검증하기 위한 UUIDUtilTest 클래스 생성:
- uuidToBytes_ShouldConvertUUIDStringToByteArray: UUID 문자열을 byte 배열로 변환하는 것을 검증
- bytesToUUIDString_ShouldConvertByteArrayToUUIDString: byte 배열을 UUID 문자열로 변환하는 것을 검증
- CreateLogServiceRequest applicationId -> appKey로 변경
- Log 도메인 객체에서 applicationId -> appKey
- LogController app-id -> appKey로 변경, Validation NotBlank로 변경
- LogSerivce에서 applicationId -> appKey로 변경
- 관련 테스트 변경
@miiiinju1 miiiinju1 requested a review from a team August 14, 2024 11:29
@miiiinju1 miiiinju1 self-assigned this Aug 14, 2024
Copy link

github-actions bot commented Aug 14, 2024

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

The new methods for creating and retrieving apps are well-structured. Ensure that the appType is validated before processing. Example:

if (appTypeStr == null || appTypeStr.isBlank()) {
    throw new IllegalArgumentException(\"appType cannot be null or empty\");
}

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

Consider validating the input parameters (appKey, level, data, timestamp) before creating the Log object to prevent potential NullPointerExceptions. Example:

if (appKey == null || appKey.isBlank()) {
    throw new IllegalArgumentException(\"appKey cannot be null or empty\");
}

Risk Level 2 - /home/runner/work/Team5-Guys/Team5-Guys/logbat/src/main/java/info/logbat/domain/log/domain/Log.java

The validation methods for appKey and timestamp are good. Ensure that the level and data are also validated to prevent invalid states. Example:

if (level == null || level.isBlank()) {
    throw new IllegalArgumentException(\"level cannot be null or empty\");
}

🛠️✅⚠️


Powered by Code Review GPT

@miiiinju1 miiiinju1 added ⭐️ Feat 새로운 기능이나 요청 🔄 Refactor 코드 리팩토링 labels Aug 14, 2024
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.

리뷰 확인해주세요 :D

전반적인 코드는 좋았습니다!

Comment on lines 15 to 18
ByteBuffer bb = ByteBuffer.wrap(new byte[UUID_BYTE_LENGTH]);
bb.putLong(uuid.getMostSignificantBits());
bb.putLong(uuid.getLeastSignificantBits());
return bb.array();
Copy link
Member

Choose a reason for hiding this comment

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

bb 보다는 byteBuffer로 사용하는게 더 직관적일 것 같아요! :D

Copy link
Member

Choose a reason for hiding this comment

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

성공 실패 응답만 주면 되니까 201로 주는건 어떨까요?

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.

노션 API 문서도 업데이트 했습니다~

Comment on lines 20 to 22
assertThat(bytes).isNotNull();
assertThat(bytes.length).isEqualTo(16);
assertThat(bytes).isEqualTo(expectedBytes);
Copy link
Member

Choose a reason for hiding this comment

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

assertAll() 로 묶는것은 어떨까요?!

@tidavid1 tidavid1 requested a review from LuizyHub August 14, 2024 13:21
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.

고칠 것을 믿고 어풉 냄겨둡니당 :D

@@ -23,16 +23,15 @@ public class LogController {

@PostMapping
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@PostMapping
@PostMapping

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.

승인입니다!! 제가 드린 제안도한번 생각해봐주시면 감사하겠습니다~

@@ -23,16 +23,15 @@ public class LogController {

@PostMapping
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 응답 status를 정하는건 어떤가요?
해당 API는 성공했을때 201을 보내야 하니 응답 코드에 대한 명시가 함수 내부에 있는것 보다 함수 정의 바로 위에 있는면이 개인적으로 더 보기 좋다고 생각해서 제안드립니다!

Suggested change
@PostMapping
@PostMapping
@ResponseStatus(HttpStatus.CREATED)

Copy link
Member

Choose a reason for hiding this comment

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

이렇게 하면 함수 리턴타입을 void로 해야할 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

반영했어요!!

if (applicationId == null) {
throw new IllegalArgumentException("applicationId는 null이 될 수 없습니다.");
private void validateAppKey(String appKey) {
if (appKey == null || appKey.isBlank()) {
Copy link
Member

Choose a reason for hiding this comment

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

저도 항상 고민인 부분인데, 컨트롤러에서도 이것에 대한 검사를 하는것 같아서 두번해도 괜찮나 생각듭니다...!
각자 맡아야 할 책임이 달라서 두군데 모두 있는면이 자연스럽다고 생각합니다!

@miiiinju1 miiiinju1 merged commit a1099c1 into dev Aug 15, 2024
1 check passed
@miiiinju1 miiiinju1 deleted the feat/#51 branch August 15, 2024 00:08
LuizyHub pushed a commit that referenced this pull request Aug 19, 2024
* refactor: 로그 레벨, 데이터 컬럼명 변경

- CreateLogRequest
  - logLevel -> level
  - logData -> data
- CreateLogServiceRequest
  - logLevel -> level
  - logData -> data
- Log
  - logData -> data
- 필드 변경에 따른 테스트 코드 수정

* feat: UUID와 byte 배열 변환을 위한 UUIDUtil 클래스 추가

UUID 문자열과 byte 배열 간의 변환을 위한 UUIDUtil 클래스를 구현했습니다.
- uuidStringToBytes: UUID 문자열을 byte 배열로 변환
- bytesToUuidString: byte 배열을 UUID 문자열로 변환

* test: UUID 변환을 검증하는 UUIDUtilTest 클래스 추가

UUID 변환을 검증하기 위한 UUIDUtilTest 클래스 생성:
- uuidToBytes_ShouldConvertUUIDStringToByteArray: UUID 문자열을 byte 배열로 변환하는 것을 검증
- bytesToUUIDString_ShouldConvertByteArrayToUUIDString: byte 배열을 UUID 문자열로 변환하는 것을 검증

* refactor: application_id를 appKey로 변경

- CreateLogServiceRequest applicationId -> appKey로 변경
- Log 도메인 객체에서 applicationId -> appKey
- LogController app-id -> appKey로 변경, Validation NotBlank로 변경
- LogSerivce에서 applicationId -> appKey로 변경
- 관련 테스트 변경

* refactor: UUIDUtil final 클래스로 변경, 기본 생성자 Private 추가

* feat: LogController 201 반환하도록 변경

* chore: bb -> byteBuffer로 변경

* test: Displayname 추가

* test: created로 response가 변경됨에 따라 테스트코드 변경

* test: 여러 개의 assertThat을 assertAll로 묶음

* refactor: Response 상태 코드를 어노테이션으로 처리 후 리턴 타입으로 void로 수정
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ Feat 새로운 기능이나 요청 🔄 Refactor 코드 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants