Copilot commented on code in PR #43335:
URL: https://github.com/apache/superset/pull/43335#discussion_r3815600338
##########
tests/unit_tests/dao/base_dao_test.py:
##########
@@ -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,
+ ):
Review Comment:
`getattr`/`hasattr` are Python builtins, and `BaseDAO` likely calls them as
builtins (not as `superset.daos.base.getattr/hasattr`). Patching
`patch("superset.daos.base.getattr")` / `patch("superset.daos.base.hasattr")`
will fail unless `superset.daos.base` defines those names. Patch
`builtins.getattr` / `builtins.hasattr` instead, or avoid patching them by
configuring `TestDAO.model_cls`/attributes such that the real
`getattr`/`hasattr` behavior is exercised.
##########
tests/unit_tests/dao/base_dao_test.py:
##########
@@ -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"),
+ ):
Review Comment:
`getattr`/`hasattr` are Python builtins, and `BaseDAO` likely calls them as
builtins (not as `superset.daos.base.getattr/hasattr`). Patching
`patch("superset.daos.base.getattr")` / `patch("superset.daos.base.hasattr")`
will fail unless `superset.daos.base` defines those names. Patch
`builtins.getattr` / `builtins.hasattr` instead, or avoid patching them by
configuring `TestDAO.model_cls`/attributes such that the real
`getattr`/`hasattr` behavior is exercised.
##########
superset/daos/base.py:
##########
@@ -255,6 +255,11 @@ def find_by_id_or_uuid(
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".
Review Comment:
The same multi-line explanatory comment is duplicated at three catch sites.
Consider shortening it (e.g., a single line noting that `OperationalError`
should propagate) or extracting a small private helper (e.g.,
`_raise_if_operational_error(ex)`) to keep the intent while reducing repeated
comment blocks that are harder to maintain consistently.
--
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]
