sadpandajoe commented on code in PR #44577:
URL: https://github.com/apache/superset/pull/44577#discussion_r4132190961
##########
superset/dashboards/filter_scope.py:
##########
@@ -57,8 +60,15 @@
def build_chart_layout_items(position_data: dict[str, Any]) ->
ChartLayoutItems:
- """Map each chart id in the layout to the layout items that render it."""
+ """Map each chart id in the layout to the layout items that render it.
+
+ A layout that is not a mapping (``position_json`` holding a JSON string or
+ array) places no charts, so every derived scope comes out empty.
+ """
chart_layout_items: ChartLayoutItems = {}
+ if not isinstance(position_data, dict):
Review Comment:
With a malformed `position_json`, this guard now lets `GET
/api/v1/dashboard/{id}` return 200 instead of 500, but
`_derive_cross_filter_scopes` drops every `chart_configuration` entry outright
when the layout can't be parsed, not just their `chartsInScope` cache. That
derived body is exactly what a dashboard properties save round-trips into
`dashboard.json_metadata` (`DashboardDAO.set_dash_metadata` merges any
client-submitted `chart_configuration` straight through for keys outside its
special-cased set). So opening and saving a dashboard's properties while the
layout is broken can now permanently discard its cross-filter chart
configuration, where the previous 500 blocked that path outright. Should
`chart_configuration` be preserved rather than dropped when the layout can't be
parsed?
##########
superset/dashboards/filter_scope.py:
##########
@@ -57,8 +60,15 @@
def build_chart_layout_items(position_data: dict[str, Any]) ->
ChartLayoutItems:
- """Map each chart id in the layout to the layout items that render it."""
+ """Map each chart id in the layout to the layout items that render it.
+
+ A layout that is not a mapping (``position_json`` holding a JSON string or
+ array) places no charts, so every derived scope comes out empty.
+ """
chart_layout_items: ChartLayoutItems = {}
+ if not isinstance(position_data, dict):
+ logger.warning("Dashboard layout is not a mapping; no charts are in
scope")
+ return chart_layout_items
Review Comment:
Agreed—`get_chart_ids_in_scope`'s `selectedLayers` branch resolves targeted
chart ids straight from `scope.selectedLayers` and the dashboard's chart list,
without consulting `chart_layout_items`, so a native filter scoped that way
still reports a chart as in-scope even when the layout is completely
unparseable. Should that branch also require the chart to actually be present
in `chart_layout_items` before targeting 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]
