fix(sqla): don't mislabel DB errors as ColumnNotFoundException in adhoc_column_to_sqla - #42889
Conversation
…oc_column_to_sqla (SC-116904) The type-probe in SqlaTable.adhoc_column_to_sqla wrapped the whole probe in a try/except SupersetGenericDBErrorException that relabeled every such error as ColumnNotFoundException. Because get_columns_description wraps any execution failure (SSL EOF, timeouts, dropped connections) as a SupersetGenericDBErrorException, real transient DB errors were mislabeled as "column not found". where_clause's adhoc-filter handling then silently dropped the filter instead of surfacing the DB error. Only the genuine empty-result case now raises ColumnNotFoundException; a real DB/connectivity failure from get_columns_description propagates unchanged. Fixes SUPERSET-PYTHON-WE6 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #1f0664Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42889 +/- ##
=======================================
Coverage 66.37% 66.37%
=======================================
Files 2857 2857
Lines 161048 161045 -3
Branches 37046 37046
=======================================
- Hits 106892 106891 -1
+ Misses 52141 52139 -2
Partials 2015 2015
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:
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — correct, well-scoped fix that removes a real mislabeling bug, with the security model intact (no authz/RLS impact; adhoc filters aren't an entitlement gate). No blockers. The root-cause analysis is precise and the refactor is behavior-preserving for every non-target exception — nicely surgical. Two items are worth addressing before merge (disclosure + a caller-level test); the rest are optional polish.
🟡 Medium — Behavior change is broader than TRADEOFFS admits
get_columns_description funnels every driver exception into SupersetGenericDBErrorException (superset/connectors/sqla/utils.py:212, blanket except Exception). So the newly-propagating path covers not just transient SSL/timeout blips (the stated target) but also the common, non-transient case: an adhoc filter whose expression references a dropped/renamed/typo'd column — that raises inside cursor.execute, becomes SupersetGenericDBErrorException, and never reaches the if not col_desc branch. On the filter path (superset/models/helpers.py:4024, the only catcher, always force_type_check=True) that flips from render-with-rejected-filter-notice to a hard 400 GENERIC_DB_ENGINE_ERROR — so a saved dashboard with a stale adhoc filter that renders today will hard-fail after deploy. This is defensible as the safer fail-closed direction (silently dropping a WHERE predicate serves under-filtered data as HTTP 200), so it's a judgment call, not a defect — but the TRADEOFFS framing ("transient DB/connectivity errors") understates the affected population. Fix: name the stale/renamed-column case in TRADEOFFS + add an UPDATING.md note; ideally distinguish "known-missing column" from "transient connectivity error" at the source so genuinely-missing columns keep degrading gracefully.
🟢 Low — No caller-level test for the behavior that actually changes
The new test_adhoc_column_type_probe_propagates_db_error asserts only the exception type at the adhoc_column_to_sqla boundary; nothing exercises the user-visible consequence through where_clause (propagate vs. silent-drop). A future widening of that except ColumnNotFoundException to also catch SupersetGenericDBErrorException would silently reintroduce this exact bug and the new test would still pass. There's a ready copy-target: test_numeric_adhoc_filter_value_is_unquoted_in_where_clause already drives get_sqla_query with a patched get_columns_description. Fix: add a get_sqla_query-level test asserting a probe SupersetGenericDBErrorException propagates (filter not appended to rejected_filter_columns), plus a small explicit test that patches get_columns_description to return [] and asserts ColumnNotFoundException. (The empty-result → 404 branch is already covered by test_adhoc_column_type_probe_raises_without_comment_safe_retry_hook, so that half is only a make-it-explicit nicety.)
🟢 Low — In-code comment is inaccurate about when ColumnNotFoundException fires
The new comment (superset/connectors/sqla/models.py:1878-1881) says "only a genuine empty result (the column truly isn't there) is a ColumnNotFoundException." That's misleading: the if not col_desc branch is reachable only when the probe succeeds but returns an empty cursor.description — a driver quirk (e.g. clickhouse-connect zero-row probes, which the utils.py retry logic exists for). A genuinely-missing column raises during execute and becomes SupersetGenericDBErrorException instead — the opposite of what the comment implies, and the exact case the whole PR turns on. Fix: reword to note it fires only for the empty-cursor.description driver-quirk case, not a genuinely-missing column.
🟢 Low — Transient probe blip is now fatal on the filter path, with no retry
get_columns_description opens its own raw connection purely for type inference (utils.py:187), and its only retry (get_column_description_retry_sql) covers empty cursor.description, not connection errors. Post-change, a momentary blip during this best-effort probe kills a chart whose main data query might have succeeded. This is a deliberate tradeoff (surfacing beats silent wrong data), not a bug — but since transient resilience is the stated motivation, consider a bounded retry / best-effort fallback for the probe, or at least call this out explicitly.
🟢 Low (nit) — Redundant inline import in the test
test_adhoc_column_type_probe_propagates_db_error re-imports from unittest.mock import patch inside the function although patch is already imported at module scope (helpers_test.py:27). Unlike the file's deferred superset-module imports, unittest.mock needs no app-context deferral. Fix: drop the inline import.
2 participants
SUMMARY
Fixes Sentry SUPERSET-PYTHON-WE6 (3677 events / 28 users, still firing) — Shortcut SC-116904.
Root cause:
SqlaTable.adhoc_column_to_sqla()(insuperset/connectors/sqla/models.py) wrapped the entire adhoc-column type-probe in atry/except SupersetGenericDBErrorExceptionthat relabeled every such error asColumnNotFoundException:But
get_columns_description()(insuperset/connectors/sqla/utils.py) wraps any exception thrown while executing the probe query — real DB/connectivity failures like SSL EOF, timeouts, and dropped connections — in aSupersetGenericDBErrorException. So theexceptabove caught two very different things and mislabeled both as "column not found":get_columns_description().Why this matters beyond Sentry noise:
ColumnNotFoundExceptionis caught insuperset/models/helpers.py(where_clause's adhoc-filter handling), where the column is treated as non-existent and the filter is silently dropped from the query. A transient SSL/connection blip during the type-probe therefore produced wrong query results (a missing filter) rather than a surfaced DB error.Fix: removed the blanket
try/except SupersetGenericDBErrorExceptionwrapper around the probe and changed the genuine zero-columns case to raiseColumnNotFoundException("Column not found")directly. Now only a true empty probe result maps toColumnNotFoundException; a real DB failure fromget_columns_description()propagates unchanged with its original message. The now-unusedSupersetGenericDBErrorExceptionimport was removed frommodels.py(verified it is not referenced anywhere else in the file). Theget_from_clause/compile_sqla_querycalls above are unchanged.TRADEOFFS
DB/connectivity errors during the adhoc-column type-probe now surface as
SupersetGenericDBErrorException(a 400GENERIC_DB_ENGINE_ERROR) instead of being silently converted into a dropped filter. This is an intentional failure-mode change: the user will see a database error where before they'd have silently gotten results with the adhoc filter missing. This matches how DB errors are handled everywhere else in this code path — surfacing a transient DB error is strictly better than silently returning wrong data — but it is a visible behavior change and is flagged here explicitly.adhoc_column_to_sqlahas other callers (models/helpers.py: groupby, orderby, select construction) that don't catchColumnNotFoundExceptiontoday. For those, a probe DB error changes from a 404ColumnNotFoundException("Column not found") to a 400SupersetGenericDBErrorExceptionwith the real DB message — still surfaced as an error either way, not a new silent failure, but the specific exception type/status code a caller might see does change.TESTING INSTRUCTIONS
Added a regression test and confirmed the genuine not-found path is unchanged.
test_adhoc_column_type_probe_propagates_db_errormocksget_columns_descriptionwithside_effect=SupersetGenericDBErrorException("SSL error: unexpected eof while reading")and assertsadhoc_column_to_sqla(..., force_type_check=True)raisesSupersetGenericDBErrorExceptionwith that message and notColumnNotFoundException.test_adhoc_column_type_probe_raises_without_comment_safe_retry_hookconfirms the genuine empty-result case still raisesColumnNotFoundException— no regression.Lint (repo-pinned ruff 0.9.7):
ADDITIONAL INFORMATION