-
Notifications
You must be signed in to change notification settings - Fork 56
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 Core] (배포) 안금서 미션 제출합니다. #114
base: goldm0ng
Are you sure you want to change the base?
Conversation
일단 답변부터 말씀드릴게요! 요것도 당연하지만 저번과 동일하게, 졸린 상태로 달다보니 이상한 말이 있을 수 있고, 그럴 때는 그냥 편하게 추가적으로 dm 으로 질문 주셔도 좋고, 댓글 달아주셔도 좋습니다 🙇 프론트엔드 코드까지 뒤져보신 것은 정말 대단하신 것 같아요 👍 (+) 여기서 더 나아가서, 만약 동명이인이 존재한다면 중복 예외가 발생하는 문제가 있을 수 있겠네요..
저는 보면서 이 글이 떠올랐어요 어떤 정책을 가져갈지는 명확하지 않은 상황이잖아요?
|
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.
고생 많으셨어요!
요것도 approve 고요
추가 질문, 수정은 언제나 환영입니다!
솔직히 요정도 했으면 정말 잘했다 싶은 느낌이라서 뭘 더 드려야될지 모르겠네요 ㅎㅎ
앞으로 어떤 방향이 더 좋으실지 질문을 드리면서 마지막 리뷰이지만 첫번째 리뷰를 마무리하려고 해요
- 생각해볼만한 아티클 소개 같이 같이 공부해보면 좋은 것들 위주로 리뷰
- 코드의 디테일 더 챙겨보기 (개인적으로 뭐가 더 나올지 모르겠네요 이미 잘 해주셔서)
- 추가적으로 공부해보고 싶은 것들 적용 후 그 부분에 대해서 리뷰
- 등등등
요 부분을 적어주시면 참고해서 반영하겠습니다!
고생하셨어요
spring.jpa.show-sql=true | ||
spring.jpa.properties.hibernate.format_sql=true | ||
spring.jpa.ddl-auto=create-drop | ||
spring.jpa.defer-datasource-initialization=true | ||
|
||
roomescape.auth.jwt.secret=Yn2kjibddFAWtnPJ2AFlL8WXmohJMCvigQggaEypa5Eaadfasdfsadfsafdsa2df= |
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.
당연하지만 강의때 말씀해주신 것처럼 요 property 는 git에 올리면 안됩니다 🙇
리뷰어의 입장에서는 이게 있으면 편하다보니 감사하기는 합니다 ㅋㅋㅋㅋㅋ
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.
앗 secret 변수 말씀하시는 거죠? 이건 민감한 정보라 git에 올리면 보안상의 문제가 발생할 것 같네요..
강의 내용 참고해서 해당 부분 수정해보도록 할게요!!!
spring.jpa.show-sql=true | ||
spring.jpa.properties.hibernate.format_sql=true | ||
spring.jpa.ddl-auto=create-drop | ||
spring.jpa.defer-datasource-initialization=true |
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.
보통 요 옵션은 application-alpha.properties 와 같은 형태로 특정 프로필로 켰을 때만 on 을 하는 편인 것 같아요!
import roomescape.authentication.jwt.JwtAuthenticationInfoExtractor; | ||
import roomescape.member.Member; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class AuthenticationService { |
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.
Service 가 떨어지면서 service 라는 네이밍은 약간 애매해진 것 같아요!
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.
미션에서 뭘 바라는지는 대충 고민해봤는데요
그냥 단순 개인의 제안으로 봐주세요
AuthenticationServixe가 진짜 service, 어노테이션이 없으면 좋나?는 잘 모르겠어요
반대로 authentication Provider, jwtAuthenticationExtractor는 이대로 있으면 변경하기 쉬울까? 도 잘 모르겠어요
제가 생각하는 방향은 다음과 같은데요
인증 방식이 바뀌었을 때 어디까지 변경이 있어야 할까를 고민해보시면 조금 더 결정하기 쉬우실 것 같아요
지금의 경우에는 authentication Service. AuthenticationConfiguration까지는 바뀌겠죠?
JwtExtractor에 직접 의존하고 있고,
그렇다면 조금 더 적은 범위만 바뀔 수 있을까요?
극단적으로 가면 configuration + 다른 인증 방식의 토큰관련 로직을 당당하는 클래스 정도만 추가되면 인증을 바꿀 수 있도록 하려면 어떻게 해야 할까요?
Authenticationservicw에서 직접 jwt를 의존하지 않고, extractor 의 인터페이스에 의존하고 진짜 service가 되고
Configuration파일에서는 관련되는 extractor 과 같은 인터페이스의 구현체를 등록하면 어떨까요?
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.
Authenticationservicw에서 직접 jwt를 의존하지 않고, extractor 의 인터페이스에 의존하고 진짜 service가 되고
Configuration파일에서는 관련되는 extractor 과 같은 인터페이스의 구현체를 등록하면 어떨까요?
저도 누누님의 의견에 동의합니다! 말씀하신 방식대로 리팩토링 한다면 변경이 발생했을 때, 변경의 범위를 최소화할 수 있을 것 같아요. 인증 방식이 변경되었을 때, WebConfig에 빈으로 등록되는 구현체만 갈아끼워주면 되는 거니까요!
사실 처음에 AuthenticationProvider을 인터페이스로 놓을 때
AuthenticationExtractor도 같이 인터페이스로 두려고 생각했었는데, 아래와 같은 고민 때문에 구현체로만 뒀었는데요!!
<고민사항>
Spring의 인증/인가 방식은 정말 다양햐더군요.
Q. 각각의 인증 방식마다 인증 로직과 방식 등이 다를 것인데,
- 누누님 말씀대로 Extractor를 인터페이스로 둘 경우
현재 구현되어 있는 메서드들이 사용되는 경우도 있을 것이고, 방식이 달라서 추가해야하는 경우도 있을텐데 인터페이스로 두는 게 적절한 방식인건지!? 궁금합니다.
뭔가 인터페이스라고 하면, 공통된 로직에서 확장하여 추가적으로 메서드를 만들어 사용하는 방식이라고 생각이 들어서 더욱 확신이 안 서는 것 같아요.
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.
그런데 이런 고민을 하기 전에 먼저 Spring의 인증 방식에 대해 알아볼 필요가 있는 것 같아,
사용 방식에 따른 인증 방식을 gpt 선생님을 이용해 분류해봤습니다.
<쿠키에서 정보를 추출하거나 사용하는 방식>
-
Form-Based Authentication : 사용자의 로그인 정보는 세션과 연관된 쿠키에 저장됩니다. 서버는 쿠키를 통해 세션을 확인하고 사용자를 인증합니다.
-
Session-Based Authentication : 세션 ID가 쿠키에 저장되며, 이를 통해 서버가 인증 상태를 유지합니다.
-
Remember-Me Authentication : 사용자가 "로그인 상태 유지"를 선택하면 쿠키에 인증 정보 또는 토큰이 저장됩니다.
<토큰에서 정보를 추출하거나 사용하는 방식>
-
Token-Based Authentication (JWT) : 클라이언트가 서버로부터 발급받은 JWT를 사용합니다. JWT는 인증 정보를 포함하며, 서버는 이를 검증하여 사용자 인증 상태를 확인합니다.
-
OAuth 2.0 : 액세스 토큰을 발급받아 API 요청 시 사용합니다. 토큰에 사용자 정보나 권한 정보가 포함될 수 있습니다.
-
OpenID Connect : OAuth 2.0의 확장으로, ID 토큰을 통해 인증 및 프로필 정보를 제공합니다.
-
SAML Authentication : SAML 토큰(XML 형식)을 사용하여 인증 정보를 교환합니다.
<기타 방식 (쿠키나 토큰 비사용 또는 선택적 사용)>
-
Basic Authentication : HTTP 헤더에 Base64 인코딩된 사용자 이름과 비밀번호를 포함하여 인증합니다. 쿠키나 토큰을 사용하지 않습니다.
-
Digest Authentication : 요청 시 헤더에 해시값을 포함하여 인증합니다. 쿠키나 토큰을 사용하지 않습니다.
-
X.509 Certificate Authentication : 클라이언트의 인증서를 사용하여 인증합니다. 쿠키나 토큰과는 관계없습니다.
-
LDAP Authentication : LDAP 서버에서 사용자 정보를 조회하여 인증합니다. 쿠키나 토큰과는 직접적인 관계는 없습니다.
-
Social Login : OAuth 2.0 기반이므로 액세스 토큰이나 쿠키가 사용될 수 있습니다.
-
Multi-Factor Authentication (MFA) : 기본 인증 방식과 조합되며, 인증 방법에 따라 쿠키나 토큰 사용 여부가 달라집니다.
-
Custom Authentication Provider : 개발자가 정의한 방식에 따라 쿠키나 토큰을 사용할 수 있습니다.
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.
이 중 대표적으로 사용되는 인증 방식은 JWT 토큰 인증 방식, OAuth 방식이라고 합니다.
(추가적으로, 토스는 어떤 인증 방식을 사용하는지도 궁금하네요!??)
이 두 인증 방식을 보니, 누누님께서 제안해주신 방법으로 리팩토링해도 현재 코드에서는 제가 질문드린 내용들을 생각하지 않아도 될 것 같네요!
참고해서 리팩토링 진행해보겠습니다~!
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.
저희는 보통
- 전용선 or vpn + oauth2
- mtls
- https + 특정 ip 만 허용 + oauth2 를 사용하는 것 같아요
근데, 이 레이어를 서비스 개발자가 알 필요는 전혀 없었던 것 같아요
인증은 다른 팀에서 전부 처리해주기 때문인데요
누누님 말씀대로 Extractor를 인터페이스로 둘 경우
현재 구현되어 있는 메서드들이 사용되는 경우도 있을 것이고, 방식이 달라서 추가해야하는 경우도 있을텐데 인터페이스로 두는 게 적절한 방식인건지!? 궁금합니다.
뭔가 인터페이스라고 하면, 공통된 로직에서 확장하여 추가적으로 메서드를 만들어 사용하는 방식이라고 생각이 들어서 더욱 확신이 안 서는 것 같아요.
일단 이런 케이스에서는 솔직히 약간 짬인것 같은데요
사용하는 쪽에서 뭐가 필요한지를 먼저 생각해보고 나서 이 기능은 뭐가 되었든 무조건 필요하다 라는 생각이 들고 + 다른 방식으로 바뀔 수 있겠다 정도이면 인터페이스로 분리할 수 있을 것 같아요
|
||
@Configuration | ||
@RequiredArgsConstructor | ||
public class AuthenticationWebConfig implements WebMvcConfigurer { |
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.
보통 요런 클래스는 인증 관련만 다루지 않게 되다보니 webConfig 같이 조금 더 범용적인 네이밍이 되면 더 좋을 것 같아요!
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.
아하! 네 수정하도록 할게요 🫡
그럼 보통은 한 클래스 내에 범용적으로 다루는 편인가요?
A 관련은 AConfig, B 관련은 BConfig 이렇게 나누지 않고요!
즉, 인증 관련 빈만 존재하지 않고 다양한 카테고리가 존재할 경우 말씀드리는 겁니다!
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.
넵 맞아요
스프링에서 그렇게 인터페이스를 만들어두어서 그렇게 진행합니다!
|
||
@ManyToOne | ||
@JoinColumn(name = "time_id") | ||
@OnDelete(action = OnDeleteAction.CASCADE) |
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.
테이블을 현재는 fk가 걸린 상태로 생성될텐데요
단순 질문이에요!
Fk가 있는 것과 없는 것중에 어떤것이 더 좋아보이시나요?
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.
단순 질문 2
직접 로직에서 순서에 맞게 삭제하는 것과 cascade 를 써보는 것을 다 경험해보셨을텐데요
두개중 어떤것이 더 좋으셨나요?
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.
A1. 있는 것이 더 좋아보입니다! 우선, 어노테이션 @manytoone, @joincolumn(name = "time_id") 을 활용해서 명시적으로 표현할 수 있어서 객체 간의 관계를 더 직관적으로 인지할 수 있다는 것이 이점인 것 같아요. 그리고 fk 가 걸린 상태로 객체가 생성되면, "데이터 무결성"을 보장할 수 있다는 게 가장 큰 이점이라고 생각합니다!
-
FK를 사용하지 않았을 경우, 잘못된 데이터 저장 가능성
ex) Reservation 테이블에서 time_id를 단순히 숫자로 저장하고 외래키(FK) 제약이 없다고 가정해봅시다.
INSERT INTO reservation (id, time_id, ...) VALUES (1, 100, ...);
위와 같이 time_id = 100으로 예약을 저장했는데, Time 테이블에는 id=100인 데이터가 없을 수도 있습니다.
이렇게 된다면 Reservation 테이블에 존재하지 않는 시간을 참조하는 데이터가 들어가게 되고, 데이터 일관성이 깨질 수 있습니다. -
FK를 사용했을 경우, 데이터 무결성 보장!
ex) FK 를 사용한다면?
-> reservation.time_id는 반드시 time.id에 존재하는 값만 참조할 수 있습니다.
ALTER TABLE reservation ADD CONSTRAINT fk_time FOREIGN KEY (time_id) REFERENCES time(id);
이에 따라, time_id=100을 가진 데이터가 time 테이블에 없으면 아래와 같은 오류가 발생한다고 합니다.
INSERT INTO reservation (id, time_id, ...) VALUES (1, 100, ...);
-- ERROR: INSERT or UPDATE on table "reservation" violates foreign key constraint "fk_time"
-- DETAIL: Key (time_id)=(100) is not present in table "time".
즉, 잘못된 데이터를 미리 차단해 데이터 무결성을 보장할 수 있습니다!
데이터 무결성 이외에도, 추가적인 FK의 이점을 서치해봤습니다.
-
삭제 시 무결성 유지
만약 time_id=1을 가진 예약이 존재하는데 time.id=1을 삭제하려 하면?
- FK가 있다면 오류 발생 → "예약이 남아있으므로 삭제할 수 없습니다."
이를 방지하기 위해 CASCADE 옵션을 설정하면, time.id=1을 삭제할 때 관련된 reservation도 함께 삭제할 수 있음. -
데이터 정합성 유지
FK가 없으면 개발자가 직접 코드에서 일일이 검증해야 함.
하지만 FK가 있으면 DB 차원에서 자동으로 보장해주므로 개발자가 신경 쓸 필요가 줄어듦.
Q. 누누님의 생각도 궁금합니다! 보통 실무에서는 어떤 FK를 걸어서 사용하나요? 그렇다면 이유는 무엇인가요?
A2. 흠,, 저는 cascade는 어노테이션 하나만으로 부모 엔티티를 삭제하면 자식 엔티티까지 삭제되는 간편함이 너무 좋았습니다!
직접 로직에서 순서에 맞게 삭제하는 것은 코드도 길어지고 번거롭더라고요! 이 방법의 이점은 순서에 맞게 삭제되는 코드를 직관적으로 확인할 수 있다는 것이라고 생각하는데, 매핑된 객체 수가 더 늘어난다면...? 🤔
한 번 두 방식의 장단점을 고민해볼 필요가 있을 것 같아요.
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.
실무에서는 FK 를 전혀 사용하지 않습니다!
무언가의 이유로 데이터를 다른 테이블로 옮기거나 다른 db 로 옮기는 마이그레이션 하는 과정에서 fk 가 걸려있으면 무조건 다 터지다보니 어지간하면 fk 없이 사용하는 것 같아요
fk 가 걸려있다면 테스트 과정도 너무 어렵고요
cascade 에 대한 부분은
https://tecoble.techcourse.co.kr/post/2023-08-14-JPA-Cascade/
이 글에서 장단점이 잘 정리된 것 같아요!
public ReservationController(ReservationService reservationService) { | ||
this.reservationService = reservationService; | ||
} | ||
|
||
@GetMapping("/reservations") | ||
public List<ReservationResponse> list() { | ||
return reservationService.findAll(); | ||
} | ||
|
||
@PostMapping("/reservations") | ||
public ResponseEntity create(@RequestBody ReservationRequest reservationRequest, MemberAuthInfo memberAuthInfo) { |
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.
전체적으로 프로젝트에 ResponseEntity 를 RawType 으로 사용하시고 계신 것이 조금 있는 것 같아요
ResponseEntity
타입으로 null 일 경우를 처리할 수 있고, 나머지의 경우에도 generic 을 추가해주면 좋을 것 같아요!
import roomescape.authentication.jwt.JwtAuthenticationInfoExtractor; | ||
import roomescape.member.Member; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class AuthenticationService { |
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.
저희는 보통
- 전용선 or vpn + oauth2
- mtls
- https + 특정 ip 만 허용 + oauth2 를 사용하는 것 같아요
근데, 이 레이어를 서비스 개발자가 알 필요는 전혀 없었던 것 같아요
인증은 다른 팀에서 전부 처리해주기 때문인데요
누누님 말씀대로 Extractor를 인터페이스로 둘 경우
현재 구현되어 있는 메서드들이 사용되는 경우도 있을 것이고, 방식이 달라서 추가해야하는 경우도 있을텐데 인터페이스로 두는 게 적절한 방식인건지!? 궁금합니다.
뭔가 인터페이스라고 하면, 공통된 로직에서 확장하여 추가적으로 메서드를 만들어 사용하는 방식이라고 생각이 들어서 더욱 확신이 안 서는 것 같아요.
일단 이런 케이스에서는 솔직히 약간 짬인것 같은데요
사용하는 쪽에서 뭐가 필요한지를 먼저 생각해보고 나서 이 기능은 뭐가 되었든 무조건 필요하다 라는 생각이 들고 + 다른 방식으로 바뀔 수 있겠다 정도이면 인터페이스로 분리할 수 있을 것 같아요
|
||
@Configuration | ||
@RequiredArgsConstructor | ||
public class AuthenticationWebConfig implements WebMvcConfigurer { |
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.
고생 많으셨어요!
궁금한 것이 있으시다면 언제든 질문해주세요
해보고 싶으셨던 방향이 성능 향상쪽에 관심이 있으시다고 하셔서 index, unique index, lock 쪽과 관련된 공부해볼 수 있는 키워드 위주로 적어드렸는데요
당연하지만 꽤나 복잡한 내용이고, 난이도가 있다보니 지금 당장 모두 다 적용하시려고 하실 필요까지는 없어요
천천히 어떤 것들이 있구나 하고 이해하는 과정이 있다면 그것만으로도 많이 배우실 수 있을 것이라고 생각합니다! 🙇
private Long id; | ||
|
||
private String name; |
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.
findByName 을 통해서 name 을 기준으로 조회하는 상황인데요
요 테이블에 index 를 추가해보면 어떨까요?
추가가 되면 좋을 인덱스
- name
- email, password
궁금해하셨던 인덱스의 경우에는 사용자가 많아지면 db 에 부하를 줄여줄 아주 좋은 방법이 됩니다!
private String date; | ||
|
||
@ManyToOne | ||
@JoinColumn(name = "time_id") |
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.
요 테이블에서도 time_id, theme_id column 에 대해서 index 를 걸어보면 어떨까요?
프로젝트 전반적으로 한번 쭉 index 를 추가해보아도 좋을 것 같아요!
물론 지금의 경우에는 사용자가 적고, db 에 용량이 적고, h2 라는 메모리 db 를 사용하다보니 장점이 드러나지 않을 수 있을 것 같아요
하지만 mysql 로 옮겨가고, db row 수가 많아지고, 사용자가 많아지면 이 효과가 극대화 될 것으로 보여요
return new WaitingResponse( | ||
waiting.getId(), | ||
waiting.getName(), | ||
waiting.getTheme().getName(), | ||
waiting.getDate(), | ||
waiting.getTime().getValue(), | ||
(long) waitings.size()); | ||
} |
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.
findBy 를 통해서 전체를 조회하는 것 대신 count 쿼리를 통해서 처리하면 조금 더 성능을 올릴 수 있을 것 같아요!
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class Waiting { | ||
|
||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long id; | ||
|
||
private String name; | ||
|
||
private String date; | ||
|
||
@ManyToOne | ||
@JoinColumn(name = "time_id") | ||
@OnDelete(action = OnDeleteAction.CASCADE) | ||
private Time time; | ||
|
||
@ManyToOne | ||
@JoinColumn(name = "theme_id") | ||
@OnDelete(action = OnDeleteAction.CASCADE) | ||
private Theme theme; | ||
|
||
@ManyToOne | ||
@JoinColumn(name = "member_id") | ||
private Member 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 + theme + time 을 기준으로 unique index 를 걸면 어떨까요?
https://velog.io/@hwan2da/JPA-Columnuniquetrue-UniqueConstraints
현재 구조상으로 중복을 예약 생성시에 검사해주고 있지 않아서 다음 케이스에서 문제가 될 수 있을텐데요
대기 생성 요청이 10번정도 들어오면, 같은 유저가 같은 theme 에 같은 시간에 10개의 예약을 만들 수 있을 것 같아요
그러면 현재 구조상에서 봤을 때 직관적이지 않은 동작을 할 것 같아요
import roomescape.theme.Theme; | ||
import roomescape.time.Time; | ||
|
||
@Entity | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class Reservation { |
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.
요 테이블에 time_id, theme_id, member_id 를 기준으로도 unique Index 를 추가해보면 어떨까요?
이것도 동일하게 unique 를 보장하면 한 유저가 동시에 2번의 예약을 하는 경우를 막을 수 있을 것 같아요
private void validateDuplicateReservation(ReservationRequest reservationRequest) { | ||
boolean exists = reservationRepository.existsByDateAndTimeIdAndThemeId( | ||
reservationRequest.date(), | ||
reservationRequest.time(), | ||
reservationRequest.theme() | ||
); | ||
|
||
if (exists) { | ||
throw new DuplicateReservationException("이미 예약이 존재합니다."); | ||
} | ||
} |
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.
이 부분은 약간 고민이 필요한 부분인데요
현재 적어주신 방법으로는 완벽한 중복 방지를 진행할 수가 없어요
예약이 되어있지 않은 상황에서 2번의 요청이 거의 동시에 따닥 사용자로부터 들어오고
2개의 요청이 거의 동시에 validateDuplicateReservation 함수를 호출했고
그 결과 양쪽 다 exists 가 false 로 등록이 될 경우에는 예약이 중복으로 진행될 수 있을 것 같은데요
지금 적용하기에는 꽤나 난이도가 있는 작업이라서 나중에 다시 읽어보셔도 좋을 것 같아요
validateDuplicate 를 하기 전에 테이블에 lock 을 잡고, 저장 이후에 lock 을 해제하는 그런 작업을 진행하게 된다면 이런 문제에서 안전하게 될텐데요
나중에는 이쪽을 읽어 보시는 것도 좋을 것 같아요!
https://juno-juno.tistory.com/111
안녕하세요. 누누님! 드디어 마지막 미션이네요 ⛳️
지금까지 열정적으로 리뷰해주셔서 감사합니다!
리뷰어님들 덕분에 저희가 무리 없이 무럭무럭 성장할 수 있었던 계기가 되었던 것 같아요.
약 5개월동안 진행된 그리디 리뷰 활동이 앞으로의 제 성장에 밑거름이 될 거라 믿어 의심치 않습니다 🙉
Spring Core 미션은 지난 Spring Data JPA 미션의 구현 및 리팩토링을 진행했을 때와 겹치는 부분이 조금 있어서 훨씬 수월하게 진행할 수 있었던 것 같습니다! 이번 미션도 지난 미션과 마찬가지로 진행하면서 겪었던 생겼던 시행착오나 고민사항들에 대해 말씀드리고 조언을 구하고 싶습니다!
<겪었던 시행착오 및 해결방안 혹은 고민사항>
[시행착오] 데이터 중복으로 인한 오류
로깅을 진행해보니, 이 예외는 "/login/check"가 Get 방식으로 요청이 들어올 때 로그인 상태인지 체크하는 로직에서 발생하고 있었습니다.
관련 코드 첨부하겠습니다.
roomescape.login.LoginController
roomescape.login.LoginService
query did not return a unique result: 2 예외는 쿼리 실행 결과가 두 개 이상의 레코드를 반환했을 때 발생한다고 합니다.
이는 findByName(memberAuthInfo.name()) 메서드가 반환한 결과가 고유하지 않다는 것을 의미하죠.
즉, 데이터베이스에 동일한 name을 가진 여러 Member가 존재하고 있다는 뜻입니다.
관련 예외는 "/login/check"가 Get 방식으로 요청이 들어올 때 로그인 상태인지 체크하는 로직에서 발생하고 있었는데요!
user-scripts.js
우선, 에러의 발생은 로그인 상태 확인 API가 호출되었을 때 해당 데이터 중복 예외의 발생으로 인해 catch문으로 들어가 발생한다는 것을 알 수 있습니다.
user-scripts.js
어느 요청을 보내도 계속 에러가 발생했던 이유는 이 코드와 관련이 있을 것 같습니다.
해당 코드는 HTML 문서가 완전히 로드되고 DOM이 준비되었을 때 실행되도록 설정된 이벤트 리스너입니다.
따라서 브라우저가 페이지를 로드할 때마다 updateUIBasedOnLogin() 함수가 호출이 되므로,
어느 요청을 보내도 로그인 상태 확인 로직은 계속 호출이 되고, 그와 동시에 초기 데이터들은 중복으로 존재하여 중복 관련 예외로 인해 계속 에러가 발생했던 것입니다!
(+) 여기서 더 나아가서, 만약 동명이인이 존재한다면 중복 예외가 발생하는 문제가 있을 수 있겠네요..
1) 중복 이름에 대한 예외처리를 추가하거나,
2) 해당 메소드를 이름으로 조회하는 게 아닌 고유한 회원 아이디로 조회하기
빈 등록
case1) @configuration과 @bean을 활용해 수동으로 빈을 등록하는 경우
case2) @Autowired와 @component의 어노테이션 활용으로 빈을 등록하는 경우
Q. 둘 중 수동으로 빈을 등록했을때의 장점이 뭐라고 생각하시나요?
보통 어떤 경우에 그렇게 구현하시나요?
데이터 초기화 작업에 관한 고민사항
case1) 초기 데이터를 (data.sql)과 같은 파일로 정의하는 경우
[장점]
[단점]
- 데이터 구조 변경 시, SQL 을 직접 수정해야 하므로 번거롭다.
- 대량의 데이터 초기화는 복잡하고 가독성이 떨어질 수 있다.
- 테스트 및 운영 환경별로 다른 데이터를 설정하기 어렵다.
- 객체 간 관계를 직관적으로 정의하기 어렵다.
case2) 이번 Core 미션처럼 DataLoader 및 TestDataLoader를 활용해 데이터를 초기화 했을 경우
[장점]
- Java 코드로 작성하므로 유지보수와 확장이 쉽다.
- 객체 간의 관계를 명확히 표현 가능하다.
- @Profile 어노테이션을 활용하면 테스트, 개발, 운영 환경에 따라 다른 데이터를 초기화할 수 있다.
- 엔티티 간 관계를 Java 객체의 참조로 표현할 수 있으므로 관계형 데이터 초기화가 명확하다.
- 단순 데이터 삽입뿐만 아니라, 데이터 생성 로직을 포함할 수 있어 복잡한 초기화 과정도 처리 가능하다.
[단점]
- 초기화 데이터가 많아질수록 애플리케이션 실행 속도가 느려질 수 있다.
- 초기화 데이터를 직접 코드로 작성해야 하므로, 소규모 데이터에는 오히려 번거로울 수 있다.
Q. case1와 case2를 비교해보면, case2의 이점이 훨씬 커보이는데 누누님도 동의하시나요?
그렇다면 실제 회사나 프로젝트에서는 둘 중 뭐를 더 많이 사용하는 편인가요?
이건 spring-mvc 때 리뷰를 받고 반영한 내용이긴 합니다만!
누누님의 생각이 궁금해서 여쭤봅니다.
기존 login 패키지에 있는 클래스들과 authentication에 있는 클래스들이 jwt 패키지에 강하게 결합되어 있어, 만약 인증 방식이 바뀌거나 하는 등의 변경 사항이 생기면 번거로운 문제가 생길 수 있기 때문에 이를 방지하고자 리팩토링을 해봤는데요!
인증 방식 변경에 대비해 인증 방식을 구현한 구현체를 바꿔 낄 수 있도록 AuthenticationProvider이라는 인터페이스를 두고 구현체를 따로 정의하여 구현해봤습니다.
기존에 있던 JwtUtils는 유틸성 클래스는 아니라고 판단이 들어, 해당 클래스의 역할을 잘 나타낼 수 있는 클래스이름인 JwtAuthenticationInfoExtractor으로 변경하였습니다.
그 후 AuthenticationProvider의 구현체인 JwtAuthenticationProvider와, jwt 토큰 관련 로직들을 담아놓은 JwtAuthenticationInfoExtractor 를 한 곳에서 관리할 수 있도록 AuthenticationService를 만들어,
인증 관련 로직을 호출할 때에는 AuthenticationService를 주입받아 활용하도록 구현했습니다.
Q1. 우선, 이 리팩토링 방식에 대해 코멘트 주실 것이 있다면 부탁드립니다!
다음은 이번 Core 미션의 7단계 요구사항 중 일부인데요.
Q2. JWT 관련 로직을 하나의 클래스로 이동 시키라고 되어있는데,
제 코드의 경우 인증 방식을 제공하는 인터페이스를 따로 두었어서 한 곳에 모으긴 좀 어려울 것 같다고 판단이 들었습니다.
그래서 인증 로직을 한 곳에서 관리해주는 AuthenticationService를 빈으로 따로 등록했는데, 이게 괜찮은 구현 방식인건지 궁금하네요!
마지막 리뷰도 항상 그래왔듯이 날카롭게 부탁드립니다!
화이팅!!!
👊🏻