[톰캣 구현하기 1 & 2단계] 오리(오현서) 미션 제출합니다 #345

carsago · 2023-09-04T07:56:24Z

부탁드립니다 우석씨

Http11Processor에서 httpRequest를 생성

Handler로 넘김

Handler에서 requestHandler로 넘기고, httpResponse로 반환

HttpResponseParser가 response를 parse하여 byte 형태로 반환

Http11Processor가 잘 처리합니다.

**** 9월 4일, 17시 11분
-- 로그인 페이지에 들어갔을 때, 로그인 여부를 확인해야하는데, 로그인 요청시에 하도록 했네요 ㅠ

sonarcloud · 2023-09-04T07:59:33Z

Kudos, SonarCloud Quality Gate passed!

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

Songusika · 2023-09-05T06:02:06Z

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

@@ -9,13 +10,15 @@
 public class InMemoryUserRepository {

    private static final Map<String, User> database = new ConcurrentHashMap<>();
+    private static final AtomicLong id = new AtomicLong(2L);


오 이런 역할을 해주는 클래스가 있었군요! 배우고 갑니다!

Songusika · 2023-09-05T06:33:14Z

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

+            return signUp(request);
+        }
+
+        throw new IllegalArgumentException();


해당 상황은 적절한 요청 핸들러가 없어서 발생하는 것이네요!
사용자가 옳바르지 않은 요청을 보내는 것이 정말 예외인 상황일까요?
어떻게 보면 이부분도 우리가 예측할 수 있는 상황인 것 같아요
예외 보다는 404를 내려주는 것이 어떨까요?

Songusika · 2023-09-05T06:40:14Z

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

+    private static final Logger log = LoggerFactory.getLogger(Http11Processor.class);
+
+    public HttpResponse handle(HttpRequest request) throws IOException {
+        if (request.getUri().equals("/") && request.getMethod() == HttpMethod.GET) {


request 에서 직접 관련 필드를 꺼내오는 것보다.
request 객체한테 직접 물어볼 수 있을 것 같아요!

이 부분에 대해서 request 한테 너무 디테일한 책임을 주면 끝도 없이 메소드가 많아 질 것 같아서, 최소한으로하고 가독성만 개선해볼게요.

Songusika · 2023-09-05T06:47:39Z

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

+    private HttpResponse getFile(HttpRequest request) throws IOException {
+        String fileUrl = "static" + request.getUri();
+        File file = new File(
+            RequestHandler.class


이 부분은 궁금한 것 인데요!
ClassLoader.getSystemClassLoader() 말고 클래스 리터럴을 사용하는 이유가 있을까요?

종종 발생하는 실수입니다

Songusika · 2023-09-05T06:54:27Z

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

+    private HttpResponse signUp(HttpRequest request) {
+        Map<String, String> body = request.getBody();
+        InMemoryUserRepository.save(new User(
+            body.get("account"),
+            body.get("password"),
+            body.get("email")
+        ));
+        return HttpResponse.found("/index.html");
+    }
+}


사용자가 acc, pw 와 같은 변수로 요청을 보낼 수 도 있을 것 같아요!

이 부분은 null체크가 필요하다는 말씀이실까요?

맞습니다!

Songusika · 2023-09-05T11:44:00Z

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

+
+    private static final RequestHandler requestHandler = new RequestHandler();
+
+    public static byte[] handle(HttpRequest request) {


HttpRequest 라는 객체를 파라미터로 받고 있어서 HttpResponse 라는 걸 예상하는데 바이트 배열을 return 하고 있네요.
파싱 부분은 Http11Processor 에서 해도 괜찮을 것 같은데 (request 에 대한 파싱도 해당 클래스에서 하기 때문입니다.)
핸들러에서 처리 후 바이트 배열을 반환하는 이유가 있나요?

Songusika · 2023-09-05T11:50:13Z

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

+
+    private final Map<String, String> values = new ConcurrentHashMap<>();
+
+    public HttpRequest(InputStream inputStream) throws IOException {


HttpResponse 객체는 별도의 파서를 두셨는데 HttpRequest 객체도 별도의 파서를 두는 것이 어떨까요? 😀
생성자를 통해서 필요한 객체를 넘기면 실제 HttpRequest가 어떤 값들로 이루어져 있는지 더 쉽게 알 수 있을 것 같아요!

테스트 코드도 더 짜기 쉬워 질 것 같습니다!

Songusika · 2023-09-05T11:53:43Z

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

+
+public class HttpResponse {
+
+    private Map<String, String> headers;


final 누락! 👀

Songusika · 2023-09-05T11:57:16Z

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

+            HttpResponse response = requestHandler.handle(request);
+            return HttpResponseParser.parse(response);
+        } catch (IOException e) {
+            throw new RuntimeException(e);


runtimeException 대신 500 에러를 내려주는 것은 어떨까요?

Songusika · 2023-09-05T12:00:28Z

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

+
+    private boolean isLogin(HttpRequest request) {
+        HttpCookie cookie = request.getCookie();
+        if (cookie == null) {


이 부분도 Optional 을 사용할 수 있지 않을까요?

Songusika

안녕하세요 오리!
좋은 코드 잘 봤습니다!! 전체적으로 깔끔해서 이해하기 쉬웠어요
간단한(?) 피드백 해드렸습니다! 꼭 적용하세요! 가 아닌 궁금해서 드린 질문도 있어요!
편하게 반영하시면 될 것 같습니다!

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

HttpStartLine -> HttpRequestLine, target -> path

sonarcloud · 2023-09-06T04:45:02Z

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

Songusika

1,2 단계 구현하시느라 고생많으셨습니다!!
전체적으로 코드가 깔끔해서 리뷰하기가 편했어요!
정말 궁금해서 드린 질문 1,2 개 있는데 편하실때 남겨주시면 감사하겠습니다!
다음 미션에서 봬요!

Songusika · 2023-09-06T06:21:46Z

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

+            HttpResponse response = requestHandler.handle(request);
+            return HttpResponseParser.parse(response);
+        } catch (IOException e) {
+            HttpResponse response = HttpResponse.found("/500.html");


Songusika · 2023-09-06T06:21:59Z

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

+            return getFile(uri);
+        }
+
+        return getFile("/404.html");


Songusika · 2023-09-06T06:24:16Z

tomcat/src/main/java/nextstep/jwp/Constant.java

@@ -0,0 +1,9 @@
+package nextstep.jwp;
+
+public class Constant {


어떤 상수를 정의하는 건지 더 상세하면 좋을 것 같아요!

Songusika · 2023-09-06T06:26:47Z

tomcat/src/main/java/nextstep/jwp/Constant.java

+    private Constant() {
+    }
+
+    public static final String CRLF = "\r\n";


System.lineSeparator() 를 활용할 수 도 있어요!

Songusika · 2023-09-06T06:31:57Z

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

+
+    public static HttpResponse found(String location) {
+        Map<String, String> headers = new LinkedHashMap<>();
+        headers.put(HttpHeaders.CONTENT_LENGTH, "0");


body가 없을 때는 Content-Length 를 0으로 명시 혹은 해당 헤더를 넣지 않아도 되는데
오리는 어떤 이유로 0으로 직접 명시한지 기준이 궁금해요! (정말 궁금)

들어가야하는 줄 알았어요. 정말 몰랐음

carsago added 25 commits September 1, 2023 18:09

study: IOStreamTest 수행

20ba0a1

study: FileTest 학습테스트

3d7328e

feat: HttpRequest 객체 추가

26695ee

feat: HTTP 요청을 전달받는 기능 추가

f2935f2

feat: CSS 및 JS 요청을 받도록 기능 추가

4a737af

refactor: RequestHandler로 책임 분리

e00afd0

refactor: HttpRequest가 queryString 없이 uri만 반환하도록 수정

ba8c655

feat: HttpRequest가 queryString을 반환하는 기능 추가

f8d4c16

feat: 로그인시 로그 기능 추가

998749b

feat: 로그인 기능 추가

c632e96

test: 로그인 테스트 추가

97d286c

feat: HttpRequest body 반환 기능 추가

8e569ed

feat: HttpRequest가 method를 반환하는 기능 추가

dcfdda2

feat: 회원가입 기능 추가

69131ba

refactor: HttpResponse 추가

68513bd

refactor: ResponseParser 도입

e5d4dc7

feat: 로그인시 쿠키 발급

865f73b

refactor: 쿠키가 없을 때만 발급받도록 변경

d77cc9c

refactor: response에 쿠키 담도록 변경

5fd93bc

feat: Session에 쿠키 저장하는 로직 추가

342014a

refactor: html 수정

6934170

chore: 패키지 분리

9286a32

refactor: login get -> post 변경

a50cf5d

style: 잘못된 개행 변경

8f0de63

refactor: 조회 요청의 HTTP method를 확인하도록 변경

4512e8e

carsago requested a review from Songusika September 4, 2023 07:56

carsago self-assigned this Sep 4, 2023

Songusika requested changes Sep 5, 2023

View reviewed changes

tomcat/src/main/java/nextstep/jwp/handler/RequestHandler.java Show resolved Hide resolved

carsago added 17 commits September 6, 2023 13:17

fix: 로그인 페이지 요청시 로그인 여부 확인하도록 변경

c0e58f3

refactor: 회원가입 로직 변경

2369585

fix: sout 제거

20c97e3

refactor: HttpRequestParser로 책임 분리

94a1a42

refactor: 개행문자 상수화

347055c

refactor: 클래스명 변경

a7f93cd

HttpStartLine -> HttpRequestLine, target -> path

refactor: 쿠키 객체 제거

a5f9c99

refactor: 없는 요청일시 404 페이지를 반환하도록 변경

44a8c4f

refactor: ResponseParser의 반환 타입 변경

1639b13

refactor: uri 변수 추출

bcc655d

refactor: httpHeaders 도입

8b4d168

refactor: final 키워드 추가

7ac9ecd

refactor: 예외시 500 page로 redirect

fd9c3e3

chore: 패키지 변경

6482a27

test: 가독성 개선

a97f014

refactor: 필요없는 null 체크 제거

ed289d0

fix: 버그 수정

c038905

Songusika approved these changes Sep 6, 2023

View reviewed changes

Songusika merged commit 54066a6 into woowacourse:carsago Sep 6, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 1 & 2단계] 오리(오현서) 미션 제출합니다 #345

[톰캣 구현하기 1 & 2단계] 오리(오현서) 미션 제출합니다 #345

carsago commented Sep 4, 2023 •

edited

Loading

sonarcloud bot commented Sep 4, 2023

Songusika Sep 5, 2023

Songusika Sep 5, 2023 •

edited

Loading

Songusika Sep 5, 2023

carsago Sep 6, 2023

Songusika Sep 5, 2023

carsago Sep 6, 2023

Songusika Sep 5, 2023

carsago Sep 6, 2023

Songusika Sep 6, 2023

Songusika Sep 5, 2023

Songusika Sep 5, 2023

Songusika Sep 5, 2023

Songusika Sep 5, 2023

Songusika Sep 5, 2023 •

edited

Loading

Songusika left a comment

sonarcloud bot commented Sep 6, 2023

Songusika left a comment •

edited

Loading

Songusika Sep 6, 2023

Songusika Sep 6, 2023

Songusika Sep 6, 2023

Songusika Sep 6, 2023

Songusika Sep 6, 2023

carsago Sep 6, 2023


		private static final RequestHandler requestHandler = new RequestHandler();

		public static byte[] handle(HttpRequest request) {


		private final Map<String, String> values = new ConcurrentHashMap<>();

		public HttpRequest(InputStream inputStream) throws IOException {


		public class HttpResponse {

		private Map<String, String> headers;

[톰캣 구현하기 1 & 2단계] 오리(오현서) 미션 제출합니다 #345

[톰캣 구현하기 1 & 2단계] 오리(오현서) 미션 제출합니다 #345

Conversation

carsago commented Sep 4, 2023 • edited Loading

sonarcloud bot commented Sep 4, 2023

Choose a reason for hiding this comment

Songusika Sep 5, 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

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

Songusika Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Songusika left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 6, 2023

Songusika 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

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

carsago commented Sep 4, 2023 •

edited

Loading

Songusika Sep 5, 2023 •

edited

Loading

Songusika Sep 5, 2023 •

edited

Loading

Songusika left a comment •

edited

Loading