gabotorresruiz commented on code in PR #44553:
URL: https://github.com/apache/superset/pull/44553#discussion_r4087698133
##########
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:
Not a blocker, but `import_` and `copy_dash` also create objects whose ids
never reach `logs`, so "`dashboard_id` is populated for every
`DashboardRestApi.*` row" in the description is a bit stronger than what lands.
I ran both on this branch. An import created dashboard `44` and wrote a
`DashboardRestApi.import_` row with `dashboard_id` and `slice_id` both `NULL`
and a `json` of just `{"path": "/api/v1/dashboard/import/", "object_ref":
"DashboardRestApi.import_"}`. And `copy_dash` from source `23` created
dashboard `24` but logged `dashboard_id=23`, so the copy's own id is not
recoverable from `logs`. `ChartRestApi.import_` is the same shape.
Recording the source on a copy is defensible, but is that the intent? If you
do want the new id as well, it is the same `allow_extra_payload` hook you used
here, with one wrinkle worth knowing: `with_dashboard` calls `f(self, dash)`
with a fixed two argument signature at line 239, so it would have to forward
`add_extra_log_payload` for that to work.
##########
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:
Answering the open question in the description, since I think you can
already have one `logs` row per id with no schema change.
`log_with_context` has an `explode` contract at lines 329 and 330: it
`json.loads` the payload key named by `explode` into `records`, and
`DBEventLogger.log` then falls back to `record.get("dashboard_id")` per record
at line 522, so the integer column gets filled row by row. It is the same
mechanism the frontend uses via `/log/?explode=events`.
I ran it on this branch: `log_with_context(action=...,
explode="dashboard_ids", dashboard_ids='[{"dashboard_id": 101},
{"dashboard_id": 102}, {"dashboard_id": 103}]')` wrote three `logs` rows
carrying `dashboard_id` 101, 102 and 103.
There is a real tradeoff, in that each exploded row's `json` is only that
one record, so bulk rows would lose the rest of the request payload. Your call.
But since the audit query in #38187 reads `l.dashboard_id`, one row per id is
the shape that actually answers it.
##########
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:
Not a blocker and nothing is broken today, but the invariant this PR rests
on is decorator order dependent, and there is already one route in the tree
that proves it.
This reads the decorator's own `kwargs`, so the derivation is silently inert
wherever a decorator above `log_this*` rewrites the signature.
`DashboardRestApi.get` is exactly that: `with_dashboard` sits above the logger
at `superset/dashboards/api.py:654-655` and calls `f(self, dash)` positionally,
so `kwargs` is empty by the time this line runs. I verified it by counting
statements, `GET /api/v1/dashboard/<slug>` emits 16 statements on this branch
with no `SELECT dashboards.id FROM dashboards WHERE dashboards.slug = ?`
anywhere in the list. That route still logs its id, but only because it calls
`add_extra_log_payload(dashboard_id=dash.id, ...)` itself at
`superset/dashboards/api.py:734`, which is the per endpoint plumbing this PR is
trying to retire.
Cheap insurance, if you want it: take the route parameters from
`request.view_args`, which Flask fills from the URL regardless of what the
decorators do to the signature, and keep `kwargs` for the rison list. Both
names are already imported at the top of this file.
```python
route_args = dict(kwargs)
if has_request_context() and request:
route_args.update(request.view_args or {})
log(**kwargs, **get_object_ids_from_view_args(view, route_args))
```
I tried it locally: the same slug request then emits 17 statements with the
lookup present, every route I exercised still resolves its id, and
`tests/unit_tests/utils/log_tests.py` stays at 20 passed. The test that would
lock it in is a `DashboardRestApi.get` case in `log_tests.py` asserting the id
arrives without the handler's own `add_extra_log_payload` call.
--
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]
