aminghadersohi opened a new pull request, #43335: URL: https://github.com/apache/superset/pull/43335
## Why A transient connection-level failure — in production, `psycopg2.OperationalError: SSL connection has been closed unexpectedly` — was being masked by three DAO lookups in `superset/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_ids`** caught `SQLAlchemyError` and re-raised it as `DAOFindFailedError`, which carries `status = 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_uuid`** and **`_find_by_column`** catch `StatementError` to absorb *type-coercion* errors (e.g. a non-UUID string) and return `None`. Because `OperationalError` is a subclass of `StatementError` (MRO: `OperationalError → DatabaseError → DBAPIError → StatementError → SQLAlchemyError`), these two also swallowed a transient connection failure and returned `None` — which every caller reads as *"the record does not exist."* This is arguably worse than the `find_by_ids` case: 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: raise` ahead 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 — other `SQLAlchemyError`s still become `DAOFindFailedError`, and genuine coercion `StatementError`s still return `None`. Order matters: the `OperationalError` clause must precede the broader `StatementError` / `SQLAlchemyError` clause. **Why `OperationalError` as the boundary?** It covers both mid-query connection drops and initial-connect failures, and is simpler and safer than narrowing on `DBAPIError.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 as `OperationalError`; 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.py` only — the shared `BaseDAO` lookup 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 / silent `None`. 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`: an `OperationalError` from `query.all()` **propagates as `OperationalError`**; a non-connection `SQLAlchemyError` still raises `DAOFindFailedError` (existing tests). - `find_by_id_or_uuid` / `_find_by_column`: an `OperationalError` **propagates**, while a genuine coercion `StatementError` **still returns `None`**. Each negative test asserts the exception **type**, not merely that something was raised. Reverting the production change makes every propagation test fail (the `OperationalError` is masked as `DAOFindFailedError` or `None` instead). ``` pytest tests/unit_tests/dao/base_dao_test.py ``` ## 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/`None` to an honest 5xx/propagated error. Rollback is a plain revert. No migration, no feature flag, no schema change. -- 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]
