[그리디] 강동현 Spring JPA (2차) 4, 5, 6 단계 미션 제출합니다. #210
Conversation
There was a problem hiding this comment.
안녕하세요 동현님~ 로또 미션 > Spring MVC > Spring JPA로 자주 뵙게 되는군요 이번에도 잘부탁드립니다 🙇 🙃
리뷰는 우선 이번 PR 미션 내용 코드 기준으로 위에서부터 아래로 읽으면서 코멘트 10개를 남겼습니다~!
미션 실행시 코드에서 컴파일 에러가 나오는데요. 일반 class에서 record class로 이동하는 과정에서 코드 반영이 덜 된 것 같은데 확인 후 수정부탁드려요
아직 못본 코드가 많기도 하고, 덜 삭제 됐다거나, 개행이 많은 클래스 파일들도 있어서 한 번 정리해주시면 좋아보여요
정리의 경우 IntelliJ의 reformat code 기능을 사용하시면 됩니다~
위 사진처럼 패키지에서 오른쪽 클릭을 하면 reformat code가 있는데 이걸 사용하면 해당 패키지 안에 있는 모든 클래스 코드를 깔끔하게 만들어줘요
오른쪽 사진은 바로 Run을 누르면 되는데, 나머지 체크 안된 기능은 나중에 시간날 때 찾아보시면 됩니다. 참고로 저는 나머지 기능은 아무것도 몰라요 ㅋㅋ
=========
질문 드릴 점
저번 미션 리팩토링시에 토큰 만료 시간을 10분으로 리팩토링 … (내용 생략)
토큰 만료 시간을 10분으로 잡으신 이유가 무엇인가요??
9분, 11분은 아름다운 시간 값이 아니라서 그런거는 이해가 가긴하는데 😁, 5분이랑 or 15분, 20분도 있는데 굳이 10분인 이유가 따로 있는지 궁금해서요
내 예약 조회 시 ReservationRepository.findByMember_Id(...)에 연관 엔티티를 즉시 로딩하도록 @entitygraph를 적용했습니다! 해당 방식으로 예약 목록을 조회할 때 theme, time을 한 번에 가져와 각 예약마다 추가 쿼리가 발생하는 N+1 문제를 제거했습니다.
entitygraph에 대해서 찾아보셨다면 JPQL 같은 직접적인 쿼리문을 사용하는 것도 보셨을 것 같은데요 (아니면 코멘트 부탁드려요)
entitygraph를 사용한 이유가 무엇이고 사용해보니 어떤 효과가 있는지 궁금해요
- 물어보는 이유: 저는 entitygraph를 한 번도 사용해본적이 없습니답
| import java.util.NoSuchElementException; | ||
|
|
||
| @RestControllerAdvice | ||
| public class ExceptionController { |
There was a problem hiding this comment.
여기도 따로 패키지로 관리하면 좋아보이네요. 따로 패키지를 두지 않은 이유가 있나요?
- 이외에도
WebConfig,PageController도 마찬가지입니다
There was a problem hiding this comment.
리팩토링을 진행할 때 신경쓴 범위가 너무 좁았던 것 같습니다!
다만 지적해주신 것처럼 공용 관심사 성격이 명확한 클래스들이기 때문에, ExceptionController를roomescape.common.ExceptionController로 이동하였습니다!
| public enum ApiError { | ||
| BAD_REQUEST_INVALID_INPUT(HttpStatus.BAD_REQUEST, 40001, "잘못된 요청입니다."), | ||
| BAD_REQUEST_ILLEGAL_STATE(HttpStatus.BAD_REQUEST, 40002, "요청을 처리할 수 없습니다."), | ||
| NOT_FOUND_RESOURCE(HttpStatus.NOT_FOUND, 40401, "리소스를 찾을 수 없습니다."), | ||
| UNAUTHORIZED_MISSING_TOKEN(HttpStatus.UNAUTHORIZED, 40101, "토큰이 없습니다."), | ||
| UNAUTHORIZED_INVALID_TOKEN(HttpStatus.UNAUTHORIZED, 40102, "토큰이 유효하지 않습니다."), | ||
| UNAUTHORIZED_EXPIRED_TOKEN(HttpStatus.UNAUTHORIZED, 40103, "토큰이 만료되었습니다."), | ||
| FORBIDDEN_ADMIN_ONLY(HttpStatus.FORBIDDEN, 40301, "관리자 권한이 필요합니다."), | ||
| INTERNAL_SERVER_ERROR(HttpStatus.INTERNAL_SERVER_ERROR, 50000, "서버 오류가 발생했습니다."); |
There was a problem hiding this comment.
요거 보면서 궁금한 점이 있는데요. 이걸 사용하면 어떤 점이 장점으로 느껴지시나요?? 특히 아래의 장점이 무엇일지 궁금해요
- 프론트엔드에서 응답값으로 아래 message를 받았을 때의 장점
- code 값을 사용해서 얻는 장점
다른 클래스들도 맨 아래에 개행이 여러줄로 되어있던데 한줄로 삭제부탁드려요~
reformat code를 사용하면 정리될 것으로 보이네요
There was a problem hiding this comment.
-
사용자에게 바로 보여줄 수 있는 문구를 서버가 일관되게 내려줄 수 있습니다. 또 문구가 바뀌더라도 서버가 책임지고 수정할 수 있어서 프론트가 화면 문구를 따로 하드코딩하지 않아도 됩니다.
-
문구는 변경되기 쉬운데, code는 상대적으로 고정이라 프론트가 code로 안정적으로 분기할 수 있습니다! 예를 들어 401 code가 떴을 때 토큰 없음/토큰 만료/토큰 무효에 따라 각기 다를 처리를 요청할 수 있습니다!
| @Column(unique = true) | ||
| private String email; |
There was a problem hiding this comment.
요거 최근에 팀에서 이야기 나왔던 내용인데, 동현님의 생각도 궁금하네요
- 여기 필드에만
@Column(unique = true)만 설정한 이유가 있나요? or 다른 필드에@Column(unique = false)를 설정할 수도 있는데 설정하지 않은 이유가 있을까요? - 만약 전자를 택하는 사람과 후자를 택하는 사람이 같이 개발을 한다면 한쪽에 맞춰주는걸 규칙으로 정하는게 나을까요?
팀에서는 이런 이야기가 나왔었어요
- 어차피 기본값이
@Column(unique = false)인데, 굳이 적어둘 필요가 없어보임 - 기본값 자료형이 boolean이라 true or false 라고는 해도, 결국엔 내가 이런 옵션이 있다는걸 알아야하기 때문에 적어두는게 좋음
- 개발할 때 중요한 사항은 아니므로 각자 스타일대로 코딩해도 문제 없음
- 코드에도 통일성이 중요하기 때문에 만약 따로따로 스타일을 가져가데 된다면 신규입사자들이 우리팀 코드를 보면 혼란스러워할 수도 있음
There was a problem hiding this comment.
-
이메일을 회원의 유일한 식별값으로 보고, 중복이 일어나지 않아야 하기 때문에 unique = true를 적용하였습니다! Column(unique = true) 는 제약사항이기 때문에, 제약이 걸리지 않는 부분보다는 제한이 걸리는 부분만 표기하는 것이 맞다고 판단했습니다!
-
저는 한쪽으로 맞추는 규칙을 정하는 게 낫다고 생각합니다!! 기능적으로는 상관없지만, 스타일이 섞이면 같은 의미의 코드가 파일마다 다르게 보이는 문제가 발생할 것이라 생각합니다!
| MemberResponse member = memberService.createMember(memberRequest); | ||
| return ResponseEntity.created(URI.create("/members/" + member.getId())).body(member); | ||
| public ResponseEntity createMember(@RequestBody MemberRequestDto memberRequest) { | ||
| MemberResponseDto member = memberService.createMember(memberRequest); |
There was a problem hiding this comment.
요거는 MemberResponse 대신 MemberResponseDto로 변경한 이유가 있나요?!
There was a problem hiding this comment.
Dto 가 명시되어야 하기 때문입니다! 제가 처음부터 구조를 짰다면 dto 폴더를 별도로 만들었을 텐데, 기존의 코드에 덧대어 코드를 작성하면서 dto의 이름을 MemberResponse고 하고 member에 들어가는 건 부자연스럽다고 생각했스빈다!
| public static Cookie createAuthCookie(String token, int maxAgeSeconds, boolean secure) { | ||
| Cookie cookie = new Cookie("token", token); | ||
| cookie.setHttpOnly(true); | ||
| cookie.setPath("/"); | ||
| cookie.setMaxAge(maxAgeSeconds); | ||
| cookie.setSecure(secure); | ||
| return cookie; | ||
| } | ||
|
|
||
| public static Cookie createExpiredAuthCookie() { | ||
| Cookie cookie = new Cookie("token", ""); | ||
| cookie.setHttpOnly(true); | ||
| cookie.setPath("/"); | ||
| cookie.setMaxAge(0); | ||
| return cookie; | ||
| } |
There was a problem hiding this comment.
쿠키를 생성하고 만료 시키는 로직이 현재는 JWT 밖에 없지만, 요게 JwtUtil에 들어갈 JWT 만을 위한 로직인가?를 생각해보면 그건 아닌 것 같은데 어떻게 생각하시나요 ?_?
그래서 저는 메서드 네이밍도 살짝 수정이 필요하다고 생각해요
There was a problem hiding this comment.
말씀해주신 부분에 동의합니다! 쿠키 생성/만료 로직은 JWT 전용 책임으로 보기 어렵다고 판단해 용도와 성격이 드러나도록 리팩토링하였습니다!
쿠키 자체의 역할이 더 잘 드러나도록, 쿠키 관련 로직을 CookieUtil로 분리하고 네이밍도 일반화했습니다. createHttpOnlyCookie, expireCookie처럼 메서드 이름만 봐도 “JWT가 아니라 쿠키를 다루는 기능”이라는 점이 바로 보이도록 수정했습니다.
| } | ||
| } | ||
| } |
| @Getter | ||
| public class MemberResponse { |
There was a problem hiding this comment.
Request, Response 모두 사용하지 않고 있는데 파일을 남겨둔 이유가 있나요??
There was a problem hiding this comment.
해당 파일은 삭제 후 리팩토링 완료했습니다만, 캐시가 잘못된건지 삭제 내역이 깃에 들어가지 않은 것 같습니다..
해결하겠습니다!
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
||
| @Service | ||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
클래스 위에 @Transactional을 붙일 수도 있는데, @Transactional(readOnly = true)를 선택한 이유가 궁금해요~
There was a problem hiding this comment.
조회용 메서드에 대해서는 (readOnly = true)를 붙이는 것이 성능향상에 좋다고 배웠습니다!
원리에 대해서는 자세히 찾아보지는 않았습니다만, 찾아보니 영속성 컨텍스트에서 수행하는 변경 감지(Dirty Checking) 때문이있군요!
readOnly = true를 설정하게 되면 스프링 프레임워크는 JPA의 세션 플러시 모드를 MANUAL로 설정하여 트랜잭션 내에서 강제로 flush()를 호출하지 않는 한, 수정 내역에 대해 DB에 적용되지 않게 되고 변경 감지를 위한 Snapshot을 따로 보관하지 않으므로 메모리를 절약할 수 있게 됩니다!
| public MemberResponseDto createMember(MemberRequestDto memberRequest) { | ||
| Member member = memberRepository.save(new Member(memberRequest.name(), memberRequest.email(), memberRequest.password(), Role.USER)); | ||
| return new MemberResponseDto(member.getId(), member.getName(), member.getEmail()); | ||
| } |
There was a problem hiding this comment.
| public MemberResponseDto createMember(MemberRequestDto memberRequest) { | |
| Member member = memberRepository.save(new Member(memberRequest.name(), memberRequest.email(), memberRequest.password(), Role.USER)); | |
| return new MemberResponseDto(member.getId(), member.getName(), member.getEmail()); | |
| } | |
| public MemberResponseDto createMember(MemberRequestDto memberRequest) { | |
| Member newMember = new Member(memberRequest.name(), memberRequest.email(), memberRequest.password(), Role.USER); | |
| Member member = memberRepository.save(newMember); | |
| return new MemberResponseDto(member.getId(), member.getName(), member.getEmail()); | |
| } |
- newMember라는게 좋은 네이밍은 아닐 수도 있긴한데, 코드를 읽기 편하게 만들기 위해서는 긴 호흡으로 읽게하지 않도록 만드는게 중요한 것 같아요
MemberResponseDto는 자바 미션에서 많이 사용했던 정적 팩토리 메서드를 적극적으로 사용해보시면 좋아보여요~
There was a problem hiding this comment.
좋은 코드에 대해 적용할 생각을 못했던 것 같습니다..
수정 완료했습니다!
+)newMember도 좋지만 저장을 위한 member라는 의미에서 memberToSave라는 네이밍을 사용하였습니다!
|
|
||
| import jakarta.validation.constraints.NotNull; | ||
|
|
||
| public record ReservationRequestDto(String name, @NotNull String date, @NotNull Long theme, @NotNull Long time) {} |
There was a problem hiding this comment.
다른 dto에도 validation을 적용하는것이 좋습니다! MemberRequestDto에도 validation을 적용하였습니다!
|
run 될 수 있도록 리팩토링 완료했습니다!
10분으로 리팩토링한 이유10분을 결정할 때 깊은 생각을 하지는 않았습니다만, 사용자가 방탈출 예약을 진행할 때 걸리는 시간은 넉넉잡아 5~10분이면 충분하다고 생각했고, 보안 노출 시간 최소화를 생각해 10분으로 설정했습니다!
기존 방식처럼 JPQL +lazy로딩 방식으로 직접적인 쿼리문을 사용했었을 때는 쿼리 문자열을 직접 작성하고 관리해야 해서 조회 조건이나 조인 대상이 바뀔 때마다 쿼리를 함께 수정해야 한다는 부담이 있었습니다. 또한 각 엔티티마다 추가 쿼리가 나가서 n+1 문제가 발생하였습니다! 그러다가 다른 스터디원(고은님)의 리뷰를 보고서 entity그래프를 적용하였습니다! EntityGraph는 기존에 사용하던 Repository의 파생 메서드를 그대로 유지한 상태에서, 어노테이션 한 줄로 “어떤 연관 엔티티를 함께 조회할지”만 선언적으로 지정할 수 있었습니다. 이 방식은 쿼리 로직이 문자열 형태로 흩어지지 않고, 로딩 전략에 대한 의도가 메서드 선언부에 드러나기 때문에 코드를 읽는 입장에서 이해하기도 쉽다고 느꼈습니다! |
2 participants
다빈님 안녕하세요! 스프링 jpa 4, 5, 6 단계 2차 미션 제출하겠습니다!
이번 미션은 순수 JPA 기반으로 구현한 로직에 Spring Data JPA를 마이그레이션하는 미션이었습니다.
그러나, 저는 4,5,6단계 1차 미션의 요구사항을 잘못보고, JPA 기반으로 구현 후 Spring Data JPA를 1단계에 적용하였습니다!
따라서 해당 미션에서는, 지난 미션에서 개인적으로 미흡하다고 느꼈던, 완성도가 높은 코드를 만드는 것에 집중하는 걸 대신 미션을 진행하였습니다.
구현 내용
토큰 만료 시간 공통 유틸로 관리
저번 미션 리팩토링시에 토큰 만료 시간을 10분으로 리팩토링했는데, 추가로 쿠키 생성과 만료 처리를 공통 유틸로 분리했습니다. 로그인과 로그아웃 시 동일한 방식으로 HttpOnly, Path, Max-Age 옵션이 적용되도록 정리했습니다. 또한 토큰 만료 상황을 명확히 구분하기 위해 만료 전용 인증 에러 코드를 추가했습니다.
에러 처리방식 정리
전역 예외 처리기(ExceptionController)를 이용하여 모든 에러 응답을 동일한 JSON 형식으로 반환하도록 통일했고, 입력 오류, 상태 오류, 리소스 존재X, 서버 오류를 각각 서로다른 에러 코드로 매핑했습니다.
N+1 문제 예방
내 예약 조회 시 ReservationRepository.findByMember_Id(...)에 연관 엔티티를 즉시 로딩하도록 @entitygraph를 적용했습니다! 해당 방식으로 예약 목록을 조회할 때 theme, time을 한 번에 가져와 각 예약마다 추가 쿼리가 발생하는 N+1 문제를 제거했습니다.
잘부탁드립니다!!