-
Notifications
You must be signed in to change notification settings - Fork 381
[MVC 구현하기 - 3단계] 배키(백은희) 미션 제출합니다. #799
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
eunjungL
left a comment
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단계 구현 정말 잘해주셨네요!
요구사항도 잘 지켜주셨고 ViewResolver에 대한 학습까지 잘 하신 것 같아요.
특별히 잘못된 곳은 없어보여 사소한 리뷰 위주로 달게 됐어요. 몇 가지 의견을 남겨뒀으니 확인 부탁드려요 😄
남은 미션 기간동안도 파이팅입니다! 🙇
| @Override | ||
| public void onStartup(final ServletContext servletContext) { | ||
| final var dispatcherServlet = new DispatcherServlet(); | ||
| final var dispatcherServlet = new DispatcherServlet(getClass().getPackage().getName()); |
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.
패키지명 동적으로 주기를 유지하셨군요! 👍 👍
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.
클로버의 이전 피드백을 적극 반영해봤습니다 😎
| final String account = request.getParameter("account"); | ||
| log.debug("user id : {}", account); | ||
|
|
||
| final ModelAndView modelAndView = new ModelAndView("json:"); |
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("jsonView")라고 쓰는 것 같더라구요, 여기서 json:으로 하신 이유는 무엇인가요?
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.
사실 이유는 딱히 없습니다😅 단순히, json 뷰임을 확인하기 위해 prefix로 뒀어요. 그 과정에서 기존 코드에 있었던, redirect: 를 참고했어요!
| ModelAndView modelAndView = adapter.handle(handler, request, response); | ||
|
|
||
| View view = new JspView(modelAndView.getViewName()); | ||
| View view = viewResolvers.resolveViewName(modelAndView.getViewName()); |
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.
ViewResolver 사용하는거 아주 좋네요 👍
| throw new NullPointerException("viewName의 값이 null입니다."); | ||
| } | ||
|
|
||
| if (viewName.startsWith(PREFIX)) { |
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.
의견)
viewName이 json:index.jsp처럼 잘못 들어오는 경우에는 jsonView인가요 jspVeiw인가요?
JsonView에서는 식별해야 할 파일이 없으니까 viewName이 특별한 의미를 갖지는 않을 것 같아요.
잘못된 값이 들어오는 걸 방지하기 위해 equals로 바꿔보는거나 예외 처리를 시도해보면 어떨까요?
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.
viewName이 json:index.jsp처럼 잘못 들어오는 경우에는 jsonView인가요 jspVeiw인가요?
앗 그렇네요! 그 부분은 미처 생각하지 못했어요😂
일단 json:index.jsp로 들어가면, DispatcherServlet으로 들어가지 않는 것 같아요.
.jsp 확장자가 붙으면 DispatcherServlet 의 service() 함수에 들어가기 전, 톰캣의 ResourceFilter에서 필터링 됩니다. 만약 .jsp 파일이 존재하지 않는다면 404페이지를 반환하고 존재하면 해당 파일을 반환합니다.
viewResolver를 통해 view를 반환하는 경우는 /hello.jsp, /index.html처럼 확장자가 붙은 url이 아니라, /hello, /index인 경우입니다. 미션 하면서 헷갈렸는데, 클로버 덕분에 찾아보네요👍 감사합니다 :)
만약 /json:index 로 호출한다면, jsonView로 판단합니다!
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.
JsonView에서는 식별해야 할 파일이 없으니까 viewName이 특별한 의미를 갖지는 않을 것 같아요.
잘못된 값이 들어오는 걸 방지하기 위해 equals로 바꿔보는거나 예외 처리를 시도해보면 어떨까요?
동의합니다~ viewName이 의미 없는데 굳이 startWith 함수를 사용할 필요가 없네요. 수정할게요 🚀
| public class JspViewResolver implements ViewResolver { | ||
|
|
||
| @Override | ||
| public View resolveViewName(String viewName) { |
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.
여기는 아무런 검사 없이 들어오면 바로 JspView로 변환해주는 것 같은데 맞을까요?
Jsp와 관련되지 않은 잘못된 ViewName이 들어왔을 때의 처리가 함께 있으면 좋을 것 같아요!
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.
네 그렇네요...! view 이름이 .jsp 로 끝나면 JspView를 반환하도록 수정했습니다~ 😊
| return (ModelAndView) method.invoke(controller, parameters); | ||
| } | ||
|
|
||
| private Object[] initParameters(final HttpServletRequest request, final HttpServletResponse 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.
ArgumentResolver를 이런 식으로 활용하셨군요~ 👍
PR 본문에는 리뷰하지 않아도 된다고 써주셨지만... 😄 이왕 구현해주신 거 배키가 어떤 궁금증에서 시작되어 ArgumentResolver를 구현해보게 된건지, 구현하면서 느꼈던 장점은 뭐가 있는지 궁금해서 질문 슬쩍 남깁니다 ^_^
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를 구현해보게 된건지
이전에 구현은 @RequestMapping을 통해서 실행할 메서드를 찾아낸 뒤, 리플렉션을 통해 해당 메서드를 실행시킵니다. 때문에 Controller 내부의 모든 실행 메서드는 Request와 Response를 모두 매개변수로 필수로 받아야 합니다. 해당 매개변수를 사용하지 않더라도요.
개발자 입장에서는 이 부분이 불편하다고 생각했습니다. 그래서 사용하지 않는 매개변수는 받지 않도록 하기 위해 ArgumentResolver를 구현했습니다~
구현하면서 느꼈던 장점은 뭐가 있는지
위에서 말했던 단점을 해결할 수 있게 되었어요. 사용하지 않는 매개변수는 쓸 필요가 없어졌어요 👍
| @@ -1,13 +1,14 @@ | |||
| package com.techcourse; | |||
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.
질문)
DisptacherServlet은 MVC 패키지로 이동했는데 DisptacherServletInitalizer는 왜 APP 패키지에 남게 되었을까요?
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.
SpringServletContainerInitializer.onStartup() 메서드에서 WebApplicationInitializer를 구현한 객체(DispatcherServletInitializer)가 생성됩니다. 생성된 DispatcherServletInitializer은 DispatcherServlet을 생성하는데, 이때 DispatcherServlet을 생성할 때 필요한 basePackage를 넣어줍니다.
public class DispatcherServletInitializer implements WebApplicationInitializer {
private static final Logger log = LoggerFactory.getLogger(DispatcherServletInitializer.class);
private static final String DEFAULT_SERVLET_NAME = "dispatcher";
@Override
public void onStartup(final ServletContext servletContext) {
final var dispatcherServlet = new DispatcherServet(getClass().getPackage().getName());
...
}
}현재 DispatcherServletInitializer은 app 모듈에 존재하기 때문에 getClass().getPackage().getName()를 호출해서 넣으면 컨트롤러가 있는 패키지 위치를 넣어주기 때문에 구현한 컨트롤러를 모두 스캔할 수 있습니다. 그러나 DispatcherServletInitializer를 mvc 모듈로 옮기면 getClass().getPackage().getName()를 호출할 때
컨트롤러가 있는 패키지 위치가 아닌 mvc 모듈의 패키지를 넣어주게 되어서 controller를 호출할 수 없습니다.
물론 패키지 위치를 명시하는 방법도 있지만, 프레임워크가 app을 알고 있는 것은 이상하다고 판단해서 app 패키지에 남겨 두었습니다~
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.
안녕하세요~ 클로버🍀
항상 생각할 부분을 던져주셔서 감사합니다 :)
덕분에 찾아보면서 새로운 것을 많이 알게 되는 것 같아요 👍
피드백 적극 반영하고 코멘트도 남겼어요.
확인 부탁드리고 혹시 부족한 점 있으면 언제든지 말씀해 주세요~~~
(추가로, 학습 테스트 업데이트 했어요..🫠)
| @Override | ||
| public void onStartup(final ServletContext servletContext) { | ||
| final var dispatcherServlet = new DispatcherServlet(); | ||
| final var dispatcherServlet = new DispatcherServlet(getClass().getPackage().getName()); |
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.
클로버의 이전 피드백을 적극 반영해봤습니다 😎
| final String account = request.getParameter("account"); | ||
| log.debug("user id : {}", account); | ||
|
|
||
| final ModelAndView modelAndView = new ModelAndView("json:"); |
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.
사실 이유는 딱히 없습니다😅 단순히, json 뷰임을 확인하기 위해 prefix로 뒀어요. 그 과정에서 기존 코드에 있었던, redirect: 를 참고했어요!
| throw new NullPointerException("viewName의 값이 null입니다."); | ||
| } | ||
|
|
||
| if (viewName.startsWith(PREFIX)) { |
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.
viewName이 json:index.jsp처럼 잘못 들어오는 경우에는 jsonView인가요 jspVeiw인가요?
앗 그렇네요! 그 부분은 미처 생각하지 못했어요😂
일단 json:index.jsp로 들어가면, DispatcherServlet으로 들어가지 않는 것 같아요.
.jsp 확장자가 붙으면 DispatcherServlet 의 service() 함수에 들어가기 전, 톰캣의 ResourceFilter에서 필터링 됩니다. 만약 .jsp 파일이 존재하지 않는다면 404페이지를 반환하고 존재하면 해당 파일을 반환합니다.
viewResolver를 통해 view를 반환하는 경우는 /hello.jsp, /index.html처럼 확장자가 붙은 url이 아니라, /hello, /index인 경우입니다. 미션 하면서 헷갈렸는데, 클로버 덕분에 찾아보네요👍 감사합니다 :)
만약 /json:index 로 호출한다면, jsonView로 판단합니다!
| throw new NullPointerException("viewName의 값이 null입니다."); | ||
| } | ||
|
|
||
| if (viewName.startsWith(PREFIX)) { |
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.
JsonView에서는 식별해야 할 파일이 없으니까 viewName이 특별한 의미를 갖지는 않을 것 같아요.
잘못된 값이 들어오는 걸 방지하기 위해 equals로 바꿔보는거나 예외 처리를 시도해보면 어떨까요?
동의합니다~ viewName이 의미 없는데 굳이 startWith 함수를 사용할 필요가 없네요. 수정할게요 🚀
| public class JspViewResolver implements ViewResolver { | ||
|
|
||
| @Override | ||
| public View resolveViewName(String viewName) { |
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.
네 그렇네요...! view 이름이 .jsp 로 끝나면 JspView를 반환하도록 수정했습니다~ 😊
| return (ModelAndView) method.invoke(controller, parameters); | ||
| } | ||
|
|
||
| private Object[] initParameters(final HttpServletRequest request, final HttpServletResponse 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.
어떤 궁금증에서 시작되어 ArgumentResolver를 구현해보게 된건지
이전에 구현은 @RequestMapping을 통해서 실행할 메서드를 찾아낸 뒤, 리플렉션을 통해 해당 메서드를 실행시킵니다. 때문에 Controller 내부의 모든 실행 메서드는 Request와 Response를 모두 매개변수로 필수로 받아야 합니다. 해당 매개변수를 사용하지 않더라도요.
개발자 입장에서는 이 부분이 불편하다고 생각했습니다. 그래서 사용하지 않는 매개변수는 받지 않도록 하기 위해 ArgumentResolver를 구현했습니다~
구현하면서 느꼈던 장점은 뭐가 있는지
위에서 말했던 단점을 해결할 수 있게 되었어요. 사용하지 않는 매개변수는 쓸 필요가 없어졌어요 👍
| @@ -1,13 +1,14 @@ | |||
| package com.techcourse; | |||
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.
SpringServletContainerInitializer.onStartup() 메서드에서 WebApplicationInitializer를 구현한 객체(DispatcherServletInitializer)가 생성됩니다. 생성된 DispatcherServletInitializer은 DispatcherServlet을 생성하는데, 이때 DispatcherServlet을 생성할 때 필요한 basePackage를 넣어줍니다.
public class DispatcherServletInitializer implements WebApplicationInitializer {
private static final Logger log = LoggerFactory.getLogger(DispatcherServletInitializer.class);
private static final String DEFAULT_SERVLET_NAME = "dispatcher";
@Override
public void onStartup(final ServletContext servletContext) {
final var dispatcherServlet = new DispatcherServet(getClass().getPackage().getName());
...
}
}현재 DispatcherServletInitializer은 app 모듈에 존재하기 때문에 getClass().getPackage().getName()를 호출해서 넣으면 컨트롤러가 있는 패키지 위치를 넣어주기 때문에 구현한 컨트롤러를 모두 스캔할 수 있습니다. 그러나 DispatcherServletInitializer를 mvc 모듈로 옮기면 getClass().getPackage().getName()를 호출할 때
컨트롤러가 있는 패키지 위치가 아닌 mvc 모듈의 패키지를 넣어주게 되어서 controller를 호출할 수 없습니다.
물론 패키지 위치를 명시하는 방법도 있지만, 프레임워크가 app을 알고 있는 것은 이상하다고 판단해서 app 패키지에 남겨 두었습니다~
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 Object findBean(Constructor constructor, Class<?>[] parameterTypes) throws Exception { | ||
| if (countMatchedParameter(parameterTypes) == parameterTypes.length) { |
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.
질문)
모든 빈을 생성한 후 그 다음 필드를 주입하는 형식이 아니라, 생성할 때부터 필드를 주입해서 생성하는 방식으로 만드는 것처럼 보여요.
그러기 위해서 모든 생성자를 가져온 후 파라미터 개수를 검사하는 것 같네요! 맞나요?
기본 생성자를 통해 필요한 Bean 모두 생성 -> 각 Bean에 필요한 필드 주입 대신 위의 방법을 선택한 이유가 무엇인가요?
| public DIContainer(final Set<Class<?>> classes) throws Exception { | ||
| this.beans = new LinkedHashSet<>(); | ||
| initBean(classes); | ||
| fieldInjection(); |
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.
질문)
여기서도 이미 필요한 의존성을 추가해서 인스턴스를 만든걸로 보이는데 fieldInjection()이 다시 불리네요!
저는 학습 테스트를 진행할 때 Stage3과 달라지는 부분이 ClassPathScanner를 활용하는 것뿐이라고 느꼈어요.
그래서 배키는 왜 아예 다른 구현법을 선택하게 됐는지 이유가 궁금해요!
안녕하세요~ 🍀클로버🍀
3단계 미션으로 돌아왔습니다~!!
잘 부탁드려요 :)
이번 미션에서 추가로
ViewResolver를 구현해 보려고 했지만, 많이 미흡합니다...controller에서json:프리픽스로 가진viewName을 반환하면JsonView처리를 해주고 그 외는JspView로 처리하도록 구현됐어요.. (사실JspView에서viewName에 해당하는jsp파일이 없으면 처리를 하지 않고null을 반환하도록 만들고 싶었으나, 구현에 실패했습니다🥲)그리고
HandlerMethodArgumentResolver를 구현해서 컨트롤러에서 필요 없는 매개변수는 굳이 받도록 설정하지 않게 구현했습니다..!개인적인 궁금증으로 찾아본 거라, 신경 쓰지 않으셔도 괜찮습니다😅