-
Notifications
You must be signed in to change notification settings - Fork 1
[Feat] #586 - default User 생성 api 구현 #587
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
Conversation
- CreateUserRequest: persentation layer - UserInfo: Service layer
- id의 경우 인증 서버에서 보내주는 id를 그대로 사용하도록 함
- UserResponseMapper 로 UserInfo를 UserResponse.Create 로 변환
|
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.
전반적으로 기존 컨벤션에 맞춰 코드 잘 짜주셨네요! LGTM~
to reviewers 답변 드리자면,
내부 API Key가 유효하지 않은 경우의 예외를 "유효하지 않은 내부 API key 입니다." 로 처리했는데 좀 더 세세하게 작성하는 게 좋다고 생각하시나요? 우선 이렇게 한 이유는 요청자는 어짜피 본인의 요청이 어느팀에서 보낸건지를 모를 수 없을 테니 어느팀에서 보낸 요청에 대한 문제인지를 메세지로 전달하는 것은 불필요하다고 판단 + 너무 세세한 정보는 노출하지 않는게 좋겠다는 생각이 있었습니다.
-> 이정도면 충분한 메세지라고 생각합니다~
create.sql로 테이블 생성 시 stamp의 user_id 에 대한 물리적 FK, app_user의 PK 전략 두 가지 문제가 존재해서 추후 싱크를 맞출 필요가 있을 것 같습니다. 우선 dev, prod에서 사용하는 DB의 경우 모두 잘 설정되어 있어서 건드리진 않았습니다.
-> 지난기수의 테이블 변경사항을 create.sql에 반영하지 않아서 싱크가 안 맞는 부분들이 있을거에요. 이번 기수에 싱크 맞춰두면 좋겠네요👍
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 | ||
| public UserInfo createUser(Long requestUserId){ |
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.
저번에 재헌님이랑 대화해보면서 나오기도 했었던 읽기/쓰기와 관련해서 코멘트 남깁니다
제가 코멘트 남긴 부분의 createUser 메서드 처럼 (이번에 추가된 메서드가 아닌 기존 메서드도 동일)
Facade 패턴에서 UserFacade 내부에서 트랜잭션을 잡아주고 있는 구조로 이해했어요
또 application/user/UserService에서도 @transactional 으로 설정되어 있는 구조로 이해했는데,
" 실제 쓰기 트랜잭션만 서비스에 두고 파사드/컨트롤러는 읽기에 집중 " 시키는거는 어떨까라는 생각이 들었습니다
저도 파사드 패턴에 대해서는 자세하게 알지 못하지만, Facade 자체에서도 트랜잭션을 잡아주게 되면서 생기는 사이드 이펙트는 없을까 ? 라는 생각에서 시작된 궁금증이였어요
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 룰을 정하면 어떤 이점이 있다고 생각하셨을까요?
제 개인적인 생각으론 필요에 따라서 @transactional 을 설정하는게 바람직한 것 같아요. 사실 지금 구조는 단순 save 메서드만 호출하는 구조이기 때문에 @transactional을 호출하지 않아도 SimplaJpaRepo에서 자동으로 트랜잭션을 걸어주지만, "추후 기능적 확장을 고려했을 때 해당 메서드에 @transactional을 걸어주는게 좋겠다"라는 생각이었습니다. 만약 다른 정보를 가져와서 함께 유저에게 저장해준다던가, 다른 정보도 함께 수정된다던가 할 때는 결국 작업의 원자성을 보장하기 위해서 @transactional을 파사드단에서도 걸어주는게 필요하다는 입장입니다.
혹시 지훈님이 걱정하시는 사이드 이펙트는 어떤 게 있을까요??
Related issue 🛠
Work Description ✏️
Merge 전 수행하기
Trouble Shooting ⚽️
Related ScreenShot 📷
Uncompleted Tasks 😅
To Reviewers 📢