gabotorresruiz commented on code in PR #43176: URL: https://github.com/apache/superset/pull/43176#discussion_r4020501045
########## superset/charts/data/form_data.py: ########## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from typing import Any, TYPE_CHECKING + +from flask import g + +if TYPE_CHECKING: + from superset.common.query_context import QueryContext + from superset.common.query_object import QueryObject + + +def set_form_data(form_data: dict[str, Any]) -> None: + """Expose chart data request fields to Jinja template macros.""" + g.form_data = form_data + + +def _serialize_query( + query: QueryObject, + form_data: dict[str, Any], +) -> dict[str, Any]: + """Serialize query fields consumed by the Jinja form-data fallback.""" + query_data = dict(query.to_dict()) + query_data["filters"] = query.filter + if query.time_range is not None: + query_data["time_range"] = query.time_range + if url_params := form_data.get("url_params"): + query_data["url_params"] = url_params + return query_data + + +def set_query_context_form_data( + query_context: QueryContext, + datasource_id: int | str, + datasource_type: str, +) -> None: + """Expose a programmatically-created query like a chart data API request.""" + form_data = query_context.form_data or {} + set_form_data( + { + "datasource": {"id": datasource_id, "type": datasource_type}, + "queries": [ + _serialize_query(query, form_data) for query in query_context.queries + ], + "form_data": form_data, + } Review Comment: Reopening this one briefly, @rusackas. On the dataset path I do not think it is covered: by the time `_serialize_query` runs, `_apply_granularity` has already deleted the `TEMPORAL_RANGE` entry from `QueryObject.filter` (it fires whenever `granularity` names the filter's column, which `query_dataset` always does when `time_range` is set), so there is nothing left for `convert_legacy_filters_into_adhoc()` to convert. I verified it on the head: after the real factory, `queries[0].filter` is `[]` and `get_time_filter()` returns `No filter`. Details and a suggested fix in my comment on `query_dataset.py`. Are the chart tool paths safe only because their `form_data` carries a top level `time_range`, or is there another mechanism I am missing for the column specific case there? ########## superset/mcp_service/dataset/tool/query_dataset.py: ########## @@ -298,8 +298,11 @@ async def query_dataset( # noqa: C901 # Step 4: Build query dict # ------------------------------------------------------------------ await ctx.report_progress(3, 5, "Building query") - # time_range is not passed through: the TEMPORAL_RANGE clause is already - # in query_filters above, alongside the effective_filters bookkeeping. + # TEMPORAL_RANGE is already in query_filters. Do not also set + # QueryObject.time_range: relative ranges must keep from_dttm/to_dttm + # in the cache key so rollovers re-execute. Jinja get_time_filter() + # still sees the range via set_query_context_form_data hoisting it + # from the TEMPORAL_RANGE filter. Review Comment: First, the cache side of 5ca36d0 checks out, and I verified it in both directions on this branch: two explicit ranges produce different `QueryObject.cache_key()` values through the real `QueryContextFactory`, identical ranges share one key, and `test_query_dataset_reexecutes_across_rollover` fails at 3d17c49 and passes here. So the rollover regression from putting `time_range` on the `QueryObject` was real and this commit fixes it. Nice catch. This comment block worries me a bit though: the claim that Jinja `get_time_filter()` still sees the range via the `set_query_context_form_data` hoist does not hold on this path. `QueryContextFactory.create()` runs `_apply_granularity`, and because `query_dataset` always sets `granularity` to the same column carrying the `TEMPORAL_RANGE` filter, that filter is deleted from `QueryObject.filter` before `execute_tabular_query` reaches `set_query_context_form_data`. I verified it on this branch: after the real factory runs, `queries[0].filter` is `[]`, `time_range` is `None`, and `ExtraCache().get_time_filter().time_range` renders `No filter` instead of the requested `Last week`. The same sequence at 3d17c49 rendered `Last week`. The new tests pass because they never run `_process_query_object`: `test_query_context_form_data_hoists_time_range_from_temporal_filter` builds the `QueryObject` by hand with the filter still attached, and `test_query_dataset_exposes_filters_to_jinja_macros` patches `QueryContextFactory.create` with `_query_context_from_factory_kwargs`, which skips `_apply_granularity` entirely. Not a blocker on its own, master never exposed the range to Jinja on this path either, so nothing regresses against master. But the comments here and in `build_query_dict` document a mechanism that does not fire, and Jinja alignment is the point of this PR. Since a rebase is needed anyway, could we make the hoist real in the same pass? `QueryContext` keeps the raw pre-processing query dicts in `cache_values["queries"]`, so `set_query_context_form_data` can fall back to those when the processed query lost the filter: ```python raw_queries = (getattr(query_context, "cache_values", None) or {}).get("queries") or [] ``` and hoist via `_time_range_from_filters(raw.get("filters"))` per query. Then turn `test_query_dataset_exposes_filters_to_jinja_macros` into a real factory test (patch `QueryContextFactory._convert_to_model` instead of `create`) so the assertion actually exercises `_apply_granularity`; that variant fails on the current head, which is exactly the coverage we are missing. Happy to dig in with you if it helps. -- 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]
