rusackas opened a new pull request, #44553:
URL: https://github.com/apache/superset/pull/44553
### SUMMARY
Fixes #38187. Supersedes #38347 by @vikash7485, whose diagnosis and
create-path plumbing this builds on (credited as co-author).
**What was broken.** Every `DashboardRestApi` / `ChartRestApi` mutation
(`post`, `put`, `delete`, `bulk_delete`, `put_filters`, `put_colors`,
`put_chart_customizations`, `copy_dash`, `restore`, ...) wrote a `logs` row
whose `dashboard_id` / `slice_id` was NULL, so a user-level audit query could
see *that* someone edited a dashboard but never *which* one.
**Root cause.** `log_with_context` only reads those ids out of the request
payload (`request.form` / `request.args` / JSON body). The id of the object a
REST route acts on lives in the URL (`/api/v1/dashboard/<pk>`), which
`collect_request_payload` never looks at.
**Mechanism.** Rather than sprinkling
`add_extra_log_payload(dashboard_id=...)` into every handler (easy to forget on
the next endpoint), `log_this_with_context` now derives the id itself, in
`superset/utils/log.py`:
- The decorator already sees the API instance (`args[0]`) and Flask's view
args. If the API's `datamodel` is backed by `Dashboard` or `Slice` (the two
models `logs` has a column for), the route's `pk` / `id_or_slug` / `id_or_uuid`
/ `uuid` / `uuid_str` parameter becomes `dashboard_id` / `slice_id`. Integers
are used as-is; a slug or UUID is resolved to the integer id with a lookup that
bypasses the soft-delete visibility filter, so `restore` and `purge` still
identify the archived row. Resolution happens *before* the handler runs, which
is what makes `delete` work without ordering tricks.
- Any route on those two APIs, present or future, gets this for free:
favorites, screenshots, thumbnails, versions, embedded config, `copy_dash`, etc.
- `post` is the one case where the id does not exist until the command runs,
so create still uses the existing `allow_extra_payload` hook (as #38347 did).
- Bulk deletes keep one `logs` row per request. An integer column cannot
hold N ids, so the full list is recorded in the JSON payload as `dashboard_ids`
/ `slice_ids`, and the integer column stays NULL for those rows. Happy to
switch that to one row per object if people would rather query it that way.
On the ids-vs-UUIDs question from #38347: `logs.dashboard_id` /
`logs.slice_id` are integer columns, so integer ids are the right thing to
write here; no schema change.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend-only; see the audit query below).
### TESTING INSTRUCTIONS
1. Create, update and delete a dashboard and a chart through the REST API
(or the UI, which uses it), including a bulk delete.
2. Run the audit query from #38187 against the metadata DB:
```sql
SELECT l.id AS log_id, u.username AS user, l.action AS action_type,
l.dashboard_id, l.slice_id, l.dttm AS timestamp
FROM logs l
LEFT JOIN ab_user u ON l.user_id = u.id
ORDER BY l.dttm DESC;
```
`dashboard_id` is populated for every `DashboardRestApi.*` row and
`slice_id` for every `ChartRestApi.*` row; `bulk_delete` rows carry the id list
in `json`.
Automated coverage hits the real routes through the test client and asserts
on the persisted `Log` row: dashboard create/update/delete/bulk
delete/filters/colors/chart customizations/copy/restore-by-UUID and chart
create/update/delete/bulk delete in
`tests/integration_tests/{dashboards,charts}/api_tests.py` and
`dashboards/soft_delete_tests.py`, plus unit tests for the new helper in
`tests/unit_tests/utils/log_tests.py`.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #38187
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
