Repository navigation
fix(dashboard): build a version copy's settings from that version - #45530
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45530 +/- ##
==========================================
+ Coverage 84.31% 84.33% +0.01%
==========================================
Files 3059 3059
Lines 202225 202319 +94
Branches 47367 47390 +23
==========================================
+ Hits 170516 170629 +113
+ Misses 28451 28432 -19
Partials 3258 3258
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| """Drop omitted chart references and use clone IDs when applicable.""" | ||
| new_id: int | None | ||
| return [ | ||
| new_id for value in values if (new_id := key_map.get(str(value))) is not None |
There was a problem hiding this comment.
Suggestion: A valid whole-number float ID such as 12.0 becomes "12.0" and misses the "12" map key, silently dropping that chart setting during the copy.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Type error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/daos/dashboard.py
**Line:** 330:330
**Comment:**
*Type Error: A valid whole-number float ID such as `12.0` becomes `"12.0"` and misses the `"12"` map key, silently dropping that chart setting during the copy.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Thanks. In practice this can't happen. Chart IDs in json_metadata and position_json are written by the frontend as JSON integers (12, never 12.0), and json.loads turns those into int, so str(value) matches the clone map's keys. The other copy and import paths key their maps the same way.
Even if a malformed payload did carry a float, the result would be that the reference is dropped, not pointed at the wrong chart. That's the safe failure for a copy. Leaving this as is.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag string-key lookup of chart IDs in dashboard copy logic for whole-number float IDs; dashboard metadata and position JSON encode these IDs as integers, and malformed references are safely dropped.
Applied to:
superset/daos/dashboard.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
LGTM with one docs nit. Reviewed the delta on top of #45528 (0099a023eb..46b2623bf2).
- Where "that version's" metadata comes from.
Dashboard.__versioned__versions the wholejson_metadatacolumn, so the snapshotcreateDashboardFromSnapshotsends is the complete settings blob as of that version. A key missing from the snapshot was really absent at that point. Defaulting it (refresh_frequency0,color_scheme"",label_colors{},cross_filters_enabledtrue, no native filters) matches what that version actually rendered. Taking it from the live dashboard would be the leak this PR removes.shared_label_colors,map_label_colorsandcolor_scheme_domainalready reset from the payload, and the frontend recomputes them on load. No migration since the versioning tables (2026-05-28) rewrites dashboardjson_metadata, so there's no schema drift between old snapshots and current validation yet. - Authz. No version ID reaches the server. The endpoint copies whatever metadata and layout the caller sends, gated on
is_editor(source), as before. This PR adds no new trust surface. The layout-chart IDOR from #45472 still applies and is still open. - Save as. The DAO change also covers the regular "Save as" flow. Both UI callers (
SaveModal→saveDashboardRequest, and version "Open as new") and the MCPduplicate_dashboardtool send the full metadata pluspositions, so none of them lose settings. - Remap. It still works after the switch. For
duplicate_slices,set_dash_metadatahas already remappedchart_configuration,crossFilters, native-filter scopes andselectedLayersto clone IDs._copy_chart_metadatathen filters those by clone ID and mapsexpanded_slicesandtimed_refresh_immune_slicesold→new. That also fixes a gap that predates this PR: Save-as-with-charts used to leave both lists on the original IDs. Archived slots become placeholders under #45528, so their entries are dropped. Soft-deleted members of a plain copy stay linked and keep their settings. - Tests. With only
superset/daos/dashboard.pyreverted to the #45528 version,test_copy_normalizes_legacy_chart_idsfails ({'1': True, '999999': True} != {'2': True}) and so doestest_copy_metadata_uses_snapshot_and_layout(live_onlyinherited). With the fix,tests/unit_tests/dao/test_copy_dashboard_legacy_ids.pyanddashboard_test.pypass (78). CI on 46b2623 is green, including test-postgres, test-mysql and test-sqlite.
UI check on a hosted environment: save a dashboard with charts A and B, set a refresh interval and a custom color scheme, and expand B. Open an earlier version that only had A via Version history → Open as new. The copy should have no refresh interval, the color scheme from that version, and no settings for B (Edit properties → Advanced).
| only those fields, so properties such as description, slug, certification, and | ||
| theme come from the dashboard as it stands today, not from the version. | ||
|
|
||
| The new dashboard's settings come from the selected version's metadata. Settings |
There was a problem hiding this comment.
Nit: the paragraph just above (lines 304-308, unchanged) now contradicts this one. It still says a new dashboard "is a copy of the live dashboard with the version's title, CSS, metadata, and layout applied over it". Its last clause is also wrong, before and after this PR: "description, slug, certification, and theme come from the dashboard as it stands today". DashboardCopySchema accepts only dashboard_title, css, json_metadata and duplicate_slices, and copy_dashboard sets nothing else apart from editors/viewers. So the copy's description, slug, certification and theme are empty or default, not inherited. Suggest folding the two paragraphs into one: title, CSS, settings and layout come from the version. Description, slug, certification and theme are not copied.
There was a problem hiding this comment.
Thanks, Matt. Fixed in b7db5b6: the docs now say title, CSS, settings and layout come from the version, while description, slug, certification and theme are not copied.
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
No blocking issues; LGTM. Dropping the live-metadata seed is correct, because Dashboard.__versioned__ stores the whole json_metadata column. The new chart-setting pruning is correct for both the shared-chart path and the duplicate-chart path. Nothing here weakens access control.
Reviewed the delta on top of #45528 (0099a023eb..b7db5b62c2) at head b7db5b62.
What I checked
- Security-relevant fields. None of
owners,roles,editors,viewers,published,slug,certified_by,theme_idor the embedded config live injson_metadata. The copy still sets editors and viewers to the creator, and leavespublished,slug, certification and theme at the new row's defaults. The version copy cannot bring any of those across. - Native-filter dataset grants.
native_filter_configurationtargets do act as a datasource grant, but only for guest users and for promiscuous-mode viewers (security/manager.py, theNATIVE_FILTERbranch). Copy callers already supplied that key verbatim before this PR, and an editor can PUT any value anyway. Removing the seed only removes inherited keys and grants nothing new. A filter that targets a since-deleted or unreadable dataset behaves the same as before, because query-time access checks still apply. - Chart access at copy time. This is unchanged. The server still links any
Slicethat the layout names. The only gate today is the frontend'sfetchReachableChartIdsswap, and #45538 adds the server-side check. This PR actually narrows the exposure: settings for charts that the client swapped to placeholders are now pruned too. - ID spaces in
_copy_chart_metadata.expanded_slicesandtimed_refresh_immune_slicesare not remapped byset_dash_metadata, so they correctly useoriginal_key_map.chart_configuration,global_chart_configuration, native-filter scopes and customization scopes have already been remapped by_remap_filter_scopes, so they correctly usecopied_key_map. Entries for charts that were not cloned keep their old IDs and drop out. An old ID cannot collide with a fresh clone ID on PG or MySQL. - Other callers still send full metadata. "Save as" (
SaveModalsends...dashboardInfo.metadata) and MCPduplicate_dashboard(source.json_metadata) both send the full metadata, so dropping the seed loses nothing for them. - Mutation tests. These all fail the unit suite: restoring
dash.params = original_dash.params, removing the_copy_chart_metadatacall, swapping the key map forexpanded_slicesor fortimed_refresh_immune_slices, and skippingglobal_chart_configuration.
Non-blocking
- Conflict with #45538.
superset/daos/dashboard.py:1104-1106: #45538 edits the samedash.params = original_dash.params/set_dash_metadata(...)lines to passallowed_chart_ids. Whichever PR merges second has to drop the seed line and keep the kwarg. Otherwise one fix silently undoes the other. The two compose cleanly in meaning: #45538 raises insideset_dash_metadatabeforecopied_idsis computed. config["id"] = int(chart_key)is untested.superset/daos/dashboard.py:403: no test fails if this line is deleted. It only matters when a storediddisagrees with its key, or isn't anintso_remap_chart_configurationskipped it. One assertion would cover it, for example achart_configurationentry{"<id>": {"id": "<id>"}}intest_copy_metadata_uses_snapshot_and_layout.- Old snapshots can fail validation (not introduced here).
DashboardJSONMetadataSchemauses marshmallow's defaultunknown=RAISE. A version saved with a metadata key the schema has since dropped will 400 on copy, unless it's the one handled byremove_show_native_filters. The client sent the snapshot's metadata before this PR too, so nothing changes here. It may be worth a follow-up if version copy is expected to work across schema changes.
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
removed
the
requires:rebase
|
Re-checked at
|
github-actions
Bot
added
the
requires:rebase
Preserve both independent UPDATING.md notices while retaining the reviewed dashboard-copy behavior. Full unit suite has only the four accepted local baseline failures; focused copy tests and branch-file hooks pass.
github-actions
Bot
added
requires:rebase
|
Re-checked at
|
github-actions
Bot
removed
the
requires:rebase
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Read through the delta on top of #45528 and the logic holds up. The remap is right in both copy modes, and Save as already sends the full synced metadata, so it won't lose settings.
Two small notes inline: whether the title wants a !, and a comment wording nit.
|
|
||
| ## Next | ||
|
|
||
| - **Dashboard copies use the supplied metadata as their complete settings.** |
There was a problem hiding this comment.
This changes the contract for any client that sends partial json_metadata to POST /copy/, not only the version fork.
Worth a ! in the title (fix(dashboard)!: ...) so it gets picked up as a breaking change? Your call, the entry here already covers the substance.
There was a problem hiding this comment.
Thanks, good call on the contract. Rather than change it for every copy caller, we made the replace-from-supplied-metadata behavior opt-in: replace_metadata on the copy request, which only the version fork sends. Save as, the MCP duplicate tool and other API clients keep seeding from the source as before. Chart-specific keys are still pruned and remapped to the copied layout in both modes. So this is no longer a breaking change, and I removed the UPDATING entry.
| # Copy callers supply the selected version's complete metadata. Seeding | ||
| # from the live dashboard would leak settings absent from that version. |
There was a problem hiding this comment.
Nit, take it or leave it. Save as and the MCP duplicate tool also land here, so pinning this on "the selected version" undersells the contract every caller now relies on.
| # Copy callers supply the selected version's complete metadata. Seeding | |
| # from the live dashboard would leak settings absent from that version. | |
| # Every copy caller (Save as, version fork, MCP duplicate) sends the | |
| # complete metadata. Seeding from the source would leak omitted settings. |
There was a problem hiding this comment.
Took it, adjusted for the opt-in: the comment now says the version fork sends complete metadata with replace_metadata, and other callers keep seeding from the source.
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
added
api
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
LGTM. Reviewed the diff and the thread replies; the fixes and regression tests cover the reported cases.
SUMMARY
What users saw. Opening an older dashboard version as a new dashboard could bring along newer settings that were absent from that version. Settings could also still refer to charts omitted from the copied layout.
Stacked on #45528, please review that first; this PR's own change is the snapshot-metadata diff in
0099a023eb..46b2623bf2.Why, in plain terms. The copy started with the live dashboard's settings and overlaid the selected version's values. Missing values therefore came from the live dashboard, and several chart-specific settings were never trimmed to the copied layout.
What this changes. Copies use the supplied version's metadata as their complete settings. Expanded charts, refresh exemptions, chart configuration and filter-scope chart references retain only charts in the copy; when charts are duplicated, those references use the new chart IDs. The source dashboard and ordinary dashboard-save behavior are unchanged.
The copy API already requires
json_metadata; clients sending partial metadata must send the full settings they want copied. Omitted settings use defaults.UPDATING.mddocuments this compatibility change. The shared metadata-save method and existing legacy filter pruning remain unchanged.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: backend copy correction without a UI change.
TESTING INSTRUCTIONS
Local validation:
ADDITIONAL INFORMATION