[톰캣 구현하기 - 3, 4단계] 준팍(박준현) 미션 제출합니다. #407

junpakPark · 2023-09-07T09:11:07Z

안녕하세요 쥬니

3,4단계 적용 후 제출합니다 ㅎㅎ

…글턴 제거

cpot5620

안녕하세요 준팍 !!

토요일 낮에 리뷰 요청 주셨는데, 리뷰가 늦어졌습니다...
축구하느라 정신이 팔려서 그만.. ⚽

코멘트를 모두 반영하실 필요는 없기 때문에, 의견만 남겨주셔도 좋습니다 !
형편 없는(?) 코멘트들이 다수 존재하여, 생각보다 수정할 내용이 없다면 테스트 코드를 작성해보시는 것도 추천드립니다 ㅎㅎ

cpot5620 · 2023-09-10T17:31:16Z

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

 public enum MimeType {



이전 리뷰에서 확인하지 못한 부분이 있었네요.

이전 커밋이라, 코멘트가 안 남겨져서 해당 클래스에 남기는 점 양해 부탁드립니다.
javascript 타입 관련해서 간단하게 읽을 만한 내용이 있는 것 같아서 링크 남깁니다 ~

저도 이 부분에 대해 고민을 해봤었습니다.
확실히 구글이나 네이버는 text/javascript를 사용하더라구요.

그러나 지원이 종료된 IE를 제외한 5대 브라우저(PC 기준)가 모두 application/javascript를 지원하고, IE의 점유율이 0.5% 미만이라는 점에서
표준을 따르는 게 더 유의미하겠다는 판단하에 이와 같이 결정했습니다 ㅎㅎ

cpot5620 · 2023-09-10T17:32:30Z

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

+    public static MimeType from(String fileExtension) {
        return MimeType.valueOf(fileExtension.toUpperCase());
    }


HttpVersion 클래스에서는 스트림을 통해 값을 반환하셨는데, 해당 클래스에서는 valueOf를 사용하셨네요 !
특별한 이유가 있을까요 ?

MimeType은 필드가 아닌 상수의 이름으로 enum을 반환하므로 기본적으로 지원하는 valueOf를 사용할 수 있었으나,
HttpVersion의 경우는 상수의 이름이 아닌 필드값이 일치하는 경우에 enum을 반환하기 때문에 valueOf를 사용할 수 없었습니다!

public enum HttpVersion { HTTP_1_0("HTTP/1.0"), HTTP_1_1("HTTP/1.1"); private final String version; ... }

-> version이 일치하는 경우 해당 Enum을 반환함.

public enum MimeType { HTML("text/html"), CSS("text/css"), JS("text/javascript"), SVG("image/svg+xml"), ICO("image/x-icon"); private final String contentType; ... }

-> contentType이 아닌 enum의 이름과 일치하는 경우 해당 Enum을 반환함.

cpot5620 · 2023-09-10T17:35:04Z

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

+        this.version = name;
+    }
+
+    public static HttpVersion from(final String field) {


메서드 파라미터마다 final 키워드가 존재하는 곳이 있고, 존재하지 않는 곳이 있네요 !

뼈대 코드는 신경쓰지 않았지만, 직접 작성하신 코드들은 통일성을 지켜주세요 !

수정 완료했습니다!

cpot5620 · 2023-09-10T17:37:01Z

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

+    @Override
+    public String toString() {
+        return "HttpCookie{" +
+                "cookies=" + cookies +
+                '}';
+    }


toString이 필요한 이유가 무엇이었을까요 !

디버깅 과정에서 필요하셨던건가요 ? ㅎㅎ

예리하시군요 ㅎㅎ 디버깅 끝났으니 지워주도록 하겠습니다...

cpot5620 · 2023-09-10T17:41:06Z

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

+    public HttpHeaders(Map<String, String> headers, HttpCookie httpCookie) {
+        this.headers = headers;
+        this.httpCookie = httpCookie;
+    }
+
+    public static HttpHeaders createRequestHeaders(Map<String, String> headers) {
+        String cookie = headers.remove("Cookie");
+
+        return new HttpHeaders(headers, HttpCookie.from(cookie));
+    }


생성자와 정적 팩토리 메서드를 혼용하고 있는데요 !

하나로 통일해서 사용하시는 것은 어떨까요 ~?

RequestHeaders랑 ResponseHeaders를 HttpHeaders라는 하나의 클래스로 관리하려다가
RequestHeaders 지워주는 거 깜빡했네요 ㅠㅠ
해당 내용 반영했습니다!

cpot5620 · 2023-09-10T18:15:33Z

tomcat/src/main/java/org/apache/coyote/http11/Renderer.java

+    private static String findResourcePath(String path) {
+        if (path.contains(".")) {
+            return path;
+        }
+        return path + ".html";
+    }


404 관련해서 예외 처리를 해두셨길래, 코멘트 남깁니다 !

만약 사용자가 localhost:8080/index... 이라는 입력을 하게 되면 404가 뜨는 것이 자연스러울 것 같은데요 ! (아닐 수도...)
현재는 ResponseBody가 비어있는 상태로 200 OK 가 내려오고 있습니다.

해당 흐름이 의도하신 흐름일까요 ? 아니라면, 404로 처리해보시는 것은 어떨까요 ?

NotFound로 연결되도록 처리하였습니다 ㅎㅎ

tomcat/src/main/java/org/apache/coyote/http11/Renderer.java

cpot5620 · 2023-09-10T18:21:02Z

tomcat/src/main/java/org/apache/coyote/http11/session/SessionManager.java

 import org.apache.catalina.Manager;

-public class SessionManager implements Manager {
+public enum SessionManager implements Manager {


SessionManager 클래스를 coyote 패키지 아래에 두신 이유가 있을까요 ?

현재 SessionManager를 HttpRequest 내부에 두고 있기에 같은 위치에 두는 게 자연스럽다고 생각했는데
쥬니는 어떻게 하셨나요?

저는 Session은 Http 자체에 조금 더 관련이 있다고 생각해서 coyote 패키지에 두었고, 해당 세션을 관리하는 SessionManager는 어플리케이션 상태(?)를 관리 한다는 느낌이 더 강해서 catalina 패키지에 두었습니당

cpot5620 · 2023-09-10T18:24:17Z

tomcat/src/main/java/nextstep/jwp/controller/DefaultController.java

+import org.apache.coyote.http11.request.HttpRequest;
+import org.apache.coyote.http11.response.HttpResponse;
+
+public class DefaultController implements Controller {


Controller를 구현한 클래스와, AbstractController를 구현한 클래스를 어떤 기준으로 나누셨는지 궁금해요 !

정적 파일을 서빙하는 컨트롤러는 Controller를 구현한게 맞나요 ? (뷰 리졸버 느끼이이임?)
(ViewResolver가 삭제된 이력이 있어서 유추해봤습니다 ㅋ.ㅋ.ㅋ)

일단 제가 Controller와 AbstractController를 나눈 기준은 비즈니스 로직이 필요한지 여부 입니당.
단순히 파일만 서빙하는 경우는 doGet()이나 doPost()를 사용할 필요가 없기 때문에 Controller를 구현했고,
비즈니스 로직이 필요한 경우 (get과 post의 분기가 필요한 경우)에는 AbstractController로 구현했습니다.

정적 파일을 서빙하는 컨트롤러는 ResourceController라는 네이밍으로 coyote 패키지 내부에 위치하고 있습니다.
뷰 리졸버는 Tomcat 미션에 적절하지 않은 네이밍이라고 생각하여 Renderer라는
Renderer라는 네이밍으로 변경하고, 내부에 로직을 더 정리했습니다.
ResourceController라는 네이밍은 좀 더 명시적으로 수정하겠습니당

cpot5620 · 2023-09-10T18:29:33Z

tomcat/src/main/java/org/apache/coyote/http11/controller/RequestMapping.java

+        if (path.contains(".")) {
+            return new ResourceController();
+        }
+        return CONTROLLERS.getOrDefault(path, new NotFoundController());


예외 처리를 하는 방법이 두 가지 정도 있을 것 같은데요.

첫 번째는 준팍이 구현해주신 대로, 각각의 예외 컨트롤러를 만드는 방법이 있을 거고,
두 번째는 커스텀 예외를 만들고, 하나의 예외 핸들러에서 처리해주는 방법이 있을 것 같아요.

개인적인 생각으로는 모든 예외 상황에 항상 정적 파일을 서빙해주기 때문에, 커스텀 예외를 사용하여 하나의 예외 핸들러에서 처리해주는 것이 조금 더 좋을 것 같다는 생각이 드는데요.
위 방법으로 구현하신 이유가 있을까요 ?

해당 부분은 파일이 없을 때 404를 보여주고, 예상치 못한 오류가 터졌을 땐 500을 터뜨리기 위해 이렇게 구성하였는데요,
커스텀 예외를 적용하여 하나의 예외 핸들러에서 처리할 경우엔 해당 오류를 어떤 식으로 분리해야할지 감이 잡히지 않습니다
혹시 쥬니는 어떤 방식을 상정하고 말씀하신걸까요?

저는 Http와 관련된 커스텀 예외 (Not Found, Unauthorized 등)을 만들어두고, 해당 예외 내부에 StatusCode를 갖도록 했어요.
그러면, 하나의 예외 핸들러에서 StatusCode를 토대로 상황에 맞는 핸들링이 가능하니까요 !

정답이 있는 부분은 아니고, 저는 이렇게 구현했다~ 자랑하고 싶어서 써봤습니다 히힛

Taste Good...

sonarcloud · 2023-09-11T12:21:14Z

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

junpakPark · 2023-09-11T12:21:54Z

훌륭한 코멘트들만 존재하여, 좀 더 다각도로 생각할 수 있었네요 ㅎㅎ 감사합니다 👍👍👍

cpot5620 · 2023-09-11T12:52:47Z

  ________                  .___      __      ___.          __                           __    
 /  _____/  ____   ____   __| _/     |__| ____\_ |__       |__|__ __  ____ ___________  |  | __
/   \  ___ /  _ \ /  _ \ / __ |      |  |/  _ \| __ \      |  |  |  \/    \\____ \__  \ |  |/ /
\    \_\  (  <_> |  <_> ) /_/ |      |  (  <_> ) \_\ \     |  |  |  /   |  \  |_> > __ \|    < 
 \______  /\____/ \____/\____ |  /\__|  |\____/|___  / /\__|  |____/|___|  /   __(____  /__|_ \
        \/                   \/  \______|          \/  \______|          \/|__|       \/     \/

junpakPark added 30 commits September 1, 2023 22:49

test: IOStreamTest 완료

757aa2a

test: FileTest 완료

15b7437

test: FileTest path에서 getResource로 방법 변경

4cb9a8c

docs : 요구사항 정리

08af0cb

feat: GET /index.html 응답 기능 구현

aa667c6

feat: css 지원 기능 구현

a721f0b

refactor: 클래스 분리

791fd98

feat: QueryString 파싱 기능 구현

41582a5

refactor: FileExtension 클래스 분리

782c35b

refactor: HttpStatus 클래스 분리

4a06f65

refactor: RequestHandler 클래스 분리

af1bc42

refactor: RequestHandler Path 로직 수정

03a14a7

feat: favicon 추가

8be2ead

feat: HTTP 302 기능 구현

030dd63

refactor: 개행 문자 lineSeparator()

aa6b77c

refactor: 클래스 분리

b7a3fda

feat: Post method 기능 구현

a8228d2

feat: 회원 가입 후 리다이렉트 완료

08ab904

refacotr: lineSeperator() "\r\n"로 수정

dd29b24

test: cache 학습테스트 완료

fda4afa

test: cache 학습테스트 완료

b4c483d

refactor: 피드백 반영

727135f

refactor: QueryStrings 일급 컬렉션 구현

04f7da6

feat: HttpCookie 구현

7fe6019

refactor: 네이밍 변경

08b0e2f

chore: 패키지 분리

7b7d82a

feat: RequestHeaders에 HttpCookie 추가

a21062c

refactor: 메서드 분리

ce3b483

refactor: HandlerMapping 상수화

9cf7608

feat: 쿠키 값을 스트링으로 반환하는 메서드 구현

7d8ddae

junpakPark added 5 commits September 7, 2023 16:57

fix: 로그인 실패시 401.html 리다이렉트

2407c89

refactor: SessionManager 싱글턴 적용

56ee2a9

test: cache 학습 테스트

10c25e9

fix: 401 리다이렉트 오류 수정

fb494d3

refactor: SessionManager Enum Singleton 적용

a29c816

junpakPark requested a review from cpot5620 September 7, 2023 09:11

junpakPark self-assigned this Sep 7, 2023

junpakPark removed the request for review from cpot5620 September 7, 2023 10:08

junpakPark and others added 14 commits September 7, 2023 22:21

chore: 코드 커버리지 확인을 위한 build.yml 수정

42f9516

refactor: 네이밍 변경, 싱글턴 적용, collection 변경

189b083

fix: GET 요청 시 Login 및 Register 기능 삭제

fec6b10

refactor: 불필요 메서드 삭제

ecf241a

refactor: statusCode 타입 변경

c09714c

refactor: 불필요 객체 삭제

d8463a2

refactor: ResponseEntity 빌더패턴 적용

c0ba83b

feat: 동시성 확장하기

07d5d93

test: 학습테스트 Thread 학습 테스트 완료

2691b61

feat: Connector Thread pool 적용

a1a5dc5

chore: 코드 커버리지 체크 제거

4b1cfd0

refactor: ResponseEntity 제거 및 Controller 인터페이스 service 메서드 void 적용, 싱…

265a9e7

…글턴 제거

feat: HttpResponseParser 객체 구현

e7ac707

refactor: 코드 스멜 제거

fe30e41

cpot5620 requested changes Sep 10, 2023

View reviewed changes

junpakPark added 3 commits September 11, 2023 20:27

refactor: 피드백 반영

45cb835

refactor: 피드백 반영

8f06001

fix: 파일을 찾을 수 없을 시 NotFound 연결

ba6a311

cpot5620 merged commit 94761d0 into woowacourse:junpakpark Sep 11, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 준팍(박준현) 미션 제출합니다. #407

[톰캣 구현하기 - 3, 4단계] 준팍(박준현) 미션 제출합니다. #407

junpakPark commented Sep 7, 2023 •

edited

Loading

cpot5620 left a comment

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023 •

edited

Loading

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 11, 2023

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023 •

edited

Loading

cpot5620 Sep 10, 2023

junpakPark Sep 11, 2023

cpot5620 Sep 11, 2023

junpakPark Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

junpakPark commented Sep 11, 2023

cpot5620 commented Sep 11, 2023

[톰캣 구현하기 - 3, 4단계] 준팍(박준현) 미션 제출합니다. #407

[톰캣 구현하기 - 3, 4단계] 준팍(박준현) 미션 제출합니다. #407

Conversation

junpakPark commented Sep 7, 2023 • edited Loading

cpot5620 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junpakPark Sep 11, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junpakPark Sep 11, 2023 • edited Loading

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 11, 2023

junpakPark commented Sep 11, 2023

cpot5620 commented Sep 11, 2023

junpakPark commented Sep 7, 2023 •

edited

Loading

junpakPark Sep 11, 2023 •

edited

Loading

junpakPark Sep 11, 2023 •

edited

Loading