-
Notifications
You must be signed in to change notification settings - Fork 305
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
Conversation
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.
안녕하세요 블랙캣 :)
3단계도 정말 잘 구현해주셨네요!
저도 어느정도 고민했던 부분인데, 사용되지 않는 HttpServletRequest, HttpServletResponse없이도 Controller가 동작할 수 있도록 변경해주신 부분이 인상깊었습니다.
사소한 코멘트들 남겨두었는데 답변 기다리겠습니다.
추가적으로 의견 나누고 싶으시다면 언제든지 코멘트나 DM주세요😀
@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")); | ||
} |
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.
LoginController는 view에 대한 클래스가 분리되어있는 것으로 확인이 됩니다.
사소하지만 둘 중 하나로 통일성있게 변경해보는 것은 어떨까요?
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.
미쳐 확인하지 못했네요!
회원가입 컨트롤러와 같이 하나의 컨트롤러로 통합하였습니다!
private String password; | ||
private String email; | ||
|
||
public RegisterRequest() { |
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.
dto패키지 내부에 있는 클래스들에 대해서는 public생성자를 명시해주셨네요.
이유가 궁금합니다!
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.
ObjectMapper
는 json 을 역직렬화 할 때 기본 생성자가 없다면 InvalidDefinitionException
예외가 터지게됩니다.
이는 Reflection API 가 클래스를 인스턴스화 할 때 기본생성자를 사용하기 때문입니다.
(@Entity
가 붙은 도메인 객체에서 기본 생성자를 넣어주는 이유와 같습니다.)
참고로 스프링에서는 기본적으로 추가적으로 jackson-module-parameter-names
모듈을 통해서 기본 생성자가 없는 경우 다른 방법(매개변수가 있는 생성자 등) 을 찾아서 역직렬화를 해준다고합니다!
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(); | ||
} | ||
} |
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.
ReflectionUtils는 static하게 사용하는 클래스로 알고 있었는데, 이 메서드는 어떤 역할을 하고 있나요?
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.
@RequestBody
어노테이션을 만들면서 필요하다고 생각한 메서드였는데
각 Resolver
로 분리하면서 해당 메서드가 필요가 없어졌네요!
원래는 인자로 넘겨준 어노테이션이 선언된 메서드의 파라미터의 타입을 반환하는 메서드입니다!
필요 없어져서 제거하겠습니다!
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); | ||
} | ||
} |
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.
메서드를 매번 호출할 때마다 ObjectMapper가 생성되고 있는 것이 아닌가요?
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.
저도 처음에는 ObjectMapper
를 static
으로 두려고했었는데요 하지만 ObjectMapper
는 멀티 쓰레드 환경에서 Thread-Safe 하지 않다고 합니다. 이는 setConfig()
, setDateFormat()
등의 객체 설정을 바꾸는 메서드가 존재해서입니다!
저문이 주신 질문에 답변하기 위해서 더 알아봤는데
ObjectMapper
를 통해서 ObjectWriter
, ObjectReader
만 따로 뽑을 수 있더라구요!
두 객체는 Thread-Safe 하다고 합니다! 따라서 두 객체로 분리해서 static
으로 선언해두었습니다!
ObjectMapper
가 Tread-Safe 하지 않다는 것을 어렴풋이만 알고 있었는데
저문 덕분에 조금 더 자세히 알아가네요! 👍
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.
저도 ObjectMapper가 Thread-Safe하지 않다는 사실은 모르고 있었는데,
블랙캣은 사소한 질문에도 효과적인 학습을 뽑아내시는군요 ㅎㅎ
덕분에 저도 배워갑니다!
public void render(final HttpServletRequest request, final HttpServletResponse response) throws Exception { | ||
view.render(Collections.unmodifiableMap(model), request, response); | ||
} |
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.
ModelAndView에서 직접 render를 하도록 변경해주신 이유가 궁금해요!
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.
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
으로 결과가 반환되기 전에
핸들러 단에서 직접 응답을 내려줄 수 있는 문제도 있겠네요! 이 부분에 대해서는 저문은 어떻게 생각하시나요?
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.
제가 질문을 드렸던 근본적인 이유는 ModelAndView에서 render를 해주는 책임 자체가 적절한가?에 대한 의문이었어요.
View 그 자체에서 render를 하는 것이 더 자연스럽지 않나?라고 생각해서, view를 뽑아서 사용하더라도 view에서 직접 render를 하는 것이 좋겠다고 생각했어요.
ModelAndView는 모델과 뷰를 담고 있는데, ModelAndView.render()
를 하게되면 Model과 View 어떤 것에서 렌더링을 하는지 명확하지 않다고 느꼈어요. ModelAndView라는 클래스 자체가 두개의 역할을 할 수 있어서 그런 고민이 든 것 같기도 하네요 ㅎㅎ
블랙캣이 가지고 있는 근거가 있기 때문에 어떤 방식으로 구현하든 문제가 없다고 생각해요.
블랙캣의 생각을 들을 수 있어서 좋은 시간이었습니다 :)
public ModelAndView handle(final Object[] arguments) throws Exception { | ||
return (ModelAndView) method.invoke(controllerInstance, arguments); | ||
} | ||
|
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.
다양한 파라미터들을 지원하게 되면서 Object배열로 파라미터를 변경해주신 걸로 이해하면 될까요?
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.
넵 맞습니다!
저번에 질문드렸던 해당 부분에 대해서는 추가 공지에 있던 것처럼 꼭 옮겨주시지는 않아도 될 것 같아요. 또한 ArgumentResolver를 통해서 컨트롤러에서 불필요한 파라미터를 제거해주신 부분이 인상깊었습니다. 마지막 요청 기다리겠습니다! |
안녕하세요 저문!
미션을 잘못 이해해서 제출이 늦어졌네요 😅
이번 미션을 진행하면서 저번 step2에서 마지막에 남겨주셨던 질문의 답변 남겨놓을게요!
당시 step2 미션에는 점진적인 리팩터링이 목적이었어요!
지금은 없어졌지만
ManualHandlerMapping
이 app 모듈에 있고DispatcherInitializer
를 mvc 모듈로 옮기게 된다면 프레임워크가 app 을 의존하는 형태이기 때문에 당시에는 app 모듈 내부에 두었습니다!현재는 프레임워크 단에서 서블릿 컨텍스트를 초기화할 수 있도록
DispatcherInitializer
또한 mvc 모듈로 이동하고 싶지만AnnotationHandlerMapping
에서 basePackage 위치를 app 단의 controller 패키지에 잡아주어야 하는데 아직해결하지 못했습니다.이부분에 대해선 다음 리뷰 요청 때 반영할게요!
구현 사항 🚀
HttpServletRequest/Response
를 항상 사용하지 않더라도 꼭 파라미터로 선언하는게 불편해서 구현해 보았습니다!@RequestBody
,@RequestParam
,HttpServletRequest/Response
,HttpSession
입니다.ArgumentResolvers
객체 내부에서 파라미터의 순서에 맞게 변환한 뒤 Object 배열로 반환합니다.AnnotationHandlerAdapter
에서 실제 메서드를 호출하는 시점에 인자로 넘겨주고 있어요!@RequestBody
나,ArgumentResolver
부분은 미션 요구사항이 아니여서 저문이 편하신대로 리뷰해주셔도 좋습니다!@MVC 마지막 미션 리뷰 감사합니다!! 🙇♂️