aminghadersohi commented on code in PR #43335:
URL: https://github.com/apache/superset/pull/43335#discussion_r3815898315
##########
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:
This pattern works as written.
`unittest.mock.patch("superset.daos.base.getattr")` resolves the original via
`getattr(module, "getattr")`, which returns the builtin, and then binds a
`getattr` name into the module globals so the function under test picks it up
(name resolution: globals before builtins). It is restored on exit. Evidence:
the pre-existing tests in this file already patch `superset.daos.base.getattr`
the same way and pass, and the `unit-tests-required` job on this PR is green
with these new tests included. Isolated repro confirms it too:
```python
import types, sys
m = types.ModuleType("t")
exec("def f(o,n):\n return getattr(o,n) if hasattr(o,n) else None",
m.__dict__)
sys.modules["t"] = m
from unittest.mock import patch
with patch("t.getattr") as g, patch("t.hasattr", return_value=True):
g.return_value = "PATCHED"
assert m.f(object(), "x") == "PATCHED" # passes
```
Keeping as-is.
##########
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:
Same as the thread on line 279 — patching the builtin name on the module
works (mock binds it into module globals, restored on exit), and these tests
pass in the green `unit-tests-required` job. Keeping as-is.
##########
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:
I looked at extracting a `_raise_if_operational_error(ex)` helper but it
reads worse than the inline form: the guard is a two-line `except
OperationalError: raise` whose whole value is being explicit and local at each
call site, and a helper that conditionally re-raises hides control flow
(readers have to trust that a normal-looking call sometimes raises). The
comment is duplicated because the three sites are genuinely independent and
each is clearer with its rationale next to it. Preferring readability over
de-duplication here, so leaving the three explicit guards.
--
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]
