gabotorresruiz commented on code in PR #44804:
URL: https://github.com/apache/superset/pull/44804#discussion_r4392876872
##########
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:
@EnxDev confirmed, still live at `631cfec`. Driving `get_chart_info` with a
`dashboard_id` over an MCP client, the shapes past the `try` come back as a
tool error with no `ctx.warning` on any channel:
- `"extraFormData": "x"` gives `Error calling tool 'get_chart_info': 'str'
object has no attribute 'get'`
- `"extraFormData": [1]` gives the same for `list`
- a non-string `id`, `name` or `filterType` gives the raw
`AppliedDashboardFilter` pydantic `ValidationError` text
The part I did not expect is that `validate_json_metadata` accepts all five.
`native_filter_configuration` is `fields.List(fields.Dict())` in
`superset/dashboards/schemas.py:203`, so it checks that each entry is a mapping
and never the inner shape. A plain `PUT /api/v1/dashboard/<id>` carrying
`{"native_filter_configuration": [{"id": "NF-1", "defaultDataMask":
{"filterState": {"value": ["US"]}, "extraFormData": "x"}}]}` is accepted and
then takes the tool down.
Your suggestion closes all five. Pydantic's `ValidationError` subclasses
`ValueError`, so moving the `append` inside the `try` is enough:
```python
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)
operator, value = _resolve_filter_operator_and_value(
extra_form_data, column
)
applied.append(
AppliedDashboardFilter(
id=flt.get("id"),
name=flt.get("name"),
filter_type=flt.get("filterType"),
column=column,
operator=operator,
value=value,
status=status.value,
)
)
except (AttributeError, TypeError, ValueError):
logger.warning(
"Ignoring malformed native filter %s for dashboard %s.",
flt.get("id"),
dashboard_id,
exc_info=True,
)
return None
```
All five then land on the same `None` plus `ctx.warning` as the nulls, and
nothing else I probed changes. It costs no per-filter granularity either, since
this branch already bails on the whole dashboard rather than skipping one
filter. @eschutho the test that would pin it is a
`test_build_applied_dashboard_filters_malformed_extra_form_data` parametrized
over `"x"` and `[1]`, plus one case with a non-string `id`.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1365,15 +1366,66 @@ def _resolve_filter_operator_and_value(
return None, None
+def _safe_json_loads_dict(
+ raw: str | None, field_name: str, dashboard_id: int
+) -> dict[str, Any] | None:
+ """Parse a JSON string expected to be an object.
+
+ Returns *None* and logs a warning when the value is not valid JSON or does
+ not decode to an object.
+ """
+ from superset.utils import json
+
+ try:
+ value = json.loads(raw or "{}")
+ except (TypeError, ValueError):
+ value = None
+ if isinstance(value, dict):
+ return value
+ logger.warning("Ignoring malformed %s for dashboard %s.", field_name,
dashboard_id)
+ return None
+
+
+def _load_native_filters_and_layout(
+ dashboard: Dashboard, dashboard_id: int
+) -> tuple[list[Any], dict[str, Any]] | None:
+ """Parse a dashboard's native filter configuration and layout.
+
+ Returns *None* and logs a warning when either cannot be read.
+ """
+ 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):
+ 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
+ )
Review Comment:
@EnxDev confirmed, still live at `631cfec`. A dashboard with
`native_filter_configuration: []` and an unreadable `position_json` comes back
with the warning saying the filters "could not be resolved" when there are none
to resolve, so an agent gets told something went wrong on a perfectly healthy
dashboard. Your early return fixes it, and I could not find a path where the
layout is needed once the filter list is empty.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1395,15 +1396,66 @@ def _resolve_filter_operator_and_value(
return None, None
+def _safe_json_loads_dict(
Review Comment:
Not this PR, but since the helper now exists: `get_dashboard_filter_context`
at `superset/charts/data/dashboard_filter_context.py:308` does the same two
unguarded `json.loads` on `json_metadata` and `position_json`, and it sits on
the chart data path rather than the metadata one. Worth pointing it at
`_safe_json_loads_dict` as a follow-up?
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1433,25 +1484,30 @@ 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
Review Comment:
Small asymmetry, and possibly moot. A non-dict entry is skipped silently
here, so `[{valid filter}, "oops"]` returns a one element list with no
`ctx.warning` and nothing in the log, while every other malformed shape now
returns `None` and tells the caller. I could not find a write path that lands
one though: `validate_json_metadata` rejects it with `Not a valid mapping
type`, and the v1 import calls `.get` on each entry in
`find_native_filter_datasets` before anything is persisted. Worth a `return
None` for consistency, or happy to leave it?
--
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]
