[톰캣 구현하기 3, 4단계] 주디(오은비) 미션 제출합니다. #497

eunbii0213 · 2023-09-11T09:00:52Z

안녕하세요 헤나 ㅎㅎ 주디입니다!
3,4단계 구현하여 PR 보냅니다~ 이번에도 잘 부탁드려요!

hyena0608 · 2023-09-11T10:34:03Z

주디 안녕하세요~
3, 4단계 고생하셨네요!

다름이 아니라 현재 PR이 잘못된거 같아서 커멘트 먼저 남겼습니다~

1, 2단계 pull하지 않고 진행하셔서 Conflict 났네요!
3, 4단계 커밋이 1개인데 잘못 날리신건지 모르겠네요 😅

확인 부탁드립니다~
(+ LMS에서 리뷰 요청 버튼 잊으시지 않았나요? 버튼 눌러주시면 더 빨리 확인 할 수 있을거 같아요~~~~~~)

hyena0608

주디 Conflict 해결 고생하셨어요~

리뷰 요청해주셔서 바로 확인해봤어요.
PR 받아서 애플리케이션 실행해봤는데 요구사항에 맞지 않는 오류가 있어요..! 🥲
확인 후 다시 리뷰 요청 부탁드리겠습니다.

01. 로그인 후 쿠키에 세션이 저장이 안돼요.

기존에 있는 gugu로 로그인 시도해봤습니다.

쿠키에 세션 정보가 없네요 ㅠㅠ

02. 회원가입 세션이 없어요.

hyena 회원가입을 진행해봤습니다.

여기도 안되네요.. 🙁

저장되나 직접 로깅해봤는데 일단 InMemory에 잘 들어가고 있네요!

03. locahost:8080/index를 찾을 수 없습니다!

04. 학습 테스트가 모두 사라졌어요! 😮

요청해주신대로 커멘트 후 머지하려고 했지만 미션 요구사항에 맞게 수정해주셔야 할 거 같아요 👍
요구사항에 맞을 때 다시 리뷰 하겠습니다!! 화이팅입니다 주디~~🚀

sonarcloud · 2023-09-13T04:59:06Z

Kudos, SonarCloud Quality Gate passed!

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

hyena0608

안녕하세요 주디~
반영 고생하셨어요!
1, 2단계에서 많이 리팩터링 해주셨네요 👍

주디가 요청해주신대로 머지 먼저 하겠습니다 🚀
그래도 시간이 되신다면 제가 남긴 커맨트를 읽고 의견도 남겨주시면 재밌을거 같네요~
학습 테스트 관해서도 이야기 나눴으면 좋을텐데 ㅜㅜ
이것도 시간되신다면 학습 테스트 진행하시고 같이 더 많이 얻어갈 수 있으면 좋겠네요!

화이팅입니다 주디~

hyena0608 · 2023-09-13T09:12:22Z

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

+import java.io.IOException;
+
+public interface Controller {
+    String process(HttpRequestParser httpRequestParser, HttpResponseBuilder httpResponseBuilder) throws IOException;


요구사항에 맞지 않아서 조금 아쉽네요 ㅜㅜ
LMS에 있는 방식으로도 구현해보시면 좋을거 같아요!

hyena0608 · 2023-09-13T09:18:29Z

tomcat/src/main/java/nextstep/jwp/presentation/GetRegisterController.java

+    @Override
+    public String process(HttpRequestParser httpRequestParser, HttpResponseBuilder httpResponseBuilder) throws IOException {
+        return httpResponseBuilder.buildStaticFileOkResponse(httpRequestParser,
+                httpRequestParser.findPathWithoutQueryString() + ".html");
+    }


쿼리 파라미터가 없다고해도 만약 index/1 이었다면 생각과 다른 결과가 나올거 같아요!
이후에 이러한 상황이 발생할 수도 있다는 것을 기억하고 있지 않다면 예외가 발생할 위험성이 있다고 생각합니다

hyena0608 · 2023-09-13T09:20:57Z

tomcat/src/main/java/nextstep/jwp/presentation/PostLoginController.java

+        String account = splitRequestBody[0].split(KEY_VALUE_SEPARATOR)[1];
+        String password = splitRequestBody[1].split(KEY_VALUE_SEPARATOR)[1];


배열에 대한 생각을 두 번해야할거 같아요 ㅜㅜ
파싱에 대한 역할을 더 쪼개면 이후에 사용하기도 읽기도 편해질 수 있겠네요!

hyena0608 · 2023-09-13T09:23:59Z

tomcat/src/main/java/nextstep/jwp/presentation/PostLoginController.java

+        try {
+            User user = InMemoryUserRepository.findByAccount(account).orElseThrow(UserNotFoundException::new);
+            addSession(user, httpRequestParser);
+            return redirect(password, user, httpRequestParser, httpResponseBuilder);
+        } catch (UserNotFoundException e) {
+            return httpResponseBuilder.buildStaticFileRedirectResponse(httpRequestParser, "/401.html");
+        }


로그인 실패를 try catch로 잡아주셨네요
저는 큰 오류가 있는 줄 알았어요 😂
로그인 실패 처리를 이런 방식으로 구현하신 이유가 궁금합니다

hyena0608 · 2023-09-13T09:27:00Z

tomcat/src/main/java/nextstep/jwp/presentation/PostLoginController.java

+    private void addCookie(HttpRequestParser httpRequestParser, Map<String, String> cookies, String uuid) {
+        if (cookies.isEmpty()) {
+            httpRequestParser.addHeader("Cookie", SESSION_ID + "=" + uuid);
+            return;
+        }
+        //cookies에 잇는 cookie들로 Cookie header 만들어줘
+        String existedCookie = joinExistedCookie(cookies);
+        httpRequestParser.addHeader("Cookie", existedCookie + "; " + SESSION_ID + "=" + uuid);
+    }


HttpRequest 객체를 컨트롤러에서 제가 내부 값을 컨트롤할 수 있네요.
클라이언트가 보낸 정보를 조작할 수 있는게 위험할 수 있지 않을까요?

hyena0608 · 2023-09-13T09:35:09Z

tomcat/src/main/java/nextstep/jwp/presentation/handler/FrontController.java

+    private final Map<String, Controller> getMappingControllers = new HashMap<>();
+    private final Map<String, Controller> postMappingControllers = new HashMap<>();
+
+    public FrontController() {
+        getMappingControllers.put("/", new RootController());
+        getMappingControllers.put("/login", new GetLoginController());
+        getMappingControllers.put("/register", new GetRegisterController());
+        postMappingControllers.put("/login", new PostLoginController());
+        postMappingControllers.put("/register", new PostRegisterController());
+    }
+
+    public String process(HttpRequestParser httpRequestParser, HttpResponseBuilder httpResponseBuilder) throws IOException {
+        String method = httpRequestParser.findMethod();
+        String path = httpRequestParser.findPathWithoutQueryString();
+        Controller controller = findController(method, path);
+
+        return controller.process(httpRequestParser, httpResponseBuilder);
+    }
+
+    public Controller findController(String method, String path) {
+        if (isStaticPath(path)) {
+            return new StaticController();
+        }
+        if (method.equals("GET") && getMappingControllers.containsKey(path)) {
+            return getMappingControllers.get(path);
+        }
+        if (method.equals("POST") && postMappingControllers.containsKey(path)) {
+            return postMappingControllers.get(path);
+        }
+        return new NotFoundController();
+    }


Http 메서드에 따라 사용하는 map이 다르네요!
그래서 분기문도 나오게 된거야 같아요.
현재는 분기문이 적지만 이후에 어떻게 해결하실지 궁금하네요.

hyena0608 · 2023-09-13T09:36:51Z

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

+    public Connector(final int port, final int acceptCount, final int maxThreads) {
        this.serverSocket = createServerSocket(port, acceptCount);
+        this.threadPoolExecutor = new ThreadPoolExecutor(
+                CORE_POOL_SIZE,
+                maxThreads,
+                KEEP_ALIVE_TIME,
+                TimeUnit.SECONDS,
+                new LinkedBlockingQueue<>(DEFAULT_ACCEPT_COUNT));
        this.stopped = false;
    }


흠.. Executors를 이용해보라고 LMS에 있었던거 같은데 ㅜㅜ
ThreadPoolExecutor를 바로 이용하신 이유가 있나요?

hyena0608 · 2023-09-13T09:53:39Z

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

+    public Map<String, String> findCookies() {
+        Map<String, String> cookie = header.entrySet().stream()
+                .filter(entry -> entry.getKey().equals("Cookie"))
+                .map(entry -> entry.getValue().split("; "))
+                .flatMap(Arrays::stream)
+                .map(line -> line.split(KEY_VALUE_DELIMITER))
+                .collect(Collectors.toMap(line -> line[0], line -> line[1]));
+        return cookie;
+    }


중간에 반환된 값을 변수로 받은 후 진행하면 더 읽기 편할거 같아요.

hyena0608 · 2023-09-13T09:55:00Z

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

+        return protocol + SPACE + status + SPACE + LINE_FEED +
+                findCookie(httpRequestParser) +
+                contentType + SPACE + LINE_FEED +
+                contentLength + SPACE + LINE_FEED +
+                LINE_FEED +
+                content;


문자열 연산이 많은데 다른 방식으로 구현해주셔도 좋을거 같아요

hyena0608 · 2023-09-13T09:58:08Z

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

+
+public enum HttpStatus {
+    OK("200", "OK"),
+    REDIRECT("302", "Moved Temporarily"),


302가 Moved Temporarily가 맞나요??
저는 다른거로 알고 있는데 혹시 보신 문서가 있으시다면 공유해주시면 감사하겠습니다!!

eunbii0213 requested a review from hyena0608 September 11, 2023 09:00

eunbii0213 self-assigned this Sep 11, 2023

eunbii0213 closed this Sep 11, 2023

eunbii0213 reopened this Sep 11, 2023

eunbii0213 force-pushed the step3 branch from c54a99d to d00a615 Compare September 12, 2023 02:27

eunbii0213 added 5 commits September 12, 2023 12:00

fix: conflict 해결

51a74e7

fix: conflict 해결

78568a1

test: 테스트 코드 수정

d6143f3

test: 테스트 작성

54d7956

fix: conflict 해결

e878221

eunbii0213 force-pushed the step3 branch from d00a615 to e878221 Compare September 12, 2023 03:09

hyena0608 requested changes Sep 12, 2023

View reviewed changes

refactor: 피드백 반영

570e2ab

hyena0608 approved these changes Sep 13, 2023

View reviewed changes

hyena0608 merged commit 90a7fab into woowacourse:eunbii0213 Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 3, 4단계] 주디(오은비) 미션 제출합니다. #497

[톰캣 구현하기 3, 4단계] 주디(오은비) 미션 제출합니다. #497

eunbii0213 commented Sep 11, 2023

hyena0608 commented Sep 11, 2023 •

edited

Loading

hyena0608 left a comment

sonarcloud bot commented Sep 13, 2023

hyena0608 left a comment

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

hyena0608 Sep 13, 2023

		String account = splitRequestBody[0].split(KEY_VALUE_SEPARATOR)[1];
		String password = splitRequestBody[1].split(KEY_VALUE_SEPARATOR)[1];

[톰캣 구현하기 3, 4단계] 주디(오은비) 미션 제출합니다. #497

[톰캣 구현하기 3, 4단계] 주디(오은비) 미션 제출합니다. #497

Conversation

eunbii0213 commented Sep 11, 2023

hyena0608 commented Sep 11, 2023 • edited Loading

hyena0608 left a comment

Choose a reason for hiding this comment

01. 로그인 후 쿠키에 세션이 저장이 안돼요.

02. 회원가입 세션이 없어요.

03. locahost:8080/index를 찾을 수 없습니다!

04. 학습 테스트가 모두 사라졌어요! 😮

sonarcloud bot commented Sep 13, 2023

hyena0608 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

hyena0608 commented Sep 11, 2023 •

edited

Loading