Repository navigation
Conversation
… schemas and chart data Some Postgres-wire-compatible engines do not implement every GUC that SQLAlchemy's postgresql dialect queries on first connect (for example `show standard_conforming_strings`). The resulting `sqlalchemy.exc.ProgrammingError` is a sibling of `OperationalError` under `DBAPIError`, so it escaped the existing handlers and surfaced as an unhandled 500 with an ERROR log from two endpoints: - GET /api/v1/database/<pk>/schemas/ (and the sibling /catalogs/), which only caught `OperationalError`. - POST/GET /api/v1/chart/data, where the connection is first opened while computing the cache key (`get_default_schema` -> `get_inspector`), outside the code that wraps query execution failures as `ChartDataQueryFailedError`. The schemas and catalogs handlers now catch `DBAPIError` (these endpoints run no user SQL, so any DBAPI error is a connect/introspection failure), keep the existing 500 response, and log a warning with the traceback so the failure is not silent. `_get_data_response` now catches `DBAPIError`, logs at WARNING, and returns a 422 with a generic, sanitized message instead of leaking the raw driver error or SQL. Genuine query execution failures are already wrapped as `ChartDataQueryFailedError` and keep their 400 response. Tradeoff: any DBAPIError acquiring the chart payload (including transient connection drops) now returns 422 rather than 500. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| QueryObjectValidationError, | ||
| ) as exc: | ||
| return self.response_400(message=sanitize_error_message(exc.message)) | ||
| except DBAPIError: |
There was a problem hiding this comment.
Suggestion: async_mode requests take _run_async before reaching this handler, so a connection failure during its cache lookup still escapes as an unhandled 500.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Incomplete implementation
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 703:703
**Comment:**
*Incomplete Implementation: `async_mode` requests take `_run_async` before reaching this handler, so a connection failure during its cache lookup still escapes as an unhandled 500.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45867 +/- ##
==========================================
+ Coverage 84.70% 84.71% +0.01%
==========================================
Files 3069 3069
Lines 205649 205800 +151
Branches 48335 48370 +35
==========================================
+ Hits 174200 174349 +149
+ Misses 28156 28152 -4
- Partials 3293 3299 +6
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:
|
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here 👋
Reviewed at dc8a966. Thanks for working on this. Turning the dialect-init ProgrammingError into a handled, sanitized response is the right call, and the chart-data test that checks the driver detail and SQL stay out of the body is a good guard. A few things I think are worth addressing before merge:
Worth fixing before merge
-
CI is red: new string missing from
messages.pot(superset/charts/data/api.py:712)
babel-extractandtests/unit_tests/scripts/translations/check_pot_drift_test.pyboth fail because_("There was an error connecting to the database")is a new msgid. Could we run./scripts/translations/babel_update.shand commit the regeneratedmessages.pot? While there, the schemas/catalogs handlers return the same text as an untranslated literal. Totally optional to share one translated string across all three. -
except DBAPIErroralso catches metadata-database errors (superset/charts/data/api.py:703,superset/databases/api.py:785,:873)
DBAPIErroris raised by Superset's own metadata DB too, not only by the configured database, and the chart-datatrydoes several metadata queries. For example,query()builds the SQL inget_query_str_extended()before its owntry(superset/models/helpers.py:2698), and that step loads RLS filters from the metadata DB (get_sqla_row_level_filters→security_manager.get_rls_filters). Native annotation layers are another such query. So if the metadata DB drops a connection, or a deploy runs ahead of a migration (ProgrammingError: column ... does not exist), that error now comes back as a 422 saying "There was an error connecting to the database", logged only at WARNING. The user is told their warehouse is broken, and the Superset-side fault drops out of 5xx/ERROR alerting. The schemas/catalogs handlers have the same issue throughget_schemas_accessible_by_user/get_catalogs_accessible_by_user.
There's already a helper for exactly this split:superset.databases.error_provenance.is_database_engine_error(#42996). It marks errors raised by configured-database engines, and nothing calls it yet. I checked on the pinned SQLAlchemy 2.0.55: an error raised fromdialect.initialize()on first connect does fire the engine'shandle_errorlistener, so theSHOW standard_conforming_stringscase gets marked. Could we narrow the handlers to something like:except DBAPIError as ex: if not is_database_engine_error(ex): raise ...
That keeps the fix for the connect failure and leaves metadata-DB and raw-cursor errors on their current path. WDYT?
Smaller suggestions
- The same connect failure now gets a 422 from
/chart/databut a 500 from/schemas/and/catalogs/. Small suggestion: could we use the same status for both (500, or 503 for "upstream unavailable")? A 4xx tells clients and dashboards the request was at fault, and transient warehouse outages would stop showing up as server errors. Happy to keep 422 if it matches the existingChartDataCacheLoadErrorconvention on purpose. - The inline comment ("Query execution failures are wrapped as ChartDataQueryFailedError…") overstates things a bit, since SQL compilation and RLS lookup run outside that wrapping. Could we adjust it once the handler is narrowed?
Testing
- With the marker check in place, it would be great to add one negative test: an unmarked
DBAPIError(as from the metadata DB) is not turned into the 422 / "error connecting" response. The current tests raise straight from a mockedcommand.execute/get_all_schema_names, so they don't tell the two sources apart.
Happy to take another look once updated!
… status codes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
Handle
sqlalchemy.exc.ProgrammingError(and other non-OperationalErrorDBAPIErrors) raised while connecting to a user database, instead of letting them escape as unhandled 500s with ERROR logs.PROBLEM
Some Postgres-wire-compatible engines don't implement every GUC that SQLAlchemy's
postgresqldialect queries on first connect (e.g.SHOW standard_conforming_strings). The resultingProgrammingErroris a sibling ofOperationalErrorunderDBAPIError, so it escapes two places:GET /api/v1/database/<pk>/schemas/(and/catalogs/) only catchOperationalError.POST|GET /api/v1/chart/data: the connection is first opened while computing the cache key (query_cache_key->get_extra_cache_keys->get_default_schema->get_inspector), which runs outside the code that wraps query-execution failures asChartDataQueryFailedError, so a rawDBAPIErrorreaches_get_data_responseunhandled.FIX
schemas/catalogs: catchDBAPIError(these endpoints run no user SQL, so any DBAPI error is a connect/introspection failure). Response is unchanged (500, "There was an error connecting to the database"); a WARNING with traceback is now logged._get_data_response: catchDBAPIError, log at WARNING, return 422 with a generic message passed throughsanitize_error_message(raw driver error / SQL is not returned).Tradeoffs
DBAPIErrorraised while acquiring the chart payload (including transient connection drops) now returns 422 instead of 500.ChartDataRestApi._run_asynchas a similar gap forasync_moderequests; left for a follow-up since the fix there differs.TESTING INSTRUCTIONS
pytest tests/unit_tests/databases/api_test.py tests/unit_tests/charts/test_chart_data_api.py(new tests fail on master, pass here).ADDITIONAL INFORMATION
🤖 Generated with Claude Code