[톰캣 구현하기 - 3, 4단계] 에단(김석호) 미션 제출합니다. #422

cookienc · 2023-09-08T14:03:28Z

안녕하세요 무민! 에단입니다. 4단계까지 완료해서 pr 보냅니다.
이번 단계에서 변경사항은 다음과 같습니다.

Controller를 도입해서 순환참조가 없게 패키지 분리
요구사항으로 인해 HttpResponse의 값을 builder 패턴에서 setter로 변경
E2E 테스트 추가
시간 나실때 천천히 리뷰해주세요. 이번에도 잘 부탁드립니다🙇‍♂️🙇🙇‍♀️

parkmuhyeun

고생하셨습니다 에단~

3, 4단계 미션 잘 구현해주신거 같아요. 🙌🙌
객체 분리 후 많은 중복이 줄어들어 깔끔해졌네요.
Http11Processor test + StubSocket 만드느라 힘드셨을거 같아요ㅎㅎ (시간 되시면 단위 테스트까지?ㅋㅋㅋㅋㅋ)

아래 코멘트들 확인 부탁드립니다!

parkmuhyeun · 2023-09-09T19:51:45Z

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

+
+import java.io.IOException;
+
+public abstract class RequestController implements Controller {


Controller interface에 다음과 같이 사용할 수 도 있을텐데 abstract class 사용한 이유가 궁금합니다!

public interface Controller { default void service(final HttpRequest request, final HttpResponse response) throws IOException { if (request.isSameRequestMethod(HttpMethod.GET)) { doGet(request, response); return; } if (request.isSameRequestMethod(HttpMethod.POST)) { doPost(request, response); return; } throw new UnsupportedOperationException("지원하지 않는 HTTP Method 입니다."); } void doPost(final HttpRequest request, final HttpResponse response); void doGet(final HttpRequest request, final HttpResponse response) throws IOException; }

default 메서드는 java8에서 기존 인터페이스를 유지하면서 새로운 기능을 추가하려고 할 때 호환성을 위해서 추가된걸로 알고 있습니다. 그래서 처음 만드는 입장에서 default가 생성된 이유와 맞지 않아서 abstract class로 생성했습니다.

parkmuhyeun · 2023-09-09T20:00:55Z

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

+        response.changeStatusLine(StatusLine.from(StatusCode.OK));
+        response.addHeader(CONTENT_TYPE, ContentType.HTML.getValue());
+        response.changeBody(new HttpBody(HOME_MESSAGE));


statusLine은 정적팩토리메서드로 HttpBody는 생성자로 생성하고 있는데 혹시 기준이 있나요?

특별한 로직 같은게 있으면 정팩메를 씁니다! StatusLine에 있는 from 메서드는 Protocol이 기본으로 들어가 있어서 로직으로 봤습니다.

parkmuhyeun · 2023-09-09T20:12:29Z

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

+        }
+    }
+
+    private static User getValidateUser(final HttpRequest request) {


메서드 추출하면서 아래 부분들에 static이 붙은거 같아요!

지우겠습니다ㅜㅜ @parkmuhyeun 혹시 이거 자동으로 없애는 팁있나요?

이거 찾아봤는데 딱히 없는거 같아요 ㅠㅠ 저도 자주 실수하는 부분..

parkmuhyeun · 2023-09-09T20:18:47Z

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

+        if (request.isRequestUriEndsWith(HTML.getFileExtension())) {
+            createHttpResponseByContentTypeAndPath(response, HTML, filePath);
+            return;
+        }
+
+        if (request.isRequestUriEndsWith(JS.getFileExtension())) {
+            createHttpResponseByContentTypeAndPath(response, JS, filePath);
+            return;
+        }
+
+        if (request.isRequestUriEndsWith(CSS.getFileExtension())) {
+            createHttpResponseByContentTypeAndPath(response, CSS, filePath);
+            return;
+        }
+
+        if (request.isRequestUriEndsWith(ICO.getFileExtension())) {
+            createHttpResponseByContentTypeAndPath(response, ICO, filePath);
+            return;
+        }


ContentType Enum 만드셨으니 충분히 중복제거 할 수 있을 거 같아요!

parkmuhyeun · 2023-09-09T20:30:51Z

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

+        return requestLine.containsRequestUri("?");
+    }
+
+    public boolean isRequestUriEndsWith(final String uri) {


isEndsWithRequestUri가 좀 더 자연스러운거 같아요!

parkmuhyeun · 2023-09-09T20:39:06Z

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


 public class SessionManager implements LoginManager {

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


혹시 ConcurrentHashMap에 대해 더 자세히 알고 싶으시다면
https://parkmuhyeun.github.io/woowacourse/2023-09-09-Concurrent-Hashmap/

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

댓글 남겼습니닼ㅋㅋㅋㅋ

parkmuhyeun · 2023-09-09T20:41:59Z

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

    private final HttpRequestParser httpRequestParser;

-    public Http11Processor(final Socket connection) {
+    public Http11Processor(final Socket connection, final RequestHandler requestHandler) {


여러 생성자가 생성되고 있는데 혹시 Http11Processor의 생성자가 언제 호출 되시는지 아시나요?

클라이언트가 요청 들어올때 생성되나요? 잘 모르겠어요

Connector에서 서버소켓이 수락한 후 process 메서드가 실행되면 그때 processor가 생성되더라구요.

그럼 요청이 들어올 때마다 현재 생성자들을 다시 생성해줄 필요가 있을까?라고 생각이 들어서 위와같이 남겼어요!

저는 상관없다고 생각해요. 왜냐하면 세션같은 건 static으로 떠있어서 공유되기 때문에 상관없을 것 같고, 객체 생성비용만 생길것같은데, 오히려 싱글톤으로 구현하면 예상하지 못한 오류가 날수도 있을것 같아요

상태가 없는 HttpRequestParser 같은 경우 요청이 들어올 때 마다 새로 생성하는 것보다 한번만 만들면 더 좋지 않을까요

parkmuhyeun · 2023-09-09T20:52:41Z

tomcat/src/test/java/nextstep/org/apache/coyote/http11/Http11ProcessorRegisterTest.java

+    class Register {
+
+        @Test
+        void register으로_POST_요청하면_해당_페이지로_이동한다() throws IOException {


해당_페이지 보다 index_html로 이동한다가 좀 더 이해하기 좋을 거 같아요

감사합니다!

parkmuhyeun · 2023-09-09T20:54:17Z

tomcat/src/test/java/nextstep/org/apache/coyote/http11/Http11ProcessorRegisterTest.java

+        }
+
+        @Test
+        void register으로_GET_요청하면_register_html로_리다이렉트_된다() throws IOException {


테스트 내용이 뭔가 잘못된거 같아요. 한번 파라미터들 확인해주세요!!

일단 제가 테스트 코드 이름을 잘못적은것 같네요. register으로_GET_요청하면_register_html로_이동한다가 맞긴한데 어떤 점이 문제가 될까요?

현재 requestURI가 login이 아니라 register가 되어야 되는거 아닌가요

아아 맞네요! 왜 저는 이게 안보였을까요.. 매의 눈이시네요

parkmuhyeun · 2023-09-09T20:59:38Z

tomcat/src/test/java/support/StubSocket.java

@@ -8,41 +17,58 @@
 import java.net.Socket;
 import java.net.UnknownHostException;
 import java.nio.charset.StandardCharsets;
+import java.util.EnumMap;

 public class StubSocket extends Socket {


👍🏻👍🏻

parkmuhyeun

코드 스멜 제거까지 Good!

몇가지 궁금한 점 코멘트 남겼으니 확인해보시고 다시 리뷰 요청 부탁드립니다!

sonarcloud · 2023-09-13T02:27:09Z

Kudos, SonarCloud Quality Gate passed!

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

cookienc · 2023-09-13T02:28:16Z

안녕하세요 무민~
꼼꼼한 리뷰 감사합니다. 덕분에 이번 미션에서 많이 배웠습니다!
리뷰 내용 반영했습니다 :)

parkmuhyeun

여기서 마무리 해도 충분할 것 같아서 이만 머지하겠습니다!
톰캣 미션하는 동안 고생 많으셨어요~ 저도 많이 배워갑니다.

남은 미션들도 화이팅 🙌

parkmuhyeun · 2023-09-13T03:46:19Z

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

    private final HttpRequestParser httpRequestParser;

-    public Http11Processor(final Socket connection) {
+    public Http11Processor(final Socket connection, final RequestHandler requestHandler) {


상태가 없는 HttpRequestParser 같은 경우 요청이 들어올 때 마다 새로 생성하는 것보다 한번만 만들면 더 좋지 않을까요

cookienc added 30 commits September 8, 2023 08:55

feat: CacheControl 인터셉터 추가

5f53414

feat: 압축 설정 추가

818588c

feat: Etag 설정 추가

b205cc0

feat: Version 및 캐싱 기능 구현

e42bd2a

test: 쓰레드 stage0 학습 테스트 진행

815ecb5

test: 쓰레드 stage1 학습 테스트 진행

35d7e57

test: 쓰레드 stage2 학습 테스트 진행

c714b06

refactor: 패키지 변경

4fd6b34

refactor: HttpHeaders 의 key 를 enummap으로 변경

7c08deb

refactor: 정적파일의 확장자를 contenttype으로 관리

8618726

refactor: 상수의 접근 제한자를 private으로 변경

0b1b0e6

refactor: HomePageMapping 에서 경로 상수화

2d9e91c

refactor: 페이지 이름을 관리하는 Path 추가

8a1647e

refactor: requestLine 빈 줄 제거

10bb2f2

refactor: QueryString 으로 리팩토링

2f774c9

refactor: keySet()을 entrySet()으로 변경

f7d6fc2

feat: Controller 추가

da45451

refactor: AbstractController 로 리팩토링

23216a7

refactor: 필요없는 파일 제거

4180472

refactor: Mapping 을 Controller 로 이름 변경

0e0cb5b

refactor: Login 관련 컨트롤러를 하나로 합침

83a2309

refactor: Register 관련 파일을 하나로 합침

3beba19

refactor: 패키지 변경

708cad5

refactor: example 패키지 CacheBustingWebConfig의 상수의 접근제한자 변경

6d188b4

refactor: 의존성 끊기 위한 패키지 분리 및 인스턴스 변수 추가

937e0f7

refactor: FrontController 를 FrontHandler 로 리팩토링

4a2e985

refactor: HttpResponseBuilder 제거

cabedba

refactor: 사용하지 않는 메서드 제거

cbc9455

test: 테스트를 HttpRequest, HttpResponse 로 리팩토링

954862a

test: StubSockect 으로 serialize 코드 옮김

694cca8

cookienc added 7 commits September 8, 2023 14:55

refactor: 로그인 테스트 하기 쉬운 구조로 변경

0ecf383

test: E2E 테스트 작성

1994cc3

test: Processor 테스트 분리

f3cfa28

fix: 자원 공유 문제 해결

c91cd6d

refactor: LoginController 메서드 분리

8ff9c92

feat: Default Max Thread Size 적용

86b6d32

refactor: SessionManager에 ConcurrentHashMap 적용

207e696

cookienc requested a review from parkmuhyeun September 8, 2023 14:03

cookienc self-assigned this Sep 8, 2023

refactor: code smell 제거

73966b1

parkmuhyeun reviewed Sep 9, 2023

View reviewed changes

parkmuhyeun self-requested a review September 9, 2023 21:05

parkmuhyeun requested changes Sep 9, 2023

View reviewed changes

cookienc added 5 commits September 12, 2023 09:28

refactor: 필요없는 static 제거

ed006cb

refactor: 메서드명 변경

93513c9

refactor: HttpResponse serialize 수정

f3c9cb7

refactor: 분기문을 stream 으로 정리

5f735eb

test: 잘못된 테스트 수정

e2b96bb

parkmuhyeun approved these changes Sep 13, 2023

View reviewed changes

parkmuhyeun merged commit c5efff1 into woowacourse:cookienc Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 에단(김석호) 미션 제출합니다. #422

[톰캣 구현하기 - 3, 4단계] 에단(김석호) 미션 제출합니다. #422

cookienc commented Sep 8, 2023

parkmuhyeun left a comment •

edited

Loading

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 13, 2023 •

edited

Loading

parkmuhyeun Sep 9, 2023

parkmuhyeun Sep 9, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 13, 2023

cookienc Sep 13, 2023

parkmuhyeun Sep 13, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 9, 2023

cookienc Sep 12, 2023

parkmuhyeun Sep 13, 2023

cookienc Sep 13, 2023 •

edited

Loading

parkmuhyeun Sep 9, 2023

parkmuhyeun left a comment •

edited

Loading

sonarcloud bot commented Sep 13, 2023

cookienc commented Sep 13, 2023

parkmuhyeun left a comment

parkmuhyeun Sep 13, 2023


		import java.io.IOException;

		public abstract class RequestController implements Controller {

[톰캣 구현하기 - 3, 4단계] 에단(김석호) 미션 제출합니다. #422

[톰캣 구현하기 - 3, 4단계] 에단(김석호) 미션 제출합니다. #422

Conversation

cookienc commented Sep 8, 2023

parkmuhyeun left a comment • 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

parkmuhyeun Sep 13, 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

cookienc Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkmuhyeun left a comment • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Sep 13, 2023

cookienc commented Sep 13, 2023

parkmuhyeun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkmuhyeun left a comment •

edited

Loading

parkmuhyeun Sep 13, 2023 •

edited

Loading

cookienc Sep 13, 2023 •

edited

Loading

parkmuhyeun left a comment •

edited

Loading