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 구현하기 3단계] 우가(윤정욱) 미션 제출합니다. #570

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

wugawuga
Copy link
Member

안녕하세요 달리!! 3단계도 많이 헤맨것 같습니다ㅠㅠ

이번에 집중한 부분은 의존성 분리였습니다.
레거시를 어떻게 제거를 해나가야 최대한 오류없이 의존성도 깔끔하게 관리될까가 고민이었는데요.

DispatcherServletInitializer 는 app 모듈에서 뺄 수가 없더라구요.
이 부분 달리 의견이 궁금합니다.

그리고 mvc 모듈에서 TomcatStarter 와 DispatcherServlet 의 위치인데요.
web 패키지 안에 servlet 패키지와 TomcatStarter.class
servlet 패키지 안에 DispatcherServlet.class 가 있습니다.

달리는 패키지를 어떻게 나누셨는지 아직 못하셨다면 어떻게 나누는게 좋을지 의견이 궁금합니다.

@waterricecake
Copy link

waterricecake commented Sep 25, 2023

저도... 헤매고 헤매고 헤매다가 일단 추후에 리펙터링을 하더라도 일단 내야지 하고 제출했습니다.
저도 우가처럼 mvc와 관련된 모든 class를 옮기고 싶었지만 ServletContainerInitializer가 어떤 식으로 작동되는지를 이해를 못해서 못옮기는 중이었습니다. DispatcherServletInitializer을 app패키지에서 mvc 패키지로 옮기니 SpringServletContainerInitializer에서
INFO: No Spring WebApplicationInitializer types detected on classpath라는 log가 생기는 것이 보여 확인해 보니
ServletContainerInitializer에서 WebApplicationInitializer.class를 찾지 못하여 initializers 가 비어서 생기는 문제인 것 같습니다. 저도 이걸 어떻게든 찾아보고 싶은데 내부 로직이 생각보다 복잡하더라고요. 그래서 못옮기고 있었습니다.
뭔가 힌트가 되는게 있다면... 실제 SpringApplication을 실행할때

public class Application {

    public static void main(String[] args) {
        SpringApplication.run(Application.class, args);
    }
}

와 같이 작동을 하는 거 보면 Spring에게 지금 현제 Class의 위치라던지 기타 정보를 이와 같은 형태로 넣어줘야하는 것 아닌가 추측하고 있습니다...

Copy link

@waterricecake waterricecake left a comment

Choose a reason for hiding this comment

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

우가! 수고하셨습니다!
우가가 어떤 것을 고민했는지 해결했는지 잘 보이는 코드였습니다.
구조가 너무 깔끔하여 리뷰남길 것이 없네요.
대신 제가 하다가 고민했던 부분에서 우가와 차이가 있어 질문 남겨 봅니다.

Comment on lines +32 to +34
final ModelAndView modelAndView = new ModelAndView(new JsonView());
modelAndView.addObject("model", result);
return modelAndView;

Choose a reason for hiding this comment

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

여기서 부터가 JsonView로 반환할때 사용되는 부분인데
앞 handler에서 반환하는 값을 Map<String, ?>라는 것을 가정하고 (혹인 이와 비슷하게 key:value의 형태)
ModelAndView가 가지고 있는 Map<String,Object> model에 하나씩 넣는 것은 어떤가요?
이럴경우 추후 JsonView에서 render하는 과정에서 'model'키를 찾아서 반환하는 과정없이 바로 인자로 넘겨줄 수 있을 것 같은데

Copy link
Member Author

Choose a reason for hiding this comment

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

Handler 가 반환하는게 Map<String, ?> 라고 가정을 한다는 게 어떤 말씀인지 와닿지가 않는 것 같습니다!
이미 addObject 로 모델이 반환될 경우 집어 넣어주고 있습니다. 하나씩 넣는 것도 어떤 말씀인지 이해가 잘 되지 않아요ㅠㅠ

코드로 보여주실 수 있으신가요?

Copy link

@waterricecake waterricecake Sep 26, 2023

Choose a reason for hiding this comment

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

Suggested change
final ModelAndView modelAndView = new ModelAndView(new JsonView());
modelAndView.addObject("model", result);
return modelAndView;
if(result instanceof Map){
return getModelAndJsonView(((Map<String,?>) result));
}
throw new IllegalAccessException();
}
private ModelAndView getModelAndJsonView(Map<String, ?> result){
final ModelAndView modelAndView = new ModelAndView(new JsonView());
for(Entry entry: result.entrySet()){
modelAndView.addObject((String) entry.getKey(),entry.getValue());
}
return new ModelAndView(new JsonView());
}

이런식으로 json도 key:value 형태이니 이런 형식의 타입으로 넣어주는 것은 어떤가요? 이번 미션에서 UserController에서는 ModelAndView를 반환하지만
Controller에서 String을 반환하는 것을 ModelAndView로 바꿔주는 것 처럼 Map형태를 반환하는 값을 Json으로 변환해준다는 것은 어떤가 하여 여쭈어 봤습니다 그럴경우 JsonView부분에서

    @Override
    public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response)
            throws Exception {

        response.setHeader("Content-Type", MediaType.APPLICATION_JSON_UTF8_VALUE);
        //final String result = objectMapper.writeValueAsString(model.get("model"));
        final String result = objectMapper.writeValueAsString(model);

        final PrintWriter writer = response.getWriter();
        writer.write(result);
    }

형태가 되지 않을까 하여 여쭈어 봤습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

다른 객체를 반환하게 될 땐 동작을 하지 않겠군요

Controller 에서 Json 으로 반환하기 위한 다양한 객체가 있을 것 같은데,
달리가 말씀하신 부분은
다른 객체를 반환하게 되면 또 그에 맞춰서 instanceof (dto or response 등등) 하나하나 만들어 줘야 합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

또 다른 궁금한 점이 있습니다.

modelAndView 를 생성해서 model 을 차곡차곡 담은 다음에,
새로운 modelAndView 를 반환하는 이유가 궁금합니다!! 모델이 하나도 없을 것 같아서요.

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다. reflection 을 따로 구현했어야 합니다

하지만 jsonView 에서 쓰기 쉽게 HandlerAdaptors 에서 만들어 넣어주는 것은 어댑터 역할은 아닌 것 같다는 생각이 드네요.

필요한 데이터를 가공하는 것 또한 JsonView 에서 진행하면 좋을 것 같은데 달리는 어떻게 생각하시나요?

Copy link

@waterricecake waterricecake Sep 26, 2023

Choose a reason for hiding this comment

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

우가의 말대로 데이터를 가공하는 것은 어댑터의 역할인 것 같습니다.

View의 역할은 받은 데이터를 클라이언트가 받을수 잇는 형태로 가공하여 보내는 역할을 하는 것이라고 생각을 하엿습니다. 그 이유는 render라는 메서드 명을 보고 파악하였습니다.

그렇다면 View가 받는 매개변수가 Map<String,?>으로 되어있는 이유는 무엇이라고 생각하나요??

만약 model이 어떤 형태이든 ObjectMapper로 해결할 수 있다면 Map<String,?> 이 아닌 Object model 이나 Object... models로도 충분했을거라고 생각하는데... 저는 ModelAndView에서 model field의 타입이 'Map<String,Object>'인 것과 연관이 있지않을까 생각을 했었습니다.

물론 코치님들의 기존 코드가 실제 설계와 같지 않을 경우도 있겠지만요

Copy link
Member Author

Choose a reason for hiding this comment

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

필요한 데이터를 가공하는 것 또한 JsonView 에서 진행하면 좋을 것 같은데 달리는 어떻게 생각하시나요?

어댑터의 역할이라고 잘못 이해하신 것 같네요.
JsonView 라고 생각을 하고 있습니다.

render 메소드 자체가 View 인터페이스의 메소드를 구현해야하기 때문에 Model<String, ?> 으로 받았습니다.
Object 였다면 달리 말씀대로 넘겼을 거에요.

주어진 View 인터페이스도 그대로 사용하는 것이 미션의 요구사항 아닐까요?
달리는 View 인터페이스의 render 메소드를 리팩터링해서 Object 나, Object ... 로 하셨는지 궁금합니다.

달리 의견에 대한 답변입니다.
최대한 JspView 랑 JsonView 를 추상화 하기 위해선 Map<String, ?> 으로 해결이 되지 않을까요?
Jsp 는 Map 으로 Json 은 Object 로 해야한다면, View 인터페이스를 만들어주신 의미가 퇴색된 것 같네요

만약 model이 어떤 형태이든 ObjectMapper로 해결할 수 있다면 Map<String,?> 이 아닌 Object model 이나 Object... models로도 충분했을거라고 생각하는데... 저는 ModelAndView에서 model field의 타입이 'Map<String,Object>'인 것과 연관이 있지않을까 생각을 했었습니다.

달리 말씀대로면 Model 과 View 자체를 분리하고 Model 인스턴스 필드에 Object 를 둬야할 것 같습니다.
그러면 JspView 에서 다시 Object 를 Map 으로 형변환을 해야하고 Json 에선 그대로 사용을 해야합니다.

이 두 과정을 한번에 묶을 수 있는게 Map 형태이고 JsonView 에서 그냥 필요한 걸 꺼내쓰면 되지 않을까요?
달리 의견이 궁금합니다.

Copy link

@waterricecake waterricecake Sep 26, 2023

Choose a reason for hiding this comment

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

어댑터의 역할이라고 잘못 이해하신 것 같네요.
JsonView 라고 생각을 하고 있습니다.

이게 맞는것 같네요. 제가 잘못 생각했습니다.


달리는 View 인터페이스의 render 메소드를 리팩터링해서 Object 나, Object ... 로 하셨는지 궁금합니다.

저는 그대로 Handler에서 반환되는 객체가 Map이라는 가정하에 Map으로 하나씩 넣어주었습니다.
JspView의 render 메서드를 보면

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

와 같이 사용이 model에서 값을 꺼내어 request에 넣어주는데 다른 view를 구현한다고하면 이런 식으로 각 key마다 값을 넣는 상황이 있을 수 도 있다고 생각하여 ModelAndView의 Model에 나누어 넣게 되었습니다.


Jsp 는 Map 으로 Json 은 Object 로 해야한다면, View 인터페이스를 만들어주신 의미가 퇴색된 것 같네요

이 말은 동의합니다. 제가 말하고 싶었던 것은

void render(Map<String, ?> model, HttpServletRequest request, HttpServletResponse response)

에서 model이 받는 값이 "model":Map<String,?> 의 model size 1개의 map을 가지고 있어서, 이런 식으로 model이 어떤 타입이든 넣는 방식때문에 Map<String,Object>를 사용할 것이라면 Object를 사용하는 것이 더 나은가 싶어서 말했던 것입니다.

또 어차피 model.get("model")을 해서 가져온 것을 형변환 없이 바로 ObjectMapper에 넣는 다면 형변환 과정은 필요없지 않나요? 뭐 물론 jspView에서는 형변환 과정이 필요하겠네요. 저도 결국 이 방법은 택하지 않았습니다.


이 두 과정을 한번에 묶을 수 있는게 Map 형태이고 JsonView 에서 그냥 필요한 걸 꺼내쓰면 되지 않을까요?

이 말은 맞는 것 같습니다. Map<String,Object>가 사용되었길레 ModelAndView의 model을 그대로 넣으면 되지 않겠나라는 생각했는데, model 자체를 Map에 넣어 보관하는 것이 좀더 유연하게 다른 상황을 대처할 수 있을 것 같네요. 대표적으로 Controller의 반환값이 List의 형태일 경우 따로 이 List를 풀 필요없이 넣어주면 되니까요. 그러면 Controller의 반환값이 무엇이든지 해결할 수 있어 항상 유연하게 사용할 수 있을 것 같습니다.

제가 어색하다고 느낀점이 이것인 것 같습니다. JspView에서는 지금 형태를 보면 넣은 모델에서 따로 어떤 값("model"에 해당하는) 꺼내지 않고 모든 key에 대해 작동을 하지만 HandlerAdaptors에서 view가 매게변수의 Map의 필드에 값을 지정하고 넣고 있습니다. 후자의 경우 HandlerAdaptors가 JsonView의 동작원리가 "model"이라는 key값을 항상 알고 사용해야 하지만 전자의 경우 이에 상관없이 값을 넣으면 된다고 생각하게 되었습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

미션 중에 있어서 프레임워크 요구사항을 지키면서 확장성까지 챙기려고 했는데요
달리는 요구사항에만 집중하신 것 같네요 ㅎㅎ

서로 다른 시선이었기 때문에 더 재밌는 토론이었던 것 같습니다 ㅎㅎㅎ

@wugawuga
Copy link
Member Author

감사합니다! 달리
달리 코멘트에 잘 와닿지 않는 부분이 있어서 코드로 요청드립니다!

@wugawuga
Copy link
Member Author

달리가 작성해주신 코드에 대한 답변입니다!!!

Comment on lines +32 to +34
final ModelAndView modelAndView = new ModelAndView(new JsonView());
modelAndView.addObject("model", result);
return modelAndView;

Choose a reason for hiding this comment

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

아 제가 잘못 적었군요;;

 private ModelAndView getModelAndJsonView(Map<String, ?> result){
        final ModelAndView modelAndView = new ModelAndView(new JsonView());
        for(Entry entry: result.entrySet()){
            modelAndView.addObject((String) entry.getKey(),entry.getValue());
        }
        return modelAndView;
    }

입니다.

다른 객체를 반환하게 될 땐 동작을 하지 않겠군요
Controller 에서 Json 으로 반환하기 위한 다양한 객체가 있을 것 같은데,
달리가 말씀하신 부분은
다른 객체를 반환하게 되면 또 그에 맞춰서 instanceof (dto or response 등등) 하나하나 만들어 줘야 합니다.

저는 Map이나 List와 같이 util에 있는 클래스의 경우와 dto, response는 다른 경우의 수라고 생각을 했습니다. Map의 경우 instanceof로 그 경우를 잡을 수 있고 또 key:value형태의 json type으로 쉽게 나타낼 수 있으니 좋지 않을 까 하여 적어보았습니다. 그저 Spring에서 Map<> 형태를 Json형태로 편하게 반환하기에 한번 여쭤보았던 것입니다. 근데 다시 생각해보니 ObjectMapper가 이를 다 해결해주겠네요. 저는 dto나 response라면 reflection을 통해 따로 필드를 읽어와서 해결해와야지 않나 생각하고 있었는데 제 생각이 짧았던것 같습니다.

Copy link

@waterricecake waterricecake left a comment

Choose a reason for hiding this comment

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

저도 이번 미션들은 혼자하다보니 뭔가 저만의 생각에만 빠진 느낌이었는데 우가 덕분에 더 넓게 볼 수 있었네요.
ㅎㅎ 저도 재밌었습니다. 이번 미션은 이만 머지하겠습니다. JDBC 미션도 화이팅입니다!

@waterricecake waterricecake merged commit fbd1bbf into woowacourse:wugawuga Sep 26, 2023
1 check failed
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