[MVC 구현하기 - 2단계] 도이(유도영) 미션 제출합니다. #507

yoondgu · 2023-09-19T14:34:50Z

안녕하세요 제나!!
지난 미션 리뷰 중
컨벤션(개행) 피드백과, 공감가는 내용 #374 (comment) 을 반영했어요!
그 외에 1단계에서 예외 처리 부분도 조금 보완했습니다.
코드 개선에 도움을 주셔서 감사합니다.

2단계에서는 app 패키지와 프레임워크 부분을 분리하는 데 신경쓰려고 했어요.
app 패키지에 있는 DispatcherServletInitializer에서 프레임워크에 필요한 객체들을 전달합니다.
또, 프레임워크 패키지 내에서도

servlet.mvc : 서블릿과 관련된 모든 클래스. 상위 클래스(인터페이스)는 이 패키지에 위치
- servlet.mvc.asis : 그 중, 프레임워크의 Legacy 코드
- servlet.mvc.tobe : 새로 적용하는 @MVC 에 대한 코드

라는 규칙으로 마음대로(?) 구분하며 진행해보았습니다.
그리고 404.jsp, 500.jsp 페이지는 어떻게 연결시키는 게 좋을까 고민하다 ExceptionHandler, ExceptionHandlerAdapter 를 만들어보았습니당.

테스트 커버리지가 꽤나 낮은데 여력이 되면 리뷰 진행하며 추가해보도록 하겠습니다..!!!
이번에도 잘 부탁드려요 🙇‍♀️🙇‍♀️

- Annotation 전용 Controller를 적용하기 전 Object 대신 제네릭으로 Controller 타입 처리

- AnnotatedController는 어노테이션으로 식별하므로 상위 클래스나 인터페이스가 있는 게 불필요해보인다.

yenawee

안녕하세요 도이

2단계 미션 진행하느라 수고하셨습니다!
역시 코드가 깔끔하고 기능도 잘 돌아가고 있네요👏

PR 내용에 있던 패키지 위치 관련해서는 저도 완전 동일한 위치에 클래스들을 위치시켰었어요 (같은 이유로..!)
지금 2단계가 레가시 MVC 와 어노테이션 기반 MVC 를 공존시키는거라 패키지 위치가 더 헷갈리는 것 같은데
3단계 진행하면서 레가시 관련 코드들을 삭제하면 좀 더 길이 보일 거 같다는 생각이 들었습니당

소소한 리뷰들 있어서 리퀘스트 체인지 드려요! 도이의 생각에 맞는 리뷰만 반영해서 리퀘스트 주시면 어프룹&머지해도 될거같습니다!! 화이팅!

yenawee · 2023-09-20T08:06:59Z

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


-public class RegisterController implements Controller {
+@Controller
+public class RegisterController {


어노테이션 기반 컨트롤러에 동일 path로 method 만 GET 인 요청을 처리하는 메서드를 추가해서 올바른 핸들러에서 올바른 메서드를 실행하고 있는지 테스트해봐도 좋을거같아요!!

덕분에 더 확실히 확인해보았습니다~!!

yenawee · 2023-09-20T08:07:41Z

mvc/src/main/java/web/org/springframework/web/bind/annotation/RequestMethod.java

@@ -6,7 +6,6 @@
 public enum RequestMethod {
    GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, TRACE;

-
    public static RequestMethod find(final String name) {


도이 저도 이번 미션을 하면서 처음 알았는데 enum 에는 valueOf() 라고 요 메서드랑 동일행동을 하는 메서드가 정의되어있더라구요!! ㅎㅎ

맞아요 그렇더라구요!!
그래서 저도 valueOf로 대체하려고 했는데,
생각해보니 대소문자 구분, 원하는 예외 발생 과 같은 로직은 직접 해당 클래스 내에서 작성하고 싶어서 그대로 두었습니다!

valueOf를 여기서 쓰고 try~catch로 예외를 전환시켜도 되지만 이게 코드가 더 깔끔하다고 생각이 되어서요....ㅎㅎ

yenawee · 2023-09-20T08:08:28Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/DispatcherServlet.java

+
+    private static final long serialVersionUID = 1L;
+    private static final Logger log = LoggerFactory.getLogger(DispatcherServlet.class);
+    private final transient HandlerMappingRegistry handlerMappingRegistry = new HandlerMappingRegistry();


transient 는 뭐인지 설명해주실수잇나요 ?? (저도 소나린트에서 이렇게 바꾸라고 한거같은데 잘 모르겠어서 놔뒀었어요!!)

저도 소나린트 보고, 뭔지 간단히만 검색해봐서 잘 모르긴 하는데요..ㅎㅎ
제가 이해한 것만 설명드리자면 (이미 아시는 내용일수도..)

DispatcherServlet은 Serializable한 클래스인데요.(최상위 부모 클래스를 보면 Serializable 인터페이스를 구현하고 있음)
이 Serializable 인터페이스는 해당 클래스를 파일에 읽고 쓰거나, 다른 서버와 주고 받는 경우를 위해서 직렬화를 가능하게 해주는 인터페이스라고 합니다.

그래서 sonarlint의 경고는 DispatcherServlet을 직렬화할 때, 해당 클래스의 인스턴스 변수도 직렬화를 할지, 아니면 직렬화 대상에서 제외할지(=transient) 정하기를 권하는 의미로 이해했어요.

해당 변수가 Primitive Type이나 Serializable 인터페이스를 구현하는 객체인 경우 문제가 없지만 그렇지 않은 경우라 발생한 경고인 것 같습니다.
만약 transient가 아니라 같이 직렬화를 하고 싶다면, Map의 value type을 Serializable 하게 바꿔주면 되는 것 같아요..!!

참고 링크 :
https://oingdaddy.tistory.com/451
https://devlog-wjdrbs96.tistory.com/268

근데 그러고보면 transient 대신 아래 방법을 사용하는게 더 나았으려나요 .. 🤔🤔 DispatcherServlet이 직렬화되는 게 정확히 어느 상황인지 모르겠어서 판단하기가 어렵네요..! 무시해도 되는걸까요 ? 제나의 의견도 궁금해요!!

DisaptcherServlet 이 상속 받는 GenericServlet 이 Serilaizable 을 구현하고 있었군용

저도 추가로 찾아보니 DispatcherServlet 이 직렬화되는 경우는 거의 없는 거같아서(개발자가 신경써줘야 할 필요도 없고 )
transient 사용하는 방법이 더 나은거같다고 생각합니다! (실제 역직렬화가 될 가능성 없어보임)

yenawee · 2023-09-20T08:09:24Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/DispatcherServlet.java

+        } catch (Exception exception) {
+            log.error("Exception : {}", exception.getMessage(), exception);
+            if (exceptionHandlerAdapter != null) {
+                exceptionHandlerAdapter.handle(request, response, exception);


exceptionHandler 에서 404, 500 처리해주는거 좋은거같아요! 저도 적용해야겠어요

yenawee · 2023-09-20T08:13:18Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/asis/ControllerHandlerAdapter.java

+        final var controller = (Controller) handler;
+        final var viewName = controller.execute(request, response);
+
+        move(viewName, request, response);


지금 Controller 에서는 String 을 반환해줘야 해서 move() 메서드가 요기로 온거같은데
실제 디스패쳐 서블릿에서는 어댑터가 ModelAndView 반환해주니 DispatcherServlet 에서 반환된 ModelAndView 를 처리해주는 식으로 3단계때 수정해보면 좋을거같아요!

yenawee · 2023-09-20T08:14:02Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/tobe/AnnotationHandlerMapping.java

        final var reflections = new Reflections(basePackage);
-        final var controllers = reflections.getTypesAnnotatedWith(Controller.class);
+        final var controllers = reflections.getTypesAnnotatedWith(
+                context.org.springframework.stereotype.Controller.class


앞에 패키지 명은 static import 되지 않을까여??

yenawee · 2023-09-20T08:25:11Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/DispatcherServlet.java

+    private transient ExceptionHandlerAdapter exceptionHandlerAdapter;
+
+    public DispatcherServlet() {
+        addHandlerMapping(new AnnotationHandlerMapping());


저도 DispatcherServlet 을 mvc 모듈로 옮겨왔는데
AnnotationHandlerMapping() 이나 HandlerExecutionHandlerAdapter() 는 app 모듈의 DispatcherServletInitializer 에서 add 해줬던거같아요

저는 app 모듈에서 mvc 모듈을 알고 있는 것은 괜찮다고 생각해서 그렇게 구현해줬었는데 사실 잘 모르겠어요 레가시 MVC 가 사라지는 3단계에서 좀 더 깔끔해질거같은데 지금 공존시키려니 헷갈리네용..

저도 DispatcherServletInitializer로 app 모듈에서 mvc 모듈을 아는 것은 괜찮다고 생각했어요.
그런데 프레임워크는 프로그램의 제어를 역으로 가져가는 것이라고 생각하면, 이 의존도 없었어야 하나 고민이 드네요..!!
제나가 말씀하신 것처럼, 레가시 MVC가 사라지면 괜찮을 것 같기도 하고요 ㅎㅎ
3단계에서 더 고민해보겠습니다!

yenawee · 2023-09-20T08:29:51Z

mvc/src/main/java/webmvc/org/springframework/web/servlet/mvc/DispatcherServlet.java

+    }
+
+    @Override
+    public void init() {


init() 에서 handlerMappingRegistry 를 init() 하는거보다 handlerMapping 을 add 하는 로직(HandlerMappingRegistry) 에서 handlerMapping 을 init 한번 해주고 add 해주는 방법은 어떠신가여 ??

근데 지금 init() 메서드가 호출이 안되는데 handlerMapping 들이 다 init() 이 되고있네요,, 왠지 잘 모르겠네욥😵‍💫

DispatcherServlet의 init 메서드가 잘 동작하고 싶은지 보고 싶은 마음에 지금처럼 했었는데요..ㅎㅎ
근데 제나 말씀처럼 더 하위 클래스(?)인 Registry가 직접 init을 하도록 하는 게 더 좋은 것 같아요!
DispatcherServlet이 신경 쓸 일이 줄어드니깐요
감사합니다 반영할게요 !!

--
DispatcherServletInitializer의 onStartUp 메서드에서, ServletContext에 DispatcherServlet을 add 할 때 init() 메서드가 호출됩니다!

오..! add 할때 호출되는군요!!
감사합니다

아 제가 문장을 부정확하게 쓴 것 같아 추가 코멘트 드려요 ㅎㅎ
add 할 때 정확히 init() 메서드가 호출된다는 뜻은 아니고, add 해줌으로써 호출된다는 뜻이었습니다..!!
디버깅을 통해 add, onStartUp 이후 init() 메서드가 호출되는 걸 확인해서, 서블릿 초기화 중 어딘가에서 호출해주나보다 정도로만 파악하고 있습니다 ㅠㅜ

정확히 어느 부분에서 호출되는지는 공식 문서에서 빠르게 파악하기는 어렵네용..

sonarcloud · 2023-09-23T04:22:05Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

45.5% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

yoondgu · 2023-09-23T04:23:15Z

남겨주신 코멘트들에 의견 남기고, 반영했습니다!
저도 제나의 의견이 궁금한 부분이 있는데 시간 되실 때 같이 얘기해봐요 ~! #507 (comment)
이번 단계에도 좋은 리뷰 감사해요 제나 !!

yenawee · 2023-09-23T09:22:53Z

안녕하세요 도이!

리뷰 반영 확인했습니당 👍👍
제가 여쭤봤던 부분에 대한 제 생각은 #507 (comment) 요기 남겨놨어요

2단계 미션하느라 고생많았습니다!!
3단계에서 봐요!!

yoondgu added 11 commits September 18, 2023 23:58

refactor: 1단계 예외처리 보완, 인터페이스 적용 전 제네릭 처리

8272518

- Annotation 전용 Controller를 적용하기 전 Object 대신 제네릭으로 Controller 타입 처리

docs: 2단계를 위한 요구사항 작성

bb0b5dd

refactor: HandlerMapping 제너릭 삭제, Object 적용

53564ac

- AnnotatedController는 어노테이션으로 식별하므로 상위 클래스나 인터페이스가 있는 게 불필요해보인다.

refactor: RegisterController를 어노테이션 기반으로 변경

c0bb78a

feat: DispatcherServlet에 AnnotationHandlerMapping 적용

8b7bb35

feat: JspView 동작하도록 구현

4931104

refactor: DispatcherServlet 패키지를 프레임워크 페키지로 이동

d98d5cb

refactor: HandlerMapping, HandlerAdapter 추상화

483e605

refactor: 예외처리 DispatcherServlet에서 처리하도록 통일

463d67d

refactor: 특정 예외에 대해 응답을 처리하는 ExceptionHandler 구현

7c7f9e0

refactor: Handler 관련 클래스 분리

a88f028

yoondgu requested a review from yenawee September 19, 2023 14:34

yoondgu self-assigned this Sep 19, 2023

yoondgu added 5 commits September 19, 2023 23:35

refactor: 사용하지 않는 메서드 삭제

1664e2c

refactor: 불필요한 Serializable 삭제

7939463

refactor: 개행 삭제

f967e24

refactor: 404 app 패키지 예외 핸들링 구현

db042ab

test: Handler 관련 테스트 작성

a54283d

yoondgu marked this pull request as ready for review September 19, 2023 15:52

chore: 로그 위치 수정

16aaccb

yenawee requested changes Sep 20, 2023

View reviewed changes

yoondgu added 2 commits September 23, 2023 13:14

refactor: Class static import로 수정

916c3a9

refactor: HandlerMappingRegistry 로 초기화 책임 이관

7990790

yenawee merged commit f3558d6 into woowacourse:yoondgu Sep 23, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 - 2단계] 도이(유도영) 미션 제출합니다. #507

[MVC 구현하기 - 2단계] 도이(유도영) 미션 제출합니다. #507

yoondgu commented Sep 19, 2023 •

edited

Loading

yenawee left a comment

yenawee Sep 20, 2023

yoondgu Sep 23, 2023

yenawee Sep 20, 2023

yoondgu Sep 23, 2023

yenawee Sep 20, 2023

yoondgu Sep 23, 2023 •

edited

Loading

yoondgu Sep 23, 2023

yenawee Sep 23, 2023

yenawee Sep 20, 2023

yenawee Sep 20, 2023

yenawee Sep 20, 2023

yenawee Sep 20, 2023

yoondgu Sep 23, 2023 •

edited

Loading

yenawee Sep 20, 2023

yoondgu Sep 23, 2023

yenawee Sep 23, 2023

yoondgu Sep 23, 2023

sonarcloud bot commented Sep 23, 2023

yoondgu commented Sep 23, 2023

yenawee commented Sep 23, 2023

[MVC 구현하기 - 2단계] 도이(유도영) 미션 제출합니다. #507

[MVC 구현하기 - 2단계] 도이(유도영) 미션 제출합니다. #507

Conversation

yoondgu commented Sep 19, 2023 • edited Loading

yenawee 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

Choose a reason for hiding this comment

yoondgu Sep 23, 2023 • 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

yoondgu Sep 23, 2023 • 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

sonarcloud bot commented Sep 23, 2023

yoondgu commented Sep 23, 2023

yenawee commented Sep 23, 2023

yoondgu commented Sep 19, 2023 •

edited

Loading

yoondgu Sep 23, 2023 •

edited

Loading

yoondgu Sep 23, 2023 •

edited

Loading