[톰캣 구현하기 1, 2단계] 재즈(함석명) 미션 제출합니다. #581
Conversation
| public class CacheControlInterceptor implements HandlerInterceptor { | ||
|
|
||
| @Override | ||
| public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) { | ||
| String cacheControl = CacheControl | ||
| .noCache() | ||
| .cachePrivate() | ||
| .getHeaderValue(); | ||
|
|
||
| response.setHeader(CACHE_CONTROL, cacheControl); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
스프링 단에서 캐시 설정을 위해 WebContentInterceptor 를 제공해주는데, 이를 이용해 보아도 좋을 것 같아욥!
There was a problem hiding this comment.
오 캐시 정책과 관련하여 이미 제공해주는 인터셉터 구현체가 있었군요!!
알려주셔서 감사해요 낙낙 😊
| char[] bodyChars = new char[contentLength]; | ||
| bufferedReader.read(bodyChars, 0, contentLength); | ||
| body.append(bodyChars); |
There was a problem hiding this comment.
contentLength 는 바이트의 길이인데요!
BufferedReader의 read에 전달되는 len은 문자의 길이입니다!
바디에 한글이 담기거나 다른 인코딩 방식을 사용한다면,
바이트의 길이와 문자의 길이가 다를텐데 이에 대해 어떻게 생각하실까요??
There was a problem hiding this comment.
정말 날카로운 지적이네요 👍👍
바이트를 다루는게 익숙치 않다보니 미처 생각하지 못했던 것 같아요.
바이트 단위로 데이터를 읽어올 수 있도록 하고, 인코딩 방식을 직접 명시해서 데이터 손실을 해결 해야할 것 같습니다.
| private static final int VALID_PARTS_LENGTH = 3; | ||
|
|
||
| private final HttpMethod httpMethod; | ||
| private final String uri; |
There was a problem hiding this comment.
1단계에서는 요청 uri에 쿼리 스트링이 넘어오는 부분이 있었는데요!
해당 부분을 생각해서 uri를 객체로 감싸면 어떨까 싶어요!!
물론 2단계로 넘어가면서 쿼리 스트링은 사라졌지만,,
기존에 있었던 기능은 계속 살려놔도 좋지 않을까 생각됩니닷!
물론 현재 요구사항은 모두 만족하고 있기 때문에 넘어가셔도 좋습니닷!
There was a problem hiding this comment.
기존에 있었던 기능은 계속 살려놔도 좋지 않을까 생각됩니닷!
요구사항이 바뀌어도 기존에 있던 기능은 가져가는게 좋겠어요.
현재는 급하게 구현하느라 책임 분리가 엉망인데 다음 단계를 진행하면서 다시 되돌려놓겠습니다 😊
| if (requestParts.length != VALID_PARTS_LENGTH) { | ||
| throw new IllegalArgumentException("올바르지 않은 요청 라인 구조입니다."); | ||
| } |
There was a problem hiding this comment.
꼼꼼한 예외처리 좋네요!!
(저는 안했는데 리팩토링 할 때 체크 해야겠네요 ㅎㅎ)
| public static HttpRequest parse(BufferedReader bufferedReader) throws IOException { | ||
| RequestLine requestLine = new RequestLine(bufferedReader.readLine()); | ||
| RequestHeaders requestHeaders = createRequestHeaders(bufferedReader); | ||
| RequestBody requestBody = createRequestBody(bufferedReader, requestHeaders); | ||
|
|
||
| return new HttpRequest(requestLine, requestHeaders, requestBody); | ||
| } |
There was a problem hiding this comment.
현재 HttpRequest가 BufferedReader를 직접 사용해서 데이터를 읽고 있는데요!
추상화 레벨이나, 책임 관점에서 스트림으로부터 읽는 역할의 객체를 따로 분리하는 건 어떠신가요??
예를들면 성능 문제 등으로 인해 Reader나 InputStream에서 읽어 오는 방식이 바뀔 수도 있을텐데 (버퍼 사이즈 조정 등)
HttpRequest가 영향을 받을 것 같아서요!!
There was a problem hiding this comment.
저도 그 부분에 대해 고민하고 있었는데 제안해주신 이유들이 다 너무 좋은 것 같아요.
구조를 유연하게 가져가면서 변경의 영향을 최소화할 수 있겠네요
다음 단계를 진행하면서 적극 반영해보겠습니다!!
| public static ContentType find(String path) { | ||
| return Arrays.stream(values()) | ||
| .filter(contentType -> path.toLowerCase().endsWith(contentType.getExtension())) | ||
| .findFirst() | ||
| .orElse(HTML); | ||
| } |
There was a problem hiding this comment.
Files.probecontenttype() 를 이용하면 MIME 타입을 구할 수 있습니닷!!
There was a problem hiding this comment.
오 따로 제공해주는 메서드가 있었군요 ㅋㅋㅋ
살펴보니 OS 파일 시스템을 통해 확장자를 파악하고 타입을 추론하는 것 같아요.
일치하는 타입이 존재하지 않으면 null을 반환하는 것과, 파일 시스템과 상호작용으로 IOException을 고려해야 하네요.
현재 방식과 비교해서 고민해보고 다음 단계에 반영해보도록 하겠습니다!!
알려주셔서 감사해요. 배워갑니다 낙낙 😊
| class CookieTest { | ||
|
|
||
| @DisplayName("쿠키 헤더로 쿠키 객체를 생성한다.") | ||
| @Test | ||
| void cookieCreateTest() { | ||
| String cookieHeader = "tasty_cookie=choco; JSESSIONID=jazz"; | ||
| Cookie cookie = new Cookie(cookieHeader); | ||
|
|
||
| Map<String, String> cookies = Map.of( | ||
| "tasty_cookie", "choco", | ||
| "JSESSIONID", "jazz" | ||
| ); | ||
|
|
||
| assertThat(cookies).isEqualTo(cookie.getCookies()); | ||
| } | ||
|
|
||
| @DisplayName("쿠키 객체로 쿠키 헤더를 생성한다.") | ||
| @Test | ||
| void toCookieHeaderTest() { | ||
| String cookieHeader = "tasty_cookie=choco; JSESSIONID=jazz"; | ||
| Cookie cookie = new Cookie(cookieHeader); | ||
|
|
||
| assertThat(cookie.toCookieHeader()) | ||
| .contains("tasty_cookie=choco") | ||
| .contains("JSESSIONID=jazz"); | ||
| } | ||
| } |
| GET, | ||
| HEAD, | ||
| POST, | ||
| PUT, | ||
| PATCH, | ||
| DELETE, | ||
| OPTIONS, | ||
| TRACE; |
There was a problem hiding this comment.
HTTP 메소드에 CONNECT 도 있어서 말씀 드려봅니다!!
다른 부분은 너무 많아서 현재 미션에서는 다 구현할 필요가 없다 생각하지만, 여기는 딱 하나만 추가하면 되어서 말씀 한번 드려보아요 ㅎㅎ
There was a problem hiding this comment.
아이고 지금 다시보니 하나를 뺴먹었네요.😅
찾아주셔서 감사해요 😊
낙낙씌~~~~~ 안녕하세요 😊
많이 기다리게 만들어 죄송해요 ㅠㅠ 😅
리팩토링은 다음 3단계 목표지만, 낙낙씌의 원활한 코드 리뷰를 위해 핸들러 호출 분기처리를 조금 개선해 봤어요.
아마 지금 몇십 줄씩 되는 핸들러 코드는 객체 책임 분리와 추상화를 통해 점차적으로 개선될 것 같아요.
어떤 리뷰든 대환영이에요 ㅎㅎ 낙낙씌에게 많이많이 배워가겠습니다 🤗
낙낙씌랑 이번 미션 같이 진행하게 되어 너무 기대되네요 😎
편할 때 천천히 리뷰주세요!!