bito-code-review[bot] commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4082940407
##########
tests/unit_tests/utils/log_tests.py:
##########
@@ -35,3 +48,105 @@ def test_log_from_status_info() -> None:
(func, log_level) = get_logger_from_status(300)
assert func.__name__ == "info"
assert log_level == "info"
+
+
+# Stand-ins for the models behind ``DashboardRestApi`` / ``ChartRestApi``
+# ``datamodel``: the helper only inspects the class name, so no ORM is needed.
+_Dashboard = type("Dashboard", (), {})
+_Slice = type("Slice", (), {})
+# A model that ``logs`` has no id column for.
+_Database = type("Database", (), {})
+
+
+def _view_for(model: type) -> SimpleNamespace:
+ """Build the minimal REST API shape the event logger inspects."""
+ return SimpleNamespace(datamodel=SimpleNamespace(obj=model))
+
+
[email protected](
+ "model,view_args,expected",
+ [
+ (_Dashboard, {"pk": 42}, {"dashboard_id": 42}),
+ (_Dashboard, {"pk": "42"}, {"dashboard_id": 42}),
+ (_Dashboard, {"id_or_slug": "7"}, {"dashboard_id": 7}),
+ (_Slice, {"pk": "3"}, {"slice_id": 3}),
+ (_Slice, {"id_or_uuid": 3}, {"slice_id": 3}),
+ (_Dashboard, {"rison": [1, 2, 3]}, {"dashboard_ids": [1, 2, 3]}),
+ (_Slice, {"rison": [5]}, {"slice_ids": [5]}),
+ # rison payloads that are not a list of ids (list endpoints,
thumbnails)
+ (_Dashboard, {"rison": {"columns": ["id"]}}, {}),
+ (_Dashboard, {"rison": []}, {}),
+ (_Dashboard, {"rison": [1, "a"]}, {}),
+ # routes with no object identifier at all (create, import, list)
+ (_Dashboard, {}, {}),
+ # a route parameter takes precedence over a rison list
+ (_Dashboard, {"pk": 9, "rison": [1, 2]}, {"dashboard_id": 9}),
+ # models without a ``logs`` column never contribute ids
+ (_Database, {"pk": 1}, {}),
+ (_Database, {"rison": [1, 2]}, {}),
+ ],
+)
+def test_get_object_ids_from_view_args(
+ model: type, view_args: dict[str, Any], expected: dict[str, Any]
+) -> None:
+ assert get_object_ids_from_view_args(_view_for(model), view_args) ==
expected
+
+
+def test_get_object_ids_from_view_args_without_datamodel() -> None:
+ """Plain views and free functions decorated with the logger are ignored."""
+ assert get_object_ids_from_view_args(None, {"pk": 1}) == {}
+ assert get_object_ids_from_view_args(object(), {"pk": 1}) == {}
+
+
+def test_get_object_ids_from_view_args_resolves_slug_and_uuid(
+ session: Session,
+) -> None:
+ """Slug and UUID routes resolve to the integer id, even when archived."""
+ from superset.models.core import FavStar # noqa: F401
+ from superset.models.dashboard import Dashboard
+
+ Dashboard.metadata.create_all(session.get_bind()) # pylint:
disable=no-member
+ dashboard = Dashboard(
+ id=100,
+ dashboard_title="audited",
+ slug="audited-slug",
+ uuid=uuid.uuid4(),
+ deleted_at=datetime.now(),
Review Comment:
<!-- Bito Reply -->
The update to use `datetime.now(timezone.utc)` correctly addresses the
reviewer's concern regarding naive datetime usage in the fixture. This change
ensures timezone-aware datetimes are used, which prevents potential comparison
bugs as noted in the review.
**tests/unit_tests/utils/log_tests.py**
```
deleted_at=datetime.now(timezone.utc),
```
##########
tests/unit_tests/utils/log_tests.py:
##########
@@ -35,3 +48,105 @@ def test_log_from_status_info() -> None:
(func, log_level) = get_logger_from_status(300)
assert func.__name__ == "info"
assert log_level == "info"
+
+
+# Stand-ins for the models behind ``DashboardRestApi`` / ``ChartRestApi``
+# ``datamodel``: the helper only inspects the class name, so no ORM is needed.
+_Dashboard = type("Dashboard", (), {})
+_Slice = type("Slice", (), {})
+# A model that ``logs`` has no id column for.
+_Database = type("Database", (), {})
+
+
+def _view_for(model: type) -> SimpleNamespace:
+ """Build the minimal REST API shape the event logger inspects."""
+ return SimpleNamespace(datamodel=SimpleNamespace(obj=model))
+
+
[email protected](
+ "model,view_args,expected",
+ [
+ (_Dashboard, {"pk": 42}, {"dashboard_id": 42}),
+ (_Dashboard, {"pk": "42"}, {"dashboard_id": 42}),
+ (_Dashboard, {"id_or_slug": "7"}, {"dashboard_id": 7}),
+ (_Slice, {"pk": "3"}, {"slice_id": 3}),
+ (_Slice, {"id_or_uuid": 3}, {"slice_id": 3}),
+ (_Dashboard, {"rison": [1, 2, 3]}, {"dashboard_ids": [1, 2, 3]}),
+ (_Slice, {"rison": [5]}, {"slice_ids": [5]}),
+ # rison payloads that are not a list of ids (list endpoints,
thumbnails)
+ (_Dashboard, {"rison": {"columns": ["id"]}}, {}),
+ (_Dashboard, {"rison": []}, {}),
+ (_Dashboard, {"rison": [1, "a"]}, {}),
+ # routes with no object identifier at all (create, import, list)
+ (_Dashboard, {}, {}),
+ # a route parameter takes precedence over a rison list
+ (_Dashboard, {"pk": 9, "rison": [1, 2]}, {"dashboard_id": 9}),
+ # models without a ``logs`` column never contribute ids
+ (_Database, {"pk": 1}, {}),
+ (_Database, {"rison": [1, 2]}, {}),
+ ],
+)
+def test_get_object_ids_from_view_args(
+ model: type, view_args: dict[str, Any], expected: dict[str, Any]
+) -> None:
+ assert get_object_ids_from_view_args(_view_for(model), view_args) ==
expected
+
+
+def test_get_object_ids_from_view_args_without_datamodel() -> None:
+ """Plain views and free functions decorated with the logger are ignored."""
+ assert get_object_ids_from_view_args(None, {"pk": 1}) == {}
+ assert get_object_ids_from_view_args(object(), {"pk": 1}) == {}
+
+
+def test_get_object_ids_from_view_args_resolves_slug_and_uuid(
+ session: Session,
+) -> None:
+ """Slug and UUID routes resolve to the integer id, even when archived."""
+ from superset.models.core import FavStar # noqa: F401
+ from superset.models.dashboard import Dashboard
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to move imports to the module level and remove the
unused `FavStar` import is technically sound for general code cleanliness.
However, as you noted, the project follows a pattern of using function-scoped
imports in test files to keep the module import-light and avoid loading
unnecessary ORM models. Given that this pattern is consistent with the rest of
the test file and sibling unit tests, maintaining the current approach is
acceptable for this specific context.
--
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]
