[Tomcat 구현하기 1 - 2단계] 페드로(류형욱) 미션 제출합니다.#578
[Tomcat 구현하기 1 - 2단계] 페드로(류형욱) 미션 제출합니다.#578seokmyungham merged 16 commits intowoowacourse:hw0603from
Conversation
seokmyungham
left a comment
seokmyungham
left a comment
There was a problem hiding this comment.
안녕하세요 페드로~ 재즈입니다 😊
프로젝트 같은 팀원에서 미션 리뷰이로 만나니 반갑네요 :)
미션 구현하느라 고생 많으셨고 코드 잘 봤습니다.
리팩토링은 다음 단계임에도 불구하고 깔끔하게 잘 구현해주셨네요 👍
사소한 코멘트만 남겨두었으니 확인해보시고
톰캣 내부 구조에 관한 더 깊은 논의들은 다음 단계에서 진행하시죠 ✈
| implementation 'org.springframework.boot:spring-boot-starter-webflux' | ||
| implementation 'org.apache.commons:commons-lang3:3.14.0' | ||
| implementation 'com.fasterxml.jackson.core:jackson-databind:2.17.1' | ||
| implementation 'pl.allegro.tech.boot:handlebars-spring-boot-starter:0.4.1' |
There was a problem hiding this comment.
페드로가 미션 프로젝트에 제안한 PR 잘 읽었습니다.
문제점을 찾아 잘 해결하셨네요 👍
| return resource != null; | ||
| } | ||
|
|
||
| public static String read(String filePath) { |
There was a problem hiding this comment.
Exception in thread "Thread-2" java.nio.file.InvalidPathException: Illegal char <:> at index 2:
제 실행 환경에서 HTTP 요청 시 매번 위와 같은 에러가 발생하네요.
로직 문제는 아닌 것 같아 직접 찾아보니 OS 마다 파일 경로를 처리하는 방식이 다르기 때문인 것 같아요.
Winodws 에서 파일 경로는 드라이브 문자(C:/Users/..)와 함께 사용되는데 이를 Unix 스타일(루트 디렉토리)로 잘못 해석하여 문제가 발생하는듯 합니다. (/C:/Users/..)
(저는 윈도우 사용자 ㅎㅎ)
URL::getPath이 문제인 것 같은데 미션 요구사항에 크리티컬한 부분은 아닌 것 같아 다음 미션 진행할 때 한 번 확인해보시면 좋을 것 같아요. 😊
There was a problem hiding this comment.
윈도우가 또... 농담이고ㅎㅎ 수정해 두겠습니다😁
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class Session implements HttpSession { |
There was a problem hiding this comment.
HttpSession은 Sevlet이 제공하여 기본 메서드가 워낙 많은데요 때문에 사용하지 않는 메서드도 많은 것 같아요.
현재 자바 코드로만 톰캣을 구현하는 것이 미션 목표이니 서블릿 의존성을 끊어보는 것은 어떨까요? 저는 Manager 인터페이스 시그니처를 수정했습니다.
There was a problem hiding this comment.
때문에 사용하지 않는 메서드도 많은 것 같아요.
저도 이것 때문에 고민을 좀 했었는데요, 'JavaDoc 까지 작성되어 있는 인터페이스 클래스를 내 입맛에 맞게 수정해서 쓰는 것이 과연 옳은가?' 를 고민해 봤을 때, 시그니처를 수정하는 것이 좀 더 어색한 구현이라고 판단해서 지금과 같이 작성해 두었어요.
이와 별개로, Servlet 자체는 톰캣의 것이라기보다는 JavaEE(현재는 Jakarta EE) 의 컴포넌트라 활용해도 큰 무리는 없을 것 같은데 어떻게 생각하시나요?
ref: https://jakarta.ee/specifications/platform/9/apidocs/jakarta/servlet/http/httpsession
|
|
||
| public class SessionManager implements Manager { | ||
|
|
||
| private static final Map<String, Session> SESSIONS = new HashMap<>(); |
There was a problem hiding this comment.
서버 세션 매니저 같은 경우, 다중 스레드 접근 시 발생하는 문제들을 고려해야 할 것 같아요.
동기화를 달성하는 방법은 페드로도 잘 알고 있을 거라 생각합니다 😊
| MediaType mediaType = MediaType.fromAcceptHeader(request.getAccept()); | ||
|
|
||
| // 로그인한 경우 index.html로 리다이렉트 | ||
| Optional<HttpSession> session = request.getSession(sessionManager); |
There was a problem hiding this comment.
HttpRequest에서 SessionManager 를 넘겨받아 세션을 가져오고 있는데 조금 어색한 것 같습니다.
현재는 책임상 HttpRequest가 세션을 관리하는 구조인 것 같고
SessionManager를 수정하면 HttpRequest도 수정해야하니 유연하지 못하다는 생각도 들어요.
sessionManager.xxx(request) 구조가 되어야할 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
세션이 Request의 소유물이라고 생각하고 구조를 잡아갔는데, 다시 생각해 보니 SessionManager가 존재하는 이상 제안해 주신 방향이 좀 더 자연스러운 것 같네요! 3단계 리팩토링을 하면서 수정해 보겠습니다~
2 participants
안녕하세요 재즈!
우연히 같은 팀원을 리뷰어로 만나게 되니 반갑네요.
3단계가 리팩토링으로 알고 있어서, 3단계까지 완료 휴 PR을 남기려고 했는데 여러 이유로 2단계까지만 완료하고 PR을 작성하게 됐어요.
때문에 코드가 아주.. 마음에 들지는 않네요.
객체 분리가 덜 된 부분도 많고 코드 스타일도 이번에는 신경쓰지 못했어요😅
리뷰 잘 부탁드립니다ㅎㅎ