EnxDev commented on code in PR #44804:
URL: https://github.com/apache/superset/pull/44804#discussion_r4163596944
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1060,13 +1077,18 @@ def build_applied_dashboard_filters(
f"Chart {chart_id} is not on dashboard {dashboard_id}"
)
- metadata = json.loads(dashboard.json_metadata or "{}")
+ metadata = _safe_json_loads_dict(
+ dashboard.json_metadata, "json_metadata", dashboard_id
+ )
+ if metadata is None:
+ return []
Review Comment:
When json_metadata is unreadable, get_chart_info comes back looking exactly
like a dashboard with no filters, and the warning only lands in the server log.
An agent will then describe the chart as unfiltered when the dashboard may
well be filtering it. Would it be worth a `ctx.warning` in
`_attach_dashboard_filters` (or a flag on the result) so the caller knows the
filters couldn't be resolved?
##########
tests/unit_tests/mcp_service/chart/test_chart_helpers.py:
##########
@@ -1438,3 +1439,59 @@ def test_waterfall_query_preserves_temporal_binding(
else:
assert "granularity" not in query
assert query.get("extras", {}).get("time_grain_sqla") == time_grain
+
+
+def _setup_dashboard_mock(mock_db, json_metadata, position_json="{}",
chart_id=1):
+ """Wire a mock db to return a Dashboard with the given fields."""
+ dashboard = MagicMock()
+ dashboard.json_metadata = json_metadata
+ dashboard.position_json = position_json
+ slc = MagicMock()
+ slc.id = chart_id
+ dashboard.slices = [slc]
+ query_chain = mock_db.session.query.return_value
+ query_chain.filter_by.return_value.one_or_none.return_value = dashboard
+
+
+@patch("superset.security_manager", MagicMock())
+@patch("superset.db")
+def test_build_applied_dashboard_filters_malformed_json_metadata(
+ mock_db,
+):
+ _setup_dashboard_mock(mock_db, json_metadata="not valid json {{{")
+ result = build_applied_dashboard_filters(dashboard_id=1, chart_id=1)
+ assert result == []
+
+
+@patch("superset.security_manager", MagicMock())
+@patch("superset.db")
+def test_build_applied_dashboard_filters_json_metadata_is_array(
+ mock_db,
+):
+ _setup_dashboard_mock(mock_db, json_metadata="[1, 2, 3]")
+ result = build_applied_dashboard_filters(dashboard_id=1, chart_id=1)
+ assert result == []
+
+
+@patch("superset.security_manager", MagicMock())
+@patch("superset.db")
+def test_build_applied_dashboard_filters_json_metadata_is_string(
+ mock_db,
+):
+ _setup_dashboard_mock(mock_db, json_metadata='"just a string"')
+ result = build_applied_dashboard_filters(dashboard_id=1, chart_id=1)
+ assert result == []
+
+
+@patch("superset.security_manager", MagicMock())
+@patch("superset.db")
+def test_build_applied_dashboard_filters_malformed_position_json(
+ mock_db,
+):
+ _setup_dashboard_mock(
+ mock_db,
+ json_metadata='{"native_filter_configuration": []}',
Review Comment:
With an empty `native_filter_configuration` this returns `[]` whether we
degrade or bail, so it only proves we don't raise.
Could we give it a filter scoped to `ROOT_ID` and assert it still comes
back? That's the case the `or {}` fallback exists for.
--
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]
