EnxDev commented on code in PR #44804:
URL: https://github.com/apache/superset/pull/44804#discussion_r4251833271
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1334,13 +1355,24 @@ def build_applied_dashboard_filters(
f"Chart {chart_id} is not on dashboard {dashboard_id}"
)
- metadata = json.loads(dashboard.json_metadata or "{}")
- native_filter_config = metadata.get("native_filter_configuration", [])
+ metadata = _safe_json_loads_dict(
+ dashboard.json_metadata, "json_metadata", dashboard_id
+ )
+ if metadata is None:
+ return None
+ native_filter_config = metadata.get("native_filter_configuration")
+ if native_filter_config is None:
+ native_filter_config = []
if not isinstance(native_filter_config, list):
- return []
- position_json = json.loads(dashboard.position_json or "{}")
- if not isinstance(position_json, dict):
- position_json = {}
+ logger.warning(
+ "Ignoring malformed native_filter_configuration for dashboard %s.",
+ dashboard_id,
+ )
+ return None
+ position_json = (
+ _safe_json_loads_dict(dashboard.position_json, "position_json",
dashboard_id)
+ or {}
+ )
Review Comment:
With position_json unreadable this falls back to `{}`, so a filter scoped to
`TAB-1` comes back as `[]` with no warning to the caller.
Would returning None here be simpler? I did ask for the test that pins the
fallback, so push back if you'd rather keep it.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1334,13 +1355,24 @@ def build_applied_dashboard_filters(
f"Chart {chart_id} is not on dashboard {dashboard_id}"
)
- metadata = json.loads(dashboard.json_metadata or "{}")
- native_filter_config = metadata.get("native_filter_configuration", [])
+ metadata = _safe_json_loads_dict(
+ dashboard.json_metadata, "json_metadata", dashboard_id
+ )
+ if metadata is None:
+ return None
+ native_filter_config = metadata.get("native_filter_configuration")
+ if native_filter_config is None:
+ native_filter_config = []
if not isinstance(native_filter_config, list):
- return []
- position_json = json.loads(dashboard.position_json or "{}")
- if not isinstance(position_json, dict):
- position_json = {}
+ logger.warning(
+ "Ignoring malformed native_filter_configuration for dashboard %s.",
+ dashboard_id,
+ )
+ return None
+ position_json = (
+ _safe_json_loads_dict(dashboard.position_json, "position_json",
dashboard_id)
+ or {}
+ )
applied: list[AppliedDashboardFilter] = []
for flt in native_filter_config:
Review Comment:
A filter entry with a null inside still raises out of this loop: `"scope":
null`, `"defaultDataMask": null` and `"targets": [null]` each come out of
get_chart_info as a raw AttributeError, since the helpers in
`dashboard_filter_context.py` use `.get(key, {})`.
Could we catch `(AttributeError, TypeError)` around the loop body and return
None so it lands on the same warning? Fine as a follow-up if you'd rather keep
this one tight.
--
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]
