[톰캣 구현하기 - 1, 2단계] 로이스(원태연) 미션 제출합니다. #307

TaeyeonRoyce · 2023-09-03T12:11:49Z

안녕하세요, 마코! 리뷰요청 드립니다.

MVC를 학습하면서 만들다 보니 레벨3의 구조를 약간 침범하고 있었다는걸 추후에 알게되었습니다.(HttpRequest, HttpResponse, Controller, ...)
여전히 리팩토링 할 부분이 많아 유지한채로 구현한 뒤, 더 개선해나갈 예정입니다.
이점 참고하여서 리뷰 해주시면 감사하겠습니다!

전체적인 흐름에 대한 플로우차트입니다! (~~악필입니다~~)

- 다중 assertions 방식에서 chaining 방식으로 변경 - logger 사용 - 람다 내부에서 메서드 호출이 한번만 발생하도록 변경

aak2075

안녕하세요 로이스~ 마코입니다!

빨리 리뷰해드리고 싶었는데 미션도 진행하고 리뷰가 생각보다 쉽지 않네요😅

벌써부터 MVC를 멋지게 구현하신것을 보고 많이 배웠습니다👍👍

자잘하게 고려하실만한 부분들이나 궁금한 부분들이 있어서 코멘트 남겼으니 확인해주세요!

aak2075 · 2023-09-04T07:44:31Z

study/src/test/java/study/FileTest.java

     */
    @Test
    void resource_디렉터리에_있는_파일의_경로를_찾는다() {
        final String fileName = "nextstep.txt";

        // todo
-        final String actual = "";
+        final URL resource = getClass().getClassLoader().getResource(fileName);


어떤 과정으로 resource를 찾아내나요?

동적으로 클래스를 포함한 자바 파일들을 메모리에 로딩하는 ClassLoader에서 해당하는 자원을 찾아 URI를 반환합니다. (이때, 최상위 BootLoader까지 위임하여 찾으므로 절대경로를 사용해야 하는 것 같습니다!
그리고 JVM에 로딩한 ClassLoader를 얻기 위해 FileTest.getClass().getClassLoader()를 사용한 것 같습니다..!

aak2075 · 2023-09-04T07:44:54Z

study/src/test/java/study/IOStreamTest.java

@@ -96,27 +99,26 @@ class OutputStream_학습_테스트 {
             * try-with-resources를 사용한다.
             * java 9 이상에서는 변수를 try-with-resources로 처리할 수 있다.
             */
+            outputStream.close();


try-with-resources를 사용하라는 요구사항을 만족시켜 주시면 좋을 것 같네요👀

README.md

aak2075 · 2023-09-04T08:54:49Z

tomcat/src/main/java/org/apache/coyote/http11/mvc/ControllerMapping.java

+
+    private String removeHtmlExtension(final String requestURI) {
+        if (requestURI.endsWith(HTML_EXTENSION)) {
+            return requestURI.substring(0, requestURI.length() - HTML_EXTENSION.length());


다른 부분에서는 requestURI.lastIndexOf(HTML_EXTENSION) 이렇게 사용하셨던데 저는 lastIndexOf가 더 깔끔한 것 같습니다

감사합니다! 일관성이 많이 흐트려저있네요. 수정했습니다!

aak2075 · 2023-09-04T10:30:22Z

tomcat/src/main/java/org/apache/coyote/http11/common/HttpCookie.java

+        final Map<String, String> cookieKeyValues = Arrays.stream(cookieHeader.split(COOKIE_SEPARATOR))
+                .map(param -> param.split(COOKIE_DELIMITER))
+                .collect(Collectors.toMap(e -> e[0], e -> e[1]));
+        return new HttpCookie(cookieKeyValues);
+    }


메시지 형식에 맞게 쿠키를 파싱하는 작업은 HttpCookie보다는 HttpRequestMessageReader의 책임에 더 가깝지 않을까요?

쿠키 형식에 맞게 요청을 읽는 역할을 Reader에서 수행 한 뒤, HttpCookie를 생성하는게 자연스러워 보이네요!
좋은 의견 감사합니다.

tomcat/src/main/java/org/apache/coyote/http11/mvc/AbstractController.java

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

aak2075 · 2023-09-04T11:06:58Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpRequestStartLine.java

+import java.util.Map;
+import org.apache.coyote.http11.common.HttpMethod;
+
+public class HttpRequestStartLine {


RFC2616에 따르면
request의 첫 라인을 Request-Line,
response의 첫 라인을 Status-Line,
둘 다를 합쳐서 Start-Line이라고 부릅니다.
현재 이 클래스는 Request에서만 사용되고 있는 것 같은데요, Request에서만 사용되는 첫 라인이라면 HttpRequestLine과 같은 이름이 더 표준 스펙에 가까운 네이밍 같습니다!

참고 자료 감사합니다!
제가 혼동해서 사용하고 있었네요.
마코 말씀대로 HttpRequestLine라는 네이밍이 제 의도에도 맞고 표준 네이밍이군요.
반영하였습니다!

aak2075 · 2023-09-04T11:12:40Z

tomcat/src/main/java/org/apache/coyote/http11/response/formatter/HttpResponseMessageWriter.java

+    private static String parseResponseHeaders(final HttpHeaders httpHeaders) {
+        return httpHeaders.getHeaders().entrySet().stream()
+                .map(entry -> String.join(": ", entry.getKey(), entry.getValue() + " "))
+                .collect(Collectors.joining("\r\n"));


문제가 되지는 않지만 login에 성공해서 302 Found로 redirect하는 경우에 어디선가 Content-Length가 잘못 담겨 반환되는 경우가 있네요.

오류 확인하고 수정했습니다!

tomcat/src/test/java/org/apache/coyote/http11/common/HttpCookieTest.java

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

HttpRequestStartLine -> HttpRequestLine

TaeyeonRoyce

마코, 꼼꼼함이 조금 부족한 코드였는데 잘 읽어주시고 좋은 리뷰 남겨주셔서 감사합니다!
남겨 주신 리뷰는 간단한 코멘트와 함께 반영 완료하였습니다!

그리고 말씀해주신 resource 찾는 방법에 대해 덕분에 학습할 수 있었습니다. 감사해요!

TaeyeonRoyce · 2023-09-04T12:31:46Z

tomcat/src/main/java/org/apache/coyote/http11/common/HttpHeaders.java

+        return new HttpHeaders(headers);
+    }
+
+    public void setHeader(final String header, final String value) {


addHeader가 더 직관적이고 좋네요! 감사합니다

TaeyeonRoyce · 2023-09-04T12:33:08Z

study/src/test/java/study/IOStreamTest.java

@@ -96,27 +99,26 @@ class OutputStream_학습_테스트 {
             * try-with-resources를 사용한다.
             * java 9 이상에서는 변수를 try-with-resources로 처리할 수 있다.
             */
+            outputStream.close();


TaeyeonRoyce · 2023-09-04T12:34:26Z

tomcat/src/main/java/org/apache/coyote/http11/common/HttpMethod.java

+
+    public static HttpMethod from(final String method) {
+        return Arrays.stream(HttpMethod.values())
+                .filter(httpMethod -> httpMethod.name().equals(method.toUpperCase()))


일관성 있게 수정했습니다!

TaeyeonRoyce · 2023-09-04T12:35:56Z

tomcat/src/main/java/org/apache/coyote/http11/mvc/ControllerMapping.java

+
+    private String removeHtmlExtension(final String requestURI) {
+        if (requestURI.endsWith(HTML_EXTENSION)) {
+            return requestURI.substring(0, requestURI.length() - HTML_EXTENSION.length());


감사합니다! 일관성이 많이 흐트려저있네요. 수정했습니다!

TaeyeonRoyce · 2023-09-04T12:38:43Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpRequestStartLine.java

+import java.util.Map;
+import org.apache.coyote.http11.common.HttpMethod;
+
+public class HttpRequestStartLine {


참고 자료 감사합니다!
제가 혼동해서 사용하고 있었네요.
마코 말씀대로 HttpRequestLine라는 네이밍이 제 의도에도 맞고 표준 네이밍이군요.
반영하였습니다!

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

TaeyeonRoyce · 2023-09-04T13:22:13Z

tomcat/src/main/java/org/apache/coyote/http11/response/formatter/HttpResponseMessageWriter.java

+    private static String parseResponseHeaders(final HttpHeaders httpHeaders) {
+        return httpHeaders.getHeaders().entrySet().stream()
+                .map(entry -> String.join(": ", entry.getKey(), entry.getValue() + " "))
+                .collect(Collectors.joining("\r\n"));


오류 확인하고 수정했습니다!

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

TaeyeonRoyce · 2023-09-04T13:46:50Z

tomcat/src/main/java/org/apache/coyote/http11/common/HttpCookie.java

+        final Map<String, String> cookieKeyValues = Arrays.stream(cookieHeader.split(COOKIE_SEPARATOR))
+                .map(param -> param.split(COOKIE_DELIMITER))
+                .collect(Collectors.toMap(e -> e[0], e -> e[1]));
+        return new HttpCookie(cookieKeyValues);
+    }


쿠키 형식에 맞게 요청을 읽는 역할을 Reader에서 수행 한 뒤, HttpCookie를 생성하는게 자연스러워 보이네요!
좋은 의견 감사합니다.

TaeyeonRoyce · 2023-09-04T14:27:01Z

study/src/test/java/study/FileTest.java

     */
    @Test
    void resource_디렉터리에_있는_파일의_경로를_찾는다() {
        final String fileName = "nextstep.txt";

        // todo
-        final String actual = "";
+        final URL resource = getClass().getClassLoader().getResource(fileName);


동적으로 클래스를 포함한 자바 파일들을 메모리에 로딩하는 ClassLoader에서 해당하는 자원을 찾아 URI를 반환합니다. (이때, 최상위 BootLoader까지 위임하여 찾으므로 절대경로를 사용해야 하는 것 같습니다!
그리고 JVM에 로딩한 ClassLoader를 얻기 위해 FileTest.getClass().getClassLoader()를 사용한 것 같습니다..!

sonarcloud · 2023-09-05T06:33:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

aak2075

리뷰 잘 반영해주셨군요 👍

더 많은 도움을 드리고 싶지만 이미 구조도 멋지고 잘 만들어져있어서 제가 크게 도움을 드릴건 없어보이네요ㅎㅎㅎ

더 리팩토링 하시고 싶으신 것도 있으신 것 같아서 머지해드릴테니 리팩토링 + 다음 단계 함께 진행해주시면 될 것 같습니다! 고생하셨습니다~

TaeyeonRoyce added 30 commits September 1, 2023 20:19

docs: 1단계 요구 사항 정리

a7059f1

feat: Http Request Method 종류 추가

5c56749

feat: Http 요청 메세지 시작줄 객체 추가

3e64009

feat: Http 요청 헤더 객체 추가

29024cd

feat: Http 요청 객체 추가

37779e1

feat: Http 요청 메세지 시작줄 생성 검증 테스트 추가

baacb35

feat: Http 요청 메세지 파서 추가

42ab1b8

refactor: 패키지 분리

be5ceea

feat: Http Response 객체 추가

0739d92

feat: Http 요청 처리 구조 구현

e23b14b

feat: index.html 처리 Controller 추가

43e6481

test: 파일 입출력 학습 테스트 작성

18b2b0e

test: 데이터 입출력 학습 테스트 작성

a97a899

feat: 정적 리소스 반환 로직 추가

aac19b1

feat: Http 응답 객체 메세지 변환 로직 추가

d701b96

feat: Http Method 추가

834b4c7

feat: 요청 자원에 맞는 자원 반환 로직 구현

88ace28

feat: css, js와 같은 자원 요청에 대한 처리 구현

c0de791

feat: Query String 파싱 추가

d1db1b5

refactor: HttpRequestMessageReader 파싱 작업 하도록 수정

989ffd2

feat: Query String을 파싱해서 아이디, 비밀번호 검증 로직 추가

b50ddfa

feat: Query String 유무에 따른 페이지 반환 로직 분리

27fc20e

feat: Login 요청을 POST method로 요청하도록 수정

dbd8130

feat: 회원가입 기능 추가

949771a

refactor: Body 읽는 메서드 분리

1f0d99a

refactor: HandlerMapping 에서 mapping되는 Controller 변경

4346b28

feat: 로그인 성공 시 Cookie에 JSessionID 발급 로직 구현

37a9fcd

docs: 요구사항 정리

3fe087b

feat: Session 추가

f4fed2f

refactor: Controller 응답 로직 개선

c5ece7b

TaeyeonRoyce added 9 commits September 3, 2023 01:20

refactor: Redirect 상태 코드 변경

404a454

fix: 출력형식에 맞게 응답 메세지 작성 시 공백 추가

80c86d0

fix: 정적 자원이 아닌 다른 형식에 대한 응답 기능 추가

e8fcea4

fix: 존재하지 않는 페이지 접근시 404페이지와 not found 상태로 응답되도록 수정

356207b

test: 페이지 요청 테스트 추가

27f6f95

refactor: 패키지 경로 및 클래스 이름 수정

a02f80e

feat: 405 상태 코드 응답 추가

6abf103

test: 테스트 추가

346ee70

docs: 요구사항 체크리스트 정리

495319c

TaeyeonRoyce requested a review from aak2075 September 3, 2023 12:11

refactor: code smell 반영

a1116ef

- 다중 assertions 방식에서 chaining 방식으로 변경 - logger 사용 - 람다 내부에서 메서드 호출이 한번만 발생하도록 변경

TaeyeonRoyce self-assigned this Sep 3, 2023

refactor: 사용자 입력 데이터 로깅 제거

26aca4b

aak2075 requested changes Sep 4, 2023

View reviewed changes

aak2075 reviewed Sep 4, 2023

View reviewed changes

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java Outdated Show resolved Hide resolved

TaeyeonRoyce added 9 commits September 4, 2023 21:28

test: 학습 테스트 방식 개선

5f19ea3

refactor: 헤더 메서드 네이밍 수정

d97a854

refactor: 대소문자 관계없이 문자열 비교 방식 수정

9e70384

refactor: 문자열 substring 방식 수정

3f55297

refactor: 표준 용어에 맞게 클래스 이름 수정

dc8bde3

HttpRequestStartLine -> HttpRequestLine

refactor: 예외 타입 수정

3897354

fix: redirect 시 body가 포함되지 않도록 수정

3f86696

fix: 메모리 누수 제거

ef811f1

refactor: 쿠키 파싱 작업 역할 분리

1f93890

TaeyeonRoyce commented Sep 4, 2023

View reviewed changes

aak2075 approved these changes Sep 5, 2023

View reviewed changes

aak2075 merged commit 6f593a7 into woowacourse:taeyeonroyce Sep 5, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1, 2단계] 로이스(원태연) 미션 제출합니다. #307

[톰캣 구현하기 - 1, 2단계] 로이스(원태연) 미션 제출합니다. #307

TaeyeonRoyce commented Sep 3, 2023 •

edited

Loading

aak2075 left a comment

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

aak2075 Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce left a comment

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

TaeyeonRoyce Sep 4, 2023

sonarcloud bot commented Sep 5, 2023

aak2075 left a comment

[톰캣 구현하기 - 1, 2단계] 로이스(원태연) 미션 제출합니다. #307

[톰캣 구현하기 - 1, 2단계] 로이스(원태연) 미션 제출합니다. #307

Conversation

TaeyeonRoyce commented Sep 3, 2023 • edited Loading

aak2075 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaeyeonRoyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 5, 2023

aak2075 left a comment

Choose a reason for hiding this comment

TaeyeonRoyce commented Sep 3, 2023 •

edited

Loading