Skip to content
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

refactor: 코드 컨벤션 적용 및 jwt filter 리팩토링 #11

Closed
wants to merge 5 commits into from

Conversation

penrose15
Copy link
Collaborator

@penrose15 penrose15 commented Jul 5, 2024

PR 올리기는 했는데... 이대로 develop에 푸시하면 민철님 것과 컨플릭트 날 듯 하므로 코드 리뷰 목적으로만 올려 놓겠습니다.

수정 사항

  1. 네이버 코드 컨벤션 xml 적용 후 전체 코드에 적용

  2. jwtAuthenticationFilter에서 early return으로 depth 4 제거

  3. jwtUtil에 위치한 예외 throw jwtAuthenticationFilter로 이동

@penrose15 penrose15 self-assigned this Jul 5, 2024
@penrose15 penrose15 linked an issue Jul 5, 2024 that may be closed by this pull request
2 tasks

return http.build();
}
@Bean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebMvcConfigurer implements 해서 CorsMvcConfig로 클래스 분리하는 건 어떠신가요??
저는 그렇게 했어서 요 방식이 궁금해서 여쭤봅니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

develop로 머지 한 이후 리팩토링 진행 해보도록 하겠습니다.


if (optionalAccessToken.isEmpty()) {
log.info("access token is empty");
filterChain.doFilter(request, response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 끝나고 바로 return 해야 되는 걸로 알고 있는데 괜찮을까요 !?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그러네요 수정하도록 하겠습니다.

Comment on lines +10 to +13
kakao:
client-id: ${KAKAO_CLIENT_ID}
client-secret: ${KAKAO_CLIENT_SECRET}
redirect-uri: ${KAKAO_REDIRECT_URL}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

카카오 벌써 구현된 건가요..??
요거 환경변수는 공유해야 하지 않을까요?? 이것도 각자 진행하나요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 아직 카카오 구현 못했는데 실수로 같이 커밋된 것 같습니다. 카카오 환경 변수는 조만간 공유해드리도록 하겠습니ㅏㄷ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㅋㅋㅋㅋㅋㅋ 마지막 ㅏㄷ.가 진짜 웃겨용

Copy link
Member

@its-sky its-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수과셨습니다~ LGTM~

@Schema(defaultValue = "password") String password) {
public LoginDto {
Objects.requireNonNull(email);
Objects.requireNonNull(password);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spring Boot Validation을 사용하지 않고 이걸 사용한 이유가 있을까요?(단순 궁금)

import io.swagger.v3.oas.annotations.tags.Tag;
import lombok.RequiredArgsConstructor;

@Tag(name = "로그인/회원가입")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 Swagger 세팅 인터페이스로 분리하면 좋을거 같습니다~

import lombok.RequiredArgsConstructor;

@Service
@Transactional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 Read 서비스인거 같은데 Transactional readOnly=true 옵션 켜도 되지 않을까요??

USER("USER")
,ADMIN("ADMIN")
;
USER("USER"), ADMIN("ADMIN");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 개행 넣는게 좋지 않을까요??

@penrose15 penrose15 closed this Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ JWT 필터 구현
3 participants