-
Notifications
You must be signed in to change notification settings - Fork 305
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에버~ 이렇게 리뷰어와 리뷰이로 만나는 건 또 색다르네요 ㅎㅎ
몇 가지 코멘트 남겨 두었습니다. 확인 부탁드려요:)
추가로 금요일에 진행했던 수업 관련 학습 테스트는 아직 안 올라온 것 같은데요, 다음 리뷰 요청에 서블릿 관련 학습 테스트도 함께 올려주시면 같이 리뷰하겠습니다!
2주간 잘 부탁드립니다!🙌
@@ -1,5 +1,8 @@ | |||
package reflection; | |||
|
|||
import java.lang.reflect.Method; | |||
import java.util.Arrays; | |||
import java.util.List; | |||
import org.junit.jupiter.api.Test; | |||
|
|||
class Junit3TestRunner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습) Reflection이란 무엇이고, 어떻게 활용할 수 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflection은 런타임의 실행 중인 자바 프로그램이 자신을 조사하거나 프로그램의 내부 속성을 조작하는 API입니다.
Reflection은 추상화를 깨는 등의 단점을 가지고 있기 때문에, 애플리케이션 단보다는 보통 프레임워크나 라이브러리를 만들 때 활용되고, 런타임에 알아낼 수 있는 정보를 통해 컴파일 타임에 작업을 할 경우 사용하기 좋습니다!
저희가 흔히 사용하는 라이브러리 중 활용되는 예시로는, JUnit 라이브러리에서 @test 어노테이션이 붙은 메서드만 실행하는 작업, Spring이 자동으로 의존성을 주입하는 작업 등이 있습니다 :)
@@ -9,5 +12,17 @@ void run() throws Exception { | |||
Class<Junit3Test> clazz = Junit3Test.class; | |||
|
|||
// TODO Junit3Test에서 test로 시작하는 메소드 실행 | |||
Junit3Test junit3Test = clazz.getConstructor().newInstance(); | |||
List<Method> testMethods = getDeclaredTestMethods(clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습) getDeclaredXX()와 getxx()의 차이는 무엇일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getDeclaredMethods
는 상속 메서드를 제외하고 직접 선언한 메서드만을 불러옵니다. 접근 제어자에 대한 제한은 없습니다. 반면, getMethods
는 상속 메서드를 포함한 모든 메서드를 반환하며, public 접근 제어자에 한해서만 반환됩니다.
관련하여 학습한 코드도 커밋해두었습니다! de7370e
log.info("handler execution handle"); | ||
Object declaringInstance = method.getDeclaringClass() | ||
.getConstructor() | ||
.newInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요청이 들어올때마다 새로운 인스턴스를 만드는 것은 불필요할 것 같은데, 에버는 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다! Reflection을 통해 컨트롤러 타입을 불러오는 과정에서 컨트롤러 인스턴스를 생성하도록 하여 불필요한 생성을 막도록 해보았어요. 추가로, HandlerExecution 클래스가 핸들러와 핸들러를 선언하는 클래스를 함께 필드로 갖도록 하였습니다 :)
} | ||
|
||
public Object getHandler(final HttpServletRequest request) { | ||
return null; | ||
HandlerKey handlerKey = createHandlerKey(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HandlerKey도 Handler인스턴스처럼 요청마다 새로운 인스턴스가 생성될 것 같은데, 개선해보면 좋을 것 같습니다:)
(저도 제 리뷰어한테 받았던 리뷰랍니다 ㅎㅎ)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 생각이에요! 하나의 서비스에서 HandlerKey가 필드로 갖는 url과 requestMethod는 한정적이기 때문에 캐싱을 적용해 중복 객체를 생성하지 않도록 하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문] 캐싱 로직을 테스트하기 위해 HandlerKeyTest 테스트 코드를 작성했는데요! private으로 생성된 CACHE 데이터를 확인하기 위해서는 테스트만을 위한 메서드를 생성하거나, reflection을 활용하여 접근해야 했어요. 리니는 둘 중 어느 방법이 더 나은 방법이라 생각하시나요? 혹은 캐싱 로직을 테스트하기 위한 좋은 방법을 알고 계시나요? 궁금합니다! 😊
There was a problem hiding this comment.
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);
}
(저도 이 부분은 테스트 없이 넘어갔었는데, 에버 덕분에 발견했습니다 ㅎㅎ)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 두 객체의 동일성을 판단하는 메서드가 제공되는 줄 몰랐네요! isSameAs 활용해서 테스트 진행해야겠어요.
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("유효하지 않은 요청 메서드입니다: " + raw); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EOL이 누락된 것 같네요😮
|
||
@DisplayName("핸들러에 http method가 선언되지 않은 경우 모든 method가 매핑되도록 한다.") | ||
@ParameterizedTest | ||
@ValueSource(strings = {"GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS", "TRACE"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안) @EnumSource
를 활용해볼 수 있을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 생각이에요 반영하겠습니다 👍
리니 좋은 리뷰 감사해요! 학습적인 부분에서도 코멘트 남겨 주셔서 많은 도움이 되었어요. 리뷰를 반영하는 과정에서 리니에게 궁금한 부분이 생겨 질문 남겼습니다! 확인 부탁드려요 감사합니다 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에버~ 데모데이가 가까워지니 정말 정신이 없는 것 같아요😂
질문해주신 부분 포함해서 몇 가지 코멘트 남겼어요!
확인해보시고, 재리뷰요청 해주세요
화이팅입니다~🙌
import java.util.Objects; | ||
|
||
public class HandlerKey { | ||
|
||
private final String url; | ||
private final RequestMethod requestMethod; | ||
private static final List<HandlerKey> CACHE = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
질문) 에버는 캐시 구현으로 List를 사용하셨네요!
제가 생각하기에는 List를 사용하는 것보다는 HashMap이 탐색에서는 성능 상 이점이 있다고 생각하는데, 에버가 List를 선택한 이유가 궁금해요
There was a problem hiding this comment.
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을 통해 데이터를 관리하도록 수정해보겠습니다!
return Arrays.stream(methods) | ||
.filter(method -> method.getName().startsWith("test")) | ||
.toList(); | ||
} | ||
|
||
@DisplayName("getMethods: 상속 메서드를 포함한 모든 public 메서드를 반환한다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍
} | ||
|
||
@DisplayName("getDeclaredMethods: 상속 메서드를 제외하고 직접 선언된 모든 메서드를 반환한다.") | ||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍
@Override | ||
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { | ||
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습) Filter의 doFilter
는 어떻게 동작하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter의 doFilter 로직은, 요청을 처리하기 전 해당 요청을 가로채어 수행됩니다. 따라서 전체적으로 전처리를 위한 작업을 진행할 때 자주 사용됩니다. chain.doFilter(request, response)
를 호출하면 또 다음 필터의 doFilter 로직이 연쇄적으로 수행되고, 필터 체이닝의 끝에 도달하면 최종적으로 서블릿이라 컨트롤러로 요청이 전달됩니다!
request.getServletContext().log("doFilter() 호출"); | ||
log.info("기존 characterEncoding = {}", response.getCharacterEncoding()); | ||
response.setCharacterEncoding("UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 미션 코드를 보고 학습 테스트를 수행해서, request에도 인코딩을 설정했었는데요, ㅎㅎ
이에 대해 제 리뷰어(망쵸)한테 받았던 참고하기 좋은 레퍼런스 공유합니다 ㅎㅎ
인코딩(Encoding)을 하는 이유
tomcat 10.1 URI 인코딩 방식
tomcat 10.1 Body 인코딩 방식
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 레퍼런스 공유 감사합니다! 인코딩 그냥 하는 거지~ 하고 넘겼는데 다시 한번 짚을 수 있어 좋았어요 :)
@@ -64,7 +64,7 @@ void getDeclaredMethods() { | |||
List<String> actual = Arrays.stream(methods) | |||
.map(Method::getName) | |||
.toList(); | |||
assertThat(actual).containsExactly( | |||
assertThat(actual).containsOnly( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
public Object getHandler(final HttpServletRequest request) { | ||
return null; | ||
HandlerKey handlerKey = createHandlerKey(request); |
There was a problem hiding this comment.
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);
}
(저도 이 부분은 테스트 없이 넘어갔었는데, 에버 덕분에 발견했습니다 ㅎㅎ)
리니 안녕하세요! 바쁠텐데 리뷰 주셔서 감사해요! 저도 역시 데모데이 주는 정신없이 흘러가네요 ㅎㅎ 같이 힘내봅시다 👊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에버~ 오늘 정말 바빠보였는데 리뷰까지 반영하시느라 수고하셨습니다.
1단계는 이만 머지해도 될 것 같아요!
여기에서 마무리하고, 얼른 2단계로 넘어가봅시다!!!
바쁘지만 조금만 더 화이팅해봐요~~😺
안녕하세요 리니
종종 인사하다가 리뷰이로 만나게 되니 반가워요 :-)
저는 미션에 시간을 투자할 의향이 있으니
마음 편하게 부담없이 리뷰 남겨주세요.
잘 부탁드립니다 😁