Conversation
develop <- feat/web/handler/static/#6
[Merge] develop <- feat/http/converter/#12
- Unhandled Error를 처리하기 위해 핸들링 대상을 Exception -> Throwable 로 변경
- Exception들의 status와 메시지를 담고 있는 ErrorCode enum 정의
- 비지니스 로직 내 발생하는 Exception인 ServiceException 클래스 정의 - 비지니스 로직 외 Error를 던지기 위한 Exception 클래스인 ErrorException 클래스 정의
- 비지니스 로직 내 Exception인 ServiceException 처리 handler 개발 - 비지니스 로직 외 Error인 ErrorException 처리 handler 개발 - 핸들링 하지 못한 Unhandled Error들을 처리하는 handler 개발
develop <- feat/exception/#16
- 요청에 대한 logging 추가 - 매칭되는 Handler가 없을 시 ServiceException 반환 - 매칭되는 PostHandler가 없을 시 ErrorException 반환
- 정적 콘텐츠에 대한 Handler & Post handler가 파일을 읽을 때 기본 경로 수정 - 정적 콘텐츠에 대한 Handler & Post handler에서 경로 마지막에 /index.html을 붙인 path를 확인하도록 로직 수정 - 테스트 추가
- ThreadPool 사용
- HttpResponse::setBody 메서드 내부에서 Content-Length, Content-Type 등의 Body 관련 헤더를 설정하도록 책임 포인트 변경 - Post Handler의 File 읽기 관련 코드 리펙토링
- HttpServlet -> ConnectionHandler - WasServlet -> Dispatcher
- 구성 요소들을 web 패키지 아래에 수평적 구조로 구성 - exception 패키지 분리
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
종합 평가
이 PR은 대규모 패키지 구조 리팩토링으로, 프레임워크와 애플리케이션 코드의 분리, 설정 및 예외 처리 패키지 분리 등이 좋은 방향입니다. 그러나 몇 가지 실행 안정성 문제와 입력 검증 누락이 있습니다.
주요 이슈:
- 리소스 누수 - HttpBufferedReaderRequestConverter에서 try-with-resources 패턴이 깨짐
- Null 참조 오류 - ErrorExceptionHandler에서 getThrowable() null 체크 부재로 NPE 발생 가능
- 응답 처리 분산 - 정상/예외 경로에서 응답 작성 로직이 분리되어 있음
- 입력 검증 부족 - RegisterHandlerImpl에서 쿼리 파라미터 검증 없음
- 하드코딩된 경로 - VariableConfig의 정적 경로는 실행 환경에 따라 불안정
- 테스트 어려움 - WebServer의 static DependencyLoader는 의존성 주입 패턴 도입 시 문제 가능
패키지 구조 평가:
구조는 명확한 책임 분리를 보여주지만, 향후 handler-adapter 패턴 도입 시 web.posthandler 패키지 명칭을 web.response 또는 web.adapter로 변경하는 것이 더 직관적일 것 같습니다.
위의 인라인 코멘트들의 수정 후 재검토를 권장합니다."
| private static final Logger logger = LoggerFactory.getLogger(HttpBufferedReaderRequestConverter.class); | ||
| public HttpRequest parseRequest(Socket connection){ | ||
|
|
||
| try (InputStream in = connection.getInputStream()) { | ||
| try { | ||
| InputStream in = connection.getInputStream(); | ||
| InputStreamReader reader = new InputStreamReader(in); | ||
| BufferedReader bufferedReader = new BufferedReader(reader); | ||
| String firstLine = bufferedReader.readLine(); |
There was a problem hiding this comment.
리소스 누수 위험이 있습니다. try-with-resources 패턴을 깨뜨렸으므로, InputStream과 BufferedReader가 적절하게 닫히지 않을 수 있습니다. \n\njava\ntry (InputStream in = connection.getInputStream();\n InputStreamReader reader = new InputStreamReader(in);\n BufferedReader bufferedReader = new BufferedReader(reader)) {\n // 로직\n}\n\n\n로 변경하거나, 최소한 finally 블록에서 명시적으로 닫으세요."
| @Override | ||
| public void handle(Throwable t, Socket connection) { | ||
| ErrorException error = (ErrorException) t; | ||
| logger.debug(error.getThrowable().toString()); |
There was a problem hiding this comment.
ErrorException의 throwable이 null일 수 있는데, 이를 확인 없이 error.getThrowable().toString()을 호출하면 NullPointerException이 발생합니다. \n\njava\nif (error.getThrowable() != null) {\n logger.debug(error.getThrowable().toString());\n}\n\n\n로 null 체크를 추가하세요."
| responseConverter.sendResponse(response, connection); | ||
|
|
||
| } catch (Exception e){ | ||
| /** | ||
| * TODO: | ||
| * ExceptionHandler 또한 HttpResponse를 반환하게 하고 | ||
| * finally에 `responseConverter.sendResponse(response, connection);` 를 넣어 | ||
| * socket에 write를 하는 포인트를 단일 포인트로 관리 |
There was a problem hiding this comment.
TODO 주석에서 언급한 대로, 현재 구조는 응답 작성 포인트가 분산되어 있습니다(responseConverter.sendResponse vs exceptionHandlerMapping.handle).\n\n일관성을 위해 ExceptionHandler도 HttpResponse를 반환하도록 리팩터링하고, 단일 finally 블록에서 응답을 전송하는 방식으로 통합하세요. 이렇게 하면 소켓 I/O 오류 처리도 더 명확해집니다."
| try { | ||
| OutputStream raw = connection.getOutputStream(); | ||
| BufferedOutputStream out = new BufferedOutputStream(raw); | ||
|
|
||
| byte[] body = response.getBody(); | ||
| if (body == null) body = new byte[0]; | ||
|
|
||
| String statusLine = |
There was a problem hiding this comment.
body가 null이면 빈 배열로 치환하는 로직이 있지만, responseConverter.sendResponse()가 예외를 던질 경우 ConnectionHandler의 finally 블록이 실행되기 전에 socket이 닫히지 않을 수 있습니다.\n\nsendResponse 내 IOException 발생 시도 예외 처리를 강화하거나, 호출부(ConnectionHandler)에서 더 명확한 예외 전파 전략을 수립하세요."
| throw new ErrorException("Static content Read IO-Error"); | ||
| } |
There was a problem hiding this comment.
파일 읽기 중 IOException이 발생하면 ErrorException을 던지는데, 이 예외의 throwable이 null이므로 ErrorExceptionHandler에서 NPE가 발생합니다(위의 ErrorExceptionHandler 이슈 참고).\n\njava\nthrow new ErrorException(\"Static content Read IO-Error\", e);\n\n\n로 원본 예외를 전달하세요."
| public static final List<String> STATIC_RESOURCE_ROOTS = List.of( | ||
| "./src/main/resources", | ||
| "./src/main/resources/static"); |
There was a problem hiding this comment.
하드코딩된 경로는 상대 경로로 시스템 동작에 따라 불안정할 수 있습니다. 프로젝트 루트 기준으로 경로를 동적으로 계산하거나, 환경 변수/설정 파일에서 읽도록 변경하세요. 현재는 서버가 다른 디렉토리에서 실행될 경우 리소스를 찾지 못할 수 있습니다."
| public class WebServer { | ||
| private static final Logger logger = LoggerFactory.getLogger(WebServer.class); | ||
| private static final int DEFAULT_PORT = 8080; | ||
| private static final DependencyLoader LOADER = new DependencyLoader(); |
There was a problem hiding this comment.
static 필드로 DependencyLoader를 선언하는 것은 문제가 될 수 있습니다. 클래스 로딩 시점에 모든 의존성이 초기화되므로, 향후 의존성 주입 패턴 도입 시나 테스트 시 수정하기 어렵습니다. 가능하면 main 메서드 내에서 인스턴스화하고 필요한 의존성만 전달하는 방식을 고려하세요."
| public ViewResponse handle(HttpRequest request) { | ||
| String userId = request.getQueryValue("userId"); | ||
| String password = request.getQueryValue("password"); | ||
| String name = request.getQueryValue("name"); | ||
| String email = request.getQueryValue("email"); | ||
| Database.addUser(new User(userId, password, name, email)); | ||
| return ViewResponse.of("/login"); |
There was a problem hiding this comment.
사용자 입력값(userId, password, name, email)에 대한 유효성 검증이 없습니다. 빈 문자열, null, SQL 인젝션 패턴 등을 검증하지 않으면 런타임 오류나 데이터 무결성 문제가 발생할 수 있습니다. \n\n최소한 다음을 추가하세요:\njava\nif (userId == null || userId.isBlank()) throw new ServiceException(ErrorCode.MISSING_PARAMETER);\n\n\n또한 RegisterHandlerImpl의 필드명과 HTML 폼의 name 속성이 불일치합니다(nickname vs name)."
1 participant
💻 작업 내용
✨ 리뷰 포인트
대대적인 패키지구조 리팩터링을 했습니다.
현재 구조가 적절한지 피드백 주세요.
📝 메모
web 패키지쪽은 handler adapter를 사용하도록 리팩터링이 예정되어있습니다.
🎯 관련 이슈
closed #24