fix(dao): don't mask transient OperationalError as a "not found" result - #43335
fix(dao): don't mask transient OperationalError as a "not found" result#43335aminghadersohi wants to merge 5 commits into
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43335 +/- ##
==========================================
- Coverage 66.73% 66.67% -0.07%
==========================================
Files 2876 2876
Lines 164201 164174 -27
Branches 37887 37873 -14
==========================================
- Hits 109577 109459 -118
- Misses 52467 52558 +91
Partials 2157 2157
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:
|
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.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Prevents transient database connection issues (OperationalError) from being incorrectly treated as “not found” results (400/None) in shared DAO lookup helpers, and adds regression tests to ensure these errors propagate as 5xx-worthy failures.
Changes:
- Update
BaseDAO.find_by_id_or_uuid,BaseDAO._find_by_column, andBaseDAO.find_by_idsto re-raiseOperationalErrorbefore broader exception handlers. - Add unit tests asserting
OperationalErrorpropagates while existingStatementError/SQLAlchemyErrorbehavior remains unchanged. - Adjust chart/dashboard DAO tests to rely on data-seeded session fixtures and assert expected records exist.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
superset/daos/base.py |
Re-raises OperationalError so transient DB failures aren’t masked as “not found”. |
tests/unit_tests/dao/base_dao_test.py |
Adds regression tests for OperationalError propagation vs. StatementError absorption. |
tests/unit_tests/charts/dao/dao_tests.py |
Uses seeded session fixture and asserts chart exists for favorite tests. |
tests/unit_tests/dashboards/dao_tests.py |
Uses seeded session fixture and asserts dashboard exists for favorite tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Review Agent Run #b7ff87Actionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
|
Thanks @bito-code-review — went through the three suggestions: "OperationalError is not a StatementError subclass" ( >>> from sqlalchemy.exc import OperationalError, StatementError
>>> issubclass(OperationalError, StatementError)
TrueMRO: Move The logic in all cases was already flagged as correct — these are comment/style notes, so no code change. |
|
cc @mikebridge — you have the most recent substantive work on |
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.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
superset/daos/base.pyonly — the sharedBaseDAOlookup helpers used across the app. Behavior changes solely on the error path: transient DB connection failures now surface as 5xx instead of a misleading 400 / silentNone. The happy path and all non-connection error handling are unchanged.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.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
Very low. The change is additive (one import + three identical 5-line guards) and only redirects an already-failing request from a misleading 400/
Noneto an honest 5xx/propagated error. Rollback is a plain revert. No migration, no feature flag, no schema change.