-
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 구현하기 - 2단계] 페드로(류형욱) 미션 제출합니다. #726
Changes from all commits
5370cdf
e8cec5f
7ab73a9
22504fc
4dfbbe1
45dfa16
01edf41
9454879
d6eb8a8
c1e936a
2a5c2ee
f501d94
c5b430f
a5d79bb
0309308
849d79f
753154a
8e6e237
3b52a1b
10d5980
1d66e3f
5fd0595
894da3c
13e946d
8f72eaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,33 @@ | ||
package com.techcourse.controller; | ||
|
||
import com.interface21.context.stereotype.Controller; | ||
import com.interface21.web.bind.annotation.RequestMapping; | ||
import com.interface21.web.bind.annotation.RequestMethod; | ||
import com.interface21.webmvc.servlet.ModelAndView; | ||
import com.interface21.webmvc.servlet.view.JspView; | ||
import com.techcourse.domain.User; | ||
import com.techcourse.repository.InMemoryUserRepository; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import com.interface21.webmvc.servlet.mvc.asis.Controller; | ||
|
||
public class RegisterController implements Controller { | ||
@Controller | ||
public class RegisterController { | ||
|
||
@Override | ||
public String execute(final HttpServletRequest req, final HttpServletResponse res) throws Exception { | ||
@RequestMapping(value = "/register", method = RequestMethod.GET) | ||
public ModelAndView view(HttpServletRequest req, HttpServletResponse res) { | ||
JspView jspView = new JspView("/register.jsp"); | ||
return new ModelAndView(jspView); | ||
} | ||
|
||
@RequestMapping(value = "/register", method = RequestMethod.POST) | ||
public ModelAndView save(HttpServletRequest req, HttpServletResponse res) { | ||
final var user = new User(2, | ||
req.getParameter("account"), | ||
req.getParameter("password"), | ||
req.getParameter("email")); | ||
InMemoryUserRepository.save(user); | ||
|
||
return "redirect:/index.jsp"; | ||
JspView jspView = new JspView("redirect:/index.jsp"); | ||
return new ModelAndView(jspView); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
package com.techcourse; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import com.interface21.webmvc.servlet.mvc.HandlerMapping; | ||
import com.techcourse.controller.LoginViewController; | ||
import jakarta.servlet.RequestDispatcher; | ||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import jakarta.servlet.http.HttpSession; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
import org.mockito.ArgumentCaptor; | ||
|
||
class DispatcherServletTest { | ||
|
||
private DispatcherServlet dispatcherServlet; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 질문) 모킹하여 테스트하기에는 테스트만을 위한 게터를 열지 않으면서, 직접 HTTP 요청을 보내 보지 않고 깔끔하게 테스트할 수 있는 방법이 있을까요? @DisplayName("어노테이션 기반 컨트롤러를 찾아서 처리한다.")
@Test
void processAnnotationBasedController() {
// given
HttpServletRequest request = mock(HttpServletRequest.class);
HttpServletResponse response = mock(HttpServletResponse.class);
when(request.getMethod()).thenReturn("GET");
when(request.getRequestURI()).thenReturn("/register");
when(request.getRequestDispatcher("/register.jsp")).thenReturn(mock(RequestDispatcher.class));
// when & then
assertThatCode(() -> dispatcherServlet.service(request, response))
.doesNotThrowAnyException(); // 맘에 안 듦
}
@DisplayName("Controller 인터페이스 기반 컨트롤러를 찾아서 처리한다.")
@Disabled("안돌아감ㅠㅠ")
@Test
void processInterfaceBasedController() {
// given
HttpServletRequest request = mock(HttpServletRequest.class);
HttpServletResponse response = mock(HttpServletResponse.class);
when(request.getMethod()).thenReturn("GET");
when(request.getRequestURI()).thenReturn("/login");
when(request.getRequestDispatcher("/login.jsp")).thenReturn(mock(RequestDispatcher.class));
// when & then
assertThatCode(() -> dispatcherServlet.service(request, response))
.doesNotThrowAnyException();
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 와 전 기수 분 코드까지 찾아봐 주시다니 감동인걸요 😄 참고하여 반영해 두었습니다 갑사합니다🔥 |
||
private HttpServletRequest request; | ||
private HttpServletResponse response; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
dispatcherServlet = new DispatcherServlet(); | ||
dispatcherServlet.init(); | ||
request = mock(HttpServletRequest.class); | ||
response = mock(HttpServletResponse.class); | ||
} | ||
|
||
@DisplayName("어노테이션 기반 컨트롤러를 찾아서 처리한다.") | ||
@Test | ||
void processAnnotationBasedController() throws ServletException { | ||
// given | ||
when(request.getMethod()).thenReturn("GET"); | ||
when(request.getRequestURI()).thenReturn("/register"); | ||
|
||
ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class); | ||
RequestDispatcher requestDispatcher = mock(RequestDispatcher.class); | ||
when(request.getRequestDispatcher(argumentCaptor.capture())) | ||
.thenReturn(requestDispatcher); | ||
|
||
// when | ||
dispatcherServlet.service(request, response); | ||
|
||
// then | ||
assertThat(argumentCaptor.getValue()) | ||
.isEqualTo("/register.jsp"); | ||
} | ||
|
||
@DisplayName("Controller 인터페이스 기반 컨트롤러를 찾아서 처리한다.") | ||
@Test | ||
void processInterfaceBasedController() throws ServletException { | ||
// given | ||
when(request.getMethod()).thenReturn("GET"); | ||
when(request.getRequestURI()).thenReturn("/login/view"); | ||
when(request.getSession()).thenReturn(mock(HttpSession.class)); | ||
registerFakeHandlerMapping(new LoginViewController()); | ||
|
||
ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class); | ||
RequestDispatcher requestDispatcher = mock(RequestDispatcher.class); | ||
when(request.getRequestDispatcher(argumentCaptor.capture())) | ||
.thenReturn(requestDispatcher); | ||
|
||
// when | ||
dispatcherServlet.service(request, response); | ||
|
||
// then | ||
assertThat(argumentCaptor.getValue()) | ||
.isEqualTo("/login.jsp"); | ||
} | ||
|
||
@DisplayName("요청에 해당하는 핸들러를 찾을 수 없으면 예외가 발생한다.") | ||
@Test | ||
void throwsWhenHandlerNotFound() { | ||
// given | ||
when(request.getMethod()).thenReturn("GET"); | ||
when(request.getRequestURI()).thenReturn("/not-found"); | ||
|
||
// when & then | ||
assertThatThrownBy(() -> dispatcherServlet.service(request, response)) | ||
.isInstanceOf(ServletException.class) | ||
.hasMessageContaining("요청에 해당하는 핸들러를 찾을 수 없습니다."); | ||
} | ||
|
||
@DisplayName("요청에 해당하는 핸들러 어댑터를 찾을 수 없으면 예외가 발생한다.") | ||
@Test | ||
void throwsWhenHandlerAdapterNotFound() { | ||
// given | ||
registerFakeHandlerMapping("test"); | ||
when(request.getMethod()).thenReturn("GET"); | ||
when(request.getRequestURI()).thenReturn("/test"); | ||
|
||
// when & then | ||
assertThatThrownBy(() -> dispatcherServlet.service(request, response)) | ||
.isInstanceOf(ServletException.class) | ||
.hasMessageContaining("요청에 해당하는 핸들러 어댑터를 찾을 수 없습니다."); | ||
} | ||
|
||
private void registerFakeHandlerMapping(Object retVal) { | ||
dispatcherServlet.addHandlerMapping(new HandlerMapping() { | ||
@Override | ||
public void initialize() { | ||
} | ||
|
||
@Override | ||
public Object getHandler(HttpServletRequest request) { | ||
return retVal; | ||
} | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package com.interface21.webmvc.servlet.mvc; | ||
|
||
import com.interface21.webmvc.servlet.ModelAndView; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
|
||
public interface HandlerAdapter { | ||
|
||
ModelAndView handle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception; | ||
|
||
boolean canHandle(Object handler); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package com.interface21.webmvc.servlet.mvc; | ||
|
||
import com.interface21.webmvc.servlet.mvc.tobe.ControllerHandlerAdapter; | ||
import com.interface21.webmvc.servlet.mvc.tobe.HandlerExecutionHandlerAdapter; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
public class HandlerAdapterRegistry { | ||
|
||
private final List<HandlerAdapter> handlerAdapters; | ||
|
||
public HandlerAdapterRegistry() { | ||
this.handlerAdapters = new ArrayList<>(); | ||
} | ||
|
||
public void initialize() { | ||
handlerAdapters.add(new ControllerHandlerAdapter()); | ||
handlerAdapters.add(new HandlerExecutionHandlerAdapter()); | ||
} | ||
|
||
public void addHandlerAdapter(HandlerAdapter adapter) { | ||
handlerAdapters.add(adapter); | ||
} | ||
|
||
public Optional<HandlerAdapter> getHandlerAdapter(Object handler) { | ||
return handlerAdapters.stream() | ||
.filter(adapter -> adapter.canHandle(handler)) | ||
.findFirst(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.interface21.webmvc.servlet.mvc; | ||
|
||
import jakarta.servlet.http.HttpServletRequest; | ||
|
||
public interface HandlerMapping { | ||
|
||
void initialize(); | ||
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. #726 (comment) 이쪽 코멘트 참고해주세요! |
||
|
||
Object getHandler(HttpServletRequest request); | ||
} |
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.
HandlerMappingRegistry과 HandlerAdapterRegistry 모두 외부(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.
현재
DispatcherServltet
은DispatcherServletInitializer
내의onStartup()
호출 직후 바로빈 인스턴스가 만들어지고, 사용자 지정
HandlerMapping
이 등록된 이후 내부적으로DispatcherServlet.init()
이 호출되어 지연 초기화되는 구조로 작성되어 있어요.프레임워크에서 자동으로 등록해 준 매핑보다는 사용자가 명시적으로 추가해 준(
dispatcherServlet.addHandlerMapping(new ManualHandlerMapping())
) 핸들러들이 먼저 검색되도록 하기 위해HandlerMapping
들도 마찬가지로 초기화가 지연될 수 있도록 구현해 두었습니다.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.
의도가 너무 좋네요 👍
저도 커스텀이 우선시 되어야 한다고 생각해요.
그런데 제가 요구사항에 의한 구조적인 문제 때문에 페드로가 어디까지를 프레임워크 역할로 또는 사용자 역할로 보고 있는지가 조금 헷갈리는 것 같아요. 어색하게 느껴지는 부분이 몇 가지 있어서 질문 드릴게요.
먼저
new ManualHandlerMapping
은DispatcherServlet
이 아닌DispatcherServletInitializer
에서 등록을 해주고 있는데반면,
new AnnotationHandlerMapping(ANNOTATION_BASED_CONTROLLER_BASE)
에서com.techcourse.controller
라는 특정 애플리케이션에 종속된 패키지를HandlerMappingRegistry
에서 선언해주고 있는 것 같아요. 이 패키지 or 핸들러 매핑은DispatcherServletInitializer
에서 주입 하지 않은 이유가 궁금해요!그리고 여전히 각
HandlerMapping
의 초기화가 외부에서 이루어져야만 하는지는 이해가 잘 가지 않아요.HandlerMappingRegistry
는 페드로 말씀처럼 사용자 추가 핸들러 매핑의 순서 조정을 위해 초기화를 지연시키는 것이 좋을 것 같아요. 그런데HandlerMappingRegistry
에서 각 핸들러 매핑의 순서가 지정될테니 각HandlerMapping
은 내부에서는 즉시 초기화가 되어도 괜찮지 않나요?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.
PR 본문에서 잠깐 언급했던 것 처럼,
AnnotationHandlerMapping
은 프레임워크 자체적으로 제공하는 '기능'으로 생각하고 구현했어요.@Controller
어노테이션만 붙여 놓으면 프레임워크에서 알아서 스캔해서 등록을 해 주는 기능이니, 사용자가 관여할 필요가 없다고 생각해서DispatcherServletInitializer
에서 따로 등록하지 않았습니다.물론 말씀주신 것 처럼 지금은 애플리케이션 패키지 경로를 상수형태로 의존하고 있어서 책임분리가 완전히 되어 있다고 보기는 힘든데요,
PR 본문에서 이렇게 말씀드렸던 이유이기도 해요!
다음 단계를 살짝 살펴 보니 asis, tobe 패키지가 드디어(!) 제거되는 것 같아서 완전한 분리를 3단계에서 수행하기 위해 현재의 구현으로 요청 드렸어요.
오.. 예리하시군요 +_+ 그렇게도 생각할 수 있겠어요.
저는
DispatcherServlet -> HandlerMappingRegistry
까지 전부 지연 초기화 전략을 택하고 있으니, 그 내부에서 관리되는 객체인HandlerMapping
역시 지연 초기화를 지원하는 것이 자연스럽다고 생각했어요. 현재HandlerMapping
을 사용하는 곳이HandlerMappingRegistry
뿐인데,HandlerMappingRegistry
가 지연 초기화로 인해 아직까지 온전한 객체로 초기화되지 않은 상태에서HandlerMapping
이 완전히 초기화되어 자신이 사용되어지기를 기다리는 시점이 존재해야 할 이유가 없다고 생각했거든요.(물론 지금은 애플리케이션 시작 직후
DispatcherServlet
이 초기화되고, 그 과정에서 모든 의존성에 대해 체인 형태로 초기화가 진행되므로 큰 의미를 가지기는 어려울 수 있을 것 같네요)하지만 객체의 사용자가 반드시
initialize()
후 사용해야 하는 것을 알고 있어야 한다는 점과, 초기화 메서드의 강제 구현으로 인해 테스트에서 불편함을 느꼈던 것은 사실이라, 이 부분은 좀 더 고민해 보고 다음 단계에서 반영해 보도록 하겠습니다🙂@jminkkk