-
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단계] 에버(손채영) 미션 제출합니다. #784
Changes from 9 commits
b6443ac
362d06d
c06d58e
113c712
ae496e4
f123475
1ba01c4
23b1747
4b9352d
dac5d9a
6c8038d
4340897
5664ca1
08874f3
9877f26
6f8d9b6
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,12 +1,17 @@ | ||
package com.techcourse; | ||
|
||
import com.interface21.webmvc.servlet.ModelAndView; | ||
import com.interface21.webmvc.servlet.View; | ||
import com.interface21.webmvc.servlet.mvc.asis.Controller; | ||
import com.interface21.webmvc.servlet.mvc.tobe.AnnotationHandlerMapping; | ||
import com.interface21.webmvc.servlet.mvc.tobe.HandlerExecution; | ||
import com.interface21.webmvc.servlet.mvc.tobe.HandlerMapping; | ||
import com.interface21.webmvc.servlet.mvc.tobe.HandlerMappingAdapter; | ||
import com.interface21.webmvc.servlet.view.JspView; | ||
import jakarta.servlet.ServletException; | ||
import jakarta.servlet.http.HttpServlet; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import java.util.Arrays; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
|
@@ -15,15 +20,18 @@ public class DispatcherServlet extends HttpServlet { | |
private static final long serialVersionUID = 1L; | ||
private static final Logger log = LoggerFactory.getLogger(DispatcherServlet.class); | ||
|
||
private ManualHandlerMapping manualHandlerMapping; | ||
private HandlerMapping[] handlerMappings; | ||
|
||
public DispatcherServlet() { | ||
} | ||
|
||
@Override | ||
public void init() { | ||
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. 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. 또한, 우리 app을 보면 tomcat을 실행시키는 코드만 있는데, 어떻게 우리가 만든 mvc를 사용할 수 있을까요? 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. 질문의 의도가 잘못 전달된 것 같네요😅 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. SpringServletContainerInitializer 클래스 내부 구현을 통해 알 수 있었습니다!
이 시점에 WebApplicationInitializer 인터페이스를 구현한 DispatcherServletInitializer 클래스의 onStartup 메서드가 실행되고, DispatcherServlet이 servletContext에 등록됨을 확인할 수 있었습니다. ServletContext에 추가된 서블릿은 서블릿 컨테이너에 의해 관리되며, DispatcherServlet의 init, service, destroy 등에 대한 작업을 처리합니다! 즉 위 방식으로 서블릿의 생명주기와 요청 처리를 담당하게 됩니다! |
||
manualHandlerMapping = new ManualHandlerMapping(); | ||
manualHandlerMapping.initialize(); | ||
handlerMappings = new HandlerMapping[]{ | ||
new ManualHandlerMapping(), | ||
new HandlerMappingAdapter(new AnnotationHandlerMapping()) | ||
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. HandlerMappingAdapter라는 이름이 혼란을 주는 것 같아요! 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. 현재 HandlerMappingAdapter 클래스는 정확히 AnnotationHandlerMapping을 호환시키기 위한 래핑 클래스이기 때문에, 클래스명을 AnnotationHandlerMappingAdapter로 바꾸도록 하겠습니다! 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. 먼저, 저는 최대한 스프링의 구현 방식을 참고하지 않고 미션을 진행하려 했습니다. 그 과정에서 ManualHandlerMapping(기존)과 AnnotationHandlerMapping(도입)의 호환성을 보장하기 위해 adapter 패턴을 떠올렸고, adapter 패턴의 대표적인 구조인 아래 방식을 채택하였습니다. 즉, 그림의 (하지만, 스프링의 구현 방식을 알아보기 위해 이번 리뷰 반영 과정에서 LMS에서 제공한 방법을 도입했습니다 😊) 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. LMS의 방법을 적용했지만, 에버가 남겨주신 답변에 대해 추가적으로 코멘트 남깁니다:)
어댑터 패턴을 적용하는 이유는 인터페이스에 맞지 않는 여러 객체들을 동일한 인터페이스로 묶어 처리하기 위함인데, 기존 구조에서는 AnnotationHandlerMapping이 직접 HandlerMapping을 구현하지 않고, 굳이 어댑터를 통해 이 역할을 분리하고 있기 때문에 중복된 역할이 발생합니다. 이는 구조를 더 복잡하게 만들고, 어댑터의 도입 목적에 어긋난다고 생각했어요! |
||
}; | ||
Arrays.stream(handlerMappings).forEach(HandlerMapping::initialize); | ||
} | ||
|
||
@Override | ||
|
@@ -33,14 +41,36 @@ protected void service(final HttpServletRequest request, final HttpServletRespon | |
log.debug("Method : {}, Request URI : {}", request.getMethod(), requestURI); | ||
|
||
try { | ||
final var controller = manualHandlerMapping.getHandler(requestURI); | ||
final var viewName = controller.execute(request, response); | ||
ModelAndView modelAndView = new ModelAndView(new JspView(viewName)); | ||
View view = modelAndView.getView(); | ||
view.render(modelAndView.getModel(), request, response); | ||
Object handler = getHandler(request); | ||
ModelAndView modelAndView = execute(request, response, handler); | ||
modelAndView.render(request, response); | ||
} catch (Throwable e) { | ||
log.error("Exception : {}", e.getMessage(), e); | ||
throw new ServletException(e.getMessage()); | ||
} | ||
} | ||
|
||
private Object getHandler(HttpServletRequest request) { | ||
for (HandlerMapping handlerMapping : handlerMappings) { | ||
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. HandlerMapping 일급 컬렉션이 생긴다면 이 책임도 위임해볼 수 있겠네요😊 |
||
Object handler = handlerMapping.getHandler(request); | ||
if (handler != null) { | ||
return handler; | ||
} | ||
} | ||
throw new IllegalArgumentException("해당 요청을 처리하는 핸들러가 없습니다: %s %s" | ||
.formatted(request.getMethod(), request.getRequestURI())); | ||
} | ||
|
||
private ModelAndView execute(HttpServletRequest request, HttpServletResponse response, Object handler) | ||
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. Handler의 종류가 늘어난다면 If문 분기도 늘어나겠네요? 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. Registry 패턴을 도입하면서 분기를 줄여보았습니다! 앞으로 새로운 어댑터가 추가되어도 support 메서드만 잘 구현한다면 불필요하게 if문이 늘어나지 않겠어요! 👍 |
||
throws Exception { | ||
if (handler instanceof Controller) { | ||
String viewName = ((Controller) handler).execute(request, response); | ||
return new ModelAndView(JspView.from(viewName)); | ||
} | ||
if (handler instanceof HandlerExecution) { | ||
return ((HandlerExecution) handler).handle(request, response); | ||
} | ||
throw new IllegalArgumentException("해당 요청을 수행할 수 없습니다: %s %s" | ||
.formatted(request.getMethod(), request.getRequestURI())); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,30 @@ | ||
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 { | ||
final var user = new User(2, | ||
@RequestMapping(value = "/register", method = RequestMethod.POST) | ||
public ModelAndView save(HttpServletRequest req, HttpServletResponse res) { | ||
User user = new User(2, | ||
req.getParameter("account"), | ||
req.getParameter("password"), | ||
req.getParameter("email")); | ||
InMemoryUserRepository.save(user); | ||
return new ModelAndView(JspView.from("/index.jsp")); | ||
} | ||
|
||
return "redirect:/index.jsp"; | ||
@RequestMapping(value = "/register", method = RequestMethod.GET) | ||
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. 페이지 rendering이 안되고 있어요! (login.jsp 수정!) 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. 헉 숨겨진 /register/view 가 있었네요. 수정하였습니다! |
||
public ModelAndView show(HttpServletRequest req, HttpServletResponse res) { | ||
return new ModelAndView(JspView.from("/register.jsp")); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package com.techcourse; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import com.interface21.webmvc.servlet.mvc.tobe.AnnotationHandlerMapping; | ||
import com.interface21.webmvc.servlet.mvc.tobe.HandlerMappingAdapter; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
class HandlerMappingTest { | ||
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. 제가 이전에 구현한 구조에 따르면 위 테스트는 Legacy MVC 방식의 요청 처리와 @MVC 방식의 요청 처리가 동시에 수행 가능한지를 테스트하기 위한 클래스였습니다! 현재는 스프링의 구조를 따르기로 하였지만, 이전에 구현했던 HandlerMapping 인터페이스를 구현한 ManualHandlerMapping과 AnnotationHandlerMappingAdapter 중 어떤 HandlerMapping이 요청을 처리하는지 확인하고 싶었습니다. 하지만 스프링 구조를 따르기로 한 지금, 위 테스트는 HandlerMappingRegistryTest가 될 것 같네요! |
||
|
||
private ManualHandlerMapping manualHandlerMapping; | ||
private HandlerMappingAdapter annotationHandlerMapping; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
manualHandlerMapping = new ManualHandlerMapping(); | ||
annotationHandlerMapping = new HandlerMappingAdapter(new AnnotationHandlerMapping()); | ||
manualHandlerMapping.initialize(); | ||
annotationHandlerMapping.initialize(); | ||
} | ||
|
||
@DisplayName("@MVC로 구현된 요청이 들어올 경우 AnnotationHandlerMapping가 해당 요청을 처리한다.") | ||
@ParameterizedTest | ||
@ValueSource(strings = {"GET", "POST"}) | ||
void should_mapAnnotationHandlerMapping_when_NewMvc(String requestMethod) { | ||
// given | ||
HttpServletRequest request = mock(HttpServletRequest.class); | ||
when(request.getRequestURI()).thenReturn("/register"); | ||
when(request.getMethod()).thenReturn(requestMethod); | ||
|
||
// when & then | ||
assertThat(manualHandlerMapping.getHandler(request)).isNull(); | ||
assertThat(annotationHandlerMapping.getHandler(request)).isNotNull(); | ||
} | ||
|
||
@DisplayName("Legacy MVC로 구현된 요청이 들어올 경우 ManualHandlerMapping가 해당 요청을 처리한다.") | ||
@ParameterizedTest | ||
@ValueSource(strings = {"/", "/login", "/login/view", "/logout"}) | ||
void should_mapManualHandlerMapping_when_LegacyMvc(String requestUri) { | ||
// given | ||
HttpServletRequest request = mock(HttpServletRequest.class); | ||
when(request.getRequestURI()).thenReturn(requestUri); | ||
when(request.getMethod()).thenReturn("GET"); | ||
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. 맞습니다. 기존 코드에서는 RequestMethod.find()하는 과정에서 예외가 발생하여 임시로 넣어둔 코드였어요! 하지만 현재 변경된 구조에서는 registry가 예외가 발생하는 곳까지 갈 수 없도록 필터링을 해주어서 예외가 발생하지 않네요. 해당 코드는 삭제했습니다! |
||
|
||
// when & then | ||
assertThat(manualHandlerMapping.getHandler(request)).isNotNull(); | ||
assertThat(annotationHandlerMapping.getHandler(request)).isNull(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
package com.techcourse; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
import com.interface21.webmvc.servlet.mvc.asis.Controller; | ||
import com.interface21.webmvc.servlet.mvc.asis.ForwardController; | ||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class ManualHandlerMappingTest { | ||
|
||
private ManualHandlerMapping handlerMapping; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
handlerMapping = new ManualHandlerMapping(); | ||
} | ||
|
||
@DisplayName("초기화를 진행할 경우 직접 입력한 uri와 핸들러를 매핑한다.") | ||
@Test | ||
void should_mapControllers_when_init() { | ||
// given | ||
HttpServletRequest request = mock(HttpServletRequest.class); | ||
when(request.getRequestURI()).thenReturn("/"); | ||
|
||
// when | ||
handlerMapping.initialize(); | ||
|
||
// then | ||
Object actual = handlerMapping.getHandler(request); | ||
assertThat(actual).isInstanceOf(ForwardController.class); | ||
} | ||
|
||
@DisplayName("해당 uri에 매핑되는 핸들러를 찾아 실행한다.") | ||
@Test | ||
void should_returnController_when_getHandlerWithIndexUri() throws Exception { | ||
// given | ||
HttpServletRequest request = mock(HttpServletRequest.class); | ||
HttpServletResponse response = mock(HttpServletResponse.class); | ||
when(request.getRequestURI()).thenReturn("/"); | ||
handlerMapping.initialize(); | ||
|
||
// when | ||
Controller controller = (Controller) handlerMapping.getHandler(request); | ||
String viewName = controller.execute(request, response); | ||
|
||
// then | ||
assertThat(viewName).isEqualTo("/index.jsp"); | ||
} | ||
|
||
@DisplayName("해당 uri에 매핑되는 핸들러가 없는 경우 null을 반환한다.") | ||
@Test | ||
void should_returnNull_when_getHandlerWithInvalidRequest() { | ||
// given | ||
HttpServletRequest request = mock(HttpServletRequest.class); | ||
when(request.getRequestURI()).thenReturn("/invalid"); | ||
|
||
// when | ||
Object handler = handlerMapping.getHandler(request); | ||
|
||
// then | ||
assertThat(handler).isNull(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
package com.interface21.webmvc.servlet; | ||
|
||
import jakarta.servlet.http.HttpServletRequest; | ||
import jakarta.servlet.http.HttpServletResponse; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
@@ -14,6 +16,10 @@ public ModelAndView(final View view) { | |
this.model = new HashMap<>(); | ||
} | ||
|
||
public void render(HttpServletRequest request, HttpServletResponse response) throws Exception { | ||
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. 제가 ModelAndView 클래스 내에 render 메서드를 생성한 이유는 아래 코드의 getter 사용을 제거하기 위함이었습니다. View view = modelAndView.getView();
view.render(modelAndView.getModel(), request, response); modelAndView에서 view 객체를 꺼내, 해당 객체의 render 메서드를 호출하는 로직이 존재했어요. 하지만 modelAndView 내부에 view 객체가 존재하기 때문에 굳이 꺼내지 않고 modelAndView의 render 메서드를 호출하면 해당 메서드가 view의 render 메서드를 호출하는 것이 더욱 객체지향스러운 방법이라 생각해 채택한 방법이었습니다! 현재도 HandlerAdapter가 ModelAndView를 반환하고 있고, 리니가 말씀주신 부분에 대해서는 충족된 코드인 것 같은데, 어떻게 생각하시나요? 😲 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. 저는 ModelAndView는 parameter와 viewName을 들고 있는 객체일 뿐이라고 생각했어요! 구체적으로 어떻게 render 할 것인지를 결정하는 책임은 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. 스프링이 제시하는 ModelAndView의 책임은 리니의 의견과 일치하는 것 같네요! 3단계에서 JsonView를 도입하는 과정에서 한 번 더 고민해보도록 할게요! |
||
view.render(model, request, response); | ||
} | ||
|
||
public ModelAndView addObject(final String attributeName, final Object attributeValue) { | ||
model.put(attributeName, attributeValue); | ||
return this; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.interface21.webmvc.servlet.mvc.tobe; | ||
|
||
import jakarta.servlet.http.HttpServletRequest; | ||
|
||
public interface HandlerMapping { | ||
|
||
void initialize(); | ||
|
||
Object getHandler(HttpServletRequest request); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package com.interface21.webmvc.servlet.mvc.tobe; | ||
|
||
import jakarta.servlet.http.HttpServletRequest; | ||
|
||
public class HandlerMappingAdapter implements HandlerMapping { | ||
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. 앞서 DispatcherServlet에 코멘트 달아놓았듯, 네이밍과 AnnotationHandlerMapping을 Wrapping하는 형태가 어색하게 다가옵니다! 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. 위에서 그림과 함께 언급 드렸듯, 새로 추가하는 클래스(AnnotationHandlerMapping)와 기존에 존재하던 클래스(ManualHandlerMapping)의 형태를 통일시키기 위해, AnnotationHandlerMapping를 AnnotationHandlerMappingAdapter로 감싸, AnnotationHandlerMappingAdapter와 ManualHandlerMapping를 같은 추상화 레벨로 맞추고 싶었어요! 하지만 역시, 이번 리뷰 반영 과정에서 스프링의 구조를 채택했다보니, 해당 구조는 사라지게 되었습니다 🥲 |
||
|
||
private final AnnotationHandlerMapping annotationHandlerMapping; | ||
|
||
public HandlerMappingAdapter(AnnotationHandlerMapping annotationHandlerMapping) { | ||
this.annotationHandlerMapping = annotationHandlerMapping; | ||
} | ||
|
||
@Override | ||
public void initialize() { | ||
annotationHandlerMapping.initialize(); | ||
} | ||
|
||
@Override | ||
public Object getHandler(HttpServletRequest request) { | ||
try { | ||
return annotationHandlerMapping.getHandler(request); | ||
} catch (IllegalArgumentException e) { | ||
return null; | ||
} | ||
} | ||
} |
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.
여러 HandlerMapping 클래스를 관리하는 클래스를 생성하였습니다! 자연스럽게 registry 패턴이 도입되네요 :)