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

6주차 과제 - 로그인 구현하기 #70

Merged
merged 28 commits into from Oct 3, 2022
Merged

Conversation

jdalma
Copy link

@jdalma jdalma commented Sep 12, 2022

로그인이 필요없는 API

  • 로그인 - POST /session
  • 회원 생성하기 - POST /users
  • 회원 수정하기 - POST /users/{id}
  • 회원 삭제하기 - DELETE /users/{id}
  • 고양이 장난감 목록 얻기 - GET /products
  • 고양이 장난감 상세 조회하기 - GET /products/{id}

로그인이 필요한 API

  • 고양이 장난감 등록하기 - POST /products
  • 고양이 장난감 수정하기 - PATCH /products/{id}
  • 고양이 장난감 삭제하기 - DELETE /products/{id}

한 주간 잘 부탁드립니다 ~

@ssisksl77
Copy link

한주간 잘 부탁드립니다!

Comment on lines 32 to 34
if(token == null || token.isBlank()){
throw new InvalidTokenException(token);
}

Choose a reason for hiding this comment

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

어쩌면 XXX.isBlank(token) 이런 함수로 대체할 수 있을 것 같아요!
객체의 메소드를 호출하는 것과 비교해서 위 코드의 장점은 null 체크를 숨기고 무엇을 하는지만 코드에 보이도록 할 수 있다는 거에요!

Copy link
Author

Choose a reason for hiding this comment

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

Spring의 StringUtils로 대체해봤습니다

.compact();
}

public Claims decode(String token) {

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 25
public class AuthenticationService {

private final JwtUtil jwtUtil;

public AuthenticationService(JwtUtil jwtUtil) {
this.jwtUtil = jwtUtil;
}

public String login(){
return jwtUtil.encode(1L);
}

public Claims parseToken(String token) {
return jwtUtil.decode(token);
}
}

Choose a reason for hiding this comment

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

JwtUtil 을 직접 사용하는 것과 비교해서 AuthenticationService 를 쓰는 것은 어떠한 장점이 있을까요?!!

Copy link
Author

Choose a reason for hiding this comment

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

인증JWT 기능에 대한 관심사가 분리되어 있기 때문에 Service가 인증에 대한 책임을 잘 맡으면 다른 도메인 컨트롤러에서도 유용하게 사용할 수 있을 것 같습니다 ㅎㅎ

Choose a reason for hiding this comment

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

좋아요! 하지만 그렇다면! Claims 를 리턴하면 JWT를 명시하게 되는 것이 될거에요!

Copy link
Author

Choose a reason for hiding this comment

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

넵 컨트롤러는 AuthenticationService.tokenValidation(String token)을 호출하게만 했고 서비스는 JWT의 존재는 모르게 userId만 받아서 사용자를 찾게만 해봤습니다.
말씀대로 Claims는 JwtUtil에서만 사용하도록 했습니다.

Comment on lines 44 to 46
if(!StringUtils.hasText(token)){
throw new InvalidTokenException(token);
}
Copy link

@ssisksl77 ssisksl77 Sep 14, 2022

Choose a reason for hiding this comment

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

너무 좋아요! 조금 아쉬운 것은 문자를 가진 것의 반대 보다는 문자가 비어있는 경우 를 테스트하는 것이 좀 더 직관적일 것 같아요!
찾아보니 로그를 직접 유틸클래스를 만들어 볼 수도 있어요!

StringUtils.isEmpty 혹은 Strings.isBlank | Strings.isEmpty 이런식으로 치면 여러가지 함수가 있을 것입니다! 이것들을 참고해보세요!

Comment on lines +21 to +22
@NotBlank
@Size(min = 4, max = 1024)

Choose a reason for hiding this comment

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

한번 경험삼아 메시지를 커스텀 해보는 것은 어떨까요?!

메시지를 커스텀하는 행위는 아주 일반적이니까요!

아니면 나아가서 커스텀 Validator를 만들어보는 겁니다!

Copy link
Author

Choose a reason for hiding this comment

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

{
    "message": null,
    "status": "BAD_REQUEST",
    "errors": [
        "[password] 크기가 4에서 1024 사이여야 합니다",
        "[email] 크기가 3에서 2147483647 사이여야 합니다"
    ]
}

일단 검증 실패 시 검증 정보를 위와 같이 반환하게 수정해봤습니다 042eaaa

커스텀 Validator는 내일 진행해보겠습니다!

Choose a reason for hiding this comment

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

    @Size(min = 4, max = 1024, message = "")

각 유효성검증마다 메시지를 바꿀 수 있는 것이 좋지 않을까요!
또 메시지는 코드가 아닌 외부 파일로 분리하는 것도 좋은 생각일 것 같아요!

Comment on lines 116 to 120
registerUser = userRepository.save((User.builder()
.name(name)
.email(email))
.password(password)
.build());

Choose a reason for hiding this comment

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

메소드 안에서만 쓰이는 변수라면 메소드 안으로 스코프를 줄이는 것이 좋을 것 같아요! 이렇게 하면 final 도 붙일 수 있을 것 같아요!

@ssisksl77
Copy link

ProductService.java 의 경우 상품에 관련된 모든 서비스가 한 곳에 있는 것 같아요!
SOLID 원칙의 S, I 를 접목해보면 어떨까요?

* @return Claims JWT의 페이로드 부분을 반환
*/
public Claims decode(String token) {
if(StringUtils.isEmpty(token) || token.isBlank()){

Choose a reason for hiding this comment

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

Strings.isBlank 이면 하나만으로 predicate이 하나로 해결될 것 같아요!

@jdalma
Copy link
Author

jdalma commented Sep 17, 2022

ProductService.java 의 경우 상품에 관련된 모든 서비스가 한 곳에 있는 것 같아요! SOLID 원칙의 S, I 를 접목해보면 어떨까요?

직접 적용은 못 해봤지만
단일 책임 원칙은 저번 과제에서도 말씀하셨던 조회랑 자원 조작에 대한 책임을 분리하면 될 것 같네요
인터페이스 분리 원칙은 어떻게 적용할 수 있을까요?? 저는 조회,자원 조작에 대한 인터페이스를 만들 것 같은데.. 제가 생각하는게 맞을까요??

@ssisksl77
Copy link

ssisksl77 commented Sep 17, 2022 via email

Comment on lines 37 to 38
User user = mapper.map(loginData , User.class);
User findUser = userService.findByEmail(user.getEmail());

Choose a reason for hiding this comment

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

Suggested change
User user = mapper.map(loginData , User.class);
User findUser = userService.findByEmail(user.getEmail());
User findUser = userService.findByEmail(emailFrom(loginData));

이런식으로 메소드를 추출해본다면 어떨까요? 구현을 감추면 좀 더 가독성이 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

데이터를 요청하지 말고 작업을 요청해라 명심하겠습니다.

@jdalma
Copy link
Author

jdalma commented Sep 18, 2022

아 맞습니다! 인터페이스로 만드는 겸! 이번에는 각 인터페이스마다 하나의 기능만을 가지도록 하는 것은 어떨까요? 조회 분리로 나누는 것보다 더 작게 나눠보는 거에요!

각 인터페이스마다 하나의 기능만을 가지도록 자바 8을 잘 사용하지는 못 하지만 함수형 인터페이스가 떠오르네요
5cb2b86 이런식으로 서비스를 위한 하나의 기능만 가진 인터페이스를 말씀하신게 맞을까요??

맞다면 서비스가 복잡해질수록 구현해야하는 인터페이스는 많아지고 타입을 일일이 지정해줘야하는데 이점이 무엇일지 모르겠습니다 ㅎㅎ
틀렸다면 어디 계층을 위한 기능별 인터페이스를 분리해야할까요??

@ssisksl77
Copy link

5cb2b86 이런식으로 서비스를 위한 하나의 기능만 가진 인터페이스를 말씀하신게 맞을까요??

함수형 인터페이스는 맞습니다. 제가 생각한 것보다 더욱더 범용적인 인터페이스를 만들어주셨군요!
하지만 이 범용적인 인터페이스를 잘 사용하면 되는 것이겠죠!!

만약 User를 생성하는 인터페이스를 만든다면, Create 라는 이름보다는 좀 더 구체적인 이름을 가졌으면 좋겠습니다.

@ssisksl77
Copy link

한주간 회고를 작성하고 링크를 올려주세요!

@jdalma
Copy link
Author

jdalma commented Sep 19, 2022

5cb2b86 이런식으로 서비스를 위한 하나의 기능만 가진 인터페이스를 말씀하신게 맞을까요??

함수형 인터페이스는 맞습니다. 제가 생각한 것보다 더욱더 범용적인 인터페이스를 만들어주셨군요! 하지만 이 범용적인 인터페이스를 잘 사용하면 되는 것이겠죠!!

만약 User를 생성하는 인터페이스를 만든다면, Create 라는 이름보다는 좀 더 구체적인 이름을 가졌으면 좋겠습니다.

특정 도메인 서비스를 위한 한 가지의 기능만 가진 인터페이스 잘 사용하는 방법이 궁금하네요! 가능하다면 다음 과제에서 피드포워드를 이어 주신다면 감사하겠습니다!

한 주간 감사했습니다 ㅎㅎ 수고하셨습니다!
회고

@johngrib johngrib merged commit 39c2010 into CodeSoom:jdalma Oct 3, 2022
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.

None yet

3 participants