fix(dao): don't mask transient OperationalError as a "not found" result - #43479
Conversation
A transient connection-level failure (e.g. "SSL connection has been closed unexpectedly") surfaces as psycopg2/SQLAlchemy OperationalError. Three DAO lookups swallowed it: - find_by_ids wrapped it in DAOFindFailedError (HTTP 400), reporting a server connection drop to the user as "<Model> <id> doesn't exist". - find_by_id_or_uuid and _find_by_column catch StatementError to absorb type-coercion errors and return None. Because OperationalError is a StatementError subclass, they silently returned None as well, which callers read as "record not found" with nothing surfaced in logs or Sentry. Add a narrow, earlier 'except OperationalError: raise' at each site so connection-level failures propagate as themselves (surfacing as a 5xx) while the intended behavior for other SQLAlchemyError/StatementError subtypes is preserved. Add regression tests for both paths at all three sites.
These chart/dashboard DAO unit tests ran find_by_id against an in-memory SQLite schema whose tables were never created, and depended on the DAO swallowing the resulting OperationalError to return None. With the masking removed they now fail with 'no such table'. Point them at the existing session_with_data fixture so the tables exist and a not-found lookup is a genuine empty result. The favorite tests were passing vacuously (the 'if not <obj>: return' guard always fired because the object was never found); with the object now present they exercise the favorite path for real, so replace the guard with an assertion.
Code Review Agent Run #fecaa6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. In Since I do not have access to the API exception handler code in this diff, I recommend checking where superset/daos/base.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43479 +/- ##
==========================================
+ Coverage 79.10% 79.11% +0.01%
==========================================
Files 2878 2878
Lines 165634 165650 +16
Branches 38294 38296 +2
==========================================
+ Hits 131023 131061 +38
+ Misses 32123 32112 -11
+ Partials 2488 2477 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`handle_api_exception` mapped `exc.DatabaseError` to HTTP 422. Because `OperationalError` is a `DatabaseError` subclass, a connection-level failure propagated by the `BaseDAO` lookup helpers still reached callers as a client error on every route wrapped by that decorator — which includes the `get/get_list/post/put/delete` handlers on every model REST API. Match `OperationalError` in its own clause ahead of the 422 handler and return 500. Other `DatabaseError` subtypes (bad SQL, missing relation) still return 422 unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
… into fix-dao-operationalerror-sc117473
Code Review Agent Run #a6c53cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Requested review from @eschutho — she owns the recent history on cc @mikebridge — you own the recent Two things worth a reviewer's attention:
Otherwise the diff matches the PR body: three additive |
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Posting on Elizabeth's behalf — this is her PR reviewer agent. Forward any pushback to her and she'll loop me back in.
This looks good overall — a well-scoped, well-tested fix, and I'm approving. Just two optional notes below on judgment calls you already flagged yourself in the description; neither blocks merge. All line numbers verified against HEAD 28dd709f.
superset/views/error_handling.py:154-162
On the 500-vs-503 question you raised: I checked for existing precedent and DatabaseRestApi.schemas (superset/databases/api.py:837-840) already catches OperationalError and returns 500 with the same reasoning, so this change actually makes the codebase more internally consistent rather than introducing a new convention. 503 would be marginally more precise semantically (it'd opt into SUPERSET_CLIENT_RETRY_STATUS_CODES), but there's no existing "503 for a transient DB blip" pattern here to align with.
WDYT — keep 500 as-is, or worth a follow-up issue to consider 503 codebase-wide later? Happy either way.
superset/daos/base.py:446-450 (and the two sibling guards at :258-262, :349-353)
On the analytic-database blast radius you called out: I traced the affected routes (DatabaseRestApi.tables, Api.query, the datasource views) and most analytic-DB failures are already wrapped into SupersetErrorException by the engine-spec/command layer before they'd reach this clause raw, so the practical surface is smaller than it might look. An unreachable analytic DB going from 422 to 500 also seems like the more honest status either way (it's not a client-fixable "unprocessable" request).
Totally optional, but if you want to narrow this further down the line, distinguishing metadata-DB vs analytic-DB OperationalError would need connection provenance threaded through — not something this PR needs to solve now.
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Posting on Elizabeth's behalf — this is her PR reviewer agent. Forward any pushback to her and she'll loop me back in.
This looks good overall — a well-scoped, well-tested fix, and I'm approving. Just two optional notes below on judgment calls you already flagged yourself in the description; neither blocks merge. All line numbers verified against HEAD 28dd709f.
superset/views/error_handling.py:154-162
On the 500-vs-503 question you raised: I checked for existing precedent and DatabaseRestApi.schemas (superset/databases/api.py:837-840) already catches OperationalError and returns 500 with the same reasoning, so this change actually makes the codebase more internally consistent rather than introducing a new convention. 503 would be marginally more precise semantically (it'd opt into SUPERSET_CLIENT_RETRY_STATUS_CODES), but there's no existing "503 for a transient DB blip" pattern here to align with.
WDYT — keep 500 as-is, or worth a follow-up issue to consider 503 codebase-wide later? Happy either way.
superset/daos/base.py:446-450 (and the two sibling guards at :258-262, :349-353)
On the analytic-database blast radius you called out: I traced the affected routes (DatabaseRestApi.tables, Api.query, the datasource views) and most analytic-DB failures are already wrapped into SupersetErrorException by the engine-spec/command layer before they'd reach this clause raw, so the practical surface is smaller than it might look. An unreachable analytic DB going from 422 to 500 also seems like the more honest status either way (it's not a client-fixable "unprocessable" request).
Totally optional, but if you want to narrow this further down the line, distinguishing metadata-DB vs analytic-DB OperationalError would need connection provenance threaded through — not something this PR needs to solve now.
The new 500 clause passes the raw driver message to `json_error_response`, which routes a bare string through `sanitize_error_message`. Driver messages quote the host, port and user of the failed connection, so pin the redaction down for embedded guest viewers — and pin down that regular users still get the detail. Co-Authored-By: Claude <noreply@anthropic.com>
… into fix-dao-operationalerror-sc117473
Code Review Agent Run #3724a0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
Why
A transient connection-level failure — in production,
psycopg2.OperationalError: SSL connection has been closed unexpectedly— was being masked by three DAO lookups insuperset/daos/base.py, so a server-side database blip was reported to users as a client error about a record that does not exist:find_by_idscaughtSQLAlchemyErrorand re-raised it asDAOFindFailedError, which carriesstatus = 400. A real "record doesn't exist" never reaches this handler (.all()returns[]), so in practice it only fired on genuine execution failures — turning a connection drop into "AnnotationLayer 5 doesn't exist" with HTTP 400, sending users and support chasing a misconfiguration that does not exist.find_by_id_or_uuidand_find_by_columncatchStatementErrorto absorb type-coercion errors (e.g. a non-UUID string) and returnNone. BecauseOperationalErroris a subclass ofStatementError(MRO:OperationalError → DatabaseError → DBAPIError → StatementError → SQLAlchemyError), these two also swallowed a transient connection failure and returnedNone— which every caller reads as "the record does not exist." This is arguably worse than thefind_by_idscase: it is a confidently wrong answer with no error at all, invisible in logs and error tracking.All three share one root cause and one remedy, so they are fixed together.
What
Add a narrow, earlier
except OperationalError: raiseahead of the existing broad handler at each of the three sites. Connection-level failures now propagate as themselves (surfacing as a 5xx) while the intended behavior is preserved exactly for every other subtype — otherSQLAlchemyErrors still becomeDAOFindFailedError, and genuine coercionStatementErrors still returnNone.Order matters: the
OperationalErrorclause must precede the broaderStatementError/SQLAlchemyErrorclause.The same masking existed one layer up, in the API error handler.
handle_api_exceptionmappedexc.DatabaseErrorto HTTP 422, andOperationalErroris aDatabaseErrorsubclass — so on every route wrapped by that decorator (which includesget_headless/get_list_headless/post_headless/put_headless/delete_headlessonBaseSupersetModelRestApi, i.e. essentially all model CRUD) the propagated error still reached the caller as a client error. The DAO fix alone would have traded a misleading 400 for a misleading 422.superset/views/error_handling.pynow matchesOperationalErrorin its own clause ahead of the 422 handler and returns 500; otherDatabaseErrorsubtypes (bad SQL, missing relation, constraint violations) still return 422 unchanged. Credit to @codeant-ai-for-open-source for catching this.Why
OperationalErroras the boundary? It covers both mid-query connection drops and initial-connect failures, and is simpler and safer than narrowing onDBAPIError.connection_invalidated, which misses initial-connect failures (a connection that was never established cannot be invalidated). Some drivers do raise a few non-connection problems asOperationalError; the cost of letting one of those through is a 500 instead of a 400 on an already-failing request — the safer direction, since it fails loudly rather than fabricating a "not found".Blast radius
Two files, both on the error path only; the happy path and all non-connection error handling are unchanged.
superset/daos/base.py— the sharedBaseDAOlookup helpers. Transient connection failures surface as themselves instead of a misleading 400 / silentNone.superset/views/error_handling.py—handle_api_exception. Connection failures return 500 instead of 422.One consequence worth calling out for review: a few routes under
handle_api_exceptionquery user-configured analytic databases rather than the metadata DB (DatabaseRestApi.tables,Api.query, theDatasourceviews,fetch_datasource_metadata). An unreachable analytic database on those routes now returns 500 where it previously returned 422. That seems like the right direction — the request was processable, the upstream was down — and it is the same defect class this PR addresses, but it is a wider behavior change than the DAO fix and maintainers may want to weigh in. Likewise, 500 was chosen to match the decorator's own generic server-error fallback; 503 would signal "transient, retry" more precisely if preferred.How to test
Regression tests added in
tests/unit_tests/dao/base_dao_test.py, covering both legs at each affected site:find_by_ids: anOperationalErrorfromquery.all()propagates asOperationalError; a non-connectionSQLAlchemyErrorstill raisesDAOFindFailedError(existing tests).find_by_id_or_uuid/_find_by_column: anOperationalErrorpropagates, while a genuine coercionStatementErrorstill returnsNone.And in
tests/unit_tests/views/test_error_handling.py, for the handler:OperationalErrorthroughhandle_api_exceptionreturns 500.ProgrammingErrorandIntegrityErrorstill return 422.Each negative test asserts the exception type, not merely that something was raised. Reverting the production change makes every propagation test fail (the
OperationalErroris masked asDAOFindFailedErrororNoneinstead).Risk & rollback
Low. The change is additive (one import + three identical guards in the DAO, one
exceptclause in the handler) and only redirects an already-failing request from a misleading 400/422/Noneto an honest 500. Rollback is a plain revert. No migration, no feature flag, no schema change. The one judgment call is the analytic-database consequence noted under Blast radius.Guest-token exposure: none. The new clause hands a bare string to
json_error_response, whose string branch routes it throughsanitize_error_message, so an embedded viewer receives the generic message rather than the driver text (host/IP/port). This is inherited fromjson_error_responseand is the same protection the adjacent 422 clause already relies on; regression tests now pin both legs.