Conversation
- 요구사항 1: index.html 반환하기
…erSignupController
…erLoginController
…aticFileController
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughHTTP 요청/응답 파싱·생성, 라우팅 구성(RequestMapper/WebConfig) 및 컨트롤러(Forward/UserSignup/UserLogin/UserList)들이 추가되고 RequestHandler가 이를 위임하도록 변경되었습니다. 관련 단위 테스트, 테스트 리소스, Gradle 테스트 의존성, HTML 폼 메서드 변경 및 IntelliJ 검사 프로필이 포함됩니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as 클라이언트
participant RH as RequestHandler
participant RM as RequestMapper
participant WC as WebConfig
participant CT as Controller
participant R as HttpResponse
Note over RH,RM: 파싱 → 라우팅 → 컨트롤러 실행 → 응답 전송
C->>RH: TCP 연결 및 HTTP 요청
RH->>RH: BufferedReader로 요청 읽음
RH->>RH: HttpRequest.from(...)
RH->>R: new HttpResponse(OutputStream, version)
RH->>RM: RequestMapper.getController(path, method)
RM->>WC: (매핑 조회)
alt 매칭된 컨트롤러 있음
RM->>CT: CT.execute(request, response)
else 매칭 없음
RM->>CT: ForwardController.execute(request, response)
end
CT-->>R: forward / redirect / notFound
R-->>C: 상태라인, 헤더, 바디 전송
sequenceDiagram
autonumber
participant C as 클라이언트
participant ULC as UserLoginController
participant Repo as MemoryUserRepository
participant R as HttpResponse
Note over ULC: 로그인 처리(인증 및 쿠키 설정)
C->>ULC: POST /user/login (userId, password)
ULC->>Repo: findById(userId)
alt 사용자 존재 및 비밀번호 일치
ULC->>R: redirectWithCookie("/index.html", "logined=true")
else 인증 실패
ULC->>R: redirect("/user/login_failed.html")
end
sequenceDiagram
autonumber
participant C as 클라이언트
participant ULCt as UserListController
participant R as HttpResponse
Note over ULCt: 쿠키 기반 접근 제어
C->>ULCt: GET /user/userList (Cookie)
alt Cookie에 logined=true 포함
ULCt->>R: forward("/user/list.html")
else 비로그인
ULCt->>R: redirect("/user/login.html")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (16)
src/test/java/controller/UserSignupControllerTest.java (1)
60-80: 두 번째 회원가입 테스트가 첫 번째 테스트와 중복됩니다.동일한 로직을 다른 데이터로 반복하는 테스트입니다. 테스트 커버리지는 높이지만, @ParameterizedTest를 사용하면 중복을 제거하고 더 많은 시나리오를 쉽게 추가할 수 있습니다.
@ParameterizedTest로 리팩토링 고려:
@ParameterizedTest @CsvSource({ "testuser,1234,홍길동,test@example.com", "getuser,5678,김철수,get@example.com" }) @DisplayName("POST 요청으로 유효한 사용자 정보를 전송하면 회원가입이 성공해야 한다") public void signupSuccessWithPostRequest(String userId, String password, String name, String email) throws IOException { // given when(mockRequest.getMethod()).thenReturn(HttpMethod.POST); when(mockRequest.getBody()).thenReturn( String.format("userId=%s&password=%s&name=%s&email=%s", userId, password, name, email) ); when(mockRequest.getQueryString()).thenReturn(null); // when controller.execute(mockRequest, mockResponse); // then verify(mockResponse).redirect(RequestPath.INDEX.getValue()); User savedUser = repository.findUserById(userId); assertNotNull(savedUser, "사용자가 저장되어야 한다"); assertEquals(userId, savedUser.getUserId()); assertEquals(password, savedUser.getPassword()); assertEquals(name, savedUser.getName()); assertEquals(email, savedUser.getEmail()); }src/main/java/controller/UserListController.java (1)
14-28: 쿠키 파싱 로직을 유틸리티로 분리하는 것을 권장합니다.쿠키 파싱 로직이 여러 컨트롤러에서 반복될 가능성이 높습니다.
HttpRequestUtils와 같은 유틸리티 클래스로 추출하여 재사용성을 높이세요.예시:
// HttpRequestUtils에 추가 public static Map<String, String> parseCookies(String cookieHeader) { Map<String, String> cookies = new HashMap<>(); if (cookieHeader == null) return cookies; for (String pair : cookieHeader.split(";\\s*")) { String[] kv = pair.split("=", 2); if (kv.length == 2) { cookies.put(kv[0], kv[1]); } } return cookies; }그런 다음 컨트롤러에서:
Map<String, String> cookies = HttpRequestUtils.parseCookies(request.getCookie()); if ("true".equals(cookies.get("logined"))) { // ... }src/test/java/controller/ForwardControllerTest.java (2)
26-29: MockitoExtension 사용을 권장합니다.
MockitoAnnotations.openMocks(this)를 수동으로 호출하는 대신@ExtendWith(MockitoExtension.class)를 사용하면 Mockito 생명주기 관리가 자동화되고 코드가 간결해집니다.다음과 같이 변경하세요:
+import org.mockito.junit.jupiter.MockitoExtension; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(MockitoExtension.class) public class ForwardControllerTest { @Mock private HttpRequest mockRequest; @Mock private HttpResponse mockResponse; private ForwardController controller; @BeforeEach public void setUp() { - MockitoAnnotations.openMocks(this); controller = new ForwardController(); }
73-85: 존재하지 않는 파일 테스트 로직을 명확히 하세요.Line 77에서
forward가 IOException을 던지도록 설정했지만, 실제ForwardController구현에서는 예외를 catch하고notFound를 호출합니다. 테스트는 이 동작을 검증하지만,forward와notFound호출 순서가 명확하지 않을 수 있습니다.호출 순서를 명시적으로 검증하려면:
// then verify(mockResponse).forward("/nonexistent.html"); verify(mockResponse).notFound(); + + // 또는 InOrder로 순서 검증 + InOrder inOrder = inOrder(mockResponse); + inOrder.verify(mockResponse).forward("/nonexistent.html"); + inOrder.verify(mockResponse).notFound();src/main/java/controller/UserSignupController.java (1)
37-39: 중복 사용자 체크를 추가하세요.동일한 userId로 여러 번 회원가입을 시도할 경우, 기존 사용자를 덮어쓰거나 중복 데이터가 저장될 수 있습니다.
다음과 같이 중복 체크를 추가하세요:
MemoryUserRepository repository = MemoryUserRepository.getInstance(); + if (repository.findUserById(userId) != null) { + log.log(Level.WARNING, "User already exists: " + userId); + response.redirect("/user/signup?error=duplicate"); + return; + } repository.addUser(newUser); log.log(Level.INFO, "New User Registered: " + newUser.getUserId());src/main/java/controller/ForwardController.java (1)
24-28: 디렉토리 트래버설 방어를 강화하는 것을 권장합니다.현재
contains("..")는 기본적인 공격을 차단하지만, URL 인코딩된 경로(%2e%2e)나 정규화되지 않은 경로(/./,//)를 우회할 수 있습니다.다음과 같이 경로를 정규화한 후 검증하세요:
+ // 2. 보안 검증 - 경로 정규화 및 디렉토리 traversal 공격 방지 - if (path.contains("..")) { + try { + String normalizedPath = Paths.get(path).normalize().toString(); + if (normalizedPath.contains("..") || !normalizedPath.startsWith("/")) { + log.log(Level.WARNING, "Directory traversal attack detected: " + path); + response.notFound(); + return; + } + } catch (Exception e) { log.log(Level.WARNING, "Directory traversal attack detected: " + path); response.notFound(); return; }src/main/java/http/enums/HttpStatus.java (1)
3-6: 추가 HTTP 상태 코드 지원을 고려하세요.현재 3개의 상태 코드만 정의되어 있지만, 향후 확장을 위해 다음 상태 코드를 추가하는 것을 고려하세요:
400 Bad Request(잘못된 요청 검증용)401 Unauthorized(인증 실패용)500 Internal Server Error(서버 오류용)OK(200, "HTTP/1.1 200 OK"), + CREATED(201, "HTTP/1.1 201 Created"), + BAD_REQUEST(400, "HTTP/1.1 400 Bad Request"), + UNAUTHORIZED(401, "HTTP/1.1 401 Unauthorized"), FOUND(302, "HTTP/1.1 302 Found"), NOT_FOUND(404, "HTTP/1.1 404 Not Found"); + INTERNAL_SERVER_ERROR(500, "HTTP/1.1 500 Internal Server Error");src/main/java/http/enums/HttpMethod.java (2)
23-24: TODO: 커스텀 예외 구현을 고려하세요.TODO 주석이 있는 대로, 프레임워크 일관성을 위해
IllegalArgumentException대신 커스텀 예외(예:UnsupportedHttpMethodException)를 사용하는 것을 고려하세요. 이렇게 하면 HTTP 프로토콜 관련 예외를 더 명확하게 처리할 수 있습니다.다음과 같은 커스텀 예외를 생성할 수 있습니다:
public class UnsupportedHttpMethodException extends RuntimeException { public UnsupportedHttpMethodException(String method) { super("Unsupported HTTP method: " + method); } }그런 다음 from 메서드에서:
- // todo: Custom Exception - throw new IllegalArgumentException("Unknown HTTP method: " + method); + throw new UnsupportedHttpMethodException(method);이 작업을 수행하기 위해 새 이슈를 생성하시겠습니까?
17-25: from 메서드 최적화를 선택적으로 고려하세요.현재 선형 검색은 2개의 enum 값에 대해서는 충분히 빠르지만, 향후 HTTP 메서드가 추가될 경우 정적 Map을 사용하여 O(1) 조회를 수행하는 것이 좋습니다.
+ private static final Map<String, HttpMethod> METHOD_MAP = + Arrays.stream(values()).collect(Collectors.toMap(HttpMethod::getValue, m -> m)); + public static HttpMethod from(String method) { - for (HttpMethod httpMethod : HttpMethod.values()) { - if (httpMethod.getValue().equals(method)) { - return httpMethod; - } - } + HttpMethod httpMethod = METHOD_MAP.get(method); + if (httpMethod == null) { + throw new IllegalArgumentException("Unknown HTTP method: " + method); + } - throw new IllegalArgumentException("Unknown HTTP method: " + method); + return httpMethod; }src/main/java/webserver/WebConfig.java (2)
20-20: null 메서드의 의미를 명확히 하세요.Line 20과 Line 29에서
method를null로 전달하고 있지만, 이것이 "모든 HTTP 메서드를 허용"하는 것인지 "GET만 허용"하는 것인지 불분명합니다. 주석이나 명시적인 상수로 의도를 명확히 하세요.다음 중 하나를 선택하세요:
옵션 1: 명시적으로 GET 지정
- controllers.put(createKey(RequestPath.ROOT.getValue(), null), new ForwardController()); + controllers.put(createKey(RequestPath.ROOT.getValue(), HttpMethod.GET), new ForwardController());옵션 2: 주석으로 의도 명확화
- // 루트 경로 + // 루트 경로 (모든 HTTP 메서드 허용) controllers.put(createKey(RequestPath.ROOT.getValue(), null), new ForwardController());옵션 3: 상수 도입
private static final HttpMethod ANY_METHOD = null; // 모든 HTTP 메서드 허용 controllers.put(createKey(RequestPath.ROOT.getValue(), ANY_METHOD), new ForwardController());Also applies to: 29-29
16-32: 중복 경로 등록 검증을 추가하는 것을 고려하세요.현재 구현에서는 실수로 동일한 경로+메서드 조합을 두 번 등록하면 나중 것이 이전 것을 조용히 덮어씁니다. 개발 중 버그를 조기에 발견하기 위해 중복 검증을 추가하는 것을 고려하세요.
public static Map<String, Controller> configureControllers() { Map<String, Controller> controllers = new HashMap<>(); // 루트 경로 - controllers.put(createKey(RequestPath.ROOT.getValue(), null), new ForwardController()); + putController(controllers, RequestPath.ROOT.getValue(), null, new ForwardController()); // 회원가입 - controllers.put(createKey(RequestPath.USER_SIGNUP.getValue(), HttpMethod.POST), new UserSignupController()); + putController(controllers, RequestPath.USER_SIGNUP.getValue(), HttpMethod.POST, new UserSignupController()); // ... 나머지도 동일하게 return controllers; } + + private static void putController(Map<String, Controller> controllers, + String path, HttpMethod method, Controller controller) { + String key = createKey(path, method); + if (controllers.containsKey(key)) { + throw new IllegalStateException("Duplicate route registration: " + key); + } + controllers.put(key, controller); + }src/test/java/controller/UserLoginControllerTest.java (1)
1-19: MockitoExtension 사용을 권장합니다.현재
MockitoAnnotations.openMocks(this)를 수동으로 호출하고 있습니다. JUnit Jupiter와 Mockito 5.x에서는@ExtendWith(MockitoExtension.class)를 사용하는 것이 권장됩니다.다음 diff를 적용하여 개선할 수 있습니다:
package controller; import db.MemoryUserRepository; import http.HttpRequest; import http.HttpResponse; import http.enums.HttpMethod; import http.enums.RequestPath; import model.User; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.junit.jupiter.MockitoExtension; import java.io.IOException; import static org.mockito.Mockito.*; +@ExtendWith(MockitoExtension.class) public class UserLoginControllerTest {그리고
setUp메서드에서 다음 라인을 제거하세요:@BeforeEach public void setUp() { - MockitoAnnotations.openMocks(this); controller = new UserLoginController();src/main/java/http/HttpStartLine.java (1)
45-57: 쿼리 스트링 파싱이 단순하지만 안전합니다.현재 구현은 첫 번째
?를 기준으로 파싱하며, 대부분의 경우 문제없이 동작합니다.만약 더 방어적인 구현을 원한다면, 다음과 같이 개선할 수 있습니다:
public String getPathWithoutQuery() { - if (path.contains("?")) { - return path.substring(0, path.indexOf("?")); - } - return path; + int queryIndex = path.indexOf("?"); + return queryIndex != -1 ? path.substring(0, queryIndex) : path; } public String getQueryString() { - if (path.contains("?")) { - return path.substring(path.indexOf("?") + 1); - } - return null; + int queryIndex = path.indexOf("?"); + return queryIndex != -1 ? path.substring(queryIndex + 1) : null; }src/main/java/http/enums/HttpHeader.java (1)
22-29: 대소문자 구분 없는 헤더 매칭을 고려하세요.HTTP 헤더는 대소문자를 구분하지 않습니다(RFC 7230). 현재 구현은 정확히 일치해야만 매칭되므로,
Content-length와Content-Length를 다르게 처리합니다.다음과 같이 개선할 수 있습니다:
public static HttpHeader from(String headerName) { for (HttpHeader header : values()) { - if (header.headerName.equals(headerName)) { + if (header.headerName.equalsIgnoreCase(headerName)) { return header; } } return null; // 알려지지 않은 헤더는 null return }src/test/java/HttpRequestTest.java (1)
18-20: 리소스 관리 책임을 명확히 하세요.
bufferedReaderFromFile()이 반환하는BufferedReader를 호출자가 닫아야 합니다. 현재 테스트에서는HttpRequest.from()이BufferedReader를 소비하지만 닫지 않아 리소스 누수가 발생할 수 있습니다.테스트 메서드에서 try-with-resources를 사용하도록 권장합니다:
@Test @DisplayName("POST 요청을 파싱할 때 메서드, URL, 헤더, 바디가 올바르게 파싱되어야 한다") public void parsePostRequestWithBodyAndHeaders() throws IOException { // given - BufferedReader br = bufferedReaderFromFile(TEST_DIRECTORY + "request/post_request.txt"); - - // when - HttpRequest httpRequest = HttpRequest.from(br); + try (BufferedReader br = bufferedReaderFromFile(TEST_DIRECTORY + "request/post_request.txt")) { + // when + HttpRequest httpRequest = HttpRequest.from(br); - // then - assertEquals(HttpMethod.POST, httpRequest.getMethod()); - ... + // then + assertEquals(HttpMethod.POST, httpRequest.getMethod()); + ... + } }src/main/java/http/HttpRequest.java (1)
73-79: 내부 객체 노출이 의도적인지 확인하세요.
getStartLine()과getHeaders()가 내부 객체를 직접 반환합니다.HttpStartLine은 불변이고HttpHeaders의 접근자도 방어적 복사를 하므로 현재는 안전하지만, API 사용자가 이 객체들을 직접 조작하려 할 수 있습니다.만약 내부 구조를 완전히 숨기고 싶다면, 이 메서드들을 제거하고 필요한 접근자만
HttpRequest에서 제공하는 것을 고려하세요. 아니면 현재처럼 유지하되, 이것이 의도된 설계임을 문서화하세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.idea/inspectionProfiles/Project_Default.xml(1 hunks)build.gradle(1 hunks)src/main/java/controller/Controller.java(1 hunks)src/main/java/controller/ForwardController.java(1 hunks)src/main/java/controller/UserListController.java(1 hunks)src/main/java/controller/UserLoginController.java(1 hunks)src/main/java/controller/UserSignupController.java(1 hunks)src/main/java/http/HttpHeaders.java(1 hunks)src/main/java/http/HttpRequest.java(1 hunks)src/main/java/http/HttpResponse.java(1 hunks)src/main/java/http/HttpStartLine.java(1 hunks)src/main/java/http/enums/ContentType.java(1 hunks)src/main/java/http/enums/HttpHeader.java(1 hunks)src/main/java/http/enums/HttpMethod.java(1 hunks)src/main/java/http/enums/HttpStatus.java(1 hunks)src/main/java/http/enums/RequestPath.java(1 hunks)src/main/java/model/UserField.java(1 hunks)src/main/java/webserver/RequestHandler.java(2 hunks)src/main/java/webserver/RequestMapper.java(1 hunks)src/main/java/webserver/WebConfig.java(1 hunks)src/test/java/HttpRequestTest.java(1 hunks)src/test/java/HttpResponseTest.java(1 hunks)src/test/java/controller/ForwardControllerTest.java(1 hunks)src/test/java/controller/UserListControllerTest.java(1 hunks)src/test/java/controller/UserLoginControllerTest.java(1 hunks)src/test/java/controller/UserSignupControllerTest.java(1 hunks)src/test/java/webserver/RequestMapperTest.java(1 hunks)src/test/resources/request/get_request.txt(1 hunks)src/test/resources/request/post_request.txt(1 hunks)src/test/resources/response/test.html(1 hunks)webapp/user/form.html(1 hunks)
🔇 Additional comments (23)
src/test/resources/response/test.html (1)
1-10: 테스트 리소스가 적절하게 작성되었습니다.HttpResponse 테스트를 위한 간단하고 유효한 HTML 구조입니다. DOCTYPE, head, body 요소가 올바르게 구성되어 있습니다.
build.gradle (1)
16-16: Mockito 의존성 추가가 적절합니다.mockito-junit-jupiter 5.12.0은 JUnit Jupiter와의 통합을 지원하며, 컨트롤러 단위 테스트에서 사용되는 @mock 및 MockitoAnnotations와 호환됩니다. Based on learnings
webapp/user/form.html (1)
58-58: 회원가입 폼의 보안 개선이 적절합니다.GET에서 POST로 변경하여 사용자 정보가 URL에 노출되지 않도록 개선되었습니다. 이는 PR 목표인 "GET→POST transition for security"와 일치하며, UserSignupController의 POST 처리 로직과 정확히 연결됩니다.
src/test/resources/request/get_request.txt (1)
1-6: GET 요청 테스트 리소스가 적절합니다.쿼리 파라미터, 헤더, 쿠키를 포함한 유효한 HTTP/1.1 GET 요청 구조입니다. HttpRequestTest의 파싱 테스트에 적합합니다.
src/main/java/controller/Controller.java (1)
8-10: 컨트롤러 인터페이스 설계가 깔끔합니다.단일 책임 원칙을 따르는 간결한 인터페이스입니다. HttpRequest/HttpResponse를 인자로 받는 execute 메서드 시그니처가 적절하며, RequestMapper를 통한 다형적 호출 패턴을 지원합니다.
src/test/java/controller/UserSignupControllerTest.java (1)
38-58: 첫 번째 회원가입 테스트 로직이 정확합니다.POST 요청 모킹, 컨트롤러 실행, 리다이렉트 검증, 그리고 저장소 상태 확인까지 모든 단계가 적절하게 구현되어 있습니다.
src/main/java/model/UserField.java (1)
3-18: 사용자 필드 enum이 잘 설계되었습니다.매직 스트링을 제거하고 타입 안전성을 제공하는 깔끔한 enum 설계입니다. UserSignupController에서 폼 파라미터 추출 시 사용되어 코드 가독성과 유지보수성을 향상시킵니다.
src/main/java/controller/UserSignupController.java (1)
42-42: 리다이렉트 로직이 적절합니다.POST 요청 후 302 리다이렉트로 메인 페이지로 이동하는 것은 PRG(Post-Redirect-Get) 패턴을 따르며, 브라우저 새로고침 시 중복 제출을 방지합니다.
src/main/java/controller/ForwardController.java (1)
30-39: 예외 처리 로직이 적절합니다.IOException을 catch하여 404 응답을 반환하고, 적절한 로그 레벨(WARNING)로 기록하는 것은 정상적인 에러 흐름입니다. 파일이 없는 것은 클라이언트 오류이므로 스택 트레이스를 기록할 필요가 없습니다.
src/main/java/http/enums/HttpStatus.java (1)
8-22: Enum 설계가 잘 구현되었습니다.불변 필드와 getter 메서드를 사용한 깔끔한 enum 구현입니다. 상태 코드와 상태 라인을 함께 관리하여 일관성을 보장합니다.
src/main/java/webserver/WebConfig.java (1)
34-39: createKey 로직이 명확합니다.경로와 메서드를 조합하여 고유한 키를 생성하는 로직이 간단하고 명확합니다. null 처리도 적절합니다.
src/main/java/http/enums/RequestPath.java (1)
1-31: 경로 상수 관리가 잘 되어 있습니다.RequestPath enum으로 경로를 중앙 관리하면 오타 방지, 리팩토링 용이성, IDE 자동완성 지원 등의 이점이 있습니다. 전체적으로 깔끔한 설계입니다.
src/test/java/controller/UserLoginControllerTest.java (1)
41-95: 테스트 케이스 구조와 커버리지가 우수합니다.Given-When-Then 패턴을 일관되게 사용하고, 성공 케이스와 다양한 실패 케이스(잘못된 비밀번호, 존재하지 않는 사용자, 빈 파라미터)를 모두 커버하고 있습니다.
@DisplayName으로 테스트 의도가 명확하게 표현되어 있습니다.src/main/java/http/HttpStartLine.java (1)
10-31: 요청 라인 파싱 로직이 정확합니다.null/empty 검증과 형식 검증(3개 파트)이 적절하며,
HttpMethod.from으로 메서드 파싱을 위임하는 설계가 깔끔합니다.src/main/java/http/enums/HttpHeader.java (1)
1-20: HTTP 헤더 enum 구조가 명확합니다.필수 헤더들이 잘 정의되어 있고,
getValue()로 실제 헤더 이름을 가져오는 구조가 적절합니다.src/main/java/http/HttpHeaders.java (1)
50-60: 헤더 존재 확인 메서드가 올바르게 구현되었습니다.
getAllHeaders()가 방어적 복사본을 반환하는 것도 좋습니다. 단, 대소문자 처리 문제는 앞서 언급한 수정으로 함께 해결됩니다.src/test/java/HttpRequestTest.java (3)
22-39: POST 요청 파싱 테스트가 포괄적입니다.메서드, URL, 버전, 헤더, Content-Length, 바디 존재 여부 및 내용까지 모두 검증하고 있어 테스트 커버리지가 우수합니다.
41-58: GET 요청 테스트가 쿼리 스트링과 쿠키를 잘 검증합니다.쿼리 파라미터 파싱과 쿠키 추출, 그리고 GET 요청에서 바디가 null인지 확인하는 등 GET 요청의 특성을 정확하게 테스트하고 있습니다.
60-78: 예외 처리 테스트가 적절합니다.잘못된 형식과 빈 요청 라인에 대해
IllegalArgumentException이 발생하는지 검증하여 에러 처리를 확인하고 있습니다.src/main/java/http/HttpResponse.java (3)
17-24: 생성자 설계가 깔끔합니다.기본 webapp 경로를 제공하는 동시에 테스트를 위한 커스터마이징이 가능한 오버로드된 생성자 구조가 좋습니다.
38-49: 리다이렉트 구현이 올바릅니다.302 상태 코드와 Location 헤더 사용이 정확하며,
redirectWithCookie()도 Set-Cookie 헤더를 적절히 추가하고 있습니다.
51-77: 404 응답과 헬퍼 메서드 구현이 깔끔합니다.응답 작성 로직이 잘 분리되어 있고,
writeBody()에서 적절히 flush하고 있습니다.src/main/java/http/HttpRequest.java (1)
14-35: HTTP 요청 파싱 로직이 견고합니다.불변 설계, 적절한 검증, 그리고 POST 요청에 대해서만 Content-Length가 있을 때 바디를 읽는 조건부 로직이 정확합니다.
HttpStartLine과HttpHeaders로 파싱을 위임하는 것도 책임이 잘 분리되어 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/controller/UserSignupController.java (1)
38-40: 검증 실패 시 HTTP 에러 응답이 누락되었습니다.입력 검증이 실패하면
return만 수행하여 클라이언트에게 응답을 보내지 않습니다. 이는 연결이 멈추거나 타임아웃될 수 있어 사용자 경험을 저해합니다.다음과 같이 400 Bad Request 응답을 추가하세요:
if (userId == null || userId.trim().isEmpty() || password == null || password.length() < 4 || name == null || name.trim().isEmpty() || email == null || !email.matches("^[A-Za-z0-9+_.-]+@[A-Za-z0-9.-]+$")) { - // TODO: Bad Request + response.badRequest("Invalid input parameters"); return; }참고:
HttpResponse에badRequest()메서드 추가가 필요합니다.
🧹 Nitpick comments (2)
src/main/java/controller/UserSignupController.java (1)
38-39: TODO: 검증 실패 시 적절한 응답을 구현하세요.현재 검증 실패 시 조기 반환만 하고 클라이언트에 응답을 보내지 않습니다. 사용자에게 검증 실패 이유를 알려주는 400 Bad Request 응답이 필요합니다.
다음과 같이 개선할 수 있습니다:
if (userId == null || userId.trim().isEmpty() || password == null || password. length() < 4 || name == null || name.trim().isEmpty() || email == null || !email.matches("^[A-Za-z0-9+_.-]+@[A-Za-z0-9.-]+$")) { - // TODO: Bad Request - return; + response.badRequest("Invalid input parameters"); + return; }
HttpResponse에badRequest()메서드 추가가 필요합니다. 구현 코드 생성을 도와드릴까요?src/test/java/controller/UserLoginControllerTest.java (1)
30-39: 테스트 격리 제약 사항을 인지하고 계십니다.개발자께서 이미 싱글톤 Repository의
clear()메서드 부재로 인한 테스트 격리 문제를 인지하고 계신 것으로 확인됩니다. 현재 프로젝트 제약 사항 내에서 적절한 선택입니다.향후 테스트 간 상태 공유로 인한 문제 발생 시 Repository 인터페이스에
clear()추가를 고려하시기 바랍니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.idea/vcs.xml(1 hunks)src/main/java/controller/UserListController.java(1 hunks)src/main/java/controller/UserLoginController.java(1 hunks)src/main/java/controller/UserSignupController.java(1 hunks)src/main/java/http/HttpHeaders.java(1 hunks)src/main/java/http/HttpRequest.java(1 hunks)src/main/java/http/HttpResponse.java(1 hunks)src/main/java/http/enums/ContentType.java(1 hunks)src/main/java/http/enums/RequestPath.java(1 hunks)src/test/java/HttpRequestTest.java(1 hunks)src/test/java/controller/UserLoginControllerTest.java(1 hunks)src/test/resources/request/post_request.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .idea/vcs.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/controller/UserLoginController.java
- src/main/java/http/HttpResponse.java
- src/main/java/http/HttpHeaders.java
- src/main/java/http/enums/ContentType.java
- src/test/resources/request/post_request.txt
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/controller/UserSignupController.java (5)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/HttpRequest.java (1)
HttpRequest(9-80)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-91)src/main/java/http/util/HttpRequestUtils.java (1)
HttpRequestUtils(8-20)src/main/java/model/User.java (1)
User(5-46)
src/main/java/http/HttpRequest.java (3)
src/main/java/http/util/IOUtils.java (1)
IOUtils(6-21)src/main/java/http/HttpStartLine.java (1)
HttpStartLine(5-58)src/main/java/http/HttpHeaders.java (1)
HttpHeaders(10-68)
src/test/java/HttpRequestTest.java (1)
src/main/java/http/HttpRequest.java (1)
HttpRequest(9-80)
src/test/java/controller/UserLoginControllerTest.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/HttpRequest.java (1)
HttpRequest(9-80)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-91)src/main/java/model/User.java (1)
User(5-46)
src/main/java/controller/UserListController.java (2)
src/main/java/http/HttpRequest.java (1)
HttpRequest(9-80)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-91)
🔇 Additional comments (19)
src/main/java/controller/UserListController.java (3)
20-20: 과거 리뷰의 보안 취약점이 올바르게 수정되었습니다.
contains()대신 정확한 키-값 파싱을 사용하도록 변경되어 부분 문자열 매칭 문제가 해결되었습니다.Objects.equals를 사용한 null-safe 비교도 적절합니다.
31-39: 쿠키 파싱 로직이 올바르게 구현되었습니다.세미콜론과 공백으로 쌍을 분리하고,
=로 키-값을 분할하며, 키가 일치하는 값을 반환하는 구현이 정확합니다.
20-20: 쿠키 검증 로직이 올바르게 개선되었습니다.이전 리뷰에서 제기된 부분 문자열 매칭 취약점이
parseCookieValue헬퍼 메서드를 통한 정확한 키-값 파싱으로 해결되었습니다.Objects.equals를 사용한 null-safe 비교도 적절합니다.Also applies to: 31-39
src/main/java/http/enums/RequestPath.java (2)
23-30: 과거 리뷰의 일관성 문제가 올바르게 해결되었습니다.이제
HttpMethod.from()과 동일하게 알 수 없는 경로에 대해IllegalArgumentException을 던지므로, null 체크 누락으로 인한 NPE 위험이 제거되었습니다.
23-30:from()메서드가 일관성 있게 개선되었습니다.이전 리뷰에서 제기된
null반환 문제가IllegalArgumentException발생으로 변경되어HttpMethod.from()과 일관된 예외 처리 방식을 따르게 되었습니다. 알 수 없는 경로에 대한 명확한 오류 메시지도 제공됩니다.src/main/java/controller/UserSignupController.java (2)
28-40: 입력 검증이 올바르게 추가되었습니다.과거 리뷰에서 요청한 필수 필드 검증, 비밀번호 길이 확인, 이메일 형식 검증이 모두 구현되었습니다. NPE 방지와 데이터 무결성이 크게 개선되었습니다.
28-37: 입력 검증 로직이 올바르게 추가되었습니다.이전 리뷰에서 제기된 NPE 및 유효하지 않은 데이터 처리 문제가 필수 필드 검증, 비밀번호 길이 검증, 이메일 형식 검증을 통해 해결되었습니다. 보안과 안정성이 향상되었습니다.
src/test/java/controller/UserLoginControllerTest.java (3)
30-39: setUp 메서드가 올바르게 구현되었습니다.Mock 초기화와 테스트용 사용자 등록이 적절히 수행됩니다. 싱글톤 Repository의 상태 공유 문제는 개발자가 인지하고 있으며,
clear()메서드가 없는 제약사항으로 확인되었습니다.
41-95: 테스트 케이스가 포괄적으로 작성되었습니다.성공 케이스(올바른 인증), 실패 케이스(잘못된 비밀번호, 존재하지 않는 사용자, 빈 파라미터)를 모두 검증하며, Mockito verify를 사용한 동작 검증도 적절합니다.
41-95: 로그인 테스트 커버리지가 충분합니다.성공 케이스, 잘못된 비밀번호, 존재하지 않는 사용자, 빈 파라미터 등 주요 시나리오를 모두 검증하고 있습니다. 테스트 로직과 어설션이 정확합니다.
src/test/java/HttpRequestTest.java (4)
22-39: POST 요청 파싱 테스트가 올바르게 작성되었습니다.메서드, URL, 버전, 헤더, 바디를 포함한 모든 요소를 검증하며, 테스트 리소스 파일을 사용한 접근 방식이 실제 HTTP 요청과 유사하여 좋습니다.
41-58: GET 요청 파싱 테스트가 포괄적입니다.쿼리 파라미터와 쿠키 파싱을 검증하고, POST와 달리 body가 null임을 확인하는 등 GET 요청의 특성을 정확히 테스트합니다.
60-78: 에러 케이스 테스트가 적절합니다.잘못된 형식과 빈 요청 라인에 대한 예외 처리를
assertThrows로 검증하여, 파싱 견고성을 확인합니다.
22-78: HTTP 요청 파싱 테스트가 포괄적입니다.POST/GET 요청의 정상 파싱, 쿼리 파라미터/쿠키 처리, 잘못된 형식/빈 요청 라인에 대한 예외 처리 등 핵심 시나리오를 모두 검증하고 있습니다. 테스트 구조가 명확하고 어설션이 정확합니다.
src/main/java/http/HttpRequest.java (5)
20-35: HTTP 요청 파싱 팩토리 메서드가 올바르게 구현되었습니다.요청 라인 검증, 헤더 파싱, POST 요청에 대한 조건부 바디 읽기가 적절히 처리됩니다.
IOUtils를 통한 Content-Length 기반 바디 읽기도 정확합니다.
41-47: 과거 리뷰의 메서드 중복 문제가 올바르게 해결되었습니다.
getUrl()은 쿼리 스트링을 포함한 전체 경로를,getPath()는 쿼리 없는 경로를 반환하도록 명확히 구분되었습니다. 이는 과거 리뷰의 Option 2 권장사항과 일치합니다.
37-80: 접근자 메서드들이 올바르게 구현되었습니다.모든 HTTP 요청 요소(메서드, URL, 헤더, 쿠키, 바디)에 대한 접근자가 적절히 위임 패턴을 사용하여 구현되었으며, 캡슐화가 잘 유지됩니다.
41-47:getUrl()과getPath()메서드가 명확하게 구분되었습니다.이전 리뷰에서 제기된 중복 문제가 해결되었습니다:
getUrl(): 쿼리 스트링을 포함한 전체 경로 반환getPath(): 쿼리 스트링을 제외한 경로만 반환각 메서드가 명확하고 구분된 역할을 수행합니다.
20-35: HTTP 요청 파싱 로직이 올바르게 구현되었습니다.팩토리 메서드가 다음을 적절하게 처리합니다:
- 요청 라인 검증 (null/empty 체크)
- HttpStartLine 및 HttpHeaders로의 파싱 위임
- POST 요청의 Content-Length 기반 바디 읽기
파싱 로직이 견고하고 명확합니다.
| // 파라미터 유효성 검사 | ||
| if (userId == null || userId.isEmpty() || password == null || password.isEmpty()) { | ||
| log.log(Level.WARNING, "Login failed: missing parameters"); |
There was a problem hiding this comment.
이런 예외처리 부분이 너무 포괄적이라는 생각도 드네요
나중에 프론트에 에러 던져줄 거 생각해보면 "bad request"보다는 "userId가 비었습니다" 같은 문구가 원인 파악하기에 좋으니까 하나씩 빼서 다른 log 찍도록 처리하면 좋을것 같습니다!
There was a problem hiding this comment.
우선은 코드래빗 리뷰대로만 수정했습니다... <- 추석 이슈 입니다..
[이전에 프론트만 했덥 입장으로..]
백엔드에서 처리할
파라미터 유효성 검사에 대해서 많이 생각을 해봤는데...
이런 부분은 프론트에서 처리하는 게 맞지 않을까요?-?
서버에서 유효성 검사를 진행하는 게 맞을까요?-? @jyun-KIM
이럴 때 파트장님의 생각은 어떠하신지가 궁금하네요.
제 생각에는 예외처리 부분에서는 userId - password가 매칭이 안되는 것? 과 같이 유효성 검사를 다 뚫고 오는 것들만 처리해주는 게 맞는 거 같아서요...
There was a problem hiding this comment.
네 맞습니다. 성근님이 더 잘 아시겠지만 실제 프로젝트에서는 ID나 비밀번호 값이 비어있는 예외는 프론트엔드에서 처리해주는 것이 일반적이죠. 그리고 백엔드에서는 말씀하신 대로 ID를 받아 사용자가 존재하는지 확인하는 것부터 시작하고요.
다만 지금은 학습하는 과정에 있으니, 발생할 수 있는 예외 상황을 백엔드에서 하나하나 명시적으로 처리하는 연습을 해보는 것이 중요하다고 생각했습니다. 이런 경험이 나중에 안정적인 서비스를 만드는 데 좋은 밑거름이 될 것이라 생각해서 자세하게 말씀드렸습니다!
There was a problem hiding this comment.
| public enum UserField { | ||
| USER_ID("userId"), | ||
| PASSWORD("password"), | ||
| NAME("name"), | ||
| EMAIL("email"); |
There was a problem hiding this comment.
UserField enum 클래스로 지정하신 이유가 있나용?
다른 enum클래스와 다르게 특정 종류에 대한 것이 아니라고 느껴지는 내용인 듯 합니다
There was a problem hiding this comment.
단순히 매직 스트링 방지 때문이었습니다!
- 상수 클래스로도 할까 고민을 했었는데, UserField로 입력될 수 있는 부분에 대한 관리로 사용했습니다.
| } catch (IOException e) { | ||
| } catch (Exception e) { | ||
| log.log(Level.SEVERE, e.getMessage()); | ||
| System.out.println(Arrays.toString(e.getStackTrace())); |
There was a problem hiding this comment.
사소하지만 에러를 "로깅"하기 위한 목적이라면 logger를 쓰는게 더 적절할거 같습니다!(바로 위처럼!)
There was a problem hiding this comment.
아직 Logger에 대해 익숙치 않아, 그냥 확인하는 방향으로는 sout을 썼었네요.. 👎
There was a problem hiding this comment.
그렇다면 단순한 입출력과 로깅의 차이에 대해 한번 고민해보시면 좋을거 같아요~!🙌
| private final HttpRequest httpRequest; | ||
| private final HttpResponse httpResponse; | ||
|
|
||
| private static final Map<String, Controller> CONTROLLERS = WebConfig.configureControllers(); | ||
|
|
||
| public RequestMapper(HttpRequest httpRequest, HttpResponse httpResponse) { | ||
| this.httpRequest = httpRequest; | ||
| this.httpResponse = httpResponse; | ||
| } |
There was a problem hiding this comment.
RequestMapper가 HttpRequest와 HttpResponse 전체 객체를 필드로 갖고 있어야 할까요??
"Mapper"라는 이름에 맞게 "요청"과 "컨트롤러"를 매핑하는게 주 목적이라면 없어도 충분할거 같아요!
There was a problem hiding this comment.
Mapper는 매핑만 하면 되는데 실행에 필요한 객체까지 들고 있네요...
HttpRequest, HttpResponse를 사용하는 것이 아니라
param으로 들어온 path, method를 통해
String key = WebConfig.createKey(path, method);
를 통해 Controller만 찾아주면 되는.. 것이었네요
| public void proceed() throws IOException { | ||
| String path = httpRequest.getPath(); | ||
| HttpMethod method = httpRequest.getMethod(); | ||
|
|
||
| Controller controller = getController(path, method); | ||
| if (controller == null) { | ||
| controller = new ForwardController(); | ||
| } | ||
|
|
||
| controller.execute(httpRequest, httpResponse); | ||
| } |
There was a problem hiding this comment.
RequestMapper의 역할이 어떤 것인지 한번 더 고민해보시면 이 메서드가 필요 없을 수도 있을거 같아요!
There was a problem hiding this comment.
Mapper에서 매핑 + 실행 까지 모두 하고 있었네요. 없어져도 될 거 같습니다!
| /** | ||
| * Controller 매핑 설정을 담당하는 클래스 | ||
| * Spring의 @Configuration과 유사한 역할 | ||
| */ |
There was a problem hiding this comment.
Controller 인스턴스를 생성하고 URL 매핑을 해준다는 점에서 유사하다고 생각하였습니다.
- Bean 등록을 하는 부분과 유사하다고 생각했습니다.
src/main/java/http/HttpResponse.java
Outdated
|
|
||
| public class HttpResponse { | ||
| private final DataOutputStream dos; | ||
| private final String webappPath; |
There was a problem hiding this comment.
사용자가 하나의 요청을 하면 하나의 요청,응답 객체를 만드는 구조로 이해했어요!
이때 어떠한 이유에서 매 응답객체마다 webappPath가 필요한 건가요?
각 응답마다 다른 webappPath를 설정해 주어야 하는 걸까요?
There was a problem hiding this comment.
static으로 변경하는 게 가장 간단히 수정할 수 있는 방법 같아, 수정했습니다!
| // 경로 정규화 및 검증 | ||
| if (path.contains("..")) { | ||
| notFound(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
요청, 응답에서 모두 이 디렉토리 리스팅에 관한 검사가 진행되는게 인상깊네요!
응답 같은 경우 개발자가 의도한 바대로 진행될텐데 그럼에도 이 부분을 고려한 이유가 있을까요?
There was a problem hiding this comment.
휴먼 에러 방지용이었는데, 크게 의미는 없었습니다... ㅎㅎ
| OK(200, "HTTP/1.1 200 OK"), | ||
| FOUND(302, "HTTP/1.1 302 Found"), | ||
| NOT_FOUND(404, "HTTP/1.1 404 Not Found"); |
There was a problem hiding this comment.
응답 Http 버전이 항상 1.1인걸까요??
변경될 여지는 없을까요?
There was a problem hiding this comment.
혹시 요청 시에 들어오는 Http 버전과 같은 의미를 띄고 있지는 않을까요?
There was a problem hiding this comment.
당연히 변경됩니다... HTTP 응답 버전은 요청 버전과 동일해야 하는 게 맞습니다....
| @BeforeEach | ||
| public void setUp() { | ||
| MockitoAnnotations.openMocks(this); | ||
| controller = new ForwardController(); |
There was a problem hiding this comment.
매 테스트 마다 FowardController를 생성해야할 이유가 있나요?
There was a problem hiding this comment.
허허 없습니다! ㅎㅎ stateless이니, 인스턴스 재사용만 하면 될 거 같습니다 ㅎㅎ..
| // 테스트용 사용자 미리 등록 | ||
| User testUser = new User("testuser", "1234", "홍길동", "test@example.com"); | ||
| repository.addUser(testUser); |
There was a problem hiding this comment.
BeforeEach에 의해 동일한 계정정보를 가진 사용자가 매 테스트마다 1개, 2개, 3개... 쌓이는거 같은데 이러면 테스트 중에 문제가 생길 수도 있을거 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
이것은 기존 repository에 clearAll과 같은 메서드가 없어 그대로 두었습니다...
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/test/java/controller/UserLoginControllerTest.java (1)
30-39: 테스트 격리 개선을 권장합니다.
UserSignupControllerTest와 동일한 테스트 격리 문제가 있습니다. 각 테스트 실행 전에 동일한 사용자를 저장소에 추가하므로,Map.put이 기존 값을 덮어쓰긴 하지만 테스트 간 상태 공유가 발생합니다.저장소에
clear()메서드를 추가하고@BeforeEach에서 먼저 호출하여 깨끗한 상태에서 테스트를 시작하는 것을 권장합니다.src/main/java/http/HttpResponse.java (1)
28-51: 경로 순회 취약점이 여전히 존재합니다.Line 30의
".."체크는 부분적인 개선이지만 여전히 여러 공격 벡터에 취약합니다:
- URL 인코딩 우회:
%2e%2e%2f또는..%2f- 절대 경로:
/etc/passwd- 다중 슬래시:
....//- 대소문자 변형 (Windows):
..\또한 다음 문제들이 있습니다:
- 파일 I/O 예외 처리 누락: Line 43의
Files.readAllBytes()에서IOException이 발생하면 스트림이 불완전한 상태로 남을 수 있습니다.- 파일 타입 검증 누락: Line 38의
Files.exists()는 디렉토리도 통과시킵니다.다음과 같이 개선하세요:
public void forward(String path) throws IOException { - // 경로 정규화 및 검증 - if (path.contains("..")) { - notFound(); - return; - } - - String filePath = WEBAPP_PATH + path; - - // 파일 존재 확인 - if (!Files.exists(Paths.get(filePath))) { - notFound(); - return; - } - - byte[] fileContent = Files.readAllBytes(Paths.get(filePath)); - String contentType = ContentType.fromFileExtension(filePath).getValue(); + try { + // 경로 정규화 및 검증 + Path normalizedPath = Paths.get(WEBAPP_PATH, path).normalize(); + Path webappBase = Paths.get(WEBAPP_PATH).toAbsolutePath().normalize(); + + // webapp 디렉토리 외부 접근 차단 + if (!normalizedPath.toAbsolutePath().startsWith(webappBase)) { + notFound(); + return; + } + + // 파일 존재 및 타입 확인 + if (!Files.exists(normalizedPath) || !Files.isRegularFile(normalizedPath)) { + notFound(); + return; + } + + byte[] fileContent = Files.readAllBytes(normalizedPath); + String contentType = ContentType.fromFileExtension(normalizedPath.toString()).getValue(); - writeStatusLine(HttpStatus.OK); - writeHeader(HttpHeader.CONTENT_TYPE, contentType); - writeHeader(HttpHeader.CONTENT_LENGTH, String.valueOf(fileContent.length)); - writeEndOfHeaders(); - writeBody(fileContent); + writeStatusLine(HttpStatus.OK); + writeHeader(HttpHeader.CONTENT_TYPE, contentType); + writeHeader(HttpHeader.CONTENT_LENGTH, String.valueOf(fileContent.length)); + writeEndOfHeaders(); + writeBody(fileContent); + } catch (IOException e) { + notFound(); + } }주요 개선사항:
Path.normalize()와startsWith()검증으로 모든 경로 조작 시도 차단Files.isRegularFile()추가로 디렉토리 접근 방지- try-catch로 파일 I/O 예외 처리
🧹 Nitpick comments (3)
src/test/java/controller/UserSignupControllerTest.java (1)
31-36: 테스트 격리를 개선하는 것을 권장합니다.
MemoryUserRepository싱글톤을 사용하면서 테스트 간 초기화가 없어 상태가 누적됩니다. 현재는 각 테스트가 서로 다른 userId를 사용하므로 실질적인 충돌은 적지만, 테스트 실행 순서에 의존하게 되어 유지보수성이 저하됩니다.저장소에
clear()메서드를 추가하고@BeforeEach또는@AfterEach에서 호출하여 테스트 격리를 보장하는 것을 고려해 보세요.src/main/java/webserver/RequestHandler.java (1)
23-36: 추상화 통합이 잘 되었습니다.
HttpRequest와HttpResponse추상화를 통해 요청 처리 로직이 명확해졌고,RequestMapper와의 통합도 깔끔합니다. 요청 버전으로 응답을 생성하는 부분도 올바릅니다.예외 처리를 더 구체화하는 것을 고려해 보세요. 현재 모든
Exception을 포괄적으로 잡고 있어 복구 가능한 오류와 치명적인 오류를 구분하기 어렵습니다. 향후 특정 예외 타입별로 다른 처리 전략을 적용할 수 있도록 개선하면 좋습니다.src/main/java/http/HttpResponse.java (1)
19-26: 스트림 생명주기 문서화를 권장합니다.현재
HttpResponse는 전달받은OutputStream을 닫지 않습니다. 이는 호출자가 스트림을 소유하고 관리한다는 의도로 보이는데, 이를 명시적으로 문서화하면 사용자의 혼란을 줄일 수 있습니다.클래스 또는 생성자에 다음과 같은 JavaDoc을 추가하는 것을 고려해보세요:
/** * HTTP 응답을 생성하고 전송하는 유틸리티 클래스. * <p> * 참고: 이 클래스는 전달받은 OutputStream을 닫지 않습니다. * 호출자가 스트림의 생명주기를 관리할 책임이 있습니다. * </p> */ public class HttpResponse { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/controller/UserListController.java(1 hunks)src/main/java/controller/UserLoginController.java(1 hunks)src/main/java/controller/UserSignupController.java(1 hunks)src/main/java/http/HttpHeaders.java(1 hunks)src/main/java/http/HttpRequest.java(1 hunks)src/main/java/http/HttpResponse.java(1 hunks)src/main/java/http/HttpStartLine.java(1 hunks)src/main/java/http/enums/HttpStatus.java(1 hunks)src/main/java/webserver/RequestHandler.java(2 hunks)src/main/java/webserver/RequestMapper.java(1 hunks)src/test/java/HttpResponseTest.java(1 hunks)src/test/java/controller/ForwardControllerTest.java(1 hunks)src/test/java/controller/UserListControllerTest.java(1 hunks)src/test/java/controller/UserLoginControllerTest.java(1 hunks)src/test/java/controller/UserSignupControllerTest.java(1 hunks)src/test/java/webserver/RequestMapperTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/controller/UserListController.java
- src/main/java/http/HttpStartLine.java
- src/test/java/controller/ForwardControllerTest.java
- src/main/java/http/HttpRequest.java
- src/main/java/http/HttpHeaders.java
- src/test/java/HttpResponseTest.java
- src/main/java/controller/UserLoginController.java
🧰 Additional context used
🧬 Code graph analysis (6)
src/main/java/webserver/RequestHandler.java (2)
src/main/java/http/HttpRequest.java (1)
HttpRequest(12-107)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-93)
src/test/java/controller/UserListControllerTest.java (2)
src/main/java/http/HttpRequest.java (1)
HttpRequest(12-107)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-93)
src/test/java/controller/UserLoginControllerTest.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/HttpRequest.java (1)
HttpRequest(12-107)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-93)src/main/java/model/User.java (1)
User(5-46)
src/main/java/controller/UserSignupController.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/HttpRequest.java (1)
HttpRequest(12-107)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-93)src/main/java/model/User.java (1)
User(5-46)
src/main/java/webserver/RequestMapper.java (1)
src/main/java/webserver/WebConfig.java (1)
WebConfig(14-40)
src/test/java/controller/UserSignupControllerTest.java (4)
src/main/java/db/MemoryUserRepository.java (1)
MemoryUserRepository(9-35)src/main/java/http/HttpRequest.java (1)
HttpRequest(12-107)src/main/java/http/HttpResponse.java (1)
HttpResponse(13-93)src/main/java/model/User.java (1)
User(5-46)
🔇 Additional comments (11)
src/main/java/http/enums/HttpStatus.java (1)
3-27: 구현이 깔끔합니다!HTTP 상태 코드를 enum으로 관리하는 것은 타입 안전성을 제공하는 좋은 접근입니다. 현재 구현된 세 가지 상태 코드(OK, FOUND, NOT_FOUND)는 기본 기능에 충분합니다.
src/test/java/webserver/RequestMapperTest.java (1)
22-90: 매퍼 테스트 범위가 적절합니다.
RequestMapper의 단위 테스트로서 올바른 컨트롤러 인스턴스를 반환하는지 검증하는 것은 적절한 범위입니다. 컨트롤러의 실행 동작은 각 컨트롤러의 개별 테스트에서 검증되고 있으므로 책임 분리가 잘 되어 있습니다.src/main/java/webserver/RequestMapper.java (1)
8-24: 매핑 책임이 명확합니다!
RequestMapper가 라우팅 로직에만 집중하고 실행은 컨트롤러에 위임하는 설계가 깔끔합니다. 2단계 조회(path+method → path만) 및 기본 폴백 전략도 합리적입니다.src/test/java/controller/UserListControllerTest.java (1)
17-84: 접근 제어 시나리오가 잘 커버되었습니다.쿠키 기반 인가 로직의 주요 경로(로그인됨/안됨)를 명확하게 테스트하고 있으며, 테스트 이름도 의도를 잘 드러냅니다.
src/main/java/controller/UserSignupController.java (1)
19-49: 전반적인 구현이 견고합니다.컨트롤러 패턴을 잘 따르고 있으며, 입력 검증, 저장소 상호작용, 리다이렉션 흐름이 명확합니다. 유효성 검증 실패 시 응답 처리만 보완하면 프로덕션 수준에 가까워질 것입니다.
src/main/java/http/HttpResponse.java (6)
1-12: 임포트 구성이 적절합니다.필요한 모든 의존성이 올바르게 임포트되었습니다.
13-26: 클래스 구조가 적절합니다.
WEBAPP_PATH를 static으로 변경한 것은 이전 피드백을 잘 반영한 개선입니다. 생성자 오버로딩도 합리적입니다.
53-57: 리다이렉트 구현이 적절합니다.302 Found 상태 코드와 Location 헤더를 사용한 표준적인 리다이렉트 구현입니다.
59-64: 쿠키와 함께 리다이렉트하는 구현이 적절합니다.Set-Cookie를 먼저, Location을 나중에 설정하는 것은 일반적인 관례를 따르고 있습니다.
66-75: 404 응답 구현이 적절합니다.간단하지만 기능적으로 올바른 404 Not Found 응답입니다.
77-92: 헬퍼 메서드들이 HTTP 프로토콜을 정확히 구현하고 있습니다.CRLF(
\r\n) 사용과 flush 처리가 적절합니다.
3 participants
전체 흐름
과제1
과제 2
최종 아키텍처
Summary by CodeRabbit