mikebridge commented on code in PR #45294:
URL: https://github.com/apache/superset/pull/45294#discussion_r4317257408
##########
superset/common/query_context_factory.py:
##########
@@ -434,8 +435,23 @@ def _apply_granularity( # noqa: C901
if filter["col"] != filter_to_remove
]
- def _apply_filters(self, query_object: QueryObject) -> None:
+ def _apply_filters(
+ self, query_object: QueryObject, form_data: dict[str, Any] | None =
None
+ ) -> None:
+ """Apply the time range, retaining the source of dashboard
overrides."""
+ extra_form_data: object = (form_data or {}).get("extra_form_data")
+ dashboard_time_override: bool = (
+ isinstance(extra_form_data, dict)
+ and "time_range" in extra_form_data
+ and extra_form_data["time_range"] == query_object.time_range
Review Comment:
I traced the producers. The normal dashboard path keeps them equal, but a
saved-context replay can retain an older top-level
`extra_form_data.time_range`. That replay case is already handled, so I think
we should keep the equality check.
- In the browser, the [time filter writes the selected
range](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset-frontend/src/filters/components/Time/TimeFilterPlugin.tsx#L91).
Native and cross-filter masks are merged into one `extra_form_data` object,
with later scalar overrides winning
([merge](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset-frontend/src/dashboard/components/nativeFilters/utils.ts#L123)).
[`buildQueryObject`](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset-frontend/packages/superset-ui-core/src/query/buildQueryObject.ts#L144)
applies that object's range last, over the chart/form-data range.
`QueryObjectFactory` computes bounds separately and preserves the supplied
`time_range` string.
- A concrete replay example is a saved context whose top-level extra range
is `Last year`, followed by a workbook export with an active `Last week`
filter. The [export reuses the saved form data and applies the active filter to
its
queries](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset/dashboards/excel_export/workbook.py#L205).
Those two values can then differ. However,
[`apply_dashboard_filter_context`](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset/charts/data/dashboard_filter_context.py#L401)
marks saved unbounded temporal filters `isExtra` at the point where it applies
the range. The [MCP saved-context override
helper](https://github.com/apache/superset/blob/1ec256c2aa9e848e2fbb8d19a074c5ed07b66d71/superset/mcp_service/chart/chart_helpers.py#L420)
does the same. `_apply_filters` preserves that existing mark even when the
equality check fails.
I exercised both replay helpers with an old top-level range and a saved `No
filter` on a missing column: the new query range was applied, `isExtra`
survived the factory, and the semantic filter-column validation accepted it. I
also checked the report payload builder; it doesn't introduce a competing
time-range override. I haven't found a built-in producer that creates the
mismatched, **unmarked** case your test refuses. Keeping that case conservative
avoids treating an unrelated chart-owned range as a dashboard override.
--
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]
