Conversation
- HttpResponse를 미리 생성한 뒤 흐름에 따라 status를 선택할 수 있도록 변경
- 필터 인터페이스 ServletFilter 정의 - ServletFilter를 chain으로 엮어 사용하는 FilterChain 클래스 개발
- Request에 대한 로그를 남기고 로깅용 request id(rid)를 남기는 AccessLogFilter 구현
- 접근 제한 path에 대한 접근 방어 필터 추가
- 필터 타입 별 필터 구성을 설정하는 FilterConfig 클래스 개발 - Path 별 필터 타입을 지정하는 SecurityConfig 클래스 개발
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| private List<ServletFilter> commonBackFilter(){ | ||
| return getOrCreate("commonFrontFilter", | ||
| () -> List.of()); |
There was a problem hiding this comment.
중복된 key로 인한 캐싱 버그: commonFrontFilter()와 commonBackFilter() 모두 동일한 key "commonFrontFilter"를 사용하고 있습니다. commonBackFilter()가 실제로는 빈 리스트를 반환해야 하지만, 캐싱으로 인해 앞의 값이 재사용될 수 있습니다.\n\n수정안: commonBackFilter()의 key를 \"commonBackFilter\"로 변경하세요."
There was a problem hiding this comment.
중복된 초기화 점검로직: config()와 setPaths() 메서드에서 동일하게 callCount > 0 검사를 수행하므로, 호출 시나리오에 따라 혼동이 생길 수 있습니다. 또한 callCount를 두 번 증가시킬 가능성이 있습니다.\n\n수정안: 메서드 인터페이스를 명확히 하기 위해 setPaths()를 private으로 만들거나, config() 메서드에서만 외부 호출을 허용하세요."
| private static final AppConfig LOADER = new AppConfig(); | ||
| private static final SecurityConfig securityConfig = new SecurityConfig(); | ||
| private static final FilterConfig filterConfig = new FilterConfig(); | ||
| private static final ExecutorService executor = Executors.newFixedThreadPool(32); |
There was a problem hiding this comment.
AppConfig 싱글톤 미보장: WebServer에서 LOADER = new AppConfig(), SecurityConfig, FilterConfig 등으로 각각 새로운 인스턴스를 생성하고 있습니다. 동시에 FilterConfig와 SecurityConfig 내부에서도 new AppConfig()를 호출하므로 싱글톤이 보장되지 않습니다. PR 설명에서 "DI가 복잡하게 꼬인 느낌"이라는 피드백과 연결됩니다.\n\n수정안: 싱글톤 패턴을 명확히 하거나, SingletonContainer 기반으로 통합 관리하도록 리팩토링하세요. 예: AppConfig를 정적 싱글톤으로 만들거나, 별도의 초기화 단계에서 통합 관리."
There was a problem hiding this comment.
generateRid() 중복 호출 위험: generateRid()를 여러 번 호출하면 rid 값이 덮어씌워지므로, 로그 추적 시 혼동을 초래할 수 있습니다. 또한 이미 rid가 설정된 후 다시 호출되는 경우 일관성이 깨집니다.\n\n수정안: 이미 rid가 존재하면 기존 값을 반환하도록 개선하거나, rid 초기화는 HttpRequest 생성 시점에 자동으로 수행하세요."
| return this; | ||
| } | ||
|
|
||
| public FilterChainContainer addPaths(FilterType type, List<String> paths) { | ||
| registeredPaths.add(FilterTypePaths.of(type, paths)); | ||
| return this; | ||
| } | ||
|
|
||
| public FilterChainContainer addPath(FilterType type, String path) { | ||
| registeredPaths.add(FilterTypePaths.of(type, List.of(path))); | ||
| return this; |
There was a problem hiding this comment.
경로 매칭 순서 문제: registeredPaths에 경로를 추가할 때마다 새로운 FilterTypePaths 객체를 생성하는데, 더 구체적인 경로가 먼저 정의되어야 합니다. 예: /restrict/**가 /**보다 먼저 등록되어야 정상 동작합니다.\n\n위험: 경로 등록 순서에 따라 의도와 다른 필터가 적용될 수 있습니다.\n\n수정안: SecurityConfig의 경로 등록 순서를 검토하거나, 경로 매칭 로직에서 가장 구체적인 패턴부터 확인하도록 변경하세요."
|
|
||
| HttpResponse response = HttpResponse.of(); | ||
| try { | ||
|
|
||
| HttpRequest request = requestConverter.parseRequest(connection); | ||
| HttpResponse response = dispatcher.handle(request); | ||
| filterChainContainer.runFilterChain(request,response); | ||
| responseConverter.sendResponse(response, connection); | ||
|
|
||
| } catch (Exception e){ | ||
| } catch (Throwable t){ | ||
| /** | ||
| * TODO: | ||
| * ExceptionHandler 또한 HttpResponse를 반환하게 하고 | ||
| * finally에 `responseConverter.sendResponse(response, connection);` 를 넣어 | ||
| * socket에 write를 하는 포인트를 단일 포인트로 관리 | ||
| */ | ||
| exceptionHandlerMapping.handle(e, connection); | ||
| exceptionHandlerMapping.handle(t, connection); | ||
| } finally { |
There was a problem hiding this comment.
HttpResponse 상태 관리 모호성: 예외 발생 시 filterChainContainer.runFilterChain() 호출이 중단되면, response 객체는 HttpStatus.NONE 상태로 유지됩니다. exceptionHandlerMapping.handle(t, connection)이 이 response 객체를 활용하지 못한다면, 예외 처리 응답이 제대로 구성되지 않을 수 있습니다.\n\n수정안: 예외 발생 시 response 객체를 활용하거나, 예외 핸들러가 자체적으로 응답을 생성하는 구조로 명확히 하세요. TODO 주석이 있으므로 추후 리팩토링 시 이를 반영하세요."
| byte[] body = in.readAllBytes(); | ||
|
|
||
| HttpResponse httpResponse = HttpResponse.of(handlerResponse.getStatus()); | ||
| httpResponse.setStatus(handlerResponse.getStatus()); |
There was a problem hiding this comment.
HttpResponse 상태 초기화 불완전: httpResponse는 HttpStatus.NONE 상태로 시작하지만, setStatus() 호출 전에 다른 렌더러가 httpResponse 객체를 변경하면 예상과 다른 결과가 나올 수 있습니다. 또한 HttpStatus.NONE이 실제로 유효한 상태인지 검증되지 않았습니다.\n\n수정안: httpResponse를 HttpStatus.NONE 상태로 초기화하는 것보다, 필요한 상태로 즉시 설정하거나, httpResponse의 유효성을 점검하는 로직을 추가하세요."
- FilterConfig 클래스 오타 수정 - SecurityConfig 클래스 중복 호출 방어 로직 수정 - HttpRequest.rid 중복 생성 방지 로직 추가
) develop <- feat/web/filter/#20
1 participant
💻 작업 내용
✨ 리뷰 포인트
필터와 필터 체인 관련 DI가 복잡하게 꼬인 느낌입니다.
FilterConfig클래스와SecurityConfig클래스가 적절한지, 구조적 개선안은 없을지 피드백 주세요.🎯 관련 이슈
closed #20