Skip to content
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

[MVC 구현하기 - 3단계] 블랙캣(송우석) 미션 제출합니다. #591

Merged
merged 13 commits into from
Sep 26, 2023

Conversation

Songusika
Copy link

@Songusika Songusika commented Sep 25, 2023

안녕하세요 저문!
미션을 잘못 이해해서 제출이 늦어졌네요 😅

이번 미션을 진행하면서 저번 step2에서 마지막에 남겨주셨던 질문의 답변 남겨놓을게요!

DispatcherInitializer는 왜 app모듈에 두셨는지 의견이 궁금해요.

당시 step2 미션에는 점진적인 리팩터링이 목적이었어요!
지금은 없어졌지만 ManualHandlerMapping 이 app 모듈에 있고 DispatcherInitializer 를 mvc 모듈로 옮기게 된다면 프레임워크가 app 을 의존하는 형태이기 때문에 당시에는 app 모듈 내부에 두었습니다!

현재는 프레임워크 단에서 서블릿 컨텍스트를 초기화할 수 있도록 DispatcherInitializer 또한 mvc 모듈로 이동하고 싶지만
AnnotationHandlerMapping 에서 basePackage 위치를 app 단의 controller 패키지에 잡아주어야 하는데 아직해결하지 못했습니다.

new AnnotationHandlerMapping("com.techcourse.controller")

이부분에 대해선 다음 리뷰 요청 때 반영할게요!

구현 사항 🚀

  • 레거시 MVC 제거
  • Jsp View 구현
  • Json View 구현
  • @ RequestBody 구현
    • 처음에 미션을 정확히 이해하지 못해서 request 요청의 body까지 json 으로 받아야하는 줄 알았습니다. 😭
  • ArgumentResolver 구현
    • 컨트롤러 HttpServletRequest/Response 를 항상 사용하지 않더라도 꼭 파라미터로 선언하는게 불편해서 구현해 보았습니다!
    • 현재 지원하는 파라미터는 @RequestBody, @RequestParam, HttpServletRequest/Response, HttpSession 입니다.
      • 위의 타입 중 개발자가 원하는 파라미터만 작성해도 됩니다. 😀
    • ArgumentResolvers 객체 내부에서 파라미터의 순서에 맞게 변환한 뒤 Object 배열로 반환합니다.
    • 이후 AnnotationHandlerAdapter 에서 실제 메서드를 호출하는 시점에 인자로 넘겨주고 있어요!

@RequestBody 나, ArgumentResolver 부분은 미션 요구사항이 아니여서 저문이 편하신대로 리뷰해주셔도 좋습니다!

@MVC 마지막 미션 리뷰 감사합니다!! 🙇‍♂️

@jeomxon jeomxon self-requested a review September 25, 2023 08:21
Copy link

@jeomxon jeomxon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 블랙캣 :)
3단계도 정말 잘 구현해주셨네요!

저도 어느정도 고민했던 부분인데, 사용되지 않는 HttpServletRequest, HttpServletResponse없이도 Controller가 동작할 수 있도록 변경해주신 부분이 인상깊었습니다.
사소한 코멘트들 남겨두었는데 답변 기다리겠습니다.

추가적으로 의견 나누고 싶으시다면 언제든지 코멘트나 DM주세요😀

Comment on lines 16 to 30
@RequestMapping(value = "/register", method = RequestMethod.POST)
public ModelAndView save(HttpServletRequest req, HttpServletResponse res) {
public ModelAndView save(@RequestBody final RegisterRequest registerRequest) {
final var user = new User(2,
req.getParameter("account"),
req.getParameter("password"),
req.getParameter("email"));
registerRequest.getAccount(),
registerRequest.getPassword(),
registerRequest.getEmail());
InMemoryUserRepository.save(user);

return new ModelAndView(new JspView("redirect:/index.jsp"));
}

@RequestMapping(value = "/register", method = RequestMethod.GET)
public ModelAndView show(HttpServletRequest req, HttpServletResponse res) {
@RequestMapping(value = "/register/view", method = RequestMethod.GET)
public ModelAndView show() {
return new ModelAndView(new JspView("/register.jsp"));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoginController는 view에 대한 클래스가 분리되어있는 것으로 확인이 됩니다.
사소하지만 둘 중 하나로 통일성있게 변경해보는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미쳐 확인하지 못했네요!
회원가입 컨트롤러와 같이 하나의 컨트롤러로 통합하였습니다!

private String password;
private String email;

public RegisterRequest() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto패키지 내부에 있는 클래스들에 대해서는 public생성자를 명시해주셨네요.
이유가 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectMapper 는 json 을 역직렬화 할 때 기본 생성자가 없다면 InvalidDefinitionException 예외가 터지게됩니다.
이는 Reflection API 가 클래스를 인스턴스화 할 때 기본생성자를 사용하기 때문입니다.
(@Entity 가 붙은 도메인 객체에서 기본 생성자를 넣어주는 이유와 같습니다.)

참고로 스프링에서는 기본적으로 추가적으로 jackson-module-parameter-names 모듈을 통해서 기본 생성자가 없는 경우 다른 방법(매개변수가 있는 생성자 등) 을 찾아서 역직렬화를 해준다고합니다!

Comment on lines 73 to 82
public Optional<? extends Class<?>> getMethodParameterAnnotatedWith(
final Class<? extends Annotation> annotation,
final Method method
) {
return Arrays.stream(method.getParameters())
.filter(it -> it.isAnnotationPresent(annotation))
.map(Parameter::getType)
.findFirst();
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReflectionUtils는 static하게 사용하는 클래스로 알고 있었는데, 이 메서드는 어떤 역할을 하고 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RequestBody 어노테이션을 만들면서 필요하다고 생각한 메서드였는데
Resolver 로 분리하면서 해당 메서드가 필요가 없어졌네요!
원래는 인자로 넘겨준 어노테이션이 선언된 메서드의 파라미터의 타입을 반환하는 메서드입니다!
필요 없어져서 제거하겠습니다!

Comment on lines 7 to 14
public static Object serialize(final String json, final Class<?> target) throws Exception {
return new ObjectMapper().readValue(json, target);
}

public static String deserialize(final Object object) throws Exception {
return new ObjectMapper().writeValueAsString(object);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드를 매번 호출할 때마다 ObjectMapper가 생성되고 있는 것이 아닌가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 처음에는 ObjectMapperstatic 으로 두려고했었는데요 하지만 ObjectMapper 는 멀티 쓰레드 환경에서 Thread-Safe 하지 않다고 합니다. 이는 setConfig(), setDateFormat() 등의 객체 설정을 바꾸는 메서드가 존재해서입니다!

저문이 주신 질문에 답변하기 위해서 더 알아봤는데
ObjectMapper 를 통해서 ObjectWriter, ObjectReader 만 따로 뽑을 수 있더라구요!
두 객체는 Thread-Safe 하다고 합니다! 따라서 두 객체로 분리해서 static 으로 선언해두었습니다!

ObjectMapperTread-Safe 하지 않다는 것을 어렴풋이만 알고 있었는데
저문 덕분에 조금 더 자세히 알아가네요! 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 ObjectMapper가 Thread-Safe하지 않다는 사실은 모르고 있었는데,
블랙캣은 사소한 질문에도 효과적인 학습을 뽑아내시는군요 ㅎㅎ
덕분에 저도 배워갑니다!

Comment on lines +28 to 30
public void render(final HttpServletRequest request, final HttpServletResponse response) throws Exception {
view.render(Collections.unmodifiableMap(model), request, response);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ModelAndView에서 직접 render를 하도록 변경해주신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getter를 사용할 경우 ModelAndView 클래스는 Model 과 View 를 각각 들고 있고,
Model은 View 에게 필요한 데이터이기 때문에 Model 을 뽑고 View 를 뽑아서 다시 View에 넣어줘야합니다.

final Map<String, Object> model = modelAndView.getModel();
final View view = modelAndView.getView();
view.render(model, request, response);

저는 굳이라는 생각이 들더라구요.
결국 내부에 둘 다 들고있는 ModelAndView 에서 직접 render 를 하는 것이 깔끔하다고 생각했습니다!
또한 DispatcherServlet 에서 핸들러 단에서 반환된 Model 값을 변경할 가능성도 사라지고요!
(물론 추가는 할 수 있지만 최소한 핸들러에서 추가한 값에 대한 것은 감출 수 있음으로)

하지만 저문의 질문을 듣고나니 핸들러에서 DispatcherServlet 으로 결과가 반환되기 전에
핸들러 단에서 직접 응답을 내려줄 수 있는 문제도 있겠네요! 이 부분에 대해서는 저문은 어떻게 생각하시나요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 질문을 드렸던 근본적인 이유는 ModelAndView에서 render를 해주는 책임 자체가 적절한가?에 대한 의문이었어요.
View 그 자체에서 render를 하는 것이 더 자연스럽지 않나?라고 생각해서, view를 뽑아서 사용하더라도 view에서 직접 render를 하는 것이 좋겠다고 생각했어요.
ModelAndView는 모델과 뷰를 담고 있는데, ModelAndView.render()를 하게되면 Model과 View 어떤 것에서 렌더링을 하는지 명확하지 않다고 느꼈어요. ModelAndView라는 클래스 자체가 두개의 역할을 할 수 있어서 그런 고민이 든 것 같기도 하네요 ㅎㅎ

블랙캣이 가지고 있는 근거가 있기 때문에 어떤 방식으로 구현하든 문제가 없다고 생각해요.
블랙캣의 생각을 들을 수 있어서 좋은 시간이었습니다 :)

Comment on lines +16 to +19
public ModelAndView handle(final Object[] arguments) throws Exception {
return (ModelAndView) method.invoke(controllerInstance, arguments);
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다양한 파라미터들을 지원하게 되면서 Object배열로 파라미터를 변경해주신 걸로 이해하면 될까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 맞습니다!

@jeomxon
Copy link

jeomxon commented Sep 25, 2023

저번에 질문드렸던 DispatcherServletInitializer는 왜 app모듈에 두셨는지 의견이 궁금해요.에 대한 답을 DM으로 주셨더라구요..ㅎㅎ
단순 궁금증에 의해서 질문드렸던 것인데 깊게 생각해주신 것 같아서 감사했습니다.

해당 부분에 대해서는 추가 공지에 있던 것처럼 꼭 옮겨주시지는 않아도 될 것 같아요.
이에대한 고민을 해보신 것만으로도 충분하다고 생각이 듭니다! (저도 물론 옮기지 못했어요..^^)

또한 ArgumentResolver를 통해서 컨트롤러에서 불필요한 파라미터를 제거해주신 부분이 인상깊었습니다.
저도 미션을 진행하면서 불필요한 파라미터들을 사용해야한다는 점이 개발을 할 때 혼란을 줄 수 있다고 많이 느끼고 있어요.
그럼 점들 고려해주셔서 추가적인 구현까지 해주셔서 리뷰어 입장에서도 많이 배울 수 있었습니다.
이 부분은 제가 직접 구현해본 부분은 아니라서 세세하게 코멘트를 남기지는 못했지만,
블랙캣이 어떤 방식으로 설계하시고 구현했는지 정도 이해할 수 있었습니다.

마지막 요청 기다리겠습니다!

@jeomxon jeomxon merged commit c8974e1 into woowacourse:songusika Sep 26, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants