EnxDev commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4087708866
##########
tests/integration_tests/dashboards/soft_delete_tests.py:
##########
@@ -389,6 +389,9 @@ def test_restore_soft_deleted_dashboard(self) -> None:
self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
rv = self.client.post(f"/api/v1/dashboard/{dashboard_uuid}/restore")
assert rv.status_code == 200
+ # the UUID route still resolves to the archived row's integer id
+ log = self.get_latest_log("DashboardRestApi.restore")
+ assert log.dashboard_id == dashboard_id
Review Comment:
`test_delete_dashboard` passes an integer pk, which needs no lookup, so it
would still pass if resolution ran after the handler. Restore doesn't prove the
ordering either, because the row is back by then. Purge by UUID is the one case
where the comment in `log.py` about resolving first really matters.
Mind adding the same two lines to `test_purge_by_owner_permanently_deletes`
further down? That's the test that would catch someone moving the lookup later.
##########
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:
You asked about one row per object, and I'd go that way. Bulk delete is what
the list view uses when you remove several dashboards at once, and `WHERE
dashboard_id = 42` won't find those deletes while the ids only live in `json`.
`DBEventLogger` already writes one `Log` per record and falls back to
`record.get("dashboard_id")`, so the work is mostly building the records. I'd
trigger it from the bulk_delete routes and not from this rison check, though.
`favorite_status` sends an id list on every list page load, and that call
shouldn't turn into 25 rows.
--
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]
