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단계 - Tomcat 구현하기] 시소(신지송) 미션 제출합니다 #524

Merged
merged 20 commits into from
Sep 11, 2024

Conversation

shin-jisong
Copy link

안녕하세요 레모네 🍋
바쁜 일주일이 벌써 지나갔네요
사전에 말씀드린대로 1단계와 2단계 완료하였습니다!

다만 기한을 맞추느라 보강하지 못한 부분이 있어서 해당 부분은 보충 예정입니다 🥲

  • 2단계의 4번 부분에서 Session 관련 기능은 구현하였으나 쿠키 조회 부분에서의 문제가 있어 수정 예정입니다
  • 학습 테스트의 경우, 완료하였으나 진행 후 커밋 되돌리기를 실행하고 미션을 진행하여서 다시 진행 예정입니다

해당 부분은 주말 내에 끝낼 예정입니다
제가 끝내기 전에 미리 리뷰 남겨 주셔도 괜찮아요👍
좋은 하루 보내세요 🎡

Copy link

@JiHyeonL JiHyeonL left a comment

Choose a reason for hiding this comment

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

안녕하세요 시소!!
리뷰가 조금 늦었죠, 죄송합니다 😢😢

지금은 Http11Processor 클래스 위주로 코멘트를 드렸어요.
전체적으로 코드가 깔끔해서 읽기 편안했네요 😄

2단계 미션 마무리 하시면, 편하게 리뷰 요청 주세요!!

import org.apache.coyote.Processor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
Copy link

Choose a reason for hiding this comment

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

시소 코드를 ide에 불러와서 확인하고 있는데요, 스프링 의존성이 없어서 해당 import가 적용되지 않아 몇몇 부분에 컴파일 에러가 나고 있어요!
Http Status를 하드코딩 하는 방식으로 수정해 주시는 것은 어떨까요?? 😄

Copy link
Author

Choose a reason for hiding this comment

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

HttpStatus가 스프링 의존성이라는 걸 놓쳤네요! 반영하였습니다 👍

BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
String requestLine = reader.readLine();

String path = readPath(requestLine);
Copy link

Choose a reason for hiding this comment

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

readPath 메서드에서 null이 반환될 경우, 아래의 if 문에서 NPE가 발생할 것 같습니다.
방어로직을 추가해 주시는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

반영하였습니다!


if (method.equals("POST")) {
Map<String, List<String>> queryParams = parsingQueryString(body);
User user = InMemoryUserRepository.findByAccount(queryParams.get("account").getFirst()).get();
Copy link

Choose a reason for hiding this comment

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

어카운트 정보가 없을 경우, 예외를 throw 하는 것이 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

반영하였습니다!

processFilesWithStatus(outputStream, "/index.html", HttpStatus.valueOf(302), Http11Cookie.sessionCookie().toString());
return;
}
processFilesWithStatus(outputStream, "/401.html", HttpStatus.valueOf(401));
Copy link

Choose a reason for hiding this comment

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

로그인에 실패할 경우, 401.html리다이렉트 하는 것이 요구사항이였죠!
제가 알기로는, 상태 코드가 3xx가 아닌 경우 Location 헤더가 있더라도 리다이렉트 처리가 되지 않습니다.
제가 직접 확인해 보고 싶은데, 컴파일 에러때문에 확인이 불가능하네요 😢
로그인 실패시 엔드포인트가 /401.html로 변경되는지 체크해 주시고, 만약 그렇지 않다면 상태 코드를 3xx로 변경해 주세요!

Copy link
Author

Choose a reason for hiding this comment

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

리다이렉트 처리가 잘 되는 것을 확인하였습니다!
재확인 부탁드립니다 👍

Comment on lines 181 to 183
String account = queryParams.get("account").get(0);
String password = queryParams.get("password").get(0);
String email = queryParams.get("email").get(0);
Copy link

Choose a reason for hiding this comment

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

시소가 이전 코드에서 값을 꺼내올 때 getFirst()를 사용하신 것을 봤는데, 이 부분도 getFirst를 사용해 주시면 좋을 것 같아요! 👍

Copy link
Author

Choose a reason for hiding this comment

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

반영하였습니다~

Comment on lines 105 to 111
private String readFileType(String path) {
int lastDotIndex = path.lastIndexOf('.');
if (lastDotIndex == -1 || lastDotIndex == path.length() - 1) {
return "";
}
return path.substring(lastDotIndex + 1);
}
Copy link

Choose a reason for hiding this comment

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

굉장히 간단하게 파일 확장자를 지정해 주셨네요!
배워갑니다 👍👍

String email = queryParams.get("email").get(0);
User user = new User(account, password, email);
InMemoryUserRepository.save(user);
processFilesWithStatus(outputStream, "/index.html", HttpStatus.OK);
Copy link

Choose a reason for hiding this comment

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

회원가입에 성공했을 경우도 리다이렉트 되는지 확인해 주세요!

Copy link
Author

Choose a reason for hiding this comment

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

위와 마찬가지로 리다이렉트 처리가 잘 되는 것을 확인하였습니다!
재확인 부탁드립니다 👍

Choose a reason for hiding this comment

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

아래에 코멘트 남겨 두었으니 확인해 주세요!

String method = readMethod(requestLine);
processRegister(method, readBody(reader), outputStream);
}
processFilesWithStatus(outputStream, path, HttpStatus.OK);
Copy link

Choose a reason for hiding this comment

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

[질문]
조건문 프로세스에서 모두 processFilesWithStatus 메서드를 사용해서 outputStream에 response를 보내셨는데,
또 다시 processFilesWithStatus를 호출하신 이유가 궁금합니다!

Copy link
Author

@shin-jisong shin-jisong Sep 10, 2024

Choose a reason for hiding this comment

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

index.html을 response로 전송한 후 웹에서 필요한 css 등을 호출할 때 해당 메서드가 호출됩니다!

if (method.equals("POST")) {
Map<String, List<String>> queryParams = parsingQueryString(body);
User user = InMemoryUserRepository.findByAccount(queryParams.get("account").getFirst()).get();
log.info("user : {}", user);
Copy link

Choose a reason for hiding this comment

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

id와 비밀번호가 모두 일치할 경우 로그를 찍어야 하는데,
현재는 비밀번호가 일치하지 않아도 로그가 찍히는 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다!

@shin-jisong
Copy link
Author

안녕하세요 레모네!
상세히 리뷰를 남겨 주셔서 감사합니다 🙇‍♂️
미션을 바쁘게 구현하는 탓에 놓친 부분이 있었는데 잘 보강한 듯해요 👍
2단계 완성, 학습 테스트 추가, 리뷰 반영, 약간의 리팩토링을 거쳐 재요청 드립니다~

@JiHyeonL
Copy link

JiHyeonL commented Sep 11, 2024

안녕하세요 시소~~!
깔끔하게 마무리 잘 해주셨네요😀

제가 확인하기로는

  • 로그인 성공 & 실패
  • 회원가입 성공

위 경우에 리다이렉트가 되지 않는 것 같아요.

image
  • 로그인 성공시 index 페이지로 넘어가지만 엔드포인트는 여전히 login 인 상태
image
  • 로그인 실패시 401 페이지로 넘어가지만 엔드포인트는 여전히 login 인 상태
image
  • 회원가입 성공시 index 페이지로 넘어가지만 엔드포인트는 여전히 register인 상태

이 부분만 수정해 주신다면 바로 머지 해 드리겠습니다!

@shin-jisong
Copy link
Author

해당 부분을 제대로 체크하지 못했네요!
제대로 구현하였습니다 👍

Copy link

@JiHyeonL JiHyeonL left a comment

Choose a reason for hiding this comment

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

리다이렉션 확인하였습니다!
수고하셨어요~~
다음 단계 미션 진행해 주세요!

@JiHyeonL JiHyeonL merged commit cad72c8 into woowacourse:shin-jisong Sep 11, 2024
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.

3 participants