Conversation
develop <- refactor/web/renderer/#31
- Socker->HttpRequest.body 변환 converter 개발 - DI 반영 - 테스트 반영
- HttpRequest 객체는 URI와 byte[]로 이루어진 body만 저장 - 필요에 따라 argument resolver에서 변환해서 사용
- web.dispatch 하위를 adapter와 argument로 분리
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
전체 검토 의견
이 PR은 HttpRequest의 body 파싱과 처리 로직을 구현하는 좋은 구조 개선입니다. 패키지 리팩토링(web.dispatch → web.dispatch.argument/resolver)과 책임 분리도 적절합니다. 다만 몇 가지 안정성 관련 이슈가 있습니다:
주요 개선사항 필요
- readLine() 구현의 \r\n 처리 - 현재 로직이 엣지 케이스에서 불안정할 수 있습니다.
- Null 참조 예외 - QueryParamsResolver에서 getContentType()과 getBody()가 null일 때의 처리 부재
- 예외 처리의 명확성 - EOF 발생 시 읽은 바이트 수 정보 추가로 디버깅 개선 권장
- QueryParameters.getQueryValue()의 범위 체크 - 키 존재 여부와 리스트 크기 검증 필요
- Content-Type 동기화 문제 - contentType 필드가 headers와 불일치할 가능성
일관성 문제
BufferedReaderHttpRequestConverter와 InputStreamHttpRequestConverter의 용도 구분이 모호합니다. 현재 설정상 InputStreamHttpRequestConverter를 사용하므로, BufferedReader 버전은 정리하거나 명확한 이유를 문서화하세요.
위 이슈들을 해결하면 안정성과 유지보수성이 크게 향상될 것입니다.
| request.setBody(body); | ||
| } | ||
|
|
||
| private String readLine(BufferedInputStream in) throws IOException { | ||
| ByteArrayOutputStream lineBuffer = new ByteArrayOutputStream(128); | ||
|
|
||
| int prev = -1; | ||
| while (true) { | ||
| int cur = in.read(); | ||
| if (cur == -1) { | ||
| if (lineBuffer.size() == 0) return null; | ||
| return lineBuffer.toString(); | ||
| } | ||
|
|
||
| if (prev == '\r' && cur == '\n') { | ||
| byte[] raw = lineBuffer.toByteArray(); | ||
| int len = raw.length; | ||
| if (len > 0 && raw[len - 1] == '\r') { | ||
| return new String(raw, 0, len - 1); | ||
| } | ||
| return lineBuffer.toString(); | ||
| } | ||
|
|
There was a problem hiding this comment.
readLine() 로직에서 \r\n 처리 오류 위험
현재 로직은 바이트 단위로 읽으면서 prev == '\r' && cur == '\n'일 때를 줄의 끝으로 판단합니다. 하지만 문제점:
lineBuffer에\r문자가 포함되어 있는 상태에서 확인하므로, 마지막\r을 제거하는 조건(if (len > 0 && raw[len - 1] == '\r'))이 불완전합니다.- 만약 버퍼에 여러 개의
\r이 있거나 예상치 못한 입력이 들어오면 정확하게 처리되지 않을 수 있습니다.
개선 방안:
if (prev == '\r' && cur == '\n') {
byte[] raw = lineBuffer.toByteArray();
// 마지막 \r 제거
String line = raw.length > 0 && raw[raw.length - 1] == '\r'
? new String(raw, 0, raw.length - 1)
: new String(raw);
return line;
}
```"| if(queryString==null) { | ||
| if (request.getContentType().strip().equalsIgnoreCase("application/x-www-form-urlencoded")) { | ||
| queryString = new String(request.getBody(), StandardCharsets.UTF_8); | ||
| } else throw new ErrorException("QueryParameterError: no query params"); //TODO: 이 부분을 ErrorException으로 처리해야할지, ServiceException으로 처리해야 할지 고민 | ||
| } |
There was a problem hiding this comment.
null 참조 오류 위험
request.getContentType()이 null을 반환할 수 있는데, 바로 .strip().equalsIgnoreCase()를 호출하면 NullPointerException이 발생합니다.
개선 방안:
String contentType = request.getContentType();
if(contentType != null && contentType.strip().equalsIgnoreCase("application/x-www-form-urlencoded")) {
queryString = new String(request.getBody(), StandardCharsets.UTF_8);
}
```"| public Optional<String> getQueryValue(String key){ | ||
| return Optional.ofNullable(params.get(key).get(0)); |
There was a problem hiding this comment.
IndexOutOfBoundsException 위험
params.get(key).get(0)은 해당 key가 없거나 리스트가 비어있을 때 예외를 발생시킵니다. 현재 parseQueryToMap은 항상 값이 추가되도록 하지만, API 설계상 명시적으로 확인하는 것이 안전합니다.
개선 방안:
public Optional<String> getQueryValue(String key){
List<String> values = params.get(key);
if(values == null || values.isEmpty()) return Optional.empty();
return Optional.of(values.get(0));
}
```"| } | ||
| } | ||
|
|
||
| private void readBody(BufferedInputStream in, HttpRequest request) throws IOException { | ||
| String contentLength = request.getHeader("Content-Length"); | ||
| if (contentLength == null) { | ||
| return; // body 없음 | ||
| } | ||
|
|
||
| int length; | ||
| try { | ||
| length = Integer.parseInt(contentLength.trim()); | ||
| } catch (NumberFormatException e) { | ||
| throw new ErrorException("Invalid Content-Length: " + contentLength, e); | ||
| } | ||
|
|
||
| if (length <= 0) return; | ||
|
|
||
| byte[] body = new byte[length]; |
There was a problem hiding this comment.
body 읽기 중 예상치 못한 EOF 발생 시 정보 손실
Unexpected EOF while reading body 예외 메시지가 사용자 입력의 불완전성으로 인한 일반적인 오류인지, 서버 버그인지 구분하기 어렵습니다. 또한 읽은 바이트 수를 로깅하면 디버깅에 도움이 됩니다.
개선 방안:
if (n < 0) {
throw new ErrorException(
String.format("HttpRequestParseError: Unexpected EOF while reading body. Expected=%d, Got=%d",
length, offset), e);
}
```"| } | ||
| public String getHeader(String key){ | ||
| return headers.get(key.toLowerCase()); | ||
| } | ||
|
|
||
| public void setHeader(String key, String value){ |
There was a problem hiding this comment.
Content-Type 설정 로직의 일관성 문제
setHeader에서만 Content-Type을 별도로 저장하므로, 만약 나중에 헤더를 직접 수정하면 contentType 필드와 실제 헤더 값이 불일치할 수 있습니다.
개선 방안:
getter에서 매번 headers에서 읽거나, 모든 헤더 수정을 통제하는 방식으로 통일하는 것을 권장합니다."
| queryString = new String(request.getBody(), StandardCharsets.UTF_8); | ||
| } else throw new ErrorException("QueryParameterError: no query params"); //TODO: 이 부분을 ErrorException으로 처리해야할지, ServiceException으로 처리해야 할지 고민 |
There was a problem hiding this comment.
요청 본문이 null일 때 NullPointerException 위험
POST 요청에서 Content-Type은 설정했으나 body가 null일 수 있습니다. request.getBody()가 null을 반환할 수 있으므로 검증이 필요합니다.
개선 방안:
byte[] body = request.getBody();
if(body == null) {
throw new ErrorException("QueryParameterError: Content-Type is form-urlencoded but body is null");
}
queryString = new String(body, StandardCharsets.UTF_8);
```"| private void setHeaders(BufferedReader bufferedReader, HttpRequest request) throws IOException { | ||
| String line; | ||
| while ((line = bufferedReader.readLine())!= null) { | ||
| if(line.isEmpty()) break; | ||
| int idx = line.indexOf(':'); | ||
| if(idx<0) throw new ErrorException("HttpRequestHeaderParseError"); | ||
| request.setHeader(line.substring(0, idx).strip(), line.substring(idx+1).strip()); | ||
| } |
There was a problem hiding this comment.
일관되지 않은 에러 처리
BufferedReaderHttpRequestConverter는 body 파싱을 구현하지 않았으면서 InputStreamHttpRequestConverter는 전체 body를 파싱합니다. 두 converter 중 어느 것을 실제 사용하는지(AppConfig에서 inputStreamHttpRequestConverter()로 설정) 명확하지 않으면 향후 유지보수 시 혼동이 발생할 수 있습니다.
BufferedReaderHttpRequestConverter는 더 이상 필요 없다면 제거하거나, 명확한 용도 구분을 문서화하세요."
1 participant
💻 작업 내용
🎯 관련 이슈
closed #34