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

[Spring Core] (배포) 정상희 미션 제출합니다. #116

Open
wants to merge 40 commits into
base: sangheejeong
Choose a base branch
from

Conversation

SANGHEEJEONG
Copy link

@SANGHEEJEONG SANGHEEJEONG commented Jan 15, 2025

안녕하세요!
드디어 마지막이라니 시원섭섭하네요(시원한 게 더 큰 것 같기도 ㅎㅎ)
사실 이번 미션 자체는 오래 걸리지 않았는데 이전 리뷰 반영하다가 오류난 부분들을 고치다 보니 조금 늦게 제출한 것 같습니다 ㅜㅜ

이번 미션을 진행하면서 생긴 문제랑 궁금증이 있는데

JWT 관련 로직을 roomescape와 같은 계층의 auth 패키지의 클래스로 분리하세요.

7단계 미션에서 auth 패키지를 roomescape와 같은 계층으로 옮기라는 말이 있었는데요
그래서 auth 패키지의 Config 클래스에 @Configuration을 붙이고 다른 클래스를 수동으로 빈 등록을 했는데 인텔리제이에서 자꾸 빨간 줄이 뜨더라구요. @SpringBootApplication이 선언된 클래스가 위치한 패키지와 그 하위 패키지까지가 기본 컴포넌트 스캔 범위라서 동등한 패키지에서는 작동하지 않았던 것 같습니다. @Configuration@Component를 포함하고 있고 컴포넌트 스캔 범위 내에 있어야 작동하는 거라고 이해했는데 제가 잘 이해한 게 맞을까요?

근데 여기서

  1. 굳이 왜 @Component를 사용하지 않고 @Configuration을 사용하는 게 요구사항인지 궁금했습니다. 약간 후자를 사용하면 싱글톤이 보장된다고 하는데 인증 과정을 왜 싱글톤으로 하는지에 대한 의문이 좀 들었습니다.
  2. auth 패키지를 roomescape와 동등한 패키지로 만들라는 것이 @Configuration 을 통해 빈을 등록하는 연습을 위한 건지 아니면 auth라는 인증 패키지는 원래 대부분 패키지를 기본 패키지와 동등한 계층으로 만들어 사용하는지 궁금합니다. 만약 후자라면 이유도 궁금합니다.

적고보니까 1,2번이 같은 질문이네요 한 번에 대답해주셔도 될 것 같습니다.

앗 그리고 조금 사소한 질문일 수 있지만,, 궁금해서 질문합니다.
제가 단위 테스트를 전체가 다 통과해야 되는 줄 알았는데 요구사항이 추가되면서 당연히 실패할 수밖에 없는 테스트가 있더라구요 (그걸 까먹고 계속 돌리고 있었어요 하하..) 예를 들어 예약 중복로직 추가하기 전 테스트와 추가한 후 테스트는 데이터가 달라야 예약이 성공한다고 뜨잖아요, 근데 이렇게 계속 코드가 변하는 와중에 보통 테스트 코드도 같이 수정을 하나요? 아니면 그때그때 새로운 테스트를 생성해서 테스트 한 후 이전 테스트는 그냥 냅두나요? (아니면 이전 테스트는 삭제나 주석처리..?)

…basic-roomescape-playground into sanghee-jpa

# Conflicts:
#	src/main/java/roomescape/auth/AuthRoleInterceptor.java
#	src/main/java/roomescape/member/Member.java
#	src/main/java/roomescape/member/MemberService.java
#	src/main/java/roomescape/reservation/MyReservationResponse.java
#	src/main/java/roomescape/reservation/Reservation.java
#	src/main/java/roomescape/reservation/ReservationController.java
#	src/main/java/roomescape/reservation/ReservationRepository.java
#	src/main/java/roomescape/reservation/ReservationService.java
#	src/main/java/roomescape/waiting/Waiting.java
#	src/main/java/roomescape/waiting/WaitingController.java
#	src/main/java/roomescape/waiting/WaitingService.java
Copy link

@kokodak kokodak left a comment

Choose a reason for hiding this comment

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

안녕하세요 상희님~! 마지막 미션을 함께 하게 돼서 영광이에요. 긴 기간동안 너무 고생 많으셨습니다.

그동안 미션을 잘 진행해주셨다는 게 코드에서 보여서 너무 좋았습니다. 전반적으로 깔끔하게 잘 작성된 것 같아요.

아래에는 상희님이 남겨주신 질문에 대한 대답을 남겨 놓을게요.

7단계 미션에서 auth 패키지를 roomescape와 같은 계층으로 옮기라는 말이 있었는데요
그래서 auth 패키지의 Config 클래스에 @configuration을 붙이고 다른 클래스를 수동으로 빈 등록을 했는데 인텔리제이에서 자꾸 빨간 줄이 뜨더라구요. @SpringBootApplication이 선언된 클래스가 위치한 패키지와 그 하위 패키지까지가 기본 컴포넌트 스캔 범위라서 동등한 패키지에서는 작동하지 않았던 것 같습니다. @configuration@component를 포함하고 있고 컴포넌트 스캔 범위 내에 있어야 작동하는 거라고 이해했는데 제가 잘 이해한 게 맞을까요?

네, 잘 이해하신 것 같아요. 그동안은 @SpringBootApplication 가 선언되어 있는 패키지 하위에서 모든 작업을 했었기 때문에 컴포넌트 스캔에 문제가 없었지만, 이 미션에서는 동일한 레벨의 패키지가 생기는 바람에 컴포넌트 스캔 범위에서 벗어나 문제가 생긴 것 같아요.

굳이 왜 @component를 사용하지 않고 @configuration을 사용하는 게 요구사항인지 궁금했습니다. 약간 후자를 사용하면 싱글톤이 보장된다고 하는데 인증 과정을 왜 싱글톤으로 하는지에 대한 의문이 좀 들었습니다.
auth 패키지를 roomescape와 동등한 패키지로 만들라는 것이 @configuration 을 통해 빈을 등록하는 연습을 위한 건지 아니면 auth라는 인증 패키지는 원래 대부분 패키지를 기본 패키지와 동등한 계층으로 만들어 사용하는지 궁금합니다. 만약 후자라면 이유도 궁금합니다.

이에 대한 내용은 코드 코멘트에 남겨두었어요. 내용이 잘 이해가 가지 않는다면 페어를 해도 좋고, 깃허브로 코멘트를 추가로 나눠도 좋을 것 같습니다.

제가 단위 테스트를 전체가 다 통과해야 되는 줄 알았는데 요구사항이 추가되면서 당연히 실패할 수밖에 없는 테스트가 있더라구요 (그걸 까먹고 계속 돌리고 있었어요 하하..) 예를 들어 예약 중복로직 추가하기 전 테스트와 추가한 후 테스트는 데이터가 달라야 예약이 성공한다고 뜨잖아요, 근데 이렇게 계속 코드가 변하는 와중에 보통 테스트 코드도 같이 수정을 하나요? 아니면 그때그때 새로운 테스트를 생성해서 테스트 한 후 이전 테스트는 그냥 냅두나요? (아니면 이전 테스트는 삭제나 주석처리..?)

매우 좋은 질문이라고 생각해요. 이 질문에 대한 답변에 앞서서, 귀찮은 테스트 코드를 굳이 작성해야하는 이유가 무엇인지 고민해볼 필요가 있어요.

상희님이 미션을 진행하면서 한번쯤 느끼셨을 것 같은데, 요구사항이 계속 변경되면서 기존 코드를 리팩터링을 하게 되면, 기존에 잘 동작하던 기능들이 여전히 잘 동작하는지 체크할 필요가 있어요.

예를 들면, JDBC에서 JPA로 마이그레이션 하는 과정이 있을 것 같아요. 그런 대규모의 기술 마이그레이션을 진행했을 경우, JPA로 옮기고 나서의 기능들은 JDBC를 사용했을 때와 모두 동일하게 동작해야 해요.

문제는, 기존의 기능들이 여전히 모두 잘 동작하고 있는가? 를 어떻게 검증할 것인지 인데요.

기능의 규모가 작다면 직접 사용자가 되어서 테스트 해볼 수 있겠지만, 기능이 많다면 그러기 쉽지 않을 거예요. 무엇보다도, 검증의 자동화가 불가능하겠죠. 매번 수동으로 테스트해야하니까요.

이런 문제를 해결하기 위해서, 상희님이 만드신 기능을 커버하는 양질의 테스트 코드를 작성해야 해요. 이렇게 되면, 기능을 새로 추가하거나 리팩터링 할 때마다, 기존의 테스트 코드를 실행해서 기능이 깨지는 부분이 있는지 쉽고 빠르게 검증할 수 있어요.

코드를 전체적으로 한번 갈아 엎었는데, 테스트를 돌렸더니 몇 개가 실패한다면, 깨지는 테스트 코드를 성공할 수 있도록 바꿔주어야 해요. 즉, 기능을 추가하거나 변경할 때 코드의 변경 범위는, 프로덕션 코드 뿐 아니라 테스트 코드도 항상 포함되어야 합니다. 현재 상황에 맞게 테스트도 항상 최신화가 되어야 해요.

결론을 정리해보자면, 프로덕션 코드가 변화하게 되면 테스트 코드 또한 그에 맞게 함께 변화되어야 해요. 그리고 작성한 모든 테스트는 반드시 모두 통과해야 합니다. 하나라도 깨지는 테스트가 있다면, 이는 테스트 코드가 outdated 되었거나, 프로덕션 코드가 잘못되었다는 것을 의미하기 때문에, 둘 중 무언가는 반드시 수정이 되어야 해요~!

import roomescape.exception.InvalidTokenException;
import roomescape.exception.MissingTokenException;
import roomescape.member.Member;

import java.util.Arrays;

@Component
// @Component
Copy link

Choose a reason for hiding this comment

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

사용하지 않는 코드는 주석 처리를 하기 보단, 삭제를 하는 것이 좋을 것 같아요!

import roomescape.time.Time;
import roomescape.time.TimeRepository;

@Profile("test")
Copy link

Choose a reason for hiding this comment

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

이 어노테이션 덕분에, 각 개발 환경에 맞는 빈을 주입해줄 수 있겠네요~! 👍

@@ -1,4 +1,4 @@
package roomescape.auth;
package auth;
Copy link

Choose a reason for hiding this comment

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

Auth 라고 이름 붙은 클래스를 모두 auth 패키지로 옮겨주신 것 같아요.

이건 아마 7단계 중 아래의 요구사항 때문이겠죠?!

JWT 관련 로직을 roomescape와 같은 계층의 auth 패키지의 클래스로 분리하세요.

여기서 두 가지 정도의 리뷰 포인트가 존재할 것 같아요.

1. JWT 관련 로직은 어디까지를 관련된 로직으로 봐야 하는가?

상희님도 이에 대해 고민을 해보셨을 것 같아요. 그리고 이에 대한 결론으로, JWTUtils 와 Auth 가 붙은 모든 클래스를 그 대상으로 보신 것 같습니다.

다만 저는 생각이 조금 달라요. 아래는 요구사항 해결 방향성에서 가져온 문장인데요!

JWT 관련 로직으로는 토큰 생성과 토큰 값 조회 기능이 있습니다.

Auth 가 붙은 클래스들에는, 토큰 생성과 토큰 값 조회에 대한 기능을 포함하고 있지 않는 것 같아요. 이는 모두 JWTUtils 라는 클래스의 책임으로 보여요.

즉, JWT 관련 로직이라는 범위에는 Auth~ 클래스가 포함되지 않을 것 같다는 생각이에요.

예를 들어, 상희님의 코드에서는 AuthClaimsArgumentResolverJWTUtils를 의존하고 있는데, 이는 JWTUtils의 기능을 이용하여 AuthClaims 객체 조립을 도울 뿐, JWTUtils에 관련된 로직이라고 보긴 어렵지 않을까 하는 생각입니다.

2. 왜 auth 패키지로 분리해야 하는가? (= 미션의 의도가 무엇인가?)

이번 리뷰가 좀 어려웠는데, 이 포인트 때문이었던 것 같아요. 이 이야기는 상희님이 PR 본문에 남겨주신 아래의 질문들에 도움이 될 것 같네요.

굳이 왜 @component를 사용하지 않고 @configuration을 사용하는 게 요구사항인지 궁금했습니다. 약간 후자를 사용하면 싱글톤이 보장된다고 하는데 인증 과정을 왜 싱글톤으로 하는지에 대한 의문이 좀 들었습니다.

auth 패키지를 roomescape와 동등한 패키지로 만들라는 것이 @configuration 을 통해 빈을 등록하는 연습을 위한 건지 아니면 auth라는 인증 패키지는 원래 대부분 패키지를 기본 패키지와 동등한 계층으로 만들어 사용하는지 궁금합니다. 만약 후자라면 이유도 궁금합니다.

아래는 7단계 요구사항 테스트 코드인데요,

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_TEST_METHOD)
public class MissionStepTest {
    @Test
    void 칠단계() {
        Component componentAnnotation = JwtUtils.class.getAnnotation(Component.class);
        assertThat(componentAnnotation).isNull();
    }
}

상희님이 만드신 JWTUtils 클래스에 @Component 어노테이션이 없어야 한다는 의도를 담은 테스트 코드예요.

이 테스트의 의도와, 7단계 미션의 맥락을 봤을 때, JWTUtils 및 auth 패키지를 스프링으로부터 완벽히 격리되어 있는 하나의 모듈로 바라보는 걸 원하지 않았을까? 라는 생각이 듭니다.

이렇게 생각해 봅시다. 상희님은 당장 JWT 관련 로직이 필요한데, 직접 구현하기에는 난이도가 너무 높다고 가정해 볼게요.

그렇다면 대안은, 이미 잘 만들어져있는 JWT 관련 라이브러리를 외부에서 끌어오는 방법이 있을 것 같아요. 이때 이 라이브러리의 패키지 명은 auth 이고, 클래스는 JWTUtils 라는 이름을 가진 거죠.

문제는, JWT 라이브러리를 만든 개발자가 스프링 기술을 전혀 고려하지 않고, 범용적으로 사용할 수 있는 POJO 형태의 코드를 작성했다는 점입니다.

즉, JWTUtils 클래스에는 @Component 와 같은 코드가 전혀 작성이 되어있지 않을 거예요. 스프링 기술을 전혀 고려하지 않았으니까요. 이 클래스가 스프링 빈으로 사용될 건지, 순수 자바 객체로 사용될 건지는, 이 라이브러리를 가져다 쓰는 개발자의 몫으로 남겨둔 거죠.

그러면 이제 다시 우리의 상황으로 돌아와서 생각해 봅시다!

우리는 스프링 기술을 사용하고 있고, 당장 JWTUtils 클래스를 스프링 빈으로 등록해서 사용해야 하는 상황입니다.

문제는, JWTUtils 클래스가 외부 라이브러리 코드이기 때문에, 우리가 수정할 수가 없다는 점이에요. 즉, @Component 어노테이션을 붙이고 싶어도 방법이 없어요.

이럴 때 어떻게 해볼 수 있을까요?

외부 모듈 코드라서, 해당 코드의 제어권이 우리에게 없을 때, @Configuration 을 통해서 수동으로 빈 등록을 할 수 있어요. 이렇게 하면 외부 모듈 코드더라도, 우리가 사용하고 있는 스프링 세계로 끌어들일 수 있겠죠.

또 다른 관점에서 볼 수도 있는데, 만약 상희님이 도메인 설계를 하고 있는데, 스프링 이라는 특정 기술에 종속적인 코드를 작성하고 싶지 않다고 가정해볼게요. 즉, 그 도메인 클래스의 import문에는 스프링 기술이 존재하면 안되는 상황입니다.

그렇게 도메인 코드를 POJO 형식으로 잘 작성했는데, 개발을 하다보니 그 도메인 객체를 스프링 빈으로 등록해서 조금 더 간편하게 사용하고 싶을 때가 있을 거예요.

이럴 때 사용해볼 수 있는게 @Configuration 이기도 합니다. 상희님이 만든 도메인 객체를 Configuration 클래스에서 스프링 빈으로 수동 등록하는 거죠. 이렇게 되면, 도메인 코드에서는 스프링 기술 종속성을 제거할 수 있으면서도, 동시에 스프링 빈 객체로서 사용할 수 있습니다.

그래서 이 미션을 진행하실 때 상희님이 가져야 할 관점을 정리해보자면,

auth 패키지 및 JWTUtils 클래스를 스프링 기술과 별개로 존재하는 외부의 모듈(라이브러리)로서 바라보는 것 일 거 같아요.

상희님이 JWTUtils 라는 코드를 외부에서 끌고 왔다고 가정하고, 코드를 다시 작성해보시면, 왜 @Configuration 을 사용해야 하는지 감이 오실 것 같아요.

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

@SANGHEEJEONG SANGHEEJEONG Jan 26, 2025

Choose a reason for hiding this comment

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

JWT 관련 로직으로는 토큰 생성과 토큰 값 조회 기능이 있습니다

JWT 관련 로직이 JWT에 직접적인 연관성을 가지는 클래스만 포함하는 것이 이 요구사항의 의도였군요!
저는 JWT를 활용하여 인증하는 흐름까지 포함하는 건 줄 알고 다 같이 옮겨버렸던 것 같습니다..!
리뷰어님 말씀대로 토큰 생성과 조회에 대한 책임은 JWTUtils가 가진다고 생각합니다.


리뷰어님의 리뷰를 보고 미션의 의도를 정리해봤는데요,

  • JWTUtils를 스프링에 의존하지 않는 POJO 형태의 유틸리티 클래스로 바라보는 것을 요구
  • 우리가 직접 수정할 수 없는 외부 라이브러리처럼 취급하여, @configuration을 통해 빈으로 수동 등록하는 방식을 연습하려는 의도

이렇게 정리할 수 있을 것 같습니다.

도메인 코드에서는 스프링 기술 종속성을 제거할 수 있으면서도, 동시에 스프링 빈 객체로서 사용할 수 있습니다.

  • 도메인 클레스에 스프링 프레임워크의 어노테이션이나 인터페이스 등을 포함하지 않는 POJO
  • 코드는 스프링과 무관하지만 애플리케이션에서는 스프링 컨테이너가 객체를 관리하도록 설정

미션의 의도대로 했을 때 얻는 이점

  • 도메인 코드가 프레임 워크 변경의 영향을 덜 받음으로써 코드의 적용이 스프링에만 국한되지 않음
  • 인증 방식이 변경되더라도 코드 수정 없이 모듈 자체만 교체하면 됨

리뷰어님 덕분에 미션에서 왜 이런 요구사항을 제시했는지 얼추 이해가 된 것 같아요~ 감사합니다.
혹시 조금이라도 틀린 내용이 있거나 잘못 이해한 부분이 있다면 알려주세요!

@@ -16,6 +18,8 @@ public class Member {
private String name;
private String email;
private String password;

@Enumerated(EnumType.STRING)
Copy link

Choose a reason for hiding this comment

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

Enum을 객체 필드로 둘 때 사용하기 좋은 어노테이션인 것 같아요~! 잘 해주신 것 같습니다 👍

@@ -26,7 +26,7 @@ public List<ReservationResponse> list() {
}

@GetMapping("/reservations-mine")
public List<MyReservationResponse> myList(AuthClaims authClaims) {
public List<MyReservationResponse> myList(@AuthCustomAnnotation AuthClaims authClaims) {
Copy link

Choose a reason for hiding this comment

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

AuthClaims 클래스만으로도 잘 동작할 것 같은데, 커스텀 어노테이션을 추가로 만들어주신 이유가 무엇인지 알려주세요~!

Copy link
Author

Choose a reason for hiding this comment

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

MVC 미션 진행할 때 리뷰 받은 내용을 적용한 부분입니다!

보편적인 방법?전략?을 설명 드리면 ArgumentResolver를 등록할 때 커스텀 어노테이션을 정의하고 해당 어노테이션이 있는지 없는지 체크하는 경우가 많아요.

제 생각엔 특정 객체로 일치를 시키면 해당 객체는 컨트롤러에 아규먼트로 오게되면 그 리졸버를 무조건 타게 되겠지만,
근데 어노테이션으로 목적을 더 명식적으로 표현해서 의도에 더 맞게 사용할 수 있는 것이 장점 같아요.

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.

3 participants