aminghadersohi commented on code in PR #43479:
URL: https://github.com/apache/superset/pull/43479#discussion_r3854835647
##########
superset/daos/base.py:
##########
@@ -433,6 +443,11 @@ def find_by_ids(
try:
results = query.all()
+ except OperationalError:
+ # A transient connection-level failure (e.g. the server dropping
the
+ # connection mid-query) surfaces as OperationalError. Let it
propagate
+ # as a 5xx instead of masking it as a 400 "record doesn't exist".
+ raise
Review Comment:
Confirmed, and fixed in 086f4418.
`handle_api_exception` maps `exc.DatabaseError` to 422, and
`OperationalError` is a `DatabaseError` subclass, so it matched that clause.
That decorator wraps `get_headless` / `get_list_headless` / `post_headless` /
`put_headless` / `delete_headless` on `BaseSupersetModelRestApi`, so this
covered essentially every model CRUD route — the DAO change traded a misleading
400 for a misleading 422 on exactly the paths that matter most. The "propagate
as a 5xx" claim in the original description was wrong for those routes.
The fix adds an `except exc.OperationalError` clause returning 500, placed
ahead of the `(IntegrityError, DatabaseError, DataError)` handler. Other
`DatabaseError` subtypes — bad SQL, missing relation, constraint violations —
still return 422 unchanged. Three regression tests in
`tests/unit_tests/views/test_error_handling.py` cover both legs
(`OperationalError` → 500; `ProgrammingError` / `IntegrityError` → 422).
Two things worth a maintainer's opinion:
1. **Analytic-database blast radius.** A handful of routes under this
decorator query user-configured analytic databases rather than the metadata DB
(`DatabaseRestApi.tables`, `Api.query`, `Datasource` views,
`fetch_datasource_metadata`). An unreachable analytic DB on those routes now
returns 500 instead of 422. I think that is the correct direction — the request
was perfectly processable, the upstream was down — and it is the same defect
class this PR is about. But it is a wider behavior change than the DAO fix, so
I'd rather flag it than bury it.
2. **500 vs. 503.** I used 500 to match the decorator's own generic
server-error fallback. 503 would communicate "transient, retry" more precisely
and speaks to your point about clients not retrying. Happy to switch if
maintainers prefer it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
