From 1a098fbd3fc02dd91fa884cd7f48a6b80b0106fe Mon Sep 17 00:00:00 2001 From: Ramiro Aquino Romero Date: Mon, 12 Jan 2026 02:08:49 -0400 Subject: [PATCH 1/2] fix: Handle missing chart references in dashboard import --- superset/commands/chart/delete.py | 136 +++++++++++++++++- .../commands/dashboard/importers/v1/utils.py | 5 +- .../commands/importers/v1/utils_test.py | 128 +++++++++++++++++ 3 files changed, 267 insertions(+), 2 deletions(-) diff --git a/superset/commands/chart/delete.py b/superset/commands/chart/delete.py index 00e6d201bcc9..fd42736029b5 100644 --- a/superset/commands/chart/delete.py +++ b/superset/commands/chart/delete.py @@ -20,7 +20,7 @@ from flask_babel import lazy_gettext as _ -from superset import security_manager +from superset import db, security_manager from superset.commands.base import BaseCommand from superset.commands.chart.exceptions import ( ChartDeleteFailedError, @@ -31,7 +31,9 @@ from superset.daos.chart import ChartDAO from superset.daos.report import ReportScheduleDAO from superset.exceptions import SupersetSecurityException +from superset.models.dashboard import Dashboard from superset.models.slice import Slice +from superset.utils import json from superset.utils.decorators import on_error, transaction logger = logging.getLogger(__name__) @@ -46,6 +48,9 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() assert self._models + # Clean up dashboard metadata before deleting charts + for chart in self._models: + self._cleanup_dashboard_metadata(chart.id) ChartDAO.delete(self._models) def validate(self) -> None: @@ -68,3 +73,132 @@ def validate(self) -> None: security_manager.raise_for_ownership(model) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex + + def _cleanup_dashboard_metadata( # noqa: C901 + self, chart_id: int + ) -> None: + """ + Remove references to this chart from all dashboard metadata. + + When a chart is deleted, dashboards may still contain references to the + chart ID in various metadata fields (expanded_slices, filter_scopes, etc.). + This method cleans up those references to prevent issues during dashboard + export/import. + """ + # Find all dashboards that contain this chart + dashboards = ( + db.session.query(Dashboard) + .filter(Dashboard.slices.any(id=chart_id)) # type: ignore[attr-defined] + .all() + ) + + for dashboard in dashboards: + metadata = dashboard.params_dict + modified = False + + # Clean up expanded_slices + if "expanded_slices" in metadata: + chart_id_str = str(chart_id) + if chart_id_str in metadata["expanded_slices"]: + del metadata["expanded_slices"][chart_id_str] + modified = True + logger.info( + "Removed chart %s from expanded_slices in dashboard %s", + chart_id, + dashboard.id, + ) + + # Clean up timed_refresh_immune_slices + if "timed_refresh_immune_slices" in metadata: + if chart_id in metadata["timed_refresh_immune_slices"]: + metadata["timed_refresh_immune_slices"].remove(chart_id) + modified = True + logger.info( + "Removed chart %s from timed_refresh_immune_slices " + "in dashboard %s", + chart_id, + dashboard.id, + ) + + # Clean up filter_scopes + if "filter_scopes" in metadata: + chart_id_str = str(chart_id) + if chart_id_str in metadata["filter_scopes"]: + del metadata["filter_scopes"][chart_id_str] + modified = True + logger.info( + "Removed chart %s from filter_scopes in dashboard %s", + chart_id, + dashboard.id, + ) + # Also clean from immune lists + for filter_scope in metadata["filter_scopes"].values(): + for attributes in filter_scope.values(): + if chart_id in attributes.get("immune", []): + attributes["immune"].remove(chart_id) + modified = True + + # Clean up default_filters + if "default_filters" in metadata: + default_filters = json.loads(metadata["default_filters"]) + chart_id_str = str(chart_id) + if chart_id_str in default_filters: + del default_filters[chart_id_str] + metadata["default_filters"] = json.dumps(default_filters) + modified = True + logger.info( + "Removed chart %s from default_filters in dashboard %s", + chart_id, + dashboard.id, + ) + + # Clean up native_filter_configuration scope exclusions + if "native_filter_configuration" in metadata: + for native_filter in metadata["native_filter_configuration"]: + scope_excluded = native_filter.get("scope", {}).get("excluded", []) + if chart_id in scope_excluded: + scope_excluded.remove(chart_id) + modified = True + logger.info( + "Removed chart %s from native_filter_configuration " + "in dashboard %s", + chart_id, + dashboard.id, + ) + + # Clean up chart_configuration + if "chart_configuration" in metadata: + chart_id_str = str(chart_id) + if chart_id_str in metadata["chart_configuration"]: + del metadata["chart_configuration"][chart_id_str] + modified = True + logger.info( + "Removed chart %s from chart_configuration in dashboard %s", + chart_id, + dashboard.id, + ) + + # Clean up global_chart_configuration scope exclusions + if "global_chart_configuration" in metadata: + scope_excluded = ( + metadata["global_chart_configuration"] + .get("scope", {}) + .get("excluded", []) + ) + if chart_id in scope_excluded: + scope_excluded.remove(chart_id) + modified = True + logger.info( + "Removed chart %s from global_chart_configuration " + "in dashboard %s", + chart_id, + dashboard.id, + ) + + if modified: + dashboard.json_metadata = json.dumps(metadata) + logger.info( + "Cleaned up metadata for dashboard %s after deleting chart %s", + dashboard.id, + chart_id, + ) diff --git a/superset/commands/dashboard/importers/v1/utils.py b/superset/commands/dashboard/importers/v1/utils.py index c506db72cb6b..20037032824c 100644 --- a/superset/commands/dashboard/importers/v1/utils.py +++ b/superset/commands/dashboard/importers/v1/utils.py @@ -75,7 +75,9 @@ def update_id_refs( # pylint: disable=too-many-locals # noqa: C901 metadata = fixed.get("metadata", {}) if "timed_refresh_immune_slices" in metadata: metadata["timed_refresh_immune_slices"] = [ - id_map[old_id] for old_id in metadata["timed_refresh_immune_slices"] + id_map[old_id] + for old_id in metadata["timed_refresh_immune_slices"] + if old_id in id_map ] if "filter_scopes" in metadata: @@ -100,6 +102,7 @@ def update_id_refs( # pylint: disable=too-many-locals # noqa: C901 metadata["expanded_slices"] = { str(id_map[int(old_id)]): value for old_id, value in metadata["expanded_slices"].items() + if int(old_id) in id_map } if "default_filters" in metadata: diff --git a/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py b/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py index 72bf490a5b02..08f440944f88 100644 --- a/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py +++ b/tests/unit_tests/dashboards/commands/importers/v1/utils_test.py @@ -199,3 +199,131 @@ def test_update_id_refs_cross_filter_handles_string_excluded(): fixed = update_id_refs(config, chart_ids, dataset_info) # Should not raise and should remap key assert "1" in fixed["metadata"]["chart_configuration"] + + +def test_update_id_refs_expanded_slices_with_missing_chart(): + """ + Test that missing charts in expanded_slices are gracefully skipped. + + When a chart is deleted from a workspace, dashboards may still contain + references to the deleted chart ID in expanded_slices metadata. This test + verifies that the import process skips missing chart references instead of + raising a KeyError. + """ + from superset.commands.dashboard.importers.v1.utils import update_id_refs + + config = { + "position": { + "CHART1": { + "id": "CHART1", + "meta": {"chartId": 101, "uuid": "uuid1"}, + "type": "CHART", + }, + }, + "metadata": { + "expanded_slices": { + "101": True, # This chart exists in the import + "102": False, # This chart was deleted and doesn't exist + "103": True, # Another deleted chart + }, + }, + } + chart_ids = {"uuid1": 1} # Only uuid1 exists in the import + dataset_info: dict[str, dict[str, Any]] = {} + + fixed = update_id_refs(config, chart_ids, dataset_info) + + # Should only include the existing chart, missing charts are skipped + assert fixed["metadata"]["expanded_slices"] == {"1": True} + # Should not raise KeyError for missing charts 102 and 103 + + +def test_update_id_refs_timed_refresh_immune_slices_with_missing_chart(): + """ + Test that missing charts in timed_refresh_immune_slices are gracefully skipped. + + When a chart is deleted from a workspace, dashboards may still contain + references to the deleted chart ID in timed_refresh_immune_slices metadata. + This test verifies that the import process skips missing chart references + instead of raising a KeyError. + """ + from superset.commands.dashboard.importers.v1.utils import update_id_refs + + config = { + "position": { + "CHART1": { + "id": "CHART1", + "meta": {"chartId": 101, "uuid": "uuid1"}, + "type": "CHART", + }, + "CHART2": { + "id": "CHART2", + "meta": {"chartId": 102, "uuid": "uuid2"}, + "type": "CHART", + }, + }, + "metadata": { + "timed_refresh_immune_slices": [ + 101, # This chart exists + 102, # This chart exists + 103, # This chart was deleted and doesn't exist + 104, # Another deleted chart + ], + }, + } + chart_ids = {"uuid1": 1, "uuid2": 2} # Only uuid1 and uuid2 exist + dataset_info: dict[str, dict[str, Any]] = {} + + fixed = update_id_refs(config, chart_ids, dataset_info) + + # Should only include existing charts, missing charts are skipped + assert fixed["metadata"]["timed_refresh_immune_slices"] == [1, 2] + # Should not raise KeyError for missing charts 103 and 104 + + +def test_update_id_refs_multiple_missing_chart_references(): + """ + Test that multiple metadata fields with missing charts are all handled gracefully. + + This comprehensive test verifies that all metadata fields properly skip + missing chart references during import. + """ + from superset.commands.dashboard.importers.v1.utils import update_id_refs + + config = { + "position": { + "CHART1": { + "id": "CHART1", + "meta": {"chartId": 101, "uuid": "uuid1"}, + "type": "CHART", + }, + }, + "metadata": { + "expanded_slices": { + "101": True, + "999": False, # Missing chart + }, + "timed_refresh_immune_slices": [101, 999], # 999 is missing + "filter_scopes": { + "101": {"region": {"immune": [999]}}, # 999 is missing + "999": {"region": {"immune": [101]}}, # Key 999 is missing + }, + "default_filters": '{"101": {"col": "value"}, "999": {"col": "other"}}', + "native_filter_configuration": [ + {"scope": {"excluded": [101, 999]}} # 999 is missing + ], + }, + } + chart_ids = {"uuid1": 1} # Only uuid1 exists + dataset_info: dict[str, dict[str, Any]] = {} + + fixed = update_id_refs(config, chart_ids, dataset_info) + + # All missing chart references should be gracefully skipped + assert fixed["metadata"]["expanded_slices"] == {"1": True} + assert fixed["metadata"]["timed_refresh_immune_slices"] == [1] + assert fixed["metadata"]["filter_scopes"] == {"1": {"region": {"immune": []}}} + assert fixed["metadata"]["default_filters"] == '{"1": {"col": "value"}}' + assert fixed["metadata"]["native_filter_configuration"] == [ + {"scope": {"excluded": [1]}} + ] From 7ba0378d01a6b3620b26531156bc857fcec4119d Mon Sep 17 00:00:00 2001 From: Ramiro Aquino Romero Date: Thu, 22 Jan 2026 11:43:59 -0400 Subject: [PATCH 2/2] perf: optimize dashboard cleanup query and add safety limits --- superset/commands/chart/delete.py | 99 +++++++++++++++++-------------- 1 file changed, 54 insertions(+), 45 deletions(-) diff --git a/superset/commands/chart/delete.py b/superset/commands/chart/delete.py index fd42736029b5..4afabc530107 100644 --- a/superset/commands/chart/delete.py +++ b/superset/commands/chart/delete.py @@ -19,6 +19,7 @@ from typing import Optional from flask_babel import lazy_gettext as _ +from sqlalchemy import func from superset import db, security_manager from superset.commands.base import BaseCommand @@ -85,15 +86,57 @@ def _cleanup_dashboard_metadata( # noqa: C901 This method cleans up those references to prevent issues during dashboard export/import. """ - # Find all dashboards that contain this chart - dashboards = ( - db.session.query(Dashboard) + + # First check how many dashboards contain this chart + dashboard_count = ( + db.session.query(func.count(Dashboard.id)) + .filter(Dashboard.slices.any(id=chart_id)) # type: ignore[attr-defined] + .scalar() + ) + + if dashboard_count == 0: + return + + # Log warning if cleaning up many dashboards + if dashboard_count > 100: + logger.warning( + "Chart %s is on %d dashboards. " + "Cleaning up metadata may take some time.", + chart_id, + dashboard_count, + ) + + # Use a reasonable limit to prevent memory issues with extremely popular charts + if dashboard_count > (safety_limit := 1000): + logger.error( + "Chart %s is on %d dashboards (exceeds safety limit of %d). " + "Skipping metadata cleanup. " + "Manual intervention may be required for export/import.", + chart_id, + dashboard_count, + safety_limit, + ) + return + + # Query only ID and json_metadata (not full Dashboard objects) + dashboards_to_update = ( + db.session.query(Dashboard.id, Dashboard.json_metadata) .filter(Dashboard.slices.any(id=chart_id)) # type: ignore[attr-defined] .all() ) - for dashboard in dashboards: - metadata = dashboard.params_dict + for dashboard_id, json_metadata_str in dashboards_to_update: + if not json_metadata_str: + continue + + try: + metadata = json.loads(json_metadata_str) + except (json.JSONDecodeError, TypeError): + logger.warning( + "Could not parse metadata for dashboard %s", dashboard_id + ) + continue + modified = False # Clean up expanded_slices @@ -102,23 +145,12 @@ def _cleanup_dashboard_metadata( # noqa: C901 if chart_id_str in metadata["expanded_slices"]: del metadata["expanded_slices"][chart_id_str] modified = True - logger.info( - "Removed chart %s from expanded_slices in dashboard %s", - chart_id, - dashboard.id, - ) # Clean up timed_refresh_immune_slices if "timed_refresh_immune_slices" in metadata: if chart_id in metadata["timed_refresh_immune_slices"]: metadata["timed_refresh_immune_slices"].remove(chart_id) modified = True - logger.info( - "Removed chart %s from timed_refresh_immune_slices " - "in dashboard %s", - chart_id, - dashboard.id, - ) # Clean up filter_scopes if "filter_scopes" in metadata: @@ -126,11 +158,6 @@ def _cleanup_dashboard_metadata( # noqa: C901 if chart_id_str in metadata["filter_scopes"]: del metadata["filter_scopes"][chart_id_str] modified = True - logger.info( - "Removed chart %s from filter_scopes in dashboard %s", - chart_id, - dashboard.id, - ) # Also clean from immune lists for filter_scope in metadata["filter_scopes"].values(): for attributes in filter_scope.values(): @@ -146,11 +173,6 @@ def _cleanup_dashboard_metadata( # noqa: C901 del default_filters[chart_id_str] metadata["default_filters"] = json.dumps(default_filters) modified = True - logger.info( - "Removed chart %s from default_filters in dashboard %s", - chart_id, - dashboard.id, - ) # Clean up native_filter_configuration scope exclusions if "native_filter_configuration" in metadata: @@ -159,12 +181,6 @@ def _cleanup_dashboard_metadata( # noqa: C901 if chart_id in scope_excluded: scope_excluded.remove(chart_id) modified = True - logger.info( - "Removed chart %s from native_filter_configuration " - "in dashboard %s", - chart_id, - dashboard.id, - ) # Clean up chart_configuration if "chart_configuration" in metadata: @@ -172,11 +188,6 @@ def _cleanup_dashboard_metadata( # noqa: C901 if chart_id_str in metadata["chart_configuration"]: del metadata["chart_configuration"][chart_id_str] modified = True - logger.info( - "Removed chart %s from chart_configuration in dashboard %s", - chart_id, - dashboard.id, - ) # Clean up global_chart_configuration scope exclusions if "global_chart_configuration" in metadata: @@ -188,17 +199,15 @@ def _cleanup_dashboard_metadata( # noqa: C901 if chart_id in scope_excluded: scope_excluded.remove(chart_id) modified = True - logger.info( - "Removed chart %s from global_chart_configuration " - "in dashboard %s", - chart_id, - dashboard.id, - ) if modified: - dashboard.json_metadata = json.dumps(metadata) + # Update only the json_metadata field + db.session.query(Dashboard).filter_by(id=dashboard_id).update( + {"json_metadata": json.dumps(metadata)}, + synchronize_session=False, + ) logger.info( "Cleaned up metadata for dashboard %s after deleting chart %s", - dashboard.id, + dashboard_id, chart_id, )