rusackas commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4095951044
##########
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:
Same pattern as `row_cls = type("Row", (), {})` in
`commands/datasource/list_test.py`, none of the suite's constructed-type locals
get annotated. Leaving it.
##########
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:
mypy infers `MagicMock` fine here, and none of the other
`mocker.patch.object` locals in this file (or the wider test suite) are
annotated either. Leaving it.
##########
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:
Same as the `mock_log` one below, not a pattern the suite follows for locals
with an inferable type. Leaving it.
##########
superset/dashboards/api.py:
##########
@@ -972,6 +976,9 @@ def post(self) -> Response:
return self.response_400(message=error.messages)
try:
new_model = CreateDashboardCommand(item).run()
+ # The id only exists once the command has run, so the event
+ # logger cannot derive it from the route.
+ add_extra_log_payload(dashboard_id=new_model.id)
Review Comment:
Good catch, updated the description to call out `import_` staying NULL and
`copy_dash` logging the source id rather than the copy's. Recording the source
seems right to me since that's the object the request actually acted on. Open
to adding `add_extra_log_payload` for the new id too if that's wanted, but
wanted to flag the `with_dashboard` wrinkle first since you already found it.
##########
superset/utils/log.py:
##########
@@ -31,12 +32,98 @@
from sqlalchemy import inspect as sa_inspect
from sqlalchemy.exc import SQLAlchemyError
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
from superset.extensions import stats_logger_manager
from superset.utils import json
from superset.utils.core import get_user_id, LoggerLevel, to_int
logger = logging.getLogger(__name__)
+# The ``logs`` table has an integer column for the dashboard or chart a request
+# touched. This maps the model behind a REST API's ``datamodel`` to that column
+# so every route on the matching API populates it without per-endpoint
plumbing.
+LOG_OBJECT_ID_COLUMNS: dict[str, str] = {
+ "Dashboard": "dashboard_id",
+ "Slice": "slice_id",
+}
+
+# Route parameters that identify the single object a REST API route acts on.
+OBJECT_ID_VIEW_ARGS: tuple[str, ...] = (
+ "pk",
+ "id_or_slug",
+ "id_or_uuid",
+ "uuid",
+ "uuid_str",
+)
+
+
+def _resolve_object_id(model: Any, identifier: Any) -> int | None:
+ """
+ Turn a route identifier (id, UUID or slug) into the model's integer id.
+
+ Slugs and UUIDs are looked up bypassing the soft-delete visibility filter
so
+ that restore and purge routes can still identify the archived row they act
+ on. Lookup failures never propagate: an unlogged id must not fail a
request.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import db
+
+ try:
+ return int(identifier)
+ except (TypeError, ValueError):
+ pass
+
+ try:
+ criterion = model.uuid == uuid.UUID(str(identifier))
+ except ValueError:
+ if not hasattr(model, "slug"):
+ return None
+ criterion = model.slug == str(identifier)
+
+ try:
+ return (
+ db.session.query(model.id)
+ .filter(criterion)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model}})
+ .scalar()
+ )
+ except SQLAlchemyError:
+ logger.debug(
+ "Could not resolve %s %r for event logging", model.__name__,
identifier
+ )
+ return None
+
+
+def get_object_ids_from_view_args(
+ view: Any, view_args: dict[str, Any]
+) -> dict[str, Any]:
+ """
+ Derive the ``dashboard_id`` / ``slice_id`` log fields for a REST API route.
+
+ ``view`` is the API instance the logged route was called on and
+ ``view_args`` are the keyword arguments Flask passed to it. The result is
+ empty unless the API is backed by a model that ``logs`` has a column for.
+
+ A single-object route (``/<pk>``, ``/<id_or_slug>``, ``/<uuid>``, ...)
+ yields e.g. ``{"dashboard_id": 42}``. A bulk route identified by a rison
+ list of ids yields ``{"dashboard_ids": [...]}`` for the JSON payload
+ instead, since the integer column can only hold one id.
+ """
+ model = getattr(getattr(view, "datamodel", None), "obj", None)
+ column = LOG_OBJECT_ID_COLUMNS.get(getattr(model, "__name__", ""))
+ if column is None:
+ return {}
+
+ for key in OBJECT_ID_VIEW_ARGS:
+ if key in view_args:
+ object_id = _resolve_object_id(model, view_args[key])
+ return {column: object_id} if object_id is not None else {}
+
+ ids = view_args.get("rison")
+ if isinstance(ids, list) and ids and all(isinstance(i, int) for i in ids):
+ return {f"{column}s": ids}
Review Comment:
Hadn't traced `explode` that far, nice find, cleaner than a schema change.
I'd rather keep this PR scoped to the id-resolution mechanism and follow up
separately on switching `bulk_delete` to one row per object, the payload
tradeoff you flagged is worth its own discussion.
##########
superset/utils/log.py:
##########
@@ -321,7 +408,10 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
with self.log_context(
action=action_str, object_ref=object_ref_str, **wrapper_kwargs
) as log:
- log(**kwargs)
+ # Resolve the object's id before the route runs so that delete
+ # and purge can still identify the row they are about to
remove.
+ view = args[0] if args else None
+ log(**kwargs, **get_object_ids_from_view_args(view, kwargs))
Review Comment:
Good catch, this is real. Pulled in the `request.view_args` fix pretty much
as you wrote it and added the `DashboardRestApi.get` case to `log_tests.py` you
described. Thanks for tracing that all the way through with the statement
count, that's more rigor than I put into the original.
##########
superset/utils/log.py:
##########
@@ -31,12 +32,98 @@
from sqlalchemy import inspect as sa_inspect
from sqlalchemy.exc import SQLAlchemyError
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
from superset.extensions import stats_logger_manager
from superset.utils import json
from superset.utils.core import get_user_id, LoggerLevel, to_int
logger = logging.getLogger(__name__)
+# The ``logs`` table has an integer column for the dashboard or chart a request
+# touched. This maps the model behind a REST API's ``datamodel`` to that column
+# so every route on the matching API populates it without per-endpoint
plumbing.
+LOG_OBJECT_ID_COLUMNS: dict[str, str] = {
+ "Dashboard": "dashboard_id",
+ "Slice": "slice_id",
+}
+
+# Route parameters that identify the single object a REST API route acts on.
+OBJECT_ID_VIEW_ARGS: tuple[str, ...] = (
+ "pk",
+ "id_or_slug",
+ "id_or_uuid",
+ "uuid",
+ "uuid_str",
+)
+
+
+def _resolve_object_id(model: Any, identifier: Any) -> int | None:
+ """
+ Turn a route identifier (id, UUID or slug) into the model's integer id.
+
+ Slugs and UUIDs are looked up bypassing the soft-delete visibility filter
so
+ that restore and purge routes can still identify the archived row they act
+ on. Lookup failures never propagate: an unlogged id must not fail a
request.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import db
+
+ try:
+ return int(identifier)
+ except (TypeError, ValueError):
+ pass
+
+ try:
+ criterion = model.uuid == uuid.UUID(str(identifier))
+ except ValueError:
+ if not hasattr(model, "slug"):
+ return None
+ criterion = model.slug == str(identifier)
+
+ try:
+ return (
+ db.session.query(model.id)
+ .filter(criterion)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model}})
+ .scalar()
+ )
+ except SQLAlchemyError:
+ logger.debug(
+ "Could not resolve %s %r for event logging", model.__name__,
identifier
+ )
+ return None
+
+
+def get_object_ids_from_view_args(
+ view: Any, view_args: dict[str, Any]
+) -> dict[str, Any]:
+ """
+ Derive the ``dashboard_id`` / ``slice_id`` log fields for a REST API route.
+
+ ``view`` is the API instance the logged route was called on and
+ ``view_args`` are the keyword arguments Flask passed to it. The result is
+ empty unless the API is backed by a model that ``logs`` has a column for.
+
+ A single-object route (``/<pk>``, ``/<id_or_slug>``, ``/<uuid>``, ...)
+ yields e.g. ``{"dashboard_id": 42}``. A bulk route identified by a rison
+ list of ids yields ``{"dashboard_ids": [...]}`` for the JSON payload
+ instead, since the integer column can only hold one id.
+ """
+ model = getattr(getattr(view, "datamodel", None), "obj", None)
+ column = LOG_OBJECT_ID_COLUMNS.get(getattr(model, "__name__", ""))
+ if column is None:
+ return {}
+
+ for key in OBJECT_ID_VIEW_ARGS:
+ if key in view_args:
+ object_id = _resolve_object_id(model, view_args[key])
+ return {column: object_id} if object_id is not None else {}
+
+ ids = view_args.get("rison")
+ if isinstance(ids, list) and ids and all(isinstance(i, int) for i in ids):
+ return {f"{column}s": ids}
Review Comment:
Agreed on triggering it from `bulk_delete` specifically rather than the
rison check generally, `favorite_status` turning into 25 rows on every list
load would be bad. Same as the reply above, keeping this one scoped to id
resolution and leaving the row-shape change as a follow-up.
--
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]
