[1 - 2 단계 Tomcat 구현하기] 호티(윤주호) 미션 제출합니다. #563

Ho-Tea · 2024-09-06T08:41:10Z

안녕하세요 아토!!
톰캣 구현하기 2-4단계까지 구현하였습니다.
3단계 리팩토링을 위해 2단계에서 구현에만 집중하다 보니 코드가 너무너무 지저분한 것 같네요ㅠㅠ
미리 죄송합니다 🙇‍♂️
가독성이 떨어지는 부분, 불편한 부분 과감히 말씀해주시면 3단계 리팩토링을 할 때 더 좋은 방향으로 반영될 수 있을 것 같아요!
잘 부탁드립니다 😊

참고

2번째 학습 테스트 testCompression()

    @Test
    void testCompression() {
        final var response = webTestClient
                .get()
                .uri("/")
                .exchange()
                .expectStatus().isOk()

                // gzip으로 요청 보내도 어떤 방식으로 압축할지 서버에서 결정한다.
                // 웹브라우저에서 localhost:8080으로 접근하면 응답 헤더에 "Content-Encoding: gzip"이 있다.
                .expectHeader().valueEquals(HttpHeaders.TRANSFER_ENCODING, "chunked")
                .expectBody(String.class).returnResult();

        log.info("response body\n{}", response.getResponseBody());
    }

(cherry picked from commit fed02f6)

(cherry picked from commit 7e91356)

hyxrxn

호티 안녕하세요! 아토입니다~~
낮 중으로 완료하려 했는데 하다보니 좀 늦어졌네요.

그리고 질문도 꽤... 많습니다.
뭔가 호티의 스타일이 있는 것 같아 보여요!
편하게 답변주시면 됩니다 ㅎㅎ

대부분의 코멘트도 제안이니 생각해보시고 이번 단계에서 반영할 부분만 반영해주세요~
그런데 의도한대로 동작하지 않는 것이 몇 가지 있어 일단은 RC 드립니다!

각 페이지를 요청할 때 간헐적으로 로드가 제대로 되지 않습니다.

로그인 성공 이후 /login 에 GET 요청시 index 페이지가 아닌 login 페이지가 보입니다.

확인해보시고 코멘트나 DM으로 연락 주세요!
캠퍼스에서 부르셔도 좋습니다.
화이팅입니다~

hyxrxn · 2024-09-08T10:17:19Z

tomcat/src/test/java/org/apache/coyote/http11/Http11ProcessorTest.java

@@ -12,6 +13,7 @@

 class Http11ProcessorTest {

+    @Disabled


이거... 깨지면 안 되는 테스트 아닌가요?!
왜 이 어노테이션이 붙어있을까요?!

1-2단계를 구현하면서 실행결과쪽을 살펴보면 http://localhost:8080 으로 접속 시 인덱스 페이지가 보여지게 되는 것을 확인할 수 있었습니다.
따라서 "/" url 접근시 Hello World를 출력하는 테스트코드는 필요 없어졌다 생각되어 @Disabled 처리했습니다!

실행 결과 예시에 그런 부분이 있었군요~ 제가 놓쳤네요.
확인되었습니다!

hyxrxn · 2024-09-08T12:26:41Z

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

+        String line = bufferedReader.readLine();
+        while (!line.isEmpty()) {
+            StringTokenizer tokenizer = new StringTokenizer(line, ":");
+            String key = tokenizer.nextToken().trim();
+            String value = tokenizer.nextToken(":").trim();
+            headerMap.put(key, value);
+            line = bufferedReader.readLine();
+        }


Suggested change

String line = bufferedReader.readLine();

while (!line.isEmpty()) {

StringTokenizer tokenizer = new StringTokenizer(line, ":");

String key = tokenizer.nextToken().trim();

String value = tokenizer.nextToken(":").trim();

headerMap.put(key, value);

line = bufferedReader.readLine();

}

String line;

while (!(line = bufferReader.readLine()).isEmpty()) {

StringTokenizer tokenizer = new StringTokenizer(line, ":");

String key = tokenizer.nextToken().trim();

String value = tokenizer.nextToken(":").trim();

headerMap.put(key, value);

}

라인을 위에서도 읽고 while문 마지막에서도 읽는 게 조금 어색해보이는데 이 방법은 어떨까요?

이게 더 깔끔해 보이네요! 반영했습니다 😊

hyxrxn · 2024-09-08T14:40:43Z

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

+                Map<RequestLine, String> requestLineElements = extractRequestLine(bufferedReader);
+                Map<String, String> headers = parseHeaders(bufferedReader);
+                String body = parseBody(bufferedReader, headers);
+                HttpResponse httpResponse = HttpResponse.from("HTTP/1.1");


new HttpReponse("HTTP/1.1") 를 사용하지 않고 private으로 막아둔 후, static과 this를 활용한 이유가 궁금해요.

정적 팩터리 메서드를 통해 어떠한 인자를 통해 객체가 생성되는지 명확히 하고싶은 이유가 있었습니다만,
다시 돌아보니 from은 모호한 표현인 것 같아 new HttpReponse("HTTP/1.1")로 수정하였습니다

hyxrxn · 2024-09-08T14:43:06Z

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

        } catch (IOException | UncheckedServletException e) {
            log.error(e.getMessage(), e);
        }
    }
+
+    private static Map<RequestLine, String> extractRequestLine(BufferedReader bufferedReader) throws IOException {


이 메서드와 밑의 메서드들도 static인 이유가 궁금합니다!

인텔리제이에서 메서드 추출 기능을 사용하면 private static으로 만들어주는데 아래와 같은 이유에 근거하기에 사용하였습니다!

객체 인스턴스에 의존하지 않는 로직
만약 추출된 코드가 클래스의 인스턴스 필드나 메서드에 의존하지 않고, 단지 입력값(파라미터)만으로 동작할 수 있는 경우, static으로 선언하는 것이 합리적입니다.
static 메서드는 클래스 레벨에서 동작하며 인스턴스 레벨에서 불필요한 상태나 의존성이 없을 때 더 간결하고 성능 면에서도 효율적일 수 있습니다.

메모리 효율성
static 메서드는 클래스 로딩 시 한 번만 메모리에 로드되기 때문에, 객체 인스턴스마다 메서드를 별도로 가지고 있을 필요가 없습니다. 따라서, 객체마다 중복되는 메서드를 피하고 메모리를 절약할 수 있습니다.

가시성 최소화 (캡슐화 원칙)
IntelliJ는 추출된 메서드를 private로 선언하여 가시성을 최소화하는 경향이 있습니다. 이는 정보 은닉을 강조하며, 클래스 외부에서는 해당 메서드를 직접 호출할 수 없도록 하여 클래스의 내부 구현이 외부에 노출되지 않도록 보호합니다.
추출된 메서드가 다른 곳에서 사용되지 않는 한 최소한의 가시성을 부여하는 것이 객체지향 설계 원칙에 부합합니다.

디폴트 동작
많은 리팩토링 도구는 기본적으로 static 메서드를 선호합니다. 왜냐하면 추출된 코드가 굳이 객체 인스턴스에 의존할 필요가 없다면, 이를 굳이 비 static 메서드로 선언하는 것은 불필요하게 클래스의 인스턴스 레벨에서 관리하게 되는 것을 의미합니다.
메서드를 static으로 선언하는 것은 클래스 자체의 유틸리티 성격을 강화하고, 불필요한 상태와 의존성을 피할 수 있게 합니다.

성능 측면
static 메서드는 호출 시 객체의 메모리에서 메서드 테이블을 탐색하지 않아도 되므로 비 static 메서드 호출보다 성능이 약간 더 좋을 수 있습니다. 물론 일반적인 경우엔 큰 차이가 없지만, 많은 호출이 발생하는 경우 이를 최적화하는 것도 하나의 이유입니다.

유틸리티 클래스로 보셨군요
확인했습니다~

hyxrxn · 2024-09-08T16:12:33Z

tomcat/src/main/java/org/apache/catalina/connector/http/HttpResponse.java

+        if (Objects.isNull(httpStatus)) {
+            return StringUtils.EMPTY;
+        }


httpStatus == null 대신 Objects.isNull(httpStatus)를 사용하고,
"" 대신 StringUtils.EMPTY를 사용한 이유가 있나요??

httpStatus == null 대신 Objects.isNull(httpStatus)를 사용한 이유

우선 null 체크가 의도된것임을 명확하게 드러내기 위함이였습니다. (== null은 그자체로 비교연산이기에)

null 체크를 더 읽기 쉽게 하고 명시적인 의미를 부여하기 위한 메서드로 isNull 메서드가 Java 8에서 추가되어서 자주 사용할려고 습관을 들였습니다!

"" 대신 StringUtils.EMPTY를 사용한 이유

해당 부분도 위의 코멘트와 비슷하게 의미를 명확하게 하기 위해서 사용하였습니다.

또한 ""로 표현한 경우 오타가 발생할 수 있지만 StringUtils.EMPTY의 경우 오타가 발생하지 않는 점에서 휴먼에러를 사전에 방지할 수 있는 장점이 있어 사용하였습니다!

합당해보이네요 👍

hyxrxn · 2024-09-08T16:24:34Z

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

+        return Map.of(RequestLine.HTTP_METHOD, requestLineElements[0],
+                RequestLine.REQUEST_URI, requestLineElements[1],
+                RequestLine.HTTP_VERSION, requestLineElements[2]);


오 이런 방식으로 enum을 사용하셨군요! key 값을 원하는 것들로 딱 한정지을 수 있다는 점이 매력적이네요.

hyxrxn · 2024-09-08T16:29:25Z

README.md

+### 4. Session 구현하기
+- [x] 쿠키에서 전달 받은 `JSESSIONID`의 값으로 로그인 여부를 체크할 수 있어야 한다.
+- [x] 로그인에 성공하면 Session 객체의 값으로 User 객체를 저장해보자.
+- [x] 로그인된 상태에서 /login 페이지에 HTTP GET method로 접근하면 이미 로그인한 상태니 index.html 페이지로 리다이렉트 처리한다.


이 기능 잘 동작하고 있나요?
로그인 하고 /login 에 GET으로 요청하니 login 페이지가 그대로 보이는 것 같네요 ㅠㅠ

실제로 테스트할 때 항상 쿠키를 모두 삭제하고 실행했기에 (쿠키가 없는 상태에서의 접근만 생각)
애플리케이션을 재실행 했을 시 전에 저장되어있는 쿠키가 있어 발생하는 에러였습니다ㅠ
수정하였습니다!

이 기능과 더불어 페이지 로딩 제대로 되는 부분까지 확인 되었습니다!

페이지 로딩은 ready() 메서드 때문이었군요
메서드를 부를 때 도착해있는 게 없으면 false가 뜨고 if문 안의 로직을 수행시키지 못하는 것으로 이해했는데 맞나요?!

hyxrxn · 2024-09-08T16:32:26Z

tomcat/src/main/java/org/apache/catalina/engine/CatalinaServletEngine.java

+            return Files.probeContentType(Paths.get(Objects.requireNonNull(CatalinaServletEngine.class.getClassLoader()
+                    .getResource("static" + url)).toURI()));


으아아악 점이 너무 많아요.....!!!!!!!!!
나눠주시면 정말 좋겠습니다...

으아아아악 getStaticResourceURL 메서드를 통해 분리하였습니다!

여전히 점이 꽤 있긴 하지만 읽기 힘든 정도는 아니네요!
넘어갑시닷

hyxrxn · 2024-09-08T16:33:41Z

tomcat/src/main/java/org/apache/catalina/engine/CatalinaServletEngine.java

+    private static void responseLoginPage(HttpResponse response) {
+        String contentType = probeContentType("/login.html");
+        String content = findStaticFile("/login.html");
+        response.addHttpStatus(HttpStatus.OK);
+        response.addHeader("Content-Type", contentType);
+        response.setBody(content);
+    }
+
+    private static void responseRegisterPage(HttpResponse response) {
+        String contentType = probeContentType("/register.html");
+        String content = findStaticFile("/register.html");
+        response.addHttpStatus(HttpStatus.OK);
+        response.addHeader("Content-Type", contentType);
+        response.setBody(content);
+    }


로직이 완전히 동일하군요!
합칠 수... 있겠죠?

hyxrxn · 2024-09-08T16:37:48Z

tomcat/src/main/java/org/apache/catalina/engine/CatalinaServletEngine.java

+        if (optionalUser.isEmpty()) {
+            loginWithInvalidAccount(response, account);
+            return;
+        }
+        User user = optionalUser.get();
+        if (user.checkPassword(password)) {
+            normalLogin(httpCookie, response, user);
+            return;
+        }
+        loginWithInvalidPassword(response, user, password);


로그인 상태를 3가지로 나누어서 처리해주셨군요!
모든 경우에서 로그가 함께 찍혀 어떤 메서드가 언제 불렸는지 알아보기 편하네요.

404 페이지를 사용해주셨는데... 존재하지 않는 url로 요청이 왔을 때도 그 페이지를 활용하면 더 좋겠군요.
(저도 제 리뷰어가 코멘트 남겨주어 생각해보게 되었답니다...!)
제안이니 원하시면 반영하시고 필요하지 않다고 느껴지면 반영하지 않으셔도 됩니다~~~

반영했습니다!

hyxrxn

답변과 수정사항 모두 잘 확인했습니다! 👍
다음 단계도 화이팅 해보아요~~~

Ho-Tea and others added 19 commits September 3, 2024 19:10

test: 학습 테스트 수행

649a603

feat: GET /index.html 응답하기

8dc6457

refactor: CSS 지원하기 전 페이지 응답 uri 수정

54b285c

refactor: RequestLine 추출 메서드 분리

877bdb5

docs: Tomcat 구현하기 1단계 기능 명세 정리

e52df7d

feat: CSS 지원하기

b7b0e30

feat: Query String 파싱

ae46265

refactor: Servlet Container 구성

10738d0

docs: Tomcat 구현하기 2단계 기능 명세 정리

5defcc2

feat: HTTP Status Code 302

bdcbc1e

feat: POST 방식으로 회원가입

265bff3

feat: Cookie에 JSESSIONID 값 저장하기

cac6364

feat: Session 구현하기

d500656

test: HTTP 활용하기 학습 테스트

452b9d1

fix: remove implementation logback-classic on gradle (woowacourse#501)

9606571

(cherry picked from commit fed02f6)

fix: add threads min-spare configuration on properties (woowacourse#502)

7806662

(cherry picked from commit 7e91356)

Merge branch 'ho-tea' into step1

a9d964f

refactor: http 패키지 분리

7131bd5

refactor: 메서드 분리

1e92216

hyxrxn requested changes Sep 8, 2024

View reviewed changes

Ho-Tea added 8 commits September 10, 2024 16:49

refactor: 패키지 분리

f0ee1e3

refactor: 헤더 추출 로직 가독성 향상 도모

7472550

fix: 페이지 요청 시 화면 깨짐 수정

d81f691

refactor: 불필요한 정적 팩토리 메서드 삭제

af465a5

refactor: 정적 페이지 호출 메서드 분리

5d604e2

fix: 이전 쿠키 삭제 후 재발행 로직 추가

f1f6d6c

refactor: 정적 리소스 URL 반환 메서드 분리

3df5686

refactor: 중복 로직 메서드 분리

331ff15

hyxrxn approved these changes Sep 11, 2024

View reviewed changes

hyxrxn merged commit d72a631 into woowacourse:ho-tea Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1 - 2 단계 Tomcat 구현하기] 호티(윤주호) 미션 제출합니다. #563

[1 - 2 단계 Tomcat 구현하기] 호티(윤주호) 미션 제출합니다. #563

Ho-Tea commented Sep 6, 2024 •

edited

Loading

hyxrxn left a comment

hyxrxn Sep 8, 2024 •

edited

Loading

Ho-Tea Sep 10, 2024

hyxrxn Sep 11, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024 •

edited

Loading

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024 •

edited

Loading

hyxrxn Sep 11, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024

hyxrxn Sep 11, 2024

hyxrxn Sep 8, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024 •

edited

Loading

hyxrxn Sep 11, 2024

hyxrxn Sep 11, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024 •

edited

Loading

hyxrxn Sep 11, 2024

hyxrxn Sep 8, 2024

hyxrxn Sep 8, 2024

Ho-Tea Sep 10, 2024

hyxrxn left a comment

		return Files.probeContentType(Paths.get(Objects.requireNonNull(CatalinaServletEngine.class.getClassLoader()
		.getResource("static" + url)).toURI()));

[1 - 2 단계 Tomcat 구현하기] 호티(윤주호) 미션 제출합니다. #563

[1 - 2 단계 Tomcat 구현하기] 호티(윤주호) 미션 제출합니다. #563

Conversation

Ho-Tea commented Sep 6, 2024 • edited Loading

참고

hyxrxn left a comment

Choose a reason for hiding this comment

hyxrxn Sep 8, 2024 • 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

Ho-Tea Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ho-Tea Sep 10, 2024 • 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

Ho-Tea Sep 10, 2024 • 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

Ho-Tea Sep 10, 2024 • 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

hyxrxn left a comment

Choose a reason for hiding this comment

Ho-Tea commented Sep 6, 2024 •

edited

Loading

hyxrxn Sep 8, 2024 •

edited

Loading

Ho-Tea Sep 10, 2024 •

edited

Loading

Ho-Tea Sep 10, 2024 •

edited

Loading

Ho-Tea Sep 10, 2024 •

edited

Loading

Ho-Tea Sep 10, 2024 •

edited

Loading