feat(refacto): get router endpoint toward clean architecture#707
Conversation
leoguillaume
changed the title
benjaminpilia
force-pushed
the
refacto_get_router_endpoint
branch
from
b990651 to
906eb85
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| async def get_router( | ||
| router_id: int = Path(description="The router ID."), | ||
| get_one_router_use_case: GetOneRouterUseCase = Depends(get_one_router_use_case_factory), | ||
| request_context: ContextVar[RequestContext] = Depends(get_request_context), | ||
| ) -> Router: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix mixed explicit/implicit returns, ensure that all control-flow paths in a function return explicitly, including the “default” or “should not happen” path. Here, get_router explicitly returns a Router for the success case and raises exceptions for known error cases, but there is no explicit handling for unexpected result values, and no explicit return at the end. The best fix, without changing intended functionality, is to make the match exhaustive by adding a final case _ that logs the unexpected value and raises InternalServerHTTPException, mirroring the behavior already used in the try/except block above. This keeps the function’s contract (never returning None, always either returning a Router or raising an HTTP exception) and removes the implicit fallthrough.
Concretely, in api/infrastructure/fastapi/endpoints/admin/routers.py, within the get_router function, adjust the match result: block (lines 115–121) by adding a final case _: clause that logs an error about the unexpected result and raises InternalServerHTTPException(). No new imports are needed because InternalServerHTTPException and logger are already in scope. No other parts of the file need to change.
| @@ -119,6 +119,16 @@ | ||
| raise RouterNotFoundHTTPException(not_found_id) | ||
| case UserIsNotAdminError(): | ||
| raise NotAdminUserHTTPException() | ||
| case _: | ||
| logger.exception( | ||
| "Unexpected result type from get_one_router use case", | ||
| extra={ | ||
| "user_id": command.user_id, | ||
| "router_id": command.router_id, | ||
| "result_type": type(result).__name__, | ||
| }, | ||
| ) | ||
| raise InternalServerHTTPException() | ||
|
|
||
|
|
||
| @router.get( |
| async def get_routers( | ||
| offset: int = Query(default=0, ge=0, description="Number of routers to skip."), | ||
| limit: int = Query(default=10, ge=1, le=100, description="Maximum number of routers to return."), | ||
| sort_by: RouterSortField = Query(default=RouterSortField.ID, description="Field to sort by."), | ||
| sort_order: SortOrder = Query(default=SortOrder.ASC, description="Sort order."), | ||
| get_routers_use_case: GetRoutersUseCase = Depends(get_routers_use_case_factory), | ||
| request_context: ContextVar[RequestContext] = Depends(get_request_context), | ||
| ) -> Routers: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
In general, to fix mixed explicit and implicit returns, you add an explicit return (or raise) on all paths, including the “should never happen” path after a match or if/elif chain. For FastAPI endpoints that are expected to always either return a response model or raise an HTTPException, the best fix is to make the fallthrough path explicitly raise an internal error, mirroring how unexpected exceptions are handled elsewhere in the function.
For get_routers specifically (lines 130–166), we should add a final raise InternalServerHTTPException() after the match block. This ensures the function never implicitly returns None, and any unhandled result variant is treated as an internal server error, consistent with the try/except above. No new imports are needed because InternalServerHTTPException is already imported at the top of the file. To keep behavior consistent across endpoints, we can optionally mirror this pattern in get_router as well, but the CodeQL finding is explicitly on get_routers; we will update get_routers only.
Concretely:
- In
api/infrastructure/fastapi/endpoints/admin/routers.py, inget_routers, after the lastcase UserIsNotAdminError(): raise NotAdminUserHTTPException(), add a final lineraise InternalServerHTTPException(). This makes the function total and removes the implicitNonereturn.
| @@ -163,3 +163,4 @@ | ||
| ) | ||
| case UserIsNotAdminError(): | ||
| raise NotAdminUserHTTPException() | ||
| raise InternalServerHTTPException() |
3 participants
No description provided.