Repository navigation
fix(dashboard): drop missing semantic-view controls on restore - #45130
mikebridge wants to merge 4 commits into
Conversation
SC-125586: archived dashboards can outlive the semantic views referenced by their native filters and display controls. Remove affected controls and cascade links inside the restore transaction, after editorship and slug validation, and include a warning in REST and MCP responses. Preserve existing views regardless of provider/access availability and do not confuse table and semantic IDs. Cover persisted cleanup, rollback, source identity and warning delivery.
SC-125586 review follow-up: legacy JSON and malformed control shapes must not make archived dashboards unrestorable. Validate the cleanup input first, preserve malformed metadata unchanged and warn when cleanup is skipped. Require ASCII semantic IDs, handle oversized numeric input, and document invalid-ID removal without expanding the response schema. Add red-first persisted restore regressions.
…-restore-missing-semantic-filters
Catch decoder RecursionError before dependency cleanup, preserving legacy metadata and allowing recovery. Cover nested roots and controls through the persisted restore path; both cases failed before the fix.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45130 +/- ##
==========================================
+ Coverage 82.72% 82.73% +0.01%
==========================================
Files 3011 3011
Lines 188350 188415 +65
Branches 43725 43740 +15
==========================================
+ Hits 155819 155893 +74
+ Misses 29761 29753 -8
+ Partials 2770 2769 -1
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}
CodeAnt PR Risk: Low Risk
Assessed commit: |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approving — this is a well-designed, genuinely well-tested fix. The existence-not-permission check is the right signal (SemanticView is a hard delete via AuditMixinNullable, so the probe is exact), the "skip + warn, never block restore" fallback is the correct recovery posture, metadata is preserved byte-for-byte on the skip path, and the transaction/rollback behavior is asserted against a real DB across all 40 semantic-restore cases. Security model is intact: restore is already gated by editorship and the view probe leaks nothing. 🟢
Two non-blocking items I'd suggest tightening, either here or in a follow-up:
1. Valid-but-non-dict metadata is falsely reported as "malformed"
In _parse_restore_metadata, a json_metadata of "null" or "[]" is valid JSON but non-dict, so it returns None and prepare_restore appends the "metadata is malformed … review the dashboard filters" warning on a dashboard that actually has nothing to clean. Legacy rows can carry these values, so users get a misleading warning telling them to review filters that are fine. Consider treating a non-dict root as "nothing to clean" (no-op, no warning) and reserving the warning for genuine json.loads / structural failures.
2. Legacy chart_customization_config semantic refs can survive restore silently
Cleanup keys off targets[].datasourceType == "semantic_view". Pre-migration chart-customization entries (shape {"customization": {"dataset": N}}, per isLegacyChartCustomizationFormat in migrateChartCustomization.ts) have no targets and no datasourceType discriminator, so a legacy control bound to a deleted semantic view is kept with no warning — the one case that defeats the fix's purpose without even flagging it. This may be an unavoidable limitation if the legacy format can't be distinguished from a plain-dataset ref — but could you confirm whether legacy entries can reference semantic views? If so, migrate-then-check; if not, worth documenting the carve-out, since the docs currently state all controls "referencing a semantic view" are removed.
A few smaller nits (hardcoded "semantic_view" literal vs DatasourceType.SEMANTIC_VIEW, duplicated key tuple across the two helpers, and a theoretical duplicate-id cascade-pruning edge) are minor and left to your discretion.
This branch has not been deployed
2 participants
SUMMARY
An archived dashboard can outlive a semantic view used by its native filters or display controls. Restoring the dashboard previously kept those dangling references, leaving broken controls in the recovered dashboard.
Restore now removes an entire control when any of its explicitly typed semantic-view targets is missing or has an invalid ID, and removes links to that control from the remaining controls'
cascadeParentIds. Table filters and other unaffected controls are preserved, including table targets with the same numeric ID as a deleted semantic view.The check uses metadata existence, not provider availability or datasource permissions: an existing view is kept even when its provider is disabled or unavailable to the restoring user. Cleanup and unarchive share the existing transaction, after editorship and slug validation. If stored metadata cannot be safely parsed or traversed, restore leaves its bytes unchanged, skips cleanup and returns a warning instead of preventing recovery.
Warnings are returned in the existing REST/MCP
messagefield. This PR does not add a UI warning toast; the archive UI does not display successful response messages. Callers should review the remaining dashboard filters before relying on its results.This complements #45071, which guards semantic-source deletion for live dependent assets. Archived dashboards are intentionally excluded from that guard, so they need this recovery handling. This is not a new concurrency protocol: a concurrent or later source deletion can still invalidate a restored dashboard.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: backend restore handling, with no UI change.
TESTING INSTRUCTIONS
SOFT_DELETEandSEMANTIC_LAYERS. Create a dashboard with a semantic-view native filter or display control, an unaffected table filter, and a control cascading from the semantic control.messagewarns about the removed control. No UI warning toast is expected.pytest tests/unit_tests/commands/dashboard/restore_semantic_filters_test.py \ tests/unit_tests/commands/dashboard/restore_test.py \ tests/unit_tests/commands/test_base_restore_command.py \ tests/unit_tests/soft_delete/test_restore_purge_hoist.py \ tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py \ tests/unit_tests/commands/chart/restore_test.py \ tests/unit_tests/commands/dataset/restore_test.pyLocal validation: the focused suite passed 124 tests. Both deeply nested JSON regression cases failed before the parser fix and passed after it. Changed-file pre-commit hooks passed, including MyPy, Ruff and pylint. PostgreSQL/MySQL and browser validation were not run locally.
Whole unit suite (
TZ=UTC): 22,668 passed, 40 skipped, 2 xfailed, with exactly three known local-environment baseline failures in unchanged numeric-contract and translation-fixture tests. All 40 semantic-restore cases passed in that full run.ADDITIONAL INFORMATION
SOFT_DELETE,SEMANTIC_LAYERSRelated: #45071. @aminghadersohi @rebenitez1802
AI-assisted implementation and review with Codex and Claude.