[MVC 구현하기 - 2단계] 에버(손채영) 미션 제출합니다. #784

helenason · 2024-09-28T16:08:24Z

안녕하세요 리니!

오랜만에 리뷰 요청 드리네요 🥲

정말 정신없는 일주일이었어요. 리니는 시간 관리 잘 하고 계신가요?

이번 리뷰도 잘 부탁드립니다!

…r 패턴 적용

linirini

에버~ 데모데이 잘 마무리하셨나요? ㅎㅎ
정말 정신없이 데모데이가 지나간 것 같아요!
2단계 미션 구현하시느라 수고하셨습니다.!
이번 미션의 요구사항은 LegacyMVC를 지원하기 위해 HandlerAdapter와 HandlerMapping을 구현하는 것이라고 생각하는데요, HandlerAdapter 관련 부분에서 미션의 의도와 조금 다르다는 느낌을 받아서 코멘트를 남겨보았습니다.
코멘트 반영 후, 재리뷰요청 해주세요~

linirini · 2024-09-28T16:28:27Z

app/src/main/java/com/techcourse/DispatcherServlet.java

@@ -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;


일급 컬렉션으로 만드면 좋을 것 같아요,
그리고 배열 대신 리스트를 사용해봐요😊

여러 HandlerMapping 클래스를 관리하는 클래스를 생성하였습니다! 자연스럽게 registry 패턴이 도입되네요 :)

linirini · 2024-09-28T16:34:03Z

app/src/main/java/com/techcourse/DispatcherServlet.java

-        manualHandlerMapping.initialize();
+        handlerMappings = new HandlerMapping[]{
+                new ManualHandlerMapping(),
+                new HandlerMappingAdapter(new AnnotationHandlerMapping())


HandlerMappingAdapter라는 이름이 혼란을 주는 것 같아요!
처음에 봤을 때, 왜 HandlerMapping에 Adapter를 저장하지?라는 생각이 먼저 들었어요.
에버가 위와 같이 네이밍한 이유가 궁금합니다!
그리고, HandlerMappingAdapter는 AnnotationHandlerMapping을 Wrapping하고 있는 것 같은데, Wrapping 한 이유가 궁금합니다!

현재 HandlerMappingAdapter 클래스는 정확히 AnnotationHandlerMapping을 호환시키기 위한 래핑 클래스이기 때문에, 클래스명을 AnnotationHandlerMappingAdapter로 바꾸도록 하겠습니다!

먼저, 저는 최대한 스프링의 구현 방식을 참고하지 않고 미션을 진행하려 했습니다.

그 과정에서 ManualHandlerMapping(기존)과 AnnotationHandlerMapping(도입)의 호환성을 보장하기 위해 adapter 패턴을 떠올렸고, adapter 패턴의 대표적인 구조인 아래 방식을 채택하였습니다.

즉, 그림의 Client가 제 코드에서의 DispatcherServlet, Client Interface가 HandlerMapping, Adapter가 AnnotationHandlerMappingAdapter(구 HandlerMappingAdapter), Service가 AnnotationHandlerMapping 클래스의 역할을 한다고 볼 수 있어요!

(하지만, 스프링의 구현 방식을 알아보기 위해 이번 리뷰 반영 과정에서 LMS에서 제공한 방법을 도입했습니다 😊)

LMS의 방법을 적용했지만, 에버가 남겨주신 답변에 대해 추가적으로 코멘트 남깁니다:)

즉, 그림의 Client가 제 코드에서의 DispatcherServlet, Client Interface가 HandlerMapping, Adapter가 AnnotationHandlerMappingAdapter(구 HandlerMappingAdapter), Service가 AnnotationHandlerMapping 클래스의 역할을 한다고 볼 수 있어요!

어댑터 패턴을 적용하는 이유는 인터페이스에 맞지 않는 여러 객체들을 동일한 인터페이스로 묶어 처리하기 위함인데, 기존 구조에서는 AnnotationHandlerMapping이 직접 HandlerMapping을 구현하지 않고, 굳이 어댑터를 통해 이 역할을 분리하고 있기 때문에 중복된 역할이 발생합니다. 이는 구조를 더 복잡하게 만들고, 어댑터의 도입 목적에 어긋난다고 생각했어요!
다양한 방식의 컨트롤러가 호환될 수 있기 위해 도입한 어댑터 패턴과 핸들러 매핑 정보를 저장하고 있는 HandlerMapping의 책임 분리가 명확하게 이루어지지 않았으며, AnnotationHandlerMappingAdapter가 HandlerMapping을 구현하면서, 기존의 다른 HandlerMapping 구현체들과의 일관성을 깨뜨릴 가능성이 있다는 점에서 리스코프 원칙도 위배된다고 생각했어요!

linirini · 2024-09-28T16:36:31Z

app/src/main/java/com/techcourse/DispatcherServlet.java

+                .formatted(request.getMethod(), request.getRequestURI()));
+    }
+
+    private ModelAndView execute(HttpServletRequest request, HttpServletResponse response, Object handler)


Handler의 종류가 늘어난다면 If문 분기도 늘어나겠네요?
다양한 방식의 컨트롤러를 지원할 수 있게 하기 위해 어댑터 패턴을 적용해보는 것이 이번 단계의 의도 중 하나라고 생각해요.
HandlerAdapter의 개념을 공부해보고, 리팩터링 해보면 좋을 것 같습니다!

Registry 패턴을 도입하면서 분기를 줄여보았습니다! 앞으로 새로운 어댑터가 추가되어도 support 메서드만 잘 구현한다면 불필요하게 if문이 늘어나지 않겠어요! 👍

linirini · 2024-09-28T16:48:08Z

mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java

@@ -14,6 +16,10 @@ public ModelAndView(final View view) {
        this.model = new HashMap<>();
    }

+    public void render(HttpServletRequest request, HttpServletResponse response) throws Exception {


ModelAndView는 컨트롤러에서의 서블릿 종속성(HttpServletRequest)을 제거하고, View 이름을 전달하는 객체로서 책임을 가진다고 생각해요.
그렇기 때문에 ModelAndView에서 view를 렌더링하는 책임까지 가지는게 어색하게 다가오는 것 같아요🤔
추가로, Controller의 반환 타입(String)과 HandlerExecution의 반환 타입(ModelAndView)은 다른데요! 또다른 Controller 형식이 생긴다면 그때에는 또다른 반환 타입을 가질 수도 있을 것 같아요!
그 부분을 ModelAndView라는 반환타입 중 하나가 변환 책임을 가지고 있는 것보다는 다양한 방식의 컨트롤러를 지원하는 HandlerAdapter에서 일관된 반환 타입을 반환할 수 있도록 변환 책임을 가지는 것이 자연스러울 것 같아요!

제가 ModelAndView 클래스 내에 render 메서드를 생성한 이유는 아래 코드의 getter 사용을 제거하기 위함이었습니다.

View view = modelAndView.getView(); view.render(modelAndView.getModel(), request, response);

modelAndView에서 view 객체를 꺼내, 해당 객체의 render 메서드를 호출하는 로직이 존재했어요. 하지만 modelAndView 내부에 view 객체가 존재하기 때문에 굳이 꺼내지 않고 modelAndView의 render 메서드를 호출하면 해당 메서드가 view의 render 메서드를 호출하는 것이 더욱 객체지향스러운 방법이라 생각해 채택한 방법이었습니다!

현재도 HandlerAdapter가 ModelAndView를 반환하고 있고, 리니가 말씀주신 부분에 대해서는 충족된 코드인 것 같은데, 어떻게 생각하시나요? 😲

저는 ModelAndView는 parameter와 viewName을 들고 있는 객체일 뿐이라고 생각했어요! 구체적으로 어떻게 render 할 것인지를 결정하는 책임은 DispatcherServlet에게 있다고 생각했습니다.
그런 의미에서 ModelAndView가 render가 가능해지면 Controller에게도 render할 수 있는 권한이 주어지겠죠? 물론 그저 제 의견입니다 ㅎㅎ
ModelAndView의 책임을 어디까지 볼지는 개인의 선택인 것 같기도 하네요 ㅎㅎ

스프링이 제시하는 ModelAndView의 책임은 리니의 의견과 일치하는 것 같네요! 3단계에서 JsonView를 도입하는 과정에서 한 번 더 고민해보도록 할게요!

linirini · 2024-09-28T16:51:45Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerMappingAdapter.java

+
+import jakarta.servlet.http.HttpServletRequest;
+
+public class HandlerMappingAdapter implements HandlerMapping {


앞서 DispatcherServlet에 코멘트 달아놓았듯, 네이밍과 AnnotationHandlerMapping을 Wrapping하는 형태가 어색하게 다가옵니다!
(힌트: AnnotationHandlerMapping가 제공하는 메서드와 HandlerMapping 인터페이스에서 제공하는 메서드의 역할이 동일한 것 같네요?)

위에서 그림과 함께 언급 드렸듯, 새로 추가하는 클래스(AnnotationHandlerMapping)와 기존에 존재하던 클래스(ManualHandlerMapping)의 형태를 통일시키기 위해, AnnotationHandlerMapping를 AnnotationHandlerMappingAdapter로 감싸, AnnotationHandlerMappingAdapter와 ManualHandlerMapping를 같은 추상화 레벨로 맞추고 싶었어요!

하지만 역시, 이번 리뷰 반영 과정에서 스프링의 구조를 채택했다보니, 해당 구조는 사라지게 되었습니다 🥲

linirini · 2024-09-28T16:54:34Z

app/src/main/java/com/techcourse/DispatcherServlet.java

@@ -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() {


DispatcherServlet의 생성과 초기화 시점이 다른 걸 확인할 수 있어요, 생성과 초기화 시점은 언제이며, 이 둘은 왜 분리되어 있을까요?
(제 리뷰어에게 받은 코멘트를 공유해봅니다😂)

또한, 우리 app을 보면 tomcat을 실행시키는 코드만 있는데, 어떻게 우리가 만든 mvc를 사용할 수 있을까요?
(마찬가지로, 제 리뷰어에게 받은 코멘트입니다 ... ㅎㅎ)

생성자와 init 메서드에 로그를 찍어 확인해본 결과, DispatcherServletInitializer 클래스의 onStartup 메서드에서 DispatcherServlet 생성자를 호출하고, init 메서드는 생성자 호출 후 이어 호출됨을 알 수 있었습니다. 이는 DispatcherServletInitializer 클래스 registration.setLoadOnStartup(1); 구문의 결과이고, 해당 코드를 삭제하면 첫 요청을 받았을 때 init 메서드가 호출됩니다!

app의 build.gradle 파일에서 mvc를 implement 받고 있기 때문에 가능한 것으로 알고 있습니다! implementation project(':mvc')

질문의 의도가 잘못 전달된 것 같네요😅
Tomcat을 start하는 코드는 있지만, app에서는 직접적으로 mvc를 호출하거나 사용하는 코드가 없습니다! 그럼에도 우리가 만든 mvc가 어떻게 사용되고 있는걸까요?
2번 힌트: SpringContainerInitializer

SpringServletContainerInitializer 클래스 내부 구현을 통해 알 수 있었습니다!

@HandlesTypes 어노테이션의 value인 WebApplicationInitializer 인터페이스를 구현한 클래스들이 onStartup 메서드의 파라미터 webAppInitializerClasses로 전달

ReflectionUtils.accessibleConstructor(waiClass).newInstance()를 통해 해당 클래스들의 인스턴스 생성

아래 코드를 통해 각 인스턴스의 onStartup 메서드 호출
for (WebApplicationInitializer initializer : initializers) { initializer.onStartup(servletContext); }

이 시점에 WebApplicationInitializer 인터페이스를 구현한 DispatcherServletInitializer 클래스의 onStartup 메서드가 실행되고, DispatcherServlet이 servletContext에 등록됨을 확인할 수 있었습니다. ServletContext에 추가된 서블릿은 서블릿 컨테이너에 의해 관리되며, DispatcherServlet의 init, service, destroy 등에 대한 작업을 처리합니다! 즉 위 방식으로 서블릿의 생명주기와 요청 처리를 담당하게 됩니다!

linirini · 2024-09-28T16:55:00Z

app/src/main/java/com/techcourse/DispatcherServlet.java

        } catch (Throwable e) {
            log.error("Exception : {}", e.getMessage(), e);
            throw new ServletException(e.getMessage());
        }
    }
+
+    private Object getHandler(HttpServletRequest request) {
+        for (HandlerMapping handlerMapping : handlerMappings) {


HandlerMapping 일급 컬렉션이 생긴다면 이 책임도 위임해볼 수 있겠네요😊

linirini · 2024-09-28T19:00:58Z

app/src/test/java/com/techcourse/HandlerMappingTest.java

+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        when(request.getRequestURI()).thenReturn(requestUri);
+        when(request.getMethod()).thenReturn("GET");


/login은 메서드 내부에서 getParameter로 request parameter를 가져오고 있기 때문에, GET 메서드를 사용하는 것은 적절하지 않아보입니다! 테스트이긴 하지만, 적절한 메서드로 설정하는 것이 좋을 것 같아서 말씀드려요 ㅎㅎ

맞습니다. 기존 코드에서는 RequestMethod.find()하는 과정에서 예외가 발생하여 임시로 넣어둔 코드였어요! 하지만 현재 변경된 구조에서는 registry가 예외가 발생하는 곳까지 갈 수 없도록 필터링을 해주어서 예외가 발생하지 않네요. 해당 코드는 삭제했습니다!

linirini · 2024-09-28T19:02:50Z

app/src/test/java/com/techcourse/HandlerMappingTest.java

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class HandlerMappingTest {


HandlerMapping 인터페이스는 mvc 모듈에 있는데, 테스트는 app 모듈에 위치해있네요!
추가적으로, 하나의 클래스에서 ManualHandlerMapping과 HandlerMappingAdapter를 함께 테스트하기보단, 해당 클래스의 테스트에 이동하는 것이 좋을 것 같아요:)

제가 이전에 구현한 구조에 따르면 위 테스트는 Legacy MVC 방식의 요청 처리와 @MVC 방식의 요청 처리가 동시에 수행 가능한지를 테스트하기 위한 클래스였습니다! 현재는 스프링의 구조를 따르기로 하였지만, 이전에 구현했던 HandlerMapping 인터페이스를 구현한 ManualHandlerMapping과 AnnotationHandlerMappingAdapter 중 어떤 HandlerMapping이 요청을 처리하는지 확인하고 싶었습니다.

하지만 스프링 구조를 따르기로 한 지금, 위 테스트는 HandlerMappingRegistryTest가 될 것 같네요!

helenason · 2024-09-29T11:04:02Z

안녕하세요 리니 😎

리뷰 주신 것 확인하고 반영했습니다!

처음 미션을 구현하는 단계에서는 최대한 스프링과 LMS가 제공한 코드를 따르지 않으려고 하였어요.
그렇다보니 더욱 리니에게 어색하게 느껴지는 부분들이 많았던 것 같습니다. 허점도 많았고요!

그래서 이번 리뷰를 반영할 때는 스프링의 구조를 따라보았습니다.
이전에 구현했던 저의 코드 의도와 함께 관련 코멘트 남겨보았으니 확인 부탁드립니다!

감사해요 😊

linirini · 2024-09-29T11:59:46Z

app/src/main/java/com/techcourse/controller/RegisterController.java


-        return "redirect:/index.jsp";
+    @RequestMapping(value = "/register", method = RequestMethod.GET)


페이지 rendering이 안되고 있어요! (login.jsp 수정!)

헉 숨겨진 /register/view 가 있었네요. 수정하였습니다!

linirini · 2024-09-29T12:00:41Z

app/src/test/java/com/techcourse/ManualHandlerAdapterTest.java

각종 HandlerAdapter와 HandlerMapping의 패키지 위치를 설정한 에버의 기준이 궁금합니다~

먼저, ManualHandlerMapping(app)과 AnnotationHandlerMapping(mvc) 클래스의 위치를 고정한 상태에서 진행했는데요. HandlerMapping 인터페이스의 경우 app 모듈에 위치하면 AnnotationHandlerMapping 클래스가 해당 인터페이스를 구현할 수 없기 때문에 자연스럽게 mvc에 위치시켰습니다! HandlerMappingRegistry 역시 app 모듈에 위치시켜야 해서 그렇게 해주었구요! HandlerAdapter의 경우에는 HandlerMapping과 일대일 매핑의 형태로 보여서 같은 방식으로 각 모듈에 위치시켜 주었습니다! 모듈 내부에서 추가로 생성한 패키지는 크게 없고, mvc 모듈 내에서 AnnotationHandler 관련 클래스만 /tobe/annotation에 위치시켰습니다 😊

linirini

에버! 남겨주신 코멘트에 대해 답변을 달았습니다! 저도 학습하는 과정이다보니 의견으로 받아들여주시면 좋을 것 같아요!
추가적으로 궁금한 부분과 버그가 있어서 빠르게 마지막 RC 남깁니다!
화이팅~

helenason · 2024-09-29T13:25:56Z

리니! 다양한 리뷰 감사해요

더 깊게 학습할 수 있는 기회가 되어 많은 도움 되었어요! 감사합니다 😊

linirini

에버~ 마지막까지 답변 달아주시느라 수고하셨습니다.
이만 머지하고 다음 단계로 넘어가봐요🤗

helenason added 9 commits September 28, 2024 16:57

refactor: ManualHandlerMapping AnnotationHandlerMapping 공존을 위해 Adapte…

b6443ac

…r 패턴 적용

chore: HandlerMapping HandlerMappingAdapter 위치 이동

362d06d

refactor: 핸들러 수행 책임 HandlerMapping 외부로 추출

c06d58e

refactor: 뷰 렌더링 로직 ModelAndView에서 호출

113c712

test: 핸들러 매핑 관련 테스트 추가

ae496e4

test: 요청이 올바른 핸들러 매핑을 찾는지 테스트

f123475

refactor: 유효하지 않은 요청에 대한 핸들러 반환 로직 호환

1ba01c4

refactor: JspView 캐시로 관리

23b1747

refactor: 불필요한 파일 제거

4b9352d

linirini requested changes Sep 28, 2024

View reviewed changes

helenason added 6 commits September 29, 2024 14:43

refactor: 어노테이션 핸들러 매핑 어댑터 이름 변경

dac5d9a

refactor: handlerMappings 리스트로 관리

6c8038d

refactor: HandlerMapping 레지스트리 패턴 도입

4340897

refactor: 스프링의 어댑터 구조 도입

5664ca1

chore: 패키지 구조 변경

08874f3

test: 테스트 보완 및 패키지 통일

9877f26

linirini reviewed Sep 29, 2024

View reviewed changes

linirini requested changes Sep 29, 2024

View reviewed changes

fix: 회원가입 페이지 조회 redirect uri 수정

6f8d9b6

linirini approved these changes Sep 30, 2024

View reviewed changes

linirini merged commit b12ac59 into woowacourse:helenason Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 - 2단계] 에버(손채영) 미션 제출합니다. #784

[MVC 구현하기 - 2단계] 에버(손채영) 미션 제출합니다. #784

helenason commented Sep 28, 2024

linirini left a comment

linirini Sep 28, 2024

helenason Sep 29, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 29, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 28, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 29, 2024

helenason Sep 29, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 28, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 29, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini Sep 28, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024

linirini Sep 28, 2024

helenason Sep 29, 2024 •

edited

Loading

helenason commented Sep 29, 2024

linirini Sep 29, 2024

helenason Sep 29, 2024

linirini Sep 29, 2024

helenason Sep 29, 2024 •

edited

Loading

linirini left a comment

helenason commented Sep 29, 2024

linirini left a comment


		import jakarta.servlet.http.HttpServletRequest;

		public class HandlerMappingAdapter implements HandlerMapping {


		return "redirect:/index.jsp";
		@RequestMapping(value = "/register", method = RequestMethod.GET)

[MVC 구현하기 - 2단계] 에버(손채영) 미션 제출합니다. #784

[MVC 구현하기 - 2단계] 에버(손채영) 미션 제출합니다. #784

Conversation

helenason commented Sep 28, 2024

linirini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

helenason commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

linirini left a comment

Choose a reason for hiding this comment

helenason commented Sep 29, 2024

linirini left a comment

Choose a reason for hiding this comment

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading