[톰캣 구현하기 - 1, 2단계] 그레이(김동욱) 미션 제출합니다. #367

Kim0914 · 2023-09-04T08:34:18Z

안녕하세요 이리내 !
리뷰 잘 부탁드립니다 ㅎㅎ

우선 큰 구조는 다음과 같습니다.

HttpRequest에서 URL 추출
URL에 해당하는 핸들러 찾기
핸들러를 이용해 요청 처리 후 HttpResponse 반환

시간이 조금 부족해서 리다이렉트 부분과 테스트를 꼼꼼히 작성하지 못했어요 😅
리뷰 반영할 때 보충해서 추가할게요 ~~

감사합니다 🙇🏻

hectick

안녕하세요 그레이~
클래스 분리를 적절히 해주신 덕분에 흐름을 따라가면서 코드 리뷰를 수월하게 할 수 있었습니다!

포스트맨도 돌려보고, 궁금한점이나 개선했으면 하는 점 몇자 남겨보았는데요,
그래서 해야할일이 좀 늘었을 수도 있겠습니다만 적절히 셀렉해서 반영해주시면 되겠읍니다~

한번 쭉 보시고 수정할 부분 수정하셔서 pr 요청 주시면 마지할게요!

hectick · 2023-09-05T03:18:37Z

tomcat/src/main/java/org/apache/common/HttpMethod.java

+
+    public static HttpMethod of(String value) {
+        return Arrays.stream(values())
+                .filter(httpMethod -> Objects.equals(httpMethod.name, value))


httpMethod.name.equals(value) 대신 Objects.equals(httpMethod.name, value))를 사용한 이유가 있나요?
문자열 비교는 문자열.equlas(문자열)를 습관적으로 써와서 특별히 이렇게 하신 이유가 있는지 궁금해서 물어봅니당

null 안정성을 위해서 Objects.equals를 주로 사용합니다.

A.equals(B)에서 A가 상수이거나 null safe 하다면 문제가 되지 않지만,
A가 null이면 NullPointerException이 발생할 수 있어서요 !

그래서 습관적으로 Objects.equals를 써요 ~

hectick · 2023-09-05T03:26:27Z

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


-            final var responseBody = "Hello world!";
+            HttpRequest httpRequest = HttpRequest.of(bufferedReader);


커밋 순서대로 보려고 했는데 HttpRequest는 같이 커밋이 안되어있는 것을 발견했습니다!
뒤늦게 나눠서 커밋했나보죠? 🫨
네(you 아니고 four) 커밋 이후에 HttpRequest 파일이 생겼네요 깜짝 놀랐읍니다 조심해주세용~

네(four 아니고 yes)

hectick · 2023-09-05T05:16:12Z

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

+
+public class HttpRequest {
+
+    private final HttpLine httpLine;


네이밍이 포괄적인것 같아요.
구글에 검색해보니 http 요청 구조에서 첫줄은 start line, request line이라고 부르던데,
HttpStartLine이나 HttpRequestLine은 어떤가요?

hectick · 2023-09-05T05:18:45Z

tomcat/src/main/java/org/apache/request/HttpLine.java

+    private static void validate(String[] splits) {
+        if (splits.length < MIN_LINE_SIZE) {
+            throw new IllegalArgumentException("올바르지 않은 HTTP 요청입니다.");
+        }
+    }


예외처리 좋네용 👍

hectick · 2023-09-05T05:26:41Z

tomcat/src/main/java/org/apache/request/HttpHeaders.java

+                .collect(collectingAndThen(
+                        toMap(line -> line[HEADER_KEY_INDEX], line -> line[HEADER_VALUE_INDEX]),
+                        HttpHeaders::new)


오오 좋은거 배워갑니다잉

hectick · 2023-09-05T06:30:24Z

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

+
+    public static ContentType of(String value) {
+        return Arrays.stream(values())
+                .filter(contentType -> Objects.equals(contentType.getValue(), value))


getValue()대신 그냥 value를 쓸 수 있는걸요~

Suggested change

.filter(contentType -> Objects.equals(contentType.getValue(), value))

.filter(contentType -> Objects.equals(contentType.value, value))

hectick · 2023-09-05T06:30:51Z

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

+    TEXT_CSS("text/css"),
+    TEXT_HTML("text/html"),
+    APPLICATION_JS("application/js"),
+    PERMIT_ALL("*/*");


permit all.. 좋네요 메모..

hectick · 2023-09-05T06:34:53Z

tomcat/src/main/java/org/apache/common/HttpStatus.java

+    OK("200"),
+    FOUND("302"),
+    UNAUTHORIZED("401"),
+    CONFLICT("409");


CONFLICT는 어떤 상황에 필요한 것인지 궁금해요

HttpStatus에 대해서 찾아보다가 서버의 현재 상태와 요청이 충돌된 경우(버져닝과 관련된..?)이 있길래 추가해봤었는데
사용하지 않아서 삭제하도록 하겠습니당

hectick · 2023-09-05T06:37:10Z

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

+    private final ContentType contentType;
+    private final String body;
+    private final HashMap<String, String> headers;
+
+    public HttpResponse(HttpStatus httpStatus, ContentType contentType, String body) {
+        this.headers = new LinkedHashMap<>();
+        this.httpStatus = httpStatus;
+        this.contentType = contentType;
+        this.body = body;
+        headers.put(CONTENT_TYPE, contentType.getValue() + ";charset=utf-8 ");
+        headers.put(CONTENT_LENGTH, body.getBytes().length + " ");
+    }


ContentType 필드를 headers 안에 저장하는데, 따로 필드로 들고있는 이유가 있나요?

마감시간에 쫓겨서 급하게 마무리한다고 그랬었는데 리팩토링할게요 ㅎㅎ

hectick · 2023-09-05T06:37:45Z

tomcat/src/main/java/org/apache/common/Content.java

@@ -0,0 +1,20 @@
+package org.apache.common;
+
+public class Content {


안쓰는 클래스 같아용
무슨 용도였나요?

body를 감싸려고 했었는데 다른 클래스를 사용하고 있어서 삭제해도 되겠네요 !

sonarcloud · 2023-09-06T05:29:30Z

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

Kim0914 · 2023-09-06T05:30:34Z

소중한 피드백 반영했습니다 ㅎㅎ

부족한 테스트도 함께 추가했어요 !

감사합니다 ~~

hectick

안녕하세요 그레이~
변경해주신 부분과 테스트 모두 잘 확인했습니다!
추가로 코멘트도 몇개 남겨보았어요!! 다만 아직 해결되지 못한 미스테리가 하나 남아있는데 이건 저도 열심히 찾아보겠습니다🥲
그럼 이만 마지할게용~

hectick · 2023-09-06T06:30:41Z

tomcat/src/main/resources/static/405.html

+        <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no" />
+        <meta name="description" content="" />
+        <meta name="author" content="" />
+        <title>404 Error - SB Admin</title>


Suggested change

<title>404 Error - SB Admin</title>

<title>405 Error - SB Admin</title>

hectick · 2023-09-06T06:33:06Z

tomcat/src/main/java/org/apache/handler/DefaultHandler.java

+        if (!httpRequest.isGet()) {
+            String content = FileReader.read(METHOD_NOT_ALLOWED_PAGE);
+            HttpResponse httpResponse = new HttpResponse(HttpStatus.METHOD_NOT_ALLOWED, content);
+            httpResponse.setContentType(ContentType.TEXT_HTML);
+            return httpResponse;


오늘 수업시간에 잠깐 나왔던 내용이라 반가워서 링크 달아보아용

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405#:~:text=The%20server%20must%20generate%20an%20Allow%20header%20field%20in%20a%20405%20status%20code%20response.%20The%20field%20must%20contain%20a%20list%20of%20methods%20that%20the%20target%20resource%20currently%20supports.

hectick · 2023-09-06T06:37:05Z

tomcat/src/main/java/org/apache/handler/LoginHandler.java

+        String[] accountQuery = queries[0].split("=");
+        String[] passwordQuery = queries[1].split("=");


쿼리 파라미터의 순서가 다르게 오는 상황도 있을까요?
ex) password=password&account=gugu

hectick · 2023-09-06T07:10:59Z

tomcat/src/main/java/org/apache/request/HttpLine.java

+    public static HttpLine of(String line) {
+        String[] splits = splitLine(line);
+        validate(splits);
+        String httpMethod = splits[HTTP_METHOD_INDEX];
+        String requestTarget = splits[RESOURCE_INDEX];
+        String httpVersion = splits[HTTP_VERSION_INDEX];
+        return new HttpLine(HttpMethod.of(httpMethod), requestTarget, httpVersion);
+    }
+
+    private static String[] splitLine(String line) {
+        return line.split(REQUEST_SPLIT_DELIMITER);
+    }


일단 null 처리를 하긴 했지만 저도 아직 정확히 알지는 못합니다...
현재 코드를 돌려보면, 크롬에서 동작할때는 한 요청당 하나의 스레드가 생기지만,
포스트맨에서 요청을 보낼 때는 한 요청에 두개의 스레드가 생기고, 그 중 첫번째 스레드가 null pointer exception이 터지더라구요.
본격적인 요청을 보내기 전에 서버에 일단 아무거나 보내보는걸까요?
혹시 원인을 먼저 알게되면 공유 부탁드립니당..ㅠ

Kim0914 added 16 commits September 2, 2023 18:47

test: IOStream 테스트 작성

e26b3e6

test: File 테스트 작성

cff01c6

feat: Http Method 객체 추가

9427171

feat: Http Line 객체 추가

ac49917

feat: Http Headers 객체 추가

01477f0

feat: Http Body 객체 추가

374b40c

feat: css를 포함한 index.html을 응답하도록 구현

2bc2b9e

feat: Query String 파싱 후 회원 조회 로그 구현

438f5a5

chore: favicon 에러 안보이도록 임시 처리

e341b44

feat: 로그인 성공 시 302, 실패 시 401을 반환하도록 구현

4e4d548

feat: 회원 가입 기능 구현

fd1f89b

refactor: 요청 URL에 맞는 핸들러 찾는 기능 구현

74c8ed4

feat: 일치하는 URL이 없을 때 404 페이지 반환

c29bb1b

feat: 로그인 시 세션 발급 기능 추가

73af359

test: 핸들러 어뎁터 테스트 추가

f3b24f2

test: 파일 읽기 테스트 추가

cc2d04f

Kim0914 requested a review from hectick September 4, 2023 08:34

Kim0914 self-assigned this Sep 4, 2023

fix: HttpHeaders 줄바꿈 삭제

451e57b

hectick requested changes Sep 5, 2023

View reviewed changes

Kim0914 added 10 commits September 5, 2023 17:35

fix: 로그인 세션 검증 수정 및 테스트 추가

099203e

chore: HttpLine 변수 명 변경

40a05af

feat: HttpStartLine에 빈 값 들어오면 예외 발생

b3523b4

chore: 사용하지 않는 메서드 삭제

59819a1

refactor: ContentType enum을 사용하도록 변경

d5cf7e6

feat: view 응답시 method를 지원하지 않으면 405 페이지 반환

09d7ccd

refactor: 사용하지 않는 HttpStatus 삭제 및 테스트 메시지 수정

4b22899

refactor: HttpResponse 생성자 필드 변경

d3e2013

refactor: redirect를 위한 Location 필드 추가

4e3e50f

test: 실패하는 테스트 수정

d3861ef

Kim0914 added 6 commits September 5, 2023 22:47

chore: 패키지 위치 변경

0b00cb2

test: 세션 테스트 추가

5f81015

test: 쿠키 테스트 추가

b3e215e

test: HttpRequest 테스트 추가

30f11ce

refactor: 핸들러 어뎁터 쿼리 스트링 메서드 분리

68bd7d0

refactor: 일치하는 메서드가 없을 시 405를 반환하도록 일괄 수정

e38ed79

hectick approved these changes Sep 6, 2023

View reviewed changes

hectick merged commit b30ffd1 into woowacourse:kim0914 Sep 6, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 1, 2단계] 그레이(김동욱) 미션 제출합니다. #367

[톰캣 구현하기 - 1, 2단계] 그레이(김동욱) 미션 제출합니다. #367

Kim0914 commented Sep 4, 2023

hectick left a comment

hectick Sep 5, 2023

Kim0914 Sep 5, 2023 •

edited

Loading

hectick Sep 5, 2023

Kim0914 Sep 5, 2023

hectick Sep 5, 2023

hectick Sep 5, 2023

hectick Sep 5, 2023

hectick Sep 5, 2023

hectick Sep 5, 2023

hectick Sep 5, 2023

Kim0914 Sep 5, 2023

hectick Sep 5, 2023

Kim0914 Sep 5, 2023

hectick Sep 5, 2023

Kim0914 Sep 5, 2023

sonarcloud bot commented Sep 6, 2023

Kim0914 commented Sep 6, 2023

hectick left a comment

hectick Sep 6, 2023

hectick Sep 6, 2023

hectick Sep 6, 2023

hectick Sep 6, 2023


		final var responseBody = "Hello world!";
		HttpRequest httpRequest = HttpRequest.of(bufferedReader);

	.filter(contentType -> Objects.equals(contentType.getValue(), value))
	.filter(contentType -> Objects.equals(contentType.value, value))

		@@ -0,0 +1,20 @@
		package org.apache.common;

		public class Content {

	<title>404 Error - SB Admin</title>
	<title>405 Error - SB Admin</title>

		String[] accountQuery = queries[0].split("=");
		String[] passwordQuery = queries[1].split("=");

[톰캣 구현하기 - 1, 2단계] 그레이(김동욱) 미션 제출합니다. #367

[톰캣 구현하기 - 1, 2단계] 그레이(김동욱) 미션 제출합니다. #367

Conversation

Kim0914 commented Sep 4, 2023

hectick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kim0914 Sep 5, 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

sonarcloud bot commented Sep 6, 2023

Kim0914 commented Sep 6, 2023

hectick 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

Kim0914 Sep 5, 2023 •

edited

Loading