Skip to content
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

[톰캣 구현하기 - 3, 4 단계] - 제나(위예나) 미션 제출합니다 #458

Merged
merged 21 commits into from
Sep 12, 2023

Conversation

yenawee
Copy link

@yenawee yenawee commented Sep 10, 2023

안녕하세요 로건
1,2 단계때 좋은 리뷰 덕분에 놓치고 있던 부분을 많이 잡을 수 있었어요 감사합니다 👍

이전 PR 에 대한 답은 아래 링크에서 확인 부탁드립니다!
#355

이전에 HttpResponse 객체는 builder 패턴을 사용해서 구현했었는데 요구사항에서 Controller 의 파라미터를 건드리면 안되어서 매개변수로 들어오는 HttpResponse 에 값을 넣어주려다보니 어쩔수 없이 setter 를 많이 사용하게 되어버렸는데요..!
혹시 로건은 Controller 의 파라미터를 안건드리면서 response 값을 어떻게 세팅해주셨는지 궁금합니다

그리고 예외상황에 대한 처리가 좀 미흡한거같아서 로건이 알고있는 예외상황(?) 에 대한 조언도 주시면 감사합니다!!

3,4단계 리뷰도 잘 부탁드립니다 🌱

@yenawee yenawee self-assigned this Sep 10, 2023
Copy link
Member

@70825 70825 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제나!
1, 2단계에서 있던 긴 코드들이 굉장히 깔끔해지고, 상수 처리를 해서 읽기 매우 편했어요!

이전에 HttpResponse 객체는 builder 패턴을 사용해서 구현했었는데 요구사항에서 Controller 의 파라미터를 건드리면 안되어서 매개변수로 들어오는 HttpResponse 에 값을 넣어주려다보니 어쩔수 없이 setter 를 많이 사용하게 되어버렸는데요..!
혹시 로건은 Controller 의 파라미터를 안건드리면서 response 값을 어떻게 세팅해주셨는지 궁금합니다

저도 setter로 설정해서 더 좋은 방법이 있나 고민을 많이 했는데, Controller 파라미터를 그대로 가진채로 하려면 setter를 사용할 수 밖에 없는 것 같아요.
그래서 톰캣에서도 정말 그렇게 사용할까 찾아보니까 apache/catalina/connector/Response에서도 setter가 많이 있긴하더라구요.


전반적으로 코드를 잘 작성해주셔서 추가적으로 수정하거나, 의견을 남길만한 부분은 ❗이모지를 달아두었고, 선택적으로 리팩터링을 진행할만한 부분은 ❓이모지를 앞에 달아두어서 참고하시면 될 것 같아요


그리고 마지막으로 이미 고민을 해보셨을지도 모르겠지만, 혹시 몰라서 코멘트 남깁니다!
이런 부분도 고민해보시면 좋을 것 같아요.

현재 Controller 인터페이스가 apache.coyote에 있는데, 이게 정말 톰캣에서도 존재할까요?
찾아보시면 Controllerapache 패키지보다 nextstep 패키지에 있는게 어울린다는 것을 알 수 있는데요.
그러면 Http11Processor는 톰캣에 있는데, 어떻게 톰캣은 스프링을 모르는 상태로 적합한 Controller를 찾아서 처리하는걸까? 라는 생각을 해보며 동작 과정을 간단하게 찾아보시면 좋을 것 같습니다.

@sonarcloud
Copy link

sonarcloud bot commented Sep 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning 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

@yenawee
Copy link
Author

yenawee commented Sep 12, 2023

안녕하세요 로건
리뷰 반영 후 리뷰 재요청드립니다!!

코멘트에 있던 내용을 좀 고민해보느라 좀 늦었어요

  1. Controller 인터페이스가 apache 패키지에 있던 이유

apache.coyote 와 jwp 패키지의 양방향 의존을 끊기 위해서 인터페이스는 coyote 에 놔두고 구현체들만 jwp 패키지에 놔두었었습니다! (의존성이 jwp -> coyote 로 흘러가게끔하기 위해)
톰캣이 Request 에 따라 Controller 를 매핑해서 반환하는 일을 하니 구체적인 클래스 말고 Controller 인터페이스는 갖고 있어도 괜찮다구 생각했어요.
근데 로건 리뷰 보고 다시 코드 확인해보니까 RequestMapping 에서 다시 양방향 의존하고 있었네요 ㅠㅠ
그리고 오늘 강의 때 catalina 가 서블릿 컨테이너의 역할을 하는 패키지라 해서 RequestMapping 랑 Controller 인터페이스의 위치를 catalina 로 옮겨야 할 거 같아요. 근데 의존성 끊으려고 추상화하려면 코드 대공사가 일어날거같아서.... 시간이 부족해서 우선 놔두기로 했습니다 🥺

  1. 톰캣은 스프링을 모르는 상태로 적합한 Controller를 찾아서 처리하는걸까?

그래서 @WebServlet 등으로 톰캣에 서블릿을 등록해주는게 아닐까여??
그래서 제 코드를 기준으로 보면 tomcat 이 어떤 url 이 어떤 controller 로 매핑되는지에 대한 정보를 RequestMapping 에 static 으로 담아두는게 아니고 Application 클래스에서 tomcat 에 주입해야 할 거 같습니다.

사실 로건 리뷰 보고 톰캣 코드를 좀 뜯어봤는데.. 너무 추상화가 많이 되어있고 복잡해서 파악이 어렵네용 😥

뭔가 Connector, Adapter, ProtocolHandler, Processor 등이 중요한 객체들인거같은데 어떤 순서대로 어떻게 동작하는지 제대로 파악하려면 오래걸릴거같아서 리뷰 요청드려요!! 로건 리뷰 덕분에 안보던 톰캣 코드도 뜯어보고 여러 생각을 해볼 수 있어서 좋았습니다 👍

리뷰 감사합니다 😊

Copy link
Member

@70825 70825 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제나!
고생 많으셨어요!
다음 미션도 화이팅입니다 🥳

@70825 70825 merged commit ffae834 into woowacourse:yenawee Sep 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants