vikash7485 opened a new pull request, #38347:
URL: https://github.com/apache/superset/pull/38347
## Description
Fixes #38187
`dashboard_id` and `slice_id` columns in the `logs` table were always
`NULL` for every REST API mutation endpoint (POST, PUT, DELETE). This
made audit logs useless for filtering by dashboard or chart.
### Root Cause
`log_with_context()` in `utils/log.py` reads IDs from the merged HTTP
payload:
```python
dashboard_id = to_int(payload.get("dashboard_id")) # always None
slice_id = to_int(payload.get("slice_id")) # always None
```
`collect_request_payload()` only reads `request.form`, `request.args`,
and `request.json` body. URL path params (e.g. `/api/v1/dashboard/42`)
are never included, so the IDs were never captured.
### Fix
Used the existing `allow_extra_payload=True` mechanism — already
proven working in `DashboardRestApi.get` — to explicitly inject
`dashboard_id` / `slice_id` into the log payload after each operation.
**11 endpoints fixed across 2 files:**
`superset/dashboards/api.py` (7 endpoints):
- `post` → `add_extra_log_payload(dashboard_id=new_model.id)`
- `put` → `add_extra_log_payload(dashboard_id=changed_model.id)`
- `put_filters` → `add_extra_log_payload(dashboard_id=pk)`
- `put_chart_customizations` → `add_extra_log_payload(dashboard_id=pk)`
- `put_colors` → `add_extra_log_payload(dashboard_id=pk)`
- `delete` → `add_extra_log_payload(dashboard_id=pk)` called **before**
`DeleteDashboardCommand.run()` so the ID is captured before the row
is gone
- `bulk_delete` → `add_extra_log_payload(dashboard_ids=item_ids)` stores
the list in the `json` column (cannot fit multiple IDs in a single
integer column — this is intentional)
`superset/charts/api.py` (4 endpoints):
- `post` → `add_extra_log_payload(slice_id=new_model.id)`
- `put` → `add_extra_log_payload(slice_id=changed_model.id)`
- `delete` → `add_extra_log_payload(slice_id=pk)` called before deletion
- `bulk_delete` → `add_extra_log_payload(slice_ids=item_ids)`
Also added missing `Callable` to the `from typing import` line in
`charts/api.py`.
### Testing
**Unit tests** — `tests/integration_tests/event_logger_tests.py`
Added `TestMutationEndpointAuditIds` with 6 regression tests that mock
`DBEventLogger.log` and assert the correct ID is passed as a kwarg:
| Test | Asserts |
|---|---|
| `test_dashboard_put_logs_dashboard_id` | `dashboard_id == 42` |
| `test_dashboard_delete_logs_dashboard_id` | `dashboard_id == 42` |
| `test_dashboard_post_logs_dashboard_id` | `dashboard_id == 42` |
| `test_chart_put_logs_slice_id` | `slice_id == 42` |
| `test_chart_delete_logs_slice_id` | `slice_id == 42` |
| `test_chart_post_logs_slice_id` | `slice_id == 42` |
**Integration tests** — `tests/integration_tests/dashboards/api_tests.py`
and `tests/integration_tests/charts/api_tests.py`
Added DB-level assertions that query the `Log` model directly after
each mutation and confirm the ID column is non-NULL:
- `test_update_dashboard` → asserts `log.dashboard_id == dashboard_id`
- `test_delete_dashboard` → asserts `log.dashboard_id == dashboard_id`
- `test_update_chart` → asserts `log.slice_id == chart_id`
- `test_delete_chart` → asserts `log.slice_id == chart_id`
**SQL verification query (run after deployment):**
```sql
SELECT id, action, dashboard_id, slice_id, dttm
FROM logs
ORDER BY dttm DESC
LIMIT 20;
```
After this fix, `dashboard_id` will be non-NULL for all
`DashboardRestApi.*` actions and `slice_id` will be non-NULL for all
`ChartRestApi.*` actions.
### Checklist
- [x] Added unit tests
- [x] Added integration tests with DB-level assertions
- [x] Tested `delete` captures ID before row deletion
- [x] `bulk_delete` stores ID list in `json` column (documented above)
- [x] `Callable` import fix in `charts/api.py`
- [x] AST syntax check passed on all changed files
- [ ] Full test suite run pending (requires Flask-capable environment)
--
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]
