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

[톰캣 구현하기 - 1, 2단계] 라온(문채원) 미션 제출합니다. #376

Merged
merged 20 commits into from
Sep 5, 2023

Conversation

mcodnjs
Copy link

@mcodnjs mcodnjs commented Sep 4, 2023

안녕하세요, 에밀 ~! 라온입니다!
톰캣 구현하기 1, 2단계 잘 부탁드립니다 ~!

굴러는 가지만 .. 아직 전체적인 리팩토링이 많이 필요하고 .. 테스트를 하나도 안짰네요 ㅎㅎ ..

전체적인 구조는 아래와 같습니다!

  1. Client 요청 후 Http11Processor에 전달
  2. HttpRequestParser로 HttpRequest 생성
  3. handleRequest() 메서드를 통해 핸들링 후 HttpResponse 반환

리뷰 반영하면서 추가적으로 request를 핸들링하는 클래스와 테스트들도 추가해보겠습니다!
리뷰 잘 부탁드립니다!
감사합니다 🙇‍♂️🙇‍♂️

@sonarcloud
Copy link

sonarcloud bot commented Sep 4, 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 11 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

@mcodnjs mcodnjs requested a review from CFalws September 4, 2023 08:58
@mcodnjs mcodnjs self-assigned this Sep 4, 2023
Copy link
Member

@CFalws CFalws left a comment

Choose a reason for hiding this comment

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

코드 직관적이고 심플하게 작성하셨네요!
이해하기 수월했고 요구사항도 잘 만족하신 거 같아요
리뷰는 3단계가 리팩토링이라, 예상치 못한 동작을 할 것 같은 부분 위주로 리뷰 작성하였습니다.
크지 않고, 리팩토링하시면서 같이 반영하시는 게 편할 거 같아 일단 어프루브 하겠습니다.
리플라이 다시면 확인할게요
고생하셨어요~


// TODO: Query Parse
for (String temp : new String(buffer).split("&")) {
String[] value = temp.split("=");
Copy link
Member

Choose a reason for hiding this comment

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

현재 상황에서는 X-WWW-FORM- ~ 타입의 데이터만 전송되지만 다른 타입에 대해서는 예상치 못한 동작을 할 거 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

혹시 이 부분에 대해서 조금 더 자세한 설명 가능할까요 ㅠㅠ ,,

// valid user
log.info("user: {}", user.get());

if (sessionId != null) { // if already have session
Copy link
Member

Choose a reason for hiding this comment

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

로그인 실패하면 401
유효한 유저지만 리퀘스트에 JSESSIONID 쿠키가 있고 유효하면 index.html로 리다이렉트(세션은 기존 세션을 유지)
세션이 없으면 세션으로 관리해주고 set-cookie 설정해주고 리다이렉트
위 플로우가 맞을까요?
그렇다면 이미 로그인 되어있지만 새로 로그인을 했다면 새로운 세션으로 관리해주는 것과 기존 세션을 유지하는 것 중 뭐가 더 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

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

로직에 대해 조금 더 정리를 하면 아래와 같은데요!

  • 이미 로그인된 유저(세션O) -> index.html로 redirect
  • 로그인하지 않은 유저(세션X) -> 세션 생성 및 set-cookie 설정
    • 로그인 성공 -> 302
    • 로그인 실패 -> 401

주신 질문에 답변을 하자면,

이미 로그인 되어있지만 새로 로그인을 했다면 새로운 세션으로 관리해주는 것과 기존 세션을 유지하는 것 중 뭐가 더 좋을까요?

저는 이미 로그인 되어 있는 유저를 계속 로그인 시킬 수 있는 것이 어색하다고 느껴져서,
이미 로그인 되어 있다면, 기존 세션을 유지하면서 + 로그인 페이지에 접근하지 못하도록 index.html로 redirect 시켜주는 로직으로 작성하였습니다! 에밀의 생각도 궁금합니다!

final HttpResponse httpResponse = HttpResponse.init();
String contentType = "text/html;charset=utf-8";

if (httpRequest.getHeader().get("Accept") != null) {
Copy link
Member

Choose a reason for hiding this comment

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

만약 accept 헤더가 없는 경우에는 전부 text/html;charset=utf-8로 content type이 설정되는 것 같아요.
그리고 accept 헤더가 있어도 여러개인 경우에는 첫번째 것으로 반영되는 것도 content-type 헤더와 실제 타입 간의 불일치를 만들 수 있을 것 같아요.
실제로 javascript 파일들은 accept 헤더가 */*로 되어 있는지 content-type이 */*이더라구요

image

Copy link
Author

@mcodnjs mcodnjs Sep 8, 2023

Choose a reason for hiding this comment

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

맞습니다 .. 해당 문제 인지하고, 리팩토링 과정에서 컨트롤할 수 있게 변경하였습니다!
Content-Type을 설정하는 로직은 3, 4단계 PR 커멘트에 설명해놓았습니다 ~!


private HttpStatusCode statusCode;
private final HttpHeaders headers;
// private final Map<String, String> header;
Copy link
Member

Choose a reason for hiding this comment

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

이거 놓치신 거 같아요~

return new HashMap<>();
}
Map<String, String> cookieValue = new HashMap<>();
for (String cookie : cookies.split("; ")) {
Copy link
Member

Choose a reason for hiding this comment

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

알고 계실 수 있지만 cookies.split("; ", number)에서
"key1=value1; key2=value2; ; "같은 입력에서
number가 음수면 뒤에 ["key1=value1", "key1=value1", ""]가 되고
number가 0이거나 없으면 ["key1=value1", "key1=value1"]가 됩니다.

팁으로 적어봤습니다^^

Copy link
Author

Choose a reason for hiding this comment

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

오 .. 전혀 몰랐습니다 감사합니다 👍

@CFalws CFalws merged commit 035a2c8 into woowacourse:mcodnjs Sep 5, 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