[톰캣 구현하기 - 3, 4단계] 달리 미션 제출합니다 #442

waterricecake · 2023-09-10T02:56:31Z

안녕하세요 아코!
이번 단계에 리펙토링을 통해 전체적으로 바꿔 보았어요.
request가 들어오면

filter를 통해 현재 jsession이 존재할 경우 맞는 유저인지 확인하여 리다이렉트를 해주거나 아님 유저가 아닐경우 401에러 페이지를 리다이렉트 해줍니다.
필터에 적용되는 path가 아니라면 handler를 통해 알맞는 path가 있는지 확인하고 없을 경우 404에러를 띄어 주었습니다.

잘부탁드립니다.

seokhwan-an

안녕하세요! 달리
먼저 3, 4단계도 고생하셨습니다!👏

3, 4단계를 진행하면서 코드의 전체적인 구조와 흐름에서 가독성이 좋아져서 코드 리뷰를 하는데 술술 읽혀서 즐거웠습니다.

코드 리뷰를 하면서 달리의 의견을 듣고 싶은 부분에 대해서 피드백을 남겼습니다! 한번 확인부탁드립니다!

seokhwan-an · 2023-09-11T10:28:20Z

tomcat/src/main/java/nextstep/jwp/config/LoginFilter.java

+ final String uri = request.getPath();
+ final var cookie = request.getCookie();


혹시 자료형을 나타낼 때 var를 사용하는 경우와 그렇지 않은 경우는 어떤 기준이신가요?

아 원래는 var를 안쓸려고 했는데 여기에 추가한 것을 깜빡했네요;;;;
var를 안써봐서 확실한 기준이 아직은 없네요;;
만약 사용한다면 메서드나 기능적인면 혹은 역할이 바뀌지 않는 객체이지만 후에 type이 변경될 수 있을때 사용할 것 같습니다.
0ad5111

seokhwan-an · 2023-09-11T13:21:56Z

tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java

+ FilterChainManager filterChainManager = new FilterChainManager();
+ filterChainManager.add(new LoginFilter());
+ filterChainManager.getInitialChain().doFilter(request, response);


FilterChainManager는 요청이 들어올 때마다 생성이 되는데 이 부분을 싱글톤으로 관리해보는 것은 어떤가요?🤔 그리고 회원가입 페이지도 필터에 추가해도 될거 같아요! 제 생각에는 로그인이 된 상태로 회원가입에 접근할 필요가 없다고 생각합니다!

0ff993d
0fca1b5
적용하면서 handler또한 싱글톤으로 관리하면 좋겠다는 생각이 들어 추가해보았습니다.

seokhwan-an · 2023-09-11T13:28:14Z

tomcat/src/main/java/org/apache/coyote/http11/filter/DefaultFilter.java

+public class DefaultFilter implements Filter {
+ @Override
+ public void doFilter(Request request, Response response, FilterChain filterChain) {
+ }


현재 DefaultFilter는 무슨 역할을 가지고 있는 필터인가요?🤔

DefaultFilter가 아무런 필터를 추가하지 않았을때 npe를 피하기 위해 설정해둔 것인데 이를 initialChain과 lastChain에 초기 생성시 정의하는 것을 잊었네요;;;;;
0fca1b5

추가하신 것 확인했습니다!

seokhwan-an · 2023-09-11T13:56:31Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

+ public FilterChainManager() {
+ this.defaultChain = new Chain(new DefaultFilter());
+ }


현재 FilterChainManager가 생성되고 바로 아무런 filter가 등록되지 않고 dofilter가 실행되게 되면 initialChain이 null이어서 에러가 발생할 것 같습니다. 초기 생성될 때 initalChain과 lastChain을 따로 정의하지 않은 이유는 무엇인가요?🤔

DefaultFilter가 아무런 필터를 추가하지 않았을때 npe를 피하기 위해 설정해둔 것인데 이를 initialChain과 lastChain에 초기 생성시 정의하는 것을 잊었네요;;;;;
0fca1b5

추가하신 것 확인했습니다!

seokhwan-an · 2023-09-11T15:26:30Z

tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerAdapter.java

+ public Controller mapping(Request request) {
+ RequestMapper requestInfo
+ = new RequestMapper(request.getMethod(), request.getPath());
+ if (request.getPath().contains(".")) {
+ return new ViewController();
+ }
+ return controllers.entrySet().stream()
+ .filter(entry -> entry.getKey().equals(requestInfo))
+ .findFirst()
+ .map(Entry::getValue)
+ .orElseThrow(NoSuchApiException::new);
+ }


현재 HandlerAdapter와 RequestMapper를 서로분리를 해주셨는데요. HandlerAdapter의 mapping 메소드는 RequestMapper에 더 적합한 기능이라고 생각이 듭니다.
제가 이해한 RequestMapper와 HandlerAdapter는 다음과 같습니다.

RequestMapper: 사용자의 요청(path, method 등)정보를 이를 해결할 수 있는 컨트롤러를 찾는 역할

HandlerAdapter: 정해진 컨트롤러를 실행할 수 있는 어뎁터를 찾는 역할

즉, mapping메소드는 현재 요청의 path와 method를 통해 실행할 컨트롤러를 찾는 것이기에 이부분은 RequestMapper에서 처리를 하는 것이 좋지 않을까요?
이 부분에 대한 달리의 생각을 듣고 싶습니다!☺️

RequestMapper는 각 컨트롤러가 담당하는 method와 url을 저장하는 값객체인데 제가 클래스명을 제대로 정하지 않아서 헤깔릴수도 있겠다 생각이 드네요. 정확한 역할은

RequestMapper: path와 method를 통해 equals와 hash를 정의하고 가지고 있음

HandlerAdapter: Map<RequestMapper,Controller>로 사용자의 요청과 requestMapper를 매칭하고 해당하는 컨트롤러를 찾아줌

입니다. 때문에 requestMapper의 클래스명을 바꾸는 것이 어떤가 생각합니다.
def103c

클래스 명칭을 수정하니 보다 명확하게 이해가 됩니다. 👍 개인적으로 궁금한 부분은 path만이 아닌 method까지 포함해서 controller를 찾는것 인데요. 그 이유는 controller 내부적으로 doPost와 doGet로 나누어져서 처리가 되기 때문에 path로만 controller를 찾는 것도 괜찮지 않을까 생각합니다.

seokhwan-an · 2023-09-12T00:06:48Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpMethod.java

+public enum HttpMethod {
+ GET("GET"),


여기도 개행 한줄 추가해 주세요!

seokhwan-an · 2023-09-12T00:13:52Z

tomcat/src/main/java/nextstep/jwp/controller/RegisterController.java

+
+

여기 개행이 두줄 입니다!😱

seokhwan-an · 2023-09-12T00:16:24Z

tomcat/src/main/java/nextstep/jwp/db/InMemorySession.java

 public class InMemorySession {
 private static final Map<User, UUID> session = new HashMap<>();


한줄 개행 추가부탁합니다!
혹시 UUID가 키 값이 아니고 User가 키 값인 이유가 있을까요? 저라면 UUID를 키값으로 가지고 저희가 필요한 정보인 User를 값으로 했을 것 같습니다. 이 부분에 대해서 달리의 의견이 궁금합니다!

오 아코 말이 맞는 것 같네요! 단순히 user가 key를 가지는 것이라 생각했는데 생각해보니 uuid를 통해 user를 찾는 것이니 이런형식이 맞겠군요.
3bd9ea4

seokhwan-an · 2023-09-12T00:16:58Z

tomcat/src/main/java/nextstep/jwp/exception/UnauthorizedException.java

+public class UnauthorizedException extends RuntimeException {
 public UnauthorizedException(String message) {


여기도 개행 추가 부탁드립니다!

seokhwan-an · 2023-09-12T00:22:19Z

tomcat/src/main/java/org/apache/coyote/http11/servlet/Servlet.java

여기도 개행이 두줄입니다!😱

sonarcloud · 2023-09-12T10:43:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

seokhwan-an · 2023-09-13T02:51:23Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

+ public static FilterChainManager getInstance(){
+ if(instance == null){
+ synchronized (FilterChainManager.class){
+ instance = new FilterChainManager();
+ }
+ }
+ return instance;
+ }


싱글톤을 잘 구현해주셨네요👍
코드 컨벤션은 잘 안 지켜졌네요..😥

Suggested change

public static FilterChainManager getInstance(){

if(instance == null){

synchronized (FilterChainManager.class){

instance = new FilterChainManager();

}

}

return instance;

}

public static FilterChainManager getInstance() {

if (instance == null) {

synchronized (FilterChainManager.class) {

instance = new FilterChainManager();

}

}

return instance;

}

seokhwan-an · 2023-09-13T02:54:36Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

@@ -11,14 +11,19 @@ public class FilterChainManager {

 public FilterChainManager() {
 this.defaultChain = new Chain(new DefaultFilter());
+ initialChain = lastChain = defaultChain;


이 부분은 두줄로 분리하는 것이 가독성이 좋을 것 같습니다!🤔

seokhwan-an · 2023-09-13T02:57:06Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

+ if(initialChain.equals(defaultChain)){
 initialChain = chain;
+ initialChain.next = defaultChain;
+ return;
+ }
+ if(lastChain.equals(defaultChain)){
+ initialChain.next = chain;


컨벤션이 지켜지지 않았네요..

Suggested change

if(initialChain.equals(defaultChain)){

initialChain = chain;

initialChain.next = defaultChain;

return;

}

if(lastChain.equals(defaultChain)){

initialChain.next = chain;

if (initialChain.equals(defaultChain)) {

initialChain = chain;

initialChain.next = defaultChain;

return;

}

if (lastChain.equals(defaultChain)) {

initialChain.next = chain;

seokhwan-an · 2023-09-13T02:59:39Z

tomcat/src/main/java/org/apache/coyote/http11/filter/DefaultFilter.java

+public class DefaultFilter implements Filter {
+ @Override
+ public void doFilter(Request request, Response response, FilterChain filterChain) {
+ }


추가하신 것 확인했습니다!

seokhwan-an · 2023-09-13T03:02:05Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

+ if(!response.isFiltered()){
+ filter.doFilter(request, response, next);
+ }


여기도 코드 컨벤션이 지켜지지 않았네요..

seokhwan-an · 2023-09-13T03:04:36Z

tomcat/src/main/java/org/apache/coyote/http11/filter/FilterChainManager.java

+ public FilterChainManager() {
+ this.defaultChain = new Chain(new DefaultFilter());
+ }


추가하신 것 확인했습니다!

seokhwan-an · 2023-09-13T03:16:07Z

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

+ public void service(Request request, Response response) {
+ if (request.getMethod().equals(HttpMethod.POST)) {
+ doPost(request, response);
+ return;
+ }
+ doGet(request, response);


여기서 만약에 put이나 delete 요청이 들어오게 되면 어떻게 동작하게 될까요?🤔

Handler adaptor에 현재 RequestInfo에 put이나 delete 메서드에 해당되는 RequestInfo가 없어 NoSuchApiException 처리가 될것 같네요. 많약 요청에 대한 controller가 필요하다면 AbstractController에 doPut, doDelete 메서드를 추가하여 구현할 것 같습니다.

seokhwan-an

안녕하세요 달리!
제 리뷰를 빠르게 반영해주셨군요!👍 감사합니다!

제가 달리에게 도움이 되는 리뷰어가 되기위해 리뷰를 할 때 최대한 사소한 것도 보다도 달리의 생각을 물어보면서 메타인지를 시켜주는 것을 목표로 했었는데 도움이 되었는지는 모르겠습니다..!
저도 달리 코드를 보면서 많이 배울 수 있는 시간이었습니다☺️

현재 요구사항에 대해서는 충분히 만족한다고 생각해서 approve merge하겠습니다.
다음 미션도 화이팅입니다!💪

p.s 제가 처음에 미처 발견하지 못한 사항에 대해 리뷰 남겨보았는데 그 부분에 대해 확인해보면 좋을 것 같습니다!

waterricecake added 7 commits September 9, 2023 13:05

refactor: 클래스 제거

d007034

refactor: handler 구조 변경

7125da6

refactor: filter 구현

f03ea85

style: 코드 컨벤션 적용

eb2e34d

feat: threadPool 적용

43c6c51

test: study 요구조건 진행

79da7b0

refactor: 불필요한 클래스 분리 통합

b6cfe11

waterricecake self-assigned this Sep 10, 2023

waterricecake added 3 commits September 10, 2023 14:25

feat: controller 추상 매서드 추가

753539f

refactor: requestline 분리

9c26df1

refactor: bufferedreader 자원관리

0ff608d

seokhwan-an requested changes Sep 12, 2023

View reviewed changes

waterricecake added 10 commits September 12, 2023 13:40

style: 코드 컨벤션 적용

8b12c21

refactor: 사용하지 않는 클래스 제거

ec7ad61

refactor: Map key value 변경

3bd9ea4

refactor: 값객체 클래스명 변경

def103c

refactor: 생성자 접근자 변경

5019ea3

refactor: filter 등록을 안할 시 npe 방지 default filter 추가

0fca1b5

refactor: var 제거

0ad5111

style: 코드 컨벤션 적용

eb87ed1

feat: handler 및 FilterChainManager 싱글톤으로 관리

0ff993d

refactor: singleton 생성자 private 으로 변경

5f963d2

seokhwan-an reviewed Sep 13, 2023

View reviewed changes

seokhwan-an approved these changes Sep 13, 2023

View reviewed changes

seokhwan-an merged commit c33e0ec into woowacourse:waterricecake Sep 13, 2023
2 checks passed

		final String uri = request.getPath();
		final var cookie = request.getCookie();

		public class InMemorySession {
		private static final Map<User, UUID> session = new HashMap<>();

		public class UnauthorizedException extends RuntimeException {
		public UnauthorizedException(String message) {

[톰캣 구현하기 - 3, 4단계] 달리 미션 제출합니다 #442

[톰캣 구현하기 - 3, 4단계] 달리 미션 제출합니다 #442

Conversation

waterricecake commented Sep 10, 2023

seokhwan-an left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

seokhwan-an Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waterricecake Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokhwan-an left a comment

Choose a reason for hiding this comment

waterricecake Sep 12, 2023 •

edited

Loading

waterricecake Sep 12, 2023 •

edited

Loading

waterricecake Sep 12, 2023 •

edited

Loading

waterricecake Sep 12, 2023 •

edited

Loading

seokhwan-an Sep 13, 2023 •

edited

Loading

waterricecake Sep 12, 2023 •

edited

Loading

waterricecake Sep 13, 2023 •

edited

Loading