refactor: exception handling and logging standardization - #97
Merged
Yu-JeSeung merged 3 commits intoApr 28, 2026
Merged
Conversation
📝 WalkthroughWalkthrough이 PR은 전역 구조화 로깅과 요청 컨텍스트, 커스텀 예외 계층 및 중앙 예외 처리기를 도입하고 관련 서비스 코드에서 로그·예외 흐름을 적용하며 Docker 빌드가 여러 디렉터리를 이미지로 복사하도록 업데이트합니다. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Middleware as "Request Logging\nMiddleware"
participant App as "FastAPI App\n(Routes/Services)"
participant Handler as "Exception\nHandlers"
participant Logger as "Logger\n(configured)"
Client->>Middleware: HTTP Request (maybe X-Request-ID)
Middleware->>Middleware: generate/extract X-Request-ID\nset request.state.request_id
Middleware->>Logger: log request start (method, path, request_id)
Middleware->>App: forward request
alt Success
App->>Logger: structured logs during processing
App-->>Middleware: Response
Middleware->>Logger: log completion (status, duration_ms, request_id)
Middleware-->>Client: Response + X-Request-ID header
else Error
App-->>Handler: exception raised
Handler->>Logger: log warning/error (inject request_id)
Handler-->>Middleware: JSON error response (error, code, request_id)
Middleware->>Logger: log failure (status, duration_ms, request_id)
Middleware-->>Client: JSON error + X-Request-ID header
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
main.py (1)
25-31:HTTPException(413)을AppError서브클래스로 통일하기 위해서는 먼저PayloadTooLargeError추가 필요 (Optional)PR의 방향성("HTTPException을 AppError 서브클래스로 교체")에 맞춰 이 미들웨어의
HTTPException(status_code=413, detail="File too large")을 교체하는 것이 좋습니다. 다만 현재core/exceptions/base.py에는 413 상태 코드용PayloadTooLargeError같은 서브클래스가 존재하지 않으므로, 먼저 다음과 같이 추가한 후:class PayloadTooLargeError(AppError): def __init__(self, message: str, *, code: str = "payload_too_large") -> None: super().__init__(message, status_code=413, code=code)그다음 미들웨어에서 이를 사용하면 됩니다. 참고로
core/exceptions/handlers.py에는 이미@app.exception_handler(HTTPException)핸들러가 등록되어 있으므로, 현재 코드도 동작상 문제는 없습니다. 다만 로그 추적 및 일관성 측면에서 명시적AppError서브클래스 사용을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 25 - 31, Add a new AppError subclass named PayloadTooLargeError (subclassing AppError, default status_code=413 and default code "payload_too_large") and replace the HTTPException raised in the limit_upload_size middleware with raising PayloadTooLargeError; locate the middleware function limit_upload_size and the AppError base class to implement the new class and then change the raise statement to use PayloadTooLargeError so the app uses a consistent AppError subtype for 413 responses.image_analysis/service.py (2)
137-174:locals().get("job_id")패턴 정리 권장.
locals()사전 조회는 변수 선언 시점에 따라 동작이 달라져 가독성과 정적 분석 친화성이 떨어집니다.try진입 전job_id: str | None = None으로 초기화한 뒤 직접 참조하면 의도가 명확해지고, 향후 리팩터 시 실수 가능성도 줄어듭니다.♻️ 제안 수정안
- try: + job_id: str | None = None + try: file_bytes = await file.read() @@ except Exception: - jid = locals().get("job_id") - if jid: - _job_store.pop(jid, None) + if job_id is not None: + _job_store.pop(job_id, None) _active_users.discard(user_id) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image_analysis/service.py` around lines 137 - 174, In enqueue_timetable_analysis, avoid using locals().get("job_id"); instead declare job_id: str | None = None before the outer try, assign to job_id where you currently generate the UUID, and in the exception handler reference that job_id variable to pop from _job_store and for any cleanup (also ensure _active_users.discard(user_id) still runs); this makes intent explicit and eliminates fragile locals() usage while keeping logic around _job_store, _active_users, and job queue timeout unchanged.
29-55:_require_spring_url이 임포트 시점에 캐시된 상수를 검증합니다.
SPRING_TIMETABLE_URL은 모듈 임포트 시 한 번 읽혀 고정되므로(Line 29),_require_spring_url는 사실상 매 호출마다 동일한 정적 값만 검사합니다. AI 요약의 "런타임 URL 검증" 의도와 다르게, 런타임에 설정이 갱신되더라도 반영되지 않습니다. 의도가 단순한 부재 가드라면 현재로도 충분하지만, 헬퍼명/주석을 그에 맞게 정리하거나 헬퍼 내부에서get_settings().spring_timetable_url을 다시 읽도록 변경하는 것이 더 일관됩니다.♻️ 제안 수정안 (헬퍼에서 매번 settings 읽기)
-SPRING_TIMETABLE_URL = settings.spring_timetable_url @@ -def _require_spring_url() -> str: - if not SPRING_TIMETABLE_URL: - raise ConfigurationError("SPRING_TIMETABLE_URL is required") - return SPRING_TIMETABLE_URL +def _require_spring_url() -> str: + url = get_settings().spring_timetable_url + if not url: + raise ConfigurationError("SPRING_TIMETABLE_URL is required") + return url🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@image_analysis/service.py` around lines 29 - 55, The helper _require_spring_url currently validates the module-level constant SPRING_TIMETABLE_URL which is captured at import time; change it to read the setting dynamically instead so runtime changes are respected: inside _require_spring_url call get_settings().spring_timetable_url (instead of using SPRING_TIMETABLE_URL), validate that value and raise ConfigurationError if missing, and update any callers or docstring accordingly (or alternatively rename the helper if you prefer it to be a one-time import guard); reference symbols: _require_spring_url, SPRING_TIMETABLE_URL, get_settings(), ConfigurationError.core/exceptions/handlers.py (1)
77-86:logger.exception호출의exc_info=exc는 중복입니다.
logger.exception(...)은 기본적으로exc_info=True로 동작하며 현재 처리 중인 예외를 자동으로 첨부합니다. 명시 전달이 잘못된 것은 아니지만 가독성을 위해 제거하거나, 명시적으로 첨부하고 싶다면logger.error(..., exc_info=exc)로 통일하는 편이 의도가 더 명확합니다.♻️ 제안 수정안
- logger.exception("unhandled_exception path=%s", request.url.path, exc_info=exc) + logger.exception("unhandled_exception path=%s", request.url.path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/exceptions/handlers.py` around lines 77 - 86, The call inside unhandled_exception_handler currently passes exc_info=exc to logger.exception which is redundant; update the logging to either remove the explicit exc_info and keep logger.exception("unhandled_exception path=%s", request.url.path) or replace the call with logger.error("unhandled_exception path=%s", request.url.path, exc_info=exc) to make the intent explicit; adjust only the logging line (referencing unhandled_exception_handler, logger.exception, logger.error and exc_info) so the exception info is attached cleanly and code readability is improved.core/logging/config.py (1)
24-37: 재호출 시 핸들러 레벨만 갱신되는 점은 좋습니다.
_CONFIGURED플래그로 중복 핸들러 부착을 방지하되, 재호출 시 루트/핸들러 레벨은 새로 반영하도록 한 분기는 운영 중 로그 레벨 변경 시나리오에서 유용합니다. 다만_SERVICE_NAME은 매 호출마다 덮어써지므로, 한 프로세스에서 서로 다른 서비스명으로 두 번 호출되면 마지막 값으로 통일된다는 점은 운용상 기억해두시면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/logging/config.py` around lines 24 - 37, The code currently overwrites the global _SERVICE_NAME on every configure_logging call which can cause inconsistent service names if configure_logging is invoked multiple times; change configure_logging so it only assigns _SERVICE_NAME when not already set (e.g., if not _CONFIGURED or if _SERVICE_NAME is falsy) while keeping the existing behavior of updating root/handler log levels on reconfiguration; update the logic around _CONFIGURED and _SERVICE_NAME in configure_logging so that _SERVICE_NAME is preserved after the first assignment but log levels are still applied on subsequent calls.core/exceptions/base.py (1)
4-55: 예외 계층 구조 설계가 명확합니다.
AppError를 베이스로 HTTP 상태 코드와code식별자를 갖는 서브클래스를 일관되게 정의했고, 키워드 전용 인자로 시그니처가 통일되어 있어 핸들러에서 일괄 처리하기 좋습니다. 특별한 결함은 없습니다.선택적으로, 핸들러에서 응답 본문에 부가 컨텍스트(예: 검증 실패 필드, 외부 시스템 식별자 등)를 실어야 하는 경우를 대비해
AppError에 선택적details: dict | None필드를 추가해 두면, 추후 각 호출부에서 시그니처를 깨뜨리지 않고 확장할 수 있어 유용합니다. 현재 사용 범위에서 필요 없다면 무시하셔도 됩니다.♻️ 선택적 확장 예시
class AppError(Exception): def __init__( self, message: str, *, status_code: int = 400, code: str = "application_error", + details: dict | None = None, ) -> None: super().__init__(message) self.message = message self.status_code = status_code self.code = code + self.details = details🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/exceptions/base.py` around lines 4 - 55, Add an optional details: dict | None field to AppError and propagate it through every subclass so callers can supply extra context without breaking existing call sites: update AppError.__init__ signature to include *, details: dict | None = None, set self.details = details, and update each subclass constructor (BadRequestError, UnauthorizedError, ForbiddenError, NotFoundError, ConflictError, ServiceUnavailableError, GatewayTimeoutError, ConfigurationError) to accept the same kw-only details: dict | None = None and pass details=details when calling super().__init__ so the extra context is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/exceptions/handlers.py`:
- Around line 46-57: The HTTPException handler currently drops
HTTPException.headers when delegating to _error_response; update
http_exception_handler to pass exc.headers through (or an empty dict when None)
into the call to _error_response so response headers like WWW-Authenticate are
preserved; locate the async function http_exception_handler and modify its
return invocation of _error_response to accept and forward exc.headers
(merging/normalizing None to {} as needed).
In `@core/logging/middleware.py`:
- Around line 18-19: 클라이언트가 보낸 X-Request-ID 값을 검증하지 않아 로그 인젝션 위험이 있으므로
request.headers.get("X-Request-ID")로 받은 값에 대해 허용 문자와 길이 검증을 추가하고 유효하지 않으면 새
UUID로 대체하도록 변경하세요; 구체적으로 core/logging/middleware.py에서 request_id =
request.headers.get("X-Request-ID") 대신 수신값을 정규식(예: UUID 패턴 또는 영숫자/하이픈만 허용)과 최대
길이 검사로 검증한 후 유효하면 set_request_id(request_id)로 전달하고, 유효하지 않으면
set_request_id(str(uuid.uuid4()))를 호출하도록 구현해 _DEFAULT_FORMAT에 들어가는
%(request_id)s에 안전한 값만 노출되게 하세요.
In `@LLM/OSS/service.py`:
- Around line 122-129: The current _get_oss_client() raises ConfigurationError
but call_oss() swallows all exceptions in its broad try/except, preventing
register_exception_handlers from handling configuration faults; either (A) move
OSS settings validation out of _get_oss_client() into app startup
(lifespan/module import) so missing OSS_API_KEY/OSS_MODEL raises
ConfigurationError during boot, or (B) narrow call_oss()'s exception handling so
ConfigurationError (or AppError) is re-raised while only transient OpenAI call
failures are caught and fall back — update the call_oss() except block to
re-raise ConfigurationError (refer to _get_oss_client, call_oss,
ConfigurationError, register_exception_handlers) or add startup validation that
uses the same checks currently in _get_oss_client().
---
Nitpick comments:
In `@core/exceptions/base.py`:
- Around line 4-55: Add an optional details: dict | None field to AppError and
propagate it through every subclass so callers can supply extra context without
breaking existing call sites: update AppError.__init__ signature to include *,
details: dict | None = None, set self.details = details, and update each
subclass constructor (BadRequestError, UnauthorizedError, ForbiddenError,
NotFoundError, ConflictError, ServiceUnavailableError, GatewayTimeoutError,
ConfigurationError) to accept the same kw-only details: dict | None = None and
pass details=details when calling super().__init__ so the extra context is
preserved.
In `@core/exceptions/handlers.py`:
- Around line 77-86: The call inside unhandled_exception_handler currently
passes exc_info=exc to logger.exception which is redundant; update the logging
to either remove the explicit exc_info and keep
logger.exception("unhandled_exception path=%s", request.url.path) or replace the
call with logger.error("unhandled_exception path=%s", request.url.path,
exc_info=exc) to make the intent explicit; adjust only the logging line
(referencing unhandled_exception_handler, logger.exception, logger.error and
exc_info) so the exception info is attached cleanly and code readability is
improved.
In `@core/logging/config.py`:
- Around line 24-37: The code currently overwrites the global _SERVICE_NAME on
every configure_logging call which can cause inconsistent service names if
configure_logging is invoked multiple times; change configure_logging so it only
assigns _SERVICE_NAME when not already set (e.g., if not _CONFIGURED or if
_SERVICE_NAME is falsy) while keeping the existing behavior of updating
root/handler log levels on reconfiguration; update the logic around _CONFIGURED
and _SERVICE_NAME in configure_logging so that _SERVICE_NAME is preserved after
the first assignment but log levels are still applied on subsequent calls.
In `@image_analysis/service.py`:
- Around line 137-174: In enqueue_timetable_analysis, avoid using
locals().get("job_id"); instead declare job_id: str | None = None before the
outer try, assign to job_id where you currently generate the UUID, and in the
exception handler reference that job_id variable to pop from _job_store and for
any cleanup (also ensure _active_users.discard(user_id) still runs); this makes
intent explicit and eliminates fragile locals() usage while keeping logic around
_job_store, _active_users, and job queue timeout unchanged.
- Around line 29-55: The helper _require_spring_url currently validates the
module-level constant SPRING_TIMETABLE_URL which is captured at import time;
change it to read the setting dynamically instead so runtime changes are
respected: inside _require_spring_url call get_settings().spring_timetable_url
(instead of using SPRING_TIMETABLE_URL), validate that value and raise
ConfigurationError if missing, and update any callers or docstring accordingly
(or alternatively rename the helper if you prefer it to be a one-time import
guard); reference symbols: _require_spring_url, SPRING_TIMETABLE_URL,
get_settings(), ConfigurationError.
In `@main.py`:
- Around line 25-31: Add a new AppError subclass named PayloadTooLargeError
(subclassing AppError, default status_code=413 and default code
"payload_too_large") and replace the HTTPException raised in the
limit_upload_size middleware with raising PayloadTooLargeError; locate the
middleware function limit_upload_size and the AppError base class to implement
the new class and then change the raise statement to use PayloadTooLargeError so
the app uses a consistent AppError subtype for 413 responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f8a1e2e-c0df-4f34-9e20-812b79ac356e
📒 Files selected for processing (17)
DockerfileDockerfiles_ossLLM/OSS/service.pyapp_oss_main.pycore/auth.pycore/exceptions/__init__.pycore/exceptions/base.pycore/exceptions/handlers.pycore/logging/__init__.pycore/logging/config.pycore/logging/context.pycore/logging/middleware.pyimage_analysis/service.pymain.pytext_filtering/service.pytext_filtering/text_filtering.pytext_filtering/text_filtering_rule.py
Comment on lines
+18
to
+19
| request_id = request.headers.get("X-Request-ID") or str(uuid.uuid4()) | ||
| set_request_id(request_id) |
There was a problem hiding this comment.
클라이언트가 보낸 X-Request-ID에 대한 검증이 없어 로그 인젝션 가능성이 있습니다.
request.headers.get("X-Request-ID") 값이 그대로 set_request_id로 들어가고 이후 _DEFAULT_FORMAT의 %(request_id)s를 통해 로그 라인에 직접 출력됩니다. 외부 클라이언트가 개행 문자나 제어 문자(\n, \r)를 포함한 헤더를 전송할 경우 위조된 로그 라인을 삽입할 수 있어, 추후 로그 분석/모니터링 신뢰도를 떨어뜨릴 수 있습니다. 길이 제한과 허용 문자(예: UUID 또는 영숫자/하이픈)만 통과시키고, 그 외에는 새 UUID로 대체하는 것을 권장합니다.
🛡️ 제안 수정안
+import re
+
+_REQUEST_ID_RE = re.compile(r"^[A-Za-z0-9._-]{1,128}$")
+
+
+def _safe_request_id(raw: str | None) -> str:
+ if raw and _REQUEST_ID_RE.match(raw):
+ return raw
+ return str(uuid.uuid4())
+
+
def register_request_logging(app: FastAPI) -> None:
`@app.middleware`("http")
async def request_logging_middleware(request: Request, call_next):
- request_id = request.headers.get("X-Request-ID") or str(uuid.uuid4())
+ request_id = _safe_request_id(request.headers.get("X-Request-ID"))
set_request_id(request_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/logging/middleware.py` around lines 18 - 19, 클라이언트가 보낸 X-Request-ID 값을
검증하지 않아 로그 인젝션 위험이 있으므로 request.headers.get("X-Request-ID")로 받은 값에 대해 허용 문자와 길이
검증을 추가하고 유효하지 않으면 새 UUID로 대체하도록 변경하세요; 구체적으로 core/logging/middleware.py에서
request_id = request.headers.get("X-Request-ID") 대신 수신값을 정규식(예: UUID 패턴 또는
영숫자/하이픈만 허용)과 최대 길이 검사로 검증한 후 유효하면 set_request_id(request_id)로 전달하고, 유효하지 않으면
set_request_id(str(uuid.uuid4()))를 호출하도록 구현해 _DEFAULT_FORMAT에 들어가는
%(request_id)s에 안전한 값만 노출되게 하세요.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/exceptions/handlers.py (1)
86-95:logger.exception에exc_info=exc전달은 중복입니다.
logger.exception(...)은 기본적으로exc_info=True로 동작하여 현재 처리 중인 예외의 스택 트레이스를 기록합니다. 명시적으로exc_info=exc를 전달해도 동작은 비슷하지만, 의도가 모호해지므로 통상 둘 중 하나를 사용합니다.♻️ 제안 수정안
- logger.exception("unhandled_exception path=%s", request.url.path, exc_info=exc) + logger.exception("unhandled_exception path=%s", request.url.path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/exceptions/handlers.py` around lines 86 - 95, The logger.exception call in unhandled_exception_handler redundantly passes exc_info=exc; logger.exception already logs the current exception (exc_info=True) so remove the exc_info=exc argument from the logger.exception("unhandled_exception path=%s", request.url.path, ...) call (or alternatively change to logger.error(..., exc_info=exc) if you intend to supply a specific exception), leaving a single clear call in unhandled_exception_handler.LLM/OSS/service.py (1)
142-156:_get_oss_client()을 두 번 호출하고 있어, 라인 143에서 얻은client변수가 사용되지 않습니다.라인 143에서
client = _get_oss_client()로 클라이언트를 받아ConfigurationError를 통과시키는 의도는 좋지만, 실제 호출은 라인 147에서_get_oss_client()를 다시 부르고 있어 변수client가 사용되지 않는 dead code가 되었습니다. 또한 두 번의 try 블록으로 분리할 필요 없이ConfigurationError만 통과시키는 형태로 단순화할 수 있습니다.♻️ 제안 수정안
- try: - client = _get_oss_client() - except ConfigurationError: - raise - try: - response = _get_oss_client().chat.completions.create( + client = _get_oss_client() + try: + response = client.chat.completions.create( model=settings.oss_model, messages=messages, temperature=kwargs.get("temperature", 0.3), max_tokens=kwargs.get("max_tokens", 64), ) return (response.choices[0].message.content or "").strip() except Exception: logger.warning("oss_call_failed", exc_info=True) return ""이렇게 하면
_get_oss_client()에서 발생하는ConfigurationError는 자연스럽게 호출자로 전파되고, 이후 OpenAI 호출 단계의 일시적 예외만 fallback으로 흡수됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LLM/OSS/service.py` around lines 142 - 156, The code calls _get_oss_client() twice and never uses the first result (client), creating dead code and unnecessary duplication; change the logic to call client = _get_oss_client() once (letting ConfigurationError propagate) and then use that client for the chat completion call (client.chat.completions.create(...)), combining the try blocks so only the external call is wrapped in the broad exception handler that logs via logger.warning("oss_call_failed", exc_info=True) and returns "" on failure; remove the unused variable and duplicate call to clean up the flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/exceptions/handlers.py`:
- Around line 55-66: The exception handler is only registered for
fastapi.HTTPException and thus misses Starlette-raised HTTPExceptions (e.g.,
404/405); update the decorator registration to use
starlette.exceptions.HTTPException so the existing
http_exception_handler(request: Request, exc: HTTPException) (which calls
set_request_id(_request_id(request)), logs and returns _error_response(...,
extra_headers=exc.headers)) will catch both Starlette and FastAPI HTTPExceptions
and ensure unified `{error, code, request_id}` responses and X-Request-ID header
for routing errors.
---
Nitpick comments:
In `@core/exceptions/handlers.py`:
- Around line 86-95: The logger.exception call in unhandled_exception_handler
redundantly passes exc_info=exc; logger.exception already logs the current
exception (exc_info=True) so remove the exc_info=exc argument from the
logger.exception("unhandled_exception path=%s", request.url.path, ...) call (or
alternatively change to logger.error(..., exc_info=exc) if you intend to supply
a specific exception), leaving a single clear call in
unhandled_exception_handler.
In `@LLM/OSS/service.py`:
- Around line 142-156: The code calls _get_oss_client() twice and never uses the
first result (client), creating dead code and unnecessary duplication; change
the logic to call client = _get_oss_client() once (letting ConfigurationError
propagate) and then use that client for the chat completion call
(client.chat.completions.create(...)), combining the try blocks so only the
external call is wrapped in the broad exception handler that logs via
logger.warning("oss_call_failed", exc_info=True) and returns "" on failure;
remove the unused variable and duplicate call to clean up the flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d2b9539-5e94-4a9f-8340-ba050bea3d34
📒 Files selected for processing (2)
LLM/OSS/service.pycore/exceptions/handlers.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/exceptions/handlers.py (1)
28-30:X-Request-ID가 덮어써질 여지가 있습니다.Line 28-30에서
extra_headers를 그대로update하면, 외부에서 전달된X-Request-ID가 내부 추적 ID를 덮어쓸 수 있습니다. 추적 무결성을 위해X-Request-ID는 항상 내부 값을 유지하는 편이 안전합니다.제안 패치
headers = {"X-Request-ID": request_id} if extra_headers: - headers.update(extra_headers) + headers.update( + {k: v for k, v in extra_headers.items() if k.lower() != "x-request-id"} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/exceptions/handlers.py` around lines 28 - 30, The current headers update uses headers.update(extra_headers) which allows an external "X-Request-ID" to overwrite the internal request_id; change the merge so the internal X-Request-ID in headers (the variable request_id) is preserved — when incorporating extra_headers (variable extra_headers) filter out any "X-Request-ID" key (case-insensitively) before calling update or only copy keys that are not "X-Request-ID" into headers to ensure the internal trace id in headers is never replaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/exceptions/handlers.py`:
- Around line 75-79: The current logger.warning call logs exc.errors(), which
may include raw user input; change it to log only the number of errors to avoid
exposing user data: replace the existing
logger.warning("request_validation_failed path=%s errors=%s", request.url.path,
exc.errors()) usage in the exception handler with a message that uses the error
count (e.g., compute count = len(exc.errors()) or similar) and include that
count instead of exc.errors() so only the error quantity is recorded.
---
Nitpick comments:
In `@core/exceptions/handlers.py`:
- Around line 28-30: The current headers update uses
headers.update(extra_headers) which allows an external "X-Request-ID" to
overwrite the internal request_id; change the merge so the internal X-Request-ID
in headers (the variable request_id) is preserved — when incorporating
extra_headers (variable extra_headers) filter out any "X-Request-ID" key
(case-insensitively) before calling update or only copy keys that are not
"X-Request-ID" into headers to ensure the internal trace id in headers is never
replaced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a43f0957-63d7-40a1-944b-3ec138d2c534
📒 Files selected for processing (1)
core/exceptions/handlers.py
Comment on lines
+75
to
+79
| logger.warning( | ||
| "request_validation_failed path=%s errors=%s", | ||
| request.url.path, | ||
| exc.errors(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastAPI + Pydantic v2에서 RequestValidationError.errors() 결과에 사용자가 보낸 input 값(원문 데이터)이 포함되는지 공식 문서/예제로 확인해 주세요.
💡 Result:
결론: FastAPI + Pydantic v2에서 RequestValidationError.errors 결과(각 error dict)에, 사용자가 보낸 원문 input 값이 포함됩니다. 또한 Pydantic v2의 ValidationError.errors 기본 동작에서도 include_input 기본값이 True라서 input이 포함됩니다. 근거 요약 1) FastAPI 문서(Handling Errors)에서 RequestValidationError가 invalid data로부터 받은 body(exc.body)를 별도로 사용할 수 있음을 명시합니다. 문서 예제는 exc.errors와 exc.body를 함께 내려주며, 이는 FastAPI의 RequestValidationError가 입력값을 가지고 있음을 보여줍니다. (다만 질문의 “errors 내부에 input이 포함되느냐”는 Pydantic v2의 errors 스펙으로 확인됩니다.) 2) Pydantic v2 공식 문서에서 errors의 ErrorDetails 딕셔너리에 input(“validation을 위해 제공된 input”) 필드가 있다고 명시합니다. 예시 출력에서도 error dict에 'input': ... 값이 포함되어 출력됩니다. 3) Pydantic v2 pydantic_core ValidationError.errors 시그니처 문서에서도 include_input 파라미터가 기본값 True이며, “각 에러의 input value 포함 여부”를 제어한다고 명시합니다. 따라서 FastAPI가 내부적으로 호출하는 Pydantic v2 errors 기본 설정에서는 input이 포함됩니다. 주의/실무 팁(보안 관점) - input 값이 에러 응답에 그대로 포함되면(기본 동작) 사용자 입력 자체를 로깅/노출할 수 있으므로, 필요하면 커스텀 exception handler에서 input을 제거(또는 include_input=False 사용)하는 방식을 고려해야 합니다. (FastAPI 문서 자체도 예시에서 errors를 가공하는 형태를 보여줍니다.)
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/handling-errors/?h=validation
- 2: https://codingeasypeasy.com/blog/fastapi-requestvalidationerror-handling-errors-gracefully-and-providing-useful-feedback/
- 3: https://docs.pydantic.dev/dev-v2/errors/errors/
- 4: https://pydantic.dev/docs/validation/2.0/api/pydantic-core/pydantic_core_init/
🏁 Script executed:
# Check if the file exists and read the relevant lines
file_path="core/exceptions/handlers.py"
if [ -f "$file_path" ]; then
echo "=== File found, checking line count ==="
wc -l "$file_path"
echo ""
echo "=== Reading lines 70-85 for context ==="
sed -n '70,85p' "$file_path"
echo ""
echo "=== Full handler function context (lines 1-100) ==="
head -100 "$file_path"
else
echo "File not found. Searching for handlers.py"
fd "handlers.py"
fiRepository: dongsooop/AI
Length of output: 3890
요청 검증 오류 로그에서 사용자 입력값 노출 방지 필요
exc.errors() 는 Pydantic v2의 기본 동작에서 사용자가 보낸 원문 데이터를 포함합니다. 운영 환경의 로그에 민감 정보가 기록되지 않도록 오류 개수만 로깅하세요.
제안 패치
- logger.warning(
- "request_validation_failed path=%s errors=%s",
- request.url.path,
- exc.errors(),
- )
+ logger.warning(
+ "request_validation_failed path=%s error_count=%d",
+ request.url.path,
+ len(exc.errors()),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.warning( | |
| "request_validation_failed path=%s errors=%s", | |
| request.url.path, | |
| exc.errors(), | |
| ) | |
| logger.warning( | |
| "request_validation_failed path=%s error_count=%d", | |
| request.url.path, | |
| len(exc.errors()), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/exceptions/handlers.py` around lines 75 - 79, The current logger.warning
call logs exc.errors(), which may include raw user input; change it to log only
the number of errors to avoid exposing user data: replace the existing
logger.warning("request_validation_failed path=%s errors=%s", request.url.path,
exc.errors()) usage in the exception handler with a message that uses the error
count (e.g., compute count = len(exc.errors()) or similar) and include that
count instead of exc.errors() so only the error quantity is recorded.
Yu-JeSeung
deleted the
refactor/exception_handling_logging_standardization
branch
April 28, 2026 01:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
관련 이슈
Close #91
🎯 배경
🔍 주요 내용
core폴더를 기준으로 공통 로깅/예외처리 체계 확립auth를 통한 JWT 공통화textfilter무지성 try/except 명시적 처리변경 요약
예외 처리와 로깅을 core/로 통합해 표준화했습니다. JWT 처리와 서비스별 에러를 커스텀 예외로 통일하고, 무분별한 try/except를 제거해 명시적 오류 흐름과 구조화된 로그를 적용했습니다.
주요 변경점
주의/리스크
다음 액션