[톰캣 구현하기 - 3, 4단계] 후추(주찬민) 미션 제출합니다. #451
Conversation
tomcat/src/main/java/nextstep/jwp/controller/AbstractController.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/nextstep/jwp/controller/LoginController.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/nextstep/jwp/controller/LoginController.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/nextstep/jwp/controller/ResourceController.java
Outdated
Show resolved
Hide resolved
| public static RequestMapping init(SessionManager sessionManager) { | ||
| AuthService authService = new AuthService(sessionManager, InMemoryUserRepository.init()); | ||
| return new RequestMapping( | ||
| List.of( | ||
| new HomeController(), | ||
| new LoginController(authService), | ||
| new RegisterController(authService), | ||
| new ResourceController() | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
RequestMapping을 요구사항에 맞게 잘 구현해주신 것 같아요!
하지만, AuthService가 결국 InMemoryUserRepository 때문에 RequestMapping까지 퍼진 느낌이에요ㅠㅠ
앞선 코멘트에서 계속 이야기 나누면 좋을 것 같습니다!
There was a problem hiding this comment.
AuthService 를 여기에서 초기화한 이유는 LoginController, RegisterController 가 동일한 AuthService를 사용하게 하기 위함이었습니다. AuthService 는 sessionManager와 InMemoryUserRepository를 생성자에서 주입 받아 초기화되는데, 서로 다르게 생성된 AuthService 인스턴스를 사용하면, 서로 다른 DB를 사용하는 것처럼 동작할테니까요.
디투의 말처럼 InMemoryUserRepository 가 static 으로 동작했다면, 각각 Controller 내에서 AuthService를 초기화해도 되겠어요. 그러면 RequestMapping이 AuthService를 참조하는 것도 끊을 수 있겠어요.
하지만 지금은 static으로 변경하지 않고 두겠습니다. static으로 바꾸는 것이 객체지향을 따르지 않는 느낌입니다. 또한 참조를 끊기 위해 static으로 만드는 것이 완벽한 해결책이라고 할 수 없을 것 같아요. 오히려 Spring의 ApplicationContext 와 같은 컨테이너를 만들고 싶은 시점이네요!
앞으로 다른 미션을 진행하면서 ApplicationContext를 만들어볼 기회가 있지 않을까 기대하면서 기다리겠습니다 ㅎㅎ
There was a problem hiding this comment.
그냥 기다리기만 하는 것도 뭐해서 singleton으로 리팩터링하고, RequestMapping과 AuthService 간의 의존관계를 끊었습니다 👍
세세한 리뷰 감사드려요!
There was a problem hiding this comment.
static으로 객체를 관리하는 것은 후추 말씀대로 객체지향에서 어긋나죠. 또, RequestMapping에서 AuthService를 주입시켜주는 것 또한 결합도가 높아지죠.
그래서 저는 스프링이 이런 문제들을 해결하기 위해 싱글턴으로 빈을 관리한 것이라고 생각했어요. 싱글턴 방식을 제안한 것도 같은 이유였습니다!
빈 자동 주입 방법에 대해 자세히 뜯어보지는 못했지만, 제가 알기로
- 의존성을 주입하기 전에 모든 빈을 싱글턴으로 생성한다.
- 의존성 주입을 하는데, application context에 있는 빈을 주입한다.
이런 포인트에서 저는 아래와 같은 코드를 생각했었습니다!
public static RequestMapping init() {
return new RequestMapping(
List.of(
new HomeController(),
new IndexController(),
LoginController.init(),
RegisterController.init(),
new ResourceController()
),
new ExceptionHandler()
);
}DI를 구현할 수는 없으니 최대한 DI에 맞도록 객체 본인에 대한 의존성만 관리하도록이요. RequestMapping은 Controller만, LoginController는 AuthService만, AuthService는 SessionManager와 InMemoryRepository 만으로 생각했었습니다.
There was a problem hiding this comment.
텍스트만으로 부족할 것 같아 그림 그려보았는데요. 저 Application Context가 마치 있다고 가정하고 같은 객체를 여러 곳에 의존성으로 주입시켜줘야하는 경우에 싱글턴 객체를 만들어 주입시켜주는 것은 어떤가? 하고 의견을 제시했던 거에요.
public static AuthService init() {
this.sessionManager = SessionManager.getInstance();
this.inMemoryUserRepository = InMemoryUserRepository.getInstance();
}이런 느낌으로요!
There was a problem hiding this comment.
그러면 SessionManager의 map이나 InMemoryRepository의 map을 static으로 둘 필요도 없어지죠!
There was a problem hiding this comment.
그림과 글로 친절히 설명해주셔서 감사합니다! 디투의 생각을 정확하게 이해하게 되었어요!
그리고 디투의 이전 리뷰의 의도도 제가 잘 이해한 것이 맞다고 생각하게 되었습니다.
디투의 리뷰를 통해 RequestMapping가 AuthService를 의존하던 관계를 끊게 되었기 때문이에요. 마지막 리뷰 때 RequestMapping의 init() 메서드도 아래와 같이 변했어요!
public static RequestMapping init() {
return new RequestMapping(
List.of(
new HomeController(),
new IndexController(),
new LoginController(),
new RegisterController(),
new ResourceController()
),
new ExceptionHandler()
);
}지금 코드에서 디투의 생각과 다른 부분은 LoginController 등에서 AuthService를 초기화하면서, LoginController가 SessionManager, InMemoryUserRepository 등을 의존하게 된 점이에요. 이 부분은 제가 의존관계를 꼼꼼히 끊어내지 못했던 부분이네요. 리팩터링을 하면 좋겠어요!
아무튼 전하고 싶은 말은, 디투의 리뷰가 잘 전달되었고, 제가 디투께 잘 배웠다는 사실입니다. 감사드립니다 😄
There was a problem hiding this comment.
안녕하세요 후추, 1,2 단계 코멘트도 확인했습니다!
추가 코멘트도 남겨놓았으니 확인해주시면 감사하겠습니다!
어떻게 분기 처리하면 좋을지 고민하셨던 것 같은데, 분기가 너무 깔끔하게 되어있는데요? 찾으려 했는데 어떤 부분인지 찾고 싶었는데, 제 눈에는 보이지 않았어서요... 혹시 어느 부분인지 말씀해주시면 다시 그 부분에 대해 깊게 고민해보겠습니다!
추가적으로, InmemoryRepository 관련해서 코멘트를 남겼습니다!
나머지 부분들에 대해서는 간단한 리뷰를 남겼으니 확인해주시면 감사하겠습니다!
그 수많은 if 문들을 이렇게 빠르게 처리하시다니... 대단하십니다. 주말 하루종일 하신 것 같아요. 정말 고생하셨습니다.
|
Kudos, SonarCloud Quality Gate passed!
|
|
안녕하세요 디투 지난 3,4 단계 리뷰 감사했습니다! 답변 모두 달아두었어요 👍 다시 한 번 리뷰 요청 드립니다! 변경된 점은 크게 아래와 같아요
예외 처리를 더 세세하게 할 수 있겠지만, 일단 예외 시 404 혹은 401만 반환하도록 했습니다. 리뷰 잘 부탁드립니다 😄 |
| public void handle(HttpResponse httpResponse, Exception e) { | ||
| if (e instanceof AuthException) { | ||
| ResourceManager manager = ResourceManager.from(UNAUTHORIZED_URL); | ||
| httpResponse.setResponseResource( | ||
| UNAUTHORIZED, | ||
| manager.extractResourceType(), | ||
| manager.readResourceContent() | ||
| ); | ||
| return; | ||
| } | ||
| if (e instanceof NotFoundException) { | ||
| ResourceManager manager = ResourceManager.from(NOT_FOUND_URL); | ||
| httpResponse.setResponseResource( | ||
| NOT_FOUND, | ||
| manager.extractResourceType(), | ||
| manager.readResourceContent() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
예외를 handle하는 것 완전 찬성합니다! 그런데 instanceof 가 조금 마음아프셨을 것 같은데요. 저도 해결할 수 있는 방법이 보이지 않아 저도 마음이 아프네요ㅠㅠㅠㅠ
There was a problem hiding this comment.
https://stackoverflow.com/questions/20431614/throw-and-catch-an-exception-or-use-instanceof
이번에 다시 공부하면서 스택오버플로우에서 이런 질문을 찾았습니다. try, catch 로 잡으면 어떨까 싶긴 한데, 더 좋은 방법은 안 떠오르네요..ㅎㅎ
| user.setId(id); | ||
| return id++; |
There was a problem hiding this comment.
와 id 증가도 구현하셨네요! 진짜 DB가 된 것 같아요 짱입니다!!
There was a problem hiding this comment.
제가 리뷰이에게 AutoIncrement를 제안했거든요. 그런데 제안만 하면 제가 리뷰를 못하니까, 저도 해봤습니다 ㅎㅎ
| private static class LazyHolder { | ||
| private static final InMemoryUserRepository INSTANCE = InMemoryUserRepository.init(); | ||
| } |
There was a problem hiding this comment.
해당 이너 클래스를 만든 것이 의도하신 것인지 궁금해요!
InMemoryUserRepository.getInstance() 을 실행하면 LazyHolder 가 다시 InMemoryUserRepository.init()을 호출하고 database가 초기화됩니다! 그렇다는 것은, getInstance()와 init()의 차이점이 없어보입니다!
There was a problem hiding this comment.
넵 의도한 게 맞습니다!
싱글턴을 만들면서, 동시성 이슈로 인한 문제가 발생하지 않을까 고려했습니다.
그래서 LazyHolder 방식을 사용했습니다.
https://blog.javarouka.me/2018/11/20/no-instance/ 관련 자료입니다!
참고로 init() 메서드는 private로 바꿔야 하는데 또 놓쳤네요. 코딩 졸릴 때 하면 안 되는 것 같습니다 하하
| throw new IllegalArgumentException("존재하지 않는 계정이거나 비밀번호가 틀렸습니다."); | ||
| } | ||
| User user = inMemoryUserRepository.findByAccount(account) | ||
| .orElseThrow(() -> new AuthException("존재하지 않는 계정이거나 비밀번호가 틀렸습니다.")); |
There was a problem hiding this comment.
| .orElseThrow(() -> new AuthException("존재하지 않는 계정이거나 비밀번호가 틀렸습니다.")); | |
| .orElseThrow(() -> new AuthException("존재하지 않는 계정입니다.")); |
There was a problem hiding this comment.
이 예외 메시지는 일부러 두루뭉술하게 표현했습니다.
존재하지 않는 계정인지, 비밀번호가 틀린 것인지 정보를 밝히지 않기 위함입니다.
"비밀번호가 틀렸습니다" 라는 메시지는 입력한 계정이 존재한다는 정보를 내재하고 있습니다. 이는 해킹 시에 악용될 여지가 있다고 판단했습니다!
| return session.getId(); | ||
| } | ||
|
|
||
| private void validatePassword(String password, User user) { | ||
| if (!user.isSamePassword(password)) { | ||
| throw new AuthException("존재하지 않는 계정이거나 비밀번호가 틀렸습니다."); |
There was a problem hiding this comment.
| throw new AuthException("존재하지 않는 계정이거나 비밀번호가 틀렸습니다."); | |
| throw new AuthException("비밀번호가 틀렸습니다."); |
윗 코멘트와 마찬가지로 자세히 표현할 수 있을 것 같아 제안했습니다!
안녕하세요 디투
3,4 단계 리뷰 요청 드려요!
지난 PR 리뷰에 대해 커멘트를 남겼어요. 여기에서 확인할 수 있어요!
고민을 하고 있는 부분은 예외 처리와 디투의 리뷰입니다.
현재는 예외 처리를 적절히 하지 못하고 있고, 모든 자원 요청을 그대로 보여주고 있어요.
둘 다 한번에 대응하는 코드를 짜고 싶은데 쉽게 떠오르지 않네요.
다른 부분부터 리뷰해주신다면 감사하겠습니다!