Skip to content

Conversation

@seunghye218
Copy link

안녕하세요~ 3단계 리팩터링 제출합니다.

기존에 요청, 응답 객체를 만들어 두었던 걸 리팩터링 해봤고 컨트롤러를 추가하며 테스트 코드도 추가해봤어요. 잘 부탁드립니다~

geoje and others added 30 commits September 5, 2024 11:11
- MimeType 열거형 추가
- 정적 URI를 못찾을 시 404 반환
- getStaticResource(HttpRequest request) -> getStaticResourceResponse(String requestPath)
- CacheControlInterceptor 추가
- ShallowEtagHeaderFilter 사용
- 캐시 max-age 1년 설정
- ETag 적용
- url에 버전 적용
- 요청 본문 파싱 로직 수정
- HttpCookie 객체 구현 및 요청, 응답 객체에 추가
- 컨트롤러 인터페이스 추가
- 정적 자원 불러오는 로직 객체 분리
- 요청 경로애 따른 컨트롤러 맵핑
- 하드 코딩 값 상수화
- 세션 쿠키에 HttpOnly 설정
- DB 인스턴스 생성 및 User id 유효값 저장
- 루트 패스 인덱스 페이지로 변경
@seunghye218 seunghye218 requested a review from jongmee September 12, 2024 03:06
@seunghye218 seunghye218 self-assigned this Sep 12, 2024
Copy link

@jongmee jongmee left a comment

Choose a reason for hiding this comment

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

마크~! 이번 단계도 훌륭하게 구현해주셨네요 👍 배워갑니다 ! 궁금한 거 몇 개 리뷰 달아 놓았어요 :)

import org.apache.catalina.StaticResourceProvider;
import org.apache.coyote.http11.AbstractController;

public class StaticResourceController extends AbstractController {
Copy link

Choose a reason for hiding this comment

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

정적 리소스(html, css, js)는 AbstractController의 모든 행위(doPost, doPatch, doDelete 등)들이 필요하지 않은 것 같은데요 ! AbstractController를 상속하는 것보다 더 좋은 방법이 있을까요 ??

제가 구현하면서 고민한 부분이라 마크의 의견이 궁금합니다!

Copy link
Author

@seunghye218 seunghye218 Sep 12, 2024

Choose a reason for hiding this comment

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

먼저 생각된 부분은 ResourceHandler를 구현하여 특정 URL 패턴에는 '/static' 등을 매칭에서 미리 반환하는 방법도 있을 것 같아요. 두번째는 RequestMapping에서 컨트롤러 맵핑이 안되었을 때, 컨트롤러 로직을 스킵하고 정적 리소스를 응답하고, 세번쨰 방법은 템플릿 엔진처럼 Controller.service 메서드가 View에 관한 값을 리턴하도록 하면 '/template'에 있는 자원을 매칭해 줄 수 있을 것 같아요.

웹 서버는 컨트롤러 없이 정적 자원을 응답할 수 있으니까 2번으로 구현하는 것이 간단하고 톰캣 구현하기 미션에 맞을 거라고 생각이 드네요...! 미아는 어떤 결론을 지으셨는지 궁금하네요!

+) 카탈리나에서 DefaultServlet 에서 다음과 같은 주석이 있어요.

대부분 웹 애플리케이션의 기본 리소스 제공 서블릿으로, HTML 페이지와 이미지와 같은 정적 리소스를 제공하는 데 사용됩니다.
DefaultServlet.java

정적 리소스는 doGet 등의 행위가 필요하다고 볼 수 있을 것 같아요.

Copy link

Choose a reason for hiding this comment

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

오호 톰캣 레퍼런스까지 👍 저는 static resource는 http request로 수정할 일이 없다고 생각했는데 DefaultServletdoPut을 보면 또 그렇게 구현되어 있지는 않네요 🤯

미아는 어떤 결론을 지으셨는지 궁금하네요!

세 가지 방법이나 !! 좋은 의견 감사합니다 🙇🏻‍♀️ 다 일리있는 의견 같아요. 저는 ResourceHandler라는 이름의 클래스가 Controller를 구현해서 service 메소드에서 static resource들을 찾아 HttpResponse에 담아주었습니다. 위에서 언급했듯 정적 리소스들은 doPut, doPost, doDelete 등이 필요하지 않다고 생각해서 AbstractController를 상속 받지 않았어요.

하지만 톰캣은 웹 서버가 아니라 서블릿 컨테이너의 기능을 하니까 마크의 구현 방식도 적절하고 확장하기 쉽다고 생각합니다! 굿

Copy link
Author

Choose a reason for hiding this comment

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

그런 방법도 있겠네요! 꼭 AbstractController를 상속할 필요는 없었는데 말이죠. 좋은 방법 알려주셔서 고마워요😄


@Override
protected void doPost(HttpRequest request, HttpResponse response) {
if (!request.hasParameter(ACCOUNT) || !request.hasParameter(PASSWORD) || !request.hasParameter(EMAIL)) {
Copy link

Choose a reason for hiding this comment

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

controller에서도 검증 👍

public static final String KEY_VALUE_SEPARATOR = "=";

private final Map<String, String> cookies;
private boolean httpOnly;
Copy link

Choose a reason for hiding this comment

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

쿠키의 여러 attribute 중 httpOnly만 따로 저장한 이유가 있을까요?

(나머지 Max-Age 등은 Map<String, String> cookies에 저장되는 것 같네요!)

Copy link
Author

@seunghye218 seunghye218 Sep 13, 2024

Choose a reason for hiding this comment

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

응답의 Set-Cookie 헤더는 여러 개가 있을 수 있고 각각이 하나의 쿠키로 보이더라고요. 그걸 뒤늦게 알아서 httpOnly만 설정했었네요. 다른 설정들도 빼두고 쿠키의 구조를 재설정해야겠네요..!

Copy link

Choose a reason for hiding this comment

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

맞아요! 쿠키를 여러 개 보내려면 Set-Cookie 헤더를 여러 개 써주면 됩니다! 현재 구조에서는 List를 헤더가 가지면 되겠네요 😊

@@ -0,0 +1,8 @@
package org.apache.coyote.http11;
Copy link

Choose a reason for hiding this comment

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

각 위치의 이유가 궁금합니다 ! 🙋🏻‍♀️

  • Controller는 coyote
  • Session은 catalina
  • HttpRequest(등등) 는 techcourse/http

Copy link
Author

@seunghye218 seunghye218 Sep 13, 2024

Choose a reason for hiding this comment

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

Manager 인터페이스가 catalina에 있어 그쪽으로 두었고 나머지는 어디다 둘지 모르겠어서 임의로 두었어요😅 catalina와 coyote의 의미를 더 알아보고 패키지를 다시 검토해볼게요!

+) Controller를 Servlet, SessionManager를 StandardSession으로 상정하고 다른 클래스도 톰캣에 맞게 패키지를 옮겨봤습니다.

Copy link

Choose a reason for hiding this comment

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

오호 덕분에 톰캣을 배워갑니다 👍 👍

Copy link

@jongmee jongmee left a comment

Choose a reason for hiding this comment

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

3단계 구현도 고생하셨습니다 ☺️ 마감일이 얼마 남지 않았고, 요구사항은 다 만족시켜주신 것 같아서 머지하겠습니다 ! 개인적으로 더 진행하고 싶은 리팩토링은 4단계에서 추가해주셔도 좋을 것 같아요!

@jongmee jongmee merged commit c601373 into woowacourse:seunghye218 Sep 13, 2024
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.

3 participants