유지보수 향상을 위해 CredentialManager 의 일급컬렉션을 생성 #1041
Conversation
This reverts commit 2a17646.
|
|
||
| private CredentialManager getDefaultCredentialManager() { | ||
| return credentialManagers.stream() | ||
| .filter(cm -> cm instanceof CookieCredentialManager) |
There was a problem hiding this comment.
instanceof를 사용한 부분이 영 마음에 안드네요.
다만, 인증 정보를 확인하기 위한 Credential-Type 헤더가 존재하지 않는다면 기본적으로 Cookie를 사용하는 CredentialManager를 사용해야 하는데, 이를 구현하는게 조금 번거로웠어요.
고민해본 방법
CredentialManager를 상속하는 DefaultCredentialManager를 만들어볼까 생각했어요.
다만, 이 경우 DefaultCredentialManager.support() 메서드는 항상 true를 반환해야 하기 때문에 List<CredentialManage>의 순서 중 가장 마지막에 위치해야 합니다.
그런데, 지금 우리는 스프링이 직접 의존성을 주입해 주고 있기 때문에 순서를 제어하기가 어려운 상황이에요.
There was a problem hiding this comment.
짱수가 고민한 방법이 충분히 좋다고 생각해요.
DefaultCredentialManager를 필드로 분리하면 순서 문제가 해결될 것 같은데 어떤가요?
|
|
||
| // when & then | ||
| mvc.perform(post("/categories") | ||
| .accept(MediaType.APPLICATION_JSON) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(createCategoryRequest))) | ||
| .andExpect(status().isUnauthorized()) | ||
| .andExpect(jsonPath("$.detail").value("인증에 대한 쿠키가 없어서 회원 정보를 찾을 수 없습니다. 다시 로그인해주세요.")) |
There was a problem hiding this comment.
우리가 Controller 를 테스트할때 detail 을 확인해야 할까요?
이 때문에 예외 메시지만 변경된 경우에도 테스트코드에 수정이 일어나는게 마음에 들지 않아서요.
다들 Controller 테스트에서 확인하고 싶은 것중에 예외 메시지가 포함되어 있나요??
There was a problem hiding this comment.
zangsu
changed the title
zangsu
changed the title
| private CredentialManager getCredentialManager(HttpServletRequest request) { | ||
| Optional<CredentialType> credentialType = getCredentialType(request); | ||
| if (credentialType.isEmpty()) { | ||
| return getDefaultCredentialManager(); | ||
| } | ||
| return credentialManagers.stream() | ||
| .filter(cm -> cm.support(credentialType.get())) | ||
| .findFirst() | ||
| .orElse(getDefaultCredentialManager()); | ||
| } |
There was a problem hiding this comment.
IDE 제안이 뜨는데 이렇게 하는 건 어떤가요?
| private CredentialManager getCredentialManager(HttpServletRequest request) { | |
| Optional<CredentialType> credentialType = getCredentialType(request); | |
| if (credentialType.isEmpty()) { | |
| return getDefaultCredentialManager(); | |
| } | |
| return credentialManagers.stream() | |
| .filter(cm -> cm.support(credentialType.get())) | |
| .findFirst() | |
| .orElse(getDefaultCredentialManager()); | |
| } | |
| private CredentialManager getCredentialManager(HttpServletRequest request) { | |
| Optional<CredentialType> credentialType = getCredentialType(request); | |
| return credentialType.map(type -> credentialManagers.stream() | |
| .filter(cm -> cm.support(type)) | |
| .findFirst() | |
| .orElse(getDefaultCredentialManager())).orElseGet(this::getDefaultCredentialManager); | |
| } |
| @Override | ||
| public boolean support(CredentialType credentialType) { | ||
| return credentialType == CredentialType.COOKIE; | ||
| } |
There was a problem hiding this comment.
CredentialType은 package-private 인 반면, 이 메서드는 public 이어서 경고가 발생해요.
짱수가 의도한 게 아니라면 CredentialType을 public으로 바꿔야겠어요~
|
|
||
| private CredentialManager getDefaultCredentialManager() { | ||
| return credentialManagers.stream() | ||
| .filter(cm -> cm instanceof CookieCredentialManager) |
There was a problem hiding this comment.
짱수가 고민한 방법이 충분히 좋다고 생각해요.
DefaultCredentialManager를 필드로 분리하면 순서 문제가 해결될 것 같은데 어떤가요?
|
|
||
| // when & then | ||
| mvc.perform(post("/categories") | ||
| .accept(MediaType.APPLICATION_JSON) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .content(objectMapper.writeValueAsString(createCategoryRequest))) | ||
| .andExpect(status().isUnauthorized()) | ||
| .andExpect(jsonPath("$.detail").value("인증에 대한 쿠키가 없어서 회원 정보를 찾을 수 없습니다. 다시 로그인해주세요.")) |
There was a problem hiding this comment.
⚡️ 관련 이슈
related #1010, #981
📍주요 변경 사항
AuthController와AuthArgumentResolver의CredentialManagers필드 타입이Stream<CredentialManager에서 일급컬렉션으로 변경됩니다.MockMvcTest에서 로그인 / 비로그인 상황을 흉내내기 위한setLogin(),setNoLogin()메서드를 추가하였습니다.🍗 PR 첫 리뷰 마감 기한
1/23(목) 18:00