[톰캣 구현하기 - 1, 2단계] 레오(차영호) 미션 제출합니다. #309

youngh0 · 2023-09-03T16:09:13Z

안녕하세요 박스터!

톰캣 미션 제출합니다!!

전체적인 흐름은 사진과 같고, 정적파일을 읽어야 할 때는 FileReader를 통해 읽어오고 있습니다.

HttpRequest 객체는 request의 전반적인 정보를 담고 있습니다. 그래서 처음 입력받은 BufferedReader를 통해 정보를 파싱하는데 코드 수정이 좀 필요할 것 같아요 ㅎ.. (괜찮은 방법 있으면 조언 부탁드립니다..)
Handler는 request method, endpoint 등등을 보고 적절하게 파일을 읽거나, 리다이렉션을 시켜서 요청을 처리하는 역할을 하고 있습니다.
ResponseEntity는 reponse status, header, body 등을 조합해 응답 String을 만들고 있습니다.

drunkenhw

안녕하세요 코드 잘 봤습니다.
handler로 매핑하는 부분 멋집니다.
몇가지 코멘트 남겼습니다. 확인해주세요

drunkenhw · 2023-09-04T01:06:26Z

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

+    JS("text/javascript", "js");
+
+    private final String contentType;
+    private final String matchingType;


matching type이라는 변수명보다 좋은 것은 없을까요?

확장자 성격이라 extension 으로 수정했습니다😀

drunkenhw · 2023-09-04T01:07:05Z

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

+    private static final Map<String, ContentType> contentTypes =
+            Collections.unmodifiableMap(Stream.of(values())
+                    .collect(Collectors.toMap(ContentType::getMatchingType, Function.identity()))
+            );


map으로 하는 것 좋아보여요
하지만 요건 상수인데 선언 위치를 이동하는 것은 어떤가요?

동의합니다!

해당 map의 위치를 enum 변수인 httpContentType, extension 위에 놓을까 고민하다가 ENUM객체의 각 변수들에 대한 정보라 ENUM객체 -> 변수명 -> map 순으로 배치했습니다!

drunkenhw · 2023-09-04T01:08:30Z

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

+    public static ContentType findMatchingType(String uri) {
+        int fileTypeStartIndex = uri.indexOf('.');
+        String uriType = uri.substring(fileTypeStartIndex + 1);
+
+        if (contentTypes.containsKey(uriType)) {
+            return contentTypes.get(uriType);
+        }
+        return contentTypes.get(HTML.matchingType);
+    }


uri를 받아서 자르고 가공해서 반환하는 것보다 matching type을 받아서 반환하는 것은 어떤가요?
그리고 if()~~~ 를 contentype.getOrDefault로 해도 좋을 것 같아요

ResponseEntity에서 한 번만 가공해서 보내주면 좋겠네요!

감사합니다 ㅎㅎ

drunkenhw · 2023-09-04T01:09:28Z

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

+        Map<String, String> cookies = Arrays.stream(cookieValue.split(COOKIE_DELIMITER))
+                .map(cookieElement -> cookieElement.trim().split("="))
+                .collect(Collectors.toMap(cookie -> cookie[0], cookie -> cookie[1]));
+        return new Cookie(cookies);


왜 세미콜론만 상수화 하셨죠? 차별하시나요?
그리고 collecting then이라는 것을 사용하면 한번에 만들 수 있을 것 같아요, 물론 가독성측면에서 취향 차이일 것 같지만요

상수화 했습니다 ㅎ..

다른 클래스도 산수화 못한거 하겠습니다!

drunkenhw · 2023-09-04T01:09:45Z

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

+    private FileReader() {
+
+    }


private good

it's good ~

drunkenhw · 2023-09-04T01:24:16Z

tomcat/src/main/java/org/apache/coyote/http11/http/ResponseEntity.java

+    private final ResponseStatus responseStatus;
+    private final List<String> responseHeaders;
+    private final String responseBody;
+    private final Map<String, String> cookies = new HashMap<>();


왜 열심히 만들어놓으신 cookie 클래스를 사용안하시고..

그러게요,, 수정했습니다

drunkenhw · 2023-09-04T01:24:47Z

tomcat/src/main/java/org/apache/coyote/http11/http/ResponseEntity.java

+        List<String> headers = List.of(
+                String.join(" ", "Content-Type:", ContentType.findMatchingType(endPoint).getContentType()),
+                String.join(" ", "Content-Length:", String.valueOf(fileData.getBytes().length))
+        );


header를 보니 list보다 다른 자료구조가 좋을 것 같아보이네용

Map 으로 수정했습니다!

List 사용한 이유가 기존 테스트코드는 isEqualTo로 response 검증해서 Map을 사용하면 header 정보를 response로 만들 때 Map 내부 header 정보들의 순서가 섞여서 LinkedHashMap 쓰려다가 그냥 List 사용했습니다.
지금은 test코드를 isEqualTo 검증에서 contains 검증으로 수정했습니다~

drunkenhw · 2023-09-04T01:26:04Z

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

+        String requestFirstLine = request.readLine();
+        String[] requestLine = validateRequestFirstLine(requestFirstLine);
+
+        RequestMethod requestMethod = RequestMethod.find(requestLine[METHOD_INDEX]);
+        String requestUri = requestLine[URI_INDEX];
+        HttpVersion httpVersion = HttpVersion.find(requestLine[HTTP_VERSION_INDEX]);
+
+        int queryStringIndex = requestUri.indexOf(QUERY_STRING_DELIMITER);
+        String endPoint = extractEndPoint(requestUri, queryStringIndex);
+        Map<String, String> queryStrings = extractQueryStrings(requestUri, queryStringIndex);
+
+        RequestHeader requestHeader = RequestHeader.from(request);
+        int contentLength = requestHeader.getContentLength();
+        if (contentLength > 0) {
+            char[] buffer = new char[contentLength];
+            request.read(buffer, 0, contentLength);
+            String requestBody = new String(buffer);
+            queryStrings = parseQueryStrings(requestBody);
+        }


클래스를 나누어 해당 객체에 parsing하는 책임을 주면 보다 코드가 깔끔해질 것 같은데 3단계에 리팩토링이 있으니 그때 하셔도 좋을 것 같네요

queryString, responseBody 정보를 파싱해서 들고 있는 RequestData 객체를 추가해서 데이터 파싱 책임을 분리해서 가독성을 높여봤습니다!

drunkenhw · 2023-09-04T01:28:56Z

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

-                    "Content-Length: " + responseBody.getBytes().length + " ",
-                    "",
-                    responseBody);
+            BufferedReader br = new BufferedReader(new InputStreamReader(inputStream));


br 조금 불편하네요 ㅋ

ㅎㅎ 수정했습니다

drunkenhw · 2023-09-04T01:31:15Z

tomcat/src/main/java/org/apache/coyote/http11/handler/Handlers.java

+
+    private static final Pattern FILE_PATTERN = Pattern.compile("css|js|ico|html");
+
+    private static final Map<String, Handler> myHandlers =


핸들러를 멋지게 분리하셨는데 이 구현체 핸들러들의 패키지를 이동해봐도 좋을 것 같아요

어떻게 이동하는게 좋을까요..? 마땅히 떠오르지 않네요..

저라면 jwp 패키지 아래에 뒀을 것 같아용 apache 패키지가 inmemory user repository를 알고 있다는 것이 별로인 것 같아서요.
물론 저는 추상화를 3단계에 하려고 다 apache에 있는데 레오는 추상화를 멋지게 해놔서 말씀드려봅니다.

sonarcloud · 2023-09-04T17:10:54Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

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

youngh0 · 2023-09-05T04:27:39Z

꼼꼼한 리뷰 감사합니다 ^.^

HttpRequest를 만들 때 queryString이나 requestBody로 들어온 데이터를 파싱 후 가지고 있는 RequestData 를 추가해 책임 분리를 통해 가독성을 높여봤습니다.

그리고 Response의 Header 정보를 Map을 통해 관리하도록 수정했습니다!!

drunkenhw

안녕하세요 레오
코드 잘 봤습니다.
아주 깔끔하고 좋았습니다. 많이 배웠습니다
궁금한 것 위주로 코멘트 달았습니다.
수고하십시오 머지 하겠습니다

drunkenhw · 2023-09-05T04:29:10Z

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

+        if (!SESSIONS.containsKey(id)) {
+            throw new IllegalArgumentException("존재하지 않는 세션입니다.");
+        }


drunkenhw · 2023-09-05T04:29:22Z

tomcat/src/main/java/org/apache/coyote/http11/handler/FileHandler.java

        String fileData = FileReader.readFile(request.getEndPoint());
-        return ResponseEntity.ok(fileData, request.getEndPoint());
+        return ResponseEntity.ok(fileData, ContentType.findMatchingType(fileExtension));


drunkenhw · 2023-09-05T04:31:40Z

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

    private final String responseBody;
-    private final Map<String, String> cookies = new HashMap<>();
+    private final Cookie cookie = Cookie.from("");


cookie도 정적 팩토리 메서드에서 같이 생성해주는 것은 어떤가요>

drunkenhw · 2023-09-05T04:34:21Z

tomcat/src/main/java/org/apache/coyote/http11/handler/Handlers.java

+
+    private static final Pattern FILE_PATTERN = Pattern.compile("css|js|ico|html");
+
+    private static final Map<String, Handler> myHandlers =


저라면 jwp 패키지 아래에 뒀을 것 같아용 apache 패키지가 inmemory user repository를 알고 있다는 것이 별로인 것 같아서요.
물론 저는 추상화를 3단계에 하려고 다 apache에 있는데 레오는 추상화를 멋지게 해놔서 말씀드려봅니다.

youngh0 added 30 commits September 2, 2023 15:39

test: classLoader를 이용해 file의 경로 찾는 테스트

60d0fd4

test: file 내용 읽는 테스트

9e8e8a3

feat: html 파일 응답

e0606ff

feat: request 첫 줄 파싱 구현

80b33fb

feat: request header 파싱 구현

5efccaa

feat: request header 파싱 구현

d4cc8f0

test: 학습 테스트 작성

07d046f

feat: uri에 맞는 contentType 반환

3ac7f2d

feat: enum 작성

d5ca376

feat: reqeust객체 구현

9bb7106

feat: root handler, file handler 구현

97874cf

feat: 요청 경로에 맞는 handler 호출하도록 일급컬렉션 구현

548be47

feat: response 객체 생성

8818748

feat: 요청에 맞는 핸들러 호출하여 response응답 로직 작성

f602ebd

feat: http request method enum 생성

fb99b04

feat: queryString 파싱

93d34fc

feat: login핸들러 구현

d5d6be6

refactor: HttpRequest 구조 수정

90cf205

feat: 로그인 성공 시 index.html 로 리다이렉션

13cb305

feat: 로그인 실패 시 401.html 로 리다이렉션

54d1706

test: file handler의 response body 검증 테스트 작성

4be7f27

test: login handler 리다이렉션 테스트

a43b33e

feat: register get 요청 핸들러 구현

24a1aa8

feat: post 메서드 추가

9e4f77e

feat: queryString, requestBody 파싱

99b59af

refactor: 불필요한 개행 제거

7521a0a

feat: registerHandler 추가

5fa3591

feat: method 로깅 추가

83ae365

feat: contentLength 반환 로직

87656fb

fix: requestBody 파싱 로직 수정

064e8de

youngh0 added 3 commits September 4, 2023 01:27

refactor: 불필요한 throws 제거

1f255ec

refactor: 테스트 코드 수정

865d053

refactor: private 생성자 추가

726480b

drunkenhw requested changes Sep 4, 2023

View reviewed changes

youngh0 added 20 commits September 4, 2023 14:10

feat: requestData 객체 생성, queryString과 requestBody 파싱 역할

dc31e1e

reafctor: HttpRequest 에 RequestData 객체 적용

81e4897

refactor: ContentType 변수 명 수정

fafeba1

refactor: static 위치 수정

1a3e896

refactor: ContentType find 로직 수정

09a3871

refactor: 응답 구분자 상수화

fdd145d

refactor: 쿠키 파싱 구분자 상수화

7ef6666

refactor: 정적 파일 요청에 맞는 핸들러 반환 로직 수정

2c8f3ff

refactor: 세션 저장 로직 수정

78f1dc7

refactor: ResponseEntity 생성 로직 수정

300d9d3

refactor: ResponseEntity 생성 로직 수정

fa58981

refactor: ResponseEntity ok 응답 메서드 파라미터 enum ContentType 받도록 수정

128ba61

refactor: header 정보 Map 으로 관리

108ff52

refactor: header 정보 contains 로 테스트

d9f9d86

test: 파일 확장자 별 ContentType 검증 추가

0976f83

refactor: 코드 위치 수정

8000976

chore: request 패키지 생성

a19a688

chore: ResponseEntity 패키지 이동

8feb647

refactor: ResponseEntity 에서 Cookie 객체 사용

d35b08b

refactor: return 타입 수정

e260d46

youngh0 added 2 commits September 5, 2023 13:23

feat: session find 방어로직 추가

3e8fa9e

test: session find 방어로직 테스트

9894a77

drunkenhw approved these changes Sep 5, 2023

View reviewed changes

drunkenhw merged commit 169ad13 into woowacourse:youngh0 Sep 5, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1, 2단계] 레오(차영호) 미션 제출합니다. #309

[톰캣 구현하기 - 1, 2단계] 레오(차영호) 미션 제출합니다. #309

youngh0 commented Sep 3, 2023

drunkenhw left a comment

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 4, 2023

youngh0 Sep 4, 2023

drunkenhw Sep 5, 2023

sonarcloud bot commented Sep 4, 2023

youngh0 commented Sep 5, 2023

drunkenhw left a comment

drunkenhw Sep 5, 2023

drunkenhw Sep 5, 2023

drunkenhw Sep 5, 2023

drunkenhw Sep 5, 2023


		private static final Pattern FILE_PATTERN = Pattern.compile("css\|js\|ico\|html");

		private static final Map<String, Handler> myHandlers =

[톰캣 구현하기 - 1, 2단계] 레오(차영호) 미션 제출합니다. #309

[톰캣 구현하기 - 1, 2단계] 레오(차영호) 미션 제출합니다. #309

Conversation

youngh0 commented Sep 3, 2023

drunkenhw 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

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

sonarcloud bot commented Sep 4, 2023

youngh0 commented Sep 5, 2023

drunkenhw 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