-
Notifications
You must be signed in to change notification settings - Fork 5
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
[BE] issue196: 토큰을 이용한 현재 사용자 조회 #200
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.
베루스! 구현 고생하셨습니다~_~
몇가지 사소한 코멘트와 궁금한 점 남겼습니다!
final List<MemberData> data = jdbcTemplate.query(sql, Map.of("id", githubId), ROW_MAPPER); | ||
|
||
if (data.size() == 1) { | ||
return Optional.of(data.get(0)); | ||
} | ||
return Optional.empty(); |
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를 받아와서 사이즈를 비교하고 0번째 인덱스를 반환한다는 표현에서는
조회결과가 없으면 Opional.empty() 이고 있으면 Opional로 감싼 값을 반환한다 라는 의미가 불명확한것 같아요..🥲
try {
final MemberData data = jdbcTemplate.queryForObject(sql, Map.of("id", githubId), ROW_MAPPER);
return Optional.of(data);
} catch (EmptyResultDataAccessException e) {
return Optional.empty();
}
해당 부분을 위와 같은 코드로 변경해보는 것은 어떨까요?? ㅇㅅㅇ
final Optional<Member> foundMember = memberRepository.findByGithubId(member.getGithubId()); | ||
|
||
if (foundMember.isPresent()) { | ||
foundMember.get().update(member.getUsername(), member.getImageUrl(), member.getProfileUrl()); | ||
return; | ||
} | ||
|
||
memberRepository.save(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.
람다에서 위와같이 변경한 이유는 가독성 측면에서 일까요?? 어떤이유에서인지 궁금합니다!!
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.
가독성이 저게 더 좋아 보여서요 ㅎㅎ
package com.woowacourse.moamoa.docs; | ||
package com.woowacourse.docs; |
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.
패키지 분리 좋네요!
|
||
@Test | ||
void findNotFoundGithubIdMember() { |
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.
@DisplayName
으로 GithubId로 찾지 못했을 때에 대한 상황을 명시해주면 좋을 것 같아요!!
|
||
@Test | ||
void notFound() throws Exception { |
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.
엇..여기도 조회가 안되었을 경우에는 @DisplayName
이 누락되어있네요..
혹시 의도해주신걸까요..??🤔
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 String sql = "SELECT github_id, username, image_url, profile_url " | ||
+ "FROM member " | ||
+ "WHERE member.github_id = :id"; | ||
final List<MemberData> data = jdbcTemplate.query(sql, Map.of("id", githubId), ROW_MAPPER); |
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.
앗 혹시 queryForObject 대신에 query 쓴 이유가 있을까요??
query는 리스트로 나오는데 githubId로 찾는 멤버는 한명만 나오니까 queryForObject로 받는게 더 편하지 않나요?
() -> memberRepository.save(member)); | ||
final Optional<Member> foundMember = memberRepository.findByGithubId(member.getGithubId()); | ||
|
||
if (foundMember.isPresent()) { |
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.
저도 기존 코드가 너무 익숙해져서 그런지 변경한 이유가 궁금하네요!
@@ -1,4 +1,4 @@ | |||
package com.woowacourse.moamoa.docs; | |||
package com.woowacourse.docs; |
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.
헉 저도 이렇게 할까 했는데 굳굳!!!
@Test | ||
void getCurrentMember() { | ||
final MemberResponse memberResponse = RestAssured.given().log().all() | ||
.header(HttpHeaders.AUTHORIZATION, token) |
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.
RESR Docs에도 추가하면 좋을 것 같아요 ~
@@ -34,7 +34,7 @@ class AuthenticationRequestMatcherTest { | |||
void matchAuthRequestIsTrue(String method, String path) { | |||
given(httpServletRequest.getMethod()) | |||
.willReturn(method); | |||
given(httpServletRequest.getServletPath()) | |||
given(httpServletRequest.getRequestURI()) |
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.
getRequestURI()
로 변경한 이유가 궁금해요!
}) | ||
@ContextConfiguration(classes = {MoamoaApplication.class}) |
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.
해당 설정이 왜 추가됐는지 궁금해요!
@@ -15,7 +15,7 @@ public class AuthRequestMatchConfig { | |||
public AuthenticationRequestMatcher authenticationRequestMatcher() { | |||
return new AuthenticationRequestMatcherBuilder() | |||
.addUpAuthenticationPath(HttpMethod.POST, "/api/studies", "/api/studies/\\d+/reviews") | |||
.addUpAuthenticationPath(HttpMethod.GET, "/api/my/studies") | |||
.addUpAuthenticationPath(HttpMethod.GET, "/api/my/studies", "/api/members/me") |
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.
👍
요약
토큰을 이용한 현재 사용자 조회
세부사항
close #196