Skip to content
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

🐷 step3 : 즐겨찾기 기능 구현 #241

Open
wants to merge 10 commits into
base: minyul
Choose a base branch
from

Conversation

minyul
Copy link

@minyul minyul commented Feb 23, 2022

안녕하세요! 마지막 리뷰면서도.. 마지막날이네요! 감사했습니다. 😀😀

구현하면서 느낀게 음..
두 부분에서 어려움을 느꼈습니다.

image
조회하는 테스트에서 이렇게 밖에 할수가 없는가!? 뭔가 안에 내부까지 값을 확인하고싶은데 제가 잘 못하는건지... 음

image
이 권한이 없는 부분은 TokenAuthenticationInterceptor 가기 전에 Resolver에서 해결을 해야한다고 생각을 하고 있거든요!?
음.... 찾지를 못해서요.. 🤔

감사합니다!

Copy link

@junwoochoi junwoochoi left a comment

Choose a reason for hiding this comment

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

안녕하세요! :)
제가 리뷰가 조금 늦었죠...ㅠㅠ 죄송합니다!!! 🙏

미션 질문 주신 내용 및 몇가지 추가적으로 도움이 될만한 부분에 대해 코멘트 달아보았는데 확인 부탁드립니다 🥺

this.userDetailsService = userDetailsService;
}

public void checkAuthentication(User userDetails, AuthenticationToken token) {

Choose a reason for hiding this comment

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

private으로 선언해볼 수 있겠네요 :)

Copy link
Author

Choose a reason for hiding this comment

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

리뷰는 천천히 해주셔도 됩니다! 이미 시간이 지나버렸는걸요! 천천히 해주셔도 괜찮습니다. 해주시는것만으로도 정말 감사하구요! 감사합니다!

Comment on lines 14 to 24
@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "member_id")
private Member member;

@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "up_station_id")
private Station source;

@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "down_station_id")
private Station target;

Choose a reason for hiding this comment

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

Favorite이 저장될 때, Member, source, target들이 함께 저장되는게 의도하신 바가 맞을까요?
라이프사이클을 모두 하나로 보신 것인지 궁금합니다 👀

Copy link
Author

Choose a reason for hiding this comment

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

음..... 이부분은 기존에 LineSection의 관계의 코드를 보고 구현했습니다.
지금 다시 생각해보면 Member에만 CascadeType.PERSIST 가 있는 것이 맞다고 생각되고
Favorite에는 굳이 CascadeType.PERSIST가 필요없다고 생각합니다.


더 나아가서 이 부분이 진짜 계속 멤돌았는데..

Section 에서 Upstation, DownStation.
Favorite 에서 Source, Target.

이부분에서 @ManyToOne을 쓰는 것이 맞나?! 하는 의문이 있습니다.

image

음.. 구간(Section) 에 역 2개 (Upstation, DownStation) 이 있으며
즐겨찾기(Favorite)에 역 2개 (Source, Target) 이 있다.

결국엔 같지만 왜 @ManyToOne이지? 라는 의문이 살아지지가 않아서요.ㅠㅠㅠㅠㅠ

Comment on lines 17 to 18
@Embedded
private Favorites favorites = new Favorites();

Choose a reason for hiding this comment

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

Member와 Favorite가 현재 양방향 관계네요!

객체 관계가 양방향인 경우 꼭 양방향이어야 하는지를 고민하고 둘 중 하나의 방향을 끊어주는 것을 추천드립니다.
단방향인 경우에는 해당 객체 참조가 반드시 필요한 것인지 고민 해보시고, 필요 없으면 끊어보시길 권해드립니다.

아래의 영상을 보시면 이해를 도울 수 있을 것 같네요 😄
https://youtu.be/dJ5C4qRqAgA?list=PLgXGHBqgT2TtGi82mCZWuhMu-nQy301ew&t=3864

Copy link
Author

Choose a reason for hiding this comment

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

양방향 관계를 끊어버렸습니다. Member 쪽에는 Favorite List 필드를 없애버렸고 Favorite에만 Member를 알게 했습니다. 제대로 했는지..는 잘모르겠지만요 ㅠ-ㅠ

Comment on lines 14 to 16
@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "member_id")
private Member member;

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.

과거 신입 프로젝트로 로그인쪽을 구현하고 코드리뷰를 받았을 때, 사용자와 관련된 부분은 직접참조, 아닌 경우에는 간접 참조를 하라고 피드백을 받았었는데요! 어떻게 생각하시나요?

그리고 준우님 말씀대로 직접 -> 간접으로 객체참조를 바꿨습니다.
혹시 잘했는지 확인해주실수있을까요?!

감사합니다.


// then
assertThat(result.statusCode()).isEqualTo(HttpStatus.OK.value());
assertThat(result.jsonPath().getList("id")).hasSize(1).containsExactly(1);

Choose a reason for hiding this comment

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

질문 주신 내용이 정확히 어떤 내부 값을 갖고 오고 싶은 걸까요?

		List<Long> ids = result.jsonPath().getList("id", Long.class);

요런식으로 id값들을 가져와서 리스트에서 assertion해보실 수 있는데 요게 궁금하신 게 맞을까요? 🙄

Copy link
Author

Choose a reason for hiding this comment

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

오오! 맞습니다 감사합니다.!

}

if (!userDetails.checkPassword(token.getCredentials())) {
throw new AuthenticationException();

Choose a reason for hiding this comment

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

AuthenticationException을 던지면 401 UNAUTHORIZED가 응답됩니다 :)
AuthenticationException클래스에 보면 @ResponseStatus(HttpStatus.UNAUTHORIZED) 어노테이션이 붙어있기 때문입니다!

Choose a reason for hiding this comment

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

AuthenticationPrincipalArgumentResolverresolveArgument에서 authentication 이 null인 경우는 로그인이 안된 케이스로 볼 수 있겠네요 :)

import static org.assertj.core.api.Assertions.assertThat;

@DisplayName("즐겨찾기")
public class FavoritesAcceptanceTest extends AcceptanceTest {

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.

로그인이 안되는 경우에는 accessToken을 ""로 넣어서 테스트를 했습니다.
다만, 비밀번호가 틀린 경우를 테스트를 하고 싶었는데..

음 🤔 .. 어떻게 구현해야할지 감이안오더라구요.
또 다른 Token을 만들어서 하자니 이게 Token이 있다는 것 자체가 이미 이메일, 패스워드가 일치한다는 전제하에
Token이 생성된거라.. 음 !!

어떻게 가능할까요!?

Choose a reason for hiding this comment

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

@minyul 로그인 password가 틀린 케이스는 현재 FavoriteAcceptanceTest에서 테스트할 부분이 아니고, AuthAcceptanceTest에서 테스트해보면 좋을 것 같네요 ;)

Copy link

@junwoochoi junwoochoi left a comment

Choose a reason for hiding this comment

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

안녕하세요 민율님!
피드백 적용해주신 부분 및 질문 주신 내용 잘 확인했습니다!
추가적으로 조금만 더 고민해주셨으면 하는 부분이 있어서 한번만 더 확인을 부탁드립니다 🙏

Comment on lines +34 to +36
if (ObjectUtils.isEmpty(authentication)) {
throw new AuthenticationException();
}

Choose a reason for hiding this comment

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

👍

accessToken = "";

// when
ExtractableResponse<Response> result = 즐겨찾기_목록_조회(accessToken);

Choose a reason for hiding this comment

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

👍

@JoinColumn(name = "member_id")
private Member member;
private Long member;

Choose a reason for hiding this comment

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

필드명이 memberId로 하는게 적합하지 않을까요?

Choose a reason for hiding this comment

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

@minyul

과거 신입 프로젝트로 로그인쪽을 구현하고 코드리뷰를 받았을 때, 사용자와 관련된 부분은 직접참조, 아닌 경우에는 간접 참조를 하라고 피드백을 받았었는데요! 어떻게 생각하시나요?

신입 프로젝트에서 어떤 상황이었는 지는 제가 정확히 알 지 못하여서 정확히 답변드리지 못하겠네요.
하지만 아마 피드백 주신 사수분의 의도가 항상 사용자가 관련된 부분은 무조건 직접참조하고, 그게 아니라면 간접 참조해라. 요런의미는 아니었을 것 같아요.

위에도 말씀드렸다시피 직접참조, 간접참조는 각자 장단점이 있습니다. 저 같은 경우는 사용자와 관련된 엔티티에서 다른 엔티티와 직접참조하면 바로 데이터를 가져와서 사용할 수 있는 장점 대비 다른 엔티티와 결합도가 높아지는 문제, N+1이 생길 위험을 항상 신경써야하는 문제가 더 크다고 생각해서 간접참조하거나, 상대방 엔티티 쪽에서 Member쪽으로 단방향 연관관계를 맺도록 하기를 선호하는 편입니다 😅

Comment on lines +17 to +23
@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "up_station_id")
private Station source;

@ManyToOne(cascade = CascadeType.PERSIST)
@JoinColumn(name = "down_station_id")
private Station target;

Choose a reason for hiding this comment

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

cascade는 없이 하는게 더 좋을 것 같아요.
Station의 라이프 사이클이 Favorite에 종속될 이유가 없을 것 같습니다!
ManyToOne의 경우 N+1을 피하기 위해 fetchType을 LAZY로 설정하시고 fetch join을 사용해보시면 좋을 것 같아요!

Choose a reason for hiding this comment

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

@minyul

음.. 구간(Section) 에 역 2개 (Upstation, DownStation) 이 있으며
즐겨찾기(Favorite)에 역 2개 (Source, Target) 이 있다.
결국엔 같지만 왜 @manytoone이지? 라는 의문이 살아지지가 않아서요.ㅠㅠㅠㅠㅠ

요 질문의 요지를 제가 잘 이해하지 못하겠습니다....... 😭
Section의 upStation, downStation은 한 섹션의 상행역과 바로 다음 하행역을 말하는 것이고,
Favorite은 source(출발지), target(도착지)라서 단순히 역을 두개 가지고 있다는 것만 같을 뿐 전혀 다른 관계라서요....
혹시 요건 따로 디엠이나 추가 댓글로 질의주시면 답변드리겠습니다!

import static org.assertj.core.api.Assertions.assertThat;

@DisplayName("즐겨찾기")
public class FavoritesAcceptanceTest extends AcceptanceTest {

Choose a reason for hiding this comment

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

@minyul 로그인 password가 틀린 케이스는 현재 FavoriteAcceptanceTest에서 테스트할 부분이 아니고, AuthAcceptanceTest에서 테스트해보면 좋을 것 같네요 ;)

Comment on lines +10 to +26
@Embeddable
public class Favorites {

@OneToMany(mappedBy = "member", cascade = {CascadeType.PERSIST, CascadeType.MERGE}, orphanRemoval = true)
private List<Favorite> favorites = new ArrayList<>();

public void add(Favorite favorite) {
this.favorites.add(favorite);
}

public void remove(Favorite favorite) {
this.favorites.remove(favorite);
}

public List<Favorite> getFavorites() {
return Collections.unmodifiableList(favorites);
}

Choose a reason for hiding this comment

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

더이상 쓰이지 않는 객체라 제거해도 좋을 것 같습니다 :)

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