-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[톰캣 구현하기 - 1, 2단계] 성하(김성훈) 미션 제출합니다. #305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 성하!
4렙 첫번째 미션 고생하셨습니다.
이번 1,2단계에서 역할 분리를 해주셔서 코드 읽기가 편했던거같아요!
리뷰한번 확이해주시고 다시 리뷰요청 해주시면 될거같아요!
- [x] GET /index.html 요청이 들어오면 index.html 파일을 응답한다. | ||
|
||
- [x] CSS, JS를 지원한다. | ||
- [x] Response Content-Type : text/css로 응답 | ||
- [x] Response Content-Type : text/javascript로 응답 | ||
|
||
- [x] Query String을 파싱한다. | ||
|
||
- [x] 로그인 여부에 따라 다른 페이지로 리다이렉트한다. | ||
- [x] 로그인에 성공하면 `index.html`로 리다이렉트한다. | ||
- [x] 로그인에 실패하면 `401.html`로 리다이렉트한다. | ||
|
||
- [x] POST 방식으로 회원가입을 할 수 있다. | ||
- [x] `/register` 경로로 접근하면 `register.html`을 보여준다. | ||
- [x] 회원가입 버튼을 누르면 POST로 회원 가입 성공 후, `index.html`로 리다이렉트 한다. | ||
|
||
- [x] 로그인 페이지에서도 로그인 버튼을 누르면 POST로 전송하도록 변경한다. | ||
|
||
- [x] 쿠키와 세션을 활용해서 로그인을 유지한다. | ||
- [x] 요청 헤더, 응답 헤더에 쿠키 설정을 추가한다. | ||
- [x] 쿠키의 세션 값으로 로그인 여부를 체크한다. | ||
- [x] 로그인에 성공하면 세션 객체의 값으로 User를 저장한다. | ||
- [x] 로그인된 상태로 로그인 페이지에 접근하면 index.html 페이지로 리다이렉트한다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이야... 저는 시간이 없어서 못했는데 꼼꼼성하..
"Content-Length: " + responseBody.getBytes().length + " ", | ||
"", | ||
responseBody); | ||
RequestHandler handler = HandlerAdapter.findRequestHandler(httpRequest.getRequestUri()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
성하표 디스패처써플릿
String responseBody = FileReader.read("/login.html"); | ||
MessageBody messageBody = MessageBody.from(responseBody); | ||
|
||
ResponseHeaders responseHeaders = new ResponseHeaders(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드에서 초기화 시켜주는게 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
핸들러가 ResponseHeader라는 객체를 필드로 가지는 건 Handler의 책임이 아닌 것 같다고 생각합니다!
그래서 처리하는 로직인 handle 안에서만 사용되도록 지역 변수로 하는게 좋다고 생각해서
변경하지 않았습니다!! 🙇🏻♂️
} | ||
|
||
private static MessageBody createRequestBody(BufferedReader br, RequestHeaders headers) | ||
throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찾았다!
개행이 들어간거같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자동 정렬?이 저렇게 한 것 같네요 ㅠㅠ 수정했습니다!
Map<String, String> headerMap = br.lines() | ||
.takeWhile(line -> !line.equals("")) | ||
.map(line -> line.split(": ")) | ||
.collect(Collectors.toMap(line -> line[0], line -> line[1])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가독성 좋게 key,value로 하는건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DM으로 얘기나눈 대로 리팩토링 했습니다!
private static final String HTML_URI = ".html"; | ||
private static final String CSS_URI = ".css"; | ||
private static final String JAVASCRIPT_URI = ".js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 enum 잘사용해 주셨는데 이것도 eum으로 정의해주면 좋을거같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 상수들은 연관성이 존재해서 ENUM으로 해도 될 것 같지만,
현재 사용되는 곳이 RequestUri에서 해당 URI로 끝나는지 판단하는 용도로만 사용하고 있습니다!
ENUM으로 정의하게 되면 여러 곳에서 사용될 수 있기 때문에 불필요한 관리 범위가 넓어진다고 생각합니다!
또, 해당 상수들로 사용하는 비즈니스 로직도 딱히 존재하지 않습니다!
그래서 이 부분은 RequestUri가 사용하는 상수로 남겨놓겠습니다!! 🙇🏻♂️
그래도 리뷰 감사합니다! ㅎㅎㅎ 한번 더 생각해볼 수 있었어요!
public enum Status { | ||
|
||
OK(200, "OK"), | ||
FOUND(302, "Found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항에 있는 401도 정의해줘서 사용하면 좋을거같아용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 요구사항 중에 '로그인이 실패하면 401.html로 리다이렉트한다' 라는 요구사항이 존재하는데요!
저도 처음에는 401.html로 리다이렉트를 하고, 401 응답코드를 주려고 정의헀었습니다!
그런데 요구사항을 만족하려면 리다이렉트를 해야하고, 리다이렉트를 하기 위해서는
401 응답코드가 아니라 302 응답코드를 응답해야했습니다!
그래서 결과적으로 401은 정의해도 안 쓰게 되더라구요! 그래서 제거하게 되었습니다!
그래서 나중에 401 응답코드를 내려줄 일이 필요하면 만들도록 하겠습니다!!
private static final String CHARSET_UTF_8 = ";charset=utf-8"; | ||
private final RequestLine requestLine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨벤션!! 컨벤션!! 컨벤션!! 컨벤션!! 컨벤션!! 컨벤션!!
|
||
private static final String SPLIT_DELIMITER = " "; | ||
private final HttpMethod httpMethod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤨
@Test | ||
@DisplayName("Http Response Header를 Http Response용으로 가공해서 반환한다.") | ||
void getHeadersForResponse() { | ||
// given | ||
ResponseHeaders responseHeaders = new ResponseHeaders(); | ||
responseHeaders.addHeader(HttpHeaderName.CONTENT_TYPE.getValue(), ContentType.TEXT_HTML.getValue()); | ||
responseHeaders.addHeader(HttpHeaderName.CONTENT_LENGTH.getValue(), "124"); | ||
|
||
String expected = String.join("\r\n", | ||
HttpHeaderName.CONTENT_TYPE.getValue() + ": " + ContentType.TEXT_HTML.getValue() + " ", | ||
HttpHeaderName.CONTENT_LENGTH.getValue() + ": " + 124 + " " | ||
); | ||
|
||
// when | ||
String headersForResponse = responseHeaders.getHeadersForResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꼼꼼한 test 😀
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요 비버!! 리뷰 감사합니다 ㅎㅎ 제안 주신 내용 중 반영하지 않은 내용도 있는데요! 제 생각과 다르게 생각하신다면 재코멘트 달아주세요!! 😃 🙇🏻♂️ |
안녕하세요! 비버~ 😃 성하입니다!
1, 2단계 구현하다보니 HttpRequest, HttpResponse가 없으면
너무 하나 클래스가 커져 버려서 클래스를 만들어서 분리했는데 알고보니 3단계더라구요..? 미리 했습니다..
(그래도 3단계때 리팩토링할 건 많을 것 같아요 ㅎㅎ;;)
전체적인 흐름
전체적인 흐름은
Http11Processor
의process()
를 보면 될 것 같아요!해당 흐름으로 구현했습니다!!
로그인 처리 로직이 복잡해서
담당하는
LoginHandler
가 너무 커진 것 같긴 한데 일단 제출합니다 😂