[그리디] 황혜림 Spring Core (배포) 7,8,9 단계 제출합니다. #176
Conversation
HyerimH
changed the title
There was a problem hiding this comment.
안녕하세요, 혜림.
너무 늦게 답장을 드리는 리뷰어 루카입니다.🐳
제가 PR에 관심이 없었던 건 아니지만, 사실 뭘 리뷰를 드려야 효과적일지 조금 오래 고민했어요.
아직 그래서 답을 찾진 못해서 질문 위주의 리뷰를 드렸습니다.
💡 리뷰 스타일
다른 분들과는 리뷰를 조금씩 나눠봐서 따로 설명드리진 않지만 혜림은 첫 리뷰여서 제 리뷰 형식을 조금 설명드릴까해요.
각 코멘트 별로 분류를 했습니다. 분류는 다음과 같습니다.
- 👍 Approve: 동의하는 코드
- 👀 Comment: 크게 문제가되는 코드는 아니지만, 의논을 해볼만한 코드
- 🤔 Request Change: 컨벤션 위반, 버그 유발, 요구사항 미이행, ... 같은 심각한 문제를 내포한 코드
🎯 리뷰 포인트
- 환경 분리 (테스트 코드를 포함한)
- jwt 패키지 보완
📜 질문 답변
해당 부분을 다시 name과 role이 들어가도록 수정해야 할까요?
이건 전적으로 혜림의 선택에 달렸습니다.
만약 role이 실시간으로 변경이 가능한 내용이라면 토큰을 이미 발행해버리면 해당 내용을 반영하기는 어려울 수 있겠죠.
하지만 로그인했을 때 특정 권한으로 로그인한 것은 안바뀌는게 일반적일것 같기두하고요.
반면, jwt 토큰은 탈취하면 쉽게 내용을 조회할 수 있는 수단이므로 공개될만한 개인정보는 숨기는게 나을 수도 있겠구요.
근데 또 사실 role은 그렇게 민감정보는 아닌 것 같기도 하구요ㅎㅎ
잘 생각해보시죠.
🔎 추가 요구사항
9단계가 선택적으로 진행된다고 하던데,
여력이 되신다면 배포 스크립트 직접 한번 작성해보시고 실제 배포를 해보시는 것을 추천드려요.
배포관련된 경험은 백엔드 개발자에게 매우매우 중요한 경험인데, 생각보다 프로젝트를하거나 현업에서 직접 배포 시나리오를 구상해서 배포 스크립트를 작성해보는 경험은 해보기 쉽지 않습니다.
🙇 Request Change
사실 이 PR만 봤을 때는 RC를 드릴 이유가 전혀 없는데요.
근데 또 그렇다고 코멘트 드릴 수 있는 내용도 충분하지는 않은 것 같아서, 조금 더 딥하게 고민해보면서 추가적인 질문이나 고민 포인트 늘려가셨으면 좋겠습니다.
이번에 리뷰를 너무 늦게 드려서 죄송합니다.
이 다음부터는 조금 더 빨리 리뷰드릴 수 있도록 할게요.
There was a problem hiding this comment.
🤔 Request Change
테스트 관련된 코드는 test 패키지로 옮겨주세요.
이번 미션에서 profile을 분리하면서 application 설정을 하는 연습을 하셨을텐데요.
test 또한 환경이고 프로덕션과는 분리될 필요가 있습니다.
환경설정으로 분리될 필요도 있지만 코드상으로도 프로덕션 코드와 테스트 코드를 분리해보면 좋겠습니다.
아마 이 코드를 test영역으로 내리면, 의도와는 다르게 동작할 수도 있을 것 같은데요.
그렇다고 이 코드가 여기 있을 것은 아닌 것 같아요. 그 방법은 그방법대로 찾아보시죠.
There was a problem hiding this comment.
아마 이 코드를 test영역으로 내리면, 의도와는 다르게 동작할 수도 있을 것 같은데요. 그렇다고 이 코드가 여기 있을 것은 아닌 것 같아요. 그 방법은 그방법대로 찾아보시죠.
우선 TestDataLoader를 test패키지 하위로 이동하였습니다! 의도와 다르게 작동한다는 걸 제대로 이해하지 못한 것 같아요..!
| @Component | ||
| @Profile("prod") | ||
| @RequiredArgsConstructor | ||
| public class DataLoader implements CommandLineRunner { |
There was a problem hiding this comment.
👀 Comment
혜림이 이번 미션에선 prod라는 profile을 선언해주셨는데요.
이 프로파일로 실행되는 환경은 구체적으로 어떤 환경인가요?
저는 관습적으로 prod 환경이라 하면 실제 고객에게 서비스하는 환경으로 느껴지는데요.
실제 서비스하는 환경이라면, 매번 빌드를 할 때마다 이런 데이터로더가 실행되면 안되지 않을까요?
서비스 데이터는 소중하니까요.
이런 설정은 소위 말하는 dev, local 같은 개발 환경에 더 적합하다고 생각합니다.
There was a problem hiding this comment.
말씀하신대로 prod는 실제 서비스 환경이라서 이렇게 매 빌드마다 데이터를 삽입하면 안되겠네요!! dev로 수정하였습니다. 추가로 properties를 yml로 바꿨고 prod, dev, test 환경별로 만들었습니다
src/main/java/jwt/AuthConfig.java
Outdated
| @Configuration | ||
| public class AuthConfig { | ||
|
|
||
| @Bean | ||
| public JwtUtils jwtUtils( | ||
| @Value("${roomescape.auth.jwt.secret}") String secretKey, | ||
| @Value("${roomescape.auth.jwt.expiration}") long expirationTime) { | ||
| return new JwtUtils(secretKey, expirationTime); | ||
| } | ||
| } |
There was a problem hiding this comment.
👀 Comment
@ConfigurationProperties라는 주입방식을 한번 살펴보고 어떤것이 더 적절한지 생각해보시면 좋겠습니다.
There was a problem hiding this comment.
👀 Comment
저는 이 configuration 이 약간 이질적으로 느껴지는데요.
- 왜 auth와 roomescape 패키지는 분리되어야할까?
- 왜 jwtUtils는 @bean 어노테이션으로 빈객체 설정이된것일까?
- 패키지명이 jwt인 이유는 무엇일까? -> 다른 인증 방법이 생기면 패키지를 또 만들어야하나?
일단 저는 여러모로 이 jwt 패키지 존재 자체가 공감이 잘 안됩니다.
이를 삭제하고 합쳐라 라는 의미의 리뷰는 아닙니다.
혜림이 질문 주신 것 처럼 이를 분리하는 이유를 조금 더 같이 찾아봐야될 것 같은데요.
일단 패키지를 분리하는 것은 생각보다 큰 의미입니다.
설정을 패키지 별로 옵션을 다르게 줄 수도 있겠죠.
사실 저도 현재로선 어떤 활용 방법 자체가 떠오르진 않는데요.
혜림님이 Auth 관련된 영역을 이쪽으로 많이 옮기고, prod, dev, test, ... 환경 별로 auth 설정을 달리하면서 어떤일이 벌어지는지 경험적으로 느끼셨으면 좋겠습니다.
There was a problem hiding this comment.
👀 Comment
@ConfigurationProperties라는 주입방식을 한번 살펴보고 어떤것이 더 적절한지 생각해보시면 좋겠습니다.
현재 값이 두개이긴 하지만 더 깔끔하고 관련 설정이 한눈에 보이는 것 같아서 @ConfiguationProperties를 사용해서 수정해보았습니다!
@Value :
단일 값을 주입받기 위해 사용
RelaxedBinding 사용 불가
@ConfigurationProperties :
클래스로 N개의 값을 바인딩하기 위해 사용
RelaxedBinding 사용 가능
주의사항
- 값의 바인딩을 위해 Setter 필요, 생성자로 바인딩시 @ConstructorBinding을 붙여야 함(스프링 3.0 미만에서만 붙여줌)
- 해당 어노테이션 사용하기 위해선 @EnableConfigurationProperties에 해당 클래스를 지정해주거나 @ConfigurationPropertiesScan을 통해 스캐닝의 대상이 되도록 해야함
+) RelaxedBinding이란
- 프로퍼티 값의 이름이 조금 달라도 유연하게 바인딩을 시켜주는 규칙을 의미함.
|
루카님 리뷰 감사합니다
|
안녕하세요 루카님😊 처음 뵙겠습니다! 벌써 마지막 미션이라니, 시간이 정말 빠른 것 같네요ㅎㅎ 마지막 미션 잘 부탁드립니다 :)
먼저 7단계의 조건이 JWT 관련 로직을 roomescape와 같은 계층의 auth 패키지의 클래스로 분리하라고 하였는데 기존에 auth 패키지는 그대로 두고 최상위에 jwt 패키지(같은 auth이름을 쓰면 혼동을 줄 수 있다 판단해 jwt로 바꿨습니다)를 만들어 JwtUtils와 AuthConfig를 두었습니다. 가장 먼저 든 의문은 왜 jwt를 이렇게 구분하는 것일까 였습니다. 확실히 기존의 auth 패키지는 인증과 인가를 처리하는 서비스, 인터셉터 등 도메인 로직과 관련이 있는 클래스지만 JWT 관련 로직은 단순히 토큰을 생성하고 검증하는 유틸리티 역할을 하고 있습니다. 이 로직을 분류하기 위한 수단인지, 테스트에 나와있는 것 처럼 @component 없이, @configuration과 @bean을 통해 필요한 빈을 수동으로 등록하는 것을 연습하기 위한 것인지 아니면 다른 뜻이 있는지 궁금합니다!
또한 DB 조회 비용이 크지않다고 판단하여 보안에 중심을 두고자 JWT에 id만 담고 필요한 정보는 DB 조회를 하도록 했는데 조건에
가 있더라구요! 해당 부분을 다시 name과 role이 들어가도록 수정해야 할까요?
9단계는 아직 구현 여부를 논의 중이라 들어서 결정되면 그때 반영해서 올리도록 하겠습니다!
제가 놓친 부분이 있는지 혹은 더 보완할 부분은 없는지 궁금합니다!