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

[Spring MVC] 김준수 미션 제출합니다. #31

Open
wants to merge 8 commits into
base: gogo1414
Choose a base branch
from

Conversation

gogo1414
Copy link

드디어 종강이네요!
1학기 동안 고생많으셨어요ㅎㅎ

3단계까지 진행은 했지만 생소한 개념이라 제대로 진행했는지 잘 모르겠어요..
config에 대한 학습 내용을 따로 진행해보고 싶은 생각이 들더군요!
아마 현 코드가 최종은 아니라 리팩토링을 진행하게되면 많은 부분이 변할거 같습니다!
어떤 의견이든 수용할 준비가 되어있으니 리뷰 잘 부탁드립니다😊

Comment on lines 22 to 30
public Member checkInvalidLogin(String principal, String credentials) {
Member member = null;
try {
member = memberDao.findByEmailAndPassword(principal, credentials);
} catch(AuthorizationException e) {
e.printStackTrace();
}
return member;
}
Copy link

Choose a reason for hiding this comment

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

"존재하지 않는 email 또는 password 입니다." 이런식으로 예외 처리하면 나중에 더 편할 것 같아요!

Copy link
Author

@gogo1414 gogo1414 Jun 26, 2024

Choose a reason for hiding this comment

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

예외 처리를 꼼꼼하게 못했는데 알려주셔서 감사해요👍

Comment on lines 46 to 48
Jws<Claims> claims = Jwts.parser()
.setSigningKey(secretKey)
.parseClaimsJws(token);
Copy link

Choose a reason for hiding this comment

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

위에랑 통일되게 Jwts.parserBuilder()를 사용하는 것이 어떨까요?

Suggested change
Jws<Claims> claims = Jwts.parser()
.setSigningKey(secretKey)
.parseClaimsJws(token);
Jws<Claims> claims = Jwts.parserBuilder()
.setSigningKey(Keys.hmacShaKeyFor(secretKey.getBytes()))
.build()
.parseClaimsJws(token);

Copy link
Author

Choose a reason for hiding this comment

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

통일성이 떨어지고 있었네요! 해당부분 발견해서 바로 수정했어요!


return !claims.getBody().getExpiration().before(new Date());
} catch (JwtException | IllegalArgumentException e) {
return false;
Copy link

Choose a reason for hiding this comment

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

예외 발생시 로그를 남기는 것이 나중에 원인을 찾을 때도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

문제가 일어나면 어디서 발생했는지 찾을 때 도움이 되겠네요!
해당 부분 참고해서 수정해보도록 하겠습니다.

Comment on lines 56 to 64
private String extractTokenFromCookie(HttpServletRequest request) {
Cookie[] cookies = request.getCookies();
for (Cookie cookie : cookies) {
if (cookie.getName().equals("token")) {
return cookie.getValue();
}
}
return "";
}
Copy link

Choose a reason for hiding this comment

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

cookies가 null일 경우 예외처리를 해주는 것도 좋을 것 같아요!

Copy link
Author

@gogo1414 gogo1414 Jun 26, 2024

Choose a reason for hiding this comment

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

감사해요! 코드의 인덴트도 2가 넘어가서 같이 수정 진행했습니다ㅎㅎ

Comment on lines 19 to 22
public LoginMemberArgumentResolver(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;
}
Copy link

Choose a reason for hiding this comment

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

@Autowired 어노테이션과 함께 생성자 주입을 하여 가독성을 좋게 할 수 있어요!

Suggested change
public LoginMemberArgumentResolver(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;
}
@Autowired
public LoginMemberArgumentResolver(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;
}

Copy link
Author

Choose a reason for hiding this comment

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

@Autowired로 변경하려고 하니 생성자를 통한 주입으로 변경을 스프링에서 추천하길래 잠깐 찾아봤어요! 공유해드리면 좋을 것 같아서 적어봅니다!

@Autowired 어노테이션을 사용하면 의존성 주입이 가능하지만, Spring에서는 생성자를 통한 의존성 주입을 권장합니다. 생성자 주입이 권장되는 이유는 여러 가지가 있습니다:

  1. 불변성: 생성자를 통해 주입된 의존성은 변경될 수 없습니다. 이는 객체의 상태를 더 안전하게 만듭니다.
  2. 테스트 용이성: 생성자 주입은 단위 테스트 작성 시 더 쉽고, Mockito와 같은 라이브러리로 주입할 의존성을 설정하기가 수월합니다.
  3. 순환 참조 방지: 생성자 주입은 컴파일 타임에 순환 의존성을 확인할 수 있도록 해주므로, 런타임에 발생할 수 있는 순환 참조 문제를 방지합니다.
  4. 명확한 의존성: 생성자를 통해 의존성을 주입하면, 해당 클래스가 어떤 의존성을 필요로 하는지 명확하게 알 수 있습니다.
💡 Spring Boot 2.0 이후, `@Autowired`를 필드에 사용하는 것보다 생성자 주입을 더 권장하고 있습니다. 그래서 IDE나 코드 검사 도구가 생성자를 사용하라는 경고를 표시하는 것입니다.

Comment on lines 36 to 37
Long memberId = jwtTokenUtil.getPayload(httpServletRequest);
Member member = memberService.findMemberById(memberId);
Copy link

Choose a reason for hiding this comment

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

토큰이 없거나 회원 정보를 찾을 수 없는 경우 예외 처리를 해주면 좋을 거 같아요!

Suggested change
Long memberId = jwtTokenUtil.getPayload(httpServletRequest);
Member member = memberService.findMemberById(memberId);
Long memberId = jwtTokenUtil.getPayload(httpServletRequest);
if (memberId == null) {
throw new IllegalArgumentException("Invalid or missing JWT token");
}
Member member = memberService.findMemberById(memberId);
if (member == null) {
throw new IllegalArgumentException("Member not found for id: " + memberId);
}

Copy link
Author

Choose a reason for hiding this comment

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

예외 처리 진행했습니다!

Comment on lines +15 to +18
public AuthConfig(LoginMemberArgumentResolver loginMemberArgumentResolver, AuthAdminInteceptor authAdminInteceptor) {
this.loginMemberArgumentResolver = loginMemberArgumentResolver;
this.authAdminInteceptor = authAdminInteceptor;
}
Copy link

Choose a reason for hiding this comment

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

@Autowired 어노테이션을 사용하여 의존성 주입을 명확하게 하는 것도 가독성에 좋을 거 같네요!

Suggested change
public AuthConfig(LoginMemberArgumentResolver loginMemberArgumentResolver, AuthAdminInteceptor authAdminInteceptor) {
this.loginMemberArgumentResolver = loginMemberArgumentResolver;
this.authAdminInteceptor = authAdminInteceptor;
}
@Autowired
public AuthConfig(LoginMemberArgumentResolver loginMemberArgumentResolver, AuthAdminInteceptor authAdminInteceptor) {
this.loginMemberArgumentResolver = loginMemberArgumentResolver;
this.authAdminInteceptor = authAdminInteceptor;
}

Comment on lines 13 to 14
private MemberService memberService;
private JwtTokenUtil jwtTokenUtil;
Copy link

Choose a reason for hiding this comment

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

final로 처리하는 것이 더 좋아보여요!
[Java] final 을 알아보자

Suggested change
private MemberService memberService;
private JwtTokenUtil jwtTokenUtil;
private final MemberService memberService;
private final JwtTokenUtil jwtTokenUtil;

Copy link

@nyeroni nyeroni left a comment

Choose a reason for hiding this comment

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

안녕하세요 준수님!
1학기 수고하셨습니당 ~~!!
전반적으로 잘 작성해주신 것 같아서 딱히 리뷰할 것이 없었네요..
간단한 final 선언이나 의존성 주입 부분, 예외처리 부분에 조금 더 신경을 쓰시면 좋을 것 같아요!!
다른 로직들은 다 잘 작성해주신 것 같아요!! 수고하셨습니다 :)

Comment on lines 16 to 17
private MemberService memberService;
private JwtTokenUtil jwtTokenUtil;
Copy link

Choose a reason for hiding this comment

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

final로 처리하는 것이 더 좋아보여요!
[Java] final 을 알아보자

Suggested change
private MemberService memberService;
private JwtTokenUtil jwtTokenUtil;
private final MemberService memberService;
private finla JwtTokenUtil jwtTokenUtil;

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

Comment on lines +16 to +19
public AuthAdminInteceptor(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;
}
Copy link

Choose a reason for hiding this comment

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

@Autowired 어노테이션을 사용하여 의존성 주입을 명확하게 하는 것도 가독성에 좋을 거 같네요!

Suggested change
public AuthAdminInteceptor(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;
}
@Autowired
public AuthAdminInteceptor(MemberService memberService, JwtTokenUtil jwtTokenUtil) {
this.memberService = memberService;
this.jwtTokenUtil = jwtTokenUtil;

Comment on lines 19 to 20
private MemberService memberService;
private AuthService authService;
Copy link

Choose a reason for hiding this comment

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

위에서 알려드린 거 처럼 final로 선언하시는 것이 좋아보여요!

Suggested change
private MemberService memberService;
private AuthService authService;
private final MemberService memberService;
private final AuthService authService;

Copy link
Author

Choose a reason for hiding this comment

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

수정 진행 했어요!

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.

2 participants