[2단계 - 로그인 구현하기] 상돌(이상진) 미션 제출합니다. #646

pricelees · 2024-09-12T07:20:24Z

프린 안녕하세요! 제출이 늦어서 죄송합니다 ㅎㅎ;;

전에 말씀드린대로 코드가 다 바뀌었어요..ㅎㅎ.. 분량이 많아져서 죄송해요.

시간 여유 생기면 커피라도 대접하겠습니다 🙇 귀한 시간 내주셔서 감사해요 😄

GIVEN53

안녕하세요 상돌~ 구현하느라 고생 많으셨습니다
크리티컬한 오류가 있어서 확인 부탁드려요
탭 사이즈도 확인해주세요!

브라우저에 세션이 없을 때 GET /login 요청 시

POST /login 요청 시 Set-Cookie 헤더 누락

GIVEN53 · 2024-09-12T08:03:07Z

study/src/main/java/cache/com/example/cachecontrol/CacheWebConfig.java

+    private WebContentInterceptor getNoCacheInterceptor() {
+        CacheControl cacheControl = CacheControl.noCache().cachePrivate();
+
+        return createWebContentInterceptorByCache(cacheControl, "/");


루트 경로에만 적용되지 않을까요?

루트 경로에만 적용되지 않을까요?

학습 테스트의 testNoCachePrivate()가 휴리스틱 캐싱을 적용해보는 것이 의도라고 생각해서 정해진 경로만 지정하였습니다 😄

GIVEN53 · 2024-09-12T08:09:31Z

tomcat/src/main/java/com/techcourse/Application.java

-        final var tomcat = new Tomcat();
-        tomcat.start();
-    }
+	public static void main(String[] args) {


탭사이즈 몇으로 사용하고 계신가요??

탭사이즈 몇으로 사용하고 계신가요??

4로 사용하고 있습니다 ㅎㅎ 탭사이즈에 제약이 있었나용?

제약은 없는걸로 아는데 원래 있던 코드의 탭 사이즈가 달라져서 여쭤봤어요!

제약은 없는걸로 아는데 원래 있던 코드의 탭 사이즈가 달라져서 여쭤봤어요!

인텔리제이 포맷 파일을 https://naver.github.io/hackday-conventions-java/ 를 쓰고있어요😄

GIVEN53 · 2024-09-12T08:18:22Z

tomcat/src/main/java/com/techcourse/web/handler/Handler.java

+	}
+
+	default HttpResponse redirect(HttpResponseHeader header, String location) {
+		header.addHeader("Location", location);


상수로 추출해볼까요? 헤더 관련 상수가 이곳저곳 흩어져있다면 enum으로 관리해도 좋겠습니다

상수로 추출해볼까요? 헤더 관련 상수가 이곳저곳 흩어져있다면 enum으로 관리해도 좋겠습니다

해당 클래스와 직접적으로 연관이 있다고 판단되는 값만 상수로 지정하게 되었는데.. 관리 포인트를 생각하면 enum으로 정의하는게 낫겠네요~~! 바로 반영할게요😄

GIVEN53 · 2024-09-12T08:24:18Z

tomcat/src/main/java/com/techcourse/web/handler/HandlerMapper.java

+		return handlers.stream()
+			.filter(handler -> handler.isSupport(requestLine))
+			.findFirst()
+			.orElseThrow(() -> new IllegalArgumentException("handler not found. " +


Http11Processor에서 IllegalArgumentException을 처리하지 않고 있습니다
예외대신 404 페이지를 바라보는 NotFoundHandler를 구현해서 리턴하면 어떨까요?

Http11Processor에서 IllegalArgumentException을 처리하지 않고 있습니다 예외대신 404 페이지를 바라보는 NotFoundHandler를 구현해서 리턴하면 어떨까요?

텔레파시 🤜🤛

GIVEN53 · 2024-09-12T08:29:43Z

tomcat/src/main/java/org/apache/coyote/http11/http/response/HttpResponse.java

+		return new HttpResponse(new HttpResponseStartLine(HTTP_VERSION, HttpStatusCode.OK), header, body);
+	}
+
+	public static HttpResponse notFound(HttpResponseHeader header, HttpResponseBody body) {


ok()와 더불어서, usages를 확인했을 때 모두 추가적인 로직없이 단순히 외부에서 header 객체를 생성 후 넘겨주고 있습니다
이렇게 구현하신 이유가 있을까요??

ok()와 더불어서, usages를 확인했을 때 모두 추가적인 로직없이 단순히 외부에서 header 객체를 생성 후 넘겨주고 있습니다 이렇게 구현하신 이유가 있을까요??

음 지금은 응답 헤더에 값을 넣고있지 않는데 헤더에 값을 넣을 여지는 열어놔야 한다고 생각했읍니다 ~~!

GIVEN53 · 2024-09-12T11:19:15Z

tomcat/src/test/java/com/techcourse/web/handler/HandlerMapperTest.java

+	void findHandler_WhenRequestRootPage() {
+		String requestPath = "/";
+
+		Class<? extends Handler> expectedHandler = RootPageHandler.class;


제네릭인 이유가 있나요?? Handler 인터페이스로 선언해도 되지 않을까요?

제네릭인 이유가 있나요?? Handler 인터페이스로 선언해도 되지 않을까요?

테스트에서 타입 비교에 사용되는데, 이 경우는 제네릭을 안 쓰면 구체 타입을 지정해야 하더라구요. 그런데 생각해보니 runTest의 파라미터만 제네릭을 써도 충분해서 구체 타입을 지정하는 방법으로 수정했습니다😄

GIVEN53 · 2024-09-12T11:21:25Z

tomcat/src/test/java/com/techcourse/web/handler/HandlerMapperTest.java

+
+		Handler handler = HandlerMapper.findHandler(request);
+
+		assertThat(handler).isInstanceOf(expectedHandler);


isExactlyInstanceOf()로 클래스가 정확히 일치하는지 검증하면 신뢰도가 더 높을 것 같습니다

isExactlyInstanceOf()로 클래스가 정확히 일치하는지 검증하면 신뢰도가 더 높을 것 같습니다

쵝오..😌

GIVEN53 · 2024-09-12T11:23:16Z

tomcat/src/test/resources/index.html

@@ -0,0 +1,106 @@
+<!DOCTYPE html>


이 html 파일은 어떤 용도인가요?

이 html 파일은 어떤 용도인가요?

getPath() 에서 NPE가 나오길래 파일을 넣었는데.. 제가 경로 설정을 잘못했군요..ㅎㅎ..

GIVEN53 · 2024-09-12T11:24:19Z

tomcat/src/main/java/com/techcourse/model/User.java

+	}
+
+	private void validateIsNotEmptyValue(String account, String password, String email) {
+		if (account == null || account.isBlank()) {


user 검증 추가 좋네요 👍🏻

user 검증 추가 좋네요 👍🏻

감사링 ㅎㅎ

GIVEN53 · 2024-09-12T11:30:14Z

tomcat/src/main/java/org/apache/coyote/http11/http/request/HttpRequestHeader.java

+			return null;
+		}
+
+		LinkedHashMap<String, List<String>> result = new LinkedHashMap<>();


구체적인 하위 타입을 선언하신 이유가 있을까요?

구체적인 하위 타입을 선언하신 이유가 있을까요?

제가 에러 휴먼이라서요..? ㅋㅋㅋ 감사합니닷

아니요? 상돌은 핸섬 휴먼입니다

아니요? 상돌은 핸섬 휴먼입니다

ㅋㅋㅋㅋㅋ 영광이에요 ㅋㅋㅋㅋ

pricelees

뿌링 안녕하세요~
남겨주신 의견들에 모두 답글 남겨놨고, 반영이 필요한 부분은 모두 반영했습니다.
(커밋을 커멘트 단위로 해서 커밋 로그를 보시는 것도 좋을 것 같아요!)

바쁜 와중에서도 디테일한 리뷰 감사합니다~~🙇

pricelees · 2024-09-12T11:40:20Z

study/src/main/java/cache/com/example/cachecontrol/CacheWebConfig.java

+    private WebContentInterceptor getNoCacheInterceptor() {
+        CacheControl cacheControl = CacheControl.noCache().cachePrivate();
+
+        return createWebContentInterceptorByCache(cacheControl, "/");


루트 경로에만 적용되지 않을까요?

학습 테스트의 testNoCachePrivate()가 휴리스틱 캐싱을 적용해보는 것이 의도라고 생각해서 정해진 경로만 지정하였습니다 😄

pricelees · 2024-09-12T11:42:14Z

tomcat/src/main/java/com/techcourse/Application.java

-        final var tomcat = new Tomcat();
-        tomcat.start();
-    }
+	public static void main(String[] args) {


탭사이즈 몇으로 사용하고 계신가요??

4로 사용하고 있습니다 ㅎㅎ 탭사이즈에 제약이 있었나용?

pricelees · 2024-09-12T11:42:32Z

tomcat/src/main/java/com/techcourse/model/User.java

+	}
+
+	private void validateIsNotEmptyValue(String account, String password, String email) {
+		if (account == null || account.isBlank()) {


user 검증 추가 좋네요 👍🏻

감사링 ㅎㅎ

pricelees · 2024-09-12T11:43:26Z

tomcat/src/main/java/com/techcourse/web/handler/Handler.java

+	}
+
+	default HttpResponse redirect(HttpResponseHeader header, String location) {
+		header.addHeader("Location", location);


상수로 추출해볼까요? 헤더 관련 상수가 이곳저곳 흩어져있다면 enum으로 관리해도 좋겠습니다

해당 클래스와 직접적으로 연관이 있다고 판단되는 값만 상수로 지정하게 되었는데.. 관리 포인트를 생각하면 enum으로 정의하는게 낫겠네요~~! 바로 반영할게요😄

pricelees · 2024-09-12T12:09:08Z

tomcat/src/main/java/com/techcourse/web/handler/HandlerMapper.java

+		return handlers.stream()
+			.filter(handler -> handler.isSupport(requestLine))
+			.findFirst()
+			.orElseThrow(() -> new IllegalArgumentException("handler not found. " +


Http11Processor에서 IllegalArgumentException을 처리하지 않고 있습니다 예외대신 404 페이지를 바라보는 NotFoundHandler를 구현해서 리턴하면 어떨까요?

텔레파시 🤜🤛

pricelees · 2024-09-12T12:44:59Z

tomcat/src/main/java/org/apache/coyote/http11/http/response/HttpResponse.java

+		return new HttpResponse(new HttpResponseStartLine(HTTP_VERSION, HttpStatusCode.OK), header, body);
+	}
+
+	public static HttpResponse notFound(HttpResponseHeader header, HttpResponseBody body) {


ok()와 더불어서, usages를 확인했을 때 모두 추가적인 로직없이 단순히 외부에서 header 객체를 생성 후 넘겨주고 있습니다 이렇게 구현하신 이유가 있을까요??

음 지금은 응답 헤더에 값을 넣고있지 않는데 헤더에 값을 넣을 여지는 열어놔야 한다고 생각했읍니다 ~~!

pricelees · 2024-09-12T12:46:15Z

tomcat/src/main/java/org/apache/coyote/http11/http/response/HttpResponse.java

+		return new HttpResponse(new HttpResponseStartLine(HTTP_VERSION, HttpStatusCode.NOT_FOUND), header, body);
+	}
+
+	public HttpResponse(HttpResponseStartLine startLine, HttpResponseHeader header, HttpResponseBody body) {


객체 내부에서만 사용하는 생성자는 private 접근 제어자를 사용하면 어떨까요?

ok, notfound 등이 이후에 추가한 것들이라 반영이 다 안되었네요..ㅎㅎ 수정했습니다👍

pricelees · 2024-09-12T12:49:53Z

tomcat/src/test/java/com/techcourse/web/handler/HandlerMapperTest.java

+	void findHandler_WhenRequestRootPage() {
+		String requestPath = "/";
+
+		Class<? extends Handler> expectedHandler = RootPageHandler.class;


제네릭인 이유가 있나요?? Handler 인터페이스로 선언해도 되지 않을까요?

테스트에서 타입 비교에 사용되는데, 이 경우는 제네릭을 안 쓰면 구체 타입을 지정해야 하더라구요. 그런데 생각해보니 runTest의 파라미터만 제네릭을 써도 충분해서 구체 타입을 지정하는 방법으로 수정했습니다😄

pricelees · 2024-09-12T12:51:21Z

tomcat/src/test/java/com/techcourse/web/handler/HandlerMapperTest.java

+
+		Handler handler = HandlerMapper.findHandler(request);
+
+		assertThat(handler).isInstanceOf(expectedHandler);


isExactlyInstanceOf()로 클래스가 정확히 일치하는지 검증하면 신뢰도가 더 높을 것 같습니다

쵝오..😌

pricelees · 2024-09-12T12:55:12Z

tomcat/src/test/resources/index.html

@@ -0,0 +1,106 @@
+<!DOCTYPE html>


이 html 파일은 어떤 용도인가요?

getPath() 에서 NPE가 나오길래 파일을 넣었는데.. 제가 경로 설정을 잘못했군요..ㅎㅎ..

pricelees · 2024-09-12T13:04:42Z

POST /login 요청 시 Set-Cookie 헤더 누락

이미 로그인이 되어 있으면 POST /login 요청을 보낼 일이 없다고 생각했으나.. 로그인 마다 새로운 sessionId를 발급받아 갱신하는 방향으로 수정했습니다~!!

GIVEN53

빠른 반영 감사합니다 상돌 👍🏻 저도 빠르게 머지드리겠습니다
18시까지 부지런히 달려보죠!

geoje and others added 27 commits September 11, 2024 11:31

fix: remove implementation logback-classic on gradle (woowacourse#501)

32f26c2

fix: add threads min-spare configuration on properties (woowacourse#502)

4fe78cc

전체 코드 롤백

aa8696a

test: http 캐시 테스트

a726a3c

feat: index.html 응답 및 css 지원 기능 구현

2537d06

feat: favicon 파일 추가

a714e5a

feat: HTTP 요청 메시지를 담는 객체 구현

27c9983

feat: HTTP 요청 메시지를 읽는 객체 구현

6afa63f

feat: HTTP 응답 메시지를 만들 때 사용하는 객체 구현

7a29f8a

remove: 미사용 클래스 삭제

d86ba66

fix: 쿼리에서 값을 조회하지 못하는 문제 해결

2e2f23a

feat: 정적 리소스 파일을 불러오는 기능 및 테스트 구현

d3af719

feat: / 경로에 대한 요청을 처리하는 기능 및 테스트 구현

cc45a5e

feat: 정적 리소스 요청을 처리하는 기능 및 테스트 구현

8b01768

feat: /login 경로에 대한 요청을 처리하는 기능 및 테스트 구현

6618122

feat: 요청에 대해 적절한 핸들러를 찾는 기능 구현

a66d132

refactor: 구현된 기능을 Http11Processor 에 반영

048c263

refactor: 응답 메시지를 만들 때 body가 없는 경우에 대한 처리 추가

a429369

feat: 로그인 302 응답 기능 구현

433d41d

refactor: http 요청 body를 읽을 때 content-length 헤더를 사용하도록 수정

7781280

feat: 로그인 기능 구현

f034f40

feat: Form 요청 body를 처리하는 기능 및 테스트 구현

5925543

feat: 회원을 저장할 때 중복 account에 대한 검증 및 테스트 구현

15adb68

feat: 회원가입 기능 및 테스트 구현

9fdcccd

feat: 로그인 성공시 쿠키 설정 기능 및 테스트 구현

b712e75

feat: 세션을 이용한 로그인 기능 구현

d8c737d

style: reformat code

a44e28e

pricelees requested a review from GIVEN53 September 12, 2024 07:20

pricelees added 2 commits September 12, 2024 18:12

style: 테스트 메서드명 수정

ff51208

refactor: 로그인 시 POST 요청을 보내도록 수정

202f711

GIVEN53 requested changes Sep 12, 2024

View reviewed changes

pricelees added 11 commits September 12, 2024 20:38

fix: 세션이 없을 때 페이지 로딩이 안되는 문제 해결

38833b5

refactor: 공통 헤더 상수화

3c59f40

refactor: Handler가 존재하지 않으면 404 페이지를 반환하도록 수정

a1f6854

refactor: 로그인 페이지에서 User를 중복으로 조회하는 로직 수정

068883a

refactor: 헤더 상수를 public으로 수정 및 RequestMessageReader에 반영

de48f3f

refactor: JSessionId 존재 유무 확인 로직 수정

8372959

refactor: 요청 헤더에서의 타입 수정

f056c5f

refactor: HttpResponse 생성자 접근 제어자 수정(public -> private)

6f3f4b7

refactor: 테스트에서의 제네릭 타입 제거

09e13b4

refactor: 테스트에서의 타입 체크 방법 수정(isInstanceof -> isExactlyInstanceOf)

b8c5de7

refactor: 미사용 html 파일 제거

a27514d

pricelees commented Sep 12, 2024

View reviewed changes

refactor: sessionId를 로그인시 항상 갱신하도록 수정

329bb3c

GIVEN53 approved these changes Sep 12, 2024

View reviewed changes

GIVEN53 merged commit e9c7df5 into woowacourse:pricelees Sep 12, 2024


		Handler handler = HandlerMapper.findHandler(request);

		assertThat(handler).isInstanceOf(expectedHandler);

[2단계 - 로그인 구현하기] 상돌(이상진) 미션 제출합니다. #646

[2단계 - 로그인 구현하기] 상돌(이상진) 미션 제출합니다. #646

Uh oh!

Conversation

pricelees commented Sep 12, 2024

Uh oh!

GIVEN53 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pricelees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

GIVEN53 left a comment •

edited

Loading