[HTTP 서버 구현하기 - 2, 3단계] 검프(김태정) 미션 제출합니다. #79
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjpark-dev
suggested changes
Sep 2, 2021
app/build.gradle
Outdated
Comment on lines
55
to
56
| minimum = 0.90 | ||
| } |
There was a problem hiding this comment.
피드백을 통해 95%까지 올릴 수 있었습니다! 감사합니다 😊
Comment on lines
34
to
39
| if (Objects.isNull(controller)) { | ||
| httpResponse.setHttpStatusCode(HttpStatusCode.NOT_FOUND); | ||
| errorResolver(httpRequest, httpResponse); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
이런 경우를 위해서 디폴트 컨트롤러를 만들어서 활용하는 것은 어떨까요?
There was a problem hiding this comment.
|
|
||
| private static final String HEADER_DELIMITER = ": "; | ||
|
|
||
| private final BufferedReader bufferedReader; |
There was a problem hiding this comment.
bufferedReader는 초기화 할 때만 쓰이는데 필드로 가지고 있을 필요가 있을까요?
| this.requestLine = requestLine; | ||
| this.httpHeaders = httpHeaders; | ||
| this.parameters = parameters; | ||
| this.httpSession = HttpSessions.getSession(httpHeaders.getSessionId()); |
There was a problem hiding this comment.
httpSession이 null이 될 수 있네요.
이 경우 getHttpSession 메소드를 거쳐야 비로소 httpSession이 초기화 되네요.
null보다는 default 값을 활용하는 것은 어떨까요?
There was a problem hiding this comment.
확실히 null에 열러있다보니 위험해 보였던거 같아요!
좋은 피드백 감사합니다 ㅎㅎㅎ
|
|
||
| import java.util.Optional; | ||
|
|
||
| public class UserService { |
There was a problem hiding this comment.
login과 register를 동시에 처리하는 UserService 좋네요. 👍
저는 왜 이 생각을 못했을까요? 😂
Comment on lines
+32
to
39
| final HttpRequest httpRequest = new HttpRequest(new HttpMessageReader(inputStream)); | ||
| final HttpResponse httpResponse = new HttpResponse(httpRequest); | ||
|
|
||
| if ("/".equals(extractedUri)) { | ||
| final String response = http200Message(DEFAULT_RESPONSE_BODY, ContentType.HTML.getContentType()); | ||
| writeOutputStream(outputStream, response); | ||
| return; | ||
| } | ||
|
|
||
| if (extractedUri.endsWith(".html")) { | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri, ContentType.HTML)); | ||
| return; | ||
| } | ||
|
|
||
| if (extractedUri.endsWith(".css")) { | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri, ContentType.CSS)); | ||
| return; | ||
| } | ||
|
|
||
| if (extractedUri.endsWith(".js")) { | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri, ContentType.JAVASCRIPT)); | ||
| return; | ||
| } | ||
|
|
||
| if (extractedUri.startsWith("/assets/img")) { | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri, ContentType.IMAGE)); | ||
| return; | ||
| } | ||
|
|
||
| if ("/login".equals(extractedUri)) { | ||
| if ("POST".equals(extractedMethod)) { | ||
| try { | ||
| final String requestBody = extractRequestBody(bufferedReader, httpRequestHeaders); | ||
| loginRequest(requestBody); | ||
| writeOutputStream(outputStream, http302Response("/index.html")); | ||
| } catch (RuntimeException exception) { | ||
| writeOutputStream(outputStream, http302Response("/401.html")); | ||
| } | ||
| return; | ||
| } | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri + ".html", ContentType.HTML)); | ||
| return; | ||
| } | ||
|
|
||
| if ("/register".equals(extractedUri)) { | ||
| if ("POST".equals(extractedMethod)) { | ||
| try { | ||
| final String requestBody = extractRequestBody(bufferedReader, httpRequestHeaders); | ||
| registerRequest(requestBody); | ||
| writeOutputStream(outputStream, http302Response("/index.html")); | ||
| } catch (RuntimeException exception) { | ||
| writeOutputStream(outputStream, http302Response("/401.html")); | ||
| } | ||
| return; | ||
| } | ||
| writeOutputStream(outputStream, httpStaticFileResponse(STATIC_PATH, extractedUri + ".html", ContentType.HTML)); | ||
| return; | ||
| } | ||
|
|
||
| writeOutputStream(outputStream, http302Response("/404.html")); | ||
| final FrontController frontController = new FrontController(); | ||
| frontController.service(httpRequest, httpResponse); | ||
| outputStream.write(httpResponse.getHttpMessage().getBytes()); | ||
| outputStream.flush(); | ||
| } catch (IOException exception) { |
Comment on lines
+12
to
+13
| public class HttpMessageReaderGetStub extends HttpMessageReader { | ||
|
|
| //then | ||
| Assertions.assertThat(httpCookie.toValuesString()).isEqualTo("tasty_cookie=strawberry; JSESSIONID=656cef62-e3c4-40bc-a8df-94732920ed46"); | ||
| } | ||
| } |
There was a problem hiding this comment.
테스트 클래스 전반적으로 EOF가 없는 경우가 많습니다.
EOF가 있으면 좋은 이유
There was a problem hiding this comment.
자동 완성에 의존하다보니, 개행을 신경쓰지 못했네요
꼼꼼한 피드백 감사합니다 :)
|
안녕하세요 에드!! |
sjpark-dev
suggested changes
Sep 2, 2021
|
|
||
| public void service(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| final String requestPath = httpRequest.getPath(); | ||
| final Controller controller = controllerMap.getOrDefault(requestPath, new RedirectController()); |
Comment on lines
+27
to
+37
| public HttpRequest(RequestLine requestLine, HttpHeaders httpHeaders, Parameters parameters) { | ||
| this(requestLine, httpHeaders, parameters, HttpSessions.getSession(httpHeaders.getSessionId())); | ||
| } | ||
|
|
||
| public HttpRequest(RequestLine requestLine, HttpHeaders httpHeaders, Parameters parameters, HttpSession httpSession) { | ||
| this.requestLine = requestLine; | ||
| this.httpHeaders = httpHeaders; | ||
| this.parameters = parameters; | ||
| this.httpSession = initializeSession(httpSession); | ||
| } | ||
|
|
There was a problem hiding this comment.
세션이 없다면 만드는 로직을 이렇게 깔끔하게 구현 할 수가 있네요 👍👍👍
| @@ -89,7 +96,24 @@ void mainPage() { | |||
| } | |||
|
|
|||
| @Test | |||
| void registerRequest() { | |||
| void 유저_로그인_테스트(){ | |||
There was a problem hiding this comment.
RequestHandlerTest에서 이 테스트 메소드를 제외하고는 모두 실패합니다. 😢
나머지도 여기처럼 expected에 Set-Cookie 헤더를 넣으면 될 것 같아요!
There was a problem hiding this comment.
리스폰스 부분에서 생긴 예외였어요!!
구현 로직 수정 후 예외 해결 완료했습니다 😊
Comment on lines
10
to
14
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
|
구현 로직 수정 및 코드 스멜 해결 완료햇습니다!! 감사합니다 🙇🏻♂️ |
|
Kudos, SonarCloud Quality Gate passed! |
sjpark-dev
approved these changes
Sep 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 에드!
두번째 PR이네요 ㅎㅎ
고민한부분
Http요청에대해 어떻게 처리 할 것인지 오래동안 고민했어요.
FrontController를 통해 FrontController가 전반적인 요청과 응답에 관여하게 했어요.
많은 구조 변경으로 길을 잃을뻔 했지만, 겨우 돌아왔어요 ㅎㅎ
HttpSession의 생명주기
HttpRequest와 HttpResponse에서 HttpSession이 어디까지 관여를 해야할까 고민했어요.
처음엔 HttpRequest에만 뒀다가 이후에 HttpResponse에 HttpRequest 의존관계를 형성함으로써 요청과 응답에 생명을 가지게 했어요.
이번 미션에 테스트를 작성하는 부분이 정말 힘들었어요. 어떻게든 작성해보며, 어떤 테스트를 해야하고 어떤 테스트를 하지 말아야 하는지 나름의 이류를 찾았어요. (최소 구현사항에서 사용되는 로직을 테스트 하는 것을 우선시한다. )
이번에도 잘 부탁드립니다!