refactor: 카카오 로그인 피드백 반영 #126

dks301 · 2020-07-28T12:36:45Z

피드백 반영 했으니 확인해주세요 :)

KIMSIYOUNG

안녕하세요 시카!
제가 컨벤션 안 지킨 부분을 다 고쳐주셨네요. 너무 감사합니다😊
사소한 피드백 남겼어요 확인해주세요. 대부분 제 의견이라, 선택적으로 반영해주세요

KIMSIYOUNG · 2020-07-29T01:43:04Z

src/main/java/com/woowacourse/pelotonbackend/certification/domain/Certification.java

@@ -25,30 +25,26 @@
 @EqualsAndHashCode(of = "id")
 @Getter
 public class Certification {
-    @Id
-    @With(value = AccessLevel.PACKAGE)
+    @Id @With(value = AccessLevel.PACKAGE)


고맙습니다😊

KIMSIYOUNG · 2020-07-29T01:44:02Z

src/main/java/com/woowacourse/pelotonbackend/common/ErrorCode.java

@@ -13,6 +13,7 @@
    MEMBER_NOT_FOUND(400, "Member-001"),
    MEMBER_DUPLICATE(400, "Member-002"),
    MEMBER_ID_INVALID(400, "Member-003"),
+    MEMBER_NOT_FOUND_WITH_KAKAO_ID(400, "Member-004"),


처음에 제안할 때는 001을 NOT_FOUND라고 생각했는데, 어떻게 생각하시나요?

직접 이야기를 들어보니 Code 의미를 알기 쉽게하기라는 취지는 좋네요 :)
다만 다른 팀원들과 조금 더 얘기해 볼 필요가 있을 것 같아요.

KIMSIYOUNG · 2020-07-29T01:44:19Z

src/main/java/com/woowacourse/pelotonbackend/config/JdbcConfig.java

@@ -28,7 +28,7 @@ public JdbcCustomConversions jdbcCustomConversions() {
            Arrays.asList(new Converter<Clob, String>() {
                @Nullable
                @Override
-                public String convert(Clob clob) {
+                public String convert(final Clob clob) {


final 귀신...👏

KIMSIYOUNG · 2020-07-29T01:45:27Z

src/main/java/com/woowacourse/pelotonbackend/config/WebMvcConfig.java

-        registry.addInterceptor(bearerAuthInterceptor)
-            .addPathPatterns("/**");
+        final List<String> pathPatterns = Arrays.asList(
+            "/api/certifications/**", "/api/members/**", "/api/missions/**",


exclude가 안되는게 아쉽네요😂

KIMSIYOUNG · 2020-07-29T01:46:58Z

src/main/java/com/woowacourse/pelotonbackend/member/application/LoginService.java

+        try {
+            final MemberResponse memberResponse = memberService.findByKakaoId(kakaoUserResponse.getId());
+
+            return JwtTokenResponse.of(jwtTokenProvider.createToken(memberResponse.getKakaoId().toString()), false);


true, false의 의미를 파악하기 어렵네요. 의미있는 상수로 분리하면 어떨까요?

private static final boolean NOT_CREATED = false; private static final boolean CREATED = true;

의미 파악이 쉽도록 수정 완료 :)

KIMSIYOUNG · 2020-07-29T01:55:31Z

src/main/java/com/woowacourse/pelotonbackend/member/application/LoginService.java

+@Service
+@Transactional
+public class LoginService {
+    private final KakaoAPIService kakaoAPIService;


LoginApiService를 사용하지 않고 KakaoApiService를 사용하는 이유가 있나요?

오 👍
LoginAPIService가 나중에 생겨서 빼먹었나봐요 수정했습니다

KIMSIYOUNG · 2020-07-29T01:55:59Z

src/main/java/com/woowacourse/pelotonbackend/member/infra/KakaoAPIService.java

 import com.woowacourse.pelotonbackend.support.dto.JwtTokenResponse;
 import reactor.core.publisher.Mono;

 @Component
 @Transactional
-public class KakaoAPIService implements LoginService {
+public class KakaoAPIService implements LoginAPIService<KakaoTokenResponse, KakaoUserResponse> {


Generic 사용 좋네요😊

KIMSIYOUNG · 2020-07-29T01:57:44Z

...ain/java/com/woowacourse/pelotonbackend/member/presentation/LoginMemberArgumentResolver.java

        if (Objects.isNull(attribute)) {
-            return Member.builder().build();
+            return MemberResponse.builder().build();


예외를 여기서 반환해도 되는데 굳이 빈 객체를 넘기는 이유가 있나요? 이 부분도 리뷰로 남겼었는데 어떻게 생각하는지 적어주시면 좋을 것 같아요.

같이 이야기 한대로 AssertionError를 던지도록 수정했어요.

추가로 GlobalExceptionHandler에

@ExceptionHandler(Exception.class) protected ResponseEntity<ErrorResponse> UnexpectedException(final Exception exception) { log.error("Unexpected Exception ! ", exception); final ErrorResponse errorResponse = ErrorResponse.of( ErrorCode.UNEXPECTED.getStatus(), ErrorCode.UNEXPECTED.getCode(), exception.getMessage()); return new ResponseEntity<>(errorResponse, HttpStatus.valueOf(ErrorCode.UNEXPECTED.getStatus())); }

예기치 못한 에러를 처리하도록 추가했습니다.

KIMSIYOUNG · 2020-07-29T01:59:40Z

...ain/java/com/woowacourse/pelotonbackend/member/presentation/LoginMemberArgumentResolver.java


-        return memberService.findByKakaoId(kakaoId).orElseThrow(() -> new MemberNotFoundException(kakaoId));
+        final long kakaoId = Long.parseLong(attribute);


지금 반영할 필요는 없지만 MemberResponse보단 차라리, 회원을 분리하는 것도 좋은 방법 같아요.

현재는 Member가 회원이면서 권한을 직접 갖는 객체지만 예를 들어 MemberAuthority와 Member를 분리한다면 MemberResponse대신 MemberAuthority라는 도메인을 가질 수 있을 것 같네요.

MemberAuthority는 Id와 권한 관련 필드만 존재하고, 실제 회원 정보 및 기능은 Member에 존재하는 형태로 생각해봤습니다.

KIMSIYOUNG · 2020-07-29T02:01:28Z

src/main/java/com/woowacourse/pelotonbackend/race/presentation/RaceController.java

@@ -62,10 +62,13 @@
    }

    @ExceptionHandler(RaceNotFoundException.class)


이 메소드가 여기 있을 이유가 있나요? GlobalExceptionHandler에서 처리하지 않나요?

ddaaac

살짝 수정할 사항 남겼습니다 :)

ddaaac · 2020-07-29T04:03:11Z

src/main/java/com/woowacourse/pelotonbackend/support/BearerAuthInterceptor.java

+        final String bearer = authExtractor.extract(request);
+        final String kakaoId = jwtTokenProvider.getSubject(bearer);
+        request.setAttribute("loginMemberKakaoId", kakaoId);



개인적으로 이부분을 메서드로 빼서 의미를 분명히 해주면 좋을 것 같아요.
exclude 패턴이 아니라면 -> 검증하고, attribute를 set 해준다는 느낌으로요.

preHandle이라는 메서드 명이 의미가 애매모호하긴 하네요.
그래도 BearerAuthInterceptor가 preHandle을 한다고 생각하면 조금 괜찮을 것 같아요!

ddaaac · 2020-07-29T04:05:37Z

src/test/java/com/woowacourse/pelotonbackend/member/application/LoginServiceTest.java

+import reactor.core.publisher.Mono;
+
+@ExtendWith(MockitoExtension.class)
+@AutoConfigureWebTestClient


테스트에서 webclient를 사용하지 않아서 이 어노테이션은 필요없겠네요.

ddaaac · 2020-07-29T04:08:07Z

src/test/java/com/woowacourse/pelotonbackend/member/application/LoginServiceTest.java

+    private LoginService loginService;
+
+    @Mock
+    private KakaoAPIService kakaoAPIService;


이 부분도 마찬가지로 LoginAPIService가 되는 건 어떨까요?

ddaaac · 2020-07-29T04:10:16Z

src/test/java/com/woowacourse/pelotonbackend/member/application/LoginServiceTest.java

+    @Test
+    void createJwtTokenUrlTest() {
+        when(kakaoAPIService.createTokenUrl(any(JwtTokenResponse.class))).thenReturn(URL);
+        when(kakaoAPIService.fetchOAuthToken(anyString())).thenReturn(Mono.just(createMockKakaoTokenResponse()));


anyString보다는 CODE_VALUE로 정확한 값을 넣어주는 게 좋을 것 같아요.
EqualsAndHashcode가 구현되지 않은 클래스들이야 어쩔 수 없이 any를 사용한다고 해도, 비교가 가능한 객체들에 대해서는 정확한 비교를 해주는 게 좋다고 생각합니다.

ddaaac · 2020-07-29T04:11:07Z

src/test/java/com/woowacourse/pelotonbackend/member/application/MemberServiceTest.java

    @Test
-    public void retrieveNewMemberTest() {
+    void findByKakaoIdTest2() {


test2라는 메서드 네이밍보다는 예외에 관련된 네이밍을 하는게 좋지 않을까요?

저는 개인적으로 DisplayName으로 명시만 해주면 괜찮다고 생각해요

ddaaac · 2020-07-29T04:12:35Z

src/test/java/com/woowacourse/pelotonbackend/member/infra/KakaoAPIServiceTest.java

+        userResponse = new MockResponse()
+            .addHeader("Content-Type", MediaType.APPLICATION_JSON_VALUE)
+            .setResponseCode(HttpStatus.OK.value())
+            .setBody(objectMapper.writeValueAsString(createMockKakaoUserResponse()));


byte는 어떨까요?

안됩니다

ddaaac

고생하셨습니다 :)

KIMSIYOUNG

tr

- 전반적인 컨벤션 맞춤

dks301 requested review from KIMSIYOUNG and ddaaac July 28, 2020 12:46

dks301 added 백엔드 back end feature 중요 급한 이슈 labels Jul 28, 2020

dks301 added this to In progress in 1차 배포 via automation Jul 28, 2020

KIMSIYOUNG reviewed Jul 29, 2020

View reviewed changes

dks301 self-assigned this Jul 29, 2020

ddaaac requested changes Jul 29, 2020

View reviewed changes

1차 배포 automation moved this from In progress to Review in progress Jul 29, 2020

dks301 requested a review from ddaaac July 29, 2020 07:38

ddaaac previously approved these changes Jul 29, 2020

View reviewed changes

dks301 requested a review from KIMSIYOUNG July 29, 2020 07:45

1차 배포 automation moved this from Review in progress to Reviewer approved Jul 29, 2020

KIMSIYOUNG previously approved these changes Jul 29, 2020

View reviewed changes

dks301 added 3 commits July 29, 2020 20:00

refactor: 카카오 API 로그인 기능 수정

ce62c15

- 전반적인 컨벤션 맞춤

refactor: 카일 피드백 반영

94210d7

refactor: 범블비 피드백 반영

0ef6836

dks301 dismissed stale reviews from KIMSIYOUNG and ddaaac via 0ef6836 July 29, 2020 11:04

dks301 force-pushed the refactor/kakao-login branch from 39fc8c9 to 0ef6836 Compare July 29, 2020 11:04

1차 배포 automation moved this from Reviewer approved to Review in progress Jul 29, 2020

dks301 merged commit bb1eb92 into woowacourse-teams:develop Jul 29, 2020

1차 배포 automation moved this from Review in progress to Done Jul 29, 2020

dks301 deleted the refactor/kakao-login branch August 3, 2020 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: 카카오 로그인 피드백 반영 #126

refactor: 카카오 로그인 피드백 반영 #126

dks301 commented Jul 28, 2020

KIMSIYOUNG left a comment

KIMSIYOUNG Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

dks301 Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

dks301 Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

dks301 Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

dks301 Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

KIMSIYOUNG Jul 29, 2020

ddaaac left a comment

ddaaac Jul 29, 2020

dks301 Jul 29, 2020

ddaaac Jul 29, 2020

ddaaac Jul 29, 2020

ddaaac Jul 29, 2020

ddaaac Jul 29, 2020

dks301 Jul 29, 2020

ddaaac Jul 29, 2020

dks301 Jul 29, 2020

ddaaac left a comment

KIMSIYOUNG left a comment


		return memberService.findByKakaoId(kakaoId).orElseThrow(() -> new MemberNotFoundException(kakaoId));
		final long kakaoId = Long.parseLong(attribute);

		@@ -62,10 +62,13 @@
		}

		@ExceptionHandler(RaceNotFoundException.class)

refactor: 카카오 로그인 피드백 반영 #126

refactor: 카카오 로그인 피드백 반영 #126

Conversation

dks301 commented Jul 28, 2020

KIMSIYOUNG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddaaac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddaaac left a comment

Choose a reason for hiding this comment

KIMSIYOUNG left a comment

Choose a reason for hiding this comment