[톰캣 구현하기 - 1단계] 기론(김규철) 미션 제출합니다.#174
Conversation
There was a problem hiding this comment.
안녕하세요 기론✋🏻✋🏻
1단계 요구사항 잘 해주셨네요. 고생 많으셨습니다👍🏻
코멘트는 대체적으로 개인 취향적인 거라 반영 안하시고 참고만 하셔도 좋을 것 같아요. 자기만의 스타일로 다음 단계 쪽쪽 진행주시면 좋을 것 같습니다. 1단계 고생많으셨습니다!!
2, 3, 4단계 다 하려면 시간이 부족할 수도? 있는데 마지막까지 홧팅입니다.
ps. 아 근데 Http11Processor.index() 테스트 코드가 통과가 안되네요.(별 문제는 아닌 것 같아서 2단계 진행하시면서 고쳐보면 좋을 듯)
| package org.apache.coyote.http11; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum Extension { | ||
| HTML("html", "text/html"), | ||
| CSS("css", "text/css"), | ||
| JS("js", "text/js"), | ||
| CSV("svg", "image/svg+xml"), | ||
| ICO("ico", "image/x-icon"), | ||
| ; | ||
| private final String name; | ||
| private final String contentType; | ||
|
|
||
| Extension(String name, String contentType) { | ||
| this.name = name; | ||
| this.contentType = contentType; | ||
| } | ||
|
|
||
| public static String convertToContentType(String url) { | ||
| int index = url.indexOf("."); | ||
| if (index == -1) { | ||
| return HTML.contentType; | ||
| } | ||
| Extension extension1 = Arrays.stream(values()) | ||
| .filter(extension -> url.endsWith(extension.name)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("알맞은 확장자가 없습니다.")); | ||
| return extension1.contentType; | ||
| } | ||
| } |
There was a problem hiding this comment.
오 Extension enum 멋있네요! 근데 클래스 네이밍을 ContentType으로 바꾸는게 좀 더 가독에 좋을 것 같기도??
추가로 convertToContentType()메서드에서 extension1이라는 변수명 바꾸면 더 좋을 것 같습니다👍🏻
There was a problem hiding this comment.
넵 Extension을 보고 ContentType으로 바꿔주는 클래스였는데 가독성이 오히려 떨어졌군요..! ContentType으로 클래스 네이밍 수정했습니다!
| InputStreamReader inputStreamReader = new InputStreamReader(inputStream); | ||
| BufferedReader bufferedReader = new BufferedReader(inputStreamReader)) { |
There was a problem hiding this comment.
놓칠 수 있는 부분인데 Reader도 try with resources로 처리해준 것 좋네요👍🏻
| } | ||
|
|
||
| private String getResponseBody(final String uri) throws IOException, URISyntaxException { | ||
|
|
| Url url = HandlerMapping.from(uri); | ||
| URL resource = this.getClass() | ||
| .getClassLoader() | ||
| .getResource(STATIC_DIRECTORY + url.getRequest().getPath()); |
There was a problem hiding this comment.
오 아주 적절한 추상화인 것 같네요! 한 수 배워갑니다.👍🏻
다만 완전 취향 차이인 것 같은데 저 같으면 url.getRequest().getPath()를 Processor에서 url.getPath()하나로 보여줬을 것 같네요.
There was a problem hiding this comment.
앗 해당 부분이 getRequest내부에서 path경로를 수정한 후, path를 반환하는 것이라 하나로 합치기에 어렵더라구요.. 아예 http11Request에서 분리를 해야할 것 같은데 2단계 수정하면서 반영해보겠습니다!
|
|
||
| private void validatePath(URL resource) { | ||
| if (Objects.isNull(resource)) { | ||
| throw new IllegalArgumentException("경로가 잘못 되었습니다. : null"); |
There was a problem hiding this comment.
resources 파일 보니까 HTTP 400, 500대 상태코드에 대한 .html파일이 있더라고요.(저도 적용은 안함ㅎ)
다음 단계 쭉쭉 진행해보면서 적용해 보면 좋을 것 같습니다👍🏻👍🏻
| package org.apache.coyote.http11.dto; | ||
|
|
||
| public class Http11Request { | ||
|
|
||
| private final String account; | ||
| private final String password; | ||
| private final String path; | ||
|
|
||
| public Http11Request(String path) { | ||
| this(null, null, path); | ||
| } | ||
|
|
||
| public Http11Request(String account, String password, String path) { | ||
| this.account = account; | ||
| this.password = password; | ||
| this.path = path; | ||
| } | ||
|
|
||
| public String getAccount() { | ||
| return account; | ||
| } | ||
|
|
||
| public String getPath() { | ||
| return path; | ||
| } | ||
| } |
There was a problem hiding this comment.
위 클래스에서 accout, password 변수는 사실 상 Login할 때 queryString 처리 용으로만 쓰고 있네요.
path는 계속 사용하니까 놔두고 다른 쿼리 처리를 위해 account, password 같은 쿼리 파라미터를 위한 변수들은 Map을 사용한 일급 컬렉션으로 대체해도 좋을 것 같기도??
|
|
||
| import org.apache.coyote.http11.dto.Http11Request; | ||
|
|
||
| public class StringParser { |
There was a problem hiding this comment.
클래스 책임을 보니까 StringParser라는 네이밍이 뭔가 추상적인 것 같기도 하네요🧐
There was a problem hiding this comment.
음.. 네이밍은 정말 어렵네요ㅎㅎ ㅠㅠ UrlParser어떤가요?! 확실히 String Parser보단 나은 것 같기도...
| @DisplayName("css파일을 읽으면 contentType이 text/css로 받는다.") | ||
| @Test | ||
| void css() { | ||
| // given | ||
| final String httpRequest = String.join("\r\n", | ||
| "GET /css/styles.css HTTP/1.1 ", | ||
| "Host: localhost:8080 ", | ||
| "Accept: text/css,*/*;q=0.1 ", | ||
| "Connection: keep-alive ", | ||
| "", | ||
| ""); | ||
|
|
||
| final var socket = new StubSocket(httpRequest); | ||
| final Http11Processor processor = new Http11Processor(socket); | ||
|
|
||
| // when | ||
| processor.process(socket); | ||
|
|
||
| // then | ||
| assertThat(socket.output().contains("text/css")).isTrue(); | ||
| } | ||
|
|
||
| @DisplayName("query string으로 들어온 파일을 html파일로 읽는다.") | ||
| @Test | ||
| void queryString() { | ||
| // given | ||
| final String httpRequest = String.join("\r\n", | ||
| "GET /login?account=gugu&password=password HTTP/1.1 ", | ||
| "Host: localhost:8080 ", | ||
| "Connection: keep-alive ", | ||
| "", | ||
| ""); | ||
|
|
| if (uri.startsWith("login")) { | ||
| return new Login(uri); | ||
| } |
There was a problem hiding this comment.
현재 localhost:8080/index.html로 접속 후, 우측 상단의 로그인 버튼을 누르면 localhost:8080/loginuri로 요청이 들어오게 되는데(사실 확장자가 없는 요청이라서 url이 이상한 것ㄱ 같기도?), 저렇게 들어오게 되면 new Login()이 생성되고 StringParser에서 파싱하는 과정에서 아래와 같은 에러가 뜨게 되네요! 쿼리 스트링이 있는 경우에만 동작하는 로직이라 그런 것 같습니다.
There was a problem hiding this comment.
앗! 그렇네요 체크리스트만 신경쓰다가 해당 부분을 신경 못썼군요.. 수정했습니다!
| User user = InMemoryUserRepository.findByAccount(http11Request.getAccount()) | ||
| .orElseThrow(() -> new IllegalArgumentException("아이디 또는 비밀번호를 잘못 입력하였습니다.")); |
There was a problem hiding this comment.
현재 이 로직이면 account는 올바른데 password가 틀린 경우에도 정상 동작하는 것으로 보입니다.
User class의 checkPassword()메서드를 이용해 개션해 보면 좋을 것 같습니다👍🏻
ex)http://localhost:8080/login?account=gugu&password=잘못된 패스워드인 경우 -> 정상 동작
There was a problem hiding this comment.
해당 부분이 다음 단계 적용으로 알고있어서 적용하다가 뺐었습니다! 다음 단계에서 적용해볼게요~!
2 participants
안녕하세요 잉지노!
다형성을 최대한 활용해보려고 했는데 아직도 쉽지가 않네요..ㅎㅎ
코드 보시다가 이해 안 가는 부분있으면 언제든 말씀주세요~
감사합니다~.~👍