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 구현하기 - 2, 3단계] 케로(김지현) 미션 제출합니다. #578

Merged
merged 14 commits into from
Sep 27, 2023

Conversation

jyeost
Copy link

@jyeost jyeost commented Sep 24, 2023

안녕하세여 블랙캣....
... 감사합니다 ...

Copy link

@Songusika Songusika left a comment

Choose a reason for hiding this comment

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

안녕하세요 케로!
코드가 전체적으로 깔끔해서 수정할 부분이 크게 없네요!
제가 궁금한 부분에 코맨트 남겨놓았습니다!

public class IndexController {

@RequestMapping(value = "/")
public ModelAndView show(HttpServletRequest req, HttpServletResponse res) {

Choose a reason for hiding this comment

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

다른 코드를 봤을 때는 final 컨벤션으로 사용하시는 것 같아요 이런 부분도 채워주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

머리로는 알지만... 마음은.... 귀찮아서 그랬습니다 함번만 봐주쇼

@@ -10,5 +10,5 @@
public @interface RequestMapping {
String value() default "";

RequestMethod[] method() default {};
RequestMethod[] method() default {RequestMethod.GET};

Choose a reason for hiding this comment

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

method 의 default 를 GET 으로 두신 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

리팩터링을 하다가 get 메서드를 매번 지정해주기 귀찮아져서
스프링의 @RequestMapping 의 메서드를 지정해주지 않아도 get처리 되는 것이 떠올라서 가장 쉬운 방법으로 구현했습니다.
(실제 스프링에서는 어노테이션에서 디폴트 처리를 해주진 않습니다... )

Comment on lines +24 to +34
return new ModelAndView(new JspView("redirect:/index.jsp"));
}

@RequestMapping(value = "/register", method = RequestMethod.GET)
public ModelAndView show(HttpServletRequest req, HttpServletResponse res) {
return new ModelAndView(new JspView("/register.jsp"));
}

@RequestMapping(value = "/register/view")
public ModelAndView show2(HttpServletRequest req, HttpServletResponse res) {
return new ModelAndView(new JspView("/register.jsp"));

Choose a reason for hiding this comment

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

show(), show2() 는 어떤 차이인가요?

Copy link
Author

@jyeost jyeost Sep 26, 2023

Choose a reason for hiding this comment

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

사실 큰 차이는 없습니다 !!!
레거시 코드를 수정해가면서 이미 배포된 api라면 두가지 버전을 다 유지 시키는 게 맞지 않을까? 해서 /register/view uri를 살려봤습니다.

블랙캣상도 안드로이드 팀인만큼 프로젝트에서 api가 바뀔때 어떤 처리를 해주고 계신지 궁금!!! 합니다!!!

Choose a reason for hiding this comment

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

저희는 현재 안드로이드와 배포를 같이 하기 때문에 아직 api 버저닝을 하진 않습니다.
다만 고민을 해보면

  1. api에 버전을 명시해서 api 명세를 작성하고 구버전 api 웹 어플리케이션 서버를 특정 기간동안 살려둔다.
    • nginX 와 같은 웹 서버를 통해서 프록시 처리를 한다.
  2. 케로의 방법 처럼 하나의 웹 어플리케이션에 구버전 api 코드를 유지한다.

케로의 팀은 어떤 방식을 사용하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

저희는 2번 방법인데 헤더에 특정 값을 넣어서 안드로이드 팀이랑 맞췄습니다 ㅎㅅㅎ
아직 버전2를 배포하지 않아서 두 버전 전부 유지 해본 경험은 없슴돠...
한번 배포한 api를 닫는건 굉장히 어렵다고 들었는데,, 버전이 여러개라면 모든 서버를 동시에 유지하기엔 비용이 아까울 것 같아요 ...

Comment on lines +21 to +22
private final transient HandlerMappingRegistry handlerMappingRegistry;
private final transient HandlerAdapterRegistry handlerAdapterRegistry;

Choose a reason for hiding this comment

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

transient 는 왜 이곳에서 필요한가요? (정말 모름)

Copy link
Author

@jyeost jyeost Sep 26, 2023

Choose a reason for hiding this comment

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

소나가 자꾸 이 클래스가 직렬화 가능 클래스라고 해당 클래스의 필드가 직렬화 가능한지 명시해 달라며 경고창을 띄워서...
직렬화 할 필요 없다고 써 준 것입니다...
이 DispatcherServlet이 왜 직렬화 가능 클래스인지는 모르겟어오.... 🥲

Choose a reason for hiding this comment

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

HttpServletGenericServlet 을 상속하고 있는데요!
GenericServletSerializable 를 구현하고 있습니다.
따라서 소나에서 직렬화 대상에서 제외하라고 transient 키워드를 사용하라고 하는 것 같아요

다시 돌아와서 GenericServlet이 왜 직렬화 가능해야하는지는 잘 모르겠어요
2000년대 초반 질문 글을 찾아봐도 찾아봐도 옛날 레거시이다 등 잘 모르겠다는 대답이 대다수네요. 😭

Copy link
Author

Choose a reason for hiding this comment

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

대신 답변해줘서 잘 얻어먹었습니다 ~~ 감사합니다 ^^*

Comment on lines +54 to +61
private void render(final ModelAndView modelAndView, final HttpServletRequest request, final HttpServletResponse response) {
final View view = modelAndView.getView();
try {
view.render(modelAndView.getModel(), request, response);
} catch (Exception e) {
throw new RuntimeException(e);
}
}

Choose a reason for hiding this comment

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

해당 로직이 ModelAndView 객체 내부에 있는 것에 대한 케로의 의견이 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

ModelAndView가 View를 들고 있다고 해서 view를 render하는 기능까지 들고있어야 할 필요는 없다고 생각합니다
지금 이 객체는 거의 Dto와 비슷한 역할을 하고 있는것 같슴돠

Comment on lines +15 to +21
public HandlerAdapterRegistry() {
handlerAdapters = new ArrayList<>();
}

public void initialize() {
handlerAdapters.add(new AnnotationHandlerAdapter());
}

Choose a reason for hiding this comment

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

HandlerAdapterRegistry 의 생성자에서 넣어도 될 것 같은데 initialize() 메서드에서 초기화하는 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

이 클래스를 사용하는 DispatcherServlet이 객체가 생성되면 init 메서드가 자동으로 실행되기 때문에 이에 맞춰서 생성 작업과 초기화 작업을 나누어서 생성 작업의 로직을 덜고자 했슴돠

Comment on lines 26 to 30
if (model.size() <= 1) {
for (String key : model.keySet()) {
return mapper.writeValueAsString(model.get(key));
}
}

Choose a reason for hiding this comment

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

model.values().iterator().next(); 를 사용하면 좀 더 깔끔하게 가져올 수 있습니다!

Copy link
Author

@jyeost jyeost Sep 26, 2023

Choose a reason for hiding this comment

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

헐 좋네유... !!!! 당장 도입!!!!!!! 갈겨!!!!!!

Comment on lines +21 to +25
if (viewName.contains(FILE_EXTENSION)) {
this.viewName = viewName;
} else {
this.viewName = viewName + FILE_EXTENSION;
}

Choose a reason for hiding this comment

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

👍

Comment on lines +61 to +69
private void doChain(final ServletRequest request, final ServletResponse response, final FilterChain chain) throws IOException, ServletException {
try {
chain.doFilter(request, response);
} catch (Exception e) {
log.error("Exception : {}", e.getMessage(), e);
throw new ServletException(e.getMessage());
}
}

Choose a reason for hiding this comment

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

어떤 예외가 발생할 수 있고 여기서 예외를 잡아주는 이유가 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

기존 코드의 DispatcherServlet에서 예외를 로깅해주고 Servlet예외로 바꿔서 던져주는 것 때문에 메인 로직이 덜 보인다고 생각되어 필터에서 해주도록 옮겼슴돠!
아마 뷰를 그리는데 실패하거나... 덩덩 수많은 예외가 잇을수 잇겟습니다....

Choose a reason for hiding this comment

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

DispatcherServlet 에서 발생하는 예외를 Filter 에서 잡아준다고 이해하면 될까요...?

Copy link
Author

Choose a reason for hiding this comment

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

네엥~ 지금 하는 작업이 로깅바께 없는것 같아서 필터에서 잡아주는것이 더 낫겟다고 판단했스영

Comment on lines +45 to +49
if (handler.isEmpty()) {
response.setStatus(HttpServletResponse.SC_NOT_FOUND);
render(new ModelAndView(new JspView("/404")), request, response);
return;
}

Choose a reason for hiding this comment

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

404처리 👍

import java.util.Map;

public class JsonView implements View {

private static final ObjectMapper mapper = new ObjectMapper();

Choose a reason for hiding this comment

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

저도 처음에는 ObjectMapperstatic 으로 두려고했었는데요 하지만 ObjectMapper 는 멀티 쓰레드 환경에서 Thread-Safe 하지 않다고 합니다. 이는 setConfig(), setDateFormat() 등의 객체 설정을 바꾸는 메서드가 존재해서입니다!

ObjectMapper 를 통해서 ObjectWriter, ObjectReader 만 따로 뽑을 수 있더라구요!
두 객체는 Thread-Safe 하다고 합니다! 추후에는 이런것도 적용해볼 수 있을 거같아요!

Copy link
Author

Choose a reason for hiding this comment

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

오 그렇군요!!!!!
덕분에 알아갑니다 감사합니당 !!!

Copy link

@Songusika Songusika left a comment

Choose a reason for hiding this comment

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

안녕하세요 케로
MVC 미션 고생하셨습니다! 🎉
코드가 전체적으로 깔끔해서 리뷰하기 좋았습니다!
몇가지 코멘트 달아서 확인해보시면 좋을것 같아요!

저번 step1에서 했던 이야기는 제가 DM으로 답변 드릴게요!
step1 혹은 step2,3 에 대해서 궁금한점 있으면 DM 주세요!
다음 미션도 화이팅입니다!

@Songusika Songusika merged commit 874ff57 into woowacourse:jyeost Sep 27, 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