Fix/sonar reliability #149
Conversation
📝 WalkthroughWalkthroughRefactors many FastAPI routes to use typing.Annotated for Query metadata, adds a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as AdminEventsAPI
participant Service as AdminEventsService
participant BG as BackgroundTasks
participant Queue as ReplayQueue
Client->>API: POST /admin/events/replay (EventReplayRequest)
Note right of API: build replay_filter\ngenerate replay_correlation_id
API->>Service: create_replay_session(replay_filter, correlation_id)
Service-->>API: replay_session_id
API->>Service: start_replay(replay_session_id, dry_run)
Service-->>API: replay_result (success or error)
alt success and enqueue requested
API->>BG: add_task(enqueue_replay, replay_session_id, correlation_id)
BG->>Queue: enqueue(replay_session_id, correlation_id)
end
API-->>Client: EventReplayResponse (session_id, status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/db/docs/notification.py`:
- Line 31: Existing DB documents with action_url:null will raise Pydantic v2
validation errors because action_url is declared as a non-nullable str; to fix,
update the document model field declaration for action_url in
backend/app/db/docs/notification.py to accept None (e.g., change action_url: str
= "" to action_url: str | None = None or use Optional[str] = None) so Beanie can
load legacy records, or alternatively implement a field validator on the same
model to coerce None -> ""; refer to the action_url field in the notification
document class when making this change.
🧹 Nitpick comments (3)
backend/app/api/routes/admin/events.py (2)
55-58: Missing lower-bound constraint onhours.
hoursallows zero or negative values since onlyle=168is specified. Consider addingge=1for consistency with similar parameters elsewhere in this PR (e.g.,older_than_hoursinreplay.py).Suggested fix
- hours: Annotated[int, Query(le=168)] = 24, + hours: Annotated[int, Query(ge=1, le=168)] = 24,
64-67: Missing lower-bound constraint onlimitfor export endpoints.Both CSV and JSON export endpoints allow
limit≤ 0. Addingge=1would match the convention used in other route files.Suggested fix (apply to both CSV line 67 and JSON line 92)
- limit: Annotated[int, Query(le=50000)] = 10000, + limit: Annotated[int, Query(ge=1, le=50000)] = 10000,backend/app/services/notification_service.py (1)
303-312: Consider extracting hardcoded API paths to constants.
"/api/v1/notifications"and"/api/v1/executions/{id}/result"(lines 308, 454, 476, 506) are scattered as string literals. If routes change, these become stale silently. A shared module of URL builders or constants would centralize them.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/openapi.json (1)
11157-11199:⚠️ Potential issue | 🟡 MinorClarify whether
action_urlshould be required or optional—the schema and implementation are inconsistent.The OpenAPI schema marks
action_urlas required, but the internal models (Notification,DomainNotification,NotificationDocument) all defaultaction_urlto an empty string. The code includes defensive null checks (e.g., inNotificationCenter.svelteandnotification_service.py), suggesting the field is expected to be empty in some cases. Either enforce the requirement at creation time (require producers to set a non-empty value) or update the API schema to reflect thataction_urlis optional. No explicit backfilling mechanism was found for legacy data.
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/admin/events.py`:
- Around line 67-70: The export endpoints allow zero or negative limits because
the Query for the limit parameter lacks a lower bound; update the limit
Annotated parameter in both export_events_csv and export_events_json to include
a ge=1 constraint (i.e., use Query(le=50000, ge=1)) so clients cannot pass 0 or
negative values; change the limit parameter declaration (the variable named
limit using Query) in both functions accordingly.
- Around line 54-61: The hours query parameter in get_event_stats currently only
enforces an upper bound (le=168) and allows zero or negative values; update the
Query for the hours parameter in the get_event_stats route to include a minimum
(e.g., ge=1) and adjust the description accordingly so it reads something like
"Lookback window in hours (1-168)"; touch the Annotated[int, Query(...)]
declaration for hours to add ge=1 and update the description text so validation
prevents 0 or negative inputs.
- Around line 193-202: The delete handler in this router (async def
delete_event) currently raises HTTPException(status_code=500) when the
service.delete_event result is falsy; change this to raise a 404 to match the
equivalent logic in backend/app/api/routes/events.py (where a missing event
returns 404) — update the HTTPException to use status_code=404 and a descriptive
detail like "Event not found" (ensure the check of deleted from
AdminEventsService.delete_event remains the same but the exception and message
are corrected for consistency).
In `@backend/app/api/routes/admin/users.py`:
- Around line 42-45: The limit query parameter in the users admin route is
missing a lower bound; update the Annotated type for limit in the function
signature (the parameter named limit in backend/app/api/routes/admin/users.py)
to include ge=1 alongside le=1000 (i.e., use Query(ge=1, le=1000)) so it matches
the bounds used in other endpoints like dlq.py and execution.py and prevents
zero/negative values.
🧹 Nitpick comments (4)
backend/app/api/routes/replay.py (1)
32-39: Consider addingErrorResponsemappings for session-based endpoints.Other route files in this PR (e.g.,
saga.py,saved_scripts.py) addresponses={404: {"model": ErrorResponse}}for endpoints that look up resources by ID. The replay session endpoints (start,pause,resume,cancel,getbysession_id) don't have these mappings, which is inconsistent with the rest of the PR.Also applies to: 42-49, 52-56, 59-63, 79-82
backend/app/api/routes/auth.py (1)
261-293: Deadexceptblock inverify_token.The
auth_service.get_current_user(request)call at Line 250 is outside thetryblock, so any authentication failure raises before entering thetry. The code inside thetry(Lines 262–277) only performs logging and constructs a return value — it won't raise a meaningful exception. This makes theexceptblock effectively unreachable.Consider removing the
try/exceptwrapper here, or moving theget_current_usercall inside thetryif the intent is to catch auth failures at this level.♻️ Suggested simplification
- try: - logger.info( - "Token verification successful", - extra={ - "username": current_user.username, - "client_ip": get_client_ip(request), - "user_agent": request.headers.get("user-agent"), - }, - ) - csrf_token = request.cookies.get("csrf_token", "") - - return TokenValidationResponse( - valid=True, - username=current_user.username, - role="admin" if current_user.is_superuser else "user", - csrf_token=csrf_token, - ) - - except Exception as e: - logger.error( - "Token verification failed", - extra={ - "client_ip": get_client_ip(request), - "user_agent": request.headers.get("user-agent"), - "error_type": type(e).__name__, - "error_detail": str(e), - }, - ) - raise HTTPException( - status_code=401, - detail="Invalid token", - headers={"WWW-Authenticate": "Bearer"}, - ) from e + logger.info( + "Token verification successful", + extra={ + "username": current_user.username, + "client_ip": get_client_ip(request), + "user_agent": request.headers.get("user-agent"), + }, + ) + csrf_token = request.cookies.get("csrf_token", "") + + return TokenValidationResponse( + valid=True, + username=current_user.username, + role="admin" if current_user.is_superuser else "user", + csrf_token=csrf_token, + )backend/app/api/routes/execution.py (1)
316-320: Bareexcept Exceptionswallows all errors including programming bugs.The catch-all here converts any exception (including unexpected bugs like
AttributeError,TypeError) into a generic 500 response, making debugging harder. Consider catching only expected exceptions (e.g., from the K8s client) and letting unexpected ones propagate to the global error handler.That said, this is pre-existing behavior and the PR only adds the
ErrorResponseannotation — so this is not a regression.backend/app/api/routes/admin/events.py (1)
144-157: String-matching on exception messages is fragile.The checks
"No events found" in msgand"Too many events" in msgcouple this handler to the exact wording of the service-layer exceptions. If the message text changes, these conditions silently stop matching and theraiseon line 157 propagates an unhandledValueErroras a 500.Consider using distinct exception types (e.g.,
EventsNotFoundError,TooManyEventsError) or structured error codes in the service layer instead.
|
1 participant
Summary by cubic
Standardized query parameter annotations and explicit error responses across auth, events, executions, sagas, DLQ, notifications, and admin APIs. Enforced notification action_url as a required string; updated OpenAPI and regenerated frontend SDK/types.
Refactors
Migration
Written for commit cf5cc8c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores