Step2 리뷰 요청드립니다~ #346

sang-eun · 2022-08-15T15:41:02Z

감사합니다!

…onFilter 추상화 및 테스트

…상화 및 테스트 추가

…avorite into step2

pch8388

안녕하세요 🙇
2단계 구현 잘해주셨는 데, 요구사항이 모두 반영되지 못한 것 같아요 😢
코멘트 확인해주시고 수정 후 리뷰요청해주세요 🔥

pch8388 · 2022-08-16T13:07:36Z

src/main/java/nextstep/auth/authentication/Authenticator.java

+package nextstep.auth.authentication;
+
+import nextstep.member.application.UserDetailsService;
+import nextstep.member.domain.LoginMember;


아래 요구사항이 반영되지 않은 것 같아요 😢

pch8388 · 2022-08-16T13:08:33Z

src/main/java/nextstep/auth/token/TokenAuthenticationInterceptor.java

        this.jwtTokenProvider = jwtTokenProvider;
    }

    @Override
-    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
+    public AuthenticationToken convert(HttpServletRequest request) throws IOException {
        String content = request.getReader().lines().collect(Collectors.joining(System.lineSeparator()));
        TokenRequest tokenRequest = new ObjectMapper().readValue(content, TokenRequest.class);


ObjectMapper 는 생성 비용이 비싸다고 알려진 객체입니다. 의존성 주입을 통해 해결해볼 수 있을까요? 😄

pch8388 · 2022-08-16T13:09:41Z

src/main/java/nextstep/member/application/UserDetailsService.java

@@ -0,0 +1,8 @@
+package nextstep.member.application;


이 인터페이스가 member 패키지에 있는게 맞을까요? 🤔

만들면서 배우는 클린아키텍쳐를 보면 서비스의 인터페이스와 그 구현체를 같이 두기에 그게 익숙해서 member에 두었네요 ㅎㅎ
UserDetailService를 auth에서만 사용하지 않을 수도 있기에 구현체와 같이 두는게 한눈에 더 들어올 것 같습니다!

물론 인터페이스와 구현체의 구분은 폴더로라도 하면 좋은데 급하게 하느라... 🙏

해당 책에서의 구현의도는 단순히 인터페이스와 구현체를 같은 패키지에 두는 것은 아닙니다 😅
클린 아키텍처에서 로버트 C.마틴은 아래와 같은 원을 그려서 설명하는데요.
조금 단순화해서 말씀드리면 원 외부의 객체만이 원 내부의 객체에 의존해야 한다는 것을 주장합니다.
말씀하신 만들면서 배우는 클린아키텍처는 DIP를 통해 의존성 격리하기 위해 인터페이스를 두고, 해당 인터페이스를 내부의 객체가 구현하고, 외부의 객체는 그 인터페이스를 사용함으로써 외부의 객체가 내부의 실제 구현객체를 알지 못하게 되는 역할을 합니다.
지금 미션에서 요구하는 것도 사실은 DIP 를 통한 의존성 격리입니다 😄

참고 영상을 통해 공부해보시면 조금 더 이해가 쉬우실 것 같아요 🔥

네 말씀하신대로 원 외부의 객체만이 원 내부의 객체를 참조해야 한다는 것은 동의합니다.

그런데 현재 구조로는 의존성 격리가 충분히 되지 않았다는 말씀일까요? 현재도 member 과 auth간에 usecase를 통해 통신이 이루어지고 있기에, dip를 통해 구현체는 직접적으로 참조가 이뤄지지 않고 있어서 usecase를 어디에 둘 것인가는 부차적인 문제라고 생각했습니다. (그래서 그중 하나로 만들면서 배우는 클린아키텍쳐 내용을 예시로 든 것이고요. 첨부해주신 영상에서도 import문이 존재하기만 해도 의존관계가 있다고 하네요!)
미션이 그런 의미라면 수정하는건 문제없습니다만 궁금해서 질문드립니다 ㅎㅎ

클린아키텍처를 언급한건 상은님이 얘기하신 부분에 대해 추가적인 언급을 드리다보니 그렇게 되었네요 😅
클린아키텍처의 내용이 단순히 인터페이스와 구현체를 같은 패키지에 두라는 의미는 아니라고 생각해서 첨언을 드려봤습니다

현재 미션에 대해 이야기하면 auth 와 member 의 성질에 대해 고민해봐야하는 데요.
auth 는 인증/인가에 대한 책임을 가지는 비즈니스 규칙과는 조금 동떨어진 공통의 성질을 지닌 패키지라고 생각됩니다
반면에 member는 이 애플리케이션만의 비즈니스 규칙을 담을수도 있는 사용자에 대한 패키지라고 볼 수 있을 것 같아요
이에 따라 auth -> member 로의 의존성이 생기지 않도록 관리하는 것이 대부분의 상황에서 유리할 거라고 생각합니다.
또한 현재는 MemberController 가 @AuthenticationPrincipal를 사용하고, LoginMember가 User를 구현하기 때문에 패키지 간의 순환참조가 생겼습니다.
이를 끊어내기 위해 DIP를 사용하라고 말씀 드린 것이었습니다. 😅

정리하자면, 미션의 의도는 DIP를 통해 (다른 방법이 있으면 쓰셔도 됩니다) 패키지간 순환참조를 끊어내자 입니다.
영상에서 보신 것과 같이 auth 나 member 둘 중의 한곳에서는 다른 패키지의 import 가 없도록 수정해보시길 바랍니다.

예시를 들어주신 만들면서 배우는 클린 아키텍처에서도 application 패키지의 인터페이스를 adapter 에서 구현 하는 형태로 개발을 하고 있습니다. 의존성 격리를 위해 DIP를 사용하면 개념상으로는 이것 저것 이야기 할 수 있겠지만, 결국엔 내부의 객체들이 외부에 의존하지 않도록 만드는 여러가지 방법들이라고 생각합니다 😄

MemberController 가 @AuthenticationPrincipal를 사용하고, LoginMember가 User를 구현하기 때문에 패키지 간의 순환참조가 생겼습니다.

여기에 참조가 있다는건 생각못했네요! 넵 수정하겠습니다

예시를 들어주신 만들면서 배우는 클린 아키텍처에서도 application 패키지의 인터페이스를 adapter 에서 구현 하는 형태로 개발을 하고 있습니다

넵 처음 댓글에서 말씀드렸듯이 물론 인터페이스와 구현체의 구분은 폴더로라도 하면 좋은데 급하게 하느라... 🙏 😂 앞으로 참고하겠습니다. 지금까지 긴 설명 감사드립니다!

저도 토론하며 재미있었습니다
참고 의 예제의 의존관계를 직접 그림으로 그려보시면 더 좋은 인사이트를 얻으 실 수 있을것 같아요 👍

pch8388 · 2022-08-16T13:22:09Z

src/test/java/nextstep/subway/unit/TokenAuthenticationInterceptorMockTest.java

+
+        boolean result = interceptor.preHandle(createMockRequest(), response, null);
+
+        assertThat(result).isFalse();


이런형태로 검증도 가능합니다 😄

Suggested change

boolean result = interceptor.preHandle(createMockRequest(), response, null);

assertThat(result).isFalse();

given(jwtTokenProvider.createToken(any(), any())).willReturn(JWT_TOKEN);

boolean result = interceptor.preHandle(createMockRequest(), response, new Object());

assertThat(result).isFalse();

assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());

assertThat(response.getContentAsString()).isEqualTo(mapper.writeValueAsString(new TokenResponse(JWT_TOKEN)));

bool만 확인하기에는 부족하다고 생각했는데 좋네요! 감사합니다.

pch8388 · 2022-08-16T13:23:13Z

src/test/java/nextstep/subway/acceptance/MemberAcceptanceTest.java

+        assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
+        assertThat(member.jsonPath().getString("email")).isEqualTo(newEmail);


인수테스트를 읽는 사람이 누구인가에 대해 고민해봐야 하는데요.
클린 애자일의 구절을 인용하면

인수테스트는 업무 분석가와 QA, 개발자가 함께 힘을 모아 작성한다.

이와 같이 인수테스트를 읽는 사람이 모두 개발자는 아닐 수 있다고 생각합니다 😃.

넵ㅎㅎ 좀더 한글화가 들어가면 좋을 것 같군요

pch8388 · 2022-08-16T13:23:28Z

src/test/java/nextstep/subway/utils/SecurityUtil.java

    public static RequestSpecification given() {
+        String token = 로그인_되어_있음(MemberData.admin.getEmail(), MemberData.admin.getPassword());


pch8388

리팩토링 잘해주셨지만, 요구사항에 대해 조금 더 확인이 필요할 것 같아요 😢
코멘트 확인하고 수정 후 리뷰요청 해주세요 🔥

pch8388 · 2022-08-18T16:16:18Z

src/main/java/nextstep/auth/authentication/Authenticator.java

@@ -1,7 +1,7 @@
 package nextstep.auth.authentication;

 import nextstep.member.application.UserDetailsService;
-import nextstep.member.domain.LoginMember;
+import nextstep.member.domain.User;


미션은 auth 패키지 내에 라이브러리를 제외한 비즈니스 패키지들이 들어오지 않아야 완료됩니다 😢
현재 auth 패키지가 member 에 의존하고 있어요

pch8388 · 2022-08-18T16:17:10Z

src/main/java/nextstep/auth/authentication/LoginMember.java

@@ -1,9 +1,12 @@
-package nextstep.member.domain;
+package nextstep.auth.authentication;


LoginMember 를 옮기시지 않아요 됩니다 😢

pch8388 · 2022-08-18T16:24:53Z

src/main/java/nextstep/member/application/UserDetailsService.java

@@ -0,0 +1,8 @@
+package nextstep.member.application;


해당 책에서의 구현의도는 단순히 인터페이스와 구현체를 같은 패키지에 두는 것은 아닙니다 😅
클린 아키텍처에서 로버트 C.마틴은 아래와 같은 원을 그려서 설명하는데요.
조금 단순화해서 말씀드리면 원 외부의 객체만이 원 내부의 객체에 의존해야 한다는 것을 주장합니다.
말씀하신 만들면서 배우는 클린아키텍처는 DIP를 통해 의존성 격리하기 위해 인터페이스를 두고, 해당 인터페이스를 내부의 객체가 구현하고, 외부의 객체는 그 인터페이스를 사용함으로써 외부의 객체가 내부의 실제 구현객체를 알지 못하게 되는 역할을 합니다.
지금 미션에서 요구하는 것도 사실은 DIP 를 통한 의존성 격리입니다 😄

참고 영상을 통해 공부해보시면 조금 더 이해가 쉬우실 것 같아요 🔥

sang-eun added 9 commits August 15, 2022 14:37

[add] form login unit test, acceptance test, 구현

c42932d

[add] bearer login acceptance test, 구현

bbd2e96

[add] 권한 기능

bc8efda

[add] 권한 기능 체크 인수테스트

fde6d3a

[modify] TokenAuthenticationInterceptor, UsernamePasswordAuthenticati…

a3ef715

…onFilter 추상화 및 테스트

[modify] BasicAuthenticationFilter, BearerTokenAuthenticationFilter 추…

c0c4ab3

…상화 및 테스트 추가

[modify] auth에서 LoginMemberService 의존성 제거

d7c23e2

[modify] step1 코드리뷰 반영

54cdba1

Merge branch 'sang-eun' of https://github.com/next-step/atdd-subway-f…

bcf2327

…avorite into step2

pch8388 requested changes Aug 16, 2022

View reviewed changes

[modify] 코드리뷰 반영

533ccdf

pch8388 requested changes Aug 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step2 리뷰 요청드립니다~ #346

Step2 리뷰 요청드립니다~ #346

sang-eun commented Aug 15, 2022

pch8388 left a comment

pch8388 Aug 16, 2022

pch8388 Aug 16, 2022

pch8388 Aug 16, 2022

sang-eun Aug 18, 2022 •

edited

Loading

pch8388 Aug 18, 2022

sang-eun Aug 20, 2022 •

edited

Loading

pch8388 Aug 20, 2022 •

edited

Loading

pch8388 Aug 20, 2022

sang-eun Aug 20, 2022

pch8388 Aug 20, 2022

pch8388 Aug 16, 2022

sang-eun Aug 18, 2022

pch8388 Aug 16, 2022

sang-eun Aug 18, 2022

pch8388 Aug 16, 2022

pch8388 left a comment

pch8388 Aug 18, 2022

pch8388 Aug 18, 2022

pch8388 Aug 18, 2022


		boolean result = interceptor.preHandle(createMockRequest(), response, null);

		assertThat(result).isFalse();

-        boolean result = interceptor.preHandle(createMockRequest(), response, null);
-        assertThat(result).isFalse();
+       given(jwtTokenProvider.createToken(any(), any())).willReturn(JWT_TOKEN);
+        boolean result = interceptor.preHandle(createMockRequest(), response, new Object());
+        assertThat(result).isFalse();
+        assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
+        assertThat(response.getContentAsString()).isEqualTo(mapper.writeValueAsString(new TokenResponse(JWT_TOKEN)));

		assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value());
		assertThat(member.jsonPath().getString("email")).isEqualTo(newEmail);

		public static RequestSpecification given() {
		String token = 로그인_되어_있음(MemberData.admin.getEmail(), MemberData.admin.getPassword());

		@@ -1,9 +1,12 @@
		package nextstep.member.domain;
		package nextstep.auth.authentication;

Step2 리뷰 요청드립니다~ #346

Are you sure you want to change the base?

Step2 리뷰 요청드립니다~ #346

Conversation

sang-eun commented Aug 15, 2022

pch8388 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sang-eun Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sang-eun Aug 20, 2022 • edited Loading

Choose a reason for hiding this comment

pch8388 Aug 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pch8388 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sang-eun Aug 18, 2022 •

edited

Loading

sang-eun Aug 20, 2022 •

edited

Loading

pch8388 Aug 20, 2022 •

edited

Loading