bito-code-review[bot] commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4085085133
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped mock variable</b></div>
<div id="fix">
`mock_log` is an untyped local. BITO.md adaptive rule [12787] requires
explicit type annotations for all mock variables in test files (`variable:
MagicMock`), and rule [13153] extends this to all locals even when inferable.
Annotate as `mock_log: MagicMock`.
</div>
</div>
<small><i>Code Review Run #011404</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped module constants</b></div>
<div id="fix">
Module-level constants `_Dashboard`, `_Slice`, `_Database` (lines 55-58)
carry no type annotations. BITO.md adaptive rule [15461] requires explicit
annotations on all module-level constants, even when inferable. Annotate each
as `: type`.
</div>
</div>
<small><i>Code Review Run #011404</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Untyped local logger</b></div>
<div id="fix">
`logger` is an untyped local holding the `DBEventLogger` instance used by
the decorator on line 140. BITO.md adaptive rule [13153] requires explicit
annotations for all locals in test files even when inferable. Annotate as
`logger: DBEventLogger`.
</div>
</div>
<small><i>Code Review Run #011404</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
