bito-code-review[bot] commented on code in PR #37042:
URL: https://github.com/apache/superset/pull/37042#discussion_r2730424988
##########
superset/commands/chart/delete.py:
##########
@@ -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
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Error Handling Gap</b></div>
<div id="fix">
The json.loads call for metadata["default_filters"] lacks error handling,
which could raise an exception if the value is malformed. It looks like this
should be wrapped in a try-except block similar to the main metadata parsing,
to log a warning and continue instead of failing the entire chart deletion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
# Clean up default_filters
if "default_filters" in metadata:
try:
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
except (json.JSONDecodeError, TypeError):
logger.warning("Could not parse default_filters for
dashboard %s", dashboard_id)
````
</div>
</details>
</div>
<small><i>Code Review Run #eacd8a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
