[톰캣 구현하기 1, 2단계] 마코(이규성) 미션 제출합니다. #332

aak2075 · 2023-09-04T06:58:11Z

버스타고 사장님 반갑습니다~👋
결국 리뷰어로 만나게 됐네요. 잘 부탁드립니다~~

전체적인 흐름은 다음과 같습니다.

HttpRequestReader 에서 request를 파싱하여 HttpRequest를 만든다.
Handler에서 파싱한 HttpRequest를 처리하여 응답을 HttpResponse를 만든다.
HttpResponseWriter에서 HttpResponse를 파싱하여 응답을 내려준다.

handler에서 핸들링하는 코드도 복잡할 뿐만 아니라 비즈니스 로직도 섞여있어서 리팩토링이 필요하나 고민이 더 필요할 것 같습니다.

부족한 점이나 궁금한 점, 피드백 얼마든지 환영합니다!😄

xxeol2

소소한 리뷰 남겼습니다!
오늘 밤이나 내일중으로 추가 리뷰 남길게요 ㅎㅎ

xxeol2 · 2023-09-04T08:16:04Z

tomcat/src/main/java/org/apache/coyote/http11/util/HttpRequestReader.java

+    }
+
+    public static HttpRequest read(InputStream inputStream) throws IOException {
+        final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream));


InputStream은 try-with-resource 구문으로 닫아주고있지만, InputStreamReader와 BufferedReader는 그러지 않네요!
랩핑한 스트림을 닫을 때 내부 스트림은 함께 닫히지만, 그 반대는 아닌걸로 알고있어요!
이 친구들도 사용이 끝나면 닫아주는건 어떨까요?

내부 코드를 확인해 보니 inputStream의 close()는 closed = true 처리를 해줄 뿐이고 다른 작업을 하지 않네요. BufferedReader의 close()는 생성할때 파라미터로 받은 Reader인 in을 in.close()를 호출하고 null처리를 해주는 식으로 처리해주고 있네요!😱 요 친구도 함께 닫아주도록 하겠습니다.

xxeol2 · 2023-09-04T08:17:00Z

tomcat/src/main/java/org/apache/coyote/http11/util/HttpRequestReader.java

+        if (requestLine == null) {
+            throw new InvalidHttpFormException();
+        }
+        final var splitLine = requestLine.split(" ");


" ", ": "와 같은 구분자를 상수로 정의해보는건 어떨까요?

아주 좋은 생각입니다💡

xxeol2 · 2023-09-04T08:18:48Z

tomcat/src/main/java/org/apache/coyote/http11/util/HttpRequestReader.java

+    private static void parseUri(HttpRequest httpRequest, String uri) {
+        if (uri.contains("?")) {
+            final var queryStartIndex = uri.indexOf("?");
+            final var queryString = uri.substring(queryStartIndex + 1);
+            final var parameters = parseParameters(queryString);
+            httpRequest.addParameters(parameters);
+        }
+
+        final var path = getFilePath(uri);
+        httpRequest.setUri(uri);
+        httpRequest.setPath(path);
+    }


이친구가 사용되는 메서드 바로 밑으로 이동하면 더 보기 좋을 것 같아요

bfs로 되어있었네요 dfs로 변경했습니다ㅎㅎㅎ

xxeol2 · 2023-09-04T08:25:32Z

tomcat/src/main/java/org/apache/coyote/http11/util/HttpRequestReader.java

+    private static Map<String, String> parseParameters(String queryString) {
+        Map<String, String> parameters = new HashMap<>();
+        String[] parametersArray = queryString.split("&");
+        for (String parameter : parametersArray) {
+            String[] split = parameter.split("=");
+            parameters.put(split[0], split[1]);
+        }
+        return parameters;
+    }


http://localhost:8080/login?account 이렇게 요청이 들어오면 어떻게 될까요?! 그리고 어떻게 처리하는게 좋을까요?

ArrayIndexOutOfBoundsException이 발생하는군요!

private static Map<String, String> parseParameters(String queryString) { return Arrays.stream(queryString.split(QUERY_STRING_DELIMITER)) .map(parameter -> parameter.split(KEY_VALUE_DELIMITER)) .filter(keyValuePair -> keyValuePair.length == 2) .collect(Collectors.toMap(keyValuePair -> keyValuePair[0], keyValuePair -> keyValuePair[1])); }

위와 같이 keyValuePair의 length가 2인지 검증하도록 변경했습니다~
고민을 하다 보니 HttpRequest를 읽는 HttpRequestReader에서 파싱을 하는 작업도 하는 것이 책임 분리가 필요하다고 생각하여 HttpParser를 만들어서 파싱하는 책임을 이 친구한테 넘겼습니다.

xxeol2 · 2023-09-04T08:36:51Z

tomcat/src/main/java/org/apache/coyote/http11/util/HttpRequestReader.java

+            if (session == null) {
+                throw new SessionNotFoundException();
+            }


올바르지 않은 세션정보가 넘어왔을 때 예외를 던지는 대신, 세션을 없는 친구처럼 여기는건 어떨까요?!
만료된 세션 정보로 요청을 보냈을 때, 접근자체가 불가능한 것 보다 로그아웃상태로 접근되는게 더 자연스러운 흐름일 것 같아요!

이거 누가 짰죠???👀

xxeol2 · 2023-09-04T08:44:18Z

tomcat/src/main/java/nextstep/handler/Handler.java

+
+        if (account.isEmpty() || password.isEmpty()) {
+            httpResponse.setHttpStatus(HttpStatus.OK);
+            httpResponse.addHeader(HttpHeaders.LOCATION, "/500.html");


400 페이지 하나 만드시죠 ㅎㅎ ㅋㅋㅋㅋ
상태코드는 왜 200인가요?!

~~오케이도 나쁘지 않죠~~
뷰도 추가하고 400으로 바꿨습니다

xxeol2 · 2023-09-04T08:49:18Z

tomcat/src/main/java/nextstep/handler/Handler.java

+        for (String parameter : parameters) {
+            String[] keyValuePair = parameter.split("=");
+            if (keyValuePair[0].equals("account")) {
+                account = keyValuePair[1];
+            }
+            if (keyValuePair[0].equals("password")) {
+                password = keyValuePair[1];
+            }
+        }


login 페이지에서 빈 칸으로 요청하면 어떻게될까요 ?!

account=&password= 이런 식으로 요청이 들어와서 ArrayIndexOutOfBoundsException이 발생하네요!!
자주 사용되는 body부분의 파싱이라 검증 로직을 추가하여 HttpParser 에서 다음과 같이 공통으로 처리하도록 변경했습니다~

public static Map<String, String> parseFormData(String formData) { return Arrays.stream(formData.split(PARAMETER_SEPARATOR)) .map(parameter -> parameter.split(KEY_VALUE_DELIMITER)) .filter(keyValuePair -> keyValuePair.length == 2) .collect(Collectors.toMap(keyValuePair -> keyValuePair[0], keyValuePair -> keyValuePair[1])); }

xxeol2 · 2023-09-04T08:51:05Z

tomcat/src/main/java/nextstep/handler/Handler.java

+        for (String parameter : parameters) {
+            String[] keyValuePair = parameter.split("=");
+            if (keyValuePair[0].equals("account")) {
+                account = keyValuePair[1];
+            }
+            if (keyValuePair[0].equals("password")) {
+                password = keyValuePair[1];
+            }
+            if (keyValuePair[0].equals("email")) {
+                email = keyValuePair[1];
+            }
+        }


얘도 마찬가지로 회원가입 페이지에서 빈 칸으로 요청하면 어떻게될까요 ?!

위에서 정의한 HttpParser 재사용👍

xxeol2 · 2023-09-04T08:54:05Z

tomcat/src/main/java/nextstep/handler/Handler.java

+            httpResponse.addHeader(HttpHeaders.LOCATION, "/500.html");
+            return;
+        }
+        InMemoryUserRepository.save(new User(account, password, email));


사소한건데 이미 존재하는 account로 가입하려할 때 예외처리해주는건 어떤가요?!

반영했습니다~
서비스 분리가 하고싶어지네요....ㅋㅋㅋㅋㅋ

xxeol2 · 2023-09-04T08:54:34Z

tomcat/src/main/java/nextstep/handler/Handler.java

+    private static void checkUser(HttpResponse httpResponse, String account, String password) {
+        InMemoryUserRepository.findByAccount(account)
+                .filter(user -> user.checkPassword(password))
+                .ifPresentOrElse(user -> loginSuccess(httpResponse, user),
+                        () -> loginFailed(httpResponse));
+    }
+
+    private static void loginSuccess(HttpResponse httpResponse, User user) {
+        httpResponse.setHttpStatus(HttpStatus.FOUND);
+        httpResponse.addHeader(HttpHeaders.CONTENT_TYPE, TEXT_HTML);
+        httpResponse.addHeader(HttpHeaders.LOCATION, INDEX_HTML);
+        final var session = new Session(UUID.randomUUID().toString());
+        session.setAttribute("user", user);
+        SessionManager.add(session);
+        httpResponse.addCookie(new HttpCookie(HttpCookie.JSESSIONID, session.getId()));
+    }
+
+    private static void loginFailed(HttpResponse httpResponse) {
+        httpResponse.setHttpStatus(HttpStatus.FOUND);
+        httpResponse.addHeader(HttpHeaders.CONTENT_TYPE, TEXT_HTML);
+        httpResponse.addHeader(HttpHeaders.LOCATION, "/401.html");
+    }


얘도 사용하는 메서드 밑으로 옮겨주시면 읽을 때 더 편할 것 같아요!

오잉 여기 아닌가요?

xxeol2

구조에 대한 리뷰를 드리고 싶었는데, 이미 구조를 깔끔하게 잘 잡아주셨네요 👍
아래 리뷰들은 변경을 요구하는 건 아니고 3단계때 반영해주셔도 좋을 것 같아요!
앞서 남긴 소소한 리뷰들 반영하시고 재요청 보내주세요!
하루살이 화이팅!

xxeol2 · 2023-09-04T13:08:47Z

tomcat/src/main/java/nextstep/handler/Handler.java

+    public static void handle(HttpRequest httpRequest, HttpResponse httpResponse)
+            throws IOException {
+        final var path = httpRequest.getPath();
+        HttpMethod method = httpRequest.getHttpMethod();
+        if (method == HttpMethod.GET && path.equals("/")) {
+            httpResponse.setHttpStatus(HttpStatus.OK);
+            httpResponse.setBody("Hello world!");
+            return;
+        }
+        if (method == HttpMethod.GET && path.isEmpty()) {
+            httpResponse.setHttpStatus(HttpStatus.OK);
+            httpResponse.addHeader(HttpHeaders.CONTENT_TYPE, TEXT_HTML);
+            httpResponse.setBody(FileReader.readFile(INDEX_HTML));
+            return;
+        }
+        if (method == HttpMethod.GET && (path.equals("/login") || path.equals("/login.html"))) {
+            Session session = httpRequest.getSession();


요청이 추가될 때 요청들이 모두 Handler의 handle 메서드에 추가되어 메서드가 너무 길어질 것 같은데요,
요청별로 하위 Handler을 만들어주는건 어떨까요?!
이건 이번 과정에서 수정을 요구하는 건 아니고, 3단계 리팩토링 때 고민해주세요!!

핸들러는 핸들링만 하도록 리팩토링 해보겠습니다 호호

xxeol2 · 2023-09-04T13:13:46Z

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

+            HttpRequest httpRequest = HttpRequestReader.read(inputStream);
+            final var httpResponse = new HttpResponse();
+            Handler.handle(httpRequest, httpResponse);
+            HttpResponseWriter.write(outputStream, httpResponse);


실제 서블릿 동작 과정과 유사하게 Handler에 httpRequest, httpResponse 객체를 넘기고 Handler가 httpResponse에 응답을 담아주는 것 정말 좋네용
따봉 드리겠슴니다.

영광입니다👍

xxeol2 · 2023-09-04T13:20:00Z

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

+    public void setPath(String path) {
+        this.path = path;
+    }
+
+    public HttpCookie getCookie(String name) {
+        return cookies.get(name);
+    }
+
+    public Session getSession() {
+        return this.session;
+    }
+
+    public void setSession(Session session) {
+        this.session = session;
+    }


Response에서는 응답을 담아주기 위해 setter가 필요했지만, Request는 클라이언트의 응답을 엔티티로 변환한것이니 도중에 변하는 것 보다 불변 객체로 만들어주는 게 좋을 것 같아요!
생성자도 빈 생성자를 열어두고 헤더/바디를 넣어주는 구조보단, 생성할 때 헤더/바디를 초기화시키는게 좋지 않을까?라는 생각이 드는데 어떻게 생각하시나요!

오! Request는 불변이 충분히 가능하네요!!
다만, 파라미터가 너무 많아서 Builder를 통해 생성하도록 수정했습니다!
Response도 래핑을 한다거나 객체를 분리하면 가능할 것 같은데 요건 다음 단계에서 리팩토링하면서 고민해보곘습니다!

sonarcloud · 2023-09-05T11:53:59Z

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

aak2075 · 2023-09-05T12:03:01Z

축제처럼 뻥뻥 터져서 저도 버스터콜 이용해야겠네요.
부족한 점이 많았는데 애쉬의 꼼꼼한 피드백 덕분에 많이 좋아졌습니다!
피드백을 반영하면서 Handler의 책임이 너무 많고 중복으로 사용되거나 하는 코드가 많았는데요,
뷰를 찾거나 바디에 메시지를 쓰는 등의 책임을 ViewResolver로 분리했습니다.
점진적으로 리팩토링하면서 핸들링하는 부분도 나누면 깔끔해질 것 같습니다.
하루살이 화이팅!

xxeol2

하산하시죠!
깔끔한 코드 잘 봤습니다 🕶
3-4단계때 뵙겠습니다아

xxeol2 · 2023-09-05T23:44:05Z

tomcat/src/main/java/common/FileReader.java

@@ -9,6 +9,7 @@
 public class FileReader {

    private static final String STATIC_RESOURCE_PATH = "static";
+    private static final String EXTENSION_HTML = ".html";


DEFAULT_FILE_EXTENSION 은 어떤가요?!
기본 확장자가 .html인걸 더 잘 알려주는 것 같아요!

xxeol2 · 2023-09-05T23:45:14Z

tomcat/src/main/java/nextstep/mvc/ModelAndView.java

+public class ModelAndView {
+
+    private final String viewName;
+    private final Map<String, String> model = new HashMap<>();


좋은데요?

xxeol2 · 2023-09-05T23:47:29Z

tomcat/src/main/java/nextstep/mvc/View.java

+        if (content == null) {
+            StringBuilder sb = new StringBuilder();
+            for (final var value : model.values()) {
+                sb.append(value);
+            }
+            httpResponse.addHeader(HttpHeaders.CONTENT_TYPE, DEFAULT_CHAR_SET);
+            httpResponse.addHeader(HttpHeaders.CONTENT_LENGTH,
+                    String.valueOf(sb.toString().getBytes().length));
+            httpResponse.setBody(sb.toString());
+            return;
+        }


early return 👍

xxeol2 · 2023-09-05T23:50:05Z

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

+    public static class Builder {
+
+        private String protocol;
+        private HttpHeaders headers;
+        private HttpMethod httpMethod;
+        private String uri;
+        private String path;
+        private Map<String, String> parameters;
+        private String body;


Builder 👍
빌더가 여러 클래스에서 생길수도있으니 HttpRequestBuilder로 정확하게 명시해주는것도 좋을 것 같아요!

aak2075 added 5 commits September 3, 2023 18:24

feat: 1단계 - HTTP 서버 구현하기

9303da4

feat: 2단계 - 1.HTTP Status Code 302

a9f593d

feat: 2단계 - 2. POST 방식으로 회원가입

f64db47

feat: 2단계 - 3,4 Cookie에 JSESSIONID 값 저장하기 및 Session 구현하기

2b05c7c

refactor: 개행 정리 및 불필요한 메서드 제거

3069ea7

aak2075 requested a review from xxeol2 September 4, 2023 06:58

aak2075 self-assigned this Sep 4, 2023

refactor: 상수 대문자로 변경

dc50303

xxeol2 requested changes Sep 4, 2023

View reviewed changes

aak2075 added 4 commits September 5, 2023 19:59

refactor: 뷰 리졸버 분리 리팩토링

00417a7

refactor: RequestBuilder 도입

7bc374c

refactor: 사용하지 않는 import 제거

91004b2

refactor: 패키지 이동

d24f44f

xxeol2 approved these changes Sep 5, 2023

View reviewed changes

xxeol2 merged commit c2626c7 into woowacourse:aak2075 Sep 5, 2023
2 checks passed

[톰캣 구현하기 1, 2단계] 마코(이규성) 미션 제출합니다. #332

[톰캣 구현하기 1, 2단계] 마코(이규성) 미션 제출합니다. #332

Conversation

aak2075 commented Sep 4, 2023

xxeol2 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxeol2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxeol2 Sep 4, 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

sonarcloud bot commented Sep 5, 2023

aak2075 commented Sep 5, 2023 • edited Loading

xxeol2 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

xxeol2 left a comment •

edited

Loading

xxeol2 Sep 4, 2023 •

edited

Loading

aak2075 commented Sep 5, 2023 •

edited

Loading