-
Notifications
You must be signed in to change notification settings - Fork 2
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
[BE] feat: 컨트롤러에서 리졸버를 통해 세션 저장 정보를 사용 #1094
Conversation
Test Results166 tests +7 163 ✅ +7 5s ⏱️ -1s Results for commit 2c015ee. ± Comparison against base commit 073cec3. This pull request removes 4 and adds 11 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
리졸버 관련 이전 리뷰에 대한 코멘트 추가했습니다
backend/src/main/java/reviewme/auth/controller/dto/LoginMember.java
Outdated
Show resolved
Hide resolved
...c/main/java/reviewme/auth/controller/exception/GuestReviewGroupSessionNotFoundException.java
Outdated
Show resolved
Hide resolved
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.
패키지에 대해서 이야기해봐요!
@RequiredArgsConstructor | ||
public class GuestReviewGroupSessionResolver implements HandlerMethodArgumentResolver { | ||
|
||
private final SessionManager sessionManager; |
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.
리졸버랑 어노테이션의 위치가 auth.controller여야 할지.. 고민되네요🤔
global.authentication 에 위치하게 하는건 어떤가요?
"인증"은 횡단 관심사라서, global 에 위치하는게 자연스럽다 생각해요.
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.
global 말고, web 패키지를 만들어서 위치하는 건 어떤가요? global은 아무 곳에도 종속되지 않고 전역으로 사용하는 느낌이어서요.
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.
web 이라고 하기에는 또 애매한 것 같아요😭
우리 api 를 앱이 사용할 수도 있으니까..
다른 대안으로 security 를 제안합니다!
관심사별로 모아주기도 하고, global 보다는 목적이 명확해보이는데 어떤가요?
사실 지피티가 추천해줬는데, 일반적으로 많이 쓰인다고도 하고~ 나름 일리적인 것 같아서 가져와봤어요 ㅎㅎ
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.
...c/main/java/reviewme/auth/controller/exception/GuestReviewGroupSessionNotFoundException.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/auth/controller/dto/LoginMember.java
Outdated
Show resolved
Hide resolved
...c/main/java/reviewme/auth/controller/exception/GuestReviewGroupSessionNotFoundException.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/reviewgroup/service/ReviewGroupLookupService.java
Outdated
Show resolved
Hide resolved
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.
반영 및 코멘트 확인 부탁합니다!
@RequiredArgsConstructor | ||
public class GuestReviewGroupSessionResolver implements HandlerMethodArgumentResolver { | ||
|
||
private final SessionManager sessionManager; |
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.
global 말고, web 패키지를 만들어서 위치하는 건 어떤가요? global은 아무 곳에도 종속되지 않고 전역으로 사용하는 느낌이어서요.
...c/main/java/reviewme/auth/controller/exception/GuestReviewGroupSessionNotFoundException.java
Outdated
Show resolved
Hide resolved
...c/main/java/reviewme/auth/controller/exception/GuestReviewGroupSessionNotFoundException.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/reviewgroup/service/ReviewGroupLookupService.java
Outdated
Show resolved
Hide resolved
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.
변경사항 확인했습니다~ 👏👏
마지막으로 한가지! 제안이 있는데,
GuestReviewGroupSessionNotFoundException 이 예외 이름을
NotExists 로 바꾸는거 어떄요?
뭔가 GuestReviewGroupSessionNotFoundException 는 404에 해당하는것처럼 느껴져요😓
사소한거라 테드가 선택적으로 반영하고, 원할 때 머지하세용~
공감합니다~ 해당 부분까지 반영하여 머지합니다! |
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
여기서 Member로 생성하여 반환하지 않는 이유는, 각 도메인에서 Member를 직접적으로 사용할 필요 없이 id만을 사용하기 때문에 굳이 다른 패키지의 객체를 전송할 필요가 없다고 생각했습니다. 내부 필드가 하나여도 dto를 통해 감싸서 전달하는 것이 안전해 보입니다.
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말