[MVC 구현하기 - 2단계] 페드로(류형욱) 미션 제출합니다.

[hw0603]

안녕하세요 몰리😁

이번 미션은 인터페이스 기반의 컨트롤러와 어노테이션 기반의 컨트롤러가 한 시스템 내에 공존할 수 있도록 구현하는 것이 목표였어요.

이에 HandlerMapping 인터페이스를 추가하여 ManualHandlerMapping과 AnnotationHandlerMapping 모두 공통 인터페이스를 구현하도록 해 두었습니다.

미션의 예시에서 등장한 새로운 RegisterController는 ModelAndView를 반환하는 반면, Controller 인터페이스를 구현하고 있는 컨트롤러들은 뷰 이름을 문자열 형태로 반환합니다.

이들 사이의 간극을 해결하기 위해 HandlerAdapter 인터페이스를 별도로 두어, 최종적으로는 ModelAndView를 반환할 수 있도록 구현했어요.

Controller 인터페이스와 어노테이션을 이미 프레임워크에서 제공하고 있으므로, 이들을 다룰 수 있는 어댑터 역시 프레임워크에서 제공해야 합니다.

HandlerMapping의 경우 동적으로 핸들러를 스캔하는 @Controller 어노테이션의 특성 상 AnnotationHandlerMapping은 프레임워크가 제공하며, ManualHandlerMapping은 프레임워크 사용자의 명시적인 매핑이라고 판단해 app 모듈 내에 위치시키고 DispatcherServletInitalizer에서 등록해 주었습니다.

미션에서 제공한 코드 상에서 DispatcherServlet이 app 모듈에 위치하고, mvc 모듈 내의 패키지인 asis와 tobe의 사전적 의미를 생각해 봤을 때 아직 패키지를 깔끔하게 분리하는 것이 무의미한 것 같아 프레임워크와 사용자 패키지 간의 최소한의 분리만 해 두었습니다. 리뷰하실 때 참고 부탁드려요!

+) mvc 모듈 내에 있는 HandlerMappingRegistry가 "com.techcourse.controller"라는 상수값을 알고 있어야 하는 것도 영 맘에 안 들긴 하네요 허허
나중에 패키지 구조가 정리되면 사용자 쪽에서 주입받거나 하는 식으로 리팩토링이 가능할 것 같아요.

이번에도 리뷰 잘 부탁합니다~~🚀

[hw0603]

질문)
예외 발생 케이스 말고 정상 동작 케이스도 테스트를 하고 싶었는데요...
뭔가 깔끔한 테스트 방법을 찾지 못해서 로컬에서만 끄적거리고 있어요.

모킹하여 테스트하기에는 HttpServletRequest의 HttpSession과 RequestDispatcher 같은 객체들까지 전부다 모킹을 해줘야 테스트가 돌아갈 것 같은데, 너무 구현에 의존적인 테스트가 될 것 같아요.

테스트만을 위한 게터를 열지 않으면서, 직접 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();
    }

[jminkkk]

안녕하세요 페드로 ~!
질문 주신 부분에 대해 열심히 찾아보고 고민을 해봤는데 저도 관련 객체들을 다 모킹을 하거나 테스트용 더블을 만드는 것 밖에 생각이 안나더라구요 🥲

그러던 중에 전 기수 분중에 페드로가 테스트하고 싶은 부분과 동일한 역할을 테스트한 코드가 있는 것 같아 가져와봤어요 (덕분에 저도 보면서 재미있었네요)

현재 코드에서는 when(request.getRequestDispatcher("/register.jsp")).thenReturn(mock(RequestDispatcher.class)); 부분을 목킹하고 있긴 한데, 페드로가 테스트하고 싶은 부분이 GET /register 요청이 잘 처리 되는지라면, 이 코드가 도움이 될 수도 있을 것 같습니다

request.getRequestDispatcher("/register.jsp")가 호출되었다는 건. 현재 요청을 "/register.jsp" 페이지로 전달할 준비가 되었다는 것을 의미이니 참고해보시면 좋을 것 같아요 👍

https://github.com/woowacourse/java-mvc/pull/500/files#diff-91350e7fa663241ea8f1287c70efc63b81b42619bf9972055b5632c17e716c66

[hw0603]

와 전 기수 분 코드까지 찾아봐 주시다니 감동인걸요 😄
읽어 봤는데 여전히 모킹을 해 줘야 하는 부분이 존재하긴 하지만 그래도 제가 생각했던 구조랑 가장 유사한 구현인 것 같아요!

참고하여 반영해 두었습니다 갑사합니다🔥

[jminkkk]

안녕하세요 페드로~!
2단계도 코드가 깔끔해서 보기 좋네요 최고 👍
질문이나 말씀해주신 부분들로부터 저도 많이 배워가는 것 같아요 🔥

끝까지 파이팅해봅시다!!!!!!!!

[jminkkk]

HandlerMappingRegistry과 HandlerAdapterRegistry 모두 외부(DispatcherServlet)에서 초기화를 하는 이유가 있을까요?
각 객체 생성 시에 내부에서 진행되도록 변경해도 될 것 같기도 해요!
페드로의 생각은 어떤가요 👀

[hw0603]

현재 DispatcherServltet은 DispatcherServletInitializer 내의 onStartup() 호출 직후 바로
빈 인스턴스가 만들어지고, 사용자 지정 HandlerMapping이 등록된 이후 내부적으로 DispatcherServlet.init()이 호출되어 지연 초기화되는 구조로 작성되어 있어요.

프레임워크에서 자동으로 등록해 준 매핑보다는 사용자가 명시적으로 추가해 준(dispatcherServlet.addHandlerMapping(new ManualHandlerMapping())) 핸들러들이 먼저 검색되도록 하기 위해 HandlerMapping 들도 마찬가지로 초기화가 지연될 수 있도록 구현해 두었습니다.

[jminkkk]

프레임워크에서 자동으로 등록해 준 매핑보다는 사용자가 명시적으로 추가해 준(dispatcherServlet.addHandlerMapping(new ManualHandlerMapping())) 핸들러들이 먼저 검색되도록 하기 위해 HandlerMapping 들도 마찬가지로 초기화가 지연될 수 있도록 구현해 두었습니다.

의도가 너무 좋네요 👍
저도 커스텀이 우선시 되어야 한다고 생각해요.

그런데 제가 요구사항에 의한 구조적인 문제 때문에 페드로가 어디까지를 프레임워크 역할로 또는 사용자 역할로 보고 있는지가 조금 헷갈리는 것 같아요. 어색하게 느껴지는 부분이 몇 가지 있어서 질문 드릴게요.

---

먼저 new ManualHandlerMapping은 DispatcherServlet이 아닌 DispatcherServletInitializer에서 등록을 해주고 있는데
반면, new AnnotationHandlerMapping(ANNOTATION_BASED_CONTROLLER_BASE)에서
com.techcourse.controller라는 특정 애플리케이션에 종속된 패키지를 HandlerMappingRegistry 에서 선언해주고 있는 것 같아요. 이 패키지 or 핸들러 매핑은 DispatcherServletInitializer에서 주입 하지 않은 이유가 궁금해요!

---

그리고 여전히 각 HandlerMapping의 초기화가 외부에서 이루어져야만 하는지는 이해가 잘 가지 않아요.
HandlerMappingRegistry는 페드로 말씀처럼 사용자 추가 핸들러 매핑의 순서 조정을 위해 초기화를 지연시키는 것이 좋을 것 같아요. 그런데 HandlerMappingRegistry에서 각 핸들러 매핑의 순서가 지정될테니 각 HandlerMapping은 내부에서는 즉시 초기화가 되어도 괜찮지 않나요?

[hw0603]

먼저 new ManualHandlerMapping은 DispatcherServlet이 아닌 DispatcherServletInitializer에서 등록을 해주고 있는데
반면, new AnnotationHandlerMapping(ANNOTATION_BASED_CONTROLLER_BASE)에서
com.techcourse.controller라는 특정 애플리케이션에 종속된 패키지를 HandlerMappingRegistry 에서 선언해주고 있는 것 같아요. 이 패키지 or 핸들러 매핑은 DispatcherServletInitializer에서 주입 하지 않은 이유가 궁금해요!

PR 본문에서 잠깐 언급했던 것 처럼, AnnotationHandlerMapping은 프레임워크 자체적으로 제공하는 '기능'으로 생각하고 구현했어요. @Controller 어노테이션만 붙여 놓으면 프레임워크에서 알아서 스캔해서 등록을 해 주는 기능이니, 사용자가 관여할 필요가 없다고 생각해서 DispatcherServletInitializer에서 따로 등록하지 않았습니다.

com.techcourse.controller라는 특정 애플리케이션에 종속된 패키지를 HandlerMappingRegistry 에서 선언해주고 있는 것 같아요

물론 말씀주신 것 처럼 지금은 애플리케이션 패키지 경로를 상수형태로 의존하고 있어서 책임분리가 완전히 되어 있다고 보기는 힘든데요,

미션에서 제공한 코드 상에서 DispatcherServlet이 app 모듈에 위치하고, mvc 모듈 내의 패키지인 asis와 tobe의 사전적 의미를 생각해 봤을 때 아직 패키지를 깔끔하게 분리하는 것이 무의미한 것 같아 프레임워크와 사용자 패키지 간의 최소한의 분리만 해 두었습니다. 리뷰하실 때 참고 부탁드려요!

+) mvc 모듈 내에 있는 HandlerMappingRegistry가 "com.techcourse.controller"라는 상수값을 알고 있어야 하는 것도 영 맘에 안 들긴 하네요 허허
나중에 패키지 구조가 정리되면 사용자 쪽에서 주입받거나 하는 식으로 리팩토링이 가능할 것 같아요.

PR 본문에서 이렇게 말씀드렸던 이유이기도 해요!
다음 단계를 살짝 살펴 보니 asis, tobe 패키지가 드디어(!) 제거되는 것 같아서 완전한 분리를 3단계에서 수행하기 위해 현재의 구현으로 요청 드렸어요.

---

HandlerMappingRegistry에서 각 핸들러 매핑의 순서가 지정될테니 각 HandlerMapping은 내부에서는 즉시 초기화가 되어도 괜찮지 않나요?

오.. 예리하시군요 +_+ 그렇게도 생각할 수 있겠어요.
저는 DispatcherServlet -> HandlerMappingRegistry 까지 전부 지연 초기화 전략을 택하고 있으니, 그 내부에서 관리되는 객체인 HandlerMapping 역시 지연 초기화를 지원하는 것이 자연스럽다고 생각했어요. 현재 HandlerMapping을 사용하는 곳이 HandlerMappingRegistry 뿐인데, HandlerMappingRegistry가 지연 초기화로 인해 아직까지 온전한 객체로 초기화되지 않은 상태에서 HandlerMapping이 완전히 초기화되어 자신이 사용되어지기를 기다리는 시점이 존재해야 할 이유가 없다고 생각했거든요.
(물론 지금은 애플리케이션 시작 직후 DispatcherServlet이 초기화되고, 그 과정에서 모든 의존성에 대해 체인 형태로 초기화가 진행되므로 큰 의미를 가지기는 어려울 수 있을 것 같네요)

하지만 객체의 사용자가 반드시 initialize() 후 사용해야 하는 것을 알고 있어야 한다는 점과, 초기화 메서드의 강제 구현으로 인해 테스트에서 불편함을 느꼈던 것은 사실이라, 이 부분은 좀 더 고민해 보고 다음 단계에서 반영해 보도록 하겠습니다🙂

@jminkkk

[jminkkk]

이 부분도 void initialize() 이 메서드까지 구현을 강제한 이유가 궁금해요!

[hw0603]

#726 (comment) 이쪽 코멘트 참고해주세요!

[jminkkk]

안녕하세요 페드로 ~!
질문 주신 부분에 대해 열심히 찾아보고 고민을 해봤는데 저도 관련 객체들을 다 모킹을 하거나 테스트용 더블을 만드는 것 밖에 생각이 안나더라구요 🥲

그러던 중에 전 기수 분중에 페드로가 테스트하고 싶은 부분과 동일한 역할을 테스트한 코드가 있는 것 같아 가져와봤어요 (덕분에 저도 보면서 재미있었네요)

현재 코드에서는 when(request.getRequestDispatcher("/register.jsp")).thenReturn(mock(RequestDispatcher.class)); 부분을 목킹하고 있긴 한데, 페드로가 테스트하고 싶은 부분이 GET /register 요청이 잘 처리 되는지라면, 이 코드가 도움이 될 수도 있을 것 같습니다

request.getRequestDispatcher("/register.jsp")가 호출되었다는 건. 현재 요청을 "/register.jsp" 페이지로 전달할 준비가 되었다는 것을 의미이니 참고해보시면 좋을 것 같아요 👍

https://github.com/woowacourse/java-mvc/pull/500/files#diff-91350e7fa663241ea8f1287c70efc63b81b42619bf9972055b5632c17e716c66

[jminkkk]

안녕하세요 페드로!
리뷰가 잘 반영해주셨고 테스트도 잘 작성해주셨네요.
요구사항이 충분히 만족되어 approve 합니다 🔥

(조금 이해가 되지 않는 부분이 있어 질문 남겼으니 다음 PR이나 코멘트로 답변 주시면 감사하겠습니다 🍀)