-
Notifications
You must be signed in to change notification settings - Fork 7
[Spring JPA] Piki 2주차 미션 제출합니다. #13
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
base: piki
Are you sure you want to change the base?
Conversation
- Member, Time, Reservation 도메인에 대해 JpaRepository 기반 전환 - Time 및 Theme Entity에 deleted 필드 추가 후 findByDeletedFalse() 적용 - schema.sql 충돌 방지를 위해 spring.sql.init.mode=never 설정 - 기존 서비스 로직을 Repository 기반으로 리팩토링하면서 API 기능 유지
++) 5단계테스트코드 추가 및 4단계테스트설정파일 JpaTest로 변경
- 쿠키에서 'token'이 없거나 비어 있을 경우 UnauthenticatedException을 발생시킴 - ArgumentResolver에서 이메일 미조회 시 MemberNotFoundException을 사용하여 명확한 예외 분기 처리 - 보안 흐름의 일관성을 위해 불명확한 return null 제거
- @LoginMember 어노테이션 + ArgumentResolver 도입으로 인증 사용자 주입 자동화 - 사용자 본인 예약 조회 응답용 MyReservationResponse DTO 추가 - ReservationService의 save 로직을 어드민/사용자 분기 처리 구조로 개선 - Reservation 엔티티에 Member 연관관계 추가 (JPA 기반) - application.properties 설정 및 data.sql 초기값 정비
- 예약자는 로그인 정보 기반 예약 생성, 관리자는 name 명시로 예약 생성 가능 (기존 기능 유지-2단계) - 예약 대기 목록 조회 기능 구현: /reservations-mine GET 시 통합 응답 - MyReservationResponse: 테스트 내 `getId()` 호출을 위해 record → 일반 클래스로 변경 - Reservation 생성자 중 `Reservation(Long id,...)` 제거, `Reservation()` → protected 처리 → JPA 프록시 생성을 위한 기본 생성자 제한 (외부 사용 차단 목적)
- IntelliJ 설정에서 `Ensure every saved file ends with a line break` 옵션 활성화 완료
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저번에 코멘트 남겼던 사항은 추가로 안남겼어요. 반영해서 재리뷰요청해주세요~
private String password; | ||
private String role; | ||
|
||
protected Member() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 빈 생성자는 왜 있어야 할까요?
접근지정자를 protected로 하신 이유가 궁금해요!
이 생성자도 lombok을 이용해 만들 수 있답니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JPA가 엔티티객체를 db에서 조회할때, 내부적으로 리플렉션을 사용해서 객체를 생성하는 데, 이 과정에서 기본생성자가 필요합니다. private으로 설정한 이유는 같은 패지키안이나 상속관계에서만 객체를 생성하도록 제한하여 객체일관성을 유지하고 외부에서의 기본 생성자 호출을 막을 수 있기때문입니다.
lombok을 이용해 만들 수 있는건 추가로 공부해봤더니 @NoArgsConstructor 로 설정할 수 있네요! 수정했습니다:)
@ManyToOne(fetch = FetchType.LAZY) | ||
private Member member; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch type을 LAZY로 설정하신 이유가 궁금해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reservation → Theme, Time 관계는 항상 필요한 정보는 아니고, 예약 정보를 조회할 때만 사용되므로 기본 fetch 전략을 LAZY로 설정했습니다.
EAGER로 설정할 경우 사용하지 않는 상황에서도 불필요하게 theme, time 테이블에 접근하게 되어 성능에 영향을 줄 수 있고, 예상치 못한 N+1 문제가 발생할 수 있다고 판단했습니다.
대신 필요한 경우에는 EntityGraph로 명시적으로 fetch join을 적용하여 필요한 데이터만 로딩하도록 제어했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 난슬님 코드를 보면 응답 dto를 만들 때 결국 Time이나 Theme의 값이 필요하기 때문에 추가 쿼리가 발생하고 있어요
(ex. 내 예약 보기 api)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 리뷰 주신 것처럼 DTO 변환 시 theme, time 정보를 사용하는 구조여서, 반복적으로 쿼리가 발생하며 N+1 문제가 생기고 있었네요..@entitygraph를 적용한 부분도 있고 아닌 부분도 있어서 발생한 것 같습니다. 추가적으로 점검하면서 waiting에서도 n+1문제가 일어나는 것을 발견하고 수정완료했습니다.
private Long id; | ||
|
||
private String name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
member를 이미 가지고 있는데 member의 name을 굳이 필드로 가지고 있을 필요가 있을까요?
db 정규화에 대해 학습해보시면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 name이 이미 member에 있는데 reservation에 저장하는건 중복이라 정규화 원칙(3차 정규형)을 위반하겠군요.
해당 필드는 제거하고, 필요한 경우 member.getName()으로 접근하도록 수정하겠습니다. 정규화 개념 짚어주셔서 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한가지 질문이 있습니다!
요구사항에는 따로 명시되어있지않지만, 회원이 탈퇴할 때 예약내역에서 이름은 그대로 유지되어야한다고했을 때, 이름을 snapshot형태로 저장하는 반정규화를 의도적으로 사용하는 방법이 필요하지않을까용..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇게 할 수도 있고, soft delete 방식 (deleted 필드를 두고 삭제 마킹을 하는 방식)을 사용하는 경우가 많아요. 저도 실무를 안 가봐서 잘은 모르지만 실무에서는 이 방식을 많이 사용한다고 알고 있습니다!
List<Reservation> findByMemberId(Long memberId); | ||
|
||
@NotNull | ||
@EntityGraph(attributePaths = {"theme", "time"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EntityGraph
는 어떤 기능을 제공하나요? 이걸 사용하신 이유가 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@entitygraph는 특정 연관 관계를 조회 시에 미리 함께 가져오도록 JPA에게 명시적으로 지시하는 기능입니다.
Reservation 목록을 조회할 때 theme과 time 정보를 함께 사용하므로, 매 반복마다 LAZY 로딩이 발생하지 않도록 EntityGraph를 사용해 한 번의 쿼리로 미리 로딩하도록 설정했습니다.
이렇게 하면 쿼리 수가 대폭 줄어들고, N+1 문제도 방지할 수 있어 실제로 성능상 이점이 있습니다.
} | ||
@Setter | ||
@Column(name = "time_value", nullable = false) | ||
private String time_value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java에서는 변수명에 스네이크케이스가 아니라 카멜케이스를 사용하는 것이 컨벤션이에요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 그렇군요 sql로 매핑할때만 스네이크케이스를 쓰고 자바에서는 카멜케이스를 사용하는 방향으로 수정했습니다!
Reservation reservation; | ||
if (request.getName() != null && !request.getName().isBlank()) { | ||
reservation = new Reservation(request.getName(), request.getDate(), time, theme); | ||
} else { | ||
reservation = new Reservation(member, request.getDate(), time, theme); | ||
} | ||
reservationRepository.save(reservation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`new Reservation(request.getName(), request.getDate(), time, theme)으로 생성한 예약을 저장하면 member 연관관계를 맺지 못할 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReservationRequest에 name이 있는 경우에도 member를 조회하고 있었지만, Reservation 객체 생성 시 name만 넘기고 member 연관관계를 맺지 않아 연관 엔티티가 null로 저장되는 문제가 발생할 수 있겠네요.
Reservation 엔티티에서 name 필드를 제거하고, 항상 Member 연관관계를 맺는 방식으로 리팩터링했습니다.
Reservation 생성자는 Member를 기반으로 생성하도록 통일하여, ReservationRequest에 name이 있을 경우에도 Member를 조회해 연관을 맺도록 수정했습니다. 또, Reservation 생성 시 불필요한 if-else 분기를 제거했습니다. (Member 조회는 여전히 요청 데이터(name) 유무에 따라 분기함)
이를 통해 정규화 원칙을 준수하고, 요구사항인 “예약 시 항상 Member와 연결되도록” 정확히 만족시키도록 반영했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map(MyReservationResponse::fromWaiting) | ||
.toList(); | ||
|
||
return Stream.concat(reservations.stream(), waitings.stream()).toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concat을 사용하는 것보다 List< MyReservationResponse>에 reservations와 waitings를 addAll 해주는 것이 더 가독성이 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 리스트에 바로 추가하는 방식이라 더 직관적이고 concat처럼 내부적으로 스트림을 생성하고 병합하는 비용이 발생하지않아서 메모리, 연산비용면에서 둘다 가볍다고 나오네용 바로 반영했습니다:)
@ManyToOne(fetch = FetchType.LAZY) | ||
private Time time; | ||
|
||
private LocalDateTime createdAt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 어디서 할당해주나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 원래 대기 순번 계산하고 정렬할때 쓰려고 waiting에 할당해주려했는데 waiting id로 rank를 결정하는 로직으로 변경되어서 삭제해도 되는 필드입니다. 깜빡하고 삭제를 못했네요. 바로 삭제하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
푸시된 코드에 삭제가 안 되어 있어요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아이코 죄송해요 바로 했습니다
참고용 관련 커밋
spring.jpa.show-sql=true | ||
spring.jpa.properties.hibernate.format_sql=true | ||
spring.jpa.ddl-auto=create-drop | ||
spring.sql.init.mode=always | ||
spring.jpa.defer-datasource-initialization=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 각 설정값들은 어떤 것을 의미할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
spring.jpa.show-sql=true
jpa가 실행하는 sql쿼리를 콘솔에 출력함으로써 디버깅할 때 JPA가 예상한대로 쿼리가 나가는지 확인할 수 있습니다. 운영환경에서는 true로 두면 성능+보안 문제가 있을 수 있습니다.(application-dev.yml, application-prod.yml 이런식으로 따로 분리해야함!!) -
spring.jpa.properties.hibernate.format_sql=true
출력되는 sql을 줄바꿈+들여쓰기로 읽기좋게 format해줌으로써 추가적인 정렬을 설정할 수 있습니다. -
spring.jpa.ddl-auto=create-drop
시작할때 db테이블을 생성하고 종료할때 테이블을 모두 삭제하는 역할. 테스트나 개발 중에는 매번 새로운 테이블 구조로 깔끔하게 시작할 수 있고, JPA 엔티티 수정 후 재실행했을 때, 테이블을 자동반영할 때 유용합니다. 다만. 실운영환경에서는 데이터 무결성이 붕괴되기때문에 사용하지않습니다. -
spring.sql.init.mode=always
schemal.sql
이나data.sql
같은 초기화 스크립트를 항상 실행하려는 설정으로 서버가 시작할 때 초기데이터를 자동으로 채워넣어줍니다. -
spring.jpa.defer-datasource-initialization=true
jpa가 테이블을 먼저 생성한 후에data.sql
을 실행하도록 순서를 조정해주는 옵션입니다. create-drop으로 db가 처음부터 새로 만들어지니까 테이블이 아직 없을때 data.sql이 먼저 실행되면 오류가 발생하기때문에 jpa가 테이블을 다 만든 후에 실행되도록 순서를 조정해줘야합니다.
public class Theme { | ||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GenerationType으로는 어떤 것들이 있을까요? 그 중 IDENTITY를 사용하기로 한 이유는 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
AUTO
jpa가 알아서 db에 맞게 자동 선택
ex) MySQL은 IDENTITY, Oralcle은 SEQUENCE -
IDENTITY
db의 auto_increment기능을 이용
insert할때 db가 자동으로 id 생성하며 직접 제어가 어려움 (insert이후에 id가 확정됨) -
SEQUENCE
db 시퀀스 객체를 이용
Oralce, PostgreSQL 등 시퀸스 지원하는 db에 사용되며 시퀸스 이름을 지정할 수 있음 -
TABLE
별도의 키 생성용 테이블을 만들어 id를 관리
느리지만 db 독립성을 확보가능 (그치만 느려서 실무에서는 잘 안씀)
IDENTITY을 사용하기로 한 이유는 일단 MySQL에서 가장 많이 쓰는 전략이고, 애플리케이션 레벨에서 id 관리를 신경 쓸 필요가 없기 떄문입니다. 또한, AUTO를 쓴다면 db를 변경했을 때 문제가 발생할 수 있기때문입니다.
- `MemberRepository`를 통해 Optional<Member> 조회 후 `ReservationService`에서 직접 orElseThrow로 검증 - 이름 또는 이메일 기반으로 회원 조회 시 `MemberNotFoundException` 커스텀 예외 던지는 로직 유지시킴 - Repository는 DB 접근만 담당하고, 비즈니스 검증 책임은 Service 계층에서 분리하여 처리 (추후 find하는게 다른 서비스에서도 쓰인다면 파일분리 고려할 것)
- 참고로 DB는 snake_case를 써야한다. - java엔티티에서 매핑할때만 @column(name = "time_value")으로 하면됨
- application.properties의 prefix 불일치로 expire-length 설정이 반영되지 않는 문제 수정 - roomescape.auth.jwt.expire-length로 키 통일하여 정상 매핑되도록 수정
- WaitingResponse에 waitingNumber 필드 추가 - WaitingRepository에 대기 순번 계산용 count 쿼리 추가 - ReservationService.createWaiting에서 순번 계산하여 반환 - findMyReservations에서 Stream.concat 대신 addAll 사용하여 가독성 개선 - findMyReservations에 @transactional(readOnly = true) 추가하여 조회 최적화
6aa6824
to
c9dfead
Compare
- Reservation 엔티티에서 name 필드 제거 (Member 연관관계로 대체) - Reservation 생성자를 Member 기반으로 통일하여 일관성 확보 - save 메서드에서 Reservation 생성 시 불필요한 if-else 분기 제거 - 요청에 name이 있는 경우와 없는 경우 모두 Member를 조회한 뒤, Member를 기반으로 예약 생성 - 정규화 원칙 준수 및 예약 생성 로직 안정성 향
- setter가 없고 순수 데이터 전달용인 DTO 클래스들을 Java record로 리팩토링 - lombok 어노테이션 제거하여 의존성 감소 - record 특성을 활용해 생성자, getter, equals, hashCode, toString 자동 생성
1주차 리뷰까지 반영완료했습니다. 1주차 pr에 질문해주셨던 코멘트에도 답글 남겼어요! 확인부탁드립니다ㅎㅎ |
예약 대기 취소가 안 되는데 확인 부탁드려요~ |
- ReservationRepository.findAll()에 @entitygraph(theme, time, member) 적용 - findByMemberId() 및 WaitingRepository에도 @entitygraph 추가 - DTO에 필요한 연관 엔티티를 명시적으로 fetch하여 Lazy 로딩 시 쿼리 중복 방지 - 순번 계산 쿼리는 단건 호출로 유지하여 성능 저하 없음
- 예약대기 ID필드를 프론트와 통일시키기 위해 MyReservationResponse의 id 필드에 @JsonProperty("id") 적용
테스트코드를 넘기는데에만 집중하다가 구현을 깜빡했네요. 죄송합니다ㅠㅡㅠ 바로 구현완료했고 잘 동작하는 것 확인했습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다~
리뷰한 내용 반영 + 테스트코드 추가 해서 5월 4일 일요일 오후 2시까지 재리뷰요청 해주세요.
@@ -45,7 +45,6 @@ public String getRole(String token) { | |||
private Claims getClaims(String token) { | |||
return Jwts.parser() | |||
.setSigningKey(key) | |||
.setAllowedClockSkewSeconds(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createToken 부분에 리뷰를 달고 싶은데 이번 pr 변경사항이 아니라 선택을 못해서 여기에 달아요.
지금 보면 email, name, role을 사용해서 토큰을 만들고 있는데, 지금 난슬님 코드 상으로는 동일한 이메일, 이름, 역할로 두 개 이상의 Member가 생길 수 있어요. 이러면 어떻게 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 지적 감사합니다! 동일한 email, name, role을 가진 사용자가 중복될 경우 토큰이 충돌될 수 있습니다.
Member의 고유 식별자인 id 값을 추가 claim에 포함시키는 방향으로 수정완료했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하나 질문사항이 있습니다!
공부해보니까 토큰충돌가능성 = 같은 정보로 생성된 토큰이 동일해질 수 있다 인것같은데, name은 동명이인이 있는걸 감안하더라도 email, name, role이 다 같으면 id로 구분하기보다는 이미 존재하는 사용자다라고 예외를 던져야 하지 않을까요..?
근본적인 문제인 중복된 사용자가 가입하는걸 방지하기 위해 회원가입 로직에 예외를 던지는 걸 추가해볼 예정입니다.
다만, 그럼 claim에 추가한 목적이 조금 헷갈려서요. 토큰충돌가능성보다는 토큰 디코딩만으로는 member의 Id를 바로 알 수 없고, 후속 요청 처리에서 추가적인 조회를 해야하는 불편함때문에 토큰 자체만으로도 사용자 식별이 가능하도록하는게 더 명확한 목적이 아닐까 생각이 듭니다.
@Transactional | ||
public ReservationResponse save(ReservationRequest request, LoginMember loginMember) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6단계 요구사항인 중복 예약이 불가능 하도록 구현하세요.
가 구현이 안 되어 있네요 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
으앙ㅜㅜ 아진짜 죄송해요.. 테스트코드의 부재가 이리 위험하네요.. 리뷰하고 매니징을 해본 입장에서 요구사항 누락된거 체크하는게 얼마나 불편한지 알아서 더 죄송한 마음입니다.. 얼렁 고치고 테스트 코드도 추가하겠습니다...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프론트에서는 이미 예약된 경우 버튼을 비활성화 처리하고 있지만, 멀티 유저 환경이나 요청 위조 등을 고려하여 백엔드 차원에서도 중복 예약 방지 로직을 추가했습니다.
Service 에 중복 예약 체크 로직을 명시적으로 삽입하여, 같은 사용자가 같은 시간 & 테마에 대해 예약을 2번 시도하는 경우 DuplicateException
을 발생시키도록 했습니다.
createWaiting
내부에도 동일한 체크 로직을 적용하였습니다. 예약과 예약대기를 완전히 독립적으로 볼 수도 있었지만, 유저 경험상 하나의 타임에는 예약 or 예약대기 중 하나만 가능하도록 제약을 두는 편이 더 자연스럽다고 판단했습니다. 이에 이미 예약된 사용자가 예약대기를 등록하는 경우도 함께 방지하기 위해 validateNotDuplicateReservation
도 함께 호출합니다.
+) 예외 메시지는 RESERVATION_ALREADY_EXISTS
를 재사용하고, 구분이 필요할 경우 WAITING_ALREADY_EXISTS
로 분리했습니다.
public class ThemeController { | ||
private final ThemeDao themeDao; | ||
private final ThemeRepository themeRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controller에서 바로 repository를 참조하고 있네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme에 한해서는 단순히 CRUD 기능만 있고 복잡한 비즈니스로직이 없어서 controller에서 repository를 직접 호출해도 문제가 발생할 가능성이 낮다고 판단했습니다. (지금 보니 정말 오만한 생각이었던 것 같아요)
그러나, 다른 reservation, member 등 대부분의 도메인에서 service 계층을 이용하고있기때문에 theme만 사용하지않으면 오히려 불균형이 생길 수 있겠네요.. 향후 확장성 & 테스트 용이성 & 책임면에서 service로 분리했습니다.
public void deleteById(Long id) { | ||
timeDao.deleteById(id); | ||
timeRepository.deleteById(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time에 soft delete 방식을 적용하려고 deleted 필드를 두신 것 같은데 정작 삭제할 때는 그냥 삭제해버리네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아이코 삭제 로직에서는 단순 deleteById()만 사용했네요. 지적해주신 내용에 따라 실제 삭제 대신 deleted 값을 true로 바꾸는 방식으로 수정했고, findByDeletedFalse() 메서드를 사용해 소프트 삭제된 데이터가 노출되지 않도록 처리했습니다.
} | ||
|
||
public Time() { | ||
@Setter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setter를 두는 것보다 Theme에서처럼 객체에게 메시지를 보내는 방식 (의미가 있는 메서드명 사용)이 더 좋아 보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 말씀해주신대로 setter 대신 의미 있는 도메인 메서드를 사용하는 방식이 더 명확하고 안전하다고 판단되어, setter를 제거함으로써 외부에서의 불필요한 상태 변경을 막고, 도메인의 일관성을 높이는 방향으로 수정했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위 리뷰와 함께 time에 대해 전체적으로 고친 커밋입니다.
- 직접 생성자를 호출하게 되면 스프링 컨테이너 관리 대상이 아니게 되어 유지보수면에서 불편함 - 일관성 있게 adminInterceptor와 같이 DI방식으로 변경
- AuthController, LoginMemberArgumentResolver에서 중복된 token null/blank 체크 제거 - CookieUtil은 쿠키 추출만 담당, JwtTokenProvider는 JWT 파싱과 검증만 담당 - AuthService는 사용자 DB 조회만 수행 TODO: JwtTokenProvider 내부로 토큰 유효성 검증 책임 이관 예정(null, 만료, 서명 오류 등) ++enum으로 errorcode관리 예정
- ErrorCode enum 전면 도입: 상태 코드(HttpStatus) 및 메시지를 일원화하여 관리 - MEMBER_NOT_FOUND 오류에 입력 이메일 정보 포함 (detail 메시지) - 모든 에러 응답에 status 코드 포함되도록 구조 통일
- 기존코드는 동일한 email, name, role을 가진 사용자가 중복될 경우 토큰 충돌 가능성이 존재 - 이를 방지하기 위해 member의 고유 식별자인 id를 추가 claim으로 포함시켜, 토큰 생성 시의 고유성 보장
- ThemeController에서 비즈니스 로직을 ThemeService로 분리하여 전체적인 SRP 및 일관성 유지
- 의미 있는 행위 메서드 markDeleted()로 soft delete 처리 방식 통일 - setter 제거를 통해 외부에서의 불필요한 상태 변경 방지
일단 답변은 다 완료했으나.. 테스트코드를 잘 작성하고싶은 마음에 요구사항과 테스트코드, 그리고 과제로 주신 이론책에 대해 공부하다보니까 리소스분배에 실패했습니다.. 근데 테스트코드를 작성하지않으니까 정신이 없어서 요구사항이 누락되거나 잘못 구현되는 경우가 너무 많네요 반성 많이 하는 중입니다.. 수요일까지 무슨 일이 있어도 꼭 해가겠습니다. 추가로 enum을 활용해서 에러처리면에서 완성도 있는 코드를 하고싶었는데 기존에 있던 예외정도만 리팩토링하고 아직 생각만하고 처리하지못한 예외가 있어서 이것도 테스트코드와 함께 보완해보겠습니다. (++Memberstatus, reservationStatus같은것도 enum으로 고치려고 합니다) 항상 좋은 리뷰 감사드립니다! |
- 동일 사용자에 대해 같은 날짜/시간/테마에 중복 예약 불가 처리 - 예약이 완료된 사용자는 예약대기 등록도 제한 - 중복 발생 시 DuplicateException과 409 에러 반환 - ErrorCode에 WAITING_ALREADY_EXISTS 추가 - 이단계 TestCode에 이미 중복된 데이터라 일자 변경
- 커스텀 예외 사용하여 변경 - MemberNotFound와 일관성있게 하기 위해 도메인별로 notFoundException 생성
✨ 기존 코드를 변경/개선한 점
🤔 미션 중 신경 써서 구현한 점
예약 대기 순번(rank)
계산을 위해 서브쿼리 기반 JPQL을 작성하고,WaitingWithRank
DTO를 만들어 응답 처리예약
과예약 대기
를 하나의 API 응답(List)으로 처리하기 위해Stream.concat()
활용ReservationService
내부에 로직이 집중되지 않도록 노력했으나, 일부 복잡성은 아직 남아 있음 → 추후 관심사 분리를 고려✏️ 느낀 점
목요일날 시험이 끝나고 몸 컨디션이 안좋아서 최대한 구현하는데만 집중했습니다..ㅠㅡㅠ record, optional, 어노테이션(Column, table, 커스텀) 등등 얕게만 공부했으나 좀 더 리팩토링이 필요해보입니다.