You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/08/27 21:35:01 UTC
(superset) branch master updated: fix(dao): don't mask transient OperationalError as a "not found" result (#43479)
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new bcc6af6c5fb fix(dao): don't mask transient OperationalError as a "not found" result (#43479)
bcc6af6c5fb is described below
commit bcc6af6c5fb213aa36266f59d339d275f7ccc0a0
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Aug 27 17:34:53 2026 -0400
fix(dao): don't mask transient OperationalError as a "not found" result (#43479)
Co-authored-by: Claude <no...@anthropic.com>
---
superset/daos/base.py | 17 +++-
superset/views/error_handling.py | 11 ++-
tests/unit_tests/charts/dao/dao_tests.py | 13 ++-
tests/unit_tests/dao/base_dao_test.py | 125 +++++++++++++++++++++++++-
tests/unit_tests/dashboards/dao_tests.py | 11 ++-
tests/unit_tests/views/test_error_handling.py | 76 ++++++++++++++++
6 files changed, 237 insertions(+), 16 deletions(-)
diff --git a/superset/daos/base.py b/superset/daos/base.py
index 0520f299fe2..5e7ca889b72 100644
--- a/superset/daos/base.py
+++ b/superset/daos/base.py
@@ -38,7 +38,7 @@ from flask_appbuilder.models.filters import BaseFilter
from flask_appbuilder.models.sqla.interface import SQLAInterface
from pydantic import BaseModel, Field
from sqlalchemy import asc, cast, desc, false, or_, Text
-from sqlalchemy.exc import SQLAlchemyError, StatementError
+from sqlalchemy.exc import OperationalError, SQLAlchemyError, StatementError
from sqlalchemy.ext.hybrid import hybrid_property
from sqlalchemy.inspection import inspect
from sqlalchemy.orm import ColumnProperty, joinedload, Query, RelationshipProperty
@@ -255,6 +255,11 @@ class BaseDAO(CoreBaseDAO[T], Generic[T]):
filter = uuid_column == model_id_or_uuid
try:
return query.filter(filter).one_or_none()
+ except OperationalError:
+ # A transient connection-level failure (e.g. the server dropping the
+ # connection mid-query) surfaces as OperationalError, a StatementError
+ # subclass. Let it propagate instead of masking it as a "not found".
+ raise
except StatementError:
# can happen if neither uuid nor int is passed
return None
@@ -341,6 +346,11 @@ class BaseDAO(CoreBaseDAO[T], Generic[T]):
try:
return query.filter(column == converted_value).one_or_none()
+ except OperationalError:
+ # A transient connection-level failure (e.g. the server dropping the
+ # connection mid-query) surfaces as OperationalError, a StatementError
+ # subclass. Let it propagate instead of masking it as a "not found".
+ raise
except StatementError:
# can happen if int is passed instead of a string or similar
return None
@@ -433,6 +443,11 @@ class BaseDAO(CoreBaseDAO[T], Generic[T]):
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
except SQLAlchemyError as ex:
model_name = cls.model_cls.__name__ if cls.model_cls else "Unknown"
raise DAOFindFailedError(
diff --git a/superset/views/error_handling.py b/superset/views/error_handling.py
index 1e48130f7d0..2985275307a 100644
--- a/superset/views/error_handling.py
+++ b/superset/views/error_handling.py
@@ -119,7 +119,7 @@ def handle_ssh_tunnel_error(ex: sshtunnel.BaseSSHTunnelForwarderError) -> FlaskR
)
-def handle_api_exception(
+def handle_api_exception( # noqa: C901
f: Callable[..., FlaskResponse],
) -> Callable[..., FlaskResponse]:
"""
@@ -151,6 +151,15 @@ def handle_api_exception(
return json_error_response(
utils.error_msg_from_exception(ex), status=cast(int, ex.code)
)
+ except exc.OperationalError as ex:
+ # A connection-level failure (the server dropping the connection
+ # mid-query, or a connection that could never be established) is a
+ # server-side fault, not something the caller can correct by
+ # changing the request. It must not fall through to the 422 handler
+ # below, which OperationalError would otherwise match as a
+ # DatabaseError subclass.
+ logger.exception(ex)
+ return json_error_response(utils.error_msg_from_exception(ex), status=500)
except (exc.IntegrityError, exc.DatabaseError, exc.DataError) as ex:
logger.exception(ex)
return json_error_response(utils.error_msg_from_exception(ex), status=422)
diff --git a/tests/unit_tests/charts/dao/dao_tests.py b/tests/unit_tests/charts/dao/dao_tests.py
index e811223a988..55e2c88658c 100644
--- a/tests/unit_tests/charts/dao/dao_tests.py
+++ b/tests/unit_tests/charts/dao/dao_tests.py
@@ -25,6 +25,7 @@ from superset.utils.core import DatasourceType
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
+ from superset.models.core import FavStar # noqa: F401
from superset.models.slice import Slice
engine = session.get_bind()
@@ -57,7 +58,7 @@ def test_slice_find_by_id_skip_base_filter(session_with_data: Session) -> None:
def test_datasource_find_by_id_skip_base_filter_not_found(
- session: Session,
+ session_with_data: Session,
) -> None:
from superset.daos.chart import ChartDAO
@@ -65,12 +66,11 @@ def test_datasource_find_by_id_skip_base_filter_not_found(
assert result is None
-def test_add_favorite(session: Session) -> None:
+def test_add_favorite(session_with_data: Session) -> None:
from superset.daos.chart import ChartDAO
chart = ChartDAO.find_by_id(1, skip_base_filter=True)
- if not chart:
- return
+ assert chart is not None
assert len(ChartDAO.favorited_ids([chart])) == 0
ChartDAO.add_favorite(chart)
@@ -80,12 +80,11 @@ def test_add_favorite(session: Session) -> None:
assert len(ChartDAO.favorited_ids([chart])) == 1
-def test_remove_favorite(session: Session) -> None:
+def test_remove_favorite(session_with_data: Session) -> None:
from superset.daos.chart import ChartDAO
chart = ChartDAO.find_by_id(1, skip_base_filter=True)
- if not chart:
- return
+ assert chart is not None
assert len(ChartDAO.favorited_ids([chart])) == 0
ChartDAO.add_favorite(chart)
diff --git a/tests/unit_tests/dao/base_dao_test.py b/tests/unit_tests/dao/base_dao_test.py
index c21c333110a..578bf59bb64 100644
--- a/tests/unit_tests/dao/base_dao_test.py
+++ b/tests/unit_tests/dao/base_dao_test.py
@@ -23,7 +23,7 @@ from unittest.mock import Mock, patch
import pytest
from sqlalchemy import Boolean, Column, Integer, String
-from sqlalchemy.exc import SQLAlchemyError
+from sqlalchemy.exc import OperationalError, SQLAlchemyError, StatementError
from sqlalchemy.orm import declarative_base
from superset_core.common.models import CoreModel
@@ -260,6 +260,129 @@ def test_find_by_ids_none_id_column():
assert results == []
+def _make_operational_error() -> OperationalError:
+ """Build an OperationalError resembling a transient connection drop."""
+ return OperationalError(
+ "SELECT 1",
+ {},
+ Exception("SSL connection has been closed unexpectedly"),
+ )
+
+
+def test_find_by_ids_operational_error_propagates():
+ """A transient OperationalError from query.all() must propagate as itself,
+ not be masked as a 400 DAOFindFailedError ("record doesn't exist")."""
+
+ with (
+ patch("superset.daos.base.db") as mock_db,
+ patch("superset.daos.base.getattr") as mock_getattr,
+ ):
+ mock_session = Mock()
+ mock_db.session = mock_session
+
+ mock_id_col = Mock()
+ mock_id_col.in_.return_value = Mock()
+ mock_getattr.return_value = mock_id_col
+
+ mock_query = Mock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.all.side_effect = _make_operational_error()
+
+ with pytest.raises(OperationalError):
+ TestDAO.find_by_ids([1, 2])
+
+
+def test_find_by_id_or_uuid_operational_error_propagates():
+ """find_by_id_or_uuid catches StatementError to absorb coercion errors;
+ an OperationalError (a StatementError subclass) must still propagate."""
+
+ with (
+ patch("superset.daos.base.db") as mock_db,
+ patch("superset.daos.base.getattr") as mock_getattr,
+ ):
+ mock_session = Mock()
+ mock_db.session = mock_session
+ mock_getattr.return_value = Mock()
+
+ mock_query = Mock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.one_or_none.side_effect = _make_operational_error()
+
+ with pytest.raises(OperationalError):
+ TestDAO.find_by_id_or_uuid("1")
+
+
+def test_find_by_id_or_uuid_statement_error_still_returns_none():
+ """A genuine coercion StatementError is still absorbed as None (unchanged)."""
+
+ with (
+ patch("superset.daos.base.db") as mock_db,
+ patch("superset.daos.base.getattr") as mock_getattr,
+ ):
+ mock_session = Mock()
+ mock_db.session = mock_session
+ mock_getattr.return_value = Mock()
+
+ mock_query = Mock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.one_or_none.side_effect = StatementError(
+ "invalid input", "SELECT 1", {}, Exception("coercion")
+ )
+
+ assert TestDAO.find_by_id_or_uuid("not-a-uuid") is None
+
+
+def test_find_by_column_operational_error_propagates():
+ """_find_by_column catches StatementError to absorb coercion errors;
+ an OperationalError (a StatementError subclass) must still propagate."""
+
+ with (
+ patch("superset.daos.base.db") as mock_db,
+ patch("superset.daos.base.getattr") as mock_getattr,
+ patch("superset.daos.base.hasattr", return_value=True),
+ patch.object(TestDAO, "_apply_base_filter", side_effect=lambda q, *a, **k: q),
+ patch.object(TestDAO, "_convert_value_for_column", return_value="value"),
+ ):
+ mock_session = Mock()
+ mock_db.session = mock_session
+ mock_getattr.return_value = Mock()
+
+ mock_query = Mock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.one_or_none.side_effect = _make_operational_error()
+
+ with pytest.raises(OperationalError):
+ TestDAO._find_by_column("name", "test")
+
+
+def test_find_by_column_statement_error_still_returns_none():
+ """A genuine coercion StatementError is still absorbed as None (unchanged)."""
+
+ with (
+ patch("superset.daos.base.db") as mock_db,
+ patch("superset.daos.base.getattr") as mock_getattr,
+ patch("superset.daos.base.hasattr", return_value=True),
+ patch.object(TestDAO, "_apply_base_filter", side_effect=lambda q, *a, **k: q),
+ patch.object(TestDAO, "_convert_value_for_column", return_value="value"),
+ ):
+ mock_session = Mock()
+ mock_db.session = mock_session
+ mock_getattr.return_value = Mock()
+
+ mock_query = Mock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.one_or_none.side_effect = StatementError(
+ "invalid input", "SELECT 1", {}, Exception("coercion")
+ )
+
+ assert TestDAO._find_by_column("name", "test") is None
+
+
def _list_with_page_size(page_size: int) -> Mock:
"""
Run ``BaseDAO.list`` with a mocked query chain and return the mock query so
diff --git a/tests/unit_tests/dashboards/dao_tests.py b/tests/unit_tests/dashboards/dao_tests.py
index 09edfacd44b..6739081118e 100644
--- a/tests/unit_tests/dashboards/dao_tests.py
+++ b/tests/unit_tests/dashboards/dao_tests.py
@@ -23,6 +23,7 @@ from sqlalchemy.orm.session import Session
@pytest.fixture
def session_with_data(session: Session) -> Iterator[Session]:
+ from superset.models.core import FavStar # noqa: F401
from superset.models.dashboard import Dashboard
engine = session.get_bind()
@@ -42,12 +43,11 @@ def session_with_data(session: Session) -> Iterator[Session]:
session.rollback()
-def test_add_favorite(session: Session) -> None:
+def test_add_favorite(session_with_data: Session) -> None:
from superset.daos.dashboard import DashboardDAO
dashboard = DashboardDAO.find_by_id(100, skip_base_filter=True)
- if not dashboard:
- return
+ assert dashboard is not None
assert len(DashboardDAO.favorited_ids([dashboard])) == 0
DashboardDAO.add_favorite(dashboard)
@@ -57,12 +57,11 @@ def test_add_favorite(session: Session) -> None:
assert len(DashboardDAO.favorited_ids([dashboard])) == 1
-def test_remove_favorite(session: Session) -> None:
+def test_remove_favorite(session_with_data: Session) -> None:
from superset.daos.dashboard import DashboardDAO
dashboard = DashboardDAO.find_by_id(100, skip_base_filter=True)
- if not dashboard:
- return
+ assert dashboard is not None
assert len(DashboardDAO.favorited_ids([dashboard])) == 0
DashboardDAO.add_favorite(dashboard)
diff --git a/tests/unit_tests/views/test_error_handling.py b/tests/unit_tests/views/test_error_handling.py
index fca458b9111..71c95e33c99 100644
--- a/tests/unit_tests/views/test_error_handling.py
+++ b/tests/unit_tests/views/test_error_handling.py
@@ -24,6 +24,7 @@ import pytest
import sshtunnel
from flask import Flask, Response
from flask_babel import Babel
+from sqlalchemy.exc import IntegrityError, OperationalError, ProgrammingError
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import QueryObjectValidationError, SupersetException
@@ -68,6 +69,81 @@ class TestHandleApiExceptionSSHTunnelError:
)
+class TestHandleApiExceptionDatabaseErrors:
+ """
+ `OperationalError` is a `DatabaseError` subclass, so it must be matched by
+ its own clause ahead of the 422 handler for other `DatabaseError`s.
+ """
+
+ def _run(self, app, ex: Exception, *, guest: bool = False) -> Response:
+ @handle_api_exception
+ def view(self: object) -> FlaskResponse:
+ raise ex
+
+ with (
+ app.test_request_context(),
+ patch(
+ "superset.security.SupersetSecurityManager.is_guest_user",
+ return_value=guest,
+ ),
+ ):
+ return cast(Response, view(self=object()))
+
+ def test_operational_error_returns_500(self, app):
+ response = self._run(
+ app, OperationalError("SELECT 1", {}, Exception("connection closed"))
+ )
+
+ assert response.status_code == 500
+
+ def test_non_connection_database_error_still_returns_422(self, app):
+ response = self._run(
+ app,
+ ProgrammingError("SELECT 1", {}, Exception("relation does not exist")),
+ )
+
+ assert response.status_code == 422
+
+ def test_integrity_error_still_returns_422(self, app):
+ response = self._run(
+ app, IntegrityError("INSERT", {}, Exception("duplicate key"))
+ )
+
+ assert response.status_code == 422
+
+ def test_operational_error_message_is_redacted_for_guest_users(self, app):
+ """
+ The 500 clause hands the raw driver message to `json_error_response`,
+ which routes a bare string through `sanitize_error_message`. A driver
+ message quotes the host, port and user of the connection it failed on,
+ so an embedded viewer must receive the generic text instead.
+ """
+ leaky = (
+ "could not connect to server: Connection refused\n\tIs the server "
+ 'running on host "analytics-prod.internal" (10.0.4.17) and accepting '
+ "TCP/IP connections on port 5432?"
+ )
+
+ response = self._run(
+ app, OperationalError("SELECT 1", {}, Exception(leaky)), guest=True
+ )
+
+ assert response.status_code == 500
+ payload = json.loads(response.data)
+ assert payload["error"] == str(GENERIC_ERROR_MESSAGE)
+ for secret in ("analytics-prod.internal", "10.0.4.17", "5432"):
+ assert secret not in response.get_data(as_text=True)
+
+ def test_operational_error_message_is_kept_for_regular_users(self, app):
+ """The redaction above is guest-only; an operator still needs the detail."""
+ response = self._run(
+ app, OperationalError("SELECT 1", {}, Exception("Connection refused"))
+ )
+
+ assert response.status_code == 500
+ assert "Connection refused" in json.loads(response.data)["error"]
+
+
class TestShowUnexpectedException:
def _build_app_with_handlers(self) -> Flask:
# A fresh, minimal Flask app per test: `set_app_error_handlers` can