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

[럿고] HTTP WAS 3단계 미션 제출합니다. #173

Merged
merged 11 commits into from
Nov 2, 2020

Conversation

ksy90101
Copy link

안녕하세요 홍시! 벌써 3단계이네요!

이번에도 몇가지 질문이 있어 같이 적어놓습니다!

  1. 세션에 어떤 값을 저장해야 할까요?
    user를 저장해야하는지, email와 같은 간단한 값을 저장해야 해야 하는지, 또한 user를 저장한다면 dto를 따로 만들어저 저장해야 할까요?

  2. 스프링에서는 생성자에서 주입하지 않고 여기서 바로 만드는데 이와 같은 방법이 괜찮은 방법일까요?

private final AtomicReference<String> id = new AtomicReference<>(String.valueOf(idGenerator.generateId()));

private final Map<String, Object> attributes = new ConcurrentHashMap<>();
  1. 테스트를 위해 MockSession을 이용하면 방법을 택했습니다. 괜찮은 방법일까요?

항상 답변과 피드백 너무 감사드립니다~
이번에도 잘 부탁드리겠습니다!

hwanghe159 and others added 8 commits September 12, 2020 21:13
* docs: README.md 작성

* feat: URL추출 메서드 및 request header 출력 메서드 작성

* feat: /index.html로 접속했을 때 index.html 파일 응답 구현

* feat: 회원가입 시 회원에 대한 정보를 서버에 저장하는 로직 구현

* feat: post 처리 및 요청 값 객체 생성

* feat: 요구사항3 완료

* feat: 302 response 구현

* refactor: 요청에 따른 응답을 컨트롤러로 처리하도록 수정

Co-authored-by: SeYun <ksy90101@naver.com>
* [럿고] 1단계 - HTTP 웹 서버 구현 미션 제출합니다. (woowacourse#101)

* docs: README.md 작성

* feat: URL추출 메서드 및 request header 출력 메서드 작성

* feat: /index.html로 접속했을 때 index.html 파일 응답 구현

* feat: 회원가입 시 회원에 대한 정보를 서버에 저장하는 로직 구현

* feat: post 처리 및 요청 값 객체 생성

* feat: 요구사항3 완료

* feat: 302 response 구현

* refactor: 요청에 따른 응답을 컨트롤러로 처리하도록 수정

Co-authored-by: 황준호 <42054054+hwanghe159@users.noreply.github.com>

* refactor: response 객체 생성

* refactor: header line 추가

* refactor: 필드명 변경, 디미터법칙 따르도록 수정

* refactor: 필드명 변경, RequestBody에 대한 예외처리, stringBuilder에서 append사용

* refactor: service 메서드 추상화

* refactor: QueryParams를 Request 내부로 이동, RequestUri, Path 생성

* feat: 쓰레드풀 적용

* refactor: / 경로 index.html 이동 controller 생성 및 전체 리팩토링

* test: Response에 대한 test작성

* test: controller 테스트 추가

* refactor: 파일 대문자 소문자로 변경

* test: 테스트 fail 해결, request, response파일 추가

* refactor: 지원하는 않는 메서드 예외 처리 리팩토링

* refactor: 리팩토링

- Header enum 삭제
- 컨트롤러 접근제어자 변경
- IllegalRequestException 메시지 변경
- 상수 이름 변경

* refactor: response test를 ByteArrayOutputStream으로 변경

Co-authored-by: SeYun <53366407+ksy90101@users.noreply.github.com>
Co-authored-by: SeYun <ksy90101@naver.com>
Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

3단계 요구사항 구현도 깔끔하게 잘 해주셨네요! 👍
몇 가지 피드백 남겼으니, 확인하셔서 반영해주세요!

Comment on lines 18 to 22
TemplateLoader loader = new ClassPathTemplateLoader();
loader.setPrefix(PREFIX);
loader.setSuffix(SUFFIX);

Handlebars handlebars = new Handlebars(loader);
Copy link

Choose a reason for hiding this comment

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

해당 객체는 한 번 초기화해 재사용하면 효율적이지 않을까요~?


String id = httpSession.getId();

response.setHeader(HttpHeaders.SET_COOKIE, "JSESSIONID=" + id + "; Path=/");
Copy link

Choose a reason for hiding this comment

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

쿠키를 사용할 때마다 개발자가 직접 매번 쿠키 구조를 만들고, 헤더에 추가를 해야 합니다.
Cookie 라는 클래스를 만들어 쿠키 구조를 관리하고, 사용자가 쿠키 헤더(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.

원하시던 방향이였는지 모르겠네요 ㅠㅠ
일단 구현했습니다~ ㅎㅎ

@Override
protected void doDelete(Request request, Response response) {
try {
String jsessionid = request.getCookie("JSESSIONID");
Copy link

Choose a reason for hiding this comment

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

개발자가 세션의 쿠키명을 기억하고 사용해야 한다면 불편하지 않을까요? 그리고 지금처럼 문자열 그대로 사용한다면 오타를 낼 가능성도 있구요!
이것저것 기억할 필요없이 Request 에 메시지 보내 세션을 반환받도록 바꿔보면 어떨까요?

Copy link
Author

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.

실제로 HttpServletRequest에도 session이 있는걸 확인할수 있네요!

import java.util.HashMap;
import java.util.Map;

public class Cookies {
Copy link

Choose a reason for hiding this comment

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

일급 콜렉션 👍

@@ -0,0 +1,14 @@
package http.session;

public interface HttpSession {
Copy link

Choose a reason for hiding this comment

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

인터페이스 활용 💯

private final static Map<String, HttpSession> httpSessionStore = new HashMap<>();

public static HttpSession getSession(String id) {
HttpSession httpSession = httpSessionStore.get(id);
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.

그렇네요! 이 메서드는 혹시 모를 대비를 위해 남겨놧습니다. 지금 당장은 기능이 필요 없다고 한들, was다 보니 필요할 경우가 생길거 같아서요!

src/main/java/controller/UserListController.java Outdated Show resolved Hide resolved
src/main/java/controller/LoginController.java Show resolved Hide resolved

public class WebSession implements HttpSession {

private Map<String, Object> attributes = new HashMap<>();
Copy link

Choose a reason for hiding this comment

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

스프링에서는 생성자에서 주입하지 않고 여기서 바로 만드는데 이와 같은 방법이 괜찮은 방법일까요?

저는 지금처럼 별도의 생성자가 없다면 필드에서 초기화하는게 코드가 한 눈에 들어와 더 좋다고 생각합니다.
물론 상황, 취향에 따라 다를 수 있습니다 :)

자세한 설명은 제 생각과 비슷한 답변이 있어 링크로 대체합니다.


DataBase.addUser(pobi);
DataBase.addUser(jun);
MockSession mockSession = new MockSession(null, "1");
Copy link

Choose a reason for hiding this comment

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

Session은 럿고가 개발한 웹 프레임워크에 포함된 기능이기 때문에 이 프레임워크를 사용해 개발하는 사용자 입장에선 통제할 수 없는 요인입니다.
이렇게 랜덤 혹은 외부의 통제할 수 없는 요인을 테스트하기 위해 Mock을 사용하는 것은 괜찮은 방법이라고 생각합니다 :)

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

피드백 반영을 아주 깔끔하게 잘 해주셨네요! 👍
간단한 피드백 남겼으니 확인 후, 다음 단계 진행해주세요~!

Comment on lines +16 to +18
if (httpSession.getAttribute("email") == null) {
throw new AuthenticationException();
}
Copy link

Choose a reason for hiding this comment

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

Session 내의 로그인 여부를 판별하는 클래스 혹은 메서드를 만들어서 관리해도 좋을 것 같아요!
그럼 여러 곳에 흩어져 있는 체크 로직을 한 곳에 모을 수 있지 않을까요?

Comment on lines +30 to +34
this.httpSession = new WebSession();
if (!"".equals(cookies.getCookieValue(WebSession.DEFAULT_SESSION_COOKIE_NAME))) {
this.httpSession = HttpSessionStore.getSession(
cookies.getCookieValue(WebSession.DEFAULT_SESSION_COOKIE_NAME));
}
Copy link

Choose a reason for hiding this comment

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

생성된 세션이 있으면 불필요한 WebSession 이 계속 생성되지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

정적 필드로 빼서 처리했습니다! 그렇네요 ㅎㅎ

User user = DataBase.findUserById(requestBodies.get("userId"));
validate(requestBodies, user);

HttpSession httpSession = HttpSessionStore.create();
Copy link

Choose a reason for hiding this comment

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

명시적인 create() 호출로 책임을 구분하는 것도 좋지만, 프레임워크에서 세션 생성을 알아서 처리해준다면 해당 프레임워크를 사용하는 개발자가 세션 생성 유무를 신경쓸 필요가 없지 않을까요?

HttpSession httpSession = request.getSession(); // 생성된 세션이 없으면 자동 생성

Copy link
Author

Choose a reason for hiding this comment

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

스프링 시큐리티를 참고해보니 boolean값을 주고 세션을 생성할지 안할지를 선택하는 부분을 보고 처리했습니다 👍

@hongsii hongsii merged commit c709ec7 into woowacourse:ksy90101 Nov 2, 2020
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.

None yet

3 participants