[톰캣 구현하기 1, 2단계] 애쉬(정설희) 미션 제출합니다. #338

xxeol2 · 2023-09-04T07:41:09Z

도이 안녕하세요!
전체적인 흐름은 아래와 같습니다.

Http11Processor에서 Request를 생성합니다.
HandlerMapping에서 url 주소를 토대로 요청을 처리할 handler를 찾고 해당 handler에게 요청을 보냅니다.
Handler내부에는 handle서비스가 HTTP Method 종류에 따라 doGet() 혹은 doPost()를 호출합니다.
Handler가 Response를 반환하고, HTTP Response의 형태로 outputStream에 써줍니다.

리팩토링할 부분도 많고, 테스트코드도 없는데(ㅠㅠ) 이는 이후 과정 진행하면서 보완하겠습니다!
리뷰하시다 코드에서 헷갈리는 부분이 있으면 DM 주세요!

yoondgu

안녕하세요 애쉬 🏹

정말 감탄하면서 봤습니다 ... 덕분에 좋은 코드 보면서 많이 배운 것 같아 감사해요!!
전체적으로 일관성 있게, 잘 설계해주신 것 같아요.
과하지 않으면서도 섬세하게 HTTP 서버를 구현해주셨다는 인상을 받았습니다.

SornaCube가 남겨준 내용, 개행 및 컨벤션에 대해서는 잘 아시리라 생각하고,
리팩터링 진행 예정이니 별도 코멘트 남기지 않았습니다!

실행해보니 기능도 잘 동작하고, 테스트 코드만 추가되면 정말 완벽하겠는데요 🫢
좋았던 부분과 같이 이야기하고 싶은 부분 위주로 가볍게 남겨보았으니 확인해주세요!

yoondgu · 2023-09-04T13:23:47Z

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

@@ -0,0 +1,25 @@
+package org.apache.coyote.http11;
+
+public enum HttpProtocolVersion {


Protocol 버전도 enum으로 관리해주셨군요 !! 꼼꼼해서 좋습니당 😀

yoondgu · 2023-09-04T13:39:07Z

tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapping.java

+public class HandlerMapping {
+
+	private static final List<RequestHandler> handlers;
+	private static final RequestHandler defaultHandler;


defaultHandler를 두는 것 너무 좋네요 !! 절차지향적일 수도 있는 부분을 잘 해결해주신 것 같아요

yoondgu · 2023-09-04T13:41:39Z

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

+	private Request readRequest(final BufferedReader reader) throws IOException {
+		final var requestLine = readRequestLine(reader);
+		final var requestHeader = readRequestHeader(reader);
+		final var requestBody = readRequestBody(reader, requestHeader);
+		return new Request(requestLine, requestHeader, requestBody);
+	}


객체를 만드는 메서드의 형식이 일관적이어서 메서드 분리만으로도 너무 깔끔하고 읽기 좋은 코드를 작성해주신 것 같아요!!
너무 술술 잘 읽힙니다 ㅎㅎㅎ

yoondgu · 2023-09-04T13:42:09Z

tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapping.java

+	static {
+		handlers = List.of(
+			new BaseRequestHandler(),
+			new LoginRequestHandler(),
+			new RegisterRequestHandler()
+		);
+		defaultHandler = new StaticContentRequestHandler();
+	}


static 블록을 사용하신 이유가 무엇인지 궁금합니다!
인스턴스를 생성하더라도 딱 한 번 실행되게 하기 위함일까 생각했는데 인스턴스 생성 자체를 막은 클래스인 것 같아서요!
가독성이 이유일까요?!

HandlerMapping 클래스는 핸들러들을 맵핑만 해주는 역할인데, 인스턴스화가 되어야하나? 라는 의문에 handle 메서드를 static 메서드로 두게되었습니다!

handlers, defaultHandler 필드를 초기화해주는걸 필드에서 바로 해줄 수도 있고 static 블럭에서 해줄수도 있었는데, 필드에서 해주면 handlers가 길어질 경우 어떤 필드가 있는지 한눈에 파악하기 어려울 것 같아 static 블럭을 활용해보았습니다!!

yoondgu · 2023-09-04T13:43:06Z

tomcat/src/main/java/org/apache/coyote/http11/handler/RequestHandler.java

+	public Response handle(final Request request) {
+		if (request.hasMethod(HttpMethod.GET)) {
+			return doGet(request);
+		} else if (request.hasMethod(HttpMethod.POST)) {
+			return doPost(request);
+		}
+		return Response.notFound();
+	}


추후 3단계 리팩터링이나 기능을 확장해 나간다면, 이 부분도 매핑(HttpMethod - doXXX 메서드)시키는 방식으로 분기를 처리해도 괜찮겠네요 !

함수형 인터페이스를 활용한 방식을 말씀하시는걸까요 ?!
사실 저도 모르게 실제 HttpServlet의 service() 메서드 구조를 따라가게된 것 같아요..
3단계 리팩토링 적용하면서 도이가 말씀해주신 방식 한 번 적용해보겠습니다 !!

네 그렇게도 할 수 있겠다고 생각해서 언급헀는데요.
사실 Http Method의 개수는 한정적이어서 굳이 Map을 두지 않고 if문 분기로 명시하는 것도 괜찮아보이긴 해요!
취향대로 하셔도 될 것 같습니다 ㅎㅎ

yoondgu · 2023-09-04T14:01:38Z

tomcat/src/main/java/org/apache/coyote/http11/handler/StaticContentRequestHandler.java

+	public boolean canHandle(Request request) {
+		return true;
+	}


이 Handler에서는 항상 true를 반환하는 이유가 뭘까요 ?
path가 파일명을 가리키지 않고 있는 경우 (ex. /wooteco, 다른 핸들러에 매핑되지 않음)에도 true를 반환하는 것이 자연스러울까요?!

항상 true를 return 함으로써 StaticContentRequestHandler는 Default 핸들러로, 다른 핸들러와 맵핑되지 않는 모든 요청이 해당 핸들러로 들어가도록 의도했습니다!
하지만 해당 핸들러가 Default 핸들러로 사용되지 않을 가능성이 있기에, path가 파일명을 가리키는 경우 canHandle을 반환하도록 하는것이 좋아보이네요!

+) canHandle 메소드가 사라지면서 해당 로직도 사라졌습니다!

yoondgu · 2023-09-04T14:04:31Z

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

+	private static final String DELIMITER = "&";
+	private static final String KEY_VALUE_SEPARATOR = "=";
+
+	private final Map<String, String> fields = new HashMap<>();


RequestBody의 내용이 항상 key=value 일까요? 물론 지금 요구사항에서는 적절하다고 생각합니다!
하지만 자료구조로만 보면 RequestUri의 QueryParameter와 동일시할 수 있겠네요 !

json 형식이나 다른 형식의 ResponseBody가 들어오면 오류가 나오겠군요 ... 🥲
그래서 Request Body에도 Content-Type이 존재하는거군요 .. ! 깊은 깨달음을 얻고갑니다!!!
이 부분도 3-4단계 진행하며 Content-Type에 따라 확장성있게 받을 수 있는 방법이 있는지 함께 고민해보겠습니다!

yoondgu · 2023-09-04T14:09:10Z

tomcat/src/main/java/org/apache/coyote/http11/response/Response.java

+ */
+public class Response {
+
+	private static final String CRLF = "\r\n";


System.lineSeperator() 를 사용하는 건 어떻게 생각하시나요 ?!

Windows는 줄바꿈을 위해 CRLF를 사용하는 반면, Unix/Linux 기반 운영체제에서는 LF를 사용한다고 합니다!
하지만, HTTP/1.1 프로토콜은 개행 문자로 CRLF를 사용한다고 합니다!
운영체제에 상관없이 잘 파싱해주기 위해서는 CRLF를 상수로 사용하는게 좋을 것 같아요!

하지만, HTTP/1.1 프로토콜은 개행 문자로 CRLF를 사용한다고 합니다!

헉 ! 몰랐던 사실이네요 !! 배워갑니다 ,, 저도 상수로 사용해야겠어요!

yoondgu · 2023-09-04T14:10:56Z

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

+	public String formatMimeType() {
+		if (!isText) {
+			return mimeType;
+		}
+		return addCharsetParam(mimeType);
+	}


Text 여부까지.. 👍👍👍

yoondgu · 2023-09-04T14:17:01Z

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

+	public static MimeType fromPath(final String path) {
+		validatePath(path);
+		for (final MimeType mimeType : values()) {
+			if (path.endsWith(mimeType.fileExtension)) {
+				return mimeType;
+			}
+		}
+		throw new IllegalArgumentException("지원하지 않는 파일 확장자입니다.");
+	}


MimeType까지 잘 고려해주셔서 애쉬의 HTTP 지식이 느껴집니다 😀
그런데 Java 기본 라이브러리에도 생각보다 File, Path 관련 제공 메서드들이 많더라구요.
그 중 이 부분을 조금 더 간단하게 해줄 수 있는 게 있을 것 같아요!

덕분에 File,Path 관련 제공 메서드들에 대해서 찾아보고 학습했습니다 감사해요!
하지만 지금은 몇가지 파일 확장자만 지원하고 있기 때문에, String.endsWith로 검사해도 충분할 것 같다는 생각이 들어요!
이 부분은 3-4단계 진행하며 확장성있는 프로그램을 만드는 과정에서 함께 고민해보겠습니다!

- coyote: HTTP 웹 서버 / catalina: 서블릿컨테이너 / jwp: 사용처 - http11: http1.1에 맞는 RequestReader/ResponseWriter 정의 - ResponseHeader commonHeader 추가

sonarcloud · 2023-09-07T14:55:10Z

Kudos, SonarCloud Quality Gate passed!

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

yoondgu

수고 많으셨습니다 애쉬!!
사실상 3단계도 완료하신 것 같네요 ㅎㅎㅎ
코드 너무 잘 봤습니다. 적절한 패키지 분리, 핸들러를 주입하는 부분이 특히 인상적이었어요.

지난 Request change도 몇 가지 생각 공유를 위한 것이었기 때문에,
이번에는 남겨주신 코멘트에 대한 답변으로 마무리하고, 바로 머지하도록 할게요 !
다음 단계 기대하겠습니다 😬

yoondgu · 2023-09-07T15:37:51Z

tomcat/src/main/java/nextstep/Application.java

+		final var handlers = Set.of(
+			new BaseRequestHandler(),
+			new LoginRequestHandler(),
+			new RegisterRequestHandler()
+		);
+		final var defaultHandler = new StaticContentRequestHandler();
+		final var handlerMapping = new HandlerMapping(handlers, defaultHandler);


어플리케이션 실행할 때 주입해주는 거 너무 좋은데요 ..
애쉬 코드 모두 훔치고 싶어지네요 🥹

yoondgu · 2023-09-07T15:49:44Z

tomcat/src/main/java/nextstep/jwp/handler/LoginRequestHandler.java

+	private static final String REQUEST_PATH = "/login";
+	private static final String LOGIN_PAGE_PATH = "/login.html";
+	private static final String REDIRECT_LOCATION = "/index.html";


url은 상수 대신 직접 쓰는 게 읽기 편하다고 생각하는 편이긴 한데,
여러 번 쓰이니까 상수로 하는 게 더 안전해 보이네요!

yoondgu · 2023-09-07T15:56:33Z

tomcat/src/main/java/org/apache/catalina/HandlerMapping.java

+	public HandlerMapping(final Set<RequestHandler> handlers, final RequestHandler defaultHandler) {
+		this.handlers = handlers.stream()
+			.collect(Collectors.toMap(
+				RequestHandler::getRequestPath,
+				handler -> handler
+			));
+		this.defaultHandler = defaultHandler;
+	}


관련 코멘트 에 답변 달았습니다 !

yoondgu · 2023-09-07T15:59:16Z

tomcat/src/main/java/nextstep/jwp/db/InMemoryUserRepository.java


 public class InMemoryUserRepository {

-    private static final Map<String, User> database = new ConcurrentHashMap<>();
+	private static final Map<String, User> database = new ConcurrentHashMap<>();
+	private static final AtomicLong idCounter = new AtomicLong();


AtomicLong 배워갑니다 😯😯

yoondgu · 2023-09-07T16:17:06Z

tomcat/src/main/java/org/apache/coyote/MimeType.java

+	public static MimeType fromPath(final String path) {
+		validatePath(path);
+		final var optionalMimeType = Arrays.stream(values())
+			.filter(mimeType -> path.endsWith(mimeType.fileExtension))
+			.findAny();
+		if (optionalMimeType.isEmpty()) {
+			throw new IllegalArgumentException("지원하지 않는 파일 확장자입니다.");
+		}
+		return optionalMimeType.get();
+	}


빼먹고 올린 코멘트가 하나 있어서 추가합니다 ㅎ.ㅎ

public static MimeType fromPath(final String path) { validatePath(path); return Arrays.stream(values()) .filter(mimeType -> path.endsWith(mimeType.fileExtension)) .findAny() .orElseThrow(() -> new IllegalArgumentException("지원하지 않는 파일 확장자입니다.")); }

이렇게 해줘도 될 것 같아요 !

xxeol2 added 22 commits September 3, 2023 12:51

style: 코드 포매팅

ace4a6f

feat: http 서버 구현

d52fc20

refactor: HttpRequest / HttpResponse 도입

e0e14af

feat: login시 redirect 기능 구현

2cd41e2

feat: login 메서드 post로 수정

323f043

refactor: 응답/요청 객체 이름에서 Http 제거

d2ede1b

refactor: buffer 읽어오는 방식 수정

f436723

refactor: 메서드 추출

8a4dbf8

feat: IllegalArgumentException 발생시 NotFound 응답 return

688b5b2

feat: 회원가입 기능 구현

7679807

feat: 로그인시 쿠키 set

c6f0672

feat: 세션 활용

8148e2d

refactor: Cookies 클래스 네이밍 수정

cdfbf0d

refactor: HashMap 생성자에서 초기화

4cc32ec

refactor: session attribute 삭제

7b2e322

refactor: session의 attribute로 user저장

e1d25ef

refactor: Handler 추상클래스로 변경

dd7b3cc

feat: 400 에러 페이지 추가

817fa61

refactor: addCookie 메서드 추가

7c7b720

refactor: 메서드 추출

7776940

refactor: HttpMethod from을 활용하도록 수정

3ac5300

refactor: InMemoryUserRepository에서 id 저장 로직 추가

57d1cd9

yoondgu requested changes Sep 4, 2023

View reviewed changes

xxeol2 added 4 commits September 6, 2023 09:06

refactor: defaultHandler requestPath 수정

0109872

refactor: HandlerMapping 방식 Map으로 수정

f2d5a75

refactor: tomcat 구조에 맞게 리팩토링

1c30838

- coyote: HTTP 웹 서버 / catalina: 서블릿컨테이너 / jwp: 사용처 - http11: http1.1에 맞는 RequestReader/ResponseWriter 정의 - ResponseHeader commonHeader 추가

refactor: 스트림을 활용한 리팩토링

f17aa67

yoondgu approved these changes Sep 7, 2023

View reviewed changes

yoondgu merged commit 3db368d into woowacourse:xxeol2 Sep 7, 2023
2 checks passed

yoondgu reviewed Sep 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 1, 2단계] 애쉬(정설희) 미션 제출합니다. #338

[톰캣 구현하기 1, 2단계] 애쉬(정설희) 미션 제출합니다. #338

xxeol2 commented Sep 4, 2023 •

edited

Loading

yoondgu left a comment

yoondgu Sep 4, 2023

yoondgu Sep 4, 2023

yoondgu Sep 4, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 5, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 5, 2023

yoondgu Sep 7, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 6, 2023

xxeol2 Sep 7, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 7, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 7, 2023

yoondgu Sep 7, 2023 •

edited

Loading

yoondgu Sep 4, 2023

yoondgu Sep 4, 2023

xxeol2 Sep 7, 2023

sonarcloud bot commented Sep 7, 2023

yoondgu left a comment

yoondgu Sep 7, 2023

yoondgu Sep 7, 2023

yoondgu Sep 7, 2023

yoondgu Sep 7, 2023

yoondgu Sep 7, 2023

		@@ -0,0 +1,25 @@
		package org.apache.coyote.http11;

		public enum HttpProtocolVersion {

[톰캣 구현하기 1, 2단계] 애쉬(정설희) 미션 제출합니다. #338

[톰캣 구현하기 1, 2단계] 애쉬(정설희) 미션 제출합니다. #338

Conversation

xxeol2 commented Sep 4, 2023 • edited Loading

yoondgu 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

yoondgu Sep 7, 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 7, 2023

yoondgu 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

xxeol2 commented Sep 4, 2023 •

edited

Loading

yoondgu Sep 7, 2023 •

edited

Loading