Feature/method preauthorize setting #232 #234
Conversation
Eseas
requested review from
taekminKwon,
K-KY,
kdh10806,
HiSeungmin and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public interface ProjectAuthorizationCheckService { | ||
| boolean checkParticipant(Long memberId, Long projectId); | ||
| boolean checkApprover(Long memberId, Long projectId); | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
코드 분석
- 패키지 네임:
com.seveneleven.util.methodauthorize는 패키지 구조가 명확하고, 역할에 맞게 네이밍이 잘 되어 있습니다. - 인터페이스 정의:
ProjectAuthorizationCheckService인터페이스는 프로젝트의 참가자와 승인자를 체크하는 메소드를 정의하고 있습니다.
개선 제안 및 버그 위험
-
설명 주석 추가: 현재 메소드에 대한 설명이 없습니다. 각 메소드에 대한 JavaDoc 주석을 추가하여, 이 메소드들이 어떤 상황에서 호출되어야 하는지에 대한 정보를 제공하는 것이 좋습니다.
/** * 참가자의 권한을 체크합니다. * * @param memberId : 회원 ID * @param projectId : 프로젝트 ID * @return : 참가 가능한 경우 true, 그렇지 않은 경우 false */ boolean checkParticipant(Long memberId, Long projectId); /** * 승인자의 권한을 체크합니다. * * @param memberId : 회원 ID * @param projectId : 프로젝트 ID * @return : 승인 가능한 경우 true, 그렇지 않은 경우 false */ boolean checkApprover(Long memberId, Long projectId);
-
Long 타입 사용에 대한 주의:
Long타입을 사용할 경우, null 값이 들어올 수 있습니다. 메소드 내부에서 null 값에 대한 처리가 필요할 수 있으므로, 메소드 설명에 null 값에 대한 동작을 명시하거나, null 체크를 하는 코드를 추가하는 것이 좋습니다. -
인터페이스 설계: 필요한 경우, 인터페이스에 기본 구현을 제공하는
default메소드를 추가하여, 기본적인 로직을 제공하는 것도 고려해볼 수 있습니다. -
예외 처리: 메소드에서 발생할 수 있는 예외 상황에 대한 처리가 필요할 경우, 예외를 던지거나 처리하는 방법에 대한 설계를 고려해야 합니다. 예를 들어, 유효하지 않은
memberId또는projectId에 대한 처리 등이 있습니다. -
네이밍 규칙: 메소드 이름은 명확하지만, 중복이 날 수 있는 이름(예:
checkParticipant,checkApprover)인지 검토하여 필요 시 다른 방식으로 네이밍하는 것도 고려하세요.
이러한 점들을 고려하여 더 나은 코드 품질을 유지할 수 있도록 개선하는 것이 좋습니다.
|
|
||
| return authorization.isPresent() && "APPROVER".equals(authorization.get().getAuthorizationCode()); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
패키지 및 클래스 구조:
- 클래스와 패키지 구조가 명확하게 잘 되어 있습니다. 기능별로 나눠져 있어 유지보수성이 좋습니다.
-
의존성 주입:
@RequiredArgsConstructor를 사용하여 의존성을 효율적으로 주입하는 방식이 좋습니다. 하지만,ProjectAuthorizationRepository가 반드시 필요하지 않은 경우를 대비해 Optional을 사용하여 의존성 주입을 하도록 개선하는 것도 고려해볼 수 있습니다.
-
메서드 중복:
checkParticipant와checkApprover메서드가 유사한 코드 구조를 가지고 있습니다.projectAuthorizationRepository.findByProjectIdAndMemberIdAndIsActive호출 부분을 공통 메서드로 빼내어 중복 코드를 줄이는 것이 좋습니다.
private Optional<ProjectAuthorization> getAuthorization(Long memberId, Long projectId) { return projectAuthorizationRepository .findByProjectIdAndMemberIdAndIsActive(projectId, memberId, YesNo.YES); }
이렇게 메서드를 추가한 후, 두 메서드에서 호출하도록 변경할 수 있습니다.
-
예외 처리:
- 현재 코드에서는
Optional을 사용하여 인가 여부를 확인하고 있으나, 만약 데이터베이스 쿼리에서 예외가 발생할 경우 (예: 데이터베이스 연결 문제) 이를 처리할 로직이 없기 때문에 예외 처리 로직을 추가하는 것이 좋습니다.
- 현재 코드에서는
-
상수 사용:
- 'APPROVER'라는 문자열이 하드코딩 되어 있습니다. 상수를 사용하여 'APPROVER'를 관리하는 것이 좋습니다. 코드의 가독성을 높이고 향후 수정 시 오타 또는 일관성 문제를 방지할 수 있습니다.
-
주석 및 문서화:
- 메서드에 대한 설명이 부족합니다. 각 메서드에 대한 간단한 주석을 추가하여 기능을 설명하면 코드 이해가 더 쉬워질 것입니다.
-
널 체크:
checkApprover메서드에서authorization.get()을 호출하기 전에,isPresent()로 확인하고 있지만, 만약 예외 상황이 발생할 수도 있는 구현이므로,orElse(null)등을 사용하여 무결성을 챙길 수 있습니다.
결론적으로, 전반적으로 잘 작성된 코드이나, 중복 코드 감소, 예외 처리, 상수 사용, 주석 추가 등의 개선 사항이 있습니다. 이를 반영하면 코드의 품질이 더욱 향상될 것입니다.
| @Retention(RetentionPolicy.RUNTIME) | ||
| @PreAuthorize("@projectAuthorizationCheckService.checkApprover(principal.id, #projectId) or hasRole('ADMIN')") | ||
| public @interface ApproverAuthorize { | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
버그 리스크
- 주석 부족: 이 애너테이션이 어떤 상황에서 사용되는지에 대한 설명이 부족합니다. 주석을 추가하여 유지보수 시 이해를 돕는 것이 좋습니다.
- 비즈니스 로직의 의존성:
@PreAuthorize에서 의존하고 있는@projectAuthorizationCheckService가 올바르게 주입되고 초기화되지 않으면 잠재적인 NullPointerException의 원인이 될 수 있습니다.
개선 제안
- 구체적인 로깅 추가: 인증과 권한 부여의 과정에서 문제가 발생할 수 있으므로, 이를 로그로 남기는 방법을 고려해 보세요. 예를 들어, 권한 체크에 실패했을 때의 로그를 남기는 것이 좋습니다.
- 예외 처리: 권한 검사가 실패할 경우 사용자에게 적절한 피드백을 줄 수 있는 예외 처리를 고려해야 합니다.
- 네이밍:
ApproverAuthorize라는 이름은 적절하나, 프로젝트가 커질 경우 네이밍 컨벤션에 따라 패키지 구조와 좀 더 일관되게 맞출 필요가 있습니다. - Javadoc 추가: 애너테이션의 사용법 및 목적을 설명하는 Javadoc을 추가하는 것이 좋습니다. 이를 통해 다른 개발자가 쉽게 이해하고 사용할 수 있습니다.
- 종속성 주입 확인:
@projectAuthorizationCheckService가@Autowired와 같은 방식으로 주입되는지 확인하십시오. Spring의 컴포넌트 스캔이 제대로 이루어져야 합니다.
이러한 점들을 고려하여 코드를 개선하면, 안정성과 유지보수성이 한층 높아질 것입니다.
| @Retention(RetentionPolicy.RUNTIME) | ||
| @PreAuthorize("@projectAuthorizationCheckService.checkParticipant(principal.id, #projectId) or hasRole('ADMIN')") | ||
| public @interface ParticipantAuthorize { | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
코드 리뷰
-
주석 추가: 현재 코드에 대한 주석이 없어서, 이 어노테이션의 역할이나 사용 목적을 설명하는 주석을 추가하는 것이 좋습니다. 나중에 다른 개발자가 이 코드를 이해하는 데 도움이 됩니다.
-
의존성 주의:
@PreAuthorize에서 사용된@projectAuthorizationCheckService.checkParticipant(...)부분에서 사용하는projectAuthorizationCheckService가 적절하게 Spring 컨텍스트에 등록되어 있는지 확인해야 합니다. 이 의존성이 없거나 잘못된 경우, 애플리케이션에서 런타임 예외가 발생할 수 있습니다. -
상수 사용:
hasRole('ADMIN')와 같은 문자열은 상수로 정의하여 사용하는 것이 좋습니다. 향후 역할 이름이 변경될 경우, 모든 사용자를 수정해야 하는 불편함이 있습니다. -
시큐리티 설정: 스프링 시큐리티 설정에서 이 어노테이션을 올바르게 인식하도록 설정되어 있는지 확인해야 합니다. 필요한 경우, 사용자 정의 메서드 등을 정의한 경우 이 부분에 대한 테스트가 필요합니다.
-
네이밍과 예외 처리:
checkParticipant메서드에서 확인하는 로직이 무엇인지 명확하면 좋습니다. 예를 들어, 이 메서드가 무엇을 체크하고 어떤 상황에서 예외를 발생시키는지를 문서화하는 것이 좋습니다. -
코드 스타일: 마지막 줄에 개행이 없는 부분을 수정하여 코드 스타일을 일관되게 유지하는 것이 좋습니다.
위의 사항들을 고려하여 코드를 수정하면 향후 유지보수 및 가독성이 높아질 것입니다.
| @@ -160,7 +160,6 @@ public ResponseEntity<APIResponse<List<LinkResponse>>> getProjectChecklistApplic | |||
| )); | |||
| } | |||
|
|
|||
|
|
|||
| /** | |||
| * 함수명 : postProjectChecklistAccept | |||
| * 해당 체크리스트 승인 요청을 승인 처리하는 함수 | |||
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
코드 형식: 코드에서 불필요한 빈 줄이 제거된 것처럼 보입니다. 일반적으로 데드 코드 정리나 공백 줄 제거는 프로젝트의 가독성을 높이는 데 도움이 됩니다. 하지만, 이런 변경이 의미가 있는지는 코드의 전체적인 흐름을 보지 않고 판단하기 어렵습니다.
-
주석:
postProjectChecklistAccept함수의 샘플이 주석으로 설명되어 있습니다. 주석이 함수의 목적을 잘 설명하고 있지만, 추가적인 세부 사항(예: 입력 파라미터, 반환 값 등)을 포함하면 더 좋을 것 같습니다. 명확한 문서화는 협업 시 큰 도움이 됩니다. -
예외 처리: 해당 코드의 기본 기능은 확인했지만,
postProjectChecklistAccept함수 내에서 예외 처리나 에러 핸들링이 어떻게 이루어지는지 확인하는 것이 중요합니다. 예외가 발생했을 때 적절한 응답을 반환하지 않으면 시스템의 안정성에 문제가 발생할 수 있습니다. -
기능적 리스크:
postProjectChecklistAccept함수의 구현 내용이 보이지 않아서 기능적 리스크를 정확히 평가하기 어렵습니다. 이 함수가 비즈니스 로직을 제대로 처리하고, 필요한 모든 검증을 수행하는지 체크할 필요가 있습니다. -
테스트 케이스: 해당 함수에 대한 단위 및 통합 테스트가 잘 구현되어 있는지 확인해보세요. 코드 변경 후에 테스트가 실패하지 않도록 하기 위해서는 테스트 케이스를 잘 관리하는 것이 중요합니다.
이와 같은 점들을 고려할 때, 전체적인 코드 구조와 흐름을 더 확대해서 보는 것이 필요합니다. 추가적인 코드 셋에 대해 논의할 수 있는 기회를 주시면 감사하겠습니다.
|
|
||
| if(authorization.isPresent() && "APPROVER".equals(authorization.get().getAuthorizationCode())) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
로깅 추가:
@Slf4j를 사용하여 로깅 기능을 추가한 점은 좋습니다. 권한이 없는 사용자가 호출했을 경우에 에러 로그를 남기는 것은 디버깅 시 유용할 것입니다. -
isPresent()대체:authorization.isPresent()를 사용하여 권한이 있는지를 확인하는 대신,Optional의map()이나filter()메소드와 함께orElse(false)식을 활용하여 좀 더 함수형 프로그래밍 스타일로 코드에 가독성을 높일 수 있습니다. 예를 들면:return authorization.map(auth -> "APPROVER".equals(auth.getAuthorizationCode())).orElse(false);
-
로그 레벨: 권한이 없는 경우
log.error로 로그를 남기고 있는데, 이 경우log.warn로 변경하는 것이 적절할 수 있습니다. 에러가 아니라 경고 수준의 메시지이기 때문입니다. -
중복 코드 제거:
checkParticipant와checkApprover메소드가 유사한 구조로 구현되어 있습니다. 이 두 메소드를 리팩토링하여 중복 코드를 최소화할 수 있습니다. 예를 들어, 권한 확인 로직을 공통 메소드로 추출할 수 있습니다. -
매직 문자열:
"APPROVER"와 같은 문자열을 코드에 직접 사용하는 것은 매직 문자열로 간주되며, 유지보수가 어려워질 수 있습니다. 이 문자열을 상수로 정의하는 것이 좋습니다. -
성능 고려: 두 메소드 모두
findByProjectIdAndMemberIdAndIsActive의 결과로Optional을 반환하는데, 이가 여러 번 호출되는 구조라면 데이터베이스 쿼리를 최적화하기 위해 필요한 모든 정보를 한 번에 로드하는 방법을 고려할 수 있습니다.
이러한 점들을 고려하면 코드의 가독성과 유지보수성을 높일 수 있을 것으로 보입니다. 전체적으로 에러 핸들링을 잘 해놓았고, 로그를 추가하여 문제를 추적하기 쉽게 한 점은 긍정적입니다.
|
methodAuthorize 디렉터리명 카멜케이스로 변경해주세요 |
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
ex)
bucket post 및 bucket list view 초기 구현
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토