Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[hoony-lab-issue23] authentication: jwt token 기능 #26
[hoony-lab-issue23] authentication: jwt token 기능 #26
Changes from 16 commits
444d5a8
ba81367
af12b06
0d80ed6
9bae468
289daa9
4e24753
a41f4a3
b3672fe
c8df5b7
2ee1c32
533f645
c117a6f
a090ace
bc1850e
d500e12
f10b793
238d351
6e2a63b
53cf055
ee671a8
70af05c
7f32020
829940b
3283a57
199c904
97af41c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
매직 스트링 제안드립니다!
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.
소스코드에 주로 여러 번 등장하는 일반적인 리터럴 값을 매직 리터럴이라고 한다.
의견 감사드립니다 ! 반영해보도록할게요 ~
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.
getClaimsFromToken
을 두 메소드에서쓰는데 동시에 Filter에서도 두 메소드를 하나의 flow에서 호출하는 이유가 있을까요?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.
이렇게 작성한 이유는 100% 제가 짠 코드가 아니기 때문인 듯 싶습니다.
곰곰히 보니까
getClaimsFromToken
함과 동시에 valid 확인을 하면 좋을 것 같습니다 !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.
토큰 생성 로직을 만드셨는데 추가를 잊으신거같네요 :)
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.
static으로 설정한 이유가 있을까요?
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.
음 어디서 본것같아서 무지성
static
이긴 합니다. 지양하는게 좋을까요?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.
물론 static이 생각할 부분이 적어서 편하기는 한데 해당 클래스를 테스트하지 않는 경우에도 static이 올라가게 되니까 굳이 필요없는 공수를 들이는것같다고 생각해서요 :)
크게 문제되는 사항은 아니긴해요 ㅎㅎ
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.
확장성(다른 테스트 메소드 추가)를 고려하면
static 선언이 약이 될수도 독이 될수도 있을 것 같아요
예를 들어 Controller Test 같은 곳에서 공통되는 uri (/api/users)는 모든 테스트 코드에서 공통적으로 쓰일테니까 URI 같은걸로 선언 해두면 확실히 편할 수 있습니다. swagger에서 사용되는 변수들도 이에 해당될 수도 있겠네요
그런데 파라미터, 변수같은 것들은 웬만하면 비슷하긴 하겠지만 테스트 케이스마다 다른 것을 쓸 수 있어서 애매할 수도 있을 것 같아요. 일단 무지성으로 다 쓰다가 나중에 공통되는 부분들 나오면 나중에 따로 model로 만들어서 가져오는 방법도 있을 것 같네요
물론 항상 애매한것도 아닌것 같아서 그냥 어떠한 방법으로 써도 될 것 같다는.. 그런 의견입니다 ㅎㅎ (사실 저도 지금 테스트 하는데 변수들 하나하나 다 쓰는데 너무 귀찮더라구용)