[1, 2단계 - Tomcat 구현하기] 리니(이예린) 미션 제출합니다. #584
Conversation
- 로그인 수행 시 POST로 바뀜에 따라 쿼리 스트링에서 requestBody로 변경 - Controller 추상 클래스로 http 메서드별 추상 메서드 제공 - Controller 추상 클래스에서 http 메서드에 따라 올바른 메서드 호출하여 연산 - 만약 존재하지 않는 메서드라면 405로 예외 처리
- contentType에 무관하게 EOF일때까지 body를 읽도록 구현하여 서버에서 무한히 기다리는 오류 발생 - 해당 오류 해결에 용이하게 하기 위해 httpRequest 분리
ashsty
left a comment
ashsty
left a comment
There was a problem hiding this comment.
안녕하세요, 리니! 이번에 리뷰어로 만나게 된 애쉬입니다...
우선 코멘트 개수 보고 조금 놀라실 수도 있는데요.
진짜 엄청나게 자잘한 질문과 제안이 전부입니다! 너무 놀라지 말아주세요...
코드를 애초부터 굉장히 깔끔하게 짜주셔서 기능적으로 개선을 제안할 만한 부분이 별로 없었던 것 같아요. 리뷰를 위해 되려 제가 공부를 더 많이 해야했을 정도로 배울 점이 많은 코드인 것 같습니다. 😊
커밋 기록을 보면서 리니가 어떤 과정의 고민을 거쳐 현재의 코드를 완성했는지도 명확히 알 수 있어서 좋았어요! 👍
그래도 자잘자잘한 코멘트를 남기다 보니 양이 꽤 많아져서 이번에는 테스트 코드 관련 리뷰는 남기지 않았습니다. 리니도 다른 테스트를 추가하신다고 했으니 관련 리뷰는 다음에 함께 하도록 할게요!
또, 제가 제안드리는 내용이 그렇게 필수적으로 반영되어야 하는 부분도 아니라고 생각될 뿐더러 다음 단계인 3단계가 리팩토링이잖아요. 이번 리뷰 내용 반영과 충분히 함께 병행할 수 있는 부분이라고 생각해 이번 1, 2단계는 우선 머지하려고 해요. 확인해주시고 괜찮으시다면 디엠 보내주세요! 바로 머지해드리도록 하겠습니다.
이번 미션은 4레벨 첫 미션인 만큼 이래저래 힘드셨을 거라고 생각하는데,
고생 많으셨습니다! 다음 한 주도 함께 잘 헤쳐나가 보아요 :D
| List<String> actual = Files.lines(path).toList(); | ||
| actual.stream().forEach(System.out::println); | ||
|
|
||
| assertThat(actual).containsOnly("nextstep"); |
There was a problem hiding this comment.
[선택] Files.lines(path) 는 파일을 스트림으로 읽기 때문에 별도 리소스 관리가 필요할 수 있어요!
try-with-resources를 사용해보는 건 어떨까요?
| List<String> actual = Files.lines(path).toList(); | |
| actual.stream().forEach(System.out::println); | |
| assertThat(actual).containsOnly("nextstep"); | |
| try (Stream<String> lines = Files.lines(path)) { | |
| List<String> actual = linew.toList(); | |
| actual.forEach(System.out::println); | |
| assertThat(actual).containsOnly("nextstep"); |
| * File, Files 클래스를 사용하여 파일의 내용을 읽어보자. | ||
| */ | ||
| /** | ||
| * Path.of() VS new File().toPath() |
| */ | ||
| // write() 메서드 내부적으로는 자동으로 크기가 조정되는 바이트 배열에 데이터를 저장하고 있다. | ||
| outputStream.write(bytes); | ||
| String actual = outputStream.toString(); |
There was a problem hiding this comment.
[선택] 인코딩 형식을 명시적으로 지정할 수도 있을 것 같아요!
| String actual = outputStream.toString(); | |
| String actual = outputStream.toString(StandardCharsets.UTF_8); |
There was a problem hiding this comment.
toString()은 인자로 아무것도 받지 않는데, 보충 설명 해주실 수 있을까요?
There was a problem hiding this comment.
다른 부분 코드 리뷰 남기려다 헷갈린 부분 같아요...!
미안합니다~~~~ ㅠㅠ 😭😭
| */ | ||
| try (inputStream) { | ||
|
|
||
| } catch (IOException e) { |
There was a problem hiding this comment.
try-catch에서 예외를 처리하지 않고 빈 블록을 유지하는 건 안티 패턴이라고 합니다!
물론 이 경우는 간단한 테스트 코드긴 하지만용!
empty catch block 키워드로 검색해보시면 좋을 것 같아요! 👍
There was a problem hiding this comment.
저도 동의합니다.!
학습 테스트라서 우선은 빈 캐치 블럭으로 두었습니다.! 짚어줘서 고마워요👍
| final InputStream bufferedInputStream = new BufferedInputStream(inputStream); | ||
|
|
||
| final byte[] actual = new byte[0]; | ||
| final byte[] actual = bufferedInputStream.readAllBytes(); |
| return response; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
파일 중에 EOL이 조금 섞여 있는 것 같아요! 👍
| } catch (UnauthorizedException e) { | ||
| log.error("Error processing request for endpoint: {}", request.getURI(), e); | ||
|
|
||
| return redirect("401.html", new HttpResponse()); |
There was a problem hiding this comment.
redirect를 호출할 때마다 new HttpResponse()로 새로운 response를 메서드 외부에서 생성하고 있는 것 같아요.
RegisterController와 동일하게 메서드 내부에서 처리하는 건 어떨까요?
There was a problem hiding this comment.
로그인 성공 시 쿠키 세팅 때문에 메서드 인자를 다르게 가져갔었는데요!
redirect(String location)으로 해당 메서드를 오버 로딩하는 방향으로 리팩터링 했습니다:>
| private String readResource(String fileName) throws IOException { | ||
| URL resource = findResource(fileName); | ||
| if (Objects.isNull(resource)) { | ||
| throw new UncheckedServletException("Cannot find resource with name: " + fileName); | ||
| } | ||
| Path path = new File(resource.getFile()).toPath(); | ||
| return Files.readString(path); | ||
| } | ||
|
|
||
| private URL findResource(String fileName) { | ||
| return getClass().getClassLoader().getResource("static/" + fileName); |
There was a problem hiding this comment.
[선택] 이 기능은 NotFoundController에서도 동일하게 구현된 메서드 같아요.
별도 유틸 클래스로 구현해보는 건 어떨까요?
There was a problem hiding this comment.
Resource라는 별도 유틸 클래스를 분리했습니다:>
There was a problem hiding this comment.
현재 잘못된 아이디로 로그인 요청을 하는 경우에도 401 페이지로 연결은 되지만
응답값이 302 FOUND로 설정되는데요.
리니가 의도한 부분일까요?
There was a problem hiding this comment.
오류 원인은 401이지만, 작업 내용은 401 페이지로 리다이랙트 하는 것이라서 302 상태 코드를 사용했는데요,
4xx 오류 코드를 반환할 상황과 302로 반환할 상황에 대한 기준을 아직 세우지 못했습니다😭
There was a problem hiding this comment.
ㅎㅎㅎ 언젠가 리니의 기준이 명확해지면 한 번 들어보고 싶네요!! 👍👍
There was a problem hiding this comment.
[질문]
현재 세션 응답을 확인해보면 위와 같은 값을 확인할 수 있었는데요.
그럼 한 번 생성된 세션은 얼마나 오랫동안 유지되는 걸까요?
There was a problem hiding this comment.
지금은 세션에 만료 시간을 설정해두지 않았기 때문에, 영구적으로 유지되는 걸로 알고 있습니다!
ashsty
left a comment
ashsty
left a comment
There was a problem hiding this comment.
안녕하세요 리니~
지난 리뷰사항이 굉장히 많았죠? ㅠㅠ
반영하느라 너무 힘드시진 않으실까 굉장히 걱정을 많이 했었는데 짧은 시간 만에 아주 깔끔한 리팩토링 & 꼼꼼한 피드백 반영이 이루어진 것 같아요. 세션 쪽이나 쿠키, 테스트 쪽도 많이 보강되었네요! 너무 고생 많으셨습니다.
커밋 단위도 피드백 크기로 작게 나눠 해주셔서 리뷰가 정말 편했어요. (리니 짱!)
이번 리뷰에서는 지난번에 말씀 드렸던 대로 테스트 관련 피드백(정말 사소합니다.) 몇 가지,
그리고 defaultCharset 메서드 관련한 코멘트 하나 남겨두었습니다.
후자의 경우는 저도 공부하며 찾아본 내용이다 보니 제가 틀린 이야기를 하고 있는 것 같다면 언제든지 DM이나 코멘트로 의견 주셔요!
남긴 피드백이 사소하기도 하고, 이미 3단계 요구사항이 어느 정도 충족되어 있는 상태다 보니 남은 리팩토링과 피드백 반영은 3단계 마저 진행하시면서 함께 처리하셔도 될 것 같아요.
1, 2단계 PR은 우선 머지해보도록 하겠습니다.
정말 고생하셨어요.
남은 리팩토링도 함께 힘내보아요! 💪💪
| return response; | ||
| } catch (InvalidResourceException e) { | ||
| log.error("Error processing request for endpoint: {}", request.getURI()); | ||
| log.error("Error processing request for endpoint: {}, message: {}", request.getURI(), e.getMessage()); |
| String[] keyValue = pair.split("="); | ||
| if (keyValue.length == 2) { | ||
| bodyParams.put(keyValue[0], keyValue[1]); | ||
| String key = URLDecoder.decode(keyValue[0], Charset.defaultCharset()); |
There was a problem hiding this comment.
Charset.defaultCharset()과 관련해서는 여기와 관련 있는 이야기가 될 것 같아요!
| return 0; | ||
| } | ||
| return body.getBytes().length; | ||
| return body.getBytes(Charset.defaultCharset()).length; |
There was a problem hiding this comment.
😶 (혹시 defaultCharset 관련 수정을 원하실까봐 마킹해둡니다!)
| @DisplayName("확장자를 기준으로 MIME 타입을 판단하여 반환한다.") | ||
| @ParameterizedTest | ||
| @CsvSource({"index.html, text/html;charset=utf-8", "styles.css, text/css", "error.svg, image/svg+xml"}) | ||
| @CsvSource({"index.html, text/html;charset=utf-8", "styles.css, text/css", "error.svg, image/svg+xml", "null, application/octet-stream"}) |
| return Files.readString(path); | ||
| } | ||
|
|
||
| private static URL findResource(String fileName) { |
There was a problem hiding this comment.
[질문] 기존의 readResource와 findResource 메서드가
각각 Resource 클래스의 read, findResource 메서드가 된 것 같은데요!
전자의 메서드는 이름이 변경되었는데 후자의 메서드는 그대로 남겨둔 리니만의 이유가 있을까요?
(궁금증입니다!)
There was a problem hiding this comment.
전자는 Static 메서드이기 때문에 Resource.read와 같은 형식으로 사용을 지향합니다. 따라서, 명시된 클래스명에서 무엇을 읽는 것인지 보인다고 생각하여 메서드명을 변경하였습니다.
findResource는 Resource 내부에서 사용하는 private method인데요, 따라서 명시적으로 Resource를 fileName으로 찾아온다는 것을 명시하고자 그대로 두었습니다!
There was a problem hiding this comment.
expected 문자열 생성 로직처럼 중복되어 나타나는 로직들이 조금 있는 것 같아 보여요.
리팩토링에서는 중복을 줄여보아도 좋겠네요! 👍
There was a problem hiding this comment.
[선택] 로직이 너무 간단해서 작성하지 않으신 것 같지만 세션 생성이나 삭제 테스트도 있으면 좋을 것 같긴 하네요! 👍
|
|
||
| class HttpCookieTest { | ||
| @Test | ||
| @DisplayName("유효한 쿠키 헤더로 HttpCookie를 생성한다.") |
There was a problem hiding this comment.
유효하지 않은 헤더(e.g. null, 잘못된 형식...)로 HttpCookie를 생성하려고 시도하면 어떻게 되나요?
이런 케이스의 테스트가 추가되어도 좋겠네요 👍👍
3 participants
애쉬, 안녕하세요!
저번에 레벨 인터뷰 때 이후로 이렇게 다시 보게 됐네요:)
이번에 1-2단계 위주로 진행한다고 했는데, 코드를 구현하면서 리팩터링을 병행했더니 3단계가 좀 섞여 있습니다😭
쿠키와 세션 관련된 부분은 시간이 촉박해서 커밋 단위를 신경쓰지 못했습니다. 관련 테스트도 작성하지 못해서 우선은 로컬 서버에서 테스트 후 리뷰 요청 드립니다. 추후에 관련 테스트는 추가하도록 하겠습니다.
추가로, 스레드와 캐시 관련 학습 테스트는 아직 수행하지 못했습니다. pr은 열려있는 상태에서 학습 테스트는 추가적으로 수행해보려고 합니다. 리뷰는 학습 테스트 외 나머지에 중점적으로 부탁드립니다!
톰캣(WAS) 개념이 부족한 상태로 미션을 시작해서, 이론을 함께 공부하면서 했습니다. 공부한 내용을 코드에도 반영시켜보려고 노력은 했지만, 이상한 부분이 있는 것 같다면 얼마든지 피드백 부탁드립니다.
2주간 리뷰 잘 부탁해요, 애쉬❣️