[톰캣 구현하기 - 2, 3, 4단계] 기론(김규철) 미션 제출합니다. by Gyuchool · Pull Request #236 · woowacourse/java-http

Gyuchool · 2022-09-07T15:24:42Z

안녕하세요 잉~!
잉의 리뷰 덕분에 여러 생각들을 해봤어요! 계속 시도하고 엎고 다시 갈아엎고를 반복했는데도 맘에 들지않네요...ㅎㅎ
점진적으로 리펙토링을 계속 해봐야겠어요...
이번에도 리뷰 잘부탁드립니다!!

Gyuchool · 2022-09-08T01:59:46Z

아 잉 저번에 말씀하신 테스트가 깨진 부분은 맥이랑 윈도우랑 개행을 세는 바이트 수가 달라서 그런거래요!
그래서 윈도우는 index.html이 5670이고 맥은 5564였나? 그랬던거 같았습니다!

Gyuchool · 2022-09-08T09:01:05Z

제출이 오늘 6시까지여서 2단계 이후, 3+4단계 추가했습니다!

-> 올린거 이제 봤네요🙏🏻 다 합쳐서 리뷰하겠습니다!!

Yboyu0u

안녕하세요 기론✋🏻 2단계 리뷰 요청 하신 거 못봐서 죄송했습니다ㅎㅎ
요구사항 구현 잘 해주셨네요.
제가 지금 할머니댁에 내려와 있는 관계로 리뷰를 조금 덜 했는데 우선 한 번 봐보시면 감사하겠습니다!
내일이나 일요일에 리뷰 한 번 남기면서 머지갈겨보도록 하겠습니다.
즐거운 명절 보내세요 기론🙏🏻👊🏼

Yboyu0u · 2022-09-09T13:43:16Z

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

+            RequestLine requestLine = RequestLine.extract(bufferedReader.readLine());
+            HttpHeaders httpHeaders = HttpHeaders.create(bufferedReader);
+            HttpCookie cookie = HttpCookie.extract(httpHeaders);
+            String requestBody = extractRequestBody(bufferedReader, httpHeaders, requestLine.getHttpMethod());


정적 팩토리 메서드로 가독성을 높이신 부분 좋네요👍🏻
개인적인 의견인데 RequestLine, headers, body, cookie 만드는 메서드를 HttpRequest에서도 정적 팩토리 패턴을 적용해 안에 넣어 보면 좀 더 깔끔해 지지 않을까? 라는 생각이 드네요.

잉지노 효과 뭐죠.. 분명 제가 객체 안에다가 넣었을 땐 buffer가 이상하게 읽어들여서 뺐었는데 잉의 리뷰대로 넣으니 바로 되네요 하하
수정했습니다!

Yboyu0u · 2022-09-09T13:43:18Z

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

+            Controller controller = RequestMapping.getController(request);
+            log.info("controller : {}", controller);
+            HttpResponse response = new HttpResponse();
+            controller.service(request, response);


여기서 HttpResponse가 빈 객체로 생성 된 후, setter들이 controller에서 적용되어 response가 최종적으로 만들어 지고 있네요. set 사용은 지양하라(객체지향적인 구조를 해칠 뿐만 아니라, 외부에서 객체의 상태가 변경되는 것은 위험하기 때문인 걸로 알고있습니다.)는 말을 많이 들었는데, 이렇게 구현하신 이유가 궁금합니다. (이렇게 구현하신 분들이 많은 것 같아서 궁금해서 여쭤봅니다ㅎㅎ)

저도 처음엔 set없이 사용했는데 실제 서블릿 구성 함수를 보니 setter를 사용했더라구요. 그래서 실제 서블릿 형태와 비슷하게 구현하려고 했어요!
왜 그렇게 사용했는지는 스프링 프레임워크의 특징 중 하나가 POJO라고 생각해요. 그래서 POJO에 입각해서 코드를 짠다면 setter를 열어도 괜찮다고 생각을 해서 저두 열어봤습니다! 맞는 생각인지는 모르겠네요 ㅎㅎ

오 그렇군요. 한 수 배워 갑니다🙏🏻

Yboyu0u · 2022-09-09T13:43:21Z

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

-                .split(" ")[1]
-                .substring(1);
+    private void submitResponse(OutputStream outputStream, HttpResponse response) throws IOException {
+        String httpResponse = response.to();


HttpResponse의 to()라는 네이밍이 너무 추상적인 것 같네요. Controller를 거친 후 완성된 response들을 출력을 위해 String으로 변환하는 기능의 메서드인데 적절한 네이밍으로 바꿔 보면 좋을 것 같네요.

추후에 바꾸려고 했는데 수정하는 것을 까먹었군요.. toResponse로 했는데 어떠신가요,,,?

Yboyu0u · 2022-09-09T13:43:23Z

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

+    private String extractRequestBody(BufferedReader bufferedReader, HttpHeaders httpHeaders, HttpMethod httpMethod)
+            throws IOException {
+        String requestBody = "";
+        if (httpMethod.equals(HttpMethod.POST)) {
+            int contentLength = Integer.parseInt(httpHeaders.get("Content-Length"));
+            char[] buffer = new char[contentLength];
+            bufferedReader.read(buffer, 0, contentLength);
+            return new String(buffer);


request의 requestLine, headers 등은 해당 클래스에서 BufferedReader를 받아 처리 하셨는데 이 부분은 Processor에서 처리하셨네요. 이렇게 처리하신 기준이 있으신가요?

이 부분은 위에서 말씀드렸던 내부에서 bufferedReader를 읽으니 이상하게 읽히더라구요..ㅜㅠ (버그인가..?)
지금은 내부에 넣어도 잘 동작하여 넣었습니다!

Yboyu0u · 2022-09-09T13:43:27Z

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

+package org.apache.coyote.http11.request;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class HttpDataRequest {
+    private static final String REQUEST_STANDARD = "&";
+    private static final String DATA_STANDARD = "=";
+    private static final int KEY_INDEX = 0;
+    private static final int VALUE_INDEX = 1;
+
+    private final Map<String, String> values;
+
+    private HttpDataRequest(Map<String, String> values) {
+        this.values = values;
+    }
+
+    public static HttpDataRequest extractRequest(String query) {
+        String[] dataMap = query.split(REQUEST_STANDARD);
+        Map<String, String> mp = new HashMap<>();
+        for (String data : dataMap) {
+            mp.put(getKey(data), getValue(data));
+        }
+        return new HttpDataRequest(mp);
+    }
+
+    private static String getKey(String dataMap) {
+        return dataMap.split(DATA_STANDARD)[KEY_INDEX];
+    }
+
+    private static String getValue(String dataMap) {
+        return dataMap.split(DATA_STANDARD)[VALUE_INDEX];
+    }
+
+    public String get(String key) {
+        return values.get(key);
+    }
+}


오 HttpRequest의 body쪽 처리를 하는 클래스로 보이는데, 이 부분은 Request와 객체 지향적으로 연결을 안 맺고 따로 Controller에서 static을 이용하는 쪽으로 코드를 짜셨네요. 이유가 궁금합니다.

해당 클래스는 POST요청으로 RequestBody가 올때만 사용을 합니다. 따라서 모든 Request에서 가지고 있는 것 보단 POST요청으로 온 부분에서만 호출하도록 했습니다!

Yboyu0u · 2022-09-09T13:43:32Z

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

+    }
+
+    public String to() throws IOException {
+


개행이 되어 있네요🙃

수정했습니다!

Yboyu0u · 2022-09-09T13:45:33Z

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

+        mp.put("Content-Type", ContentType.from(request.getPath()));
+        mp.put("Content-Length", String.valueOf(resource.getBytes().length));


header field들을 상수로 빼서 관리하는 것도 괜찮을 것 같아요.

좋아요! 뺏습니다~!

Yboyu0u · 2022-09-09T13:49:45Z

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

+package org.apache.coyote.http11.request;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.catalina.HttpSession;
+import org.apache.catalina.SessionManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class HttpCookie {
+    private static final Logger log = LoggerFactory.getLogger(HttpCookie.class);
+    private static final int KEY_INDEX = 0;
+    private static final int VALUE_INDEX = 1;
+    private static final String REQUEST_COOKIE_NAME = "Cookie";
+    private static final String SESSION_ID = "JSESSIONID";
+    private static final String KET_VALUE_STANDARD = "=";
+
+    private final Map<String, String> values;
+
+    public HttpCookie() {
+        values = new LinkedHashMap<>();
+    }
+
+    private HttpCookie(Map<String, String> values) {
+        this.values = new LinkedHashMap(values);
+    }
+
+    public static HttpCookie extract(HttpHeaders httpHeaders) {
+        String cookie = httpHeaders.get(REQUEST_COOKIE_NAME);
+        log.info("cookie : {}", cookie);
+        if (cookie == null) {
+            return new HttpCookie();
+        }
+        Map<String, String> mp = new LinkedHashMap<>();
+        var datas = cookie.split(" ");
+        for (var data : datas) {
+            mp.put(data.split(KET_VALUE_STANDARD)[KEY_INDEX], data.split(KET_VALUE_STANDARD)[VALUE_INDEX]);
+        }
+        return new HttpCookie(mp);
+    }
+
+    public static HttpCookie ofJSessionId(String id) {
+        SessionManager sessionManager = new SessionManager();
+        HttpSession session = sessionManager.findSession(id);
+        return new HttpCookie(Map.of(SESSION_ID, session.getId()));
+    }
+
+    public String getResponse() {
+        var sb = new StringBuilder();
+        for (var entry : values.entrySet()) {
+            sb.append(entry.getKey()).append("=").append(entry.getValue());
+        }
+        return sb.toString();
+    }
+
+    public String getSession() {
+        log.info(SESSION_ID + ": {}", values.get(SESSION_ID));
+        return values.get(SESSION_ID);
+    }
+
+    public boolean isEmpty() {
+        return values.isEmpty();
+    }
+
+    @Override
+    public String toString() {
+        return "HttpCookie{" +
+                "values=" + values +
+                '}';
+    }
+}


쿠키 불변 객체 처리 멋있네요👍🏻👍🏻

감사합니당

Yboyu0u

기론 고생하셨습니다.👍🏻 별거 아닌 리뷰 하나 더 남겼는데 확인 한 번 해 보시면 좋을 것 같습니다.
궁금한 사항이 있는데 톰켓에서 세션이 어떻게 돌아가는지 아시나요?

Yboyu0u · 2022-09-12T08:11:10Z

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

+import java.util.Arrays;
+import org.apache.coyote.http11.utils.UrlParser;
+
+public enum ContentType {


Files.probeContentType(path)라는 메서드가 있더라고요. 한 번 알아보시면 좋을 것 같아요👍🏻

Yboyu0u · 2022-09-12T08:11:52Z

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

    public void stop() {
        stopped = true;
        try {
+            executorService.shutdown();


제가 놓쳤던 부분이었는데 기론 꺼 보고 수정했습니다ㅋㅋㅋㅋ👍🏻

Gyuchool added 16 commits September 3, 2022 22:47

refactor: change class name from extension to contentType

4cda4cf

fix: enter to login page without query string

ebe1bdf

docs: add funtion requierments

970f090

feat(step1): finish login redirect 302

d1a8f6a

test: refactor processor and add redirect test

6221e81

refactor: process and IO class seperate

bc735ca

docs: update readme

802f91c

refactor: use parser to httpRequest

b1e0b08

feat: join and login use to POST method

7da246c

refactor: url has HttpMethod attribute

7555354

test: register and login test refactor and add

ede702f

refactor: httpResponse return logic change

41f7358

feat: store JSESSIONID in cookie

b4417f1

feat: implement a session

2d473dd

refactor: code formatting

1eeecb3

refactor: httpRequest refactoring

9dc379c

Gyuchool added 5 commits September 8, 2022 11:12

feat: httpResponse refactoring

0aed009

feat: add controller interface

032ea53

docs: update readme

cb60e21

refactor: fix to go to clear controller url path

5d3090d

feat: apply threadpool

39bc028

Gyuchool changed the title ~~[톰캣 구현하기 - 2단계] 기론(김규철) 미션 제출합니다.~~ [톰캣 구현하기 - 2, 3, 4단계] 기론(김규철) 미션 제출합니다. Sep 8, 2022

refactor: delete unuse code

d3b2dc4

Yboyu0u suggested changes Sep 9, 2022

View reviewed changes

Gyuchool added 2 commits September 11, 2022 22:10

refactor: change buffer read logic position

8c4e003

refactor: extract as constant and change method naming

47e4bda

Yboyu0u approved these changes Sep 12, 2022

View reviewed changes

Yboyu0u merged commit 105ee2d into woowacourse:gyuchool Sep 12, 2022

		mp.put("Content-Type", ContentType.from(request.getPath()));
		mp.put("Content-Length", String.valueOf(resource.getBytes().length));

Conversation

Gyuchool commented Sep 7, 2022

Uh oh!

Gyuchool commented Sep 8, 2022

Uh oh!

Gyuchool commented Sep 8, 2022 • edited by Yboyu0u Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yboyu0u 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!

Yboyu0u 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gyuchool commented Sep 8, 2022 •

edited by Yboyu0u

Loading