Skip to content

Conversation

geoje
Copy link
Member

@geoje geoje commented Sep 20, 2024

안녕하세요 미아!
예전 수업 때 낙낙 크루까지 세 명 이서 토론한 이후로 얘기해본 적이 없었네요!
그 때 동시성 테스트 하셨다 했던게 정말 인상 깊게 남아있었는데 이전 톰캣 미션 마지막 단계에서 동시성 테스트 해보려 하니 정말 어렵더군요... 😥

구현 사항은 아래와 같습니다!

0. 학습 테스트

  • 여러 Reflection 관련 클래스와 메서드를 사용하여 Class Method Parameter 등의 타입에 대해 학습하였습니다.

1. @MVC Framework 테스트 통과하기

  • AnnotationHandlerMappingTest 의 제공된 2개의 테스트를 통과하였습니다.
    • AnnotationHandlerMappingReflection 을 통해 구현하였습니다.
  • 동일한 요청(Endpoint 와 Method 가 동일) 처리하는 2개의 핸들러가 등록 될 시 (최초 initialize() 때) 예외가 발생합니다.
  • 주어진 상황으로는 모든 컨트롤러의 파라미터에 requestresponse 2개의 인자라 필수로 요구되는데 이를 동적으로 대응하기 위해 HandlerExecution.handle 메서드를 이에 맞도록 구현하였습니다.

2. JspView 클래스를 구현한다.

  • DispatcherServlet 에 정답이 거의 나와있어 이를 기반으로 redirect 또는 forward 로 구현하였습니다!
  • 서블릿에 대한 지식이 부족했어서 처음에 갈피를 못 잡다가 Spring Boot DispatcherServletView Resolver 쪽을 공부한 후 뷰와 모델에 대해 인지를 하니 어떤 것을 해야 할지 알게 되었습니다.

레벨4 일정 상 상당히 바쁘실텐데 시간날 때 리뷰 부탁드립니다!
감사합니다!! 🤗

Copy link

@jongmee jongmee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 새양~! 오랜만이예요 ☺️ ㅋㅋㅋㅋ 그걸 아직도 기억하신다니 감동 🥹

요구사항보다 디테일하게 구현해주시고 정말 대단합니다 ! 테스트도 아주 꼼꼼히 작성해주셔서 코드 이해에도 도움이 됐어요. 새양의 의견이 궁금한 점들 위주로 리뷰 남겼습니다 🙇🏻‍♀️

(학습 테스트도 확인했습니다! 다 잘 작성해주신 것 같아서 특별한 리뷰는 남기지 않았습니다)


public ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response) throws Exception {
return null;
List<Object> preparedArgs = List.of(request, response);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 리스트는 클래스 정보(getClass)만을 사용하는 것 같은데 아예 상수(static final)로 놓는 것도 코드 이해에 도움이 될 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗.. 혹시 static final 로 어떻게 놓는 건지 알려주실 수 있나요?
제가 작성할 때 이 리스트의 의미는 메서드에 입력받은 모든 파라미터들 이었습니다!
조금 더 확장해서 @RequestMapping 어노테이션이 있는 메서드의 요구하는 파라미터에 DI 를 해주기 위한 것들을 담아두는 리스트 였습니다!

메서드 내 지역변수로 밖에 존재할 수 없을 것 같다는 생각이 들었어요.. 😓

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpServletRequest.class랑 HttpServletResponse.class를 가진 리스트를 상수로 두면 되지 않을까? 싶었는데 동적으로 파라미터를 바꾸는 의도였다면 굿입니다 👍

public HandlerExecution(Method method, Class<?> controllerClass)
throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
this.method = method;
this.controllerInstance = controllerClass.getConstructor().newInstance();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HandlerExecution이 reflection으로 컨트롤러 인스턴스를 만드는 책임을 가지는게 적절할까요? 새양의 의견이 궁금합니다 👀

Copy link
Member Author

@geoje geoje Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

적절하지 않습니다! 컴포넌트 스캐닝을 하여 @Controller 어노테이션을 붙은 녀석들을 인스턴스로 생성하여 어떠한 공간속에 저장해둔 뒤 이 HandlerExecution 은 단순히 requestresponse 등으로 핸들러를 실행시켜주어야 한다고 생각합니다!

하지만 이 미션은 스프링을 구현하는게 아니다 보니 그런 객체 관리까지 신경쓰지 않았습니다. 😅

return new HandlerExecution(method, clazz);
} catch (Exception e) {
log.error("Failed to find HandlerExecution for class {}", clazz, e);
return null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러 클래스에 public 생성자가 없는 등의 오류는 예외를 발생시켜야 하지 않을까요? (validateDuplicated 처럼요!)

프레임워크를 사용하는 사용자가 오류를 인지해야 할 것 같아요 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다! 해당 부분은 앞에 catch 문을 하나 추가하여 IllegalArgumentException 을 발생시킨 뒤 테스트 코드까지 작성해두었습니다!

이 부분은 TDD 하려고 private 생성자를 가지지는 클래스를 test 쪽에 만들었다가 깜빡하고 안했더군요 😅

@DisplayName("컨트롤러의 메서드가 요구하는 파라미터를 동적으로 대응하여 의존성을 주입한다.")
void passArgumentsWithRequiredParameters() throws Exception {
joinPathOfControllerAndMethod();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 테스트는 아래 테스트를 호출만 하고 있는데 어떤 역할을 하나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

당시 DisplayName 에 적힌 부분을 어떻게 테스트 해야 할지 막막해서 그냥 호출하고 예외가 발생하지 않는 것으로 간주했는데 이 부분이 조금 어색한 것 같아요!

단위적으로 하는 부분인 HandlerExecutionTest 를 따로 만들어 Reflection 으로 메서드 호출 시점에 예외가 발생하지 않는지로 검증하였습니다!

import com.interface21.webmvc.servlet.ModelAndView;

@Controller
@RequestMapping("/api")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분까지 구현하셨다니 디테일 👍👍👍

@geoje
Copy link
Member Author

geoje commented Sep 21, 2024

미아! 안녕하세요~
코멘트 달아주신 부분 몇 가지 반영해 보았습니다!
놓치고 지나친 부분에 대해 다시 한 번 되짚을 수 있어 좋았어요!
감사합니다~ 🤗

Copy link

@jongmee jongmee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새양 고생하셨습니다! 요구사항에 맞게 꼼꼼하게 잘 구현해주신 것 같아요 👍

다음 단계에서 봐요 ☺️

@jongmee jongmee merged commit 1aa4261 into woowacourse:geoje Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants