[톰캣 구현하기 1, 2단계] 지토(김지민) 미션 제출합니다 #356
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
안녕하세요 지토! 코코닥입니다 ㅎㅎ
구현의 완성도도 높은데 테스트도 많이 작성해주셔서 놀랐습니다. 역시 지토네요🥸
지저분한 코드를 작성해주셨다했는데, 저는 잘 모르겠습니다 ㅎㅎ..
전체적으로 가독성이 너무 좋았고, 재밌는 구조를 지니고 있어서 코드 리뷰하면서 즐거웠어요. 1, 2단계에 대한 기능도 충실히 구현해주신 것 같구요. 덕분에 공부도 되고 좋았습니다.
다만, 제가 4시간 뒤면 예비군을 가야해서.. 추가적인 핑퐁은 불가능할 것 같아요 🥲
궁금했던 점들이나 개선하면 좋을 것 같은 점들을 리뷰로 남겨놨는데, 머지가 되어도 한번 봐주시면 감사하겠습니다.
일단 지토의 미션에 방해가 안되도록 바로 Merge 하겠습니다. 다음 단계때는 더 좋은 리뷰 남기도록 노력할게요. 고생하셨습니다 지토~!
| } | ||
|
|
||
| public HelloWorldContext(final String rootContextPath, final String staticResourcePath) { | ||
| if (rootContextPath == null || rootContextPath.isEmpty() || rootContextPath.isBlank()) { |
There was a problem hiding this comment.
null 여부를 먼저 확인해서 뒤의 코드에서는 NPE를 방지할 수 있겠네요. 좋습니다!
다만 isEmpty() 와 isBlank() 를 동시에 검증하는 것이 의미가 있을까요?
또한 staticResourcePath 의 값도 같이 검증해주면 어떨까요?
There was a problem hiding this comment.
rootContext는 empty나 null string을 허용해서는 안 된다고 생각해서 검증을 진행했었는데, 생각해보니 필요한 검증을 전혀 수행하고 있지 못하고 있었네요...수정하겠습니다! 감사합니다!
staticResourcePath도 외부에서 설정할 수도 있는 만큼 말씀해주신대로 검증을 추가하도록 하겠습니다!
| private void initHandlers() { | ||
| handlers.add(new WelcomeHandler(rootContextPath)); | ||
| handlers.add(new LoginHandler("/login", rootContextPath)); | ||
| handlers.add(new LoginPageHandler("/login", rootContextPath, "login.html", staticResourcePath)); | ||
| handlers.add(new RegisterPageHandler("/register", rootContextPath, "register.html", staticResourcePath)); | ||
| handlers.add(new RegisterHandler("/register", rootContextPath)); | ||
| } |
There was a problem hiding this comment.
Handler들을 만들어주셨군요. 고수의 냄새가 납니다 ㅎㅎ..
한가지 궁금한 점은, handlers의 자료구조로 Map과 List가 후보에 있었을텐데, Map<String, List<Handler>> 와 같은 방식을 사용하지 않고 List를 사용하신 이유는 무엇일까요?
There was a problem hiding this comment.
Map의 경우 Handler 외부에서 어떤 path가 어떤 handler인지를 매핑해야 한다는 문제가 있었습니다
제 코드 상으로는 HelloWorldContext에서 Map을 사용했다면 LoginHandler는 /login과 매핑된다는 것을 명시해줘야한다고 생각했습니다
List를 사용한다면 supports() 메서드를 활용한다면 HelloWorldContext에서는 어떤 핸들러가 어떤 경로와 매핑되는지 알 필요가 없다고 생각해 이와 같이 작성했습니다
근데 문제는 HelloWorldContext 내부에서 Handler를 생성해서 별 차이가 없어지게 된 것 같습니다..ㅜ
There was a problem hiding this comment.
관련한 문제는 대규모 변경...을 통해 HelloWorldContext에서 mapping된 내용이 보이지 않도록 리펙토링을 진행했습니다!
| private final HttpRequestBody body; | ||
| private final QueryParameters queryParameters; | ||
| private final HttpCookie cookie; | ||
| private SessionManager sessionManager; |
There was a problem hiding this comment.
SessionManager가 Session에 대한 인메모리 DB처럼 동작해서, static 변수로 빼내도 될 것 같은데 지토는 어떻게 생각하시나요?
There was a problem hiding this comment.
코코닥이 말씀하신 부분에 동의합니다! 지금 코드는 인메모리 DB처럼 동작하고, static으로 분리해도 큰 상관이 없는 상태가 된 것 같습니다
다만 제 의도랑 현재 코드랑 좀 엇나간 것 같습니다...
저는 하나의 Context는 하나의 SessionManager를 가지도록 하고 싶었는데 지금은 이러나 저러나 큰 상관이 없어진 것 같습니다..
해당 부분 리펙토링 해서 개선해보도록 하겠습니다 ㅜㅜ
There was a problem hiding this comment.
아 제가 제 코드를 잘못 이해하고 있었네요..코드 짠지 얼마나 됐다고 남의 코드가 되어버렸습니다..
제 의도로 하나의 Context는 하나의 SessionManager를 가지도록 하고 싶었다고 말씀드렸는데 다음과 같은 상황이라고 보시면 될 것 같습니다
HelloWorldContext context1 = new HelloWorldContext("/app1");
HelloWorldContext context2 = new HelloWorldContext("/app2");이 때 /app1/login을 통해 로그인을 수행했다고 하더라도 /app2/login은 그래도 login.html이 출력되는 걸 의도했습니다
/app1에 대한 세션에는 유저가 저장되어 있지만 /app2에 대한 세션에는 유저가 저장되어 있지 않은 그런 상황이라고 보시면 될 것 같습니다
그래서 지금 상황...은 말씀해주신거랑 살짝 다른 느낌이라 해당 부분은 계속 유지해도 괜찮을까요?
| private Response process(final Request request) throws IOException { | ||
| for (final Handler handler : handlers) { | ||
| if (handler.supports(request)) { | ||
| request.initSessionManager(SESSION_MANAGER); |
There was a problem hiding this comment.
SessionManager를 넘겨서 초기화해주어야하는 이유는 무엇일까요?
아직까지는 SessionManager가 인스턴스 상태 값을 가지는 친구가 아닌 것 같아 여쭤봅니다!
There was a problem hiding this comment.
위에 코멘트랑 동일한 상황이라고 보시면 될 것 같습니다
각 HelloWorldContext별로 세션을 분리하고 싶어서 위와 같이 HelloWorldContext 인스턴스 별로 별도의 SessionManager를 만들어줬기 때문에 이와 같이 Request에 SessionManager를 넘겨줬다고 봐주시면 될 것 같습니다
|
|
||
| private Response process(final Request request) throws IOException { | ||
| for (final Handler handler : handlers) { | ||
| if (handler.supports(request)) { |
There was a problem hiding this comment.
메서드명이 짧고 직관적이라 가독성이 굉장히 좋은 것 같아요.
| this.resourceName = resourceName; | ||
| } | ||
|
|
||
| public static Url from(final String url) { |
| return url.substring(0, queryParameterDelimiterIndex); | ||
| } | ||
|
|
||
| private static String findResourceName(final String target) { |
There was a problem hiding this comment.
resource 라고 하면 보통 정적 리소스를 떠올리게 돼서 메서드명이 조금 헷갈리는 것 같아요.
다른 메서드명을 고민해보시는 건 어떨까요?
There was a problem hiding this comment.
URL 관점에서는 확실히 어울리지 않는 네이밍이었네요..반성합니다 네이밍 진짜 싫네요
조금 더 많이 고민해보고...변경하도록 하겠습니다...
|
|
||
| private static boolean hasType(final ContentType contentType, final String[] target) { | ||
| return Arrays.stream(target).anyMatch(type -> | ||
| type.endsWith(contentType.extension) || type.contains(contentType.accept)); |
There was a problem hiding this comment.
지토는 Content-Type 을 찾을 때 고려해야 할 우선순위를 왜 이렇게 결정하셨나요?
There was a problem hiding this comment.
제가 이해하기로는 파일 확장자명이 1순위고 Accept가 2순위로 Content-Type을 지정한 이유를 질문해주신 것 같은데 맞나요..?
일단 제가 이해한 질문대로 답변드리면 저는 가장 정확한 Content-Type은 파일 확장자명이라고 생각했습니다
Accept는 순위를 통해서 여러 Content-Type을 지정할 수 있어서 위와 같이 생각했습니다
근데 파일 확장자명이 명확하지 않은 경우 Accept에 명시된 Content-Type 중 서버가 제공할 수 있는 타입을 아무거나 고르면 된다고 알고 있어서 위와 같이 설정해주었습니다
| return Arrays.stream(ContentType.values()) | ||
| .filter(contentType -> hasType(contentType, targetType)) | ||
| .findAny() | ||
| .orElse(ContentType.JSON); |
There was a problem hiding this comment.
적절한 Content-Type을 못 찾았을 때, 예외가 아닌 JSON을 반환해주는 이유가 궁금합니다!
There was a problem hiding this comment.
별 다른 생각이 없었습니다..
지금 리뷰를 보니 예외를 반환해주는게 더 자연스럽겠네요
찾아보니까 Accept에 명시한 리소스가 없거나 지정한 리소스를 Accept에 해당하는 Content-Type으로 제공해주지 못하는 경우 406 Not Acceptable을 반환해주네요
이걸 기반으로 리펙토링 하도록 하겠습니다! 좋은 리뷰 정말 감사드립니다
| TEXT_HTML(".html", "text/html", "text/html;charset=utf-8"), | ||
| CSS(".css", "text/css", "text/css"), | ||
| JS(".js", "text/javascript", "text/javascript;charset=utf-8"), | ||
| ICO(".ico", "image/apng", "image/apng"), | ||
| JSON(".json", "application/json", "application/json"); |
There was a problem hiding this comment.
확장자명이나 Accept, Content를 한 곳에서 관리할 수 있군요.. 배워갑니다
안녕하세요, 코코닥! 오랜만에 인사드립니다
처음에는 미션이 톰캣 구현하기라고 되어 있어서 Embedded Tomcat을 분석하고 구현하려고 했는데, 잘못된 선택이었던 것 같습니다..
실력부족으로 인해 촉박하게 코드를 작성하다보니 좀 많이..지저분하고 여러모로 읽기 힘든 코드가 되었네요.. 양해 부탁드립니다 ㅜㅜ
리뷰 잘 부탁드립니다..