Skip to content

Comments

[1 - 2단계 방탈출 예약 대기] 조이썬(이영수) 미션 제출합니다.#65

Merged
Hyunta merged 54 commits intowoowacourse:youngsu5582from
youngsu5582:step1
May 20, 2024
Merged

[1 - 2단계 방탈출 예약 대기] 조이썬(이영수) 미션 제출합니다.#65
Hyunta merged 54 commits intowoowacourse:youngsu5582from
youngsu5582:step1

Conversation

@youngsu5582
Copy link

@youngsu5582 youngsu5582 commented May 16, 2024

안녕하세요! 아서!!
이번에 리뷰를 받게된 조이썬 입니다! 잘 부탁드립니다!!

알파카(@slimsha2dy) 와 함께 했고
웹개발에 대한 경험은 Nest Framework(Typescript 기반) 으로 로컬 개발은 했으나
스프링은 처음이라서 중요한 부분들에 대해 공부하며 미션을 구현하고 있습니다!

최대한 빠르게 구현 후, 리뷰 올려서
피드백을 받으며 리팩토링&코드를 다듬으려고 했는데
생각보다 오래 걸렸네요..!

피드백 받고 싶은 부분

  • 유연하고 확장성 있는 설계를 해나가는 방법 및 방향성에 대해 피드백 받으며 얘기해보고 싶습니다!
  • 현재 테스트를 작성하는 것에 피로함을 느끼고(내 예약, 인기 테마 기능등) RestAssured 로 구현 안한 부분이 존재하는데
    테스트를 편하게, 리팩토링 내성이 있게 테스트를 작성하는 법에 대해 얘기해보고 싶습니다!

신경쓴 부분

Interceptor 순서 제어

현재 저는 로그인&어드민 부분, Resolver 주입 부분을 최대한 중복 검사를 피하는 부분으로 구현을 했습니다!
Request Scope 로 빈의 생애 주기 관리
해당 내용을 기반으로

@Component
@Scope(value = SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)
public class TokenContextRequest {
    private TokenLoginOutput tokenLoginOutput;

    public TokenLoginOutput getTokenLoginOutput() {
        if(tokenLoginOutput == null) {
            throw new UnauthorizedException();
        }
        return tokenLoginOutput;
    }

    public void setTokenLoginOutput(final TokenLoginOutput tokenLoginOutput) {
        this.tokenLoginOutput = tokenLoginOutput;
    }
}
//LoginInterceptor
@Override
public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) {
    final String token = tokenProvider.parseToken(request);
    final TokenLoginOutput output = memberService.loginToken(token);
    tokenContextRequest.setTokenLoginOutput(output);
    return true;
}
//AdminInterceptor
@Override
public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) {
    final var output = tokenContextRequest.getTokenLoginOutput();
    if (output.isAdmin()) {
        return true;
    }
    throw new ForbiddenException(request.getRequestURI());
}

이렇게 구현하며 느낀점으로는

  • preHandle 이 return false 를 하는 경우가 있을까?
  • 순서에 의존이 된다, 경로를 신경 써야 한다

가 있었으나,
중복된 코드 생략( 불필요한 DB 접근 역시도 가능 ) 때문에 사용해야 한다고 생각했습니다.
이때, 순서나 경로와 같은 개발자가 신경써야 하는 컨텍스트 는 어쩔수 없는 부분인지 궁금합니다!!

JPA 에서의 DTO 반환...?

현재 저희 기능 요구사항 중

사용자는 예약 가능한 시간을 확인하고, 원하는 시간에 예약을 할 수 있습니다.

해당 요구사항을 위해서

public record AvailableReservationTimeResult(boolean isBooked, long timeId, String startAt) {
}

기존 Dto 가 있었습니다.

하지만, JPA 를 사용하며
해당 Domain 만 return 을 하게 됨을 깨달았습니다.
따라서 Dto 를 반환하게 하는 방법을 찾아봤는데

  • JPQL 을 통한 쿼리 구문 내 새로운 DTO 생성
  • NativeQuery 통해 값들 주입 + interface 로 선언

이 두가지의 방법을 찾았습니다.

JPQL 을 통해서는 직접 파일(Dto) 경로명을 지정해야 해서 변경에 취약해지는 점 때문에
변화에 유연하게 대응할 수 있는 interface 방식의 NativeQuery 를 사용했습니다.

이때 두 방법중 어떤 방법을 사용해야 하는지, 장단점에 대해서 찾아보려 했으나
못 찾아서 질문하고 JPA에서 도메인(도메인을 받아 서비스단에서 로직 처리) 이 아닌 Dto 접근 방식이 맞는지 궁금합니다!


신경 쓴 부분은 여기까지 이며
아서가 해주는 리뷰에 대해 답변&수정하며 코드를 리팩토링 해나가고 싶습니다!
다시 한번 잘 부탁드립니다! ☺️☺️
이번 미션 수정한 코드

( 코드적 으로 궁금한 부분은 Files Changed 에 남기겠습니다! )

JPA 의존성 추가
- 이에 따른 테스트 코드 추가
- 이에 따른 테스트 코드 추가
- 이에 따른 테스트 코드 추가
- 이에 따른 테스트 코드 추가
Copy link
Author

@youngsu5582 youngsu5582 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드적인 궁금함은 여기까지 이며
부족한&구현하지 않은 테스트 코드는 주신 리뷰와 함께 반영하며 추가 하겠습니다! 🫡🫡
( 제 의견을 말하는 것보다, 단순히 묻는식의 코멘트가 많은거 같네요,,, 찾아봤는데 만족한 검색을 찾지 못했습니다 죄송합니다 😭😭 )

}

public void deleteReservation(final long id) {
reservationRepository.deleteById(id);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jdbc 를 사용한 기존 코드에서는
query 문을 날린후, 쿼리 결과의 값이 1인지 아닌지로 boolean 을 반환하여
삭제를 성공했는지 or 삭제할 값이 없었는지와 같은 로직을 수행했습니다.
jpa 는 delete 부분을 찾아보니

void reservationRepository.deleteById(id);

와 같이 아무 return 타입을 하지 않았습니다.
찾아보니

@Repository
public interface FruitRepository extends JpaRepository<Fruit, Long> {
    Long deleteById(Long id);  // To get deleted record count
}

와 같이 재정의 가능한 건 발견했으나
JPA 는 왜 delete 할때 default 로 void 를 할까 궁금해졌습니다.

단순 delete 구문이 삭제를 못하거나, 잘못 삭제할 경우가 크게 없는 점
+없는걸 삭제 하든 있는걸 삭제 하든 결국 사용자가 기대하는건 같은 점
등을 통해서 void 로 지정해도 상관 없다 라고 생각하는데요
해당 접근 방식이 맞을까요? 🥲

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 그렇게 생각합니다.
삭제할 때 몇개의 row가 삭제됐는지 꼭 필요한 로직이 아니라면 void로 지정해도 상관 없을 것 같습니다


@Override
public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) {
if (request.getMethod().equals("GET") && CHECK_LIST.contains(request.getRequestURI())) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은
/reservations 의 GET 부분 때문에
추가가 되었습니다.

다른 reservation 경로는 상관 없으나
/reservations GET 요청은 LoginInterceptor 가 필요 없기 때문에 추가되었습니다.

이 부분을 제거 하고 싶은데
WebConfigruation 에서 어떻게 설정을 해야 할지 잘 모르겠네요..

이렇게 경로 세밀 제어 + 메소드 별 제어는
어떻게 설계 및 지정을 해야 할까요...?
( GET /posts 와 같이 로그인 없이 조회 같은 것만 제외 )

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 삭제하지 않으셔도 괜찮을 것 같습니다.
오히려 직관적으로 로그인이 필요하지 않은 페이지를 정의할 수 있도록 해당 Interceptor에서 관리하는게 좋아보이는데요.
제거하고 싶으신 이유가 있을까요?

SpringSecurity를 사용하면 메서드, 경로 별로 조금 더 디테일하게 관리는 할 수 있습니다만,
특정 경로에서 메서드로 분리해야하는 상황이라면 저는 Interceptor 하위에 구현하는게 나아보이긴 합니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제거하고 싶었던 이유로는
인터셉터 에서 비즈니스 로직이 아닌 외적인 경로 및 메소드 제어를
한다고 생각해서 였습니다..!

이를 위해 interceptor 보다 먼저 작동하는 filter 를 통해 해당 부분을 제어할까 했으나
이 역시도 오버 엔지니어링 같네요..!

interceptor 가 해당 제어 부분도 가져도 괜찮을 거 같습니다!

Copy link

@Hyunta Hyunta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 조이썬,
1,2단계 미션 잘 구현해주셨네요.
비교 커밋 부분을 봤는데도 불구하고 fileChange가 너무 많아서
제가 누락한 부분이 있을 수도 있을 것 같아요.
그런 부분이 있으면 커멘트 달아주시거나 DM 주세요.

public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) {
final String token = tokenProvider.parseToken(request);
final TokenLoginOutput output = memberService.loginToken(token);
final var output = tokenContextRequest.getTokenLoginOutput();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입 지정을 하지 않은 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분에는 크게 없습니다,, 아무 생각없이 var 을 사용한거 같습니다 🥲🥲

이건 레벨1적인 내용이긴 하나
아서는 var 을 즐겨 사용하는지 궁금합니다!

저는 var을
DTO 와 같이 정확한 명시가 필요 없으며 & 변경의 가능성 이 높거나
id,email 같이 단순한 일급 객체 등등 - 변수의 타입을 교체 하더라도 같은 기능을 한다고 기대

와 같은 경우에서는 적극 사용해도 괜찮다고 생각하는데 아서의 의견이 궁금합니다..! 🫡

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

취향의 문제인듯 합니다.
저는 타입 추론 언어를 좋아하지 않습니다.
직관적으로 내용을 파악했으면 좋겠는데 걸림돌처럼 느껴지더라구요.
개인적으로 타입을 지정해주는 편을 선호합니다.
그래서 var를 사용하지는 않습니다.


@Override
public boolean preHandle(final HttpServletRequest request, final HttpServletResponse response, final Object handler) {
if (request.getMethod().equals("GET") && CHECK_LIST.contains(request.getRequestURI())) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 삭제하지 않으셔도 괜찮을 것 같습니다.
오히려 직관적으로 로그인이 필요하지 않은 페이지를 정의할 수 있도록 해당 Interceptor에서 관리하는게 좋아보이는데요.
제거하고 싶으신 이유가 있을까요?

SpringSecurity를 사용하면 메서드, 경로 별로 조금 더 디테일하게 관리는 할 수 있습니다만,
특정 경로에서 메서드로 분리해야하는 상황이라면 저는 Interceptor 하위에 구현하는게 나아보이긴 합니다.

Comment on lines 21 to 22
@ManyToOne
private ReservationTime time;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchType에 대해서 한번 알아볼까요?
기본값으로 ManyToOne은 fetchType.Eager로 설정되어있습니다 관련해서 발생할 수 있는 성능 이슈에 대해서 알아봅시다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eager Loading

Eager 는 하위 모든 엔티티를 한번에 같이 로딩

  • 한번에 모든 값을 가져 오므로, 메모리 및 성능 저하의 우려가 있다.

Lazy Loading

Lazy 는 하위 엔티티 가 필요할때 다시 DB에 쿼리를 보내 로딩을 한다고 알고 있습니다!
( 이때 Lazy 로딩을 할때 하위 객체는 프록시 객체로 만들어집니다 )

  • 트랜잭션 범위를 벗어날 시,LazyInitializationException 발생 - 접근할 수 없으므로
  • 필요할때마다 쿼리를 보내므로, 각 요소마다 쿼리 가 전부 나갈 수 있다 - N+1 문제

이때 이러한 Lazy 의 문제점을 해결하기 위해서
Eager 로 변경 or Fetch Join 을 사용 하라는 내용은 봤으나
아직 실제로 적용 및 확인을 해볼 기회는 없었습니다

혹시나, 틀린 내용이 있거나 키워드 가 부족하다면 더 얘기해줘도 좋습니다! 🙂

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 찾아주셨네요ㅎㅎ
지금 당장 적용할 곳이 없다면 보류하시는 것도 좋은 방법인 듯합니다.
추후에 쿼리가 많이 나간다 싶을 때 고려해봐주세요!

Comment on lines +27 to +28
@Enumerated(EnumType.STRING)
private ReservationStatus reservationStatus;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnumType.STRING으로 지정해주신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enumerated 의 default 값은 ordinal 이고
DB에 저장되는 값을 확인해본 결과 0,1 같은 순서 값으로 저장이 되는걸 발견했습니다.

그렇기에, 이 값은 Enum 이 추가되는 도중 순서를 실수로 잘못 넣을시
모든 값이 깨질 우려가 있다고 생각해서 STRING - VARCHAR 로
변화에는 유연하게& 실수에 안전하게
하는게 좋다고 생각했습니다!

Comment on lines 30 to 31
public Reservation() {
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기본생성자가 왜 필요할까요?
사용할 수 있는 접근제어자는 어떤게 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단순히 빈 생성자를 통해 인스턴스 생성하고
리플렉션 을 통해 값을 넣는다로만 알고 있었는데
public 이 아닌 protected 도 가능하네요..!

단순 빈 생성자만 있으면 되는 점 + 굳이 public 으로 노출할 필요가 없는 점

을 생각해서 protected 로만 지정을 할 꺼 같은데 해당 접근이 맞을까요??

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네! 잘 찾아주셨네요ㅎㅎ
말씀해주신 것처럼 JPA에서 Entity를 프록시로 생성하기 위해서 protected 이상의 기본 생성자를 요구하고 있습니다.
꼭 public이 아니어도 된다면 protected로 지정해주는 편이 좋을 것 같습니다.

Comment on lines 48 to 58
@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final ReservationTime that = (ReservationTime) o;
return Objects.equals(id, that.id);
if (this == o) return true;
if (!(o instanceof final ReservationTime that)) return false;
return Objects.equals(startAt, that.startAt);
}

@Override
public int hashCode() {
return Objects.hash(id);
return Objects.hashCode(startAt);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReservationTime 이라는 클래스의 id는 고유 식별자 아닐까요?
애초에 등록될 때 동일한 시간을 갖도록 한다면 id로도 비교할 수 있겠네요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 해당 부분에 대해서 미션1,2 동안 계속 고민을 해왔습니다,,,
구구 한테도 물어보고 리뷰어님들 한테도 물어보며 저만의 의견을 내렸습니다..!

DB적 관점으로 보면은 Ok ( ID 가 같으면, 당연히 다른 값들도 같을테니까 )
하지만 객체 지향적으로 봤을때 그것이 같은가?
( DB 에 데이터 저장 전이거나, 데이터 중복 검사를 할때는? )

-> equals & hashCode 는 어느 상황에서든 동일하게 적용이 되어야 한다!
-> 애매하다면 차라리 아예 구현을 하지 않는 것 역시 방법
도메인에 통용되는 규칙을 정해서 사용 역시 방법 ( 우리 프로젝트는 id가 같으면 같은 객체로 판단 )
필요에 따라 id 기준 중복 제거 or 전체 필드 같은지 확인하려면 직접 해도 OK!

와 같은 결론을 내렸는데
아서의 의견은 id 를 고유 식별자 로 보고 equals&hashCode 를 하는 대상으로 보는 걸까요? 🤩

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 제 의견은 조이썬이 정리해주신게 맞습니다.
id가 동일하면 내용과 관계없이 동일한 내용으로 보도록 구현할 것 같아요.
그게 고유 식별자의 주된 역할이라고 생각해서요.

그런데 조이썬의 입장도 이해가 갑니다.
원하시는 방향을 선택해보시면 될 것 같습니다.

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
private LocalTime startAt;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Column 이라는 애노테이션에서 unique를 보장해볼 수 있을 것 같습니다.

Copy link
Author

@youngsu5582 youngsu5582 May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

중복된 값을 피하기 위해 @Column(unique = true) 어노테이션 을 사용해야 했네요!

추가적인 궁금한 점으로
Application 단에서 완벽하게 검증을 했다면 unique 가 필요할까?
라는 생각이 들었습니다.

현업에서는 외래키를 사용하지 않는다 답변한 PR 라는 내용에 대해서 공부 했었는데

유니크 제약조건은
성능이나 복잡도 증가와 같은 부분에도 외래키와 다르게 현업에서 사용하는지 궁금합니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Application에서도 unique를 검증해주고, Domain에서도 검증을 해주는 편이 좋아보입니다.
나중에 엔티티를 파악할 때도 unique가 작성되어있으면 판단할 수 있으니까요.
저희 팀으로 국한지어서 설명드리면 저희는 외래키도 사용하고, 유니크 제약조건도 사용합니다.


현업 이라는 키워드가 참 마법 같은 단어죠ㅎㅎ
그런데 저희 회사도 팀마다 규칙이 다 달라요. 마찬가지로 다른 회사도 그렇습니다.

저도 우테코 생활할 때 많이 했던 고민이고 실수인데 현업 이라는 개념을 만들지 않으셨으면 좋겠어요ㅎㅎ
제가 조이썬한테 현업에서 외래키를 사용하니까 꼭 사용해주세요 라고 주장을 펼치게 되면 어떻게 반박할 수 있나요?

외래키를 써서 정말 치명적인 문제가 있지 않는 이상 반박하기가 너무 어려울 거에요.
(막말로 치명적인 에러가 있더라도 거의 발생안하고 사용하기에 편하다면 그냥 쓰는 팀도 있을 겁니다.)
그리고 현업이라는게 회사마다 다르고, 팀마다 다르기 때문에 어떻게 뭐가 옳고 그른지를 따지기에 적절한 기준이 아닐 수 있어요.

앞으로 이제 프로젝트를 하실텐데 취준생끼리 진행하는 토론에서 현업 이라는 근거만큼 폭력적인게 없는 것 같습니다.
네카라쿠배에서 이렇게 한데 라는 근거를 가져가면 반박하기가 너무 어렵잖아요.

제가 회사에 와서 느낀건 오히려 우테코때 고민했던 내용들이 더 깊었던 것 같은데? 라고 느껴지는 순간들도 종종 있습니다.
워낙 빠르게 변하고 바쁘기 때문에 매번 기준에 대해서 깊이있는 고민을 하기가 어려운 것 같아요.
회사마다 다르겠지만 저는 다른 회사도 별반 다르지 않을 거라고 확신합니다.

지금처럼 현업에서는 어떻게할까 자료를 찾아보고 왜 그렇게 할까 근거를 가져가시는 모습은 정말 좋은 것 같습니다.
바빠지시면서 고민을 하거나 설득하기 어려울 때 현업에서 이렇게 한데 라는 주장을 하지 않으셨으면 좋겠어서 조금 길게 커멘트를 남겨보았습니다ㅎㅎ
(지금 잘못하셨다는건 아니고, 현업에 대한 이야기를 해주셔서 도움이 됐으면 하는 마음에 공유드립니다.)

Comment on lines 33 to 40
public Reservation(final Long id, final ReservationDate date, final ReservationTime time, final Theme theme, final Member member, final ReservationStatus reservationStatus) {
this.id = id;
this.date = date;
this.time = time;
this.theme = theme;
this.member = member;
this.reservationStatus = reservationStatus;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자에는 id를 제외해주어도 됩니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생각해보니 아이디 생성 전략을 DB 에 위임하는 경우에
애초에 ID를 포함하는 생성자를 만들필요도, 만들지 않는게 더 좋은거 같네요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아서!!
해당 피드백을 반영하며 느낀점으로
다른 엔티티 에 의존받지 않는
즉, Reservation 처럼 최상위 엔티티가 아니면
id 를 가진 생성자도 반 필수라는 생각이 들었는데요..!

  • 존재하는 엔티티들을 통해 상위 엔티티인 Reservation을 생성할때
  • 테스트 코드에서 Reservation 을 생성할 때

하지만, JPA+ID 자동생성 전략을 사용하는 한
id 가 있는 생성자 는 절대 필요할 이유가 없다! 라는 생각도 드는데요,,

설계나 생각의 방향을 잘못하고 있는 걸까요? 😭😭

Copy link
Author

@youngsu5582 youngsu5582 May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에서 말한 내용들은

@Test
@DisplayName("특정 테마에 대한 예약이 존재하면 예외를 발생한다.")
void throw_exception_when_delete_id_that_exist_reservation() {
    final ThemeOutput themeOutput = themeService.createTheme(
            ThemeFixture.getInput());

    final ReservationTimeOutput timeOutput = reservationTimeService.createReservationTime(
            new ReservationTimeInput("10:00"));
    final Member member = memberRepository.save(MemberFixture.getDomain());

    reservationRepository.save(Reservation.fromComplete(
            "2024-04-30",
            ReservationTime.from(timeOutput.id(), timeOutput.startAt()),
            Theme.of(themeOutput.id(), themeOutput.name(), themeOutput.description(), themeOutput.thumbnail()),
            member
    ));
    final var themeId = themeOutput.id();

    assertThatThrownBy(() -> themeService.deleteTheme(themeId))
            .isInstanceOf(ExistReservationException.class);
    }

와 같이 Service 단에서 다른 Service 를 호출할때 Dto 로 구성되어 있어
다시 새로운 도메인을 만들때 발생했는데

@Test
@DisplayName("특정 테마에 대한 예약이 존재하면 예외를 발생한다.")
void throw_exception_when_delete_id_that_exist_reservation() {
    final Theme theme = themeRepository.save(ThemeFixture.getDomain());
    final ReservationTime reservationTime = reservationTimeRepository.save(ReservationTimeFixture.getDomain());
    final Member member = memberRepository.save(MemberFixture.getDomain());
    reservationRepository.save(Reservation.fromComplete(
            "2024-04-30",
            reservationTime,
            theme,
            member
    ));
    final var themeId = theme.getId();

    assertThatThrownBy(() -> sut.deleteTheme(themeId))
            .isInstanceOf(ExistReservationException.class);
}

이와 같이 비교적 명확하게 되는거 같은데
특정한 상황이 아니면 생성자에 id 를 없애는게 일반적인지 궁금합니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 Reservation에서 하위 엔티티에 대해서 Id만 들고있다면 생성자에 해당 Id 값은 필요할 것 같습니다.
id의 생성전략이 DB에 있는 이상 애플리케이션 단에서 생성할 이유는 없을 것 같습니다.

그런데 조이썬의 구조로 봤을 때는 하위 엔티티의 경우 DTO -> Entity 과정에서 Id가 필요하게 되겠네요.
말씀해주신 것처럼 이럴 경우에는 id를 추가해주는 편이 빠르게 문제를 해결할 수 있을 것 같습니다ㅎㅎ

@Hyunta
Copy link

Hyunta commented May 17, 2024

이때 두 방법중 어떤 방법을 사용해야 하는지, 장단점에 대해서 찾아보려 했으나
못 찾아서 질문하고 JPA에서 도메인(도메인을 받아 서비스단에서 로직 처리) 이 아닌 Dto 접근 방식이 맞는지 궁금합니다!

ReservationTimeRepository라면 ReservationTime를 반환한 뒤 후 작업은 Service에서 진행하는게 직관적일 것 같습니다.

추가로 해당 DTO를 추상화해서 인터페이스로 관리하신게 Repository에서 값을 받기 위함이었을까요?
조회하는 작업일 뿐인데 오버헤드가 많이 발생하게 됐네요.
개인적으로 Repository에서 도메인을 받아와서 Service에서 DTO로 변환해주는 편이 복잡도가 많이 줄어들 것 같습니다.

MemberApiController 내 회원가입 기능 이동
관련 테스트 코드 반영
- 회원가입 대한 인수 테스트 구현
관련 테스트 코드 반영
- 로그인 대한 인수 테스트 구현
- 삭제 기능 구현 대한 인수 테스트 구현
API Controller Test 코드 제거
- 관련 코드 반영
[관련 피드백 링크](woowacourse#65 (comment))
@youngsu5582
Copy link
Author

youngsu5582 commented May 19, 2024

아서!
반영해주신 피드백을 최대한 반영하려 했는데 아서의 의도대로 잘 반영이 된지 모르겠네요 🥲🥲

피드백 주신 내용에 저의 의견 및 궁금함을 담았습니다!

추가적인 내용으로는
브라운의 우아콘 ATDD 발표를 보며 인수 테스트에 흥미가 생겨
인수 테스트를 만들고 있습니다! ( 인수 테스트에 대한 방향성이나 부족한 점에 대해 말씀해주셔도 너무 좋습니다! 🙇‍♂️ )

다시 한번 리뷰 해주셔서 감사하고, 잘 부탁 드립니다! 🫡
변경된 부분

Copy link
Author

@youngsu5582 youngsu5582 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드적인 부분에 대해서 궁금함은 여기까지 이며
리뷰 주실때까지 인수 테스트 작성을 하고 있을꺼 같습니다! 감사합니다!! 🫡🫡

ON rt.id = r.time_id AND r.date = ? AND r.theme_id = ?
""", nativeQuery = true)
List<AvailableReservationTimeResult> getAvailableReservationTimeByThemeIdAndDate(LocalDate date, Long themeId);
@Transactional
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분에서
@Transactional 을 걸지 않으니

{
    "message": "No EntityManager with actual transaction available for current thread - cannot reliably process 'remove' call",
    "status": false
}

와 같이 ReservationTimeAcceptanceTest 에서 예외가 뜨는걸 확인했습니다.

메소드 파싱을 통해 만드는 delete 구문은 기본적으로 트랜잭션을 걸어주지 않는다는 사실을 발견했는데

단순히 Transactional 을 걸면 되겠구나 자로 생각하면 될까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 처음 알게되어서 좀 찾아봤는데,
직접 구현한 커스텀 메서드는 JpaRepository에서 관리하는 대상이 아니라서 따로 트랜잭션을 관리해줘야 되나봅니다.
네 우선 작성해주신데로 처리하시고 넘어가시죠ㅎㅎ

return availableReservationTimeResult.stream()
.map(AvailableReservationTimeOutput::toOutput)
.toList();
public static List<AvailableReservationTimeOutput> toOutputs(final List<ReservationTime> reservationTimes, final List<ReservationTime> bookedTimes) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은
총 시간,이미 예약된 시간 들을 통해서
DTO 를 만드는 로직 입니다.

제 생각으로는
이 두 매개변수를 가지고 명확하게 변한을 할수 있으므로 괜찮다 라고 생각했는데

이 역시도 비즈니스 로직의 일환이라 생각하고
Service 단으로 이동하는게 더 좋은 코드일까요??

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 DTO를 생성하는 로직은 DTO 내부에서 관리할 수 있다면 DTO 내부에 위치시키는 것이 적절하다고 생각합니다.
Service단으로 옮기게 되면 Service가 불필요한 로직들로 너무 뚱뚱해질 것 같습니다.

final List<AvailableReservationTimeResult> availableReservationTimeResults =
reservationTimeRepository.getAvailableReservationTimeByThemeIdAndDate(input.date(), input.themeId());
return AvailableReservationTimeOutput.toOutputs(availableReservationTimeResults);
final List<ReservationTime> alreadyBookedReservationTimes =
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 JPA 제공 메소드들을 통해 값들을 들고온 후
서비스 로직 에서 값을 변환 및 처리하게 변경했습니다!

하면서 느낀점으로는

JOIN 문을 포기함으로써 결국 쿼리가 한번 더 나가게 되었다고 느꼈습니다.

가벼운 쿼리 or 로직 이라면 코드 가독성&편의성 + 명확함을 가져다 주기에
쿼리를 두 번 보내게 해도 상관없을까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

경우에 따라서 쿼리 2번이 나을 수도 있습니다.
쿼리의 가벼움보다는 빈도를 보고 결정하는게 좋아보입니다.
만약 쿼리를 자주 사용해야한다면 한번으로 줄이는게 좋을 것 같습니다.

import static roomescape.acceptance.step.MemberStep.로그인;
import static roomescape.acceptance.step.MemberStep.멤버_생성;

@AcceptanceTest
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인수 테스트를 시도해보며 느낀점은
완벽하게 요구 사항 및 플로우 를 정의하고 ATDD 로 시작을 한다고 할때

단순히 TDD,기능 동작을 위해서 작성하는 테스트 코드가 아니라면

Service,Repository 테스트가 필요할까 에 대한 생각이 들었습니다. ( Controller 는 인수 테스트의 범주 + 서비스가 비즈니스 로직을 담당하기에 필요없다고 판단 )

애초에 방향의 범주를 잘못 잡는걸까요?
( ATDD 를 동작시키기 위해 서비스,레포지토리의 TDD 가 필요한거 같기도 합니다! )

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 ATDD는 API의 완성을 확인하기 위해서 작성하고,
레이어별 테스트코드는 각 기능이 정상적으로 수행되는지, 엣지케이스에 대한 검증을 하기 위해서 사용합니다.
Repository 단은 모르겠지만 Service 단에서 디테일한 것들을 잡기 위해서는 필요해보이지 않나요?
제가 질문을 잘 이해를 못한 걸 수도 있을 것 같은데, 만약 답변이 더 필요하다면 커멘트나 DM 주세요.

Copy link

@Hyunta Hyunta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 조이썬,
남겨주신 커멘트 확인하고 답변 남겨놨습니다.
혹시 이해가 안가시거나 궁금한 점이 있으면 DM 주세요.
다음 단계 진행을 위해서 머지하도록 하겠습니다.

@Hyunta Hyunta merged commit 85223f3 into woowacourse:youngsu5582 May 20, 2024
Hyunta pushed a commit that referenced this pull request May 29, 2024
* feat : migration code

* feat(ReservationTimeRepository) : JDBC->JPA 변환 구현

JPA 의존성 추가
- 이에 따른 테스트 코드 추가

* feat(ThemeRepository) : JDBC->JPA 변환 구현

- 이에 따른 테스트 코드 추가

* feat(MemberRepository) : JDBC->JPA 변환 구현

- 이에 따른 테스트 코드 추가

* feat(ReservationRepository) : JDBC->JPA 변환 구현

- 이에 따른 테스트 코드 추가

* feat(ThemeRepository) : 인기 테마 가져오는 기능 JPA 통해 구현

- 이에 따른 테스트 코드 추가

* feat(ReservationTimeRepository) : 예약 가능한 시간 받는 기능 구현

- 이에 따른 테스트 코드 추가

* feat(ReservationTimeRepository) : 예약 가능한 시간 받는 기능 구현

- 이에 따른 테스트 코드 추가

* feat(MemberService) : Dao -> Repository 교체

반영된 코드 수정

* feat(ThemeService) : Dao -> Repository 교체

반영된 코드 수정

* feat(ReservationTime) : Dao -> Repository 교체

반영된 코드 수정

* feat(Reservation) : Dao -> Repository 교체

반영된 코드 수정

* feat(AvailableReservationTime) : record 삭제,이름 변경

* refactor(dao,mapper) : JPA 변환으로 삭제

jdbc 의존성 삭제

* feat(Member) : 저장시, Enum 문자열 값으로 저장되게 변경

* test : 불필요한 테스트 코드 삭제

* docs : 기능 요구사항 초안 정리

* feat(Reservation) : 예약 상태 추가

반영된 코드 수정

* feat(ReservationService): 자신이 예약한 예약들 가져오는 기능 구현

* feat(ReservationController): 자신이 예약한 예약들 응답 기능 구현

* feat(MemberPageController): 내 예약 페이지 응답 기능 구현

* fix(ReservationApiController): API 응답 dto 수정

* refactor: Inserter 패키지 이동

* test: 내 예약 페이지 응답 기능 테스트 구현

* feat : 초기 데이터 설정,스키마 파일 삭제

* test : DB 초기화 로직 변경

* feat(Theme) : ID 생성 전략 추가

* fix : test 시에도 지연 생성 설정 추가

* refactor : dao->repository 로 패키지명 변경

* refactor(all) : 코드 자동 정렬

* refactor(ReservationApiController) : RequestMapping 경로 추가로 다시 경로 응집성 있게 변경

* refactor(LoginApiController) : ResponseCookie 로 변경

* refactor(ThemeApiController) : RestController 로 변경

* refactor(All) : 사용하지 않는 불필요한 코드들 제거

* feat(Thumbnail) : 확장자 검사 기능 제거

* test : SpringBootTest 로 변경

* feat(Interceptor) : 인터셉터 순서 지정해 Login->Admin 순으로 동작하게 변경

- 관련 코드 수정

* test(Acceptance) : 인수테스트 초기 설정

* refactor(AuthController) : 컨트롤러 명 변경, 기능 이동

MemberApiController 내 회원가입 기능 이동
관련 테스트 코드 반영
- 회원가입 대한 인수 테스트 구현

* feat(MemberService) : 로그인 중 발생하는 예외 변경

관련 테스트 코드 반영
- 로그인 대한 인수 테스트 구현

* feat(ReservationService) : 삭제한 값이 없을때 예외 발생 기능 구현

- 삭제 기능 구현 대한 인수 테스트 구현

* test(AuthAcceptanceTest) : 로그인 확인 기능에 대한 인수 테스트 구현

* feat(Member,MemberRepository) : 이메일 문자열로 검색하게 변경, 칼럼명 지정

- 반영된 코드 수정
[관련 피드백 링크](#65 (comment))

* feat(Reservation) : 불필요한 생성자내 Id 매개변수 제거

- 관련 코드 반영

* test : 인수 테스트 전환으로 인해 컨트롤러 테스트 제거

* refactor(Member) : 불필요한 정적 팩토리 제거

- 관련 코드 변경
[관련 피드백 링크](#65 (comment))

* refactor : 빈 생성자 접근 제한자 변경

[관련 피드백 링크](#65 (comment))

* feat(Theme,ReservationTime,Member) : 불필요한 생성자내 Id 매개변수 제거

API Controller Test 코드 제거
- 관련 코드 반영
[관련 피드백 링크](#65 (comment))

* feat(TokenContextRequest) : 불필요한 ScopeBean 삭제

[관련 피드백 링크](#65 (comment))

* feat : unique 제약 조건 추가

[관련 피드백 링크](#65 (comment))

* test(Interceptor) : controller->config 패키지로 이동

* feat(ReservationTimeService) : 예약가능 시간 조회 기능 로직 변경

[관련 피드백 링크](#65 (comment))

* refactor(Reservation) : 예약 날짜 검색 메소드명 자연스럽게 변경

* fix(Reservation) : 누락된 예약 날짜 검색 메소드명 변경 내역 수정

* test(Fixture) : 불필요한 Acceptance 내 fixture 제거

* test(Theme) : 테마 생성,삭제 대한 인수 테스트 구현

* feat(AuthApiController) : 멤버 생성시 결과 반한하게 기능 구현

- 관련 코드 수정

* test(ReservationAcceptanceTest) : 예약 생성 인수 테스트 구현

* test(ReservationTimeAcceptance) : 예약 가능한 시간 조회 기능에 대한 인수 테스트 구현

* test(ReservationTimeAcceptance,ThemeAcceptance) : 예약이 존재할 때 삭제 인수 테스트 구현

* test(ReservationAcceptance) : 본인 예약 조회 인수 테스트 구현

* refactor(AcceptanceTest) : 실행 클래스 명시,불필요한 구문 삭제

* docs(README) : 3단계 기능 요구사항 초안 정리

* feat(Reservation) : 예약 상태 변수 제거

- 반영된 코드 수정

* refactor(Reservation) : 테이블,객체명 변경

Reservation -> ReservationInfo

* fix(user-script) : 회원가입 경로 변경

* refactor(ReservationInserter) : 불필요한 파일 삭제

- 반영된 코드 수정

* feat(ReservationInfo) : 예약에 대한 날짜,테마,시간을 저장하는 엔티티 분리

* feat(Reservation) : 예약정보,멤버를 가지는 엔티티로 변경

- 반영된 코드 수정
- DatabaseCleaner 에도 반영

* feat(Waiting,WaitingService) : 대기 생성하는 기능 구현

- 이에 따른 테스트 코드 추가

* feat(WaitingController) : 대기 생성 요청/응답 수행하는 API 구현

기능 요구사항 완료에 따른 체크
- getReferenceById -> getById로 교체

* feat(WaitingController) : 대기 삭제하는 요청/응답 수행

기능 요구사항 완료에 따른 체크
- 프론트 단 반영

* docs(README) : 3단계 기능 요구사항 추가

* feat(WaitingService) : 예약이 없을때 대기 불가능 기능 구현

메소드 매개변수 변경,부정 메소드 추가
- 이에 따른 테스트 코드 추가

* feat(WaitingService) : 내 예약 조회시 대기 예약도 포함하게 추가

- 기능 완료에 따른 요구사항 체크
- 이에 따른 테스트 코드 추가

* docs(README) : 4단계 기능 요구사항 초안 정리

* test(WaitingAcceptanceTest) : 예약 취소중 발생 시나리오 작성

* feat(WaitingService) : 예약자는 예약 대기 불가능하게 기능 구현

- 이에 따른 테스트 코드 추가

* feat(WaitingService) : 예약자 취소 시,다음 대기자가 예약자가 되는 기능 구현

- 이에 따른 테스트 코드 추가

* test(WaitingAcceptanceTest) : 첫 번째 시나리오 구현

- Step 중복 코드 제거

* feat(WaitingService,ReservationService): 당사자 아니면 취소 못하게 기능 구현

- 이에 따른 테스트 코드 추가

* feat(WaitingController,ReservationController): 삭제 요청시 로그인 정보 받게 추가

* test(WaitingAcceptanceTest) : 세 번째 시나리오 구현

* feat(AdminPageController) : 예약 대기 관리 페이지 응답 기능 구현

js,html 반영

* feat(AdminApiController) : 운영자가 대기 조회,거절하는 기능 구현

* test(WaitingAcceptanceTest) : 두 번째 시나리오 구현

운영자 활용 로직을 위해 context.sql 에서 운영자 삽입
- 기능 완료 따른 요구 사항 체크

* refactor : 도메인 내 멤버변수 한 칸씩 구분, toString 재정의

* fix(TokenProvider) :  DB 초기화 추가

* refactor : 대기 도메인이 멤버 검증하게 수정

* build : 샘플용 데이터 추가

* refactor : TODO 삭제, 사용하지 않는 코드 제거

* refactor : 정적 팩토리명 from 으로 전부 변경

[관련 피드백 링크](#108 (comment))
[관련 피드백 링크](#108 (comment))

* refactor : 어노테이션 변경, 응답 타입 변경

[관련 피드백 링크](#108 (comment))
[관련 피드백 링크](#108 (comment))

* feat : 서버 에러 경로 로깅에 출력 기능 구현

[관련 피드백 링크](#108 (comment))

* feat : 예약 취소 기능에 @transactional 추가

[관련 피드백 링크](#108 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants