[톰캣 구현하기 - 1, 2단계] 케로(김지현) 미션 제출합니다. #363

jyeost · 2023-09-04T08:28:08Z

안녕하세여 바론 ㅎㅅㅎ
케로입니더 ㅎㅎ 리뷰를 열심히 해달라고 부탁했지만....
2단계 구현을 전부 다하지는 못한 점 미리 사과드리옵니다....

시간 관계상 2단계의 2번째까지 구현 완료하였으며
테스트 코드도 작성하지 못했습니다.... ヽ(；▽；)ノ

최대한 보기 좋게 작성하려 노력했는데
로직이 복잡하다 보니.... (변명)(눈물)
이해가 안되는 점이 있다면 바아로 디엠 부탁드립니다...

…용하도록 query파라미터와 분리함

somsom13

안녕하세요 케로~!~!!! 바론입니다 🙇‍♀️

전체적인 구조가 너무 깔끔해서 코드 리뷰 하면서 많이 배웠습니다 ㅎ.ㅎ
그래서 걱정하신 것과는 다르게 로직를 이해하기 어렵지 않았어용 ( •̀ .̫ •́ )✧

케로가 말씀해주신 것 처럼 아직 구현이 안 된 부분들을 마저 구현해주시면 좋을 것 같아요!

간단한 코멘트와 질문들만 남겼습니당 ㅎ.ㅎ
고생하셨어요~!~~!! 👏 👍 ❤️‍🔥

somsom13 · 2023-09-04T14:41:17Z

study/src/test/java/study/IOStreamTest.java

@@ -96,6 +99,7 @@ class OutputStream_학습_테스트 {
             * try-with-resources를 사용한다.
             * java 9 이상에서는 변수를 try-with-resources로 처리할 수 있다.
             */
+            outputStream.close();


케로쨩 저도 처음에 이렇게 학습 테스트 작성했었는데 위에 적힌 todo를 보니까 try-with-resource 를 활용하라고 되어있더라구요 (•_•)

케로도 못 보고 지나친 것 같아서 코멘트 남겨봅니당 ㅎ.ㅎ 이건 이번 미션 관련 코멘트가 아니라서 그냥 스킵하셔도 돼요~!

기차나서 이악물고 모른척 했는데(?)
덕분에 수정했습니다 ◡̈♥

somsom13 · 2023-09-04T14:43:04Z

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

+public interface Controller {
+
+    Response handle(Request request);
+}


Controller 인터페이스 짱이네용 👍 👍 깔꼼 그자체

somsom13 · 2023-09-04T14:48:31Z

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

+        if(!requestBody.containsKey(QUERY_ACCOUNT_KEY) || !requestBody.containsKey(QUERY_PASSWORD_KEY)){
+            return new PathResponse("/401", HttpURLConnection.HTTP_UNAUTHORIZED, "Unauthorized");
+        }


로그인에 필요한 입력값이 모두 들어오지 않아서 예외 페이지를 반환하는 상황인 것 같아요!

혹시 401 Unauthorized 상태코드를 선택하신 이유가 있을까요 ?.?

로그인 실패 상황을 어떻게 표현할까 하다가 이렇게 작성했습니다..
지금은 커스텀 예외를 던지도록 변경하였습니다

somsom13 · 2023-09-04T14:57:06Z

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

+
+    @Override
+    public Response handle(Request request) {
+        return new StringResponse("Hello world!", HttpURLConnection.HTTP_OK, "OK");


HttpURLConnection 처음봐서 찾아봤는데 내부에 상수로 HTTP 상태코드값을 가지고 있네요!!! 😮 😮

그런데 아쉽게 "OK" 같은 상태코드에 대한 설명은 따로 관리하지 않는 것 같아서 상태코드 숫자와 의미를 함께 관리하는 다른 방법을 생각해볼 수도 있을 것 같아요! (저는 상태코드값이랑 이름이 아직도 헷갈리더라구요 ㅎㅎ)

이건 제 개인적인 의견이라 케로가 선호하시는 방법으로 3단계 리팩터링 때 고민해보시면 좋을 것 같습니당~!

구현할때는 시간이 없어서 이렇게 막구현 했습니다 ㅠㅠ
리팩터링 하면서 Enum 클래스로 따로 관리하니 확실히 보기 깔끔하더라구영!!
감사합니다 바론 ㅎㅅㅎ

somsom13 · 2023-09-04T15:03:25Z

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

+            return new PathResponse("/index", HttpURLConnection.HTTP_MOVED_TEMP, "Temporary Redirect");
+        }
+
+        return new PathResponse("/401", HttpURLConnection.HTTP_UNAUTHORIZED, "Unauthorized");


이 Response는 401.html 정적 파일을 읽어와서 반환하는 것 같은데
위에서 GET /login 에 대해서는 StaticResponse를 사용하고 여기에서는 PathResponse를 사용하신 이유가 따로 있는지 궁금해요!!

pathResponse를 만드는게 더 쉬웠나... 그래서 그랬던 것 같아요...
다만 코드 변동이 많아서 지금은 없는 객체입니다 ㅜㅜ

현재는 ResponseEntity 객체를 하나두고 여러가지 팩토리 메서드로 분리하여 관리하도록 하였습니다 !!

somsom13 · 2023-09-04T15:40:11Z

tomcat/src/main/java/org/apache/coyote/request/Request.java

+        RequestMessage header = RequestMessage.from(bufferedReader);
+
+        return getRequest(method, pathFromStartLine, header.getRequestBody());


header라는 이름의 RequestMessage에서 body가 추출되는게 사알짝 헷갈리게 느껴지는 것 같아요 ╯︿╰

header와 body가 명확하게 이해될 수 있도록 할 수 있는 다른 방법은 없을지 고민해보면 좋을 것 같아요~!!

reader라는 객체를 만들어서 startLine headers body를 각각 읽어올 수 있도록 수정했습니다 !

somsom13 · 2023-09-04T15:42:07Z

tomcat/src/main/java/org/apache/coyote/response/PathResponse.java

+public class PathResponse implements Response{
+    private static final String FILE_TYPE_FIXTURE = "html";
+    private static final String FILE_EXTENSION_DELIMITER = ".";
+
+
+    private final String path;


상수 위의 개행이 아래로 이사를 가버린 것 같습니다 （＞人＜；）

짜잔~!! 클래스를 업애버렸습니다 ㅎㅅㅎ

somsom13 · 2023-09-04T15:44:00Z

tomcat/src/main/java/org/apache/coyote/response/StaticResponse.java

+    private final int statusCode;
+    private final String statusValue;
+
+    public StaticResponse(final String fileType, final String path, int statusCode, String statusValue) {


파라미터에 final 키워드를 통일해서 붙이면 좋을 것 같아요!.! 😊

다른 클래스 내의 변수, 파라미터도 final 키워드를 통일하면 좋을 것 같습니당

이제서야 인텔리제이 설정을 추가하였습니다... !!!
감사합니다 ㅜㅜ 흑흑

somsom13 · 2023-09-04T15:47:18Z

tomcat/src/main/java/org/apache/front/DynamicController.java

+    static {
+        urlMapper.put("/", new HelloWorldController());
+        urlMapper.put("/login", new LoginController());
+        urlMapper.put("/register", new RegisterController());
+    }
+


정말 어마어마하네요 케로쨩... o(￣▽￣)ｄ
맛있게 배워갑니다

굳굳 따봉따봉>ㅡ<

somsom13 · 2023-09-04T15:52:20Z

tomcat/src/main/java/org/apache/front/Proxy.java

+        if(request.isFile()){
+            return doProcess(staticController, request);
+        }
+        return doProcess(dynamicController, request);


정적 파일 요청과 동적 요청을 분리하는 부분이군요!!!!
이렇게 두 종류의 요청을 처리하는 controller를 분리하면 어떤 장점이 있나용?.?

그나저나 정말 어마어마합니다 케로쨩...

스프링에서 톰캣이 서블렛을 띄워줄때 정적 파일 요청이라면 어뎁터 핸들러를 매핑하지 않는다고 알고 있어서 비슷하게 처리 하고 싶어서 고민하다가 나온 결과입니다... ヽ(；▽；)ノ
이렇게 작성해주니 예외처리를 한 곳에서 하기 편하던데용?!?!

오홍 그렇군요~~!!!! 확실이 굳이 정적 파일을 위해 매핑할 필요가 없어질 것 같아요 👍 따봉 드립니다

…리하였다.

…지정하도록 구현

…, 해당 세션 매니저에 Jsessionid에 일치하는 회원정보가 있다면 index로 redirect하도록 구현

sonarcloud · 2023-09-10T08:37:45Z

Kudos, SonarCloud Quality Gate passed!

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

somsom13

케로..... 짧은 시간 내에 어마어마한 수정과 기능 구현을 해오셨네요 😲

원래도 좋았던 구조가 더 좋아진 것 같아서 따봉들 잔뜩과 아아주 간단한 코멘트 하나를 남겨두었습니다 ㅎ.ㅎ

너무 고생하셨어요~!~!! 👍 ❤️‍🔥

somsom13 · 2023-09-10T12:47:29Z

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

+        if (request.isPost()) {
+            return login(request);
+        }
+        if (request.isGet() && request.hasQueryString()) {
+            return loginInConsole(request);
+        }


미션에 주어진 두 가지 로그인 방법이 다 되는군여 따따봉 👍 👍

somsom13 · 2023-09-10T12:48:56Z

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

+        final User user = InMemoryUserRepository.findByAccount(account)
+                .orElseThrow(MemberNotFoundException::new);
+        if (user.checkPassword(password)) {
+            log.info("user : {}", user);
+        }
+        return ResponseEntity.fromViewPath(request.httpVersion(), request.getPath(), ResponseStatus.OK);


요롷게 되면 account가 없는 회원에 대해서는 처리가 잘 되지만, account는 맞는데 비밀번호가 틀린 회원도 로그인이 되었다고 처리되어 버릴 것 같아요! 😿

앗 밑의 login 메서드는 이 상황도 처리가 되어있네용 ㅎ.ㅎ 확인해씁니다~~~

사실 쿼리 파라미터로 로그인 하는 경우는 없을 것 같아서 수정하지 않았습니다 ㅎㅎ

somsom13 · 2023-09-10T12:51:27Z

tomcat/src/main/java/org/apache/coyote/common/FileType.java

+    private static final Map<String, FileType> map = new HashMap<>();
+
+    static {
+        for (FileType fileType : FileType.values()) {
+            map.put(fileType.extension, fileType);
+        }
+    }


댑악 👍 😮 😲

somsom13 · 2023-09-10T12:54:00Z

tomcat/src/main/java/org/apache/coyote/common/PathUrl.java

이렇게 따로 관리하니까 엄청 깔끔하네용

클래스 분리 좋와용 ㅎㅅㅎ

somsom13 · 2023-09-10T12:56:32Z

tomcat/src/main/java/org/apache/coyote/request/Cookie.java

+        for (final String oneCookie : requestCookie.split("; ")) {
+            if (!oneCookie.contains("=")) {
+                continue;


요런 문자열들 나중에 상수로 추출되면 더 이해하기 좋을 것 같아요~!~!!
(￣︶￣*))

근데 상수로 추출할때 어떤 필드명을 써야 이해하기 쉬울까요...
전 사실 이 쪽이 보기 더 편하답니다... ( ﾉω-、)

somsom13 · 2023-09-10T13:01:05Z

tomcat/src/main/java/org/apache/coyote/request/RequestReader.java

+        final ArrayList<String> lines = new ArrayList<>();
+        while (true) {
+            final String line = reader.readLine();
+            if ("".equals(line)) {
+                break;
+            }
+            lines.add(line);
+        }
+        return lines;


BufferedReader에 담긴 모든 라인들을 다 읽어오는 거라면 lines() 메서드를 활용해보는 것은 어떨까용~??~?? (그런 상황이 아니라면 지금 흐름이 좋은 것 같습니다 ㅎ.ㅎ)

마지막 requestBody라인을 제외하고 모두 읽어오는 메서드 입니다 ..!
근데 body도 한꺼번에 읽어와서 처리할 수 있나 해봐야겠어용 ㅎㅅㅎ

somsom13 · 2023-09-10T13:02:43Z

tomcat/src/main/java/org/apache/coyote/response/header/ContentLength.java

Content-Length 와 Content-Type 이 따로 객체 분리가 된 이유가 있을까요 ?.?

각각 header에서 한 라인을 담당하고 있기 때문에 분리해주는 것이 header를 만들기 편할 것 같다고 생각했습니다

somsom13 · 2023-09-10T13:03:10Z

tomcat/src/main/java/org/apache/coyote/response/header/ResponseHeader.java

+        return headers.stream()
+                .collect(Collectors.joining(" " + System.lineSeparator()));
+    }


somsom13 · 2023-09-10T13:08:02Z

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

@@ -23,7 +23,7 @@ void process() {
        processor.process(socket);

        // then
-        var expected = String.join("\r\n",
+        var expected = String.join(System.lineSeparator(),


HTTP는 라인 구분자로 \r\n 을 쓴다고 알고있어요! 이 상황에서 System.lineSeparator() 를 쓰게된다면 운영체제에 따라 구분자가 달라질 수도 있지 않을까요?.?

((근데 지금보니까 제가 지난번 리뷰에서는 System.lineSeparator() 짱이라고 리뷰 남겼었네요.. 와리가리해서 죄송합니다 케로쨩... 😓 😓 ))

헉.... 아뇨
저갓은 핑프에게 좋은 정보를 알려주셔서 감사합니다...
찾아보니까 맞네용...ㅜㅜ
바로 수정하겠습니다.

jyeost added 7 commits September 2, 2023 21:54

test: study test 작성

54ab392

feat: 1단계 구현

15841e5

feat: 응답에 응답 코드와 이름을 유동적으로 보이게 수정함, 로그인 성공시 index로 리다이렉트 하도록 기능 추가

e64a4be

feat: post 메서드를 이용해 회원가입과 로그인 기능 추가

eb29264

refactor: request 헤더 정보와 바디 정보를 조회하는 클래스를 분리함. post사용시 requestbody를 사…

c6811d3

…용하도록 query파라미터와 분리함

refactor: response 띄어쓰기 때문에 테스트 실패하는 것 수정

9164868

refactor: BufferedReader 자원 반환 안되어서 빌드 실패 오류 나던것 수정

eacc340

somsom13 requested changes Sep 5, 2023

View reviewed changes

jyeost added 16 commits September 5, 2023 17:08

refactor: 테스트 코드 자원 반납 수정

0fa5274

study: cache study 미션 코드 작성

0f06bee

feat: request의 startLine을 각 클래스로 분리하였다.

5f94dd4

feat: request의 header와 requestBody를 각 클래스로 분리하여 관리하도록 하였다.

6fd3f05

feat: static 파일 응답에 대한 부분 전부 수정

6267087

feat: text를 return하는 응답 기능 수정

16a4c45

feat: login post 메서드 요청시 requestBody 정보로 로그인을 시도하고 성공시 redirect하는 기능 추가

01129fb

feat: login get요청시 view 만 반환하는 기능 추가

dd69461

feat: register의 post와 get요청 처리 기능 추가

05113e8

refactor: 코드 정리

c53a97c

feat: 특정 예외 상황에 예외 view를 보여주기 위해 예외를 던지고 해당 예외에서 view의 정보를 받을 수 있도록 처…

1f82212

…리하였다.

feat: 테스트 케이스를 추가하고 request를 생성할때 Reader객체를 입력받아 생성되게 하여 확장성 잇도록 만듦

768114a

refactor: StringBuilder를 사용하던 것을 스트림으로 해결하도록 수정

1b8117c

feat: cookie 클래스를 만들고 로그인 성공시 요청 헤더에 JssesionId가 없다면 응답에 Set-cookie를 …

4b274d1

…지정하도록 구현

feat: 로그인 성공시 응답에 set-cookie를 설정하고, 다시 로그인 페이지 요청시 쿠키에 Jsessionid가 있고…

529170a

…, 해당 세션 매니저에 Jsessionid에 일치하는 회원정보가 있다면 index로 redirect하도록 구현

teat: 테스트 깨지는 것 수정

c1f7020

somsom13 approved these changes Sep 10, 2023

View reviewed changes

somsom13 merged commit 1064589 into woowacourse:jyeost Sep 10, 2023
2 checks passed

		RequestMessage header = RequestMessage.from(bufferedReader);

		return getRequest(method, pathFromStartLine, header.getRequestBody());

[톰캣 구현하기 - 1, 2단계] 케로(김지현) 미션 제출합니다. #363

[톰캣 구현하기 - 1, 2단계] 케로(김지현) 미션 제출합니다. #363

Conversation

jyeost commented Sep 4, 2023

somsom13 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

jyeost Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 10, 2023

somsom13 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

somsom13 Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyeost Sep 5, 2023 •

edited

Loading

somsom13 Sep 10, 2023 •

edited

Loading