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
Authorization 헤더를 응답하도록 변경 #981
Authorization 헤더를 응답하도록 변경 #981
Changes from 17 commits
c4a885a
1766805
b61fdf2
a24bdcc
6e8bd43
8569298
d77eee0
320e655
b968ca2
f27c763
0a46b27
fae61a7
a278e16
873c9d1
9eb729a
41f5e1a
794a2e9
38b5f72
c917034
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.
이 리스트는 혹시 페어할 때 이야기 나누었던 것처럼 Adapter로 발전할 예정일까요?
Adapter 같은 일급 컬렉션으로 추출되면 적절한 CredentialManager를 반환하는지 테스트하기 더 좋을 것 같네요👍
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.
아마 프론트 코드 변경 이후엔 다시
List<CredentialManager>
->CredentialManager
로 변경될 것 같아요. 그래서 지금 예상은 Adapter 로 변경되지 않을 것 같네용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.
오홍... 뭔가 우리 클라이언트의 환경이 무려 4개(웹, 익스텐션, vs 플러그인, intellij 플러그인)이기도 하고 ....!
호환성을 생각한다면 Mapping 객체를 하나 두는 게 더 낫다고 생각하긴 하는데 짱수는 어떤가요...!
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.
아하,,, 그렇군요!!
그렇다면 지금처럼 여러개의
CredentialManager
가 공존해야 하는 상황이 지속될 수 있겠네용~~Mapping 객체 하나 더 두는 방향으로 수정해 보겠습니당~~
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.
해당 작업을 #1010 이슈로 인가합니다!
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.
Member
객체의 nullable 여부는 controller 메서드의 파라미터에 존재하는@AuthenticationPrinciple
의 속성에 따라 결정됩니다.그래서 nullable 여부를 바로 볼 수 있는 controller 에서
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.
이것도 cm, 풀네임 둘 중 하나로 통일해주세오~
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.
헉 놓쳤다!!