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단계] 바론(이소민) 미션 제출합니다. #560

Merged
merged 8 commits into from
Sep 27, 2023

Conversation

somsom13
Copy link

안녕하세요 에코! 바론입니다 😸 🙇

드디어 마지막 단계인 step3 이군용..

이번 단계에서는 요구사항에 주어졌던

  1. JspView 구현하기
  2. JsonView 구현하기
  3. asis 패키지 제거 및 기존 레거시 Controller -> 어노테이션 기반의 Controller 로 리팩터링

을 진행했습니다!

그리고 몇 가지 테스트 코드도 추가했습니다

이번 리뷰도 잘 부탁드립니다~~!!! 감사합니다

@echo724 echo724 self-requested a review September 25, 2023 11:30
Copy link

@echo724 echo724 left a comment

Choose a reason for hiding this comment

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

안녕하세요 바론~ 이번 미션도 깔끔하게 잘 작성해주시고 테스트 케이스까지 작성해주셨군요! 데모데이 일정도 있어서 정신 없으셨을텐데 지난 미션 토대로 잘 해주신 것 같습니다. 다만 조금 더 해봤으면 좋겠는 부분이 있어서 한 번 이야기 주고 받으면 좋겠다는 생각에 Request Changes를 했습니다.
예외 처리 관련 부분인데요, 현재 webapp 경로를 보면 500 외에도 404 401 등 여러 예외 페이지들이 정의되어있습니다. 이 페이지들을 사용해서 예외가 터졌을때 적절하게 예외를 처리하려면 어떻게 설계를 해야할까요? 구현 직접 안해주셔도 되고 예외도 404 정도만 생각해보시면 좋을 것 같습니다. ㅎㅎ (사실 이 부분 미션하면서 제일 고민이 많았어서 그렇습니다)


final var requestDispatcher = request.getRequestDispatcher(viewName);
requestDispatcher.forward(request, response);
private void render(final ModelAndView modelAndView, final HttpServletRequest request,
Copy link

Choose a reason for hiding this comment

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

이 부분은 ModelAndView의 역할로 넘겨줘도 되지 않을까 싶습니다! 단순히 ModelAndView에서 Model만 꺼내서 render하니깐요. ModelAndView에서면 내부 필드만 사용하면 되기때문에도 좋을꺼 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

오호 좋습니다!! 불필요한 getter 지우기.. 그것도 말씀해주신대로 내부적으로 view.render 를 호출하도록 하면 되니 아주 쉽게 수정이 되는군용 ㅋ.ㅋ 👍 👍 감사합니다!!

@@ -0,0 +1,8 @@
package webmvc.org.springframework.web.servlet.view.exception;

public class JsonViewParseException extends RuntimeException {
Copy link

Choose a reason for hiding this comment

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

커스텀익셉션 좋습니다~👍

Comment on lines 42 to 44
log.error("Exception : {}", e.getMessage(), e);
throw new ServletException(e.getMessage());
}
Copy link

Choose a reason for hiding this comment

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

사실 이정도만 해줘도 되지만, 만약 존재하지 않는 경로로 요청을 한다면 어떻게 404.jsp를 띄어줄 수 있을까요? 저도 이부분 고민을 좀 해봐서 같이 의견 나눠보면 좋을것 같아요!~

Copy link
Author

Choose a reason for hiding this comment

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

안녕하세요 에코:) 너무 맛있는.. 코멘트였습니다. 🍰

에코가 주신 리뷰를 보면서 app에서 발생하지만 처리되지 않는 예외, DispatcherServlet 처리 과정에서 핸들링을 제대로 하지 못해 발생하는 예외, 그리고 에러 페이지 반환에 대해 많이 고민을 해보았는데요...!

처음에는 간단하게 DispatcherServlet에서 직접 jsp 파일을 읽고, response에 write 하는 방법 또는 302상태코드와 함께 예외 페이지로 redirect를 보내는 방법을 생각했었습니다.

그런데 갑자기 "DispatcherServlet이 app모듈에 개발자가 정의한 페이지를 읽어 반환하는 것이 맞을까?" 라는 의문이 들더라구요!

제 생각은 다음과 같습니다.

  1. DispatcherServlet은 app단에서 발생한 예외를 handle할 책임이 없습니다.
    app(즉 개발자가 비즈니스 로직을 직접 작성하는 부분) 에서 발생하는 예외를 어떻게 처리할 것인지 (어떤 예외 페이지를 반환할지) 에 대해서는 app모듈에서 결정되어야 한다고 생각해요!
    저희가 Spring 을 사용할 때 ExceptionHandler 를 정의하지 않으면 Internal Server Error 라는 메세지가 담긴 기본 오류 페이지 (White Label Page) 가 반환되는 것 처럼, 지금 코드에서도 app에서 예외가 발생하면 DispatcherServlet은 그저 "처리되지 않은 서버 예외 중 하나" 로 취급하고 Servlet 처리할 수 있는 방법으로 핸들링하면 될 것 같아요. app에서 발생한 오류를 app에서 핸들링하는 로직은 지금 controller의 orElse -> redirect:/500.jsp 같은 코드로 처리가 잘 되어있는 것 같구요 :)

  2. DispatcherServlet의 역할에서 발생하는 예외는 DispatcherServlet에서 핸들링하는 것이 맞지만, app 모듈에 작성된 예외 페이지를 반환하는 것 보다 Servlet 또는 Servlet Container가 제공하는 기본 예외 페이지를 반환하는 것이 옳지 않을까? 라는 생각이 듭니다.
    이건 에코가 말씀해주신 "존재하지 않는 경로로 요청을 한 경우" 에 해당하는 상황이 될 것 같아요 :)
    app모듈 (개발자가 직접 개발하는 영역) 외에 DispatcherServlet에서 발생하는 오류 (존재하지 않는 경로로의 요청) 는 저도 에코의 리뷰처럼 당연히 DispatcherServlet에서 처리하는 것이 맞다고 생각해요!
    하지만, 이 때도 app모듈에 있는 개발자가 직접 추가한 예외 페이지를 읽어서 반환하는 것은 DispatcherServlet의 책임이 아니라고 생각합니다.

그래서 결론적으로는 DispatcherServlet에서 적절한 핸들러를 찾지 못해 발생하는 예외는 sendError 를 통해 404 상태코드와 함께 기본 에러 페이지를 표시하도록 했고, 그 외의 처리되지 않은 오류들 (ex. app단에서 발생했지만 handle 되지 않은 예외)는 500 상태코드와 함께 기본 에러 페이지를 표시하도록 구성해보았어요.

에코는 어떤 방향으로 고민을 하셨고, 어떻게 해결하셨는지 의견을 들어볼 수 있으면 너무 좋을 것 같아요!
구구절절 긴 글 읽느라 고생많으셨습니다 😅 💦 너무 재밌는 코멘트 감사합니다 😸

Copy link

Choose a reason for hiding this comment

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

ㅎㅎ 좋은 고민거리를 남겨드린 것 같아서 뿌듯하네요! 바론 얘기해주신대로 직접 jsp 페이지를 Dispatcher 서블릿에서 반환하는 것은 의존관계가 맞지 않는 것 같아요! 저 또한 Dispatcher 서블릿에서는 sendError로 상태코드만 전달하도록 하는 형식으로 구현했습니다! 저도 의존관계를 생각하면서 예외를 처리해주려고 하니 어렵더라구요 ㅜ 그래도 깊은 고민과 생각을 하신 것 같아서 다행인것 같습니다!

@somsom13
Copy link
Author

안녕하세요 에코:) 바론입니다!

남겨주신 코멘트들이 아주아주 도움이 많이 되었어요. 감사합니다 ㅎㅎ
특히 에러 페이지 반환에 대해서는 생각도 안해본 내용들을 다시 고민해 볼 수 있었네요 👍

저의 생각을 최대한 자세하게 적다보니 구구절절 코멘트가 되어서.. 읽기 귀찮으실 수도 있을 것 같아요. 하지만 잘 부탁드립니다 ㅎ.ㅎ 🙇 🙇

@echo724
Copy link

echo724 commented Sep 27, 2023

안녕하세요 바론! 다시 한 번 늦은 리뷰 죄송합니다 🙇‍♂️ 지난 미션을 워낙 깔끔하게 잘 작성하셨어서 크게 커멘트 달게 없었네요..! 다음 미션도 화이팅 하시구 추석 연휴도 잘 보내시길 바랄게요~~!

@echo724 echo724 merged commit 1120550 into woowacourse:somsom13 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