[MVC 구현하기 1단계] 하디(전동혁) 미션 제출합니다. #352

jundonghyuk · 2023-09-13T04:01:22Z

안녕하세요 폴로 하디입니다.

구현하는데 고민이 많았습니다.

ReflectionUtils는 사용하려 했으나 제가 봤을 때 뭔가 .. 더 가독성이 떨어져 보여서 사용하지 않았습니다.
ControllerScanner를 통해 어노테이션 클래스 끌어오기
기본 생성자가 존재하고 필드가 없는 @Controller 어노테이션이 붙은 클래스만 생성가능

인자가 있으면 어떤 값으로 초기화해야할지 몰라서 일단은 이렇게 구현했습니다 !
감사합니다

green-kong

안녕하세요 하디!
평소에 하디의 코드를 볼일이 없었어서 몰랐는데,
굉장히 세심하게 코드를 작성하시네요!!
제가 생각하지 못한 부분들이 하디의 코드에선 처리되고 있는 것을 보고 많이 반성하고 배울 수 있었습니다!

약간의 코멘트 남겨두었는데요.
하디의 의견이 궁금해서 Request Changes로 해둘게요!
의견만 남겨주시고, 반영은 2단계 때 하셔도 무방합니다~
수고많으셨어요!

green-kong · 2023-09-14T00:16:04Z

.../test/java/webmvc/org/springframework/web/servlet/mvc/tobe/AnnotationHandlerMappingTest.java

+    @Test
+    void givenWrongUrl_whenGetHandler_thenReturnNull()  {
+        final var request = mock(HttpServletRequest.class);
+        String wrongUrl = "/wrong-url";
+
+        when(request.getAttribute("id")).thenReturn("gugu");
+        when(request.getRequestURI()).thenReturn(wrongUrl);
+        when(request.getMethod()).thenReturn("POST");
+
+        final var handlerExecution = (HandlerExecution) handlerMapping.getHandler(request);
+        Assertions.assertThat(handlerExecution).isNull();
+    }
+
+    @Test
+    void givenWrongRequestMethod_whenGetHandler_thenReturnNull() {
+        final var request = mock(HttpServletRequest.class);
+        String wrongRequestMethod = "PATCH";
+
+        when(request.getAttribute("id")).thenReturn("gugu");
+        when(request.getRequestURI()).thenReturn("/post-test");
+        when(request.getMethod()).thenReturn(wrongRequestMethod);
+
+        final var handlerExecution = (HandlerExecution) handlerMapping.getHandler(request);
+        Assertions.assertThat(handlerExecution).isNull();
+    }


잘못된 url 또는 method로 들어온 요청에 대한 테스트까지 꼼꼼하시네요!

green-kong · 2023-09-14T00:26:23Z

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

+        for (Object base : basePackage) {
+            registerFromBasePackage((String) base);
+        }


basePackage가 Object[] 의 형태라 for문을 돌리신건가요?
Reflections의 생성자를 확인해보면 Object 가변인자를 받는 생성자도 존재하고있습니다.
저도 대충 훓어 본거지만, basePackage에 있는 모든 값들에 대해 스캔할 수 있는 Reflections가 생성되네요~

public Reflections(Object... params) { this(ConfigurationBuilder.build(params)); }

감사합니다. 가변인자를 받고 있었네요 ..!

열어보지 않아서 몰랐습니다. 수정 하겠습니다 !

green-kong · 2023-09-14T00:30:30Z

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

+        } catch (NoSuchMethodException e) {
+            log.error("NoArgConstructor doesn't exist.");
+        } catch (SecurityException e) {
+            log.error("Check permission");
+        } catch (IllegalAccessException e) {
+            log.error("Constructor is not accessible.");
+        } catch (IllegalArgumentException e) {
+            log.error("Type of Arguments doesn't matched.");
+        } catch (InstantiationException e) {
+            log.error("The instance is abstract class.");
+        } catch (InvocationTargetException e) {
+            log.error("Exception occurs during constructing.");
+        } catch (ExceptionInInitializerError error) {
+            log.error("Initializing fails.");
+        }
+        throw new IllegalArgumentException("Getting instance using constructor fails.");


Exception으로 catch한 poorExceptionHandling을 한 제 자신을 반성하게 됩니다...👍

green-kong · 2023-09-14T00:55:24Z

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

+    public Map<Class<?>, Object> getControllers() {
+        Set<Class<?>> controllers = reflections.getTypesAnnotatedWith(Controller.class);
+        return instantiateControllers(controllers);
+    }


Suggested change

public Map<Class<?>, Object> getControllers() {

Set<Class<?>> controllers = reflections.getTypesAnnotatedWith(Controller.class);

return instantiateControllers(controllers);

}

public Map<Class<?>, Object> getControllers() {

Set<Class<?>> controllers = reflections.getTypesAnnotatedWith(Controller.class);

return controllers.stream()

.collect(Collectors.toMap(Function.identity(), this::generateInstance));

}

private Object generateInstance(final Class<?> controller) {

try {

return controller.getDeclaredConstructor().newInstance();

} catch (NoSuchMethodException e) {

log.error("NoArgConstructor doesn't exist.");

} catch (SecurityException e) {

log.error("Check permission");

} catch (IllegalAccessException e) {

log.error("Constructor is not accessible.");

} catch (IllegalArgumentException e) {

log.error("Type of Arguments doesn't matched.");

} catch (InstantiationException e) {

log.error("The instance is abstract class.");

} catch (InvocationTargetException e) {

log.error("Exception occurs during constructing.");

} catch (ExceptionInInitializerError error) {

log.error("Initializing fails.");

}

throw new IllegalArgumentException("Getting instance using constructor fails.");

}

스트림을 이용해서 이렇게 변경할 수도 있을 것 같아요!

개인적으로 저는 예외가 발생할 수 있는 부분만 따로 메서드를 분리해두면 익셉션이 발생했을때 어떤 라인에서 캐치문으로 빠졌는지 디버깅이 편해져서 요런식으로 분리하는 것을 선호합니다~ 요런 방법도 있다는 걸 참고만 해주세욯ㅎ(개인 취향)

폴로 스트림 활용을 정말 잘하시네요 ㅎㅎ 많이 배웁니다 !

인스턴스만 생성해주는 generateInstance만 따로 빼놔서 오히려 가독성이 더 좋아지는것 같아요 반영하게습니다 ㅎㅎ

green-kong · 2023-09-14T01:32:56Z

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

+            RequestMapping requestMapping = method.getAnnotation(RequestMapping.class);
+            String url = requestMapping.value();


method.getAnnotation()의 내부를 확인해보시면,

public <T extends Annotation> T getAnnotation(Class<T> annotationClass) { Objects.requireNonNull(annotationClass); return annotationClass.cast(declaredAnnotations().get(annotationClass)); }

요런 식으로 구현되어있어요.
declaredAnnotation()의 반환값이 Map자료구조 인데요.
Map에서 get을 해오는 방식이라, null이 반환 될수도 있을것 같네요!
method가 특정 어노테이션을 포함하고 있는지 체크할 수 있는 메서드가 있는데(isAnnotationPresent()) 이걸 한번 사용해보시면 어떨까요?
근데 사실 isAnnotationPresent()가 내부적으로 getAnnotation을 사용하고 있어서, null체크만 해주어도 될것같긴 하네요~

private List<Method> getRequestMappingMethods(Class<?> handler) { return Arrays.stream(handler.getMethods()) .filter(method -> method.isAnnotationPresent(RequestMapping.class)) .collect(Collectors.toList()); }

현재 AnnotationHandlerMapping.java의 58 ~ 62번 라인입니다 !

메서드 리스트를 가져올 때 RequestMapping이 달려있는 것만 가져오고 있는데요 !

혹시 이 로직을 말씀하신걸까요 ?

앗 제가 이부분을 놓쳤네요 죄송합니다죄송합니다죄송합니다

green-kong · 2023-09-14T01:40:11Z

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

+    private boolean isInvalidRequestMapping(String url, RequestMethod[] requestMethods) {
+        return url.isBlank() || requestMethods.length == 0;
    }


@RequestMapping 의 value와 method의 default가 각각 ""(빈스트링) 빈배열이라 이런 검증을 추가 해주신 것 같아요!
개인적인 의견인데, value가 빈스트링이거나, method가 빈배열이라면 예외가 발생해야 되는것 아닐까요?
흠. 얘네개 왜 default를 이렇게 설정했는지 궁금해지네요..

그건 그렇고 이런부분까지 세심하게 검증을 하시는게 놀랍네요! 전 안했거든요ㅋㅋ👍

공감합니다 ..ㅋㅋㅋㅋ 사실 어노테이션을 열어보고 디폴트 값이 설정되어 있길래 그거로 잡았습니다 ㅎㅅㅎ

green-kong · 2023-09-14T01:40:53Z

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

+                HandlerKey handlerKey = new HandlerKey(url, requestMethod);
+                HandlerExecution handlerExecution = new HandlerExecution(instance, method);
+                handlerExecutions.put(handlerKey, handlerExecution);
+                log.info("uri = {}, method = {}, name = {} is mapped!", url, requestMethod.name(), method.getName());


세심한 로그👍

green-kong · 2023-09-14T01:47:00Z

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

+import java.util.Map;
+import java.util.Set;
+
+public class ControllerScanner {


getControllers() 를 통해서 @controller 어노테이션이 붙은 클래스를 가져오는 역할을 하네요!
이번 미션에서는 컨트롤러만 스캔을 하지만, 원래라면 다른 어노테이션에 대해서도 스캔이 필요할 것 같아요(@service, @repository, 등등..)
모든 어노테이션 마다 Scanner클래스를 만들기는 힘들 것 같으니,
이 클래스를 어노테이션.class를 인자로 받아 인자로 받은 어노테이션이 붙은 클래스들을 스캔할 수 있는 클래스로 변경해보면 범용적으로 쓸 수 있는 클래스가 될것 같은데,
어떻게 생각하시나요??

너무 좋습니다 ! 특정 어노테이션이 달린 클래스나 인터페이스만 가져와보도록 수정해보겠습니다 ~!

👍

green-kong

부족한 리뷰였을텐데 꼼꼼히 반영해주셔서 감사합니다!
Scanner가 범용적으로 사용될 수 있을 것 같아서 아주 탐나네요.
제 커밋으로 훔쳐가도록 하겠습니다 ㅋㅋ

그리고 이제 본건데 브랜치가 realStep1이네요 ㅋㅋㅋ 수고 많으셨습니다
realStep1은 merge 하겠습니다!
2단계에서 만나용~

jundonghyuk added 12 commits September 12, 2023 15:31

test: Junit4Test 작성

be2c663

test: Junit4Test 작성

ca01f32

test: ReflectionTest 작성

3662225

test: ReflectionsTest 작성

cbb7e7d

test: ServletTest 작성

de091a5

test: CharacterEncodingFilter 변경

31b2626

feat: HandlerExecution 구현

d585ffa

feat: AnnotationHandlerMapping 기능 구현

a452ee6

test: AnnotationHandlerMappingTest 테스트 추가

104e66f

feat: ControllerScanner 기능 구현

1d92d92

refactor: ControllerScanner 이용하여 개선

ab974ba

refactor: 불필요한 import 삭제

b7f7073

green-kong requested changes Sep 14, 2023

View reviewed changes

jundonghyuk added 3 commits September 14, 2023 21:16

refactor: Reflections 생성시 basePackages를 가변인자로 전달

91e7103

refactor: stream을 활용한 인스턴스 생성

b7db87b

refactor: ControllerScanner를 AnnotationScanner로 범용적이게 변경

043fc13

green-kong approved these changes Sep 15, 2023

View reviewed changes

green-kong merged commit ae007c2 into woowacourse:jundonghyuk Sep 15, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MVC 구현하기 1단계] 하디(전동혁) 미션 제출합니다. #352

[MVC 구현하기 1단계] 하디(전동혁) 미션 제출합니다. #352

jundonghyuk commented Sep 13, 2023

green-kong left a comment

green-kong Sep 14, 2023

green-kong Sep 14, 2023

jundonghyuk Sep 14, 2023

green-kong Sep 14, 2023

green-kong Sep 14, 2023

jundonghyuk Sep 14, 2023

green-kong Sep 14, 2023

jundonghyuk Sep 14, 2023

green-kong Sep 15, 2023

green-kong Sep 14, 2023

jundonghyuk Sep 14, 2023

green-kong Sep 14, 2023

green-kong Sep 14, 2023

jundonghyuk Sep 14, 2023

green-kong left a comment •

edited

Loading

		RequestMapping requestMapping = method.getAnnotation(RequestMapping.class);
		String url = requestMapping.value();

[MVC 구현하기 1단계] 하디(전동혁) 미션 제출합니다. #352

[MVC 구현하기 1단계] 하디(전동혁) 미션 제출합니다. #352

Conversation

jundonghyuk commented Sep 13, 2023

green-kong 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

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

green-kong left a comment • edited Loading

Choose a reason for hiding this comment

green-kong left a comment •

edited

Loading