-
Notifications
You must be signed in to change notification settings - Fork 56
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 MVC(인증)] 정원영 미션 제출합니다. #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원영님
코드 되게 잘 작성해줬네요 👍
코드 리뷰 및 의견은 저 역시도 하나의 취준생이므로 동등하게 의견을 제시한 거라고 생각해주세요!
( 들이박아도 OK )
궁금한 점 및 개선점들에 대한 답변입니다.
과도한 관심사 분리는 오히려 독이다.
해당 부분은 테스트를 짜면 조금 더 기준을 느낄거 같아요.
지금 작성한 코드로 테스트를 한번 작성해보세요.
- 비밀번호가 없으면 에외를 발생한다.
- 토큰이 만료기간이 지나면 에외를 발생한다.
등등
테스트를 작성하다 보면 테스트를 어렵게 만드는 요소들이 있을겁니다.
( 외부 API, 랜덤, 외부 라이브러리 )
-> 이런 요소들이 있다면 왠만하면 분리를 합니다.
하지만, 얘는 분리를 해야 하나? 테스트에 어려움이 있지 않네??
-> 라고 생각들면 당장 분리를 할 필요는 없다고 생각합니다.
Member 클래스 확장에 따른 ripple effect
왜 rippple effect 라 생각하는지 이해가 잘 안가네용.
토큰 서비스를 조금 더 분리를 한다면 어떻게 할 계획인가요?
커밋을 남길때 항상 고민이 되는 사항이 커밋을 정리해야하는가?입니다.
커밋을 정리하려면 코드를 하나하나 복붙해가며 변경사항을 커밋해야해서 오래 걸립니다...
해당 부분이 어떤 상황인지 모르겠어서 좀 더 자세히 설명해주세요.
작업환경이 IDEA 이고, Git Stash 나 Git Amend 를 사용하면 큰 어려움이 없을거 같은데 조금 더 상황을 설명해주세요~
리뷰에서 남긴것처럼 테스트 코드 작성해주세요.
궁금한 거 있으면 편하게 연락주세요~
( 말투는 키보드만 잡으면 여포가 되어버려서 지송염😘~ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한번에 올릴려고 했는데 어쩌다가 잘못 올려버렸네요.. 다음부턴 한번에 코멘트 달도록 하겠습니다~
테스트 코드를 거의 안짜다 보니 테스트에 대한 지식이 너무 부족하다고 생각해요.. 이번 스터디를 하며 이런 역량을 좀 길러야겠다는 생각이 드네요!! 아래는 추가적인 설명입니당
Ripple Effect
이 문제는 createToken 메서드에 있다고 생각합니다. TokenService의 목적은 토큰의 생명주기를 관리하는 것이지 '로그인토큰'을 관리하는 곳은 아니라고 생각합니다. 토큰을 로그인이 아닌 다른 목적으로 사용(인증이 필요한 곳에 토큰을 사용하게끔, 매번 DB를 찌를 수 없으니)하게 된다면 MemberTokenDto를 매개변수로 받아선 안됩니다.
만약 좀 더 범용성 있게 만든다면 ClaimsMap 만 받아서 Map안의 요소들만 이용해 토큰을 생성해야한다고 생각합니다. 지금은 MemberTokenDto를 받기에 로그인 토큰만 만들수 있다. 가 핵심입니다. -> 다른 토큰 쓰려면 다 뜯어고쳐야함 -> Ripple Effect 큼
커밋 정리
커밋을 남기면서 하는건 한번에 완벽한 코드를 짤 수 없기 때문이라고도 생각합니다. 근데 이러한 사항은 리뷰하는 사람입장이나 커밋을 되돌아보는 사람입장에선 알 필요가 없습니다. 정리에 대한 전제조건은 하나의 pr에서 입니다. 아래와 같은 상황을 가정해봅시다.
1번 : feat 1-> feat 2 -> refact 1 -> feat 3 -> push
커밋을 남기는 사람 입장에서는 refact 1에서 개선사항이 있으니 리팩토링을 진행했을 겁니다. 하지만 리뷰를 하는 사람입장에선 개선한건지 안한건지 알 필요가 없다는 것이지요. 그래서 정리를 하면 아래와 같이 되는 겁니다.
2번: feat 1 (refact 1 포함) -> feat 2 -> feat 3 -> push
리뷰 하는 사람입장에서는 feat1이 개선된 내용인지 개선전인지 알 필요가 없죠. 그저 1, 2, 3 순서로 개발했구나만 알면 되는겁니다. 오히려 커밋수는 줄었으니 보기도 편할겁니다. 이러한 관점에서 커밋 정리가 필요한 건지 궁금한겁니다. 물론 위와 같이 간단한 경우에는 amend를 활용하는 방식으로 합칠 수 있겠죠. 하지만 커밋이 정말 많고 feat 1이 feat2, feat3에도 영향이 가서 결국 amend를 한다면 feat2도 고쳐야하고 feat 3도 고쳐야하는.. 그런상황에 정리하기가 어렵다는 겁니당
여담으로 커밋을 1번처럼 남기면 리뷰할때 커밋순서로 보면 뒤죽박죽에 이럴거면 왜 커밋을 남겼나 라는 생각이 듭니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ripple Effect접근이 맞습니다. 이건 간단한 사례로 정말 객체간 역할을 분리해봐야지 하고 클린 코드를 시도해봤는데 이는 한번에 정하는 것보다 계속 코드를 짜나가면 늘어갈 겁니다. ( 저도 아직 한참 부족하구요 ) 커밋 정리저는 결국 커밋이란 다른 사람들에게 보여주고 싶은 자신의 메시지라고 생각해요. 굳이 세세한 모든 요소들을 보여줄 필요는 없어요. 스스로가 다른 사람들에게 의도가 될 정도로 묶어서 처리하면 될 거 같아요. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덕분에 많은 깨달음을 얻는 것 같아요! 사실 이번에 Mock에 대해서 처음 공부해보고 써봐서 모르는게 많은 것 같습니다! 좀 더 학습한 후에 활용해보는게 좋은 것 같아요.. 달아주신 링크는 좋은 글인것 같습니다!!
Mock과 관련된 질문이 있어 코멘트 하나 남깁니다!
이메일과 비밀번호가 맞지않는 경우 MemberDao에서 null을 반환하는 것이 아닌 Spring 추상화 Excpetion이 발생
다 고친줄 알았는데 좀 많이 남았더군요. 2단계는 머지완료되면 바로 올리도록 하겠습니다! (완성됨) password 검증요구사항에 따라 email와 password중 어떤 것이 틀린 것인지 알기위해 password를 들고오도록 변경함
위와 같은 이유로 이전처럼 email password를 단순히 한번에 확인하는 로직이 더 좋지 않을까라는 생각을 함 TimeProvider가만 고민해보니 ConfigurationProperties가 같은 역할임 -> TimeProvider를 만들지 않고 JwtConfig 활용 |
구현시 고민한 점ArgumentResolver의 책임을 고민해봄
그럼 ArgumentResolver가 서비스를 호출해도 괜찮은 건가?
남은 고민ArgumentResolver의 테스트를 작성해야하는데 막혔습니다.. 만약 Controller에 대한 테스트를 작성하는건 문제가 있다고 생각했습니다.
ArgumentResolver test 라고 쳐도 안나오네요.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이제 2~3단계 진행해도 될 거 같네요. 👍
추가로, TimeProvider
에 대해선
#120 (comment)
해당 내용에 의견 남겨서 이로 갈음합니다!
|
||
@Configuration | ||
@ConfigurationProperties("roomescape.auth.jwt") | ||
public class JwtConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record 여도 될 거 같네용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigurationProperties를 위해서 setter 메서드가 존재해야하는 줄 알았는데 지금보니 생성자도 지원하게 바뀌었다고 하네요..
private static final String ROLE_CLAIM = "role"; | ||
|
||
private final JwtConfig jwtConfig; | ||
private Key key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final 을 붙여도 괜찮을 거 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러네요!
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.context.annotation.Configuration; | ||
|
||
@Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration 을 선언한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenService에서 주입받아서 사용하려고 했습니다!
메서드로 빈을 등록하거나 하지는 않지만 jwt에 대한 설정을 나타내는 것 같기도 해서 붙였습니다. 근데 그냥 @Component
를 붙이는게 더 맞는 것 같기도하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 질문한 이유는
ConfigurationProperteis
-@EnableConfigurationProperties
로 해도 될거 같은데 Configuration 을 선언해서 질문했습니다.
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
@SpringBootTest | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드에 Transactional 을 붙인 이유가 있나요?
( 틀린게 아니라, 의견을 묻는 겁니당. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 현재 상황에 좋은 방식은 아니라고 생각합니다.
붙인 이유
- beforeEach로 save를 호출하니 동일한 member가 있다고 예외가 터져서 입니다.
- 생성자로 초기화 해주려고 했으나 무슨 이유에선지 MemberDao가 주입되기 전에 자꾸 save를 시도해서 방법을 바꾸었습니다.
- 물론 event로 처리할 수 있지만 가장 간단한 Transactional 을 붙였습니다.
안좋다고 생각하는 이유
- 테스트에서 조회만 하고 수정은 안하는데 Transaction을 사용할 이유가 없다. (리소스 낭비)
- 테스트 DB에 Member가 있었다면 조회를 위한
setUp()
을 안해도 됐을 것이다. (사실 이게 제일 좋은 방법이라고 생각합니다.)
현재 상황외
@Transactional
을 테스트에 붙이는 것이 좋은 것인가는 조금 고민을 해봐야할 것 같습니다. 습관적으로 붙인다면 전파 옵션이 Requires_new인 상황에 매우 복잡해집니다. 그래서 저는 습관적으로 @Transactional
붙이는 것을 지양하고 있습니다. 가능하면 로직을 다 알고 있을 때 해당 어노테이션을 붙이는게 좋은 것 같아요 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 말한 내용들도 맞긴 하지만
가장 큰 문제점은 테스트 메소드가 트랜잭션으로 묶여서 기존 로직과 의도하지 않은 결과가 나올 수 있는걸로 알고 있어요.
이에 대해선 테스트에 Transactional
만 쳐도 많이 나올거니 제가 자세히 말하는건 생략할게요 🙂
저희 팀은 로직이 엔티티의 특정 상태를 바꾸는 것이라면 @Transactional
을 사용했던거 같아용.
그리고
습관적으로 붙인다면 전파 옵션이 Requires_new인 상황에 매우 복잡해집니다.
해당 부분과 관련해서
JPA Transactional 잘 알고 쓰고 계신가요?
성능적으로도 측정한 아티클 있어서 남기고 갑니다~
String actualToken = tokenService.createToken(memberTokenDto); | ||
MemberTokenDto actualClaims = tokenService.getMemberClaims(actualToken); | ||
|
||
assertThat(actualToken).isNotEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분은 바로 assertThat 을 해도 통과할거 같네요.
( assertThat(actualClaims).isEqualTo(memberTokenDto)
)
그리고, 이 이유는 뭔가요? ( 레코드 동등성 + isEqualTo 가 뭐하는지 알려주세요. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 생각해보니 그러네요 equalTo
는 결국 equals
메서드를 이용해서 비교합니다. record는 equals와 hashcode를 자동으로 생성해주기 때문에 현재 상황에서는 굳이 하나씩 비교할 필요가 없을 것 같네요!
|
||
public String loginWithEmailAndPassword(String email, String password) { | ||
|
||
Member member = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분에 member 가 존재하는 이유가 있나요? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 마지막에 고친부분이 잘못 고친거였네요... 저도 이부분을 보면서 "member가 필요없는데? 쿼리가 두번 나가네?" 하고 바로 삭제해버린것 같습니다. 반환값에 member에 대한 정보가 필요해서 findByEmailAndPassword
반환값을 받으려고 했습니다. 아마 현재 코드로 테스트를 돌리면 통과하지 못할 것 같네요..
푸시하기전에 테스트를 돌리고 푸시했어야했는데 아주 기초적인 실수를 했네요.. 수정하겠습니다!
@@ -40,6 +40,14 @@ public Member findByEmailAndPassword(String email, String password) { | |||
); | |||
} | |||
|
|||
public String findPasswordByEmail(String email) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생각해보니 아직 JPA 가 아니군요.😭
비밀번호 검증 부분은 나중에 JPA 부분에서 다시 도전해봐도 될 거 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배워가는게 많아서 너무 좋은 것 같아요 😆
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.context.annotation.Configuration; | ||
|
||
@Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenService에서 주입받아서 사용하려고 했습니다!
메서드로 빈을 등록하거나 하지는 않지만 jwt에 대한 설정을 나타내는 것 같기도 해서 붙였습니다. 근데 그냥 @Component
를 붙이는게 더 맞는 것 같기도하네요
|
||
@Configuration | ||
@ConfigurationProperties("roomescape.auth.jwt") | ||
public class JwtConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigurationProperties를 위해서 setter 메서드가 존재해야하는 줄 알았는데 지금보니 생성자도 지원하게 바뀌었다고 하네요..
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
@SpringBootTest | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 현재 상황에 좋은 방식은 아니라고 생각합니다.
붙인 이유
- beforeEach로 save를 호출하니 동일한 member가 있다고 예외가 터져서 입니다.
- 생성자로 초기화 해주려고 했으나 무슨 이유에선지 MemberDao가 주입되기 전에 자꾸 save를 시도해서 방법을 바꾸었습니다.
- 물론 event로 처리할 수 있지만 가장 간단한 Transactional 을 붙였습니다.
안좋다고 생각하는 이유
- 테스트에서 조회만 하고 수정은 안하는데 Transaction을 사용할 이유가 없다. (리소스 낭비)
- 테스트 DB에 Member가 있었다면 조회를 위한
setUp()
을 안해도 됐을 것이다. (사실 이게 제일 좋은 방법이라고 생각합니다.)
현재 상황외
@Transactional
을 테스트에 붙이는 것이 좋은 것인가는 조금 고민을 해봐야할 것 같습니다. 습관적으로 붙인다면 전파 옵션이 Requires_new인 상황에 매우 복잡해집니다. 그래서 저는 습관적으로 @Transactional
붙이는 것을 지양하고 있습니다. 가능하면 로직을 다 알고 있을 때 해당 어노테이션을 붙이는게 좋은 것 같아요 😁
String actualToken = tokenService.createToken(memberTokenDto); | ||
MemberTokenDto actualClaims = tokenService.getMemberClaims(actualToken); | ||
|
||
assertThat(actualToken).isNotEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 생각해보니 그러네요 equalTo
는 결국 equals
메서드를 이용해서 비교합니다. record는 equals와 hashcode를 자동으로 생성해주기 때문에 현재 상황에서는 굳이 하나씩 비교할 필요가 없을 것 같네요!
private static final String ROLE_CLAIM = "role"; | ||
|
||
private final JwtConfig jwtConfig; | ||
private Key key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러네요!
|
||
public String loginWithEmailAndPassword(String email, String password) { | ||
|
||
Member member = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 마지막에 고친부분이 잘못 고친거였네요... 저도 이부분을 보면서 "member가 필요없는데? 쿼리가 두번 나가네?" 하고 바로 삭제해버린것 같습니다. 반환값에 member에 대한 정보가 필요해서 findByEmailAndPassword
반환값을 받으려고 했습니다. 아마 현재 코드로 테스트를 돌리면 통과하지 못할 것 같네요..
푸시하기전에 테스트를 돌리고 푸시했어야했는데 아주 기초적인 실수를 했네요.. 수정하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 추가로 남겨용~
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
@SpringBootTest | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 말한 내용들도 맞긴 하지만
가장 큰 문제점은 테스트 메소드가 트랜잭션으로 묶여서 기존 로직과 의도하지 않은 결과가 나올 수 있는걸로 알고 있어요.
이에 대해선 테스트에 Transactional
만 쳐도 많이 나올거니 제가 자세히 말하는건 생략할게요 🙂
저희 팀은 로직이 엔티티의 특정 상태를 바꾸는 것이라면 @Transactional
을 사용했던거 같아용.
그리고
습관적으로 붙인다면 전파 옵션이 Requires_new인 상황에 매우 복잡해집니다.
해당 부분과 관련해서
JPA Transactional 잘 알고 쓰고 계신가요?
성능적으로도 측정한 아티클 있어서 남기고 갑니다~
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.context.annotation.Configuration; | ||
|
||
@Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 질문한 이유는
ConfigurationProperteis
-@EnableConfigurationProperties
로 해도 될거 같은데 Configuration 을 선언해서 질문했습니다.
구현시 중점적으로 생각한 사항
과도한 관심사 분리는 오히려 독이다.
평소에는 확장성에 초점을 두고 코드를 작성하곤 했습니다. 그 결과, 오버엔지니어링 뿐만 아니라 과도한 책임분리가 일어난다고 생각했습니다. 결국 확장성을 고려한다는 것은 유지보수의 용이성을 고려한다는 뜻이라고 생각합니다. 만약 유지보수의 가능성이 적다면 확장성을 덜 고려해도 된다고 생각했습니다. ("변경 가능성이 적은 구현체를 interface화 하는 것은 안좋다"와 같은 맥락)
그래서, jwt를 조작하는 util 클래스, tokenProvider 분리, 제네릭을 활용한 tokenService 메서드의 범용성 고려 등은 제외했습니다.
인증에 관련된 내용은 따로 빼는것이 옳다
로그아웃 기능이 Member 쪽에 구현되어있었습니다. 그래서 처음에는 Member 쪽에 기능을 구현하려고 했습니다. 하지만, Member에 대해서는 많은 작업이 이루어질 가능성이 있고 인증과는 다른 관심사라고 생각했습니다. 따라서, auth 패키지를 새로 만들어 구현했습니다.
추가 개선점
토큰의 다양한 활용
현재는 Member 클래스를 기준으로 구현되어있기 때문에 범용성이 떨어집니다.
Member 클래스 확장에 따른 ripple effect
만약 Member 클래스에 다양한 정보가 들어오고, 해당 정보들이 토큰에 들어가야한다면 ripple effect가 큽니다. 1번과 마찬가지로 범용성이 떨어진다고 볼 수 있습니다. 이러한 점은 토큰 서비스를 조금 더 분리해내면서 해결 가능하지만 현재 요구사항에는 과하다고 생각하여 하지 않았습니다.
예외처리의 부족
현재는 커스텀 예외를 만들어 제공한더던지, 각 예외에 따른 Handling이 부족합니다. 하지만 기존 코드가 전체 Exception(ExceptionHandling)을 받아 처리하고 따로 커스텀 예외를 만들어 처리하지 않기 때문에, 추가하기 위해선 전반적인 리팩토링이 필요하다고 생각했습니다. 따라서, 기존 코드를 최대한 따라가는 방향으로 구현했습니다.
개인적으로 궁금한 사항
커밋을 남길때 항상 고민이 되는 사항이
커밋을 정리해야하는가?
입니다. 커밋을 정리하면 보는 사람이 매우 편합니다 하지만 커밋을 정리하려면 코드를 하나하나 복붙해가며 변경사항을 커밋해야해서 오래 걸립니다... 어떻게 생각하시는지 궁금합니다.위 의견에 대한 비판, 동의, 조언, 첨언, 수정 등 매우 환영합니다!! 리뷰 잘 부탁드려요~