bito-code-review[bot] commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4095953348
##########
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", (), {})
Review Comment:
<!-- Bito Reply -->
The suggestion to annotate these module-level constants is consistent with
the project's coding standards for explicit typing. While these are simple
stand-in objects, adding the `: type` annotation ensures compliance with the
project's static analysis rules and maintains consistency across the codebase.
**tests/unit_tests/utils/log_tests.py**
```
_Dashboard: type = type("Dashboard", (), {})
_Slice: type = type("Slice", (), {})
_Database: type = type("Database", (), {})
```
##########
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(timezone.utc),
+ )
+ session.add(dashboard)
+ session.commit()
+
+ view = _view_for(Dashboard)
+ assert get_object_ids_from_view_args(view, {"id_or_slug": "audited-slug"})
== {
+ "dashboard_id": 100
+ }
+ assert get_object_ids_from_view_args(view, {"uuid": str(dashboard.uuid)})
== {
+ "dashboard_id": 100
+ }
+ assert get_object_ids_from_view_args(view, {"uuid_str":
str(uuid.uuid4())}) == {}
+ assert get_object_ids_from_view_args(view, {"id_or_slug": "missing"}) == {}
+
+
+def test_log_this_with_context_derives_object_id_from_route(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ """``log_this_with_context`` fills ``dashboard_id`` from the route's pk."""
+ mock_log = mocker.patch.object(DBEventLogger, "log")
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to annotate the `mock_log` variable is a standard
practice for improving type safety and readability in test files. While type
inference often works for `MagicMock`, explicit annotations help maintain
consistency across the test suite and assist static analysis tools. Applying
this suggestion is recommended to align with the project's coding standards.
**tests/unit_tests/utils/log_tests.py**
```
mock_log: MagicMock = mocker.patch.object(DBEventLogger, "log")
```
##########
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(timezone.utc),
+ )
+ session.add(dashboard)
+ session.commit()
+
+ view = _view_for(Dashboard)
+ assert get_object_ids_from_view_args(view, {"id_or_slug": "audited-slug"})
== {
+ "dashboard_id": 100
+ }
+ assert get_object_ids_from_view_args(view, {"uuid": str(dashboard.uuid)})
== {
+ "dashboard_id": 100
+ }
+ assert get_object_ids_from_view_args(view, {"uuid_str":
str(uuid.uuid4())}) == {}
+ assert get_object_ids_from_view_args(view, {"id_or_slug": "missing"}) == {}
+
+
+def test_log_this_with_context_derives_object_id_from_route(
+ app_context: None, mocker: MockerFixture
+) -> None:
+ """``log_this_with_context`` fills ``dashboard_id`` from the route's pk."""
+ mock_log = mocker.patch.object(DBEventLogger, "log")
+ logger = DBEventLogger()
Review Comment:
<!-- Bito Reply -->
The user is acknowledging a review comment regarding an untyped local
variable `logger` in `tests/unit_tests/utils/log_tests.py`. By stating they are
"leaving it" because it is "not a pattern the suite follows for locals with an
inferable type," the user is effectively declining the suggestion to add an
explicit type annotation.
--
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]
