fix(dashboard-import): dashboard import fails with keyerror when referencing deleted charts in expanded_slices metadata

superset/commands/chart/delete.py

@@ -19,8 +19,9 @@
 from typing import Optional
 
 from flask_babel import lazy_gettext as _
+from sqlalchemy import func
 
-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 +32,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 +49,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 +74,140 @@ 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.
+        """
+
+        # 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_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
+            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
+
+            # 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
+
+            # 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
+                # 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
+
+            # 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
+
+            # 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
+
+            # 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
+
+            if modified:
+                # 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,
+                    chart_id,
+                )

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:

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]}}
+    ]