feat: 사용자 불러오기 관련 기능 및 테스트 작성 - #5
galaxy4276 wants to merge 9 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| sourceDirs.add(kaptMain) | ||
| generatedSourceDirs.add(kaptMain) | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 코드 패치는 Gradle 빌드 스크립트 파일입니다. 다음은 몇 가지 제안 사항입니다.
-
의존성 관리 섹션에서 구버전의
spring-boot-starter-data-jpa를 사용하고 있으므로 최신 버전으로 업그레이드하는 것이 좋습니다. -
java.lang.IllegalArgumentException런타임 예외가 발생할 수 있기 때문에 환경 변수의 값을 확인하고 비어 있지 않은 경우에만 값을 가져와야 합니다. -
implementation "com.infobip:infobip-spring-data-jpa-querydsl-boot-starter:8.0.0"같은 QueryDSL과 Spring Data JPA를 지원하는 외부 라이브러리가 추가됐지만, 해당 라이브러리를 사용하기 위한 필수 의존성인querydsl-apt스코프와kotlin("kapt")플러그인이 빠져 있으면 컴파일 오류가 발생합니다. 따라서 이 부분을 수정해야 합니다. -
idea플러그인을 사용하여 IntelliJ IDEA를 사용하는 경우에만 적절하며, 개발자 도구를 이용하는 모든 프로그래머에게 해당하지 않습니다.
| return JPAQueryFactory(entityManager) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
위 코드는 QueryDSL을 사용하여 JPA를 쿼리하는 코드입니다.
-
코드 구조:
QueryDslConfig클래스는 Spring의 Configuration 어노테이션을 통해 스프링 구성 클래스임을 나타내고 있습니다.jpaQueryFactory()메소드는EntityManager를 파라미터로 받아서JPAQueryFactory객체를 반환해줍니다. 이 객체는 QueryDSL에서 제공하는 쿼리 빌더(Query Builder)인com.querydsl.jpa.impl.JPAQueryFactory의 인스턴스 입니다. -
@PersistenceContext어노테이션:entityManager필드에 자동으로 값이 주입됩니다. 이 값은Jakarta Persistence API의 EntityManager 객체를 참조합니다. -
장점: QueryDSL은 코드 가독성이 좋고 유지보수하기 쉬운 SQL처럼 보이는 JPA 쿼리 작성을 도와줍니다.
QueryDslConfig클래스를 사용하면 EntityManager를 재사용할 수 있으므로 애플리케이션 전체에서 공유 할 수있는 종속성을 최소화하고 성능을 개선할 수 있습니다. -
권장 사항:
QueryDslConfig클래스에 대한 유닛 테스트 작성을 고려해보세요. 테스트를 작성함으로써 의도치 않은 동작을 피하고 올바른 결과를 제공하는지 확인 할 수 있습니다.
|
|
||
| override fun getAuthority(): String = userRole.toString() | ||
|
|
||
| } |
There was a problem hiding this comment.
위 코드 패치는 com.blueone.app.global.security 패키지 내부에서 UserRole 객체를 이용한 권한 부여 클래스를 정의합니다. 보안과 직관성 측면에서 약간의 개선 사항이 있을 수 있습니다.
개선 제안:
- 생성자 파라미터에
@NonNull어노테이션을 추가하여userRole인자가 null인 경우 발생할 수 있는 예외를 방지하도록 합니다. getAuthority()메소드의 구현에서toString()대신name()을 사용하여 명시적으로 열거형 이름 문자열을 반환하는 것이 좋습니다.
이 외에 더 많은 개선 사항이 가능할 수 있으나 코드 컨텍스트와 코드 베이스 크기 등을 고려하여 최소한의 개선을 제안해 드렸습니다.
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
위 코드 패치는 Spring Security UserDetails 인터페이스를 구현한 BlueOneUserDetails 클래스로서, User 엔티티에서 필요한 정보들을 가져와 생성 됩니다.
getAuthorities() 함수가 가장 주목할 만한 부분인데, 이 함수는 UserDetails 인터페이스를 위반하지 않기 위해 반드시 구현되어야 한다. 해당 함수는 Collection 을 반환하는데, 이 예제에서는 하나의 BlueOneAuthority 객체만 담겨있는 리스트를 반환합니다.
그 외에는 isAccountNonExpired(), isAccountNonLocked(), isCredentialsNonExpired() 함수들이 false 를 반환하여 비활성화 시킨 것으로 보입니다. 만약 이 함수들이 true 값을 반환하도록 하면 계정 유효성 검사 기능들이 활성화됩니다.
어쨌든 해당 코드는 특별한 문제점 없이 안전하게 사용될 수 있습니다. 다만, 불필요한 공간 낭비를 줄일 목적으로, var 대신 val을 사용한다면 더 나은 구현이 될 수 있는 점 등에 대해서는 개선이 가능합니다.
| return BlueOneUserDetails.from(user) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
이 코드 패치는 BlueOneUserDetailsService 라는 사용자 인증 관련 서비스 클래스입니다.
loadUserByUsername 메서드에서는 username으로 받은 값이 Long 형태로 변환되어 userRepository 에서 해당 사용자를 찾은 뒤 UserDetails 타입으로 반환됩니다. 만약 사용자가 존재하지 않으면 UserNotFoundException 예외가 발생합니다.
loadUserById 메서드도 유사한 기능을 수행하는데, 이 때는 userId 값으로 사용자를 찾습니다.
이 클래스의 큰 문제점은 보안상 해롭지 않은 예외 처리가 다소 과하다는 것입니다. 그리고 loadUserByUsername() 함수에서의 userId 변환이 잘못된 입력값에 대해 예외처리되지 않는 등 가용성과 안정성 면에서 개선할 여지가 있습니다.
또한 이 코드에서는 UserDetailsService를 상속하여 구현하고 있는데, 이 방법 대신 ReactiveUserDetailsService 요구사항에 맞게 인터페이스를 사용할 수도 있다는 점 참고 부탁드립니다.
개선 사항으로는 오류 발생 시 특정 로그 기록이나 알림 전송, 특정 사용자만 권한을 부여할 수 있는 처리 등이 있을 수 있습니다.
| return DelegatingPasswordEncoder(id, encoders) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
이 코드 패치에서는 스프링 시큐리티를 사용하는 앱의 보안 구성을 위한 클래스를 정의하고 있다.
코드적으로 현재 보안 필터 매핑 및 BCryptPasswordEncoder로 작성된 DelegatingPasswordEncoder가 포함되어 있다. Spring Security를 사용할 때 이것은 고급 추가 옵션에서 적용할 수 있는 것으로 보입니다.
또한, filterChain (http : HttpSecurity) 메소드는 HttpSecurity 빌더를 받아서 SecurityFilterChain을 반환합니다.
이 코드 패치는 이상이 없어보이며 향후 개선 사항을 찾을 수 있기 때문에 유지 관리 할 수있을 것입니다.
| @Temporal(TemporalType.TIMESTAMP) | ||
| private val endDate: Date, | ||
|
|
||
| ) { |
There was a problem hiding this comment.
해당 코드 패치에는 @Temporal 어노테이션이 제거되어 있는데, 이는 startDate와 endDate 프로퍼티가 Date 타입으로 선언되었기 때문입니다. 이 경우 해당 프로퍼티를 사용할 때마다 알맞은 포맷으로 변환해야 할 필요가 있습니다.
이러한 경우 java.time 패키지의 LocalDateTime 등의 타입을 사용하면 해당 타입들이 @Temporal 어노테이션을 대신하여 적절한 데이트타임 타입으로 매핑하므로 좀 더 편리합니다. 따라서 startDate와 endDate 프로퍼티를 LocalDateTime 타입으로 변경하는 것이 좋습니다.
| @Temporal(TemporalType.TIMESTAMP) | ||
| private val bookingDate: LocalDate, | ||
|
|
||
| @Embedded |
There was a problem hiding this comment.
이 코드 패치는 java.util.Date에 대한 참조를 제거하고 LocalDate를 사용하여 클래스 및 속성을 업데이트하는 것 같습니다. 따라서 이전에 발생했던 문제 중 하나는 더 이상 발생하지 않을 것입니다. 그러나 "@TeMPOraL" 애노테이션의 자바 표준 버전에 따라 부작용이 있을 수 있습니다.
간략한 개선 제안으로는 생성자 매개 변수를 비롯하여 클래스 필드에 대한 주석과 설명 추가가 유용할 수 있습니다. 그리고 모든 필요한 개별 getter/setter 메소드를 사용하여 접근할 수 있도록 합니다. 마지막으로 isPenalty 필드에 대한 약간의 정보를 제공한다면 더욱 좋을 것입니다.
| fun getRole(): UserRole = UserRoleConverter().serialize(this.role) | ||
| fun getEncryptedPassword(): String = this.password | ||
| fun getCreatedDate(): LocalDate = this.createUpdateDateSet.createdDate | ||
| fun getUpdatedDate(): LocalDate = this.createUpdateDateSet.updatedDate |
There was a problem hiding this comment.
위 코드 리뷰에 대한 제 생각은 다음과 같습니다.
- 수정된 코드가 매우 작아 보입니다. 변경사항은 필드에 Nullable 속성을 추가하고 getter 함수를 단일 표현식으로 변경하는 것입니다.
- 원래 코드와 비교하여, 코드 리베이스에서 생성될 빌드 버전, 프로젝트 목표 등등 고려해야 할 다른 요소는 없는 것 같습니다.
- 코드 리뷰어들이 짚어야 할 위험점이나 개선할 점 같은 것도 없어 보입니다.
따라서, 이 코드가 기존 시스템에 영향을 미치지 않으면서 정상적으로 작동할 가능성이 크기 때문에 전체적으로 순수한 리팩토링 역할에 충분한 것으로 볼 수 있습니다.
| return Optional.ofNullable(user) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
위 코드는 Spring Data JPA 프로젝트 내에 위치한 User 객체의 데이터 CRUD 작업 중 R(read)에 해당하는 부분을 담당하는 UserRepository 클래스입니다.
코드 리뷰 결과, 다음과 같은 사항이 포함됩니다:
- 특정 유저 ID를 이용하여 DB에서 유저 객체를 조회하는 기능(
findById())을 제공하고 있습니다. - Querydsl 라이브러리를 이용하여 쿼리를 작성하고 있습니다.
- 코드 상에서 별도의 버그나 잘못된 점을 발견하지는 못했습니다.
- 추가적으로,
Optional클래스를 이용하여 검색 결과 값이 null인 경우 예외를 발생시키지 않고 null 값을 반환하게 된다는 점에 유의해야 합니다. 이 부분은 클라이언트 코드 상에서 handling이 필요합니다.
만약 이 UserRepository 클래스를 사용하는 서비스 레이어에서 해당 메소드 호출 결과를 적절히 처리하지 않을 경우 NPE(NullPointerException) 등이 발생할 가능성이 있으니 주의가 필요합니다.
| level: | ||
| org.hibernate: | ||
| SQL: debug | ||
| type.descriptor.sql: trace |
There was a problem hiding this comment.
이 코드 패치는 스프링 JPA에 대한 설정을 변경하고, 커스텀 데이터 소스와 로깅 수준을 추가하는 것처럼 보입니다.
추가된 설정 중 일부는 다음과 같습니다.
hibernate.ddl-auto: hibernate 구조의 자동 생성logging.level: org.hibernate 패키지에서 SQL 구문을 디버그 레벨로 저장하도록 구성
이 코드 패치에 대한 버그 및 개선 제안은 대상 시스템에 대한 이해와 관련이 있습니다. 하지만, 가독성을 위해서 yaml 파일의 가독성을 보강하는 것이 개선될 수 있습니다. 예를 들어 등호(=) 사이와 변수명, 값을 지정하는 콜론(:) 사이에 공백을 넣어서 코드 가독성을 향상시킬 수 있습니다.
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
위 코드는 테스트 코드 입니다. 아래는 몇 가지 개선할 점과 버그 예측입니다:
개선할 점:
- @DisplayName 애노테이션을 사용하여 테스트 메소드의 의도를 보다 명확하게 드러낼 수 있습니다. (예: "get_test_user_by_load_user_by_username" --> "loadUserByUsername()에서 User 불러오기 성공")
- EntityManager 객체에 대한 설명이 없는데 필드에 대해 주석을 달아 정리할 수 있습니다.
- 엔티티 매니저 객체를 직접 인스턴스화하는 대신 Spring이 제공하는 @PersistenceContext 애노테이션을 사용하면 더 나은 어노테이션 구문을 사용할 수 있습니다.
버그 예측:
- @transactional 어노테이션을 추가해서 해당 클래스에 대한 모든 메서드가 트랜잭션 내에서 실행되도록 해야합니다. 그렇지 않으면 데이터베이스 일관성 문제가 발생할 수 있습니다.
- UserNotFoundException은 예상된 예외이며 확인되지 않은 경로로 예외가 발생하지 않도록 합니다.
- userId 변수는 null 값을 포함할 수 있으므로 안전한 호출을 사용해야 합니다. 예를 들어,
userId?.let { userDetailsService.loadUserByUsername(it) }입니다.
1 participant
사용자 불러오기 관련 기능 및 테스트 작성