[Spring MVC (인증)] 정원영 미션 제출합니다. #122

gardenzeeero · 2025-01-26T06:54:29Z

2단계 3단계 미션 제출입니다.

구현하면서 한 고민

Admin 개념

현재 URL을 /admin/** 로 admin 페이지에 대한 경로를 정의하고 있음
하지만 이것을 admin이라는 개념으로 보기엔 어려운 것 같음
그렇게 되면 admin 패키지에 admin 관련 내용들이 들어가게 됨
admin 권한이더라도 패키지는 각각의 도메인에 들어가야한다고 생각함 adminReservation은 reservation 패키지에
근데 현재 admin에 대한 전체 interceptor 구현임
따라서 interceptor라는 패키지를 하나 새로 만들어서 구현함

추가로 궁금한 점

현재 interceptor에서 서비스를 이용하기 때문에 서비스에서 터지는 예외는 고려하지 못함
-> 괜찮은 건지 잘 모르겠음. 인터셉터가 애초에 접근하지 못하도록 막는건데 예외의 종류를 알려주는게 옳은 것인지

2단계 피드백으로 record의 assertThat.isEqualTo 로 비교가능할 것 같다고 하셨는데 id가 할당 되기 전이라 record 동등 비교는 불가능할 것 같습니다.. 2단계 피드백 주석으로 남겨뒀습니다!

youngsu5582

원영님 이번에도 코드 잘 작성 해주셨네요!
가벼운 MVC 적인 요소부터 리뷰 시작해보겠습니다!

Admin 개념

이 내용은 제가 직접 설명하는 것보다
Admin 모듈 구현 방식 + Admin 은 도메인일까?
해당 내용에서 자신만의 관점을 만들수 있을거 같아서 남깁니다.

현재 interceptor에서 서비스를 이용하기 때문에 서비스에서 터지는 예외는 고려하지 못함
-> 괜찮은 건지 잘 모르겠음. 인터셉터가 애초에 접근하지 못하도록 막는건데 예외의 종류를 알려주는게 옳은 것인지

해당 내용은 어떤 면에서 이렇게 느꼈는지 설명해주시면 추가적인 답변 달겠습니다! 🫡

youngsu5582 · 2025-01-27T14:03:32Z

src/test/java/roomescape/MissionStepTest.java

+                .get("/admin")
+                .then().log().all()
+                .statusCode(200);
+    }
 }


파일들에 개행 오류가 보이는데
https://hyeon9mak.github.io/github-no-newline-at-a-end-of-file/
해당 블로그 참고해주세용 🙂

이런게 있는지는 몰랐네요! 수정하겠습니다!

youngsu5582 · 2025-01-27T14:07:15Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
+        String token = extractToken(request);
+
+        if (token == null) {


해당 부분에서 null 을 통해 확인하는 것보다 Optional 을 그대로 넘겨줘도 괜찮겠다는 생각은 드는데 어떤가요?

동의합니다! extractToken을 사용하는 입장에서는 nullable을 확실하게 아는 것이 좋겠네요!

youngsu5582 · 2025-01-27T14:08:09Z

src/test/java/roomescape/interceptor/AdminInterceptorTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+
+@SpringBootTest
+@Transactional


이 부분도 2단계 남긴 피드백 보고 의견 남겨주세요~

Transactional에 대해 많은 자료를 찾아봤는데 영속성 컨텍스트 때문에 발생하는 유효하지 않은 테스트가 가장 큰 문제더군요!
이에 대한 명확한 답보다는 주의해서 쓰자 vs 어떠한 방식으로든 명시적 선언이 낫다 인것 같아요

근데 확실히 클래스 레벨에서 메서드 레벨로 내릴 필요는 있는 것 같네요. 주의할 점이 있다면 메서드마다 확인 할 수 있도록 표시하는게 맞는 것 같아요. 추가 테스트를 작성 할 때도 트랜잭션에 대해 한번 더 고민하게 되구요

테스트외에 서비스의 Transactional에 대해서도 코멘트를 남겨주셨네요.
엔티티의 특정 상태를 바꾼다라는 말이 조금 애매한 것 같아요. 단순 조회, 단건 조회가 아닌 경우에 Transactional이 사용된다로 이해할 수 있을까요? (진짜 상태만 바꾸는 거라면 모든 행위에 해당되지 않나 라는 생각입니다. 조회도 비영속 -> 영속이니)

youngsu5582 · 2025-01-27T14:11:11Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+
+import java.util.Arrays;
+
+public class AdminInterceptor implements HandlerInterceptor {


Interceptor 에는 preHandle 말고 또 어떤 메소드들을 오버라이딩 할 수 있나요?

Intercetpor는 총 3개의 메소드를 제공합니다. preHandle, postHandle, afterCompletion이 있습니다.

postHandle과 afterCompletion의 차이는 예외 상황과 실행시점에 있습니다.

postHandle의 경우 예외가 터지면 실행이 되지 않는 반면 afterCompletion은 어떤 상황에서도 실행이 됩니다.

postHandle은 핸들러 어댑터가 실행 직후 실행되는 반면 afterCompletion은 뷰가 렌더링 된 이후에 실행됩니다.

youngsu5582 · 2025-01-27T14:11:27Z

src/main/java/roomescape/config/WebConfig.java

    @Override
    public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
        resolvers.add(new LoginMemberArgumentResolver(authService));
    }
+
+    @Override
+    public void addInterceptors(InterceptorRegistry registry) {


order가 1인 이유는 뭔가요?

만약 어느 자리에 들어가도 되는 Inteceptor라면 order를 넣지 않았을 것입니다. 하지만 권한과 관련된 로직은 어느 순서에 들어가냐에 따라 다르게 흘러갈 수 있습니다. (예외 등)
따라서 다음 Interceptor를 만들때 해당 Interceptor와의 순서도 고려해야한다는 의미로 남겨두었습니다.

youngsu5582 · 2025-01-27T16:09:01Z

src/main/java/roomescape/auth/LoginMemberArgumentResolver.java

@@ -11,6 +11,7 @@

 import java.util.Arrays;

+//2단계


해당 부분에 AuthService 가 있고 이에 대해서도 고민을 해준걸로 파악했어요.
저도 해당 요소는 결국 비즈니스 로직이 아닌, 웹의 요청을 처리하기 위한 클래스이므로 괜찮다고 결론을 내렸어요.

추가적으로,

AuthService 를 통해 멤버를 찾고

Service 단 로직에서 또 멤버를 찾아서 로직을 수행한다.
로 파악을 했는데 이때 LoginMember 내부에 Member 를 가져도 되는거에 대해선 어떻게 생각하나요?
( 그러면, 다시 한번 더 조회를 할 필요는 없겠죠 )

의견을 들려주세용

ArgumentResolver는 공통 관심사의 분리를 위해 만들어진 도구라고 생각합니다.Service단에서 멤버를 또 찾아서 로직을 수행은 ArgumentResolver의 잘못된 사용이라고 생각합니다. 두가지 상황을 생각해볼 수 있습니다.

다른 Service에서 Member를 조회한다
이 상황은 파라미터로 필요한 정보를 못받아서 DB를 조회하는 상황입니다. ArgumentResolver를 통해 필요한 정보를 파라미터로 받았다면 추가적인 Member 조회 로직은 없을 것입니다.

파라미터로 정보를 받았지만 부족하다.
Controller와 Service 자체가 잘못 설계된 것이라고 생각합니다. 우선 Service는 완전한 정보를 받을 수 없는데 파라미터로 정보를 받고 다시 조회를 하는 상황이 이상합니다. Controller 또한 완전한 정보를 줄 수 없는데 ArgumentResolver를 사용하는 것이 이상합니다.
이런 경우 그냥 서비스에서 필요한 정보를 조회(Repository 사용)하거나 내부에서 다른 서비스를 통해 조회하거나 ArgumentResolver로 모든 정보를 추출하고 서비스에 제공하는게 맞는 것 같습니다.

장황하게 설명했지만 CookieValue를 생각하면 될 것 같습니다. Cookie를 추출하는 과정이 매번 동일하기 때문에 ArgumentResolver로 Cookie를 추출했습니다. 하지만 Controller에도 똑같은 Cookie를 또 추출한다는 것은 이상합니다. ArgumentResolver를 사용하지 않거나, 처음부터 Controller에서 Cookie를 추출해야하는 것이죠.

추가로, AuthService가 제공하는 정보의 양 > LoginMember가 제공하는 정보의 양인 상황에 내부에 Member를 가지는 것이 유효한 것 같습니다. 근데 개인적으론 굳이 그렇게 만들 필요가 있을까? 라는 생각입니다. 오히려 LoginMember가 제공하는 정보의 양이 더 많아야하는 것 아닌가? 라는 생각입니다.

youngsu5582 · 2025-01-27T16:13:19Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+        }
+
+        MemberDetailResponse member = authService.loginCheckWithToken(token);
+        if (!member.role().equals("ADMIN")) {


해당 부분도 Member 가 스스로가 어드민인지 알려줄 수 있을거 같아요.

그게 더 좋은 방식인 것 같네요! 생각하지 못했네요 수정하겠습니다!

youngsu5582 · 2025-01-27T16:14:39Z

src/main/java/roomescape/auth/AuthService.java

@@ -27,6 +27,7 @@ public String loginWithEmailAndPassword(String email, String password) {
        Member member = null;
        try {
            validatePasswordByEmail(email, password);
+            member = memberDao.findByEmailAndPassword(email, password);


해당 로직 내부에서 return 문 까지 하지 않은 이유가 있나요?

아래와 같이 한줄로 줄일 수 있긴 합니다.
return tokenService.createToken(MemberTokenDto.fromEntity(memberDao.findByEmailAndPassword(email, password)))

하지만 try-catch로 예외를 핸들링하고 있어서 핸들링하는 예외가 터지는 곳만 try 안에 넣는게 좋다는 생각을 했습니다. tokenService에서 발생하는 예외는 잡지 않는다는 의도였습니다.

gardenzeeero

현재 interceptor에서 서비스를 이용하기 때문에 서비스에서 터지는 예외는 고려하지 못함
-> 괜찮은 건지 잘 모르겠음. 인터셉터가 애초에 접근하지 못하도록 막는건데 예외의 종류를 알려주는게 옳은 것인지

제 의도는 관리자가 아니라 접근이 안되는 것인데 다른 이유들을 보여주는 것이 맞는 가에 대한 고민이었습니다. (try-catch로 다른 예외들을 처리했어야하나)

질문하고 보니 어차피 인증쪽 로직이라 보여도 상관없겠다는 생각이 들었네요 해결되었습니다 👍

gardenzeeero · 2025-01-31T04:03:59Z

src/main/java/roomescape/auth/AuthService.java

@@ -27,6 +27,7 @@ public String loginWithEmailAndPassword(String email, String password) {
        Member member = null;
        try {
            validatePasswordByEmail(email, password);
+            member = memberDao.findByEmailAndPassword(email, password);


아래와 같이 한줄로 줄일 수 있긴 합니다.
return tokenService.createToken(MemberTokenDto.fromEntity(memberDao.findByEmailAndPassword(email, password)))

하지만 try-catch로 예외를 핸들링하고 있어서 핸들링하는 예외가 터지는 곳만 try 안에 넣는게 좋다는 생각을 했습니다. tokenService에서 발생하는 예외는 잡지 않는다는 의도였습니다.

gardenzeeero · 2025-01-31T06:11:34Z

src/main/java/roomescape/auth/LoginMemberArgumentResolver.java

@@ -11,6 +11,7 @@

 import java.util.Arrays;

+//2단계


ArgumentResolver는 공통 관심사의 분리를 위해 만들어진 도구라고 생각합니다.Service단에서 멤버를 또 찾아서 로직을 수행은 ArgumentResolver의 잘못된 사용이라고 생각합니다. 두가지 상황을 생각해볼 수 있습니다.

다른 Service에서 Member를 조회한다
이 상황은 파라미터로 필요한 정보를 못받아서 DB를 조회하는 상황입니다. ArgumentResolver를 통해 필요한 정보를 파라미터로 받았다면 추가적인 Member 조회 로직은 없을 것입니다.

파라미터로 정보를 받았지만 부족하다.
Controller와 Service 자체가 잘못 설계된 것이라고 생각합니다. 우선 Service는 완전한 정보를 받을 수 없는데 파라미터로 정보를 받고 다시 조회를 하는 상황이 이상합니다. Controller 또한 완전한 정보를 줄 수 없는데 ArgumentResolver를 사용하는 것이 이상합니다.
이런 경우 그냥 서비스에서 필요한 정보를 조회(Repository 사용)하거나 내부에서 다른 서비스를 통해 조회하거나 ArgumentResolver로 모든 정보를 추출하고 서비스에 제공하는게 맞는 것 같습니다.

장황하게 설명했지만 CookieValue를 생각하면 될 것 같습니다. Cookie를 추출하는 과정이 매번 동일하기 때문에 ArgumentResolver로 Cookie를 추출했습니다. 하지만 Controller에도 똑같은 Cookie를 또 추출한다는 것은 이상합니다. ArgumentResolver를 사용하지 않거나, 처음부터 Controller에서 Cookie를 추출해야하는 것이죠.

추가로, AuthService가 제공하는 정보의 양 > LoginMember가 제공하는 정보의 양인 상황에 내부에 Member를 가지는 것이 유효한 것 같습니다. 근데 개인적으론 굳이 그렇게 만들 필요가 있을까? 라는 생각입니다. 오히려 LoginMember가 제공하는 정보의 양이 더 많아야하는 것 아닌가? 라는 생각입니다.

gardenzeeero · 2025-01-31T06:21:16Z

src/main/java/roomescape/config/WebConfig.java

    @Override
    public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
        resolvers.add(new LoginMemberArgumentResolver(authService));
    }
+
+    @Override
+    public void addInterceptors(InterceptorRegistry registry) {


만약 어느 자리에 들어가도 되는 Inteceptor라면 order를 넣지 않았을 것입니다. 하지만 권한과 관련된 로직은 어느 순서에 들어가냐에 따라 다르게 흘러갈 수 있습니다. (예외 등)
따라서 다음 Interceptor를 만들때 해당 Interceptor와의 순서도 고려해야한다는 의미로 남겨두었습니다.

gardenzeeero · 2025-01-31T06:25:03Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+
+import java.util.Arrays;
+
+public class AdminInterceptor implements HandlerInterceptor {


Intercetpor는 총 3개의 메소드를 제공합니다. preHandle, postHandle, afterCompletion이 있습니다.

postHandle과 afterCompletion의 차이는 예외 상황과 실행시점에 있습니다.

postHandle의 경우 예외가 터지면 실행이 되지 않는 반면 afterCompletion은 어떤 상황에서도 실행이 됩니다.

postHandle은 핸들러 어댑터가 실행 직후 실행되는 반면 afterCompletion은 뷰가 렌더링 된 이후에 실행됩니다.

gardenzeeero · 2025-01-31T06:26:13Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception {
+        String token = extractToken(request);
+
+        if (token == null) {


동의합니다! extractToken을 사용하는 입장에서는 nullable을 확실하게 아는 것이 좋겠네요!

gardenzeeero · 2025-01-31T07:19:01Z

src/main/java/roomescape/interceptor/AdminInterceptor.java

+        }
+
+        MemberDetailResponse member = authService.loginCheckWithToken(token);
+        if (!member.role().equals("ADMIN")) {


그게 더 좋은 방식인 것 같네요! 생각하지 못했네요 수정하겠습니다!

gardenzeeero · 2025-01-31T07:21:57Z

src/test/java/roomescape/MissionStepTest.java

+                .get("/admin")
+                .then().log().all()
+                .statusCode(200);
+    }
 }


이런게 있는지는 몰랐네요! 수정하겠습니다!

gardenzeeero · 2025-01-31T09:42:40Z

src/test/java/roomescape/interceptor/AdminInterceptorTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+
+@SpringBootTest
+@Transactional


Transactional에 대해 많은 자료를 찾아봤는데 영속성 컨텍스트 때문에 발생하는 유효하지 않은 테스트가 가장 큰 문제더군요!
이에 대한 명확한 답보다는 주의해서 쓰자 vs 어떠한 방식으로든 명시적 선언이 낫다 인것 같아요

근데 확실히 클래스 레벨에서 메서드 레벨로 내릴 필요는 있는 것 같네요. 주의할 점이 있다면 메서드마다 확인 할 수 있도록 표시하는게 맞는 것 같아요. 추가 테스트를 작성 할 때도 트랜잭션에 대해 한번 더 고민하게 되구요

테스트외에 서비스의 Transactional에 대해서도 코멘트를 남겨주셨네요.
엔티티의 특정 상태를 바꾼다라는 말이 조금 애매한 것 같아요. 단순 조회, 단건 조회가 아닌 경우에 Transactional이 사용된다로 이해할 수 있을까요? (진짜 상태만 바꾸는 거라면 모든 행위에 해당되지 않나 라는 생각입니다. 조회도 비영속 -> 영속이니)

gardenzeeero added 8 commits January 26, 2025 15:08

feat: 어드민 페이지 interceptor 구현

dd4df6f

feat: 어드민 페이지 interceptor 등록 구현

15068ce

fix: email과 password로 로그인 시 회원정보를 반환

a745575

refactor: jwt key를 불변으로 변경

dc78e2b

refactor: Configuration 어노테이션을 Component로 변경

20e888c

style: 2단계 주석 추가

470fe70

test: adminInterceptor 테스트 작성

95bc2ec

style: 2단계 피드백 주석 추가

a711900

youngsu5582 suggested changes Jan 27, 2025

View reviewed changes

gardenzeeero commented Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spring MVC (인증)] 정원영 미션 제출합니다. #122

[Spring MVC (인증)] 정원영 미션 제출합니다. #122

gardenzeeero commented Jan 26, 2025 •

edited

Loading

youngsu5582 left a comment

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

youngsu5582 Jan 27, 2025

gardenzeeero Jan 31, 2025

gardenzeeero left a comment

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025

gardenzeeero Jan 31, 2025


		import java.util.Arrays;

		public class AdminInterceptor implements HandlerInterceptor {

[Spring MVC (인증)] 정원영 미션 제출합니다. #122

Are you sure you want to change the base?

[Spring MVC (인증)] 정원영 미션 제출합니다. #122

Conversation

gardenzeeero commented Jan 26, 2025 • edited Loading

구현하면서 한 고민

추가로 궁금한 점

youngsu5582 left a comment

Choose a reason for hiding this comment

Admin 개념

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

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

gardenzeeero 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

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

gardenzeeero commented Jan 26, 2025 •

edited

Loading