Copilot commented on code in PR #34418:
URL: https://github.com/apache/superset/pull/34418#discussion_r2260761703
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,7 +139,47 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+ fixed = update_cross_filter_scoping(fixed, id_map)
+ return fixed
+
+def update_cross_filter_scoping(
+ config: dict[str, Any], id_map: dict[int, int]
+) -> dict[str, Any]:
+ # fix cross filter references
+ fixed = config.copy()
+
+ cross_filter_global_config = fixed.get("metadata", {}).get(
+ "global_chart_configuration", {}
+ )
+ scope_excluded = cross_filter_global_config.get("scope",
{}).get("excluded", [])
+ if scope_excluded:
+ cross_filter_global_config["scope"]["excluded"] = [
+ id_map[old_id] for old_id in scope_excluded if old_id in id_map
+ ]
+
+ if "chart_configuration" in (metadata := fixed.get("metadata", {})):
Review Comment:
[nitpick] The walrus operator assignment creates a subtle dependency where
`metadata` is used later in the function. Consider extracting `metadata`
assignment to a separate line for better readability: `metadata =
fixed.get("metadata", {})` followed by `if "chart_configuration" in metadata:`
```suggestion
metadata = fixed.get("metadata", {})
if "chart_configuration" in metadata:
```
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,7 +139,47 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+ fixed = update_cross_filter_scoping(fixed, id_map)
+ return fixed
+
+def update_cross_filter_scoping(
+ config: dict[str, Any], id_map: dict[int, int]
+) -> dict[str, Any]:
+ # fix cross filter references
+ fixed = config.copy()
+
+ cross_filter_global_config = fixed.get("metadata", {}).get(
+ "global_chart_configuration", {}
+ )
+ scope_excluded = cross_filter_global_config.get("scope",
{}).get("excluded", [])
+ if scope_excluded:
+ cross_filter_global_config["scope"]["excluded"] = [
+ id_map[old_id] for old_id in scope_excluded if old_id in id_map
+ ]
+
+ if "chart_configuration" in (metadata := fixed.get("metadata", {})):
+ # in cross_filter_scopes the key is the chart ID as a string; we need
to update
+ # them to be the new ID as a string:
+ metadata["chart_configuration"] = {
+ str(id_map[int(old_id)]): columns
+ for old_id, columns in metadata["chart_configuration"].items()
+ if int(old_id) in id_map
+ }
+ # now update scope excluded to use new IDs:
+ for excluded_charts in metadata["chart_configuration"].values():
+ if "id" in excluded_charts and excluded_charts["id"] in id_map:
+ excluded_charts["id"] = id_map[excluded_charts["id"]]
+ scope = excluded_charts.get("crossFilters", {}).get("scope", {})
+
+ if not isinstance(scope, dict):
+ continue
+
+ excluded_scope = scope.get("excluded", [])
+ if excluded_scope:
+ excluded_charts["crossFilters"]["scope"]["excluded"] = [
Review Comment:
Potential KeyError if `crossFilters` or `scope` keys don't exist in the
dictionary. Consider using `.get()` method or add existence checks before
accessing nested dictionary keys.
```suggestion
# Ensure the nested dictionaries exist before assignment
cross_filters = excluded_charts.setdefault("crossFilters",
{})
scope_dict = cross_filters.setdefault("scope", {})
scope_dict["excluded"] = [
```
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,7 +139,47 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+ fixed = update_cross_filter_scoping(fixed, id_map)
+ return fixed
+
+def update_cross_filter_scoping(
+ config: dict[str, Any], id_map: dict[int, int]
+) -> dict[str, Any]:
+ # fix cross filter references
+ fixed = config.copy()
+
+ cross_filter_global_config = fixed.get("metadata", {}).get(
+ "global_chart_configuration", {}
+ )
+ scope_excluded = cross_filter_global_config.get("scope",
{}).get("excluded", [])
+ if scope_excluded:
+ cross_filter_global_config["scope"]["excluded"] = [
+ id_map[old_id] for old_id in scope_excluded if old_id in id_map
+ ]
+
+ if "chart_configuration" in (metadata := fixed.get("metadata", {})):
+ # in cross_filter_scopes the key is the chart ID as a string; we need
to update
+ # them to be the new ID as a string:
+ metadata["chart_configuration"] = {
+ str(id_map[int(old_id)]): columns
+ for old_id, columns in metadata["chart_configuration"].items()
+ if int(old_id) in id_map
+ }
+ # now update scope excluded to use new IDs:
+ for excluded_charts in metadata["chart_configuration"].values():
+ if "id" in excluded_charts and excluded_charts["id"] in id_map:
+ excluded_charts["id"] = id_map[excluded_charts["id"]]
+ scope = excluded_charts.get("crossFilters", {}).get("scope", {})
+
+ if not isinstance(scope, dict):
+ continue
+
+ excluded_scope = scope.get("excluded", [])
+ if excluded_scope:
+ excluded_charts["crossFilters"]["scope"]["excluded"] = [
Review Comment:
The variable name `excluded_charts` is misleading as it represents
configuration for a single chart, not multiple charts. This could cause
confusion about the data structure being processed.
```suggestion
for chart_config in metadata["chart_configuration"].values():
if "id" in chart_config and chart_config["id"] in id_map:
chart_config["id"] = id_map[chart_config["id"]]
scope = chart_config.get("crossFilters", {}).get("scope", {})
if not isinstance(scope, dict):
continue
excluded_scope = scope.get("excluded", [])
if excluded_scope:
chart_config["crossFilters"]["scope"]["excluded"] = [
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
