[톰캣 구현하기 1,2단계] 여우(조승현) 미션 제출합니다. #314
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
여우🦊의 코드를 리뷰할 기회를 이렇게 얻게 되네요^^
코드를 엄청 깔금하게 작성해주고, HttpRequestHandler를 만들어서 상황에 따라 클래스 분리도 잘해주셔서 코드를 읽으며 흐름 따라가기 매우 좋았고, 이해하기도 편했습니다!! 여우 코드 최고 👍👍
여우 코드를 보며도 많은 아이디어를 얻고, 공부해볼 수 있었네요!
코멘트로는 일부 놓친것같은 요구조건과, 간단한 생각 의견들 남겨보았습니다.
확인해보고 시간 여유 맞춰서 반영해볼 부분들만 고민해보고 반영해복거나 의견 주시 될 것 같습니다.
이해 안되는 부분 있으면 언제든 호출해주세요^~^
| public static HttpRequest from(Socket connection) { | ||
| return HttpRequestParser.parseFromSocket(connection); | ||
| } |
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
사용한 reader (stream)을 close하는 부분이 없는 것 같네요!
사용을 다 한 이후 수동으로 close를 해주거나 try-with-resource 로 자동으로 close될 수 있도록 해주는게 어떨까요?
There was a problem hiding this comment.
흐미 생각도 못했는데 예리한 지적이네요 필립~~~!!!!
리팩토링하면서 신기한 사실을 발견했는데,
socket.getInputStream()을 호출하는 try-with-resource문이랑
socket.getOutputStream()을 호출하는 try-with-resource문이 다르면
InputStream을 호출하는 try문이 끝나고 소켓을 닫아버리면서 문제가 생기네요!
그래서 위 두 메서드를 하나의 try-with-resource에서 처리하도록 개선하여 문제를 해결했습니다 감사합니다~
There was a problem hiding this comment.
헉ㄱ 해당 부분은 저도 확인하지 못했던 부분인데,, 여우 덕분에 저도 지식을 얻어가네요..!!
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); | ||
|
|
||
| final StringTokenizer stringTokenizer = new StringTokenizer(bufferedReader.readLine()); |
| final String[] headerEntry = header.split(": "); | ||
| requestHeaders.put(headerEntry[0], headerEntry[1]); |
There was a problem hiding this comment.
문자열 분리를 위하여 StringTokenizer와 String의 split메소드 모두 사용하였는데, 두가지중에 어떤것을 사용할지 결정하는 여우의 기준이 있는지 궁금합니다!
- 여기에서
0,1은 key와 value에 대한 인덱스로 상수로 빼두어도 좋을 것 같아요. (위에getCookies메소드에서도 분리한 상수로 사용할 수 있을 것 같습니다.)
There was a problem hiding this comment.
오 지금까지 StringTokenizer는 띄어쓰기를 기준으로만 문자열을 뽁뽁 분리해주는 줄 알았는데,
구분자를 인자로 넘겨주면 해당 구분자를 기준으로 문자열을 나누어 주는군요!
이걸 알았다면 split은 쓰지 않았을 것입니다
There was a problem hiding this comment.
ㅇㅎ!
저는 개인적으로 split도 적당히 사용하는것도 괜찮지 않을까 하는 생각도 하기는 했었어요!
찾아보니 StringTokenizer가 새로운 객체를 생성하지 않기때문에 성능면에서는 일반적으로 유리하다고는 하네요. 그러고 split 은 정규식을 사용하고 새로운 객체를 생성해야해서 성능적으로 떨어지고,, 대신 정규식을 쓰니 좀 더 유연하게 쓸 수 있다고 하고요.
추가로 저의 개인 생각은 위의 같은 경우 0, 1을 각각 HEADER_NAME_INDEX, HEADER_VALUE_INDEX와 같이 상수로 만들어서 쓴다면 tokenizer보다 가독성적인 측면은 좋은게 아닐까 하는 고민도 하고 있었어요...
그래서 이것에 대한 여우의 의견도 궁금했습니다!!
|
|
||
| final Map<String, String> headers = getHeaders(bufferedReader); | ||
| final Cookies cookies = getCookies(headers); | ||
| final StringBuilder requestBody = getRequestBody(bufferedReader); |
There was a problem hiding this comment.
body를 가져온 다음 StringBuilder의 기능을 활용해서 내용을 추가하는게 아니라면 getRequestBody가 StringBuilder가 아닌 String을 반환하는건 어떨까 하는 지극히 개인적인 생각입니다..ㅎ
There was a problem hiding this comment.
오우씻 필립 눈썰미 뭐냐고~~~~~~~~~~~~~~~~~~
StringBuilder 그대로 반환했다면 requestBody 값이 중간에 훼손될 수도 있었겠군요
고쳤습니다~!
| this.cookies.add(key, value); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
빌더 만들면서 '아 미션에 빌더 쓴 사람 내밖에 없을거다 진짜' 하고 위풍당당하게 코드 짰는데
우테코 사람들 다쓰고있었음 개슬프다 진짜
| private void returnIndexPage(OutputStream outputStream, User user) throws IOException { | ||
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); | ||
|
|
||
| final String sessionId = UUID.randomUUID().toString(); | ||
| httpResponse.addCookie("JSESSIONID", sessionId); | ||
| saveUserToSession(user, sessionId); | ||
|
|
||
| httpResponse.flush(); | ||
| } |
There was a problem hiding this comment.
미션 요구조건에
로그인에 성공하면 응답 헤더에 http status code를 302로 반환하고 /index.html로 리다이렉트 한다.
라고 나와있는데, 구현은 200에 body에 index.html 파일을 읽어서 반환해주는 것 같네요.
요구조건에 맞춰주면 좋을 것 같습니다!
There was a problem hiding this comment.
200 대신 302 작업을 하도록 변경했습니다.
근데 리다이렉트용 빌더 만드는 코드 좀 잘 짠 것 같아서 뿌듯해요 한번 봐주세요
| final Optional<String> account = httpRequest.getQueryParameter("account"); | ||
| final Optional<String> password = httpRequest.getQueryParameter("password"); |
There was a problem hiding this comment.
2단계 2번 요구사항을 보면
로그인 페이지도 버튼을 눌렀을 때 GET 방식에서 POST 방식으로 전송하도록 변경하자.
위와같이 나와있는데 시간나면 반영해봐도 좋을 것 같아요!
There was a problem hiding this comment.
허거덩.. 요구사항을 제대로 읽지 않았군요. 반영했습니다~!!
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseStatus("302") | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); |
There was a problem hiding this comment.
302의 경우에는 Location header를 통해서 redirect되는 uri를 지정해주어 브라우저에서 해당 uri로 redirect 될 수 있도록 해주어야 할 것 같습니다.
해당 경우는 login 요청을 보냈는데, 해당 요청에서 login이 아닌 index.html의 내용을 그냥 body에 담아주고 있는데, index.html에 대한 요청을 브라우저가 다시 요청할 수 있도록 해주는게 302의 의미 아닐까요..?
의미적으로도 login요청을 보냈는데, 해당 요청에 대한 응답으로 메인(index)의 응답이 오는것도 어색하다고 생각됩니다.
관련내용은 본 자료에서 링크된 rfc-문서에서 아래와 같이 SHOULD로 좀 강하게 표현해주는 것 같습니다.
The server SHOULD generate a Location header field in the response containing a URI reference for the different URI.
There was a problem hiding this comment.
자료까지 친절히 함께 담아 보여주시다니.. 🥹
위 피드백과 함께 반영하기 위해 302 응답을 보내면서
각 응답에 알맞는 response 메시지를 보낼 수 있도록 response 메시지를 만드는 작업도 별도의 utils 클래스로 분리해 보았어요!
이 과정에서 private -> protected로 접근자 수정이 많이 일어났는데, 요게 괜찮은 형태인지 한 번 봐주시면 감사하겠습니다!
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseStatus("302") | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); |
There was a problem hiding this comment.
여기도 302인데 redirect uri를 넘겨주는게 아닌 body로 index.html파일 내용을 보내주고 있네요
…html로 302 리다이렉트하도록 변경
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
흐미 생각도 못했는데 예리한 지적이네요 필립~~~!!!!
리팩토링하면서 신기한 사실을 발견했는데,
socket.getInputStream()을 호출하는 try-with-resource문이랑
socket.getOutputStream()을 호출하는 try-with-resource문이 다르면
InputStream을 호출하는 try문이 끝나고 소켓을 닫아버리면서 문제가 생기네요!
그래서 위 두 메서드를 하나의 try-with-resource에서 처리하도록 개선하여 문제를 해결했습니다 감사합니다~
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); | ||
|
|
||
| final StringTokenizer stringTokenizer = new StringTokenizer(bufferedReader.readLine()); |
|
|
||
| final Map<String, String> headers = getHeaders(bufferedReader); | ||
| final Cookies cookies = getCookies(headers); | ||
| final StringBuilder requestBody = getRequestBody(bufferedReader); |
There was a problem hiding this comment.
오우씻 필립 눈썰미 뭐냐고~~~~~~~~~~~~~~~~~~
StringBuilder 그대로 반환했다면 requestBody 값이 중간에 훼손될 수도 있었겠군요
고쳤습니다~!
| final String[] headerEntry = header.split(": "); | ||
| requestHeaders.put(headerEntry[0], headerEntry[1]); |
There was a problem hiding this comment.
오 지금까지 StringTokenizer는 띄어쓰기를 기준으로만 문자열을 뽁뽁 분리해주는 줄 알았는데,
구분자를 인자로 넘겨주면 해당 구분자를 기준으로 문자열을 나누어 주는군요!
이걸 알았다면 split은 쓰지 않았을 것입니다
| this.cookies.add(key, value); | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
빌더 만들면서 '아 미션에 빌더 쓴 사람 내밖에 없을거다 진짜' 하고 위풍당당하게 코드 짰는데
우테코 사람들 다쓰고있었음 개슬프다 진짜
| final Optional<String> account = httpRequest.getQueryParameter("account"); | ||
| final Optional<String> password = httpRequest.getQueryParameter("password"); |
There was a problem hiding this comment.
허거덩.. 요구사항을 제대로 읽지 않았군요. 반영했습니다~!!
| private void returnIndexPage(OutputStream outputStream, User user) throws IOException { | ||
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); | ||
|
|
||
| final String sessionId = UUID.randomUUID().toString(); | ||
| httpResponse.addCookie("JSESSIONID", sessionId); | ||
| saveUserToSession(user, sessionId); | ||
|
|
||
| httpResponse.flush(); | ||
| } |
There was a problem hiding this comment.
200 대신 302 작업을 하도록 변경했습니다.
근데 리다이렉트용 빌더 만드는 코드 좀 잘 짠 것 같아서 뿌듯해요 한번 봐주세요
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseStatus("302") | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); |
There was a problem hiding this comment.
자료까지 친절히 함께 담아 보여주시다니.. 🥹
위 피드백과 함께 반영하기 위해 302 응답을 보내면서
각 응답에 알맞는 response 메시지를 보낼 수 있도록 response 메시지를 만드는 작업도 별도의 utils 클래스로 분리해 보았어요!
이 과정에서 private -> protected로 접근자 수정이 많이 일어났는데, 요게 괜찮은 형태인지 한 번 봐주시면 감사하겠습니다!
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseStatus("302") | ||
| .responseBody(new FileHandler().readFromResourcePath("static/index.html")) | ||
| .build(outputStream); |
| response.append("Content-Type: ") | ||
| .append(this.contentType) | ||
| .append(";charset=") | ||
| .append(this.charSet) | ||
| .append(" ") | ||
| .append(System.lineSeparator()); | ||
| for (Cookie cookie : cookies) { | ||
| response.append("Set-Cookie: ") | ||
| .append(cookie.getKey()) | ||
| .append("=") | ||
| .append(cookie.getValue()) | ||
| .append(" ") | ||
| .append(System.lineSeparator()); | ||
| } | ||
| response.append("Content-Length: ") | ||
| .append(this.contentLength) | ||
| .append(" ") | ||
| .append(System.lineSeparator()); |
There was a problem hiding this comment.
헉쓰 .. 일리있는 말인 것 같습니다!
응답 메시지에 어떤 정보를 넣어야 하는지에 따라 응답 메시지의 구성을 동적으로 변경할 수 있도록
HttpResponseParser를 만들어 사용하도록 개선했습니다!
302의 경우에는 더 적은 정보를 필요로 하므로
HttpResponseParser를 상속한 HttpRedirectResponseParser를 만들었어요!
근데 이 두 클래스에서 제공하는 메서드가 정적 메서드이다보니 유지보수성 면에서 약간 후퇴한 건 아닌가 하는 걱정이 드는데, 한 번 확인해주시면 감사하겠습니다!
There was a problem hiding this comment.
🦊여우🦊!! 코멘트 늦어서 미안해요ㅠㅜ
여우가 처음부터 코드 깔끔하게 짜줘서 이번에 수정된 부분들도 쉽게 따라가면서 확인할 수 있었어요!!
Redirect를 위한 response와 responseParser가 만들어진 부분과, 구현등에 대한 제 의견은 여기에 한번에 남겨볼께요.
이번에 헤더가 달라질 수 있는 경우에 대해서 redirect가 있어서 이 경우를 예시로 들었는데, 들어갈 헤더의 종류는 경우에 따라 언제든지 다양하게 변할 수 있을 가능성이 조금은 있다고 생각해요.. 지금같은 구조라면 response로 나가게 될 헤더의 조합이 다양해질때마다 매번 하위클래스들이 생성이 되고 여우가 걱정한데로 구조가 복잡해져서 유지 보수가 힘들어질 수 있다고 생각해요...
이런 구조로 설계가 된 이유가 제 생각에는 여우가 각각의 헤더들(Location, Content-Type, Content-Lenth 등)을 각각의 필드로 가지고 있기 때문에 복잡해진 것 같아요.
만약에 Header값들을 하나로 관리하는 Header (또는 Headers)클래스를 만들고 해당 헤더객체에 response에서 내보내 줄 헤더값들만 추가를 해주었다면 필요한 헤더들만 쉽게 응답에 출력해 줄 수 있지 않았을까 해요..! 이러면 경우에 따라서 클래스가 분리될 필욛도 없고요!!
예시로는 여우가 만든 Cookies, Cookie처럼 해당 값이 있는 경우에만 내보내주는 식으로 했으면 어땟을까 합니다!!
여우가 구현한 쿠키처럼 key, value를 가지는 header를 만들어도 좋고, 아니면 Map을 통해서 구현을 하던가 하는식으로 내보내줄 해더들만 가지고 있도록이요!
혹은 이 방법이 아니더라도 더 좋은 방법도 한번 생각이 나게 된다면 3단계를 진행하며 리팩터링 해보셔도 좋을 것 같아요!
다른 내용들도 코멘트로 간단히 남겨두어서 확인해보면 될 것 같아요^^
고생했어요~~ 궁금한점이나 같이 이야기해보고싶은것들 있음 언제든 호출해주셔�여 😄
| private void returnUnauthorizedPage(OutputStream outputStream) throws IOException { | ||
| final HttpResponse httpResponse = new HttpResponse.Builder() | ||
| .responseBody(new FileHandler().readFromResourcePath("static/401.html")) | ||
| .responseStatus("401") | ||
| .build(outputStream); | ||
| httpResponse.flush(); | ||
| } |
There was a problem hiding this comment.
이거는 저도 궁금하고 고민이여서 여우는 어떠한 생각으로 이러한 방식으로 구현을 했는지 궁금해서 코멘트 하나 남겨요..!!
로그인이 실패했을때 처리할 수 있는 방법이 2가지가 생각이 났는데 각각 아래와 같을 것 같아요.
- 302 redirect로 401.html을 요청하게 한다.
- 401에러와 함께 401페이지 response를 보여준다.
사용자가 느끼기에는 일단 아래와 같은 차이가 있을 것 같아요.
1번의 경우에는 로그인이 실패하면 인터넷의 주소창 자체가 401로 바뀌며 페이지가 이동된다.
2번의 경우 login이라는 주소(url)에 남아있으면서 401화면이 보이게 된다.
혹시 여우가 2번의 경우처럼 해당 페이지에서 유지를 하면서 401페이지가 보이는것이 더 좋다고 생각한 이유가 있을까요??
전 무엇이 일반적이다 또는 좋을것 같다라는 생각이 잘 안들어서요ㅠㅜ 그냥 만들기 나름인건가 해서요ㅠㅜㅜㅜㅋㅋㅋㅋ
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); | ||
|
|
||
| final StringTokenizer stringTokenizer = new StringTokenizer(bufferedReader.readLine()); |
| final var inputStream = socket.getInputStream(); | ||
| final var bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
헉ㄱ 해당 부분은 저도 확인하지 못했던 부분인데,, 여우 덕분에 저도 지식을 얻어가네요..!!
| final String[] headerEntry = header.split(": "); | ||
| requestHeaders.put(headerEntry[0], headerEntry[1]); |
There was a problem hiding this comment.
ㅇㅎ!
저는 개인적으로 split도 적당히 사용하는것도 괜찮지 않을까 하는 생각도 하기는 했었어요!
찾아보니 StringTokenizer가 새로운 객체를 생성하지 않기때문에 성능면에서는 일반적으로 유리하다고는 하네요. 그러고 split 은 정규식을 사용하고 새로운 객체를 생성해야해서 성능적으로 떨어지고,, 대신 정규식을 쓰니 좀 더 유연하게 쓸 수 있다고 하고요.
추가로 저의 개인 생각은 위의 같은 경우 0, 1을 각각 HEADER_NAME_INDEX, HEADER_VALUE_INDEX와 같이 상수로 만들어서 쓴다면 tokenizer보다 가독성적인 측면은 좋은게 아닐까 하는 고민도 하고 있었어요...
그래서 이것에 대한 여우의 의견도 궁금했습니다!!
3단계 요구사항을 침범하지 않기 위해
리팩토링을 하지 않았읍니다.
이해되지 않는 것이 있으시다면
곧바로 달려가 보충설명을 드리겠습니다! 편하게 호출해 주십시오