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 - Step 2] 효오 미션 제출합니다. #59

Merged
merged 32 commits into from
Oct 21, 2019

Conversation

hyojaekim
Copy link

안녕하세요. 효오입니다.

pr 보내고 계속해서 테스트 추가하고, ViewResolver, ArgumentResolver 를 구현할 예정입니다.

항상 좋은 리뷰 주셔서 감사합니다! :)

Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요 효오!
코드가 깔끔하네요 💯
몇가지 피드백 남겼으니 확인부탁드려요 :)

response.getWriter().write(json);
}

private String toJson(Map<String, ?> model) throws JsonProcessingException {

Choose a reason for hiding this comment

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

JsonUtils라는 클래스가 있는데 해당 클래스로 옮겨보면 어떨까요 :)

Copy link
Author

Choose a reason for hiding this comment

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

놓친 부분이 있었네요! 피드백 감사합니다.


@RequestMapping(value = "/", method = RequestMethod.GET)
public ModelAndView home(HttpServletRequest request, HttpServletResponse response) {
request.setAttribute("users", DataBase.findAll());

Choose a reason for hiding this comment

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

layer 를 분리해보면 어떨까요?
https://wckhg89.tistory.com/13 (layer 부분만 참고하셔서 보면 좋을 것 같아요!)

Copy link
Author

@hyojaekim hyojaekim Oct 15, 2019

Choose a reason for hiding this comment

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

Service Layer 만드려고 생각은 했으나, MVC 프레임워크에 집중하려고 했습니다!
추가 미션으로 ArgumentResolver를 구현하고, Service Layer는 일부만 구현했습니다 :)

- add ControllerHandlerAdapter
- add HandlerExecutionHandlerAdapter
- DispatcherServlet에 HandlerAdapter 로직 적용
- Adapter를 찾지 못할 경우 예외 추가
- HandlerMapping 리턴 타입 변경
- UserCreateController -> CreateUserController
- CreateUserService -> UserCreateService
- 로그인 체크 기능 추가
- 유저 찾는 기능 추가
- 로그인 성공
- 로그인 실패
@hyojaekim
Copy link
Author

안녕하세요. 정훈님!
이번에 MVC 프레임워크를 구현하면서 요구사항 외에 다른 것들을 구현하려고 노력해봤습니다.
ArgumentResolver를 만들었는데 이게 맞는건지 잘 모르겠네요..
시간이 걸릴까봐 일단은 Service Layer는 일부만 만들고 제출하고, 계속해서 리팩토링 하겠습니다.

Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요 효오!
깔끔하네요 👍
몇가지 피드백 남겼으니 확인부탁드려요 :)

}

private Object matchParameter(Parameter parameter) {
if (parameter.isAnnotationPresent(RequestParam.class)) {

Choose a reason for hiding this comment

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

ArgumentResolver 역시 handlerAdapters와 같이 interface로 구현하면 추가될 때마다 분기를 없앨 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

HandlerAdapter 처럼 interface로 구현을 했는데,
ArgumnetResolver의 Adapter들이 네이밍이 적절한지 궁금합니다!

this.response = response;
}

public Object[] resolve(Method method) {

Choose a reason for hiding this comment

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

parameter type이 맞지 않으면 4xx 예외를 주는 것은 어떨까요?

Copy link
Author

@hyojaekim hyojaekim Oct 19, 2019

Choose a reason for hiding this comment

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

클라이언트에서 유효한 요청을 받은 다음
해당 메소드를 찾고 메소드의 parameter를 가져오는 역할을 하는데
피드백 내용을 토대로 일치하지 않는 parameter에 대해서 예외를 던지도록 수정했습니다.

그런데 클라이언트에서 유효한 요청을 보냈기 때문에 4xx 클라이언트 요청에 대한 오류보다
서버 오류가 맞겠다고 싶어서 500 에러로 처리하려고 합니다. 괜찮나요?!

private Object[] getParameters(HttpServletRequest request, HttpServletResponse response) {
        try {
            ArgumentResolver argumentResolver = new ArgumentResolver(request, response);
            return argumentResolver.resolve(method);
        } catch (NotMatchParameterException e) {
            response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
            throw new NotMatchParameterException();
        }
    }

또 상태 코드를 설정할 때, 위와 같이 상태 코드를 설정하도록 변경 했는데 어떤지 궁금합니다.

Choose a reason for hiding this comment

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

스프링에서 RequestParam 어노테이션을 사용할 때 500 서버에러를 상태코드로 받나요?
서버에러는 말 그대로 서버에서 예기치 못한 에러가 발생했을 때 나오는 것이죠!

http status code는 client 측에 우리가 약속된 http 상태 코드로 어떤 부분이 잘못되었다는 것인지 알려주기 위해서 사용하죠!

@RequestParam("userId") String userId
url?userI=abc 로 요청을 보냈을 경우 500에러를 반환해주어야 할까요?


import static org.junit.jupiter.api.Assertions.assertTrue;

public class ArgumentResolverTest {

Choose a reason for hiding this comment

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

조금 더 다양한 케이스가 있으면 좋을 것 같아요!

return new ModelAndView(result.toString());
}

return (ModelAndView) result;

Choose a reason for hiding this comment

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

type casting하지 않아도 handle의 return이 ModelAndView인 것 같은데 맞나요??

Copy link
Author

Choose a reason for hiding this comment

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

같이 수정을 했어야 했는데 피드백 감사합니다!

- NotMatchParameterException
- ArgumentResolver 생성 테스트
- 파라미터 타입 안맞을 때, 예외 발생하는 테스트
- UserCreatedDto -> String
- 변경된 리턴 타입에 맞게 테스트 수정
Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요 효오!
인자 매핑까지 깔끔하게 구현해주셨네요 💯
관련하여 몇가지 간단한 피드백과 질문해주신 것에 대한 답변 남겼습니다!
미션 진행하느라 고생많으셨습니다 :)
추가적인 질문은 언제든 DM 주세요!

public boolean match(Parameter parameter) {
Class<?> parameterType = parameter.getType();

return parameterType.equals(HttpServletRequest.class);

Choose a reason for hiding this comment

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

isAssignableFrom를 사용해보면 어떨까요?
HttpServletRequest.class.isAssignableFrom(parameterType)
NPE가 발생할 수 있는 곳에는 항상 유의하면 좋을 것 같아요!

import java.lang.reflect.Parameter;
import java.util.*;

public class ArgumentResolver {

Choose a reason for hiding this comment

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

객체를 argument로 받을 수 있도록 처리하면 좋았을텐데 아쉽네요!

@jeonghoon1107 jeonghoon1107 merged commit 1e5b2c1 into woowacourse:hyojaekim Oct 21, 2019
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