EnxDev commented on code in PR #44804:
URL: https://github.com/apache/superset/pull/44804#discussion_r4263476072
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1403,25 +1454,29 @@ 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", [])
- if not isinstance(native_filter_config, list):
- return []
- position_json = json.loads(dashboard.position_json or "{}")
- if not isinstance(position_json, dict):
- position_json = {}
+ parsed = _load_native_filters_and_layout(dashboard, dashboard_id)
+ if parsed is None:
+ return None
+ native_filter_config, position_json = parsed
applied: list[AppliedDashboardFilter] = []
for flt in native_filter_config:
if not isinstance(flt, dict):
continue
if flt.get("type", "") == "DIVIDER":
continue
- if not _is_filter_in_scope_for_chart(flt, chart_id, position_json):
- continue
-
- extra_form_data, status = _extract_filter_extra_form_data(flt)
- column = _get_filter_target_column(flt)
+ try:
+ if not _is_filter_in_scope_for_chart(flt, chart_id, position_json):
+ continue
+ extra_form_data, status = _extract_filter_extra_form_data(flt)
+ column = _get_filter_target_column(flt)
+ except (AttributeError, TypeError):
+ logger.warning(
+ "Ignoring malformed native filter %s for dashboard %s.",
+ flt.get("id"),
+ dashboard_id,
+ )
+ return None
operator, value = _resolve_filter_operator_and_value(extra_form_data,
column)
Review Comment:
The try stops one line short. A static default whose `extraFormData` isn't
an object (`"x"` or `[1]`) raises AttributeError from this call, and a
non-string `name`, `id` or `filterType` raises a pydantic ValidationError from
the constructor below.
Could the try run through the `append` and catch `ValueError` as well? Not
blocking, these shapes are rarer than the nulls.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1403,25 +1454,29 @@ 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", [])
- if not isinstance(native_filter_config, list):
- return []
- position_json = json.loads(dashboard.position_json or "{}")
- if not isinstance(position_json, dict):
- position_json = {}
+ parsed = _load_native_filters_and_layout(dashboard, dashboard_id)
+ if parsed is None:
+ return None
+ native_filter_config, position_json = parsed
applied: list[AppliedDashboardFilter] = []
for flt in native_filter_config:
if not isinstance(flt, dict):
continue
if flt.get("type", "") == "DIVIDER":
continue
- if not _is_filter_in_scope_for_chart(flt, chart_id, position_json):
- continue
-
- extra_form_data, status = _extract_filter_extra_form_data(flt)
- column = _get_filter_target_column(flt)
+ try:
+ if not _is_filter_in_scope_for_chart(flt, chart_id, position_json):
+ continue
+ extra_form_data, status = _extract_filter_extra_form_data(flt)
+ column = _get_filter_target_column(flt)
+ except (AttributeError, TypeError):
+ logger.warning(
+ "Ignoring malformed native filter %s for dashboard %s.",
+ flt.get("id"),
+ dashboard_id,
+ )
Review Comment:
Nit, take it or leave it. This catch spans three helpers, so a real bug in
one of them would also be logged as a malformed filter, and the line doesn't
say which field tripped.
Keeping the traceback makes both cases easy to tell apart:
```suggestion
logger.warning(
"Ignoring malformed native filter %s for dashboard %s.",
flt.get("id"),
dashboard_id,
exc_info=True,
)
```
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -286,7 +286,13 @@ async def _attach_dashboard_filters(
await ctx.warning("Dashboard not accessible: %s" % (str(exc),))
return ChartError(error=str(exc),
error_type="DashboardNotAccessible")
- if dashboard_filters:
+ if dashboard_filters is None:
+ await ctx.warning(
+ "Dashboard %s filter configuration or layout is malformed; "
+ "dashboard filters applied to this chart could not be
resolved."
+ % (dashboard_id,)
+ )
+ elif dashboard_filters:
Review Comment:
No test reaches this branch. The only one that calls
`_attach_dashboard_filters` has the helper return None, which is what codecov
is flagging on this file.
Could we add one that returns a filter and asserts it lands on
`result.filters.dashboard_filters`?
--
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]
