[MVC 구현하기 - 3단계] 페드로(류형욱) 미션 제출합니다. #804
Conversation
There was a problem hiding this comment.
안녕하세요 페드로 🙌
어느덧 이번 미션 마지막 PR이네요 🥲
DispatcherServletInitializer도 mvc 모듈로 빼고 싶었는데, 구조 상 컨트롤러 클래스 자동 스캔 타겟 패키지 정보를 전달해 줄 수 있는 게이트웨이 역할의 객체가 하나는 app 패키지에 남아 있어야 하겠더라구요.
mvc 모듈 내의 SpringServletContainerInitializer의 구현을 보면 WebApplicationInitializer 클래스들을 전달받아 인스턴스화가 진행되는 것을 확인할 수 있는데, Tomcat이 app에서 실행되는 이상 app 내부의 DispatcherServletInitializer는 필수불가결한 요소라고 판단했습니다.
제가 알기로 스프링 부트는 페드로가 말씀해주신 것처럼 동작하고, 스프링에서는 애플리케이션 설정을 하려면 개발자가 직접 xml 파일 또는 WebApplicationInitializer의 구현체를 작성해야 하는 것으로 알고 있어요!
그래서 저는 애플리케이션 설정을 위해 직접 작성한 구현체 DispatcherServletInitializer가 app 에 위치하는 건 자연스럽다고 느껴지는 것 같아요. (오히려 mvc 모듈에 있었다면 오잉 스러웠을 것 같기도...?)
제가 페드로의 의도와 다르게 이해한 부분 있다면 편하게 말씀해주세요 😁
이번 PR도 파이팅하면서 마지막까지 달려봅시다~ 🔥
| } | ||
|
|
||
| // @Service, @Repository 애노테이션이 붙은 클래스들을 모두 찾아 반환 | ||
| public static Set<Class<?>> getAllClassesInPackage(String packageName, List<Class<? extends Annotation>> annotations) { |
There was a problem hiding this comment.
아이고 인자로 받은 annotations에 대한 스트림을 만들었어야 했는데 잘못되어 있었네요🥲
메서드명은 오버로딩을 활용하기 위해 두 메서드를 통일해 둔 것이었어요.
getAllClassesInPackage(basePackage):@Service,@Repository만 스캔getAllClassesInPackage(basePackage, targetAnnotations): 인자로 받은 어노테이션 타입을 전부 스캔
메서드에 대한 기본 스캔 동작을 지정해주기 위함이었는데 조금 어색한가요?
There was a problem hiding this comment.
ClassPathScanner가 반드시 어노테이션만 스캔할까 라는 생각이 들었는데 어쨋든 ClassPathScanner의 역할이 특정 조건을 만족하는 클래스들을 찾는 것이고, 어노테이션 기반 스캔은 그 중 하나의 방법일 뿐이니,,, 오히려 오버로딩이 좋은 것 같네요
굿 넘어가시죠 🔥
mvc/src/main/java/com/interface21/webmvc/servlet/mvc/legacy/Controller.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private boolean hasSingleEntry(Map<String, ?> model) { | ||
| return model.size() == 1; |
There was a problem hiding this comment.
개인적으로 size 관련 검증 (특히 0이나 1)에서는 관련 검증 메서드를 추출하고 메서드명을 잘 부여하는 것 만으로도 충분한 가독성을 챙길 수 있다고 생각해서 상수 추출은 잘 하지 않는 편이예요.
private static final int ONE = 1;
...
private boolean hasSingleEntry(Map<String, ?> model) {
return model.size() == ONE;이같은 구현은 오히려 가독성을 해칠 뿐더러 의미 없는 static 변수만 늘어난다고 생각하는데 몰리의 생각은 어떠신가요?
There was a problem hiding this comment.
"model에 데이터가 1개면 값을 그대로 반환하고 2개 이상이면 Map 형태 그대로 JSON으로 변환해서 반환한다."
개인적으로 저는 위 요구사항에서 "?개면 값을 그대로 반환한다" 라는 부분이 인상 깊었는데요.
스프링이나 웹 프레임워크를 사용했을 때 Json이 key-value 형식이 아니라 값만 반환하는 것을 본적이 없어서 신기한 요구사항이었다고 생각했습니다. 그래서 단순히 1이라는 숫자를 상수로 빼라라는 것이 아니라, 우리 요구사항에만 있는 숫자에 조금 더 의미를 담아볼 수 있지 않을까 하고 드린 리뷰이긴 합니다.
private static final int VALUE_ONLY_COUNT = 1;
private boolean isValueOnly(Map<String, ?> model) {
return model.size() == VALUE_ONLY_COUNT;요론 것처럼요...ㅎㅎ
그런데 값을 그대로 반환하는데 조건이 1에서 변경이 될 수 있나를 생각해보면 아닌 것 같기도 해서 이 부분은 페드로의 선택에 맡기겠습니다 😁
| @@ -22,8 +18,8 @@ public class DispatcherServlet extends HttpServlet { | |||
| private final HandlerMappingRegistry handlerMappingRegistry; | |||
| private final HandlerAdapterRegistry handlerAdapterRegistry; | |||
|
|
|||
| public DispatcherServlet() { | |||
| handlerMappingRegistry = new HandlerMappingRegistry(); | |||
| public DispatcherServlet(String basePackage) { | |||
There was a problem hiding this comment.
DispatcherServlet 생성자가 basePackage였군요..!
There was a problem hiding this comment.
이래서 눈치 빠른 애들은...
ㅋㅋㅋㅋㅋ저도 작성하면서 조금 찜찜했던 부분이었는데요.. HandlerMappingRegistry가 지연 초기화될때 자동으로 스캔한 컨트롤러들이 등록되니까 HandlerMappingRegistry가 basePackage를 가지는 것은 자연스러운데, 문제는 app 에서 이곳으로 정보를 넘겨줄 방법이 DispatcherServlet의 생성자 말고는 떠오르지 않았었어요.
조금 더 찾아 보니 ServletContext를 통해 전달하는 것이 더 자연스러울 것 같아 변경하였습니다🙂
좋은 인사이트 감사해요👍
|
|
||
| @Override | ||
| public void onStartup(final ServletContext servletContext) { | ||
| final var dispatcherServlet = new DispatcherServlet(); | ||
| dispatcherServlet.addHandlerMapping(new ManualHandlerMapping()); | ||
| final var dispatcherServlet = new DispatcherServlet(CONTROLLER_SCAN_BASE); |
There was a problem hiding this comment.
DispatcherServlet이 애플리케이션의 베이스 패키지가 아닌 컨트롤러 패키지를 주입하도록 한 이유가 궁금해요!
요렇게 되면 애플리케이션의 베이스인 com.techcourse 하위이지만 /controller 하위는 아닌 @controller 클래스는 인식이 불가능 할 것 같은데, com.techcourse.controller로 컨트롤러의 위치를 제한한 이유가 따로 있을까요?
There was a problem hiding this comment.
현재 구현 상 /controller 하위가 아닌 컨트롤러 클래스들이 없기 때문입니다.
DispatcherServletInitializer도 app 모듈에 위치하는 이상 사용자의 관리 대상인데, app 모듈의 개발을 진행하면서 컨트롤러를 /controller 패키지 하위에 두도록 컨벤션을 정했다면 최적화 관점에서 봤을 때 굳이 다른 패키지를 스캔할 이유가 없다고 생각했어요.
만약 패키지 구조에 변경이 생겨 다른 패키지에 컨트롤러 클래스를 둬야 할 경우가 생겼다고 하더라도, 사용자가 얼마든지 베이스 패키지를 수정할 수 있으므로 문제가 되지 않는다고 판단했습니다.
There was a problem hiding this comment.
com.techcourse 까지는 동적으로 찾도록 변경해 두었어요!
There was a problem hiding this comment.
사용자가 성능을 고려해서 베이스 패키지를 직접 컨트롤러 패키지로 작성한 것을 의도하셨다면 납득이 가네요 👍
| @@ -1,4 +1,4 @@ | |||
| package com.interface21.webmvc.servlet.mvc.tobe; | |||
| package com.interface21.webmvc.servlet.mvc.mapping; | |||
There was a problem hiding this comment.
HandlerExecutionHandlerAdapter를 포함해서, /mapping에 위치한 객체들은 어떤 기준으로 패키지를 구분했는지 궁금해요 👀
There was a problem hiding this comment.
HandlerMapping 인터페이스의 유일한 구현체인 AnnotationHandlerMapping과, 해당 클래스가 동작하기 위해 필요한 클래스들을 모아 두었어요.
HandlerExecutionHandlerAdapter의 경우 HandlerMapping과 직접적인 관계는 없지만, HandlerExecution이 /mapping 패키지에 위치하므로, 어댑터도 동일한 곳에 있는 것이 자연스럽다고 생각했습니다!
app/src/main/java/com/techcourse/controller/LoginController.java
Outdated
Show resolved
Hide resolved
몰리 안녕하세요~👋 주말은 잘 보내고 계신가요ㅋㅋㅋ
3단계는
JSON응답을 지원할 수 있는 뷰를 구현하고, 기존의Controller인터페이스 기반 레거시 컨트롤러들을 제거하는 비교적 간단한(?) 단계였어요.제거 단계에서 기존의 수동 매핑이 필요했던 부분들이 없어지면서 자연스럽게 사용자의 명시적인 컨트롤러 매핑에 대한 우선순위 부여가 무의미해져서, 지난번에 리뷰 주셨던
HandlerMapping자체의 지연 초기화 로직은 제거하였습니다. 없어지니 좀 더 깔끔한 느낌도 들고 좋네요🤣마지막 단계에서 드디어
DispatcherServlet이mvc모듈로 내려오고, 패키지 구조가 조금 정리되었어요.리팩토링 위주였던 단계인 만큼 기존의
asis패키지에 존재하던 코드들을 완전히 삭제하기보다는 남겨 두는 것이 의미가 있을 것 같아,@Deprecated로 마킹해 두고 제거하지는 않았습니다.보기 불편하시면 해당 어노테이션으로 검색해서 마킹된 클래스만 제거해 주시면 될 거예요.
DispatcherServletInitializer도mvc모듈로 빼고 싶었는데, 구조 상 컨트롤러 클래스 자동 스캔 타겟 패키지 정보를 전달해 줄 수 있는 게이트웨이 역할의 객체가 하나는app패키지에 남아 있어야 하겠더라구요.mvc모듈 내의SpringServletContainerInitializer의 구현을 보면WebApplicationInitializer클래스들을 전달받아 인스턴스화가 진행되는 것을 확인할 수 있는데, Tomcat이app에서 실행되는 이상app내부의DispatcherServletInitializer는 필수불가결한 요소라고 판단했습니다.스프링부트의 경우 프레임워크에 톰캣을 내장하고 있고, 사용자 모듈 내에
@SpringBootApplication을 명시한 후SpringBootApplication.run(RealApplication.class)와 같이 부스스트랩하기 위한 기반 클래스정보를 넘겨주기 때문에 컴포넌트 스캔 방식으로 외부 모듈의 컨트롤러들을 프레임워크단으로 등록할 수 있는 것으로 이해했어요.혹시 제가 잘못 이해하고 있는 부분이 있다면 알려 주시면 감사하겠습니다🙃 이번 리뷰도 잘 부탁드릴게요!