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] 한명수 미션 제출합니다. #26

Open
wants to merge 10 commits into
base: mangsuyo
Choose a base branch
from

Conversation

mangsuyo
Copy link

@mangsuyo mangsuyo commented Jun 23, 2024

다들 종강 너무 축하드려요! 🥳
한 학기동안 너무 고생하셨고 여름방학에도 힘차게 달려봐요!

이번 미션은 저에게 많이 낯선 미션이였습니다 😭
미션 요구사항을 해결하는 데에도 많이 벅찼었네요.!

알려주고 싶거나 얘기하고 싶은 것들을 편하게 리뷰해주시면 감사하겠습니다!

추가로 SecretKey를 외부에 노출하지 않고 application.properties로부터 @Value("${roomescape.auth.jwt.secret}")를 이용해 사용하고 싶었는데, 스캔이 안된 건지 계속 null이 뜨더라고요.

이유를 아신다면 코멘트 달아주시면 감사하겠습니다 :)

@che7371
Copy link

che7371 commented Jun 28, 2024

안녕하세요 명수님! 리뷰가 늦어진 점 사과드리겠습니다. 죄송합니다. 이번 미션은 저에게 많이 낯설고 난이도가 있었던 미션인 것 같아요. 요구사항도 좀 헷갈렸고 아무래도 새로운 개념을 다루다 보니까 더 시간이 걸린 것 같습니다. 그래서 저도 고퀄의 리뷰를 드리고 싶지만 솔직히 리뷰보다는 명수님 코드 보면서 배워가는 게 더 많을 것 같아요! 그래도 열심히 해보겠습니다.

Copy link

@che7371 che7371 left a comment

Choose a reason for hiding this comment

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

리뷰를 남기긴 했는데 사실상 리뷰가 아니라 명수님의 코드에서 제가 배워가는 부분을 남긴 것 같아요. 좋은 리뷰를 남겨드리지 못해 죄송해요. 다음번 리뷰어는 실력이 좋으신 분 만나시길 빌겠습니다. 저도 얼른 공부해서 명수님 뒤를 따라가도록 하겠습니다.😎 포기하지않고 끝까지 해내신 점 멋있어요! 저는 아직 이 부분에 대한 개념이 부족해서 제 눈에는 수정하면 더 좋을 것 같은 점이 보이지 않는 것 같아요. approve 해놓겠습니다!

@Component
public class JwtProvider {

@Value("${roomescape.auth.jwt.secret}")
Copy link

Choose a reason for hiding this comment

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

페어 진행했을 때 키 값을 숨기지 않고 일단 그냥 진행을 했던 것 같은데 잘 해결하셨군요!!

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.

감사합니다 :) 추가로 application.properties 를 감추는 것도 진행해보면 좋을 것 같습니다!

.claim("role", member.getRole())
.signWith(Keys.hmacShaKeyFor(secretKey.getBytes()))
.compact();
}
Copy link

Choose a reason for hiding this comment

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

힌트에 나와있는 토큰 생성을 잘 이용하신 것 같아요! 저는 MemberService 안에 토큰 생성 코드를 넣었는데 이렇게 따로 빼서 하는 방법도 있군요!

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 :)

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.

감사합니다 :)

public String login(MemberLoginRequest request) {
Member member = memberDao.findByEmailAndPassword(request.email(), request.password());
if(member == null){
throw new IllegalArgumentException("Invalid email or password");
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.

감사합니다 :)

),
id
);
}
Copy link

Choose a reason for hiding this comment

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

findById 메소드를 추가하신거군요!

Copy link

Choose a reason for hiding this comment

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

ArgumentResolver를 잘 이해하시고 코드를 짜신 것 같아요. 이번 스터디 참석 못하신 것 같던데 8주차 JPA에 공통 피드백에 트랙 리더님께서 좋은 링크를 걸어두셨어요. 리더님도 정리가 잘 되어있다고 꼭 읽어보라고 하셨는데, 코드 보다가 생각나서 남깁니다. 들어가시기 귀찮으실까봐..ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

챙겨주셔서 감사합니다 !! (안귀찮습니다 감사합니다 👍)

}
return true;
}

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.

감사합니다 :)

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

2 participants