reynoldmorel commented on code in PR #37042:
URL: https://github.com/apache/superset/pull/37042#discussion_r2700557439
##########
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
+ ],
+ },
+ }
Review Comment:
You could add a general function that builds a new config for you, so you
can modify as you wish for each test
##########
superset/commands/chart/delete.py:
##########
@@ -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()
+ )
Review Comment:
[Personal concern, since I don't have context]
How many records are we expecting to load here? I think we should
investigate that, since it could break the server, also not sure if we should
be safe by adding a limit.
Make sure we don't have a status column that marks records as deleted,
because that could affect the number of records returned here.
##########
superset/commands/chart/delete.py:
##########
@@ -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)
Review Comment:
[Personal concern, since I don't have context]
It doesn't feel really safe to do this since the I don't know how many
models we could get here and also `_cleanup_dashboard_metadata` could be time
consuming. I would suggest to investigate if this won't cause any performance
issues, and if not, I would write a comment denoting why this solution works
and what scenario will cover, so if at some point in the future it fails, devs
would know what to consider to fix it.
##########
superset/commands/chart/delete.py:
##########
@@ -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,
+ )
Review Comment:
Looks like this pattern is repeated a lot, can't we just put it behind a
function and do something like:
```
(modified, updated_expanded_slices_metadata) =
delete_metadata_by_chart("expanded_slices", metadata, chart_id) or modified
(modified, updated_timed_refresh_immune_slices_metadata) =
delete_metadata_by_chart("timed_refresh_immune_slices",
updated_expanded_slices_metadata, chart_id) or modified
```
For different patterns you can write a condition to check whether the
property needs a unique pattern or not to get its value where `chart_id` might
be located
Also I don't think you should be updating metadata and dashboard properties
by object reference, this could lead to unpredictable behavior if this function
keeps growing and could become harder to maintain.
I would return a new updated metadata reference in general from
`delete_metadata_by_chart`, you could use `copy.deepcopy`
--
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]
