Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MVC 구현하기 - 1단계] 에버(손채영) 미션 제출합니다. #694

Merged
merged 21 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
df6fa40
test: 학습테스트 Reflection
helenason Sep 20, 2024
cb172db
feat: 핸들러 매핑 로직 추가
helenason Sep 20, 2024
6e4c0e8
feat: 뷰 로직 JspView로 위임
helenason Sep 20, 2024
f0cda4b
refactor: 핸들러 매핑 reflection 사용하도록
helenason Sep 20, 2024
2ecca46
feat: RequestMapping method 선언하지 않은 경우 모든 HTTP method 지원
helenason Sep 20, 2024
e312664
test: 유효하지 않은 요청의 경우 예외 발생
helenason Sep 20, 2024
4e45a35
refactor: 핸들 로직 reflection 사용하도록
helenason Sep 20, 2024
e76390e
refactor: DispatcherServlet service 메서드 ModelAndView를 다루도록
helenason Sep 20, 2024
aa674d8
style: 불필요한 import문 제거
helenason Sep 20, 2024
d4d274b
refactor: HandlerKey 캐싱
helenason Sep 22, 2024
0118167
refactor: HandlerExecution 내에서 매번 인스턴스 생성하지 않도록
helenason Sep 22, 2024
ba17506
test: EnumSource 활용하여 파라미터 주입
helenason Sep 22, 2024
de7370e
test: getMethods vs getDeclaredMethods 학습테스트 작성
helenason Sep 22, 2024
613a0c1
style: 재정의 메서드 순서 및 구조 통일
helenason Sep 22, 2024
5b297ce
style: EOF
helenason Sep 22, 2024
54f3b36
test: 학습테스트 Servlet
helenason Sep 22, 2024
fdd3078
test: 학습테스트 Filter
helenason Sep 22, 2024
8ace1d8
test: 포함 여부 확인 로직 수정
helenason Sep 22, 2024
6afede4
style: 불필요한 주석 제거
helenason Sep 22, 2024
279fc92
test: 동등성 대신 동일성 테스트하도록 및 테스트를 위한 비즈니스 코드 제거
helenason Sep 25, 2024
21fcb37
refactor: HandlerKey 캐시 자료구조 변경
helenason Sep 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions app/src/main/java/com/techcourse/DispatcherServlet.java
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package com.techcourse;

import com.interface21.webmvc.servlet.ModelAndView;
import com.interface21.webmvc.servlet.View;
import com.interface21.webmvc.servlet.view.JspView;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.interface21.webmvc.servlet.view.JspView;

public class DispatcherServlet extends HttpServlet {

Expand All @@ -25,27 +27,20 @@ public void init() {
}

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

try {
final var controller = manualHandlerMapping.getHandler(requestURI);
final var viewName = controller.execute(request, response);
move(viewName, request, response);
ModelAndView modelAndView = new ModelAndView(new JspView(viewName));
View view = modelAndView.getView();
view.render(modelAndView.getModel(), request, response);
} catch (Throwable e) {
log.error("Exception : {}", e.getMessage(), e);
throw new ServletException(e.getMessage());
}
}

private void move(final String viewName, final HttpServletRequest request, final HttpServletResponse response) throws Exception {
if (viewName.startsWith(JspView.REDIRECT_PREFIX)) {
response.sendRedirect(viewName.substring(JspView.REDIRECT_PREFIX.length()));
return;
}

final var requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
package com.interface21.web.bind.annotation;

import java.util.Arrays;

public enum RequestMethod {
GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, TRACE
GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, TRACE;

public static RequestMethod find(String raw) {
return Arrays.stream(values())
.filter(value -> value.name().equals(raw))
.findAny()
.orElseThrow(() -> new IllegalArgumentException(String.format("유효하지 않은 요청 메서드입니다: %s", raw)));
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
package com.interface21.webmvc.servlet.mvc.tobe;

import com.interface21.context.stereotype.Controller;
import com.interface21.web.bind.annotation.RequestMapping;
import com.interface21.web.bind.annotation.RequestMethod;
import jakarta.servlet.http.HttpServletRequest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.reflections.Reflections;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class AnnotationHandlerMapping {

private static final Logger log = LoggerFactory.getLogger(AnnotationHandlerMapping.class);

private static final int EMPTY_REQUEST_METHODS = 0;

private final Object[] basePackage;
private final Map<HandlerKey, HandlerExecution> handlerExecutions;

Expand All @@ -21,9 +30,57 @@ public AnnotationHandlerMapping(final Object... basePackage) {

public void initialize() {
log.info("Initialized AnnotationHandlerMapping!");
Reflections reflections = new Reflections(basePackage);
Set<Class<?>> controllerTypes = reflections.getTypesAnnotatedWith(Controller.class);
controllerTypes.forEach(this::mapControllerHandlers);
}

private void mapControllerHandlers(Class<?> controllerType) {
try {
Object controller = controllerType.getConstructor().newInstance();
Method[] handlers = controllerType.getDeclaredMethods();
mapHandlerToExecution(controller, handlers);
} catch (Exception e) {
throw new RuntimeException(e);
}
}

private void mapHandlerToExecution(Object controller, Method[] handlers) {
Arrays.stream(handlers)
.filter(handler -> handler.isAnnotationPresent(RequestMapping.class))
.forEach(handler -> addMapper(handler, new HandlerExecution(controller, handler)));
}

private void addMapper(Method handler, HandlerExecution handlerExecution) {
RequestMapping requestMapping = handler.getAnnotation(RequestMapping.class);
List<HandlerKey> handlerKeys = createHandlerKeys(requestMapping);
handlerKeys.forEach(handlerKey -> handlerExecutions.put(handlerKey, handlerExecution));
}

private List<HandlerKey> createHandlerKeys(RequestMapping requestMapping) {
String uri = requestMapping.value();
RequestMethod[] requestMethods = requestMapping.method();
if (requestMethods.length == EMPTY_REQUEST_METHODS) {
requestMethods = RequestMethod.values();
}
return Arrays.stream(requestMethods)
.map(requestMethod -> HandlerKey.from(uri, requestMethod))
.toList();
}

public Object getHandler(final HttpServletRequest request) {
return null;
HandlerKey handlerKey = createHandlerKey(request);

Choose a reason for hiding this comment

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

HandlerKey도 Handler인스턴스처럼 요청마다 새로운 인스턴스가 생성될 것 같은데, 개선해보면 좋을 것 같습니다:)
(저도 제 리뷰어한테 받았던 리뷰랍니다 ㅎㅎ)

Copy link
Member Author

@helenason helenason Sep 22, 2024

Choose a reason for hiding this comment

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

좋은 생각이에요! 하나의 서비스에서 HandlerKey가 필드로 갖는 url과 requestMethod는 한정적이기 때문에 캐싱을 적용해 중복 객체를 생성하지 않도록 하였습니다.

d4d274b

Copy link
Member Author

Choose a reason for hiding this comment

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

[질문] 캐싱 로직을 테스트하기 위해 HandlerKeyTest 테스트 코드를 작성했는데요! private으로 생성된 CACHE 데이터를 확인하기 위해서는 테스트만을 위한 메서드를 생성하거나, reflection을 활용하여 접근해야 했어요. 리니는 둘 중 어느 방법이 더 나은 방법이라 생각하시나요? 혹은 캐싱 로직을 테스트하기 위한 좋은 방법을 알고 계시나요? 궁금합니다! 😊

Choose a reason for hiding this comment

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

테스트를 위한 메서드가 production에 생기는 것보단, reflection을 활용하는 것이 더 좋아보여요! assertJ의 extract 메서드도 reflection으로 구현되어있다는 점을 생각한다면 reflection으로 검증하는 것도 괜찮다고 생각합니다!
혹은 검증하고자 하는 것이 이미 존재한다면 있는 인스턴스를 반환하고, 아니라면 새로운 인스턴스를 생성한다 라면 다음과 같이 테스트를 작성해볼 수도 있을 것 같아요!

    @DisplayName("동일한_URL과_RequestMethod로_생성된_인스턴스는_동일해야한다")
    @Test
    void getAlreadyExistingInstance() {
        // given
        String url = "/test";
        RequestMethod method = RequestMethod.GET;

        // when
        HandlerKey firstInstance = HandlerKey.of(url, method);
        HandlerKey secondInstance = HandlerKey.of(url, method);

        // then
        assertThat(firstInstance).isSameAs(secondInstance);
    }

(저도 이 부분은 테스트 없이 넘어갔었는데, 에버 덕분에 발견했습니다 ㅎㅎ)

Copy link
Member Author

@helenason helenason Sep 24, 2024

Choose a reason for hiding this comment

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

오 두 객체의 동일성을 판단하는 메서드가 제공되는 줄 몰랐네요! isSameAs 활용해서 테스트 진행해야겠어요.

279fc92

HandlerExecution handlerExecution = handlerExecutions.get(handlerKey);
if (handlerExecution == null) {
throw new IllegalArgumentException(
String.format("해당 요청에 대응하는 핸들러가 없습니다: %s %s", request.getMethod(), request.getRequestURI()));
}
return handlerExecution;
}

private HandlerKey createHandlerKey(HttpServletRequest request) {
String requestURI = request.getRequestURI();
RequestMethod requestMethod = RequestMethod.find(request.getMethod());
return HandlerKey.from(requestURI, requestMethod);
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,26 @@
package com.interface21.webmvc.servlet.mvc.tobe;

import com.interface21.webmvc.servlet.ModelAndView;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import com.interface21.webmvc.servlet.ModelAndView;
import java.lang.reflect.Method;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class HandlerExecution {

private static final Logger log = LoggerFactory.getLogger(HandlerExecution.class);

private final Object controller;
private final Method handler;

public HandlerExecution(Object controller, Method handler) {
this.controller = controller;
this.handler = handler;
}

public ModelAndView handle(final HttpServletRequest request, final HttpServletResponse response) throws Exception {
return null;
log.info("handler execution handle");
return (ModelAndView) handler.invoke(controller, request, response);
}
}
Original file line number Diff line number Diff line change
@@ -1,37 +1,56 @@
package com.interface21.webmvc.servlet.mvc.tobe;

import com.interface21.web.bind.annotation.RequestMethod;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

public class HandlerKey {

private final String url;
private final RequestMethod requestMethod;
private static final List<HandlerKey> CACHE = new ArrayList<>();

Choose a reason for hiding this comment

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

질문) 에버는 캐시 구현으로 List를 사용하셨네요!
제가 생각하기에는 List를 사용하는 것보다는 HashMap이 탐색에서는 성능 상 이점이 있다고 생각하는데, 에버가 List를 선택한 이유가 궁금해요☺️

Copy link
Member Author

@helenason helenason Sep 25, 2024

Choose a reason for hiding this comment

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

List가 HandlerKey의 묶음을 나타내기 가장 적절한 자료구조라고 생각했어요! 물론 HashMap이 성능 상 이점은 있지만 Map에 대한 key를 생성하는 것이 불필요하다는 판단을 하기도 했습니다. 하지만 리니의 PR 코드와 함께 추가적으로 고민하다보니, url과 requestMethod를 조합해 Map의 key로 사용하여 성능을 높이는 방향도 충분히 좋다고 생각했어요. 따라서 HashMap을 통해 데이터를 관리하도록 수정해보겠습니다!

21fcb37


public final String url;
public final RequestMethod requestMethod;

public HandlerKey(final String url, final RequestMethod requestMethod) {
public static HandlerKey from(String url, RequestMethod requestMethod) {
return CACHE.stream()
.filter(handlerKey -> handlerKey.url.equals(url) && handlerKey.requestMethod == requestMethod)
.findAny()
.orElseGet(() -> new HandlerKey(url, requestMethod));
}

private HandlerKey(final String url, final RequestMethod requestMethod) {
this.url = url;
this.requestMethod = requestMethod;
CACHE.add(this);
}

@Override
public String toString() {
return "HandlerKey{" +
"url='" + url + '\'' +
", requestMethod=" + requestMethod +
'}';
protected static void cleanCache() {
CACHE.clear();
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof HandlerKey)) return false;
HandlerKey that = (HandlerKey) o;
return Objects.equals(url, that.url) && requestMethod == that.requestMethod;
if (this == o) {
return true;
}
if (!(o instanceof HandlerKey)) {
return false;
}
HandlerKey handlerKey = (HandlerKey) o;
return Objects.equals(url, handlerKey.url) && requestMethod == handlerKey.requestMethod;
}

@Override
public int hashCode() {
return Objects.hash(url, requestMethod);
}

@Override
public String toString() {
return "HandlerKey{" +
"url='" + url + '\'' +
", requestMethod=" + requestMethod +
'}';
}
}
14 changes: 12 additions & 2 deletions mvc/src/main/java/com/interface21/webmvc/servlet/view/JspView.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.interface21.webmvc.servlet.view;

import com.interface21.webmvc.servlet.View;
import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.slf4j.Logger;
Expand All @@ -14,18 +15,27 @@ public class JspView implements View {

public static final String REDIRECT_PREFIX = "redirect:";

private final String viewName;

public JspView(final String viewName) {
this.viewName = viewName;
}

@Override
public void render(final Map<String, ?> model, final HttpServletRequest request, final HttpServletResponse response) throws Exception {
// todo
log.info("jsp view render");

model.keySet().forEach(key -> {
log.debug("attribute name : {}, value : {}", key, model.get(key));
request.setAttribute(key, model.get(key));
});

// todo
if (viewName.startsWith(REDIRECT_PREFIX)) {
response.sendRedirect(viewName.substring(REDIRECT_PREFIX.length()));
return;
}

RequestDispatcher requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.interface21.web.bind.annotation;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

class RequestMethodTest {

@DisplayName("전달받은 문자열과 형태가 같은 요청 메서드를 반환한다.")
@Test
void should_findRequestMethod_when_givenValidString() {
// given
String raw = "GET";

// when
RequestMethod requestMethod = RequestMethod.find(raw);

// then
assertThat(requestMethod).isEqualTo(RequestMethod.GET);
}

@DisplayName("전달받은 문자열이 유효하지 않은 경우 예외가 발생한다.")
@Test
void should_throwException_when_givenInvalidString() {
// given
String raw = "INVALID";

// when & then
assertThatThrownBy(() -> RequestMethod.find(raw))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("유효하지 않은 요청 메서드입니다: " + raw);
}
}
Loading