[MVC 구현하기 - 2단계] 몰리(김지민) 미션 제출합니다. #738

jminkkk · 2024-09-24T18:55:52Z

안녕하세요 알파카 🔥
프로젝트는 잘하고 계신가요 저는 데모데이 직전이라 정신이 없네요 🫥
이번 PR 욕심을 덜어내려고 했으나,,, 하다보니 욕심이 생겨 File Changed가 많이 잡혔네요 😅

전체적인 과정은 다음과 같습니다.

요청으로부터 DispatcherServlet은 HandlerMapping을 통해 Handler를 조회합니다.
조회한 Handler들은 여러 유형(RequestMapping Annotation or extends Controller) 으로 구현되어 있기 때문에 HandlerAdapter를 통해 일관된 ModelAndView를 반환하도록 처리합니다.
DispatcherServlet은 반환받은 ModelAndView를 View에게 넘겨 결과를 렌더링합니다.

이번 단계도 편하게 많은 이야기 나누면 좋을 것 같아요 리뷰 잘 부탁드립니다 🙌

…착된 어노테이션으로부터 핸들러 키 조회 역할 부여

…on으로 변경

…하여 변경

jminkkk · 2024-09-24T19:01:13Z

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

+public class RequestMappingInfo {
+
+    private final RequestMapping requestMapping;


고민이 하나 있습니다 파카알

Info라는 용어가 불용어임을 인지하고 있는데요.
RequestMapping를 받아 요청 정보(여기에서는 HandlerKey) 를 반환하는 적절한 객체의 이름이 도저히 생각나지 않더라구요.

더 적절한 이름이 없다면 불용어를 사용하더라도, 기존 Spring 구현에서 비슷한 의미로 사용되는 객체의 이름을 활용하는 것이 최선을 것 같아 이처럼 작성하게 되었습니다 🥲

이에 대해 파카는 어떻게 생각하시나요?

저도 고민해봤는데 지금보다 더 명확한 이름이 생각나지 않긴 하네요.. :몰리슬픔이2:
일단 충분히 역할과 의미는 잘 전달되는 것 같아서 괜찮다고 생각합니다.
킵고잉하시죠.

slimsha2dy

안녕하세요 몰!
제가 아직 2단계 미션을 제대로 진행하지 못해서 리뷰가 너무 지엽적으로 느껴지는 것 같기도 하네요.. 죄송합니다. 🙇
아마 2단계 미션을 제대로 진행하고 나서 추가적인 리뷰를 드리게 될 수도 있을 것 같습니다.
아무튼 일단 미션의 요구사항은 아주 잘 반영된 것으로 보이네요.
전체적인 구조도 너무 깔끔하고 관심사의 분리도 잘 된 것 같다고 생각합니다.
특히 테스트도 꼼꼼하게 잘 짜주신 것 같아요!
아마 별다른 이유나 추가 리뷰가 없다면 다음 리뷰 요청 때 머지를 할 수 있을 것 같습니다.
리뷰 보시다가 궁금한 점 생기시면 편하게 dm 주세요~

slimsha2dy · 2024-09-27T17:03:09Z

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

+
+public class ControllerHandlerAdapter implements HandlerAdapter {
+
+    private static final ControllerHandlerAdapter INSTANCE = new ControllerHandlerAdapter();


위의 DispatcherServlet이나 ManualHandlerMapping들도 하나의 인스턴스만 사용될 것 같은데 요녀석만 싱글톤으로 관리하는 이유가 궁금합니다람쥐.

생성자에 인자를 받고 있는 객체들도 있고 여러 이유로 하나의 인스턴스만 사용하는 객체들을 모두 싱글톤으로 만들기는 어려울 것 같아요. setter를 열고 싶지 않기도 하구요 🤔

현재는 일관성 있게 싱글톤으로 구현된 객체들을 제거하고 추후 스프링의 실제 구현과 비슷하게 빈 레지스트리를 통해 싱글톤으로 "관리"하는 방법이 좋을 것 같다는 생각이 드네요!

요 친구의 싱글톤 제거했습니다!

slimsha2dy · 2024-09-27T17:16:39Z

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

+        return new ModelAndView(view);
+    }
+
+    private String getViewName(Controller controller, HttpServletRequest request, HttpServletResponse response) {


완전 대박 사소한 부분이라 듣고 흘리셔도 됩니다.
개인적으로는 실제로 핸들러를 실행하는 로직이 getViewName이라는 메서드에 있는게 직관적이지 않게 느껴지는 것 같아요.
try-catch 때문에 분리하신 것 같은데 아예 차라리 분리를 안 해도 될 것 같고 아니면 메서드 명을 좀 더 직관적으로 바꿔도 괜찮을 것 같아요. 근데 저도 고민해봤는데 명확한 메서드명이 마땅치가 않은 것 같긴 하네요..ㅎㅎ
지금도 읽기 불편하진 않으니까 참고만 해주세요 🙇

억 그러네요
제가 너무 불편해요 바로 변경하겠습니다 🔥

slimsha2dy · 2024-09-27T17:22:55Z

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

+    boolean supports(final Object handler);
+
+    ModelAndView handle(final Object handler, final HttpServletRequest request, final HttpServletResponse response);
+


인터페이스 마지막 줄에 개행이 있네용.
밑에 HandlerMapping도 마찬가지던데 인터페이스에만 적용하는 컨벤션이신가요?

웁스 수정햇씁니다 😅

slimsha2dy · 2024-09-27T17:28:55Z

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

+public class RequestMappingInfo {
+
+    private final RequestMapping requestMapping;


저도 고민해봤는데 지금보다 더 명확한 이름이 생각나지 않긴 하네요.. :몰리슬픔이2:
일단 충분히 역할과 의미는 잘 전달되는 것 같아서 괜찮다고 생각합니다.
킵고잉하시죠.

slimsha2dy · 2024-09-27T17:36:32Z

mvc/src/main/java/com/interface21/webmvc/servlet/view/JspView.java

+    }
+
+    private boolean isForwardView(String viewName) {
+        return !viewName.startsWith(REDIRECT_PREFIX);


단순히 궁금해서 여쭤보는겁니다!
여기선 사라진 이전에 비슷한 역할을 하던 메서드 isRedirectView에선 viewName이 null인지 체크했었는데 여기선 빼신 이유가 뭔가용.

꼼꼼하십니다람쥐 수정해놓겠슴다!

slimsha2dy · 2024-09-27T17:41:53Z

mvc/src/test/java/com/interface21/webmvc/servlet/mvc/ControllerHandlerAdapterTest.java

+            modelAndView.getView().render(modelAndView.getModel(), request, response);
+            assertAll(
+                    () -> assertThat(argumentCaptor.getValue()).isEqualTo("/test.jsp"),
+                    () -> assertThat(modelAndView.getModel()).isEmpty()


getModel()이 비어있는지 테스트하는 부분은 어떤 목적이신지 궁금합니다.
modelAndView가 잘 반환됐는지 확인하는 건가요?

modelAndView가 잘 반환됐는지 확인하는 건가요?

맞습니다! ControllerHandlerAdapter는 Controller를 Extends한 핸들러(ex. TestExtendsController)들의 결과를 처리합니다.
ControllerHandlerAdapter 처리하고 있는 TestExtendsController는 View만 반환하도록 되어 있고 따로 Model은 건들이지 않는데요..
이를 테스트에 나타내고 싶었습니다

slimsha2dy · 2024-09-27T17:46:40Z

mvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/AnnotationHandlerMappingTest.java

-        final var response = mock(HttpServletResponse.class);
+    @Nested
+    @DisplayName("핸들러 조회 성공")
+    class HasHandler {


이전 테스트들까지 전부 신경쓰는게 넘나리 멋지네요.
앞으로 몰라리를 《테스트 코드의 여왕》이라 불러도 괜찮을까요?

slimsha2dy

미션을 진행하고 나서 새롭게 리뷰가 아주 조금 추가됐는데 확인 부탁드릴게요!
그리고 현재 어플리케이션을 실행하고 회원 가입 페이지로 이동하면 에러가 뜨는데 이거도 확인해주세욥!!

slimsha2dy · 2024-09-29T15:43:33Z

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

    }

    @Override
    protected void service(final HttpServletRequest request, final HttpServletResponse response) throws ServletException {
        final String requestURI = request.getRequestURI();
        log.debug("Method : {}, Request URI : {}", request.getMethod(), requestURI);

+        final HandlerMapping handlerMapping = handlerMappingRegistry.getHandlerMapping(request);


과제 lms의 인터페이스 예시에서는 HandlerMappingRegistry에서 바로 getHandler를 통해 핸들러를 찾아오는 형태인데, 몰리는 HandlerMapping을 가져온 후 다시 getHandler를 통해 핸들러를 찾아오도록 나눈 이유가 따로 있으신가요?
제 개인적으로는 나누지 않는 편이 좀 더 깔끔할 것 같아서요!

제가 구현했을 당시에는 lms 힌트가 공개 되어있지 않았는데 지금 확인해보니 꽤나 많은 부분이 추가되어 제시되어있네요 😅
당시 구현했을 때 생각은 HandlerMappingRegistry를 HandlerMapping에 대한 일급 컬렉션만으로 생각했었어요.
Handler 자체를 반환할 생각을 못했네요. 저도 제시된 부분이 더 깔끔한 것 같아서 변경하겠습니다. 😁

slimsha2dy · 2024-09-29T16:05:44Z

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


    public DispatcherServlet() {
+        this.handlerMappingRegistry = new HandlerMappingRegistry("com.techcourse");


이것도 사소한 부분이긴 한데 만약 어떤 개발자가 이 서블릿과 저희의 mvc를 통해 애플리케이션을 개발하려 할 때 이 생성자를 찾아와서 패키지명을 바꾸는 것은 쉽지 않을 것 같아요.
몰리는 어떻게 생각하시나요?

저는 현재의 DispatcherServlet이 완전한 프레임워크의 역할인가 라고 생각했을 때 조금은 아니라고 생각이 드는데요.
실제 스프링이라면 @SpringBootApplication이 부착된 클래스를 기준으로 패키지를 인식하여 주입하여 사용자가 직접 수정할 일이 없지만 현재 구조의 구현에서는 어쩔 수 없는 부분이라고 생각해요. 😅

저도 몰리 말씀에 완전 동의합니다!
어떤 방법을 적용해도 스프링 프레임워크처럼 깔끔한 해결책이 되기는 힘들 것 같아요.
그럼에도 제 개인적으로 지금 구조의 단점은 발생할 가능성도 적지 않고 치명적이라고 생각해서 말씀을 드려봤습니다.
다만 몰리의 생각대로 지금 구조에서 개선하기에는 한계가 있으므로 어떻게든 개선하더라도 임시방편의 느낌이 드는 것도 맞는 것 같아요.

slimsha2dy · 2024-09-29T16:21:19Z

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

    }

    @Override
    public void init() {
-        manualHandlerMapping = new ManualHandlerMapping();
-        manualHandlerMapping.initialize();
+        handlerMappingRegistry.add(new ManualHandlerMapping());


HandlerMappingRegistry와 HandlerAdapterRegistry는 각 생성자에서 초기화를 통해 리스트에 HandlerMapping이나 HandlerAdapter를 추가하고 있는 구조인데 ManualHandlerMapping만 따로 여기서 추가하게 되면 관리의 불편함이 생길 수도 있을 것 같아요.
메멘토 몰리의 생각은 어떠신가요

이 부분도 이전 코멘트와 비슷한 답변이 될 것 같은데요. 저는 현재의 DispatcherServlet가 프레임워크와 사용자 역할 사이에 있는, 프레임워크를 사용하는 사용자가 처리해줘야 할 마지노선이라고 생각하고 2단계를 구현했어요.
ManualHandlerMapping은 사용자가 커스텀한 핸들러 매핑이라고 생각해서 HandlerMappingRegistry 내부에서 선언하는 건 조금 어색하다고 느꼈습니다. 🥲

넵 저도 ManualHandlerMapping이 레지스트리 내부에 있는 것은 많이 어색하다고 생각합니다!

다만 레지스트리를 초기화작업하는 로직이 응집되어 있는 편이 좋다고 생각해서 남긴 리뷰였습니다.
이전 리뷰로 남긴 것과는 다른 좀 추가적인 얘기가 될 수 있기는 한데 저는 레지스트리가 어댑터와 핸들러매핑을 사용하기 위한 컬렉션처럼 느껴져서 이를 사용하는 쪽에서 초기화해주는 식으로 구현하는 편이 책임이 분산되지 않아서 일관적이라고 생각했거든요!
이에 대해서는 어떻게 생각하시나요?

응집도 관점에서 볼 수도 있겠군요 👍
구현할 때의 생각은 어노테이션 등을 통해 리플렉션으로 동적으로 작성된 사용자 커스텀 HandlerMapping을 가져올 수 없다면,
커스텀과 프레임워크 제공 HandlerMapping을 Registry에 등록하는 부분이 분리되더라도 사용자가 변경할 수 있는 부분까지만 제한하는 게 맞다고 생각했습니다.

초기화 후에 새로운 HandlerMapping를 추가하는 게 걸리긴 하지만 커스텀 HandlerMapping을 등록하는 부분이기도 하고 DispatcherServlet이기도 하고 또 DispatcherServlet를 초기화할 때 이루어지기 때문에 괜찮지 않을까 생각했습니다 ...ㅎㅎ
~~답변이 되었을지 모르겠네요~~

+) 지금 생각해보니 베이스 패키지를 알면 isAssignableFrom등을 통해 특정 인터페이스의 구현체를 동적으로 찾아낼 수 있을 것 같아요. 그렇게 되면 레지스트리 내부에서 초기화 시에 모든 HandlerMapping을 추가하도록 할 것 같습니다 😁

slimsha2dy

수고하셨어요 메멘토 몰리!
이번 단계는 머지하도록 하겠습니다.
다음 단계도 화이팅입니다~ ㅎㅎ

jminkkk added 19 commits September 23, 2024 21:16

refactor(Junit4TestRunner): 오타 제거

e3c4591

refactor(RequestMappingInfo): RequestMapping에 대한 정보를 가지고 있는 객체 추출 및 부…

c9c01da

…착된 어노테이션으로부터 핸들러 키 조회 역할 부여

refactor(HandlerMapping): HandlerMapping 추상화

6965040

feat(ManualHandlerMapping): 사용자 수동 등록용 매핑이 HandlerMapping 상속받도록 변경

7dd0312

refactor(AnnotationHandlerMappingTest): 불필요한 throw Exception 제거

f8f3fcb

refactor(AnnotationHandlerMapping): HandlerMapping을 상속하도록 변경

efde0e7

refactor(HandlerExecution): 핸들러 메서드 실행 중 오류 발생 시 전파 제거 및 처리하도록 변경

9564350

refactor(HandlerAdapter): 핸들러의 타입별 반환을 ModelAndView로 전환하는 객체 추상화

e131433

refactor(/sample): Annotation, Controller 상속 각 경우 Test용 객체 생성

3c91ba7

refactor(ControllerHandlerAdapter): Controller 상속인 경우 Adapter 구현체 생성

0b5172d

refactor(HandlerAdapterRegistry): 요청 Handler에 맞는 일급 컬렉션 생성

cb85bac

refactor(HandlerMappingRegistry): HandlerMapping 에 대한 일급 컬렉션 생성

2247e9a

refactor(HandlerAdapter): HandlerAdapter 구현체를 싱글톤으로 수정

b699168

refactor(RegisterController): RegisterController를 요구사항과 동일하게 Annotati…

57c62b7

…on으로 변경

refactor(JspView): 렌더링 로직을 더 간결하도록 처리 순서 변경

d903139

refactor(ControllerHandlerAdapterTest): View 테스트 로직 ArgumentCaptor 사용…

65e69f0

…하여 변경

refactor(RegisterController): 어노테이션 기반 핸들러 반환 타입 올바르게 변경

70a2448

refactor(ManualHandlerMappingTest): 요청 처리 Controller 조회 로직 변경

ee39ba3

feat(DispatcherServlet): 두가지 Handler를 모두 처리할 수 있도록 변경

8af8b7d

jminkkk commented Sep 24, 2024

View reviewed changes

slimsha2dy requested changes Sep 27, 2024

View reviewed changes

jminkkk added 5 commits September 29, 2024 16:15

feat(HandlerMapping): 예외 메시지 자세하게 통일

8341179

feat(ControllerHandlerAdapter): 컨트롤러를 실행 시키는 메서드명을 책임이 명확하게 변경

0032d2c

feat(HandlerAdapter, HandlerMapping): 파일 코드 블럭 내의 마지막 줄 개행 제거

f6f39ec

feat(JspView): forward 인지 확인하는 메서드에 메서드 인자 null 체크

c747a64

refactor: 싱글톤 제

1a8435a

slimsha2dy requested changes Sep 29, 2024

View reviewed changes

jminkkk added 3 commits September 30, 2024 02:11

refactor(HandlerMappingRegistry): Handler를 반환하도록 변

ae65f71

refactor(ControllerScanner): Controller 어노테이션이 부착된 클래스를 찾아 반환하는 Scanner

da2ba72

refactor(DispatcherServlet): Handler 찾는 로직 변경

87a1743

refactor(login.jsp): 변경된 api로 통일

d8ed947

slimsha2dy approved these changes Sep 30, 2024

View reviewed changes

slimsha2dy merged commit 113a4e5 into woowacourse:jminkkk Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 - 2단계] 몰리(김지민) 미션 제출합니다. #738

[MVC 구현하기 - 2단계] 몰리(김지민) 미션 제출합니다. #738

jminkkk commented Sep 24, 2024

jminkkk Sep 24, 2024

slimsha2dy Sep 27, 2024

slimsha2dy left a comment

slimsha2dy Sep 27, 2024

jminkkk Sep 29, 2024 •

edited

Loading

slimsha2dy Sep 27, 2024

jminkkk Sep 29, 2024

slimsha2dy Sep 27, 2024

jminkkk Sep 29, 2024

slimsha2dy Sep 27, 2024

slimsha2dy Sep 27, 2024

jminkkk Sep 29, 2024

slimsha2dy Sep 27, 2024

jminkkk Sep 29, 2024

slimsha2dy Sep 27, 2024

slimsha2dy left a comment

slimsha2dy Sep 29, 2024

jminkkk Sep 29, 2024 •

edited

Loading

slimsha2dy Sep 29, 2024

jminkkk Sep 29, 2024 •

edited

Loading

slimsha2dy Sep 30, 2024 •

edited

Loading

slimsha2dy Sep 29, 2024

jminkkk Sep 29, 2024

slimsha2dy Sep 30, 2024

jminkkk Oct 2, 2024 •

edited

Loading

slimsha2dy left a comment

		public class RequestMappingInfo {

		private final RequestMapping requestMapping;


		public class ControllerHandlerAdapter implements HandlerAdapter {

		private static final ControllerHandlerAdapter INSTANCE = new ControllerHandlerAdapter();

		boolean supports(final Object handler);

		ModelAndView handle(final Object handler, final HttpServletRequest request, final HttpServletResponse response);


		public DispatcherServlet() {
		this.handlerMappingRegistry = new HandlerMappingRegistry("com.techcourse");

[MVC 구현하기 - 2단계] 몰리(김지민) 미션 제출합니다. #738

[MVC 구현하기 - 2단계] 몰리(김지민) 미션 제출합니다. #738

Conversation

jminkkk commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slimsha2dy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk 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

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

Choose a reason for hiding this comment

slimsha2dy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jminkkk Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

slimsha2dy Sep 30, 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

jminkkk Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

slimsha2dy left a comment

Choose a reason for hiding this comment

jminkkk Sep 29, 2024 •

edited

Loading

jminkkk Sep 29, 2024 •

edited

Loading

jminkkk Sep 29, 2024 •

edited

Loading

slimsha2dy Sep 30, 2024 •

edited

Loading

jminkkk Oct 2, 2024 •

edited

Loading