mikebridge opened a new pull request, #45472:
URL: https://github.com/apache/superset/pull/45472
### SUMMARY
**TL;DR:** Copying a dashboard with a layout that leaves out some of the
source dashboard's current charts failed with a generic `400 Arguments are not
correct`, and nothing was logged. This PR fixes the copy and logs the cause
when a copy fails.
**Background.** In version history, "Open as new dashboard" forks an older
version by posting that version's layout to `POST /api/v1/dashboard/<id>/copy/`
(without `duplicate_slices`). The copy endpoint rebuilds the new dashboard's
charts from `json_metadata.positions`.
**Problem.** `DashboardDAO.copy_dashboard` first linked all of the source's
current charts to the new dashboard (`dash.slices = original_dash.slices`).
Then `set_dash_metadata` replaced that list with the charts in the layout. For
each chart the layout dropped, the chart's unloaded `Slice.dashboards` backref
collection was left holding a queued append followed by a remove. On flush,
SQLAlchemy issued `DELETE FROM dashboard_slices` for a row that was never
inserted. That raised `StaleDataError`, which the command's transaction handler
turned into `DashboardCopyError`, and the API returned the generic 400.
`on_error` logs only exceptions that carry an `exception` attribute, so nothing
was logged.
Any older version that doesn't contain every chart the dashboard has today
hits this. It also hits an empty layout. It happens on PostgreSQL and SQLite
alike.
**Changes.**
- `DashboardDAO.copy_dashboard` links the source's charts only when no
layout is sent. With a layout, `set_dash_metadata` remains the only place that
sets membership. The `duplicate_slices` path is unchanged, and the integration
test covers it with partial and empty layouts too.
- `copy_dash` logs `DashboardCopyError` with `exc_info`, so the underlying
database error appears in the logs. The response is unchanged. The log sits in
the REST route so the shared `on_error` helper, which every command's
transaction uses, keeps its behavior. The MCP duplicate tool still reports the
error without logging its cause.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behavior).
Before: `POST /api/v1/dashboard/<id>/copy/` with a layout that omits a
current chart returns `400 {"message": "Arguments are not correct"}`. After: it
returns `200`, and the new dashboard has exactly the charts in the layout. The
source dashboard is unchanged.
### TESTING INSTRUCTIONS
- Unit (SQLite): `pytest
tests/unit_tests/dao/test_copy_dashboard_legacy_ids.py -k dropped_from_layout`.
It fails with `StaleDataError` before the fix.
- Integration (run on PostgreSQL): `pytest
tests/integration_tests/dashboards/api_tests.py -k
"copy_dashboard_layout_without_current_charts or
copy_dashboard_logs_database_error"`. Both fail before the fix.
- Manually: with version history on, add a chart to a dashboard, then open
an earlier version in version history and choose "Open as new dashboard". The
new dashboard opens with that version's charts.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] 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
--
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]
