-
Notifications
You must be signed in to change notification settings - Fork 388
[HTTP 서버 구현하기 - 2, 3단계] 현구막(최현구) 미션 제출합니다. #77
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
Conversation
Controllers 쪽에 URI 매칭 책임이 옮겨감
패키지 controller에서 server로 위치도 변경
첫 로그인이라면 로그인 후 200 OK과 함께 index.html로 이동 이미 로그인 했다면 302 FOUND와 함께 indexh.html로 이동
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 단계에서 충분히 구현을 잘해두셔서, 이번 단계는 크게 문제가 될 부분들이 보이지 않았네요 ㅎㅎ
잘하셨습니다.
주요 로직에서는 문제가 있는부분이 따로 보이지 않네요.
대신 코드를 보면서 몇가지 의문점이나 고민이 생기는 부분들에 대해서 코멘트를 남겨두었는데, 꼭 반영하실 필요는 없어요.
현구막이 보고 수정할 필요성이 느껴지지 않으시다면, 이에 대한 현구막의 의견을 남겨주세요.
확인후 Approve 해드리겠습니다!
번외로 지금 잘못된 url을 /을 중첩된 상황으로 받을시,
html만 로드되고 다른 컨텐츠들이 깨집니다.
원하신다면, 한번 원인파악을 해보셔도 좋겠어요.
이부분은 반영하지 않으셔도 괜찮습니다. (제 코드도 방금확인해보니 터지네요 ㅋㅋㅋㅋㅋㅋ)
| return HttpResponse.redirect(FOUND, "/index.html"); | ||
| } | ||
|
|
||
| LoginRequest loginRequest = getLoginRequest(httpRequest); | ||
| loginService.login(loginRequest); | ||
| LoginResponse loginResponse = loginService.login(loginRequest); | ||
|
|
||
| LOGGER.debug("Login Success."); | ||
|
|
||
| return HttpResponse.redirect(HttpStatus.FOUND, "/index.html"); | ||
| StaticResource resource = staticResourceService.findByPath("/index.html"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index.html이 이제 슬슬 중복해서 쓰이네요. 정적 페이지 리소스들은 이제 ENUM 으로 관리해보시는거 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 생각이에요!
| private Long autoIncrementId; | ||
| private long autoIncrementId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전에 리뷰 남겨드린거 잘 적용해주셧네요!
여담으로 이런 키값은 Atomic 해야하는데, 만약 save가 동시에 불릴경우, 문제가 발생할수도 있을꺼 같다는 이야기와, 해결책을 공유받은게 있어서 현구막에게도 공유해드려요.
AtomicLong이라는게 있는데 이친구는 쓰레드 세이프 하다고 하네요!
이부분은 어차피 디비가 임시 방편적인 부분이라 중요한건 아니라 적용하실 필요는 없고 이런게 있다~ 정도 지식을 더 얻고 넘어가심 되겠어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoIncrementId 스레드 세이프에 관해서 생각해보지 못했어요! 👍 👍
심지어 database필드가 ConcurrentHashMap으로 선언되어 있는걸 봐놓고도... 허허허
database필드가 ConcurrentHashMap 으로 선언되어서 스레드에 안전하다곤 하지만
InMemoryUserRepository.save(User user) 메서드에는 단순하게 put하고 끝내버려서 살짝 위험해보이기도 하네요.
(그보다 앞 단의 RegisterService 에서 검증을 진행하고 있는데, 이것만으로는 불충분하다는 생각)
InMemoryUserRepository.save(User user) 쪽으로 검증을 옮겨서 최대한 '삽입되기 직전 순간'에 검증을 진행하도록 해야겠어요!!
| LOGGER.info("Parsed HTTP Request!!!\n\n{}\n\n", httpRequest); | ||
| HttpResponse httpResponse = controllers.doService(httpRequest); | ||
| HttpResponse httpResponse = requestMapping.doService(httpRequest); | ||
| LOGGER.info("Return HTTP Response!!!\n\n{}\n\n", httpResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗, 이 부분은 제 불찰이 맞아요!!! 사실 이렇게 구성하게 된 단계가 있어요 ...
- 테스트코드에서 response body에 담겨오는 static resource의 내용을 확인하고 싶었다.
- 테스트코드에 찍히는 로깅 내용이 지나치게 길어서! 테스트코드용
.html파일들을 따로 짧게 만들어서 로깅해봤다. - '적당히 짧고 예쁘게 나오네!!' 하고 만족한 후, 로깅을 그대로 프로덕션 코드에 적용했다.
그렇게 지금 사태가 벌어졌습니다 😅
HTTP access는 중요한 정보지만, body 값을 모두 로깅하기엔 지나치게 긴거같네요.
헤더까지만 로깅 하도록 변경해보겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpRequest의 경우 body 값을 raw하게 확인하는게 더 좋은거 같아서 현 상태를 유지하고,
HttpResponse에 대해서만 헤더까지만 로깅하도록 변경했어요!
| private void flushBytes(OutputStream outputStream, HttpResponse httpResponse) | ||
| throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저에겐 이전것이 훨씬 좋아 보여요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 백번천번 동의합니다... 저건 자동 코드 리포맷팅이 만들어낸 작품... 😞
일부러 HardWrap 라인 신경쓰지 않고 조금씩 넘겨서 코드 작성하다가 코드 리포맷팅 싹 돌렸더니...
너잘이 공유해주신 이야기인데, GoogleJavaStyle 기준 HardWrap 값 100이 과거 1024 * 720 해상도에 맞춘 값이라고 하더라구요. 그런데 요즘 모니터 크기도 커지고 해상도도 늘어남에 따라 HardWrap을 조금씩 늘리는 추세라고 해요!
저도 120 정도로 늘려서 다시 리포맷팅 해볼게요!!
| public Object findObject(String sessionId, String attributeName) { | ||
| if (sessions.containsKey(sessionId)) { | ||
| HttpSession httpSession = sessions.get(sessionId); | ||
| return httpSession.getAttribute(attributeName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분을 보고 조금 이상하다고 느껴서 왓는데, findObject는 세션 여부를 찾아서, 리턴하네요.
그런데 그 리턴값을 사용하는게 아니고 리턴값을 받긴하는데, 예외가 터지면 실패했다 처리하고 별일없으면 통과시키는 로직이에요.
저는 개인적으로 이 과정이 조금 이상하다고 느끼고 있는데, 어떻게 생각하시나요?
차라리 그냥 있는지 없는지 여부를 확인하고 ture, false로 리턴하는게 좀 더 좋지 않을까요?
저는 개인적으로 이렇게 try-catch로 분기를 조절하는것을 선호하진 않아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% 적극 동의합니다! 제가 코드를 리뷰해도 포츈과 같은 이야기를 했을거 같아요.
HttpSessions의 인지부조화에 갖혀서, 'HttpSessions는 원래 LoginService 것이 아닌데 LoginService를 위한 boolean 메서드가 생기는게 올바른 일인가?' 라는 생각이 들었고 위와 같이 try-catch 를 이용한 코드를 작성했어요.
만일 HttpSessions가 제가 만든 객체가 아닌 제공되는 라이브러리라고 해도, HttpSessions를 한 번 더 Wrapping해서 LoginService에서 사용하기 적합한 형태로 변환하는게 더 좋은 방향 같아요!
있는지 없는지 여부를 boolean 형태로 반환하는 형식으로 바꿔보도록 하겠습니다!
(다른 곳에선 has**** 메서드를 잘만 만들어놓고, 여기선 try-catch를 떠올렸다는게 웃프네요 😂 )
|
|
||
| public StaticResource findByPathWithExtension(String path, String extension) throws IOException { | ||
| public StaticResource findByPathWithExtension(String path, String extension) | ||
| throws IOException { | ||
| return findByPath(path + extension); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 이전이 더 가독성이 좋다고 생각이 드네요.
| public boolean isAlreadyLogin(HttpCookie httpCookie) { | ||
| try { | ||
| httpSessions.findObject(httpCookie.getParameter(SESSION_PARAMETER), "user"); | ||
|
|
||
| return true; | ||
| } catch (SessionNotFoundException | SessionAttributeNotFoundException | QueryParameterNotFoundException e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpSessions 의 findObject 리뷰와 같이 묶어서 봐주심 좋겠어요,
| public LoginResponse login(LoginRequest loginRequest) { | ||
| User user = findByUserAccount(loginRequest.getAccount()); | ||
| user.checkPassword(loginRequest.getPassword()); | ||
|
|
||
| UUID uuid = UUID.randomUUID(); | ||
| HttpSession httpSession = new HttpSession(uuid.toString()); | ||
| httpSession.setAttribute("user", user); | ||
| httpSessions.addSession(httpSession); | ||
|
|
||
| return new LoginResponse(SESSION_PARAMETER, httpSession.getId()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이부분을 보면서 고민을 많이했네요.
지금 구조는 LoginService에 HttpSessions 를 주입해주셨다곤 하지만,
LoginService를 만드는 RequestMapping의 loadContext() 를 보면 처음엔 httpSessions는 텅 비어있는 상태에요.
https://github.com/woowacourse/jwp-dashboard-http/blob/136c296c85a7206c780ed30e0c3103997c5bb376/app/src/main/java/nextstep/jwp/server/RequestMapping.java#L31-L32
결국 login 메소드를 호출할때서야 addSession들을 하면서 세션들을 채우는 형태인데, 이게 HttpSessions의 주입으로 볼수 있는건가?
라는 생각이 드네요.
제가 아직 미션을 진행하지 못한상황이라 ㅠ 인사이트가 적어서 이상함을 느끼고 있는걸수도 있긴해서
저도 제나름대로 구현하다가 이부분에 대해서 생각이 바뀌면 다시 말씀드릴게요!
일단 지금 제가볼때는 주입받고 있는 형태라고 보긴 힘들다고 생각이 들어요.
그리고 findByUserAccount 메소드가 한번에 눈에 읽히지 않았네요.
xxx.findBy가 아니다보니 무엇을 찾는지 눈에 안들어왔어요
의도하신건 findUserByAccount 인게 아닐까 싶네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
결국 login 메소드를 호출할때서야 addSession들을 하면서 세션들을 채우는 형태인데, 이게 HttpSessions의 주입으로 볼수 있는건가? 라는 생각이 드네요.
저도 고민이 정말 많았고 지금 구조에 확신을 못느껴서 포츈이 의문을 가지시는게 당연하다 생각합니다 😰
제가 생각한 것들에 대해 조금 더 풀어보자면.. HttpSessions는 HttpSession을 채우는 빈 책꽂이 정도로만 생각했어요. (말 그대로 Session들을 저장하는 공간) 그래서 그 책꽂이가 얼마나 채워져있는지 상태에 관계없이 LoginService는 책꽂이를 전달 받은대로 사용 할 수 있게 구성했어요. User 객체를 HttpSession에 담아내야했으므로 LoginService에 위치하는게 최선인거 같았구요! (Controller 너머까지 User객체가 나가는 게 가장 최악이라 생각했어요.)
이렇게 구성하고 나니 인지부조화가 찾아왔었는데요, HttpSessions가 마치 LoginService에 속한 개념처럼 느껴졌어요.
제가 알고 있는 세션 저장소는 톰캣, 즉 저희 코드 기준 WebServer-RequestMapping-RequestHandler 사이 어딘가에 속해있는데
'비즈니스로직을 담당하는 LoginService가 이걸 품는게 말이 되나?' 라는 인지부조화였어요. 이것 때문에 하루를 고민했습니다 😭
그래서 '주입' 이라는 표현을 다시 생각해봤어요. 이전에 제가 생각하는 주입은 장난감에 배터리를 뺏다꼈다 하는 느낌이었는데요,
배터리의 원래 위치를 '장난감'이 아닌, '배터리 충전기'로 바꿔서 생각해봤어요!
배터리의 원래 위치는 원래 배터리 충전기고, 장난감은 편의를 위해서 배터리를 빌려다 쓴다.
HttpSessions의 원래 위치는 톰캣이고, LoginService는 User 객체를 담기 위해 HttpSessions를 주입받아 쓴다.
이렇게 생각하고 나니 인지부조화가 해결되는거 같아서, 현재 구조를 구성하게 됐어요!!
제 생각이 잘 전달될지 모르겠네요 😰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 findByUserAccount 메소드가 한번에 눈에 읽히지 않았네요.
xxx.findBy가 아니다보니 무엇을 찾는지 눈에 안들어왔어요
의도하신건 findUserByAccount 인게 아닐까 싶네요
👍 👍 👍 (웨지식 따봉난무)
같은 Service 에 속하는 개념이라 단순하게 생각했는데,
포츈이 말씀해주신 대로 findUserByAccount가 훨씬 직관적인거 같아요.
단어 순서가 바뀌는걸로 직관성이 눈에 띄게 올라가네요 허허!!!
RequestMapping의 findByUri도 findControllerByUri로 변경해두었습니다! 👍 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 미션을 직접 구현해보고 보니까, HttpSessions를 Static하게 관리하지 않으셔서
어떻게 해결할까 고민하시다가 이런 구조가 된것같네요 ㅋㅋ 충분히 이해가 갔습니다!
ID 값이 멀티 스레드에 안전하도록 변경 User register 시도시 중복 account 검증을 register 직전에서 수행하도록 변경
HardWrap 100 -> 120 증가
|
Kudos, SonarCloud Quality Gate passed! |
|
포츈! 피드백 주신 사항 반영해보았어요!! ' try-catch를 이용한 분기처리에 대해서는 입이 100개여도 할 말 없습니다!! 👍 👍 👍 👍 👍 좋은 리뷰 감사합니다!! 이번 리뷰도 잘 부탁드려요!!!! |
unluckyjung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더이상 리뷰 할것이 보이지 않네요. 리뷰 반영 하시느라 수고많으셨습니다!
테스트 커버리지까지 훌륭하네요.
남겨주신 의견도 잘 읽어보았어요.
저희가 DB를 사용하는 환경이 아니다보니, 애매한 구조가 나온것으로 판단되네요 ㅎㅎ;
다음 미션도 화이팅 입니다!
| public boolean isAlreadyLogin(HttpCookie httpCookie) { | ||
| try { | ||
| httpSessions.findObject(httpCookie.getParameter(SESSION_PARAMETER), "user"); | ||
| String sessionId = httpCookie.getParameter(SESSION_PARAMETER); | ||
|
|
||
| return true; | ||
| } catch (SessionNotFoundException | SessionAttributeNotFoundException | QueryParameterNotFoundException e) { | ||
| return false; | ||
| } | ||
| return httpSessions.hasObject(sessionId, "user"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
훨씬낫네요!












헤이 포츈! 디스 피알 이스 포유! 🙌
이번 2, 3단계에서 2단계는 저번 PR에 거의 충족했더라구요!
3단계를 진행하면서 크게 3가지 고민이 있었어요.
1. HttpSessions는 어디에?
'
HttpSessions를 어디에 두어야 하는가?' 때문에 거의 하루를 고민했던거 같아요...정말매우많이 고민 하다가 결국
LoginService에 주입을 했는데, 그 이유는 주입되는 객체가 꼭 그 곳에 존재한다는 건 아니다 라는 생각이 들었기 때문이에요.HttpSessions는LoginService에 속하는 개념이 아니지만, 편의를 위해서 주입될 수 있다고 생각하니 인지부조화가 해결되면서 현재와 같이 구성할 수 있었어요! (물론 이게 괜찮은 방식인진 모르겠지만 😅)2. HttpStatus 302가 아니면 브라우저가 무시를 해요
기존 401 Unauthorized, 409 Conflict 등을 통해 리다이렉팅 동작을 유도한게 잘 된다고 생각했는데, 어제 로컬 테스트를 진행하니까 안되더라구요?!
브라우저단에서 302 FOUND 상태코드가 아니라면 Location 헤더를 포함하고 있어도 무시하는거 같았어요.
그렇다고 409.html, 401.html 을 전달하면서 302 FOUND 상태코드를 사용하는건 영 찝찝해서...
401, 409 상태코드의 Response Body에 409.html, 401.html을 포함해서 전달하는 방식으로 바꿨어요!!
RFC2616 문서에도 1xx, 204, 304를 제외하곤 Body를 포함해선 안된다는 제한이 없어서, 안심하고 적용했습니다!
3. 컨트롤러에서 Response를 파라미터로 주지 않고 return 하는 형태
이번 구현에서 컨트롤러에 Response를 파라미터로 주지 않고, 컨트롤러에서 Response를 만들어 반환하는 형태가 어떤 문제를 가지는지 깨달을 수 있었어요.
Request와 Response를 RequestMapping, Interceptor, Controller 등 여러 단계를 거치면서 미리 설정할 수 있다는 장단점을 모두 포기하게 되었고, 특히나 1번으로 이야기했던 'HttpSessions를 어디에 두어야하는가?'에 대해 고민이 길어지는 문제가 발생했었어요.
기존 HTTP 서버 프레임워크들이 채택한 방식(void한 메서드, Request/Response 파라미터)은 Request와 Response를 객체가 아닌 단순한 텍스트 덩어리로 취급한다는 느낌을 받아서 저만의 방식으로 풀어내려 해본건데, 왜 그런 방식을 채택했는지 깨닫는 시간이었습니다 😭 ...
이번 리뷰도 잘 부탁드려요!!!!!! 🙇♂️ 👍 👍 👍
테코톡도 화이팅!!