jkde7721
commented
5 months ago 일단 코드 전반적으로 리뷰 남기겠습니다
p1
예외 처리 방법
22 해당 PR의 For Reviewer 부분에 저희 프로젝트에서의 예외 처리 방법에 대해 코멘트 달아두었습니다.
- 현재 서현님 구현에서는 각 예외마다 커스텀 예외 클래스를 정의하여 던져주고 있는데, 저는 이 경우 예외 클래스가 매우 많아져 관리에 어려움이 있으며, 해당 예외를 처리하기 위해 중복된 메소드가
GlobalExceptionHandler클래스에 추가될 것이라 생각하였습니다. 따라서BusinesException이라는 비즈니스 로직 관련 커스텀 예외를 정의하고 각 예외의 구분은ErrorResponseenum에 정의하였습니다. 그리고 Service 클래스 내에서는new BusinessException(ErrorCode.NOT_FOUND_USER_PROFILE)이런 형식으로 예외를 던지고 있습니다. - 서현님이 구현하신 방식이 각 예외 클래스만으로 예외를 구분할 수 있다는 장점도 있지만, #22 pr 코멘트에 따르면 상호 협의된 사항이라고 생각되어 빠른 수정 부탁드립니다. (+
GlobalExceptionHandler클래스 내 추가된 메소드들도 모두 삭제해주세요!)
현재 로그인한 유저의 ID를 얻는 방법
-
현재 게시글 작성 api를 보면
writerId를@RequestBody나@PathVariable로 프론트로부터 직접 받아오고 있는데, 현재JwtAuthenticationFilter에서 모든 api 요청 시마다 전달되는 JWT 토큰을 파싱하여SecurityContextHolder라는 일종의 저장 공간에 인증객체를 넣어주고 Controller 단에서는 현재 요청을 보낸 유저의 ID를@AuthUserId어노테이션을 통해 얻어올 수 있습니다.@RequiredArgsConstructor @RequestMapping("/api/v1/user") @RestController public class UserController { //... @GetMapping public ResponseEntity<UserProfileDetailsResponse> getUserProfileDetails(@AuthUserId String userId) { //여기 UserProfileDetailsData userProfileDetailsData = userService.getUserProfileDetails(userId); UserProfileDetailsResponse response = userMapper.toUserProfileDetailsResponse(userProfileDetailsData); return ResponseEntity.ok(response); } }
엔티티 내 createdAt, lastModifiedAt 필드
- Spring Data JPA는 엔티티의 생성시각, 수정시각을 자동으로 기록해주는 Auditing이라는 기능을 제공해줍니다. 참고
- 현재
BaseTimeEntity클래스에서 해당 필드 정의되어 있으며 각 엔티티에서는 단순히extends BaseTimeEntity하시면 됩니다. (기존User,UserProfile클래스 등 참고) - 따라서 게시글, 댓글 생성 시 요청 DTO에서
createdAt필드를 받아올 필요도 없습니다. - 모든 엔티티마다 중복된
createdAt,lastModifiedAt를 선언하는 것은 불필요한 것 같아 빠른 수정 부탁드립니다.
p2
Mapper 클래스의 의존 계층
@RequiredArgsConstructor
@RequestMapping("/api/v1/user")
@RestController
public class UserController {
private final UserService userService;
private final UserTokenService userTokenService;
private final UserEmailAuthService userEmailAuthService;
private final UserMapper userMapper;
//...
}현재 제 코드를 보시면 Controller 계층에서 Mapper 클래스를 의존하고 있습니다.
- 저는 Mapper 클래스를 도입한 이유가 서비스 계층에는 온전히 비즈니스 로직 만을 포함하기 위함이고, 이를 위해 웹 계층의 요청 또는 응답 DTO와 Service를 분리하고자 하였습니다. 따라서 Mapper 클래스가 엔티티 <-> DTO 간 매핑을 담당하고 있으며 Service는 해당 매핑에 대해서는 알지 못하는? 것이 좋다고 생각하여 Mapper를 Controller에 위치시켰습니다.
- 추가로 Controller -> Service -> Repository 같은 레이어드 아키텍처에서는 의존성 방향이 한 방향으로만 흘러야 변경에 따른 영향도가 적어집니다. 현재 구현해주신 코드에서는 business(Service) 계층이 전혀 무관한? web(Mapper) 계층을 의존하는 형태가 됩니다. 일단 저는 이러한 의견으로 Mapper 클래스를 Controller 계층에 두었는데 또 다른 의견 있으시면 코멘트 부탁드립니다.
@Setter의 사용
- 일반적으로 일관성을 해친다 등의 이유로 엔티티에는
@Setter의 사용을 지양합니다.@ModelAttribute의 경우@Setter어노테이션이 있어야지 요청 파라미터가 바인딩이 되어서 어쩔 수 없이@Setter가 필요한데, 해당 경우를 제외하고 엔티티, DTO에 선언된@Setter어노테이션 모두 제거 부탁드립니다. (+ Mapper 대상이 되는 클래스에도@Setter불필요합니다.) - 해당글 참고하시면 좋을 것 같습니다.
일부 엔티티 연관관계 매핑 부재
GeneralPost,Comment엔티티 등 작성자에 대한 연관관계 매핑 대신writerId필드만이 존재합니다. 작성자는User엔티티와 연관관계 가질 수 있습니다. (+ 혹시 연관관계를 맺지 않고writerId필드로 대체한 이유가 있으실까요?)
DTO 클래스의 Request, Response 분리
- 기존 코드를 보시면
web.dto.request,web.dto.response패키지를 구분하여 요청 dto, 응답 dto를 구분하고 있습니다. 클래스 네이밍 또한SignUpRequest,SignUpResponse와 같이 하고 있습니다. - 저는 요청 dto와 응답 dto가 현재는 같은 형태이더라도 변경 사항이 자주 발생할 것이기 때문에(웹 계층에 속해 프론트 단과 가장 밀접하게 맞닿아 있기 때문) 이 둘을 나누는 것이 맞다고 생각하는데 서현님 생각은 어떠신가요?
Service 클래스 레벨의 @Transactional 선언
- 게시글 조회 메소드의 경우 단순히 데이터를 조회만 해오기 때문에 굳이 write 트랜잭션을 설정해줄 필요가 없습니다.
- 데이터 일관성 및 성능 향상 등의 이점으로 조회만 발생하는 메소드에는
@Transactional(readOnly = true)선언으로 수정 부탁드립니다. 참고 - 기존의 Service 클래스 참고하시면 클래스 레벨에는
@Transactional(readOnly = true)어노테이션이, write 작업이 필요한 메소드(ex. 유저 정보 수정 등)에는 메소드 레벨에@Transactional어노테이션이 선언되어 있습니다. 참고하시어 수정 부탁드려요.
도메인이 다른 Repository 의존
- 현재
PostLikeFilterServiceImpl에서는 엄연히 다른 도메인인 user의UserRepository를 의존하고 있습니다. 하나의 도메인 엔티티, 레포지토리는 최대한 동일한 도메인의 서비스에서만 접근하도록 구현하는 것이 좋습니다. 각 도메인끼리는 서로간 블랙박스처럼 동작해야 한다고 생각하시면 좋을 것 같아요. 현재UserRepository를 의존한 이유가 해당userId의User엔티티를 조회해오기 위함인데, 해당 유저가 존재하지 않는 경우에 대한 예외 처리도PostLikeFilterServiceImpl클래스에서 해주고 있습니다. (클래스의 책임을 따지자면 게시글 도메인에서 유저 도메인에 대한 예외 처리를 해주고 있는 것은 적절치 않습니다.) - 기존
UserService클래스 보시면 다음과 같이 예외 처리를 해주면서User엔티티를 조회해오는 메소드가 이미 정의되어 있습니다. 즉UserRepository가 아닌UserService클래스 의존해서User엔티티 조회하도록 수정 부탁드려요.public User getUser(String userId) { return userRepository.findById(userId) .orElseThrow(() -> new BusinessException(NOT_FOUND_USER)); } - 해당글 참고하시면 좋을 것 같습니다. (+ 해당 글처럼 Service 클래스에서 엔티티가 아닌 별도의 DTO로 넘기도록 추후 수정해도 좋을 것 같습니다.)
p3
@Table, @Column 어노테이션의 name 속성 지정
- 현재 모든 엔티티에서
@Table,@Column어노테이션의name속성으로 테이블 이름, 컬럼 이름을 지정해주셨는데, 자바의 캐멀 케이스 형식의 필드 이름을 스프링은 스네이크 케이스의 컬럼 이름으로 자동 매핑해줍니다. 따라서 해당 코드들은 불필요한? 것이라 생각되는데 따로 지정해주신 이유가 있을까요? 합당한 이유가 있다면 일관성을 위해 제 코드 부분도 이렇게 수정하겠습니다.
@Getter
@Setter
@NoArgsConstructor
@AllArgsConstructor
@Builder
@Entity
@Table(name = "comment")
public class Comment {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@Column(name = "comment_id")
private Long commentId;
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "post_id", referencedColumnName = "post_id")
private GeneralPost postId; // 이 댓글이 속한 게시글 id
@Column(name = "writer_id")
private String writerId;
@Column(name = "content", columnDefinition = "TEXT")
private String content;
@Column(name = "created_at", columnDefinition = "TIMESTAMP")
private LocalDateTime createdAt;
@Column(name = "last_modified_at", columnDefinition = "TIMESTAMP")
private LocalDateTime lastModifiedAt;
@Column(name = "anonymous", columnDefinition = "TINYINT(1)")
private boolean anonymous;
@Column(name = "like_count", columnDefinition = "BIGINT")
private long likeCount;
}p4
코드 컨벤션 관련
- 위와 같이 브랜치 네이밍 컨벤션 정했습니다. 따라주시면 좋을 것 같습니다. (
feature/로 시작) - 테스트 코드 작성 커밋에는
testprefix 사용해주시면 좋을 것 같습니다.
p5
서비스 인터페이스 사용
현재 서현님 구현에서는 Service 인터페이스를 정의한 후 ServiceImpl을 구현하여 작성한 반면 저는 바로 Service 클래스를 사용하였습니다. 이거는 인터페이스로 구체적인 클래스에 의존하지 않도록하는 서현님 방법이 더 좋은 것 같아 제 코드를 수정하겠습니다.
게시글 및 댓글 작성 시 유저 role을 받아와 guest인 경우 custom exception 발생시키는 것도 곧 추가하도록 하겠습니다. => 이거는
SercurityConfig클래스 보시면securityFilterChain메소드 내에서 각 role에 따라 가능한 api 요청을 지정하고 있어 컨트롤러 단에서 별도의 작업을 하실 필요는 없습니다. 좀더 자세히 설명드리면SecurityFilter중ExceptionTranslationFilter에서 권한이 없는 api 요청을 한 경우AccessDeniedException예외를 발생시키며 해당 예외는AccessDeniedHandler의 구현체인JwtAuthorizationFailureHandler(우리 프로젝트에서 직접 정의)가 처리하고 있습니다.
기능 구현하시느라 수고 많으셨습니다...! 다만 기존 코드들을 조금더 살펴보고 코드를 작성하셨으면 좋았을 것 같다는 아쉬움은 조금 듭니다. 또한 pr 날려주신 코드 실행시켜 보면 Mapper 뿐만 아니라 다른 부분에서도 오류가 나고 있습니다. 다음 PR은 모든 api가 정상 동작하는 상태로 날려주세요. 제가 코멘트 남긴 부분에서 이해되지 않는 부분은 댓글 달아주시고 자세한 설명은 수요일날 전달드릴께요 추가로 구현하시면서 변경된 DB 테이블이나 API 스펙 모두 erdcloud, postman에도 적용 부탁드려요. 또한 포스트맨 내 게시글 api 관련 각 요청에 Add Example로 요청, 응답 JSON도 작성해주시면 좋을 것 같습니다.
gitguardian[bot]
💻 구현 내용
(24. 07. 10 추가)
public double calculateAverageGrade() { if (reviews == null || reviews.isEmpty()) { return 0.0; } return reviews.stream() .mapToDouble(MedicineReview::getGrade) .average() .orElse(0.0); }로 각 약에 대한 평균 별점을 계산해서 return 하는 메서드를 추가했습니다.UserPrivacy.java
@PostLoad private void calculateAgeRange() { if (this.birth != null) { int age = Period.between(this.birth, LocalDate.now()).getYears(); if (age < 20) { this.ageRange = "10대"; } else if (age < 30) { this.ageRange = "20대"; } else if (age < 40) { this.ageRange = "30대"; } else if (age < 50) { this.ageRange = "40대"; } else { this.ageRange = "50대 이상"; } } else { this.ageRange = "Unknown"; } }로 가입한 회원의 나이대를 계산하여 medicinereviewresponse에 표시되도록 구현했습니다.🛠️ 개발 오류 사항
테스트 코드 실행 시 dto를 entity로 변환하는 과정에서 객체에 null 값이 할당되어 제대로 매핑되지 않는 에러가 발생하고 있습니다.
GeneralPost post = GeneralPost.builder() .postId(1L) .writerId(2L) .categoryId(category) .writerName("John Doe") .categoryName("Tech") .title("How to Test MapStruct") .content("Content of the post") .anonymous(true) .images("image1.jpg") .likeCount(100) .commentCount(5) .scrapCount(10) .viewCount(1500) .tags("Java, Spring Boot, Testing") .createdAt(LocalDateTime.now()) .lastModifiedAt(LocalDateTime.now().plusHours(2)) .build();이렇게 post 객체를 생성해도,GeneralPostDto dto = postMapper.toDto(post);디버깅 결과 이 실행 코드 부분에서 dto에 null이 저장된다고 뜹니다.java.lang.IllegalStateException: @NotNull method org/jetbrains/plugins/scss/index/SassScssIndexedRootProvider.getAdditionalRootsToIndex must not return null라는 에러가 오늘 아침에 갑자기 뜨고 있습니다. 이를 해결하지 못하고 PR을 날리게 되었습니다. 관련하여 찾아보고 있습니다.🗣️ For 리뷰어
(24. 07. 10 추가)
chart나form_code_name에서 받아와야 합니다. 그래서drug_shape_code를 만들어 정제는 1, 연질캡슐은 2, 경질캡슐은 3으로 구분하여 필터링되게 하고자 하는데 괜찮을까요? (그런데 해당 작업은 약별로 일일히 진행해야 하고 + 약이 추가된다면 수작업으로 추가해야 하는 부분이긴 한데 이후에 크게 추가될 약이 없을 것 같아 괜찮을 것 같기도 합니다. 의견 남겨주시면 반영해서 구현해보겠습니다.)to-do