[톰캣 구현하기 - 3, 4단계] 조이(김성연) 미션 제출합니다. #473

yeonkkk · 2023-09-11T04:42:39Z

안녕하세요 콩하나!!! 🫘 조이입니다!!!!!

지난 리뷰에 대한 답변은 1, 2단계 PR에 남겨두었습니다.
리뷰를 꼼꼼하게 해주셔서 너무 좋았어요 ㅎㅎ 짱짱👍

2단계 구현과 3단계 리팩터링 과정에서 생각보다 시간이 오래걸렸습니다.
그리고 4단계는 솔직히 아직 공부를 많이 못 한 것 같습니다만, 같이 소통하면서 보완해 나가겠습니다🧐

구조는 크게 변하지 않았고, 2~4단계를 한번에 하다보니 추가된 코드가 조금 많은 것 같습니다.
리뷰하기에 편하실지 모르겠어요... 걱정 되네요 ㅠ 혹시 이해 안 되는 부분이 있으시면 코멘트 남겨주시거나 DM 주시면 감사하겠습니다!!

테스트는 이전에 말씀드린대로 1차 리뷰 후에 추가하도록 하겠습니다.
그럼 리뷰 잘 부탁드리겠습니다!
좋은 하루 되세요🙂

kong-hana01 · 2023-09-11T08:01:12Z

tomcat/src/main/java/nextstep/jwp/model/User.java

    private final Long id;
    private final String account;
    private final String password;
    private final String email;

-    public User(Long id, String account, String password, String email) {
+    public User(final Long id, final String account, final String password, final String email) {


발빠르게 적용하셨군요! ㅎㅎ

둘다 급하게 작성하느라 놓친 부분이네요.. 감사합니다 ㅎㅎ 반영하겠습니다!! 🙂

굿굿~~

kong-hana01 · 2023-09-11T08:05:59Z

tomcat/src/main/java/org/apache/catalina/Manager.java

     *
     * @return the request session or {@code null} if a session with the
     *         requested ID could not be found
     */
-    HttpSession findSession(String id) throws IOException;


throws IOException;

조이는 throws IOException이 붙어져있는 이유에 대해 생각해보신 적이 있으신가요!?
저도 조이와 같이 throws IOException를 없앴는데요.

이번에 구구 코치가 Controller interface의 시그니처를 유지하라는 이야기를 해주셨잖아요.
거기에도 Throws Exception이 포함되어있었거든요!
그래서 혹시 Throws Exception을 없애면 안되는 이유가 있나? 하는 고민이 있었어요.

이 부분도 마찬가지로 throws IOException이 기본적으로 붙어져있는 이유에 대해 생각해보신 적이 있으신지 궁금합니다!
저도 잘 모르겠어서 같이 이야기 나눠보고 싶네요!

저도 궁금해서 찾아봤는데, 정확한 이유는 잘 모르겠네요 ㅠ
그래서 맞는지는 모르겠지만 추측을 좀 해봤어요!

실제 톰캣에서 적용되는 findSession() 메서드는 세션을 찾는 작업을 수행할 때, 세션 데이터를 디스크에서 읽어오는 경우에 입출력 작업이 발생할 수 있다고 하네요. 그 과정에서 IOException이 발생할 수 있을 것 같아요!
(찾아보니 네트워크 클러스터에서 세션을 찾는 경우에와 같은 입출력 작업도 발생할 수 있다네요)

콩하나는 어떻게 생각하시나요?

오 그런 경우도 있겠군요!
말씀해주신 내용이 맞는 것 같아요!!

저도 단순히 Manager를 구현한 클래스 중 어딘가에서 I/O 작업을 할 수 있으니
해당 인터페이스에 명시적으로 throws IOException을 던진 것이겠거니~ 했어요!

조이 덕분에 하나 더 알아갑니다~

kong-hana01 · 2023-09-11T08:06:39Z

tomcat/src/main/java/org/apache/catalina/Session.java

+public class Session {
+
+    private final String id;
+    private final Map<String, Object> values = new ConcurrentHashMap<>();


new ConcurrentHashMap<>();

good~~

kong-hana01 · 2023-09-11T08:09:30Z

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

+        response.setStatusCode(HttpStatusCode.OK)
+                .addContentTypeHeader(TEXT_HTML.getType())
+                .addContentLengthHeader(requireNonNull(resource).getBytes().length)
+                .setResponseBody(new HttpResponseBody(resource));


오... 저는 왜 빌더 패턴을 적용할 생각을 못했을까요??
훨씬 깔끔해지네요!!

kong-hana01 · 2023-09-11T08:13:12Z

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

+
+        response.setStatusCode(HttpStatusCode.FOUND)
+                .addContentTypeHeader(TEXT_HTML.getType())
+                .addLocationHeader(NOT_FOUND_PAGE.getUri());


GET/POST 요청이 아닌 경우에는 모두 404 페이지로 리다이렉트 시키시는 걸까요? ㅎㅎ

확인해보니 405라는 statusCode가 존재하네요!

request의 메소드를 서버에서 알고 있지만 이를 지원하지 않는 경우에는 405 예외를 던지네요!
저도 이 부분은 구현이 안되어있는데 이렇게 또 하나 배워갑니다 ㅋㅋㅋㅋ

저도 405 를 내려주는 것이 좋을 것 같다고 생각은 들었지만, 405.html 가 존재하지 않아서 고민을 좀 했습니다..
그냥 405 를 내려주고 예상하지 못한 페이지를 보게 되면, 사용자 입장에서 당황스러울 것 같다는 생각이 들어서 우선 404.html로 리다이렉트 시키는 방향을 생각해봤습니다. 지금 생각해보니 그런 의도였으면 500.html을 활용하는게 더 좋았겠네요 ;;ㅎㅎ

하지만 정상적인 서비스라면 콩하나 말대로 405를 내려주는게 맞는 것 같아요!!!
해당 메서드로 요청을 처리 할 수 없을 때 405를 내려주는 것만 알고 allow 헤더에 대해서는 몰랐는데, 콩하나 덕분에 하나 알아갑니다 ㅎㅎ

그런데 만약 405 status code를 내려준다면 어떤 view를 반환하는게 좋을까요?
제가 고민했던 부분이라 콩하나의 의견이 궁금합니다!!

405 status code를 내려준다면 문자열이나 json 형태로 전달할 것 같네요!

일반적으로 405 status code가 발생하는 경우는 GET이나 POST와 같이 자주 사용하는 메소드가 아니라
다른 메소드일 것으로 생각되는데요.
그런 경우에는 서버와 클라이언트가 json 값을 주고받을 것으로 생각되네요!
그래서 그냥 문자열이나 json을 전달할 것 같아요~

kong-hana01 · 2023-09-11T13:22:20Z

tomcat/src/main/java/org/apache/catalina/util/FileLoader.java

+        final File file = new File(resource.getFile());
+        final Path path = file.toPath();
+
+        return new String(Files.readAllBytes(path));


저도 FileLoader의 역할을 하는 객체를 만들었는데요.
역시 조이 코드가 훨배 더 깔끔하네요! 👍 👍

콩하나 코드 다 훔쳐보고 왔으니 기만하지 마시죠!!✨👀‼️
잘 작성하셨으면서~~~~~

kong-hana01 · 2023-09-11T13:26:19Z

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

+        final Map<String, String> cookies = Pattern.compile(ENTRY_DELIMITER)
+                .splitAsStream(cookieHeader.trim())
+                .map(cookie -> cookie.split(KEY_VALUE_DELIMITER))
+                .collect(Collectors.toUnmodifiableMap(cookie -> cookie[KEY_INDEX], cookie -> cookie[VALUE_INDEX]));


오! 이렇게 map을 사용할 수 있군요! ㅋㅋㅋㅋㅋㅋ
또또또또또 다섯개 배워갑니다!

오또ㄴㅔ요^^~~~~~( ^-^)~~~

kong-hana01 · 2023-09-11T13:32:07Z

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

+        if (!cookies.containsKey(key)) {
+            return null;
+        }
+        return cookies.get(key);
+    }


if (!cookies.containsKey(key)) { return null; } return cookies.get(key);

return cookies.get(key)

아래의 형식만 남겨도 되겠네요!

key 값이 존재하지 않으면 null을 return 해주는군요?!
배워갑니다 ㅎㅎ

사실 이렇게 말한 저도 코드 작성할 때에는 getOrDefault() 메소드를 사용했어요 ㅋㅋㅋㅋ

ㅋㅋㅋㅋㅋㅋ확실한게 좋죠 ~~~

kong-hana01 · 2023-09-11T13:33:03Z

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

+        if (cookie != null && cookie.contains("JSESSIONID")) {
+            return cookie.getValue("JSESSIONID");
+        }
+        return null;


그러면 여기서도 cookie.getValue("JSESSIONID")만 호출해도 되겠네요~

kong-hana01 · 2023-09-11T13:33:56Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

+
+    public HttpResponse addContentLengthHeader(final Object value) {
+        this.header.addContentLength(value);
+        return this;


오~~ 이런 느낌으로 빌더 느낌을 줄 수 있겠네요~
와... 미션 리뷰 하나에 이렇게 많이 배우다니 참 알차네요

콩하나
건강왕 칭찬왕 친절왕
왕왕왕

건강... 실패...

kong-hana01

안녕하세요 블루벨벳 조이!
콩하나입니다!

이번에도 아주 깔끔하고 이쁘게 구조를 잘 짜셨더라고요!!
역시... 계속 감탄해가면서 코드 리뷰를 했습니다!
덕분에 잘 배웠습니다!

제가 감히 이 아름다운 코드에 리뷰를 드려도 될지는 모르겠지만...!
몇 가지 의견과 질문을 남겨드렸어요!!
한번 확인해보시고 여러 가지 같이 의견 나눠보면 좋을 것 같네요!
고생하셨어요!!

kong-hana01

조이! 제가 깜빡하고 남겨드리려던 리뷰를 안남겨드려서 하나 추가로 남겨드려요~

kong-hana01 · 2023-09-12T01:21:09Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

@@ -67,7 +71,7 @@ private void process(final Socket connection) {
            return;
        }
        var processor = new Http11Processor(connection);
-        new Thread(processor).start();
+        executorService.execute(processor);


아! 어제 이것도 남겨드리려고 했는데 깜빡하고 있다가 집가는 길에 생각나더라고요 ㅎㅎ

쓰레드의 작업을 처리를 수행하는데 사용되는 메소드가 두 개 인 걸 알고 계시나요??

execute()

submit()

둘 다 사용할 수 있지만 예외가 발생했을 때 execute() 메소드는 쓰레드를 새로 생성하고,
submit() 메소드는 기존 쓰레드를 그대로 사용한다고 하네요!

https://hudi.blog/java-thread-pool/

두 메서드 차이가 궁금했는데 그런 차이가 있었군요!! 감사해요 콩하나 ㅎㅎ
submit() 을 가져가는게 더 효율적인 것 같네요!

sonarcloud · 2023-09-12T08:48:13Z

Kudos, SonarCloud Quality Gate passed!

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

yeonkkk

안녕하세요 콩하나~
하나, 둘, 셋 ~ 블루벨벳 조이 입니다! ^^ (?)

이번에도 꼼꼼한 리뷰 너무 감사드려요 ㅎㅎ
콩하나의 리뷰 덕분에 새롭게 생각해본 것들도 많고, 미처 고려하지 못한 부분들도 발견할 수 있어서 좋았습니다 !!
그리고 칭찬을 너무 많이 해주셔서 기분이 매우 좋아지네요 ㅋㅋ 최고의 리뷰어🫡

코멘트 내용 반영 및 답변 남겨두었으니 편하실 때 확인 부탁드려요!
그럼 좋은 하루 되세요 ㅎㅎ👍

yeonkkk · 2023-09-12T06:47:40Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

@@ -67,7 +71,7 @@ private void process(final Socket connection) {
            return;
        }
        var processor = new Http11Processor(connection);
-        new Thread(processor).start();
+        executorService.execute(processor);


두 메서드 차이가 궁금했는데 그런 차이가 있었군요!! 감사해요 콩하나 ㅎㅎ
submit() 을 가져가는게 더 효율적인 것 같네요!

yeonkkk · 2023-09-12T06:55:55Z

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

+
+        response.setStatusCode(HttpStatusCode.FOUND)
+                .addContentTypeHeader(TEXT_HTML.getType())
+                .addLocationHeader(NOT_FOUND_PAGE.getUri());


저도 405 를 내려주는 것이 좋을 것 같다고 생각은 들었지만, 405.html 가 존재하지 않아서 고민을 좀 했습니다..
그냥 405 를 내려주고 예상하지 못한 페이지를 보게 되면, 사용자 입장에서 당황스러울 것 같다는 생각이 들어서 우선 404.html로 리다이렉트 시키는 방향을 생각해봤습니다. 지금 생각해보니 그런 의도였으면 500.html을 활용하는게 더 좋았겠네요 ;;ㅎㅎ

하지만 정상적인 서비스라면 콩하나 말대로 405를 내려주는게 맞는 것 같아요!!!
해당 메서드로 요청을 처리 할 수 없을 때 405를 내려주는 것만 알고 allow 헤더에 대해서는 몰랐는데, 콩하나 덕분에 하나 알아갑니다 ㅎㅎ

그런데 만약 405 status code를 내려준다면 어떤 view를 반환하는게 좋을까요?
제가 고민했던 부분이라 콩하나의 의견이 궁금합니다!!

yeonkkk · 2023-09-12T06:57:14Z

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

+
+    private boolean existSession(final HttpRequest request) {
+        final Session session = Authorizer.findSession(request);
+        return session != null;


그게 더 역할에 맞는 것 같다고 느껴지네요 ! 수정하겠습니다 ㅎㅎ

yeonkkk · 2023-09-12T06:57:37Z

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

+        if (notExistSession(request) && notExistAccount(user)) {
+            InMemoryUserRepository.save(user);
+        }
+        redirectToDefaultPage(response);


ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ

yeonkkk · 2023-09-12T06:59:46Z

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

+
+    static {
+        controllers.put(ROOT_REQUEST_URL, new RootController());
+        controllers.put(DEFAULT_PAGE.getUri(), new DefaultController());


맞습니다 !! enum 클래스로 작성했습니다 ㅎㅎ

yeonkkk · 2023-09-12T07:13:28Z

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

+        final Map<String, String> cookies = Pattern.compile(ENTRY_DELIMITER)
+                .splitAsStream(cookieHeader.trim())
+                .map(cookie -> cookie.split(KEY_VALUE_DELIMITER))
+                .collect(Collectors.toUnmodifiableMap(cookie -> cookie[KEY_INDEX], cookie -> cookie[VALUE_INDEX]));


오또ㄴㅔ요^^~~~~~( ^-^)~~~

yeonkkk · 2023-09-12T07:14:00Z

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

+        if (!cookies.containsKey(key)) {
+            return null;
+        }
+        return cookies.get(key);
+    }


key 값이 존재하지 않으면 null을 return 해주는군요?!
배워갑니다 ㅎㅎ

yeonkkk · 2023-09-12T07:17:49Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

+
+    public HttpResponse addContentLengthHeader(final Object value) {
+        this.header.addContentLength(value);
+        return this;


콩하나
건강왕 칭찬왕 친절왕
왕왕왕

yeonkkk · 2023-09-12T07:22:45Z

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

+import java.util.Map;
+import org.apache.coyote.http11.request.HttpRequest;
+
+public class RequestMapping {


컨트롤러 작성할 때 같이 작성하다보니 패키지 분리가 안 됐네요.. !!!!
RequestMapping 은 URI에 매핑되는 controller를 찾아서 반환해주는 역할을 하기 때문에, controller 랑은 조금 다른 개념인 것 같아요.
오히려 controller 패키지에 넣기보다는 그 상위 패키지에 넣어야 할 것 같습니다!!
놓친 부분인데 감사합니다 ㅎㅎ

yeonkkk · 2023-09-12T08:04:36Z

tomcat/src/main/java/org/apache/catalina/Manager.java

     *
     * @return the request session or {@code null} if a session with the
     *         requested ID could not be found
     */
-    HttpSession findSession(String id) throws IOException;


저도 궁금해서 찾아봤는데, 정확한 이유는 잘 모르겠네요 ㅠ
그래서 맞는지는 모르겠지만 추측을 좀 해봤어요!

실제 톰캣에서 적용되는 findSession() 메서드는 세션을 찾는 작업을 수행할 때, 세션 데이터를 디스크에서 읽어오는 경우에 입출력 작업이 발생할 수 있다고 하네요. 그 과정에서 IOException이 발생할 수 있을 것 같아요!
(찾아보니 네트워크 클러스터에서 세션을 찾는 경우에와 같은 입출력 작업도 발생할 수 있다네요)

콩하나는 어떻게 생각하시나요?

kong-hana01

고생 많았습니다 조이!
너무너무너무너무너무 잘해주셔서 이만 머지하도록 할게요~~

다음 미션도 화이팅하시죠!

kong-hana01 · 2023-09-13T06:49:42Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

@@ -71,7 +71,7 @@ private void process(final Socket connection) {
            return;
        }
        var processor = new Http11Processor(connection);
-        executorService.execute(processor);
+        executorService.submit(processor);


submit할게요~

yeonkkk added 13 commits September 6, 2023 15:39

refactor: HttpRequestUri가 path와 queryString을 가지도록 수정

aa3dd74

refactor: 개행 및 final 컨벤션 통일

1074eca

refactor: toString 메서드명 변경

85c96fa

feat: Cookie & Session 구현

4ffb6f7

refactor: 역할 분리를 위한 Controller 및 RequestMapping 추가

0bb79b4

chore: 패키지 변경

59e023f

refactor: Thread Pool 적용

3bb9396

refactor: 동시성 컬렉션 적용

9674e73

refactor: 매직 리터럴 상수화

0922f5d

refactor: 메서드 위치 수정, 불필요한 코드 제거

c1870e8

refactor: User 생성자 추가, password 비교 로직 변경

7d6ae10

refactor: static import, 메서드 분리

838dd4a

refactor: 상수 접근자 변경, 로그인 로그 추가

aa653ca

kong-hana01 reviewed Sep 11, 2023

View reviewed changes

kong-hana01 requested changes Sep 11, 2023

View reviewed changes

kong-hana01 reviewed Sep 12, 2023

View reviewed changes

yeonkkk added 6 commits September 12, 2023 17:24

refactor: 허용되지 않은 메서드로 요청할 경우 상태코드 405를 반환하도록 수정

7bd3ef3

refactor: Authorizer를 통해 session 존재 여부를 확인하도록 변경

a814c60

chore: RequestMapping 패키지 변경

c5ff7bf

refactor: 404 페이지 반환 방식 변경

c85a3e9

refactor: containsKey() 검증 제거

42235a3

refactor: 쓰레드 작업 처리 메서드 변경

ecbff03

yeonkkk commented Sep 12, 2023

View reviewed changes

kong-hana01 mentioned this pull request Sep 13, 2023

[톰캣 구현하기 2, 3, 4단계] 콩하나(최한빈) 미션 제출합니다. #475

Merged

kong-hana01 approved these changes Sep 13, 2023

View reviewed changes

kong-hana01 merged commit 04fbff6 into woowacourse:yeonkkk Sep 13, 2023
2 checks passed

[톰캣 구현하기 - 3, 4단계] 조이(김성연) 미션 제출합니다. #473

[톰캣 구현하기 - 3, 4단계] 조이(김성연) 미션 제출합니다. #473

Conversation

yeonkkk commented Sep 11, 2023

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

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

kong-hana01 left a comment

Choose a reason for hiding this comment

kong-hana01 left a 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 12, 2023

yeonkkk 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

kong-hana01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment