Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(인증)] 정원영 미션 제출합니다. #117
[Spring MVC(인증)] 정원영 미션 제출합니다. #117
Changes from 25 commits
3c2620a
751823c
0587b38
95214f6
e5fa217
fcc4e23
a4c1804
e059346
7863060
7c7ee83
b526907
95f6ebf
6d28e8f
34a3e9e
c2275a7
c59ae63
556d351
3ce8e0f
7521e7f
e9cc2ec
288dc07
9e7f03b
95cdb12
bf3068b
eca6383
996a524
2c2ca03
8f96052
85e3be5
4d861b5
ec3f8cf
a47ee15
9dd8f56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분에 member 가 존재하는 이유가 있나요? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 마지막에 고친부분이 잘못 고친거였네요... 저도 이부분을 보면서 "member가 필요없는데? 쿼리가 두번 나가네?" 하고 바로 삭제해버린것 같습니다. 반환값에 member에 대한 정보가 필요해서
findByEmailAndPassword
반환값을 받으려고 했습니다. 아마 현재 코드로 테스트를 돌리면 통과하지 못할 것 같네요..푸시하기전에 테스트를 돌리고 푸시했어야했는데 아주 기초적인 실수를 했네요.. 수정하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration 을 선언한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenService에서 주입받아서 사용하려고 했습니다!
메서드로 빈을 등록하거나 하지는 않지만 jwt에 대한 설정을 나타내는 것 같기도 해서 붙였습니다. 근데 그냥
@Component
를 붙이는게 더 맞는 것 같기도하네요There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 질문한 이유는
ConfigurationProperteis
-@EnableConfigurationProperties
로 해도 될거 같은데 Configuration 을 선언해서 질문했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record 여도 될 거 같네용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigurationProperties를 위해서 setter 메서드가 존재해야하는 줄 알았는데 지금보니 생성자도 지원하게 바뀌었다고 하네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final 을 붙여도 괜찮을 거 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생각해보니 아직 JPA 가 아니군요.😭
비밀번호 검증 부분은 나중에 JPA 부분에서 다시 도전해봐도 될 거 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드에 Transactional 을 붙인 이유가 있나요?
( 틀린게 아니라, 의견을 묻는 겁니당. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 현재 상황에 좋은 방식은 아니라고 생각합니다.
붙인 이유
안좋다고 생각하는 이유
setUp()
을 안해도 됐을 것이다. (사실 이게 제일 좋은 방법이라고 생각합니다.)현재 상황외
@Transactional
을 테스트에 붙이는 것이 좋은 것인가는 조금 고민을 해봐야할 것 같습니다. 습관적으로 붙인다면 전파 옵션이 Requires_new인 상황에 매우 복잡해집니다. 그래서 저는 습관적으로@Transactional
붙이는 것을 지양하고 있습니다. 가능하면 로직을 다 알고 있을 때 해당 어노테이션을 붙이는게 좋은 것 같아요 😁There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 말한 내용들도 맞긴 하지만
가장 큰 문제점은 테스트 메소드가 트랜잭션으로 묶여서 기존 로직과 의도하지 않은 결과가 나올 수 있는걸로 알고 있어요.
이에 대해선
테스트에 Transactional
만 쳐도 많이 나올거니 제가 자세히 말하는건 생략할게요 🙂저희 팀은 로직이 엔티티의 특정 상태를 바꾸는 것이라면
@Transactional
을 사용했던거 같아용.그리고
해당 부분과 관련해서
JPA Transactional 잘 알고 쓰고 계신가요?
성능적으로도 측정한 아티클 있어서 남기고 갑니다~