[1 - 2 단계 Tomcat 구현하기] 테드(김규태) 미션 제출합니다. #572

Kimprodp · 2024-09-06T08:48:12Z

안녕하세요 타칸! 오랜만입니다.😊

미션 1-2 단계 구현 완료해서 리뷰 요청드립니다.

두 번째 학습(cache)은 시간이 부족하여 진행하지 못했습니다.

구현하면서 여러 책임이 합쳐지게 되어 나름대로 클래스를 분리하며 진행해봤습니다.
그래도 아직 개선할 부분이 많이 보이긴 하는데요. 편하게 리뷰 부탁드립니다!

jhon3242

안녕하세요 테드! 😁 오랜만이네요! 잘 지내고 있죠? ㅎㅎ
대부분 스프링의 구조를 따라서 확장가능하고 가독성 있게 코드를 너무 잘 작성하셨네요! 많이 배워갑니다! 😊
2단계까지 빠르게 구현하셨네요! 궁금한 부분은 질문), 수정이 필요하다고 생각하는 부분은 의견) 키워드로 남겼어요! 확인 부탁드릴게요!!

jhon3242 · 2024-09-06T10:11:37Z

study/src/main/resources/application.yml

@@ -6,4 +6,5 @@ server:
    accept-count: 1
    max-connections: 1
    threads:
+      min-spare: 2


min-spare 를 설정한 이유와 2인 근거가 궁금해요!

이건 메인 브랜치 변경사항 받아오면서 생긴 부분인 것 같아요!

jhon3242 · 2024-09-06T10:25:54Z

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

+    public HttpRequest(BufferedReader reader) throws IOException {
+        this.requestLine = extractRequestLine(reader);
+        this.headers = extractHeaders(reader);
+        this.requestBody = extractRequestBody(reader);
+
+        log.info("request header: {}", headers);
+    }


질문) 체크 예외까지 해주면서 BufferedReader 를 파라미터로 받은 이유가 궁금해요! 외부에서 최대한 처리하고 모든 값을 읽은 다음에 파라미터로 넘겨주는 방식 대신 이러한 방식을 선택하신 이유가 무엇인가요?😊

우선은 Http11Processor 에서 분리를 위한 목적으로만 했었는데요, HttpRequest에서 값을 분리하고, 예외 처리하는 책임이 없는 것이 맞을 것 같아 HttpRequestParser를 둬서 요청을 분리하는 책임을 나누는 것으로 변경했습니다.

jhon3242 · 2024-09-06T10:35:06Z

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

+    public String extractRequestBody(BufferedReader reader) throws IOException {
+        try {
+            int contentLength = Integer.parseInt(headers.get("Content-Length"));
+            char[] buffer = new char[contentLength];
+            reader.read(buffer, 0, contentLength);
+            return new String(buffer);
+
+        } catch (NumberFormatException e) {
+            return null;
+        }
+    }


의견) containsKey 를 활용하는 방법도 있겠네요! 물론 "Content-Length" 값의 대한 검증은 필요하겠지만요 😊

이 부분은 어차피 body가 없으면 null을 넣기 위함이라 불필요한 조건이 추가되는 것 같아 NumberFormatException만 체크하면 될 것 같습니다!

jhon3242 · 2024-09-06T10:38:49Z

tomcat/src/main/java/org/apache/catalina/handler/RequestMapping.java

+public class RequestMapping {
+
+    private final Map<String, Controller> controllerMapping = new HashMap<>();
+
+    public RequestMapping() {
+        controllerMapping.put("/login", new LoginController());
+        controllerMapping.put("/register", new RegisterController());
+    }
+
+    public Controller getController(HttpRequest request) {
+        String path = request.getRequestLine().getPath();
+        Controller controller = controllerMapping.get(path);
+        if (controller == null) {
+            return new ViewController();
+        }
+
+        return controller;
+    }
+}


의견) 해당 객체는 싱글톤으로 구현해도 좋을 것 같아요!

jhon3242 · 2024-09-06T10:42:20Z

tomcat/src/main/java/org/apache/catalina/controller/ViewController.java

+    private void responseDefaultPage(HttpResponse response) {
+        response.setStatus200();
+        response.setContentTypeHtml();
+        response.setResponseBody("Hello world!");
+    }


의견) "1-2 CSS 지원하기" 요구사항의 gif 와 같이 / 요청에 대해 "대시보드" 페이지가 나오도록 바꿔주는건 어떨까요?😊

이렇게 하면 Http11ProcessorTest가 통과하지 못해서 그냥 두었었어요..

저는 그래서 그냥 테스트를 지웠습니다 ㅎㅎ 테드 편하신대로 하면 좋을 것 같아요!

jhon3242 · 2024-09-06T11:08:23Z

tomcat/src/main/java/org/apache/catalina/controller/LoginController.java

+import org.apache.coyote.http11.request.HttpRequest;
+import org.apache.coyote.http11.response.HttpResponse;
+
+public class LoginController extends AbstractController {


질문) 로그인 컨트롤러가 아파치 패키지 내부에 위치시킨 이유가 궁금해요!

비즈니스 로직을 처리하는 클래스가 아파치에 있는 것이 이상하네요.. techcourse 패키지로 구조를 변경하였습니다!

jhon3242 · 2024-09-06T11:12:04Z

tomcat/src/main/java/org/apache/catalina/controller/RegisterController.java

+        if (requestBody == null) {
+            throw new IllegalArgumentException("RequestBody is missing in the request");
+        }


의견) IllegalArgumentException 가 발생하면 해당 예외를 처리해주는 catch 구문이 없어서 확인하기 어려울 것 같아요!

예외가 발생할 경우 400을 응답하도록 catch 구문을 추가했습니다~

jhon3242 · 2024-09-06T11:13:53Z

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

+//    public HttpCookie(String cookie) {
+//        setCookies(cookie);
+//    }
+//
+//    private void setCookies(String cookie) {
+//        for (String cookiesPart : cookie.split(" ")) {
+//            String[] keyValue = cookiesPart.split("=");
+//            cookies.put(keyValue[0], keyValue[1]);
+//        }
+//    }


의견) 지워주려다 까먹은 것 같네요😆

jhon3242 · 2024-09-06T11:16:33Z

tomcat/src/main/java/org/apache/catalina/controller/LoginController.java

+        String cookie = request.getHeader("Cookie");
+        Map<String, String> cookies = new HashMap<>();
+        for (String cookieParts : cookie.split(" ")) {


의견) 만약 "Cookie" 헤더가 없다면 NPE가 발생하지 않을까요??🧐

꼼꼼하네요 👍 쿠키가 없으면 null을 반환하도록(로그인 페이지를 응답할 수 있게) 변경했습니다~

jhon3242 · 2024-09-06T11:19:55Z

tomcat/src/main/java/org/apache/catalina/controller/LoginController.java

+        }
+
+        String jsessionId = cookies.get("JSESSIONID");
+        SessionManager sessionManager = new SessionManager();


질문) SessionManager 의 메서드들을 정적 메서드로 만들어서 호출하는 방식 대신 매번 SessionManager 객체를 생성해서 사용하는 이유가 궁금해요!😄

세션 데이터가 하나로 관리되고, 매번 인스턴스를 생성할 필요가 없으므로 static 메서드로 활용하도록 변경했습니다!

jhon3242

안녕하세요 테드! 타칸입니다!
전반적으로 많이 리팩터링 되었네요!
가독성 있게 코드를 잘 작성하시고 구조가 실제 스프링과 유사한 구조로 구현하신 것 같아 많이 배웠습니다!
몇가지 검증에 대한 커멘트 남겨두었는데 다음 PR때 반영해주시면 될 것 같아서 이만 merge 하겠습니다!
추가로 다음 PR에는 테스트 코드도 같이 추가해주시면 좋을 것 같습니다! 수고하셨습니다!😊

jhon3242 · 2024-09-10T02:27:11Z

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

+            String[] requestLineParts = firstLine.split(" ");
+            method = requestLineParts[0];
+            String uri = requestLineParts[1];
+            protocol = requestLineParts[2];


의견) 배열에 인덱스로 접근하는 로직은 NPE가 발생하지 않도록 검증하는 로직이 필요할 것 같아요! requestLineParts 의 length를 검증하는 로직을 추가해주면 어떨까요??

jhon3242 · 2024-09-10T02:27:29Z

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

+            String[] headerLineParts = readLine.split(": ");
+            headers.put(headerLineParts[0], headerLineParts[1]);


의견) 이 부분도 마찬가지로 배열의 크기에 대한 검증이 필요할 것 같아요!

jhon3242 · 2024-09-10T05:02:49Z

tomcat/src/main/java/com/techcourse/handler/RequestMapping.java

+        if (controller == null) {
+            return new ViewController();
+        }
+


제안) ViewController도 싱글톤으로 구현하는게 좋을 것 같아요!😊

jhon3242 · 2024-09-10T05:09:57Z

tomcat/src/main/java/com/techcourse/session/SessionManager.java

+
+public class SessionManager {
+
+    private static final Map<String, Session> SESSIONS = new HashMap<>();


의견) 지금은 동시성을 처리할 필요는 없지만 추후 동시성을 고려한다면 ConcurrentHashMap으로 바꿔주면 좋을 것 같네요!

jhon3242 · 2024-09-10T05:13:48Z

tomcat/src/main/java/com/techcourse/controller/LoginController.java

+    private Session extractSession(HttpRequest request) {
+        String cookie = request.getCookie();
+        if (cookie == null) {
+            Map<String, String> cookies = new HashMap<>();
+            for (String cookieParts : cookie.split(" ")) {
+                String[] keyAndValue = cookieParts.split("=");
+                cookies.put(keyAndValue[0], keyAndValue[1]);
+            }
+
+            String jsessionId = cookies.get("JSESSIONID");
+            return SessionManager.findSession(jsessionId);
+        }
+
+        return null;
+    }


의견) 해당 메서드는 LoginController 보다는 SesseionManager의 책임이라고 느껴지는 것 같은데 테드의 생각이 궁금해요!🧐

쿠키에서 JSessionId를 추출하는 책임을 쿠키가 가지고 있는 것이 맞는 것 같아서 조정했습니다

jhon3242 · 2024-09-10T05:19:06Z

tomcat/src/main/java/com/techcourse/controller/RegisterController.java

+            Map<String, String> requestFormData = request.getFormData();
+            String account = requestFormData.get("account");
+            String password = requestFormData.get("password");
+            String email = requestFormData.get("email");
+
+            User user = new User(account, password, email);


의견) reqeustForm 에 account 가 없거나 email 이 없거나 등에 대한 검증은 추가해주면 좋을 것 같아요!

반영 완료 👍

jhon3242 · 2024-09-10T05:19:43Z

tomcat/src/main/java/com/techcourse/controller/RegisterController.java

+            response.setStatus400();
+            response.setResponseBody(e.getMessage());
+            log.info("Bad Request: {}", e.getMessage());
+


불필요한 개행이네요😁

3단계 하면서 공백도 다시 확인했습니다 👍

jhon3242 · 2024-09-10T05:23:20Z

tomcat/src/main/java/com/techcourse/session/Session.java

+    private final String id;
+    private final Map<String, Object> values = new HashMap<>();


👍👍 배워갑니다!

geoje and others added 20 commits September 5, 2024 11:11

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

fed02f6

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

7e91356

test: 스터디 테스트 작성

ba5c36d

refactor: 자동 정렬 이전 파일로 교체

1079350

refactor: import 변경

8309210

refactor: 요청에 따라 자원을 반환하도록 변경

88ff134

feat: HttpRequest 추가

a3a5a38

feat: HttpResponse 추가

87faf32

refactor: Http11Processor에서 요청과 응답 분리

ed82d8a

refactor: 로그인 메서드 post로 변경

e80274a

refactor: HttpRequest에 requestBody 추가

055439f

refactor: HttpResponse에 상태코드 추가

20b3b92

feat: 요청을 처리하는 컨트롤러 구현

236a0f6

feat: 매핑된 컨트롤러를 찾아서 반환하는 RequestMapping 추가

06e8288

feat: ViewResolver 및 View 추가

5f3c7f8

refactor: Http11Processor에서 Controller를 사용하도록 변경

b9d83f4

refactor: HttpRequest 에서 key 값으로 찾도록 변경

280caf5

refactor: HttpResponse 에 쿠키를 포함하는 기능 추가

2cb85c0

feat: 쿠키 및 세션 추가

d9bdda1

refactor: 로그인 시, 세션 확인하는 기능 추가

c509f08

Kimprodp requested a review from jhon3242 September 6, 2024 08:48

jhon3242 requested changes Sep 6, 2024

View reviewed changes

Kimprodp added 8 commits September 9, 2024 20:33

refactor: ReqeustParser를 통해 HttpRequest 를 만들도록 변경

455ea74

refactor: HttpRequest 관련 클래스 분리

e094dda

refactor: RequestMapping 싱글톤으로 변경

ce0ca5d

refactor: RegisterController에서 request의 기능을 활용하도록 변경

ace0a51

refactor: 로깅 추가 및 수정

de3b27b

refactor: Accpet로 response의 ContentType 구성하도록 변경

ea605f6

refactor: 정의되지 않은 요청 시, 404 페이지 반환하도록 변경

6209a7a

refactor: 사용하지 않는 주석 제거

6ad5478

Kimprodp added 4 commits September 9, 2024 22:43

refactor: 패키지 구조 변경

9a67e20

refactor: post 요청에서 body가 존재하지 않을 경우 400을 반환하도록 변경

4c7fa83

refactor: cookie에 대한 NPE 처리

2fe0e5a

refactor: SessionManager 를 static 으로 활용하도록 변경

c45a246

jhon3242 approved these changes Sep 10, 2024

View reviewed changes

jhon3242 merged commit 5c73e56 into woowacourse:kimprodp Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1 - 2 단계 Tomcat 구현하기] 테드(김규태) 미션 제출합니다. #572

[1 - 2 단계 Tomcat 구현하기] 테드(김규태) 미션 제출합니다. #572

Kimprodp commented Sep 6, 2024

jhon3242 left a comment

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 6, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 10, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 Sep 6, 2024

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024 •

edited

Loading

jhon3242 Sep 6, 2024

Kimprodp Sep 9, 2024

jhon3242 left a comment

jhon3242 Sep 10, 2024

jhon3242 Sep 10, 2024

jhon3242 Sep 10, 2024

jhon3242 Sep 10, 2024

jhon3242 Sep 10, 2024

Kimprodp Sep 13, 2024

jhon3242 Sep 10, 2024

Kimprodp Sep 13, 2024

jhon3242 Sep 10, 2024

Kimprodp Sep 13, 2024

jhon3242 Sep 10, 2024

		String[] headerLineParts = readLine.split(": ");
		headers.put(headerLineParts[0], headerLineParts[1]);


		public class SessionManager {

		private static final Map<String, Session> SESSIONS = new HashMap<>();

		private final String id;
		private final Map<String, Object> values = new HashMap<>();

[1 - 2 단계 Tomcat 구현하기] 테드(김규태) 미션 제출합니다. #572

[1 - 2 단계 Tomcat 구현하기] 테드(김규태) 미션 제출합니다. #572

Conversation

Kimprodp commented Sep 6, 2024

jhon3242 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

Kimprodp Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhon3242 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

Kimprodp Sep 9, 2024 •

edited

Loading