Refacto clean architecture routers#658
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
| extra={ | ||
| "user_id": request_context.get().user_id, | ||
| "router_name": body.name, | ||
| "error_type": type(e).__name__, | ||
| }, |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix log injection you should sanitize or normalize any user-controlled values before logging them, especially those that may contain newline or control characters. For plain-text logs, a common mitigation is to strip \r and \n (and optionally other control characters) so that user input cannot create extra log lines or visually forge entries.
For this specific case in api/infrastructure/fastapi/endpoints/admin_router.py, we should sanitize body.name before it is passed into the extra dict. The simplest fix that doesn’t change existing functionality is to derive a sanitized version of the router name (e.g., safe_router_name) in the exception handler and use that in the log entry instead of body.name. We can mimic the example pattern by removing newline characters with .replace('\r', '').replace('\n', ''). This avoids modifying the request handling logic or the error responses; it only affects what is logged. No new imports are required, since we use basic string methods.
Concretely:
- Inside the
except Exception as e:block, before callinglogger.exception, definesafe_router_name = body.name.replace('\r', '').replace('\n', '') if body and body.name is not None else None(or similar, handling potentialNone). - Use
safe_router_namein theextradict as the value for"router_name"instead ofbody.name. - Leave the rest of the function unchanged.
| @@ -42,11 +42,14 @@ | ||
| cost_completion_tokens=body.cost_completion_tokens, | ||
| ) | ||
| except Exception as e: | ||
| safe_router_name = None | ||
| if body is not None and getattr(body, "name", None) is not None: | ||
| safe_router_name = body.name.replace("\r", "").replace("\n", "") | ||
| logger.exception( | ||
| "Unexpected error while executing create_router use case", | ||
| extra={ | ||
| "user_id": request_context.get().user_id, | ||
| "router_name": body.name, | ||
| "router_name": safe_router_name, | ||
| "error_type": type(e).__name__, | ||
| }, | ||
| ) |
| async def create_router( | ||
| body: CreateRouter = Body(description="The router creation request."), | ||
| create_router_use_case: CreateRouterUseCase = Depends(create_router_use_case), | ||
| request_context: RequestContext = Depends(get_request_context), | ||
| ) -> CreateRouterResponse: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix “explicit returns mixed with implicit returns” in a function that has a declared return type, you ensure that all code paths either explicitly return a value of the annotated type or explicitly raise an exception, and that no control path can fall off the end of the function. For create_router, all known variants of result are handled, but there is no fallback for unexpected values, so the function might in theory finish without a return or raise. The safest, non‑behavior‑changing fix is to add a default case _ to the match that logs the unexpected state and raises an InternalServerHTTPException. This maintains the existing pattern: unexpected conditions are treated as internal server errors and logged.
Concretely, in api/infrastructure/fastapi/endpoints/admin_router.py, inside the match result: block of create_router, after the last specific case InsufficientPermissionError(): branch, add a case _: branch that logs an error (including the type and value of result and the request/user context, similar to the earlier logging) and then raises InternalServerHTTPException(). This ensures there is no implicit fall-through return, all control paths end in either a return CreateRouterResponse or a raise, and no new imports or helpers are needed because logger and InternalServerHTTPException are already imported in this file.
| @@ -61,3 +61,14 @@ | ||
| raise RouterAlreadyExistsHTTPException(name) | ||
| case InsufficientPermissionError(): | ||
| raise InsufficientPermissionHTTPException() | ||
| case _: | ||
| logger.error( | ||
| "Unexpected result type from create_router use case", | ||
| extra={ | ||
| "user_id": request_context.get().user_id, | ||
| "router_name": body.name, | ||
| "result_type": type(result).__name__, | ||
| "result": str(result), | ||
| }, | ||
| ) | ||
| raise InternalServerHTTPException() |
| async def execute( | ||
| self, | ||
| user_id: int, | ||
| name: str, | ||
| router_type: ModelType, | ||
| aliases: list[str], | ||
| load_balancing_strategy: RouterLoadBalancingStrategy, | ||
| cost_prompt_tokens: float, | ||
| cost_completion_tokens: float, | ||
| ) -> CreateRouterUseCaseResult: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix “explicit returns mixed with implicit returns” you should ensure that all control paths in the function return explicitly, and that no path can fall through to an implicit None. This often means adding a final return statement that either returns a sensible default or raises an error, consistent with the function’s type hints and usage.
For this specific execute method in api/use_cases/admin/_createrouterusecase.py, the simplest and safest fix without changing existing behavior is to add an explicit return at the end of the function, after the match block. Since all meaningful paths already return a valid CreateRouterUseCaseResult, this final return is effectively a defensive fallback; returning None would contradict the type alias, so the best option is to raise an exception indicating an unexpected state. That keeps the function total with respect to explicit returns and avoids silently returning None. Concretely, add a line like raise RuntimeError("Unexpected result from create_router") after the match block (line 55), indented to align with the match statement. No new imports or other definitions are required.
| @@ -52,3 +52,5 @@ | ||
| return CreateRouterUseCaseSuccess(router) | ||
| case error: | ||
| return error | ||
|
|
||
| raise RuntimeError("Unexpected result from create_router") |
e224553 to
8ccb02f
Compare
bf152da to
2065774
Compare
f19d6d6 to
9098eab
Compare
229bafc to
7a75078
Compare
No description provided.