feat(database): identify configured database connection errors - #42996
Conversation
dosubot
Bot
added
the
data:databases
Code Review Agent Run #702eb3Actionable 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. |
github-actions
Bot
added
the
github_actions
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42996 +/- ##
==========================================
- Coverage 66.41% 66.41% -0.01%
==========================================
Files 2858 2859 +1
Lines 161446 161460 +14
Branches 37190 37191 +1
==========================================
+ Hits 107222 107230 +8
- Misses 52187 52191 +4
- Partials 2037 2039 +2
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:
|
Antonio-RiveroMartnez
changed the title
4b3ac17 to
9d45ecf
Compare
github-actions
Bot
removed
the
github_actions
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: clean, well-tested mechanism that does exactly what it claims for connection errors — but the handle_error listener never fires for query/statement errors (Superset runs queries on raw DBAPI cursors), so the feature misses its dominant target case, and the tests only exercise the Core path Superset doesn't use.
Verified solid (checked empirically against the repo's pinned SQLAlchemy 1.4.54, not just read): no circular import; from sqlalchemy.engine import ExceptionContext resolves; the callback is closure-free so the engine cache can't retain Database instances; each cached engine gets exactly one listener and cache hits never re-add; no listener/memory leak on the private prequery path (500-iter GC test clean); suppress is load-bearing and correctly never masks the real error; the marker doesn't alter the exception's type/message/orig; connection-establishment errors are marked.
🟡 Medium — handle_error never fires for query errors; feature misses the dominant case
handle_error is a SQLAlchemy Core event — it only fires for Connection.execute(). Every real Superset query path runs on a raw DBAPI cursor: SQL Lab (get_raw_connection → cursor.execute, sql_lab.py) and charts/datasets (get_df → db_engine_spec.execute → cursor.execute, core.py:932, db_engine_specs/base.py:2321). Reproduced on 1.4.54: raw cursor.execute() does not trigger handle_error, so a Gamma user's invalid SELECT against a configured Postgres yields an OperationalError with is_database_engine_error(exc) == False — the exact case the feature exists to identify. Only connection-establishment failures get marked.
Compounding this, the PR's own test_statement_error_is_marked_without_changing_exception asserts on engine.connect() + connection.execute(text(...)), a path Superset never uses for queries, giving false confidence that statement errors are covered.
Fix: mark at the raise site where Superset already catches the raw driver error and knows its provenance (e.g. inside BaseEngineSpec.execute / get_dbapi_mapped_exception, or get_raw_connection) so statement errors are covered too — or explicitly scope the feature and docstrings to "connection errors" and add a test through get_raw_connection/cursor.execute that proves what's actually covered.
🟢 Low — sqlalchemy_exception=None errors silently unmarked, undocumented & untested
error_provenance.py mark_database_engine_error: handle_error also fires when context.sqlalchemy_exception is None (SQLAlchemy's should_wrap=False path — non-DBAPI-Error exceptions surfacing during execution, e.g. UnicodeDecodeError/socket errors, and greenlet/BaseException cancellations). Then setattr(None, ...) raises and is swallowed by suppress, so the propagated exception (context.original_exception) goes unmarked and is_database_engine_error returns False for a genuine configured-engine error. The docstrings only mention the "later listener replaces the exception" case, and test_marker_assignment_failure_does_not_mask_error uses a synthetic UnmarkableError, not None.
Fix: fall back to marking context.original_exception when sqlalchemy_exception is None, or at minimum document the gap and add a None-path test.
🟢 Low — engine event.listen tests assert on total spy count, not the handle_error call
core_test.py — test_get_sqla_engine, test_get_sqla_engine_caches_engine_per_url, test_get_sqla_engine_user_impersonation{,_email}, and test_get_sqla_engine_does_not_cache_unsaved_instances — use listen.assert_called_once_with(...) / call_count == 2 on the global event.listen spy. This passes only because the mocked engine is built as return_value before the spy installs (so sqlite's two internal connect listens aren't counted) and because they call private _get_sqla_engine (no prequery listener). Ironically the PR's own new test_cached_engine_has_one_instance_listener_without_database_closure does it right — it filters call_args_list to args[1] == "handle_error". Any second event.listen added to the path later breaks all five with a misleading "Called 2 times".
Fix: use listen.assert_any_call(engine, "handle_error", mark_database_engine_error) or filter to handle_error calls, matching the sibling test.
🟢 Low — redundant __closure__ is None assertion
test_cached_engine_has_one_instance_listener_without_database_closure asserts callback is mark_database_engine_error and then callback.__closure__ is None. Once identity holds, __closure__ is a fixed property of that module-level symbol, so the second line can't catch anything the first doesn't (a closure-wrapping refactor would fail the is check first). Harmless, but drop it or, if the intent is to guard against a future per-instance closure leaking Database refs, assert it against the callback pulled from the engine's actual registered listeners.
🟢 Low — is_database_engine_error has no in-tree consumer
The read helper is referenced only by its own unit test. Landing a neutral extension hook ahead of a consumer is a legitimate pattern, but with no end-to-end exercise the Medium coverage gap above produces no failing test — the feature merges looking complete while delivering nothing at real query call sites.
Fix: land a consumer (or a fast follow), or state in the PR/module which out-of-tree extension consumes it and which SQLAlchemy execution mode it observes.
Review assisted by a multi-agent pass (5 review dimensions, each finding adversarially verified against SQLAlchemy 1.4.54; 9 candidate findings were reproduced-against and dropped).
9d45ecf to
e109eec
Compare
Antonio-RiveroMartnez
changed the title
|
Thanks @rebenitez1802 — addressed the actionable points in
You are correct. This PR does not add raw DB-API query attribution. I chose the alternative suggested in the review: scope the API to configured-database connection failures and document/test the boundary. A real I did not add a broad catch to
The listener now falls back to
The tests now select/assert the
Removed. One suggestion was intentionally not implemented:
This PR provides a small extension API without changing Superset's error handling. Adding an artificial in-tree consumer only to demonstrate usage would expand behavior and scope. The module and PR instead state exactly what extensions can consume and which failures it identifies. A behavior-changing consumer can be reviewed separately if Superset needs one. Validation: provenance tests 10 passed, |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Identify connection errors from database connections configured in Superset.
Superset uses separate SQLAlchemy engines for:
A connection error alone does not reliably show which kind of engine produced it. Extensions otherwise have to guess from the exception class or message, which can be identical across databases.
This PR adds a SQLAlchemy
handle_errorlistener to every engine created for a configured database. The listener marks the existing exception. It does not catch, replace, reclassify, or otherwise change the error.Python extensions can check it through:
This works for connection failures from all SQLAlchemy-backed database engines without parsing driver-specific messages. It also observes errors raised through SQLAlchemy Core.
Scope and limitation
This API is intended to identify connection failures from configured database engines. SQLAlchemy's
handle_errorevent also observes SQLAlchemy Core operations, but Superset normally executes queries directly through raw DB-API cursors. Those raw cursor errors bypass the event and are not marked by this PR;is_database_engine_error()returnsFalsefor them. A test records this boundary explicitly.This PR intentionally does not catch every exception leaving
get_raw_connection(). That context also contains logging, extension, and other application work, so a broad catch could incorrectly label an unrelated database error as coming from the configured engine. Wrapping or replacing every driver cursor would be invasive and could break driver-specific behavior. Refactoring the engine-spec execution API could cover raw query errors safely, but it is a larger cross-engine behavior change and should be proposed and reviewed separately if needed.The conservative behavior is therefore: connection errors are identified; raw query errors and anything uncertain remain unidentified.
The listener is installed once, immediately after engine creation and before the engine is cached. Existing exception types, messages, tracebacks, OAuth handling, HTTP responses, and query behavior remain unchanged. If SQLAlchemy does not create a wrapper exception, the listener marks the original exception instead.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this change has no UI impact.
TESTING INSTRUCTIONS
Run:
pytest -q tests/unit_tests/databases/error_provenance_test.py pytest -q tests/unit_tests/models/core_test.py pytest -q tests/integration_tests/model_tests.py \ -k 'impersonate_user_presto or adjust_engine_params_mysql or impersonate_user_trino or impersonate_user_hive or test_get_sqla_engine'Validated locally:
tests/unit_tests/models/core_test.py: 76 passed.css_templatestable. The errors occurred during test setup, before the selected test bodies ran.The tests cover:
ADDITIONAL INFORMATION
The implementation uses SQLAlchemy 1.4's
handle_errorevent andExceptionContext. These assumptions should be reviewed as part of a future SQLAlchemy major-version upgrade.If another
handle_errorlistener later replaces the marked exception, the replacement is intentionally not marked because it is a different exception.