[톰캣 구현하기 - 1, 2단계] 후추(주찬민) 미션 제출합니다 #386
Conversation
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/SessionManager.java
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
|
안녕하세요 디투 😄 이번 작업에서는 HttpRequest, HttpResponse 를 객체로 분리하는 것에 초점을 맞추었어요 재리뷰 요청 드립니다 잘 부탁드려요! |
|
고생하셨습니다 후추. |
tomcat/src/main/java/org/apache/coyote/http11/request/body/RequestBody.java
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/request/body/RequestBody.java
Show resolved
Hide resolved
| public Map<String, String> parametersMap() { | ||
| return new HashMap<>(parametersMap); | ||
| } |
There was a problem hiding this comment.
하지만 추후에 사용할 수도 있는 메소드기도 하고, 톰캣에서는 진짜 구현했을 거라고 생각해요. 오버엔지니어링이라는 생각이 들진 않아요. 윗 코멘트를 남긴 이유는 해당 사항을 지금 코멘트를 남기면서 확인을 해서인데요. 이런 부분 같은 경우에는 주석을 활용해보면 어떨까요?
There was a problem hiding this comment.
저도 디투 의견에 동의합니다. 기본적인 getter, setter 는 사용하지 않더라도 남겨두는 것이 경험 상 편했어요. 다만 주석을 활용할 생각은 못 해봤습니다. 이런 경우, 디투는 어떤 식으로 주석을 달아두셨나요?
There was a problem hiding this comment.
저는 필요한 상황에 맞게 사용하고 있지 않는 메소드는 없게끔 코드를 작성하는 스타일인데요. 그래서 사용하고 있지 않은 메소드를 작성하지 않는 편이에요. (린트에 대한 압박도 있어서요ㅋㅋ) 하지만 나중에 작성해야하는 코드가 많아지는지라 쪼금 시간상으로 불편하긴 하죠ㅋㅋㅋ
후추의 방법도, 제 방법도 취향에 따라 다르다고 생각합니다.
이렇게 길게 말한 이유는, 정말 정말 개인 취향 차이이기 때문에 후추의 방법이 저랑 다르다고 해서 잘못되었다는 것이 아니라고 말씀드리고 싶었어요. 그리고 주석에 대한 생각도 코멘트를 작성하면서 떠올렸던 아이디어였습니당. 그래서 여쭈어본 말씀에 대해 대답하자면, TODO나 FIXME 와 같은 키워드(하이라이트 되는) 주석을 하나 만들어 관리할 것 같아요! 가령 WILL USE...?
tomcat/src/main/java/org/apache/coyote/http11/request/header/HttpCookie.java
Show resolved
Hide resolved
| public HttpCookie(Map<String, String> parametersMap) { | ||
| this.parametersMap = parametersMap; | ||
| } |
There was a problem hiding this comment.
주관적인 생각입니다.
쿠키의 경우 http 통신에서 header로 분류되는 것은 맞지만 헤더의 기능을 한다고 보이지는 않아요.
왜냐하면 다른 헤더들과 달리 브라우저에 쿠키값을 세팅하거나 session을 갱신하거나 저장하잖아요.
같은 헤더더라도 조금 특별한 헤더라는 생각이 들어서 저는 일반 헤더들과 분리해서 HttpRequest에 CookieHeader 라는 명칭으로 필드 선언을 했는데, 이 방법에 대해서는 어떻게 생각하시나요?
후추의 생각이 궁금해요!
There was a problem hiding this comment.
디투 말씀대로 Cookie가 다른 헤더들과 달리, 특별하게 쓰인다고 느껴요. 하지만 저는 HTTP 통신에서 Header 라는 큰 개념 안에 Cookie 가 포함된다는 사실에 주목했어요. 코드 상에서도 HTTP와 추상화 수준을 동일하게 맞추려고 했고요. 그래서 RequestHeader 안에 HeadersMap(다른 헤더들)과 HttpCookie를 필드로 두었습니다.
디투의 방식인 HttpRequest에 CookieHeader 로 필드를 갖는 것도 사용하기에 좋아보여요. 하지만 다른 개발자가 CookieHeader를 찾을 때 HTTP와 추상화 수준이 다른 것을 의아하게 여기지 않을까 생각되네요!
There was a problem hiding this comment.
오오 맞아요. 헤더라고 생각하는데 다른 레벨에 객체가 있다면 동료 개발자들이 의아하게 여길 것 같아요. 동의합니다!! 설득되었습니다!
| } | ||
|
|
||
| public boolean hasContent() { | ||
| return contentLength() > 0; |
There was a problem hiding this comment.
MINIMUN_LENGTH 상수를 int로 바꾸어서 사용하는 것은 어떻게 생각하시나요?
상수 자체로 보았을때에도 int 자료형이 옳다고 생각해서요.
만약 string 형태로 변환해야한다면 사용하는 부분에서 바꾸어야 한다고 생각합니다!
There was a problem hiding this comment.
디투는 우테코 7기에서 진짜 좋은 리뷰어로 활동하실 것 같군요!
디투의 꼼꼼함에 놀라고 갑니다!
상수 0 은 int로 바꾸었습니다. 그리고 이름도 EMPTY_CONTENT_LENGTH로 바꾸었습니다!
| public class Http11Processor implements Runnable, Processor { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(Http11Processor.class); | ||
| private static final String DEFAULT_BODY = "Hello world!"; | ||
| private static final String HTTP_VERSION = "HTTP/1.1"; |
There was a problem hiding this comment.
HTTP_VERSION이 해당 겍체의 상수라는 생각이 들지 않아요ㅠㅠ
브라우저 입장에서 HTTP/1.1로 요청을 했으면 서버에서 마찬가지로 HTTP/1.1 로 응답을 주어야한다고 생각합니다.
request의 http version을 반환해서 response까지 전달하는 방법은 어떻게 생각하시나요?
There was a problem hiding this comment.
디투는 천재가 분명합니다!
저도 HTTP/1.1을 이 객체가 갖고 있는 것이 이상하다고 생각했어요. 하지만 별달리 둘 곳이 없어서 어물쩡 넘어갔는데, request의 version과 맞춰준다는 것이 대단히 합리적이네요.
한편, client의 http version을 서버가 항상 맞춰줄 수는 없다고 생각해요. 가령, client가 HTTP/3 로 요청을 보냈는데, 서버가 이를 지원하지 않을 수도 있으니까요. 이럴 때는 서버가 예외를 던질지, 다른 응답을 할지 궁금해지네요.
client와 server가 http version을 어떤 과정으로 결정하는지 공부해보는 것도 재미있을 것 같아요. 기록해두었다가 나중에 공부해봐야겠어요. 키워드 알려주셔서 감사합니다~
There was a problem hiding this comment.
저도 후추 말씀 덕분에 더 고민해보게 되었는데요.
Http11Processor 객체를 사용하는 이유가 해당 버전만 받으라는 뜻 같네요.
분리되어 각각의 버전이 관리되었던 것 같아요.
그러면 http version 은 지금 객체에서 관리해야하는 것이 가장 합리적일 것 같아요.
덕분에 배워갑니다 후추!! 감사해요!
| public boolean consistsOf(HttpMethod httpMethod, String... requestUris) { | ||
| return Arrays.stream(requestUris) | ||
| .anyMatch(requestUri -> requestLine.consistsOf(httpMethod, requestUri)); | ||
| } |
There was a problem hiding this comment.
해당 메소드에서 "/login"과 "/login.html"을 모두 수용하려고 매개변수를 여럿 받을 수 있게 설정하신 것 같아요!
저는 네이버 웹사이트로 참고를 많이 하는데요. 네이버를 보면 index.html을 제외하면 파일 확장자가 들어간 url 은 자체 404를 띄워주거나 index 페이지로 리다이렉트 시키더라구요!
그래서 저 또한 잘못된 주소는 index 페이지를 보여주었는데 이 방법에 대해서는 어떻게 생각하시나요?
저는 이렇게 설정한 이유가 해킹 편리성 방지라고 생각했어요. 웹 리소스 코드를 확인하고 싶으면 불편하게 확인하라는 의미기도 하죠!
jwt나 세션 만료 시간을 채우지 못하게끔 최대한 막으려구요!
There was a problem hiding this comment.
네이버 웹사이트로 실험해보니 정말 그렇네요! 디투가 말씀해주신 내용 모두 공감이 가요. 저도 그와 같이 처리해야 할 것 같습니다!
tomcat/src/main/java/org/apache/coyote/http11/request/line/RequestLine.java
Show resolved
Hide resolved
| if (requestBody.isBlank()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
오 string이라 isBlank()를 사용해주셨군요! 깜빡하고 있었는데 배워갑니다!!!
There was a problem hiding this comment.
isEmpty, isBlank 맨날 헷갈렸는데, 이번에 "문자열은 isBlank 가 Best"
라고 외웠습니다. 부끄러운 TMI 남겨봐요ㅋㅋㅋㅋㅋ
There was a problem hiding this comment.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 아 꿀팁 감사합니다. 절대 안까먹을 것 같아요ㅋㅋㅋ 와!!
| import java.net.URL; | ||
| import java.nio.file.Files; | ||
|
|
||
| public class FileManager { |
There was a problem hiding this comment.
파일 IO 관련해서 한 객체로 관리하는 것 진짜 좋은 것 같아요!
저도 적용해보려구요.
There was a problem hiding this comment.
감사합니다! 그런데 이름이 마음에 안 드네요. 왠지 FileManager 라 하면 여러 파일을 관리하는 객체일 것 같아서요. 디투가 보기에는 이 이름으로 사용해도 괜찮을까요? 혹시 더 나은 이름이 생각나신다면 추천 부탁드립니다 ..ㅎㅎ
There was a problem hiding this comment.
저는 지금 네이밍이 정말 좋다고 생각했어요.
지금은 file read 밖에 하고 있지 않지만 file write, file close 등등 추가적인 것이 생기면 FileManager가 되는거잖아요! 다른 객체와 이름이 겹치지도 않고요.
이런 생각 때문에 더 좋은 네이밍이 생각나지 않네요ㅠㅠ
| public enum HttpContentType { | ||
| TEXT_HTML("text/html", "html"), | ||
| TEXT_CSS("text/css", "css"), | ||
| TEXT_JAVASCRIPT("text/javascript", "js"), |
There was a problem hiding this comment.
보통 오래된 웹에서는 text/javascript를 사용하곤 하는데요. 이는 예전 버전과 호환성이 좋기 때문에 사용한다고 생각해요.
하지만 지금 저희 미션에서는 application/javascript 를 사용하더라도 전혀 문제되지 않는 것 같아용.
조금 더 최신 형태인 application/javascript를 사용하는 것에 대해서는 어떻게 생각하시나요?
There was a problem hiding this comment.
application/javascript가 존재하는지 몰랐네요. 이참에 두 mime type 을 공부했습니다. 디투 말대로 application/javascript 로 하는 것이 추천되는 방식이네요! 수정하겠습니다
| private ResponseBody responseBody; | ||
|
|
||
| public HttpResponse(String httpVersion) { | ||
| this.responseLine = new ResponseLine(httpVersion, null); |
There was a problem hiding this comment.
null을 주기보다는 완성시키지 못하면 서버 에러라는 뜻으로 Internal server error는 어떻게 생각하시나용?
There was a problem hiding this comment.
null 보다 훨씬 좋습니다!
하지만 이것이 완성되지 못한 이유를 분명히 알아서 해당 status로 set하는 로직이 필요할 것 같아요. 예외를 어떻게 다뤄야할지 고민해봐야 할 것 같습니다.
일단 INTERNAL SERVER ERROR 로 두었습니다!
There was a problem hiding this comment.
아하! 네네 맞아요 완성되지 못한 이유를 안다면 setter로 status를 변경하는 것에 적극 찬성입니다.
만약 완성시키지 못한 경우는 서버 개발자의 실수라고 생각하고 일부러 internal server error를 띄운다는 뜻이었어요:)
정말 깜빡하고 status를 못 바꾸어준 것이니깐요.
| assertThat(socket.output()).containsAnyOf( | ||
| "HTTP/1.1 302 Found ", | ||
| "Location: /index.html", | ||
| "Set-Cookie: JESSIONID=" | ||
| ); |
There was a problem hiding this comment.
containsAnyOf()는 매개변수 중에 어느 하나라도 포함되면 테스트를 통과시킵니다.
그래서 contains()를 사용하는 것은 어떻게 생각하시나요?
There was a problem hiding this comment.
contains()를 왜 사용하지 않았는지 과거의 저에게 의문이 드네요!
| assertThat(socket.output()).containsAnyOf( | ||
| "HTTP/1.1 302 Found ", | ||
| "Location: /index.html" | ||
| ); |
| assertThat(fileContent).isEqualTo( | ||
| "/*!\n" | ||
| + " * Start Bootstrap - SB Admin v7.0.2 (https://startbootstrap.com/template/sb-admin)\n" | ||
| + " * Copyright 2013-2021 Start Bootstrap\n" | ||
| + " * Licensed under MIT (https://github.com/StartBootstrap/startbootstrap-sb-admin/blob/master/LICENSE)\n" | ||
| + " */\n" | ||
| + " // \n" | ||
| + "// Scripts\n" | ||
| + "// \n" | ||
| + "\n" | ||
| + "window.addEventListener('DOMContentLoaded', event => {\n" | ||
| + "\n" | ||
| + " // Toggle the side navigation\n" | ||
| + " const sidebarToggle = document.body.querySelector('#sidebarToggle');\n" | ||
| + " if (sidebarToggle) {\n" | ||
| + " // Uncomment Below to persist sidebar toggle between refreshes\n" | ||
| + " // if (localStorage.getItem('sb|sidebar-toggle') === 'true') {\n" | ||
| + " // document.body.classList.toggle('sb-sidenav-toggled');\n" | ||
| + " // }\n" | ||
| + " sidebarToggle.addEventListener('click', event => {\n" | ||
| + " event.preventDefault();\n" | ||
| + " document.body.classList.toggle('sb-sidenav-toggled');\n" | ||
| + " localStorage.setItem('sb|sidebar-toggle', document.body.classList.contains('sb-sidenav-toggled'));\n" | ||
| + " });\n" | ||
| + " }\n" | ||
| + "\n" | ||
| + "});\n"); | ||
| } |
There was a problem hiding this comment.
와 정말 꼼꼼하게 테스트하시는군요!!
물론 꼼꼼히 테스트하면 100% 확실한 테스트가 될 수 있다고 생각해요.
하지만 저는 시간상의 이유로 내용을 모두 읽어오는 테스트를 사용하지 않는 편인데요!
시간상이라고 하는 이유는 리소스 내용이 바뀌면 이 테스트 또한 바뀌어야하기 때문이에요.
사실 확인하고 싶은 것은 이 파일 존재하는지, 존재하지 않는지 이기 때문에 존재 여부만 테스트 할 것 같습니다.
어떤 서비스냐에 따라 다르겠지만, 현재 톰캣 상황에서는 잘못된 리소스 내용으로 보여준다고 하더라도 치명적인 문제가 것이라고 생각되지 않아서요.
따로 프론트 딴에서의 테스트를 믿을 것 같아용
There was a problem hiding this comment.
저도 서비스 개발이라면 위와 같은 테스트를 짜지는 않았을 것 같아요!
파일의 내용을 읽는 기능(문법)을 처음 다뤄보아서, 이러한 테스트코드로 기능을 파악하고자 했습니다.
말씀해주신 것처럼 현재 테스트코드는 리소스 내용이 바뀌면 테스트가 깨지게 됩니다. 테스트 코드가 프로덕션 코드가 아니라, 리소스의 내용에 의지하는 안 좋은 테스트입니다. 하지만 학습 측면에서는 도움이 되었어요. 굳이 지울 필요도 없다고 생각하기에 일단 두겠습니다!
There was a problem hiding this comment.
말씀해주셔서 감사합니다!
네네 학습 측면에서는 눈으로 직접 참고할 수 있기 때문에 좋은 테스트 같아요:)
|
후추! 코드가 완전 객체지향적으로 바뀌었어요!!!! |
시간이 부족해서 구현에 급급했습니다
많이 아쉬운 코드를 제출합니다 🥲