[톰캣 구현하기 - 3, 4단계] 허브(방대의) 미션 제출합니다. #431

greeng00se · 2023-09-09T11:13:29Z

안녕하세요 디노 🦖 주말 잘 보내고 계신가요?

3, 4단계 미션 리뷰 요청드립니다!

전체적인 의존성 방향은 다음과 같이 구성되어있습니다.

graph LR
  jwp --> catalina --> coyote

세부적인 부분은 아래와 같이 구성했습니다.

graph LR
	subgraph coyote
		M[Mapper]
		HP[Http11Processor] --> M
		HP --> HttpRequestParser
		HP --> HttpResponseGenerator
  end

  subgraph catalina
		RM[RequestMapper] -.-> M
    AC[AbstractController] -.-> C[Controller]  
		StaticController -.-> AC  
		SM[SessionManger] -.-> Manager
		TC[Tomcat] --> RM
		RM --> C
		RM --> SM
  end

  subgraph jwp
    LC[LoginController] -.-> AC
		Application --> TC
  end

조금 더 꼼꼼하게 코드를 작성하고 싶은 부분이 있었는데, 시간 관계상 그러지 못한 부분이 조금 많았던 것 같아요!
디노랑 이야기해보면서 조금 더 개선해 나가고 싶습니다! 호호..

jjongwa

안녕하세요 허브~🌿 다이노입니다🦖

코드리뷰 재밌네요 ㅇㅡㅇ
이번에는 커밋 순서대로 따라가기 보단 전체적인 구조와 패키지 의존성 방향을 따라가 보며 이해해 봤어요.
어렵네요.
간단한 컨벤션 + 궁금한 부분 리뷰 남겼습니다!

제가 남긴 코멘트 중에 이해가 안되는 부분이 있으면 답글 남겨 주셔도 되고 직접 물어보셔도 됩니다!
그럼 이제 전 제 코드 리팩터링하러.. 이만 총총

jjongwa · 2023-09-10T15:42:34Z

tomcat/src/main/java/org/apache/catalina/startup/Tomcat.java

 connector.start();

 try {
 // make the application wait until we press any key.
 System.in.read();
 } catch (IOException e) {
 log.error(e.getMessage(), e);
- } finally {
+ } finally{


Suggested change

} finally{

} finally {

우테코 스타일 적용했는데 자동 저장하니까 공백이 없게 되네요 ㅠㅠ 리포맷 풀고 수정했습니다 👍

jjongwa · 2023-09-10T15:45:31Z

tomcat/src/main/java/org/apache/catalina/SessionManager.java

+
+public class SessionManager implements Manager {
+
+ private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();


구구의 리뷰에서 본건데요
#440 (comment)

상수 이름은 모두 대문자로 된 UPPER_SNAKE_CASE를 사용하며, 각 단어는 밑줄 하나로 다음 단어와 구분됩니다. 하지만 상수란 정확히 무엇일까요?

상수는 내용이 거의 불변이고 메서드에 감지할 수 있는 부작용이 없는 정적 최종 필드입니다. 예를 들어 원시값, 문자열, 불변 값 클래스, null로 설정된 모든 것이 포함됩니다. 인스턴스의 관찰 가능한 상태 중 하나라도 변경될 수 있다면 상수가 아닙니다. 객체를 절대 변경하지 않겠다는 의도만으로는 충분하지 않습니다.

이에 따르면 sessions로 사용하는게 코딩 스타일 가이드에 맞는 것 같습니다.!

호호 감사합니다! 상수명을 일관성있게 수정했습니다 👍

jjongwa · 2023-09-10T15:59:23Z

tomcat/src/main/java/nextstep/Application.java

+ new Tomcat()
+ .addController("/", new HomeController())
+ .addController("/login", new LoginController())
+ .addController("/register", new RegisterController())
+ .start();


각 컨트롤러를 RequestMapper에서 static으로 미리 넣어주지 않고 Tomcat의 addController 메서드를 사용하는 이유가 무엇인가요?

제가 허브의 코드를 보며 이해한 바로는 컨트롤러들(/, /login, /register)이 jwp단에 위치하기 때문에 catalina단에 존재하는 tomcat과 RequestMapper가 모르게 해야 하기 위해서 인것 같은데요.

제가 이해하지 못한 다른 이유가 있다면 코멘트 부탁드립니다.!

말씀해주신대로 RequestMapper에서 넣어주게 된다면 현재 코드에서 의존성 방향이 양방향이 됩니다.
또한 로그인과 회원가입과 같은 비즈니스 로직은 Tomcat을 사용하는 쪽에서 등록해줘야한다고 생각했습니다!

jjongwa · 2023-09-10T16:06:22Z

tomcat/src/main/java/org/apache/catalina/startup/Tomcat.java

 public class Tomcat {

 private static final Logger log = LoggerFactory.getLogger(Tomcat.class);
+ private static final int DEFAULT_THREAD_COUNT = 250;
+
+ private final Mapper mapper = new RequestMapper(new StaticController());


다른 컨트롤러들과 StaticController의 패키지 위치가 다른 이유는 무엇인가요?

staticController의 경우 기본적으로 resources에 있는 정적 파일에 대한 요청이 있을 때 사용됩니다.
실제로 웹서버를 사용할 때도 따로 정적 파일에 대한 부분을 지정해주지 않는 것을 생각하여 catalina 패키지에 두었습니당 😄

하나 배워갑니다..!ㅎㅎ

jjongwa · 2023-09-10T16:21:31Z

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

+ final String generate = httpResponseGenerator.generate(httpResponse);
+ outputStream.write(generate.getBytes());
+ outputStream.flush();
+ } catch (final Exception e) {
+ LOG.error(e.getMessage(), e);
+ internalServerError(outputStream);
 }
- final String account = requestBody.get(ACCOUNT);
- final String password = requestBody.get(PASSWORD);
- return InMemoryUserRepository.findByAccount(account)
- .filter(user -> user.checkPassword(password))
- .map(this::loginSuccess)
- .orElseGet(() -> new ResponseEntity(HttpStatus.UNAUTHORIZED, "/401.html"));
- }
-
- private ResponseEntity loginSuccess(final User user) {
- final String uuid = UUID.randomUUID().toString();
- final ResponseEntity responseEntity = new ResponseEntity(HttpStatus.FOUND, INDEX_PAGE);
- responseEntity.setJSessionId(uuid);
- final Session session = new Session(uuid);
- session.setAttribute("user", user);
- sessionManager.add(session);
- return responseEntity;
 }

- private ResponseEntity register(final RequestLine requestLine, final RequestBody requestBody) {
- if (requestLine.getHttpMethod() == HttpMethod.GET) {
- return new ResponseEntity(HttpStatus.OK, REGISTER_PAGE);
- }
- final String account = requestBody.get(ACCOUNT);
-
- if (InMemoryUserRepository.findByAccount(account).isPresent()) {
- return new ResponseEntity(HttpStatus.CONFLICT, "/409.html");
- }
-
- final String password = requestBody.get(PASSWORD);
- final String email = requestBody.get(EMAIL);
- InMemoryUserRepository.save(new User(account, password, email));
- return new ResponseEntity(HttpStatus.FOUND, INDEX_PAGE);
+ private void internalServerError(final OutputStream outputStream) throws IOException {
+ final HttpResponse httpResponse = new HttpResponse(HttpVersion.HTTP_1_1)
+ .setHttpStatus(HttpStatus.INTERNAL_SERVER_ERROR)
+ .sendRedirect("/500.html");
+ final String response = httpResponseGenerator.generate(httpResponse);
+ outputStream.write(response.getBytes());
+ outputStream.flush();


final String generate = httpResponseGenerator.generate(httpResponse); outputStream.write(generate.getBytes()); outputStream.flush();

요 부분 함수 추출하는거 어떠신가요? (그냥 의견임.)

너무 좋아요 👍 덕분에 중복 제거 깔끔하게 했습니다!

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

jjongwa · 2023-09-10T16:51:17Z

tomcat/src/main/java/org/apache/catalina/RequestMapper.java

+ private void setSessionToHttpRequest(final HttpRequest httpRequest) {
+ final String sessionId = httpRequest.parseSessionId();
+ if (sessionId == null) {
+ return;
+ }
+ final Session session = sessionManager.findSession(sessionId);
+ httpRequest.setSession(session);
+ }
+
+ private void addSession(final HttpResponse httpResponse) {
+ final Session session = httpResponse.getSession();
+ if (session == null) {
+ return;
+ }
+ sessionManager.add(session);
+ }


Request의 parseSessionId와 Response의 getSession 에서 null 체크를 하고 여기서 try-catch로 잡는 방식은 어떠신가요?

메서드에서 null을 반환한다면 추후에 다른 로직에 해당 메서드가 사용되는 상황에서 null 체크를 까먹을 수도 있고, 계속 null 반환 여부를 염두에 둔 채 개발을 진행해야 한다는 번거로움이 생긴다고 생각해서요.!

해당 부분은 try-catch 대신 Session을 관리하는 SessionManager에서 null 체크를 하도록 수정했습니다!

jjongwa · 2023-09-10T17:00:58Z

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

 return Arrays.stream(body.split(SEPARATOR))
 .map(field -> field.split(DELIMITER))
 .collect(collectingAndThen(


현재 로그인과 회원가입의 입력칸에 아무것도 넣지 않았을 때
해당 부분의 로직에서
index에 해당하는 value가 없으면 IndexOutOfBoundError 발생 -> Http11Processor 에서 catch -> 500 Response 반환
의 흐름을 통해 처리하고 있는데요.

Suggested change

return Arrays.stream(body.split(SEPARATOR))

.map(field -> field.split(DELIMITER))

.collect(collectingAndThen(

return Arrays.stream(body.split(SEPARATOR, 2))

.map(field -> field.split(DELIMITER, 2))

.collect(collectingAndThen(

의 형태로 바꾸어 빈 문자열을 받은 뒤 이에 따른 예외 처리를 진행해 주는 방식은 어떤가요?

body.split에 2를 넣어주는건 account, email, password 3개를 받을 때 오류를 발생시킬 것 같습니다!
field의 경우 2개로 받아서 생성하는 부분 너무 좋은 것 같아요 👍

아 그러네요 ㅎㅎ 잘못 봤습니다.!

jjongwa · 2023-09-10T17:09:52Z

tomcat/src/main/java/org/apache/coyote/http11/common/Headers.java

+ }
+
+ public void addHeader(String line) {
+ if (line == null || line.isEmpty()) {


isBlank() 어떠신가요?

좋아용 👍

jjongwa · 2023-09-10T17:13:16Z

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

지금 사용되고 있는 쪽은 request지만 HTTPMethod의 의미상 common 패키지에 둔 걸로 이해하면 될까요?

넵 맞습니다!

greeng00se · 2023-09-11T08:40:37Z

안녕하세요 다이노 🦖
꼼꼼히 리뷰를 남겨주셔서 리뷰 반영하면서 너무 재밌었습니다!
이전의 RequestMapper의 책임이 너무 큰 것 같아서 아래와 같이 Adapter 클래스를 추가하였습니다.
감사합니다 👍

graph LR
	subgraph coyote
		HP[Http11Processor] --> A
		HP --> HttpRequestParser
		HP --> HttpResponseGenerator
		A[Adapter]
  end

  subgraph catalina
		RA[RequestAdapter] -.-> A
		AC[AbstractController] -.-> C[Controller]
		StaticController -.-> AC
		SM[SessionManger] -.-> Manager
		TC[Tomcat] --> RA
		RA --> C
		RA --> Manager
		RA --> RM
		RM[RequestMapper] --> C
  end

  subgraph jwp
		LC[LoginController] -.-> AC
		Application --> TC
  end

sonarcloud · 2023-09-11T08:45:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 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

jjongwa

안녕하세요 허브~🌿 다이노입니다🦖

리뷰 남긴 부분에 대한 답글 확인했습니다!

허브가 남겨 주신 그래프가 코드를 이해하는데 아주 큰 도움이 되었고,
전체적인 구성과 로직에 대한 근거와 사고 과정을 따라가는 과정에서
제가 오히려 많이 배웠다고 생각합니다ㅎㅎ 즐거웠어요~~

요구사항도 모두 만족했기에(이전에 이미 만족했지만) 이만 머지하도록 할께요ㅎㅎ 다음 미션도 파이팅입니다~~~~!!

greeng00se added 30 commits September 4, 2023 21:57

feat: HTTP 활용 테스트 진행

8f80ba8

feat: HttpVersion 추가

5cde353

feat: Http11Exception을 제외한 CustomException 제거

d36d48d

feat: RequestLine에 HttpVersion 적용

2aa52b6

feat: Path 클래스 추가 및 적용

99c9efb

feat: HttpRequest, HttpRequestParser 추가

c425310

feat: Thread 실습 내용 추가

ef65b92

refactor: RequestHeader Headers로 변경

680320e

feat: Controller와 AbstractController 추가

1feabc5

refactor: Session catalina 패키지로 변경

9d2d82b

feat: HttpResponse 객체 추가

6048c94

refactor: 컨트롤러 throws Exception 제거

3ea8e9c

feat: HttpRequest parseCookie 추가

7096104

feat: RequestBody 기본 생성자 추가

cadc5bc

feat: 세션 clear 메서드 추가

e46a70e

feat: LoginController 추가

978aebf

feat: HomeController 추가

858e10e

feat: RegisterController 추가

94504e8

feat: Static Controller 추가

ac71648

feat: RequestMapper 추가

4df9960

feat: Tomcat에 Mapper 적용

a159ff0

feat: HttpResponseGenerator HttpResponse 받아서 반환값 생성하는 기능 추가

96098be

feat: 알 수 없는 예외가 발생하는 경우 Internal Server Error 반환하는 기능 추가

e6e0a9e

refactor: 톰캣 실행 부분 수정

ff4a5c3

test: redirect에 대한 검증부 수정

4458086

docs: README 수정

d7bd953

feat: 최대 ThreadPool 크기 250으로 적용

fd7a306

feat: ConcurrentHashMap 사용

c0c6e36

fix: JSESSION_ID 가져올 때 getOrDefault 사용하도록 수정

990d183

feat: SessionManager를 RequestMapper가 의존하도록 수정

4d6dea6

greeng00se self-assigned this Sep 9, 2023

greeng00se requested a review from jjongwa September 9, 2023 11:13

greeng00se added 2 commits September 9, 2023 20:17

chore: 빌드 설정 수정

69b0f73

remove: 사용하지 않는 필드 제거

801ddc8

jjongwa requested changes Sep 10, 2023

View reviewed changes

greeng00se added 9 commits September 11, 2023 14:35

fix: finally 후 space 추가

2330205

refactor: 상수명 일관성 있게 수정

67f7dd2

refactor: Http11Processor 중복 제거

139bef6

refactor: isEmpty -> isBlank로 수정

812e5f8

feat: 사용자 생성시 검증하는 부분 추가

9254604

refactor: session null check 부분 수정

643c045

refactor: final 빠진 부분 추가

1c49621

feat: Adapter 추가

351882a

test: 세션 관련 테스트 수정

5825bae

fix: 불필요한 구문 제거

da4e1f6

jjongwa approved these changes Sep 11, 2023

View reviewed changes

jjongwa merged commit b0695b9 into woowacourse:greeng00se Sep 11, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 허브(방대의) 미션 제출합니다. #431

[톰캣 구현하기 - 3, 4단계] 허브(방대의) 미션 제출합니다. #431

greeng00se commented Sep 9, 2023

jjongwa left a comment

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

jjongwa Sep 10, 2023

greeng00se Sep 11, 2023

greeng00se commented Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

jjongwa left a comment


		public class SessionManager implements Manager {

		private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();

[톰캣 구현하기 - 3, 4단계] 허브(방대의) 미션 제출합니다. #431

[톰캣 구현하기 - 3, 4단계] 허브(방대의) 미션 제출합니다. #431

Conversation

greeng00se commented Sep 9, 2023

jjongwa left a 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

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

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

greeng00se commented Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

jjongwa left a comment

Choose a reason for hiding this comment