[톰캣 구현하기 - 3, 4단계] 라온(문채원) 미션 제출합니다. #433

mcodnjs · 2023-09-09T13:33:31Z

안녕하세요 에밀! 돌아왔습니다 ㅋㅋ!

빠르게 머지해주신 덕분에 3, 4단계 생각할 시간이 많았는데요
톰캣의 coyote와 catalina의 역할에 대해서 많이 고민하였습니다!
현재 구조가 상당히 추상적이라 .. 기존 톰캣의 구조와 비교하는 것도 힘들더라구요 ..
그래서 그냥 제 마음대로 구현해봤습니다 ^^ ; 어색한 부분이 있다면 .. 따끔한 피드백 부탁드리겠습니다 🙇‍♂️🙇‍♂️

추가적으로 HttpResponse 의 Content-Type 을 정하는 ResourceContentTypeResolver 클래스를 생성하였는데요,
이해하는데 어려움이 있으실거 같아 아래 커멘트에 추가 설명 달아놓았습니다!

이번에도 잘 부탁드립니다 ~! 감사합니다 🙇‍♂️

mcodnjs · 2023-09-09T19:29:42Z

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

+
+public class ResourceContentTypeResolver {
+
+    public String getContentType(final HttpHeaders headers, final String resourceName) {


해당 클래스의 메서드는 들어온 요청의 헤더와 확장자를 파싱하여 response의 Content-Type 을 결정하는 메서드입니다!

로직은 아래와 같습니다 ~!

Request Header의 Content-Type 의 첫번째 value를 가져온다.

해당 value에 지원하는 MediaType이 있다면 바로 반환한다.

지원하는 MediaType이 없다면, 1번 과정을 Accept 헤더로 동일하게 반복한다.

지원하는 MediaType이 없다면, 요청 리소스의 확장자를 파싱하여 MediaType의 subType과 비교하여 MediaType을 반환한다.

지원하는 MediaType이 없다면, null이 반환되고, HttpException이 발생한다.

아래의 경우를 예시로 들면, Content-Type 헤더가 존재하지 않으므로
Accept 헤더의 첫번째 값인 text/html을 나타내는 MediaType(TEXT_HTML)을 찾아 String(text/html)으로 반환합니다!

GET /index.html HTTP/1.1 Accept: text/html,application/xhtml+xml,application/xml;q=0.9

Content-Type의 경우 해당 Http 메시지의 타입을 나타내는 것이라 잘못된 Content-Type을 보내는 경우가 있을 거 같아요.
예를 들어

Http Request
POST /login HTTP/1.1
Content-Length: 12
Content-Type: application/x-www-form-urlencoded

account=gugu&password=password

Http Response
HTTP/1.1 200 OK
Content-Length: 3300
Content-Type: application/x-www-form-urlencoded
...
실제로는 Content-Type: text/html이 가야하는데
이렇게 응답이 가게 됩니다.

요청을 다음처럼 보냈을 때

curl --location --request GET 'localhost:8080/login.html' \ --header 'Content-Type: application/x-www-form-urlencoded' \ --data-urlencode 'heelo=world'

응답이 다음처럼 오게 됩니다.
헤더

바디

아아 .. 제 능지였습니다 ..
왜 로직을 짜면서 Content-Type을 봐야한다고 생각했는지 모르겠네요 😅 Content-Type 제거하였습니다!

CFalws

라온 리팩토링 잘해주셔서 수월하게 리뷰 했습니다.
많이 바쁘실텐데 리뷰 남긴 것 중에 하시고 싶으신 것만 선택적으로 반영하셔도 됩니다.
그리고 다른 의견 혹은 라온의 원래 의도, 부담없이 댓글 남겨주시면, 확인하도록 하겠습니다 ^_^
고생 많으셨습니다👍

CFalws · 2023-09-10T11:06:29Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


Controller 인터페이스에서 throws 를 하고 있는데요 해당 예외를 잡아서 처리하는 부분이 HttpProcessor에도 있고 HttpController에도 있는데요
예외가 던져지는 경우는 아래로 보입니다.

해당 요청을 처리할 컨트롤러가 없을 때

해당 타겟이 해당 메서드를 지원하지 않을 때

위의 경우는 404 NOT FOUND
아래의 경우는 405 METHOD NOT ALLOWED를 반환해야하는데
현재는 같이 처리되고 있는 걸로 보입니다.

두 가지 경우를 분리하는 것은 어떨까요?

아하 그렇네요 .. 현재는 HttpController의 service() 메소드 내부에서 모두 컨트롤되서 404로 반환될거 같아요
이 부분에 대해서 HttpException에 status필드와 msg 필드를 추가하여 catch 내부에서 컨트롤 하였습니다!

CFalws · 2023-09-10T11:23:54Z

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

+        for (String temp : new String(buffer).split("&")) {
+            String[] value = temp.split("=");
+            if (value.length >= 2) {
+                body.put(value[0], URLDecoder.decode(value[1], StandardCharsets.UTF_8));
+            }
+        }


application/x-www-application-form-urlencoded 타입의 문자열을 파싱하는 로직이 queryString 부분과 해당 부분에 중복으로 존재하는 것 같은데
독립적인 부분으로 분리하는 건 어떨까요?

허허 .. 천재신가요 에밀?

CFalws · 2023-09-10T11:27:59Z

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

+        return httpHeaders;
+    }
+
+    private Map<String, String> parseRequestBody(final String contentLengthHeader) throws IOException {


request body로 현재 전달되는 데이터가 하나의 형식 뿐이지만
다른 타입의 body가 요청데이터로 전달된다면 for 문을 타지 않아 빈 Map이 전달될 것 같은데요
해당 문제를 방지하는 것도 좋아보입니다.

물론 이건 요구사항이 아니기에(다른 타입의 요청이 들어오지 않기에) 쓰루하셔도 됩니다~

1, 2 단계 미션에서 자세한 설명 요청 주셔서 여기에 달도록 하겠습니다.

예를 들어
POST /uri/example HTTP/1.1
Content-Type: application/json
Content-Length: 14

{"key": "value"}

위와 같은 요청이 들어왔을 때 empty 상태인 Map이 전달되어서 body에 값이 있지만 마치 없는 것처럼 동작하는 버그가 발생할 수 있다는 의미였습니다

아하 ... 완전히 간파해버렸습니다. 제가 Content-Type에 대해 무지했네요 ..
해당 부분은 현재 미션에서는 말씀 주신 것처럼 Content-Type: application/x-www-form-urlencoded로만 요청이 오기에 문제가 없어 보였네요 ..! 다만, 이 부분은 ... 시간 관계상 .. 추후 .. TODO로 남겨놓겠습니다 ㅎㅎ 감사합니다 👍

CFalws · 2023-09-10T11:46:35Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    protected void doGet(final HttpRequest request, final HttpResponse response) throws Exception {
+    }
+
+    protected void doPost(final HttpRequest request, final HttpResponse response) {
+    }
+}


doGet 메서드에서 Excpetion을 던지도록 하셨군요~
해당 메서드를 사용하는 사람 입장에서 doGet이 던지는 예외가 무엇일지 어떤 예외를 잡아서 처리를 해야할지 모호하다는 생각이 드는데요
예외를 내부에서 처리하거나 예외 전환을 통해 조금 더 디테일한 정보를 주는 방식 등은 어떻게 생각하실까요?

doMethod() 에서 던지는 Exception은 IOException 으로 명시적으로 지정해주었습니다 ~

CFalws · 2023-09-10T11:58:58Z

tomcat/src/main/java/org/apache/catalina/session/SessionManager.java

 public class SessionManager implements Manager {

-    private static final Map<String, Session> SESSIONS = new HashMap<>();
+    private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();


인스턴스 메서드에서 static 필드에 접근하고 있네요
혹시 해당 방식으로 구현하신 이유가 있을까요?

싱글톤으로 구현하고 싶어 해당 방법을 사용했습니다 ..!

private static final Map<String, Sessions> SESSIONS = new ConcurrentHashmap<>(); -> private final Map<String, Sessions> SESSIONS = new ConcurrentHashmap<>();

위와 같이 수정해도 해당 SESSIONS 객체가 런타임에 유일하다는 것은 보장되는 것 같습니다
어떤 방식을 써도 상관없어서 취향대로 해주시면 될 것 같네요 ~

CFalws · 2023-09-10T12:02:56Z

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

+        try {
+            final HttpRequestLine requestLine = parseRequestLine();
+            final HttpHeaders requestHeaders = parseRequestHeader();
+            if (requestHeaders.getHeaderValue(CONTENT_LENGTH) != null) {


Nullable한 값을 반환하는 메서드의 경우에는 Optional을 반환하는 것도,
해당 메서드를 사용하는 사람에게 Null 처리를 강제할 수 있다는 점에서 좋은 것 같습니다 ^_^

참고로 구구의 리뷰인데요
null을 반환하면 이런 문제가 있을 수 있다고 합니다.

참고 리뷰까지 감사합니다~
Map의 경우 존재하지 않는 key에 대한 null 반환은 자연스럽다고 생각하여 이부분은 그대로 유지했구요
null을 명시적으로 반환하는 로직만 Optional을 반환하도록 변경하였습니다!

CFalws · 2023-09-10T12:14:55Z

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

+import static org.apache.coyote.http11.common.HttpHeaderType.ACCEPT;
+import static org.apache.coyote.http11.common.HttpHeaderType.CONTENT_TYPE;
+
+public class ResourceContentTypeResolver {


해당 클래스는 상태가 없는데 인스턴스화를(유틸성 클래스가 아닌) 하신 이유가 궁금합니다~

상태가 없는 유틸성 클래스가 맞습니다!
static 메서드로 수정하였는데, 유틸성 클래스가 static 메서드를 사용하라는 의미가 맞나요 ?!

넵 유틸성 클래스라면 불필요한 인스턴스화를 막기 위해 모든 메서드를 static으로 두고, private 생성자를 통해 인스턴스화를 컴파일 타임에 막는 것도 좋을 것 같아요

CFalws · 2023-09-10T12:34:19Z

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

+
+public class ResourceContentTypeResolver {
+
+    public String getContentType(final HttpHeaders headers, final String resourceName) {


Content-Type의 경우 해당 Http 메시지의 타입을 나타내는 것이라 잘못된 Content-Type을 보내는 경우가 있을 거 같아요.
예를 들어

Http Request
POST /login HTTP/1.1
Content-Length: 12
Content-Type: application/x-www-form-urlencoded

account=gugu&password=password

Http Response
HTTP/1.1 200 OK
Content-Length: 3300
Content-Type: application/x-www-form-urlencoded
...
실제로는 Content-Type: text/html이 가야하는데
이렇게 응답이 가게 됩니다.

요청을 다음처럼 보냈을 때

curl --location --request GET 'localhost:8080/login.html' \ --header 'Content-Type: application/x-www-form-urlencoded' \ --data-urlencode 'heelo=world'

응답이 다음처럼 오게 됩니다.
헤더

바디

CFalws · 2023-09-10T12:49:51Z

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

+            httpResponse.addHeader(CONTENT_TYPE, TEXT_HTML.stringifyWithUtf());
+            httpResponse.addHeader(LOCATION, "/401.html");
+            httpResponse.setStatusCode(FOUND);
+            return;


Body가 없으면 Content-Type 헤더를 설정하지 않아도 되는 걸로 알고 있습니다

아앗 .. 그렇죠 .. 감사합니다 👍
필요없는 헤더 타입은 다른 로직에서도 모두 제거하였습니다!

CFalws · 2023-09-10T13:10:16Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    @Override
+    public void service(final HttpRequest httpRequest, final HttpResponse httpResponse) {
+        try {
+            if (!canHandle(httpRequest)) {
+                throw new HttpException("There is no controller to handle.");
+            }
+            final String method = httpRequest.getMethod();
+            if ("GET".equals(method)) {
+                doGet(httpRequest, httpResponse);
+            }
+            if ("POST".equals(method)) {
+                doPost(httpRequest, httpResponse);
+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


현재는 해당 uri에 HTTP 메서드가 허용되는지 두 군데에서 관리되고 있습니다.
canHandle과 doGet, doPost override가 그것인데요.

요청
GET /login HTTP/1.1

위와 같은 요청을 처리하는 컨트롤러를 만든다고 가정했을 때,
canHandle이 true를 반환하고 doGet을 override 하지 않으면 doGet 메서드가 아무일도 하지 않게 됩니다.
반대로 canHandle은 false를 반환했는데 doGet이 override 되어 있다면 예외를 던지고 NOT_FOUND를 발생시키게 되는데요.

톰캣 같은 경우에는 doGet 같은 메서드 오버라이드 여부를 통해 처리를 하는 것으로 알고 있습니다.
이처럼 관리 포인트를 한 가지로 만들 수 있는 방법도 생각해보시면 좋을 거 같습니다.
(이 리뷰는 그냥 생각해보실만한 포인트라 남겨둔 것이고 그냥 쓰루하셔도 됩니다.)

톰캣 같은 경우에는 doGet 같은 메서드 오버라이드 여부를 통해 처리를 하는 것

오 이 부분은 몰랐습니다. 저는 DispatcherServlet에서 처리하는지 알았어요!
오버라이드 여부로 처리하면 관리 포인트를 한 가지로 만들 수 있는 것도 좋네요!

현재 상황에서 간단하게 canHandle() 메서드 없이 이를 체크할 수 있는 방법으로는
HttpController 내부의 doMethod()가 default로 405를 던지게 만들 수 있는거 같은데요,
(라고 썼는데, 직접 톰캣의 구현 방식을 보니 그렇게 구현하고 있었네요 ㅎㅎ)

다만, canHandle()을 구현한 이유는
DispatcherServlet은 @Controller와 @RequestMapping 어노테이션을 통해 매핑 가능한 URI를 찾는데요,
현재는 그 방식에 한계가 있어 Controller와 RequestMapping을 찾는 역할을 각 컨트롤러에게 책임을 부여하기 위해 사용했어요!

물론 Map.of("/login", LoginController) <- 이렇게 매핑하는 방식도 있겠지만,
이 방식대로라면 Controller에 무조건 하나의 매핑된 URI/login만 처리할 수 있을거 같아서 ..
~~List로 가지고 있어도 되겠네요 ㅎㅎ..~~

이 둘을 모두 챙기고자 ... 선택한 방식은

컨트롤러의 canHandle() 메서드에서는 /index1, /index2 등 URI를 핸들링할 수 있는지 구현하고,

IndexController의 doGet() 메소드에서 doGetIndex1(), doGetIndex2() 등을 호출하는 방법으로 구현하였습니다 ..

추가적으로, RequestMapping.getController() 에서 이미 canHandle()로 검증하고 있기 때문에 service() 메서드에서 검증할 필요가 없었네요 ..

마음에 안들지만 .. canHandle()에서는 매핑할 수 있는 URI만을 검증하고 싶었습니다 ㅠㅠ
혹시 더 좋은 구조나 방법이 있다면 말씀 부탁드리겠습니다!

에밀 덕에 관련해서 조금 찾아봤는데요!
이미 아시는 내용일거 같지만, 공부한 내용을 조금 공유하자면,

DispatcherServlet은 Spring에서 구현하고 있고, HttpServlet 까지 Tomcat에서 구현하고 있네요
사진은 구현/상속 관계 순입니다!

말씀 주신 것처럼 Tomcat의 Http 관련 최상단의 클래스인 HttpServlet에서 default로 405를 처리해주고 있네요!

protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { String msg = lStrings.getString("http.method_get_not_supported"); sendMethodNotAllowed(req, resp, msg); }

덕분에 많은 정보 알아갑니다 🙇‍♂️👍

수정하신 방식 좋은데요? 👍
(추신 왜인지 모르겠지만 예전 코드에 코멘트가 달려버렸네요ㅋㅋ)

mcodnjs

안녕하세요, 에밀
정말정말 꼼꼼하고 자세한 리뷰 감사합니다! 덕분에 정말 많이 배웠습니다 ..
말씀 주신 부분은 모두 커멘트 달고 수정하였는데요,
추가적으로 리뷰하실 사항 있으시면 언제든지 마음껏 달아주셔도 됩니다 🙇‍♂️
아래 사진은 .. 저 이해하려고 만들었다가 도움이 되실까 해서 커멘트에도 참고합니다 ㅎㅎ

mcodnjs · 2023-09-11T04:57:23Z

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

+            httpResponse.addHeader(CONTENT_TYPE, TEXT_HTML.stringifyWithUtf());
+            httpResponse.addHeader(LOCATION, "/401.html");
+            httpResponse.setStatusCode(FOUND);
+            return;


아앗 .. 그렇죠 .. 감사합니다 👍
필요없는 헤더 타입은 다른 로직에서도 모두 제거하였습니다!

mcodnjs · 2023-09-11T05:54:54Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    @Override
+    public void service(final HttpRequest httpRequest, final HttpResponse httpResponse) {
+        try {
+            if (!canHandle(httpRequest)) {
+                throw new HttpException("There is no controller to handle.");
+            }
+            final String method = httpRequest.getMethod();
+            if ("GET".equals(method)) {
+                doGet(httpRequest, httpResponse);
+            }
+            if ("POST".equals(method)) {
+                doPost(httpRequest, httpResponse);
+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


톰캣 같은 경우에는 doGet 같은 메서드 오버라이드 여부를 통해 처리를 하는 것

오 이 부분은 몰랐습니다. 저는 DispatcherServlet에서 처리하는지 알았어요!
오버라이드 여부로 처리하면 관리 포인트를 한 가지로 만들 수 있는 것도 좋네요!

현재 상황에서 간단하게 canHandle() 메서드 없이 이를 체크할 수 있는 방법으로는
HttpController 내부의 doMethod()가 default로 405를 던지게 만들 수 있는거 같은데요,
(라고 썼는데, 직접 톰캣의 구현 방식을 보니 그렇게 구현하고 있었네요 ㅎㅎ)

다만, canHandle()을 구현한 이유는
DispatcherServlet은 @Controller와 @RequestMapping 어노테이션을 통해 매핑 가능한 URI를 찾는데요,
현재는 그 방식에 한계가 있어 Controller와 RequestMapping을 찾는 역할을 각 컨트롤러에게 책임을 부여하기 위해 사용했어요!

물론 Map.of("/login", LoginController) <- 이렇게 매핑하는 방식도 있겠지만,
이 방식대로라면 Controller에 무조건 하나의 매핑된 URI/login만 처리할 수 있을거 같아서 ..
~~List로 가지고 있어도 되겠네요 ㅎㅎ..~~

이 둘을 모두 챙기고자 ... 선택한 방식은

컨트롤러의 canHandle() 메서드에서는 /index1, /index2 등 URI를 핸들링할 수 있는지 구현하고,

IndexController의 doGet() 메소드에서 doGetIndex1(), doGetIndex2() 등을 호출하는 방법으로 구현하였습니다 ..

추가적으로, RequestMapping.getController() 에서 이미 canHandle()로 검증하고 있기 때문에 service() 메서드에서 검증할 필요가 없었네요 ..

마음에 안들지만 .. canHandle()에서는 매핑할 수 있는 URI만을 검증하고 싶었습니다 ㅠㅠ
혹시 더 좋은 구조나 방법이 있다면 말씀 부탁드리겠습니다!

mcodnjs · 2023-09-11T05:58:10Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


아하 그렇네요 .. 현재는 HttpController의 service() 메소드 내부에서 모두 컨트롤되서 404로 반환될거 같아요
이 부분에 대해서 HttpException에 status필드와 msg 필드를 추가하여 catch 내부에서 컨트롤 하였습니다!

mcodnjs · 2023-09-11T07:41:46Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    @Override
+    public void service(final HttpRequest httpRequest, final HttpResponse httpResponse) {
+        try {
+            if (!canHandle(httpRequest)) {
+                throw new HttpException("There is no controller to handle.");
+            }
+            final String method = httpRequest.getMethod();
+            if ("GET".equals(method)) {
+                doGet(httpRequest, httpResponse);
+            }
+            if ("POST".equals(method)) {
+                doPost(httpRequest, httpResponse);
+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


에밀 덕에 관련해서 조금 찾아봤는데요!
이미 아시는 내용일거 같지만, 공부한 내용을 조금 공유하자면,

DispatcherServlet은 Spring에서 구현하고 있고, HttpServlet 까지 Tomcat에서 구현하고 있네요
사진은 구현/상속 관계 순입니다!

말씀 주신 것처럼 Tomcat의 Http 관련 최상단의 클래스인 HttpServlet에서 default로 405를 처리해주고 있네요!

protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { String msg = lStrings.getString("http.method_get_not_supported"); sendMethodNotAllowed(req, resp, msg); }

덕분에 많은 정보 알아갑니다 🙇‍♂️👍

mcodnjs · 2023-09-11T08:16:04Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    protected void doGet(final HttpRequest request, final HttpResponse response) throws Exception {
+    }
+
+    protected void doPost(final HttpRequest request, final HttpResponse response) {
+    }
+}


doMethod() 에서 던지는 Exception은 IOException 으로 명시적으로 지정해주었습니다 ~

mcodnjs · 2023-09-11T14:34:21Z

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

+import static org.apache.coyote.http11.common.HttpHeaderType.ACCEPT;
+import static org.apache.coyote.http11.common.HttpHeaderType.CONTENT_TYPE;
+
+public class ResourceContentTypeResolver {


상태가 없는 유틸성 클래스가 맞습니다!
static 메서드로 수정하였는데, 유틸성 클래스가 static 메서드를 사용하라는 의미가 맞나요 ?!

mcodnjs · 2023-09-11T14:39:37Z

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

+
+public class ResourceContentTypeResolver {
+
+    public String getContentType(final HttpHeaders headers, final String resourceName) {


아아 .. 제 능지였습니다 ..
왜 로직을 짜면서 Content-Type을 봐야한다고 생각했는지 모르겠네요 😅 Content-Type 제거하였습니다!

mcodnjs · 2023-09-11T15:10:51Z

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

+        return httpHeaders;
+    }
+
+    private Map<String, String> parseRequestBody(final String contentLengthHeader) throws IOException {


아하 ... 완전히 간파해버렸습니다. 제가 Content-Type에 대해 무지했네요 ..
해당 부분은 현재 미션에서는 말씀 주신 것처럼 Content-Type: application/x-www-form-urlencoded로만 요청이 오기에 문제가 없어 보였네요 ..! 다만, 이 부분은 ... 시간 관계상 .. 추후 .. TODO로 남겨놓겠습니다 ㅎㅎ 감사합니다 👍

mcodnjs · 2023-09-11T15:12:57Z

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

+        for (String temp : new String(buffer).split("&")) {
+            String[] value = temp.split("=");
+            if (value.length >= 2) {
+                body.put(value[0], URLDecoder.decode(value[1], StandardCharsets.UTF_8));
+            }
+        }


허허 .. 천재신가요 에밀?

mcodnjs · 2023-09-11T15:41:14Z

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

+        try {
+            final HttpRequestLine requestLine = parseRequestLine();
+            final HttpHeaders requestHeaders = parseRequestHeader();
+            if (requestHeaders.getHeaderValue(CONTENT_LENGTH) != null) {


참고 리뷰까지 감사합니다~
Map의 경우 존재하지 않는 key에 대한 null 반환은 자연스럽다고 생각하여 이부분은 그대로 유지했구요
null을 명시적으로 반환하는 로직만 Optional을 반환하도록 변경하였습니다!

sonarqubecloud · 2023-09-11T16:14:55Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 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

CFalws

라온 빠르게 리뷰 적용해주셨네요 ~
덕분에 리뷰 하면서 많이 배우고 고민해볼 수 있었습니다.
이번 미션 고생하셨고 다음 미션도 화이팅입니다^_^

CFalws · 2023-09-12T05:12:34Z

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

+import static org.apache.coyote.http11.common.HttpHeaderType.ACCEPT;
+import static org.apache.coyote.http11.common.HttpHeaderType.CONTENT_TYPE;
+
+public class ResourceContentTypeResolver {


넵 유틸성 클래스라면 불필요한 인스턴스화를 막기 위해 모든 메서드를 static으로 두고, private 생성자를 통해 인스턴스화를 컴파일 타임에 막는 것도 좋을 것 같아요

CFalws · 2023-09-12T05:55:59Z

tomcat/src/main/java/org/apache/catalina/session/SessionManager.java

 public class SessionManager implements Manager {

-    private static final Map<String, Session> SESSIONS = new HashMap<>();
+    private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();


private static final Map<String, Sessions> SESSIONS = new ConcurrentHashmap<>(); -> private final Map<String, Sessions> SESSIONS = new ConcurrentHashmap<>();

위와 같이 수정해도 해당 SESSIONS 객체가 런타임에 유일하다는 것은 보장되는 것 같습니다
어떤 방식을 써도 상관없어서 취향대로 해주시면 될 것 같네요 ~

CFalws · 2023-09-12T06:08:27Z

tomcat/src/main/java/org/apache/catalina/controller/HttpController.java

+    @Override
+    public void service(final HttpRequest httpRequest, final HttpResponse httpResponse) {
+        try {
+            if (!canHandle(httpRequest)) {
+                throw new HttpException("There is no controller to handle.");
+            }
+            final String method = httpRequest.getMethod();
+            if ("GET".equals(method)) {
+                doGet(httpRequest, httpResponse);
+            }
+            if ("POST".equals(method)) {
+                doPost(httpRequest, httpResponse);
+            }
+        } catch (final Exception e) {
+            httpResponse.setStatusCode(NOT_FOUND);
+            httpResponse.setBody(e.getMessage());
+        }


수정하신 방식 좋은데요? 👍
(추신 왜인지 모르겠지만 예전 코드에 코멘트가 달려버렸네요ㅋㅋ)

CFalws · 2023-09-12T06:13:54Z

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

+import nextstep.jwp.controller.LoginController;
+import nextstep.jwp.controller.RegisterController;
+import nextstep.jwp.controller.RootController;


현재 패키지 의존이 nextstep -> org.apache.coyote -> nextstep(컨트롤러들) 으로 순환의존이 있는데요
nextstep 패키지가 서버 개발자의 코드, coyote는 톰캣의 코드라고 가정하면, (구구께서 말씀하신 것처럼)
톰캣의 코드가 서버 개발자의 코드에 의존하고 있는 상태인데요
해당 부분에 대해 생각해보시는 것도 좋을 것 같습니다😃

mcodnjs added 25 commits September 6, 2023 01:36

refactor: request에 HttpHeaders 적용

f54bffd

refactor: 패키지 구조 변경

f8d24eb

refactor: HttpHeaders 내부에 HttpCookie 추가

fe717dd

refactor: RequestHandler 및 Controller 객체 생성

293f988

test: Http 활용하기 테스트 구현

fbbd644

refactor: Controller 핸들링 구조 변경

2bb6e1f

refactor: Controller 메서드별 로직 추가

9aec72c

refactor: Processor 내부의 handleRequest 메서드 삭제

f0bf5bb

refactor: ResourceReader 구현

f5fd662

refactor: ResourceContentTypeResolver 구현

9687137

refactor: MediaType 구현

e16df12

refactor: MediaType value 수정

32a7681

refactor: 리소스 핸들링하는 메소드 분리

407db0a

refactor: 패키지 구조 분리

33e7826

test: thread test 진행

317e434

feat: step4 - executors로 thread pool 적용

99a5154

feat: step4 - 동시성 컬렉션 사용

e66ea7f

refactor: 세션 패키지 분리

7f2ed15

refactor: HttpController의 추상메서드를 구체메서드로 변경

ab7dfee

refactor: session 패키지 위치 변경

d6b199c

test: coyote 패키지 테스트 구현

9d1f8ff

test: Controller 테스트 구현

8237e9e

refactor: SessionManager 싱글톤 구현

cbd7b69

refactor: todo 주석 제거

9d6802e

refactor: RequestMapping 주입 수정

a0efa42

mcodnjs commented Sep 9, 2023

View reviewed changes

mcodnjs self-assigned this Sep 9, 2023

mcodnjs requested a review from CFalws September 9, 2023 19:33

CFalws requested changes Sep 10, 2023

View reviewed changes

refactor: content type 헤더 제거

ef23b65

mcodnjs added 5 commits September 11, 2023 23:31

refactor: exception handling 로직 수정

a5493f7

refactor: ResourceContentTypeResolver static 메서드로 수정

dea46b0

refactor: getResourceContentType 로직 수정

4a96971

refactor: url encoded 메서드 분리

1e6d57c

refactor: null 반환 Optional 처리

8039dde

mcodnjs commented Sep 11, 2023

View reviewed changes

refactor: Controller의 handle 메서드 수정

f6e401c

mcodnjs mentioned this pull request Sep 11, 2023

[톰캣 구현하기 - 3, 4단계] 제리(권현재) 미션 제출합니다. #415

Merged

CFalws approved these changes Sep 12, 2023

View reviewed changes

CFalws merged commit ec1ccde into woowacourse:mcodnjs Sep 12, 2023


		public class ResourceContentTypeResolver {

		public String getContentType(final HttpHeaders headers, final String resourceName) {

[톰캣 구현하기 - 3, 4단계] 라온(문채원) 미션 제출합니다. #433

[톰캣 구현하기 - 3, 4단계] 라온(문채원) 미션 제출합니다. #433

Uh oh!

Conversation

mcodnjs commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcodnjs Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CFalws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CFalws Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcodnjs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mcodnjs commented Sep 9, 2023 •

edited

Loading

mcodnjs Sep 9, 2023 •

edited

Loading

CFalws Sep 12, 2023 •

edited

Loading

CFalws Sep 12, 2023 •

edited

Loading