sadpandajoe commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4424272706
########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,131 @@ +# 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. +""" +Derive a ``query_context`` payload from a chart's stored ``params``. + +Charts imported via a v1 ZIP bundle persist with ``Slice.query_context = NULL`` +(the importer never synthesizes one), so the first +``GET /api/v1/chart/{pk}/data/`` returns HTTP 400 "Chart has no query context +saved" (Apache Superset #33615). This module builds a valid ``query_context`` +payload from the chart's viz ``params`` + its importer-resolved datasource, so +the imported row becomes queryable on first read — or classifies the chart +non-derivable and returns ``None`` (honest-fail; never a fabricated context). + +The mapping itself is **not** reimplemented here. Instead this delegates to the +canonical, server-side form-data → query-context builder +``superset.common.form_data_query_context.build_query_context_from_form_data`` +— the same one the dashboard Excel export and the MCP chart service already use +to rebuild a context for charts that predate persisted query contexts. Reusing +it (rather than maintaining a second, partial form-data-to-query mapping) means +the synthesized context matches what the chart's own ``buildQuery`` would emit +at read time, so an imported chart returns exactly the rows it defines — no +broadened result set, no dropped filter/ordering/role/post-processing, and no +viz-specific semantics (time axis, pivot axes, server pagination, contribution, +aggregation) silently lost (#33615 review). + +The datasource is taken from the importer-resolved id/type **only**, never from +``params`` (ADR-014 authz/RLS): the synthesized context names the same real +datasource the authz layer vets at read time. +""" + +from __future__ import annotations + +from typing import Any + +from superset.common.form_data_query_context import ( + build_query_context_from_form_data, +) + +# Datasource-less viz types render static/markdown content rather than a +# datasource-backed query; a NULL query_context is the *correct* state for them +# (FR-003), not a bug. Kept as a small, explicit, conservatively-expanded set. +# NOTE: `handlebars` is deliberately NOT here — it renders a template over query +# results and issues a real datasource-backed query, so it must stay on the +# derivable path. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "divider"}) + + +def build_query_context_config( + params: dict[str, Any] | None, + viz_type: str, + datasource_id: int | None, + datasource_type: str = "table", +) -> dict[str, Any] | None: + """ + Map a chart's ``params`` + resolved datasource to a ``query_context`` payload. + + :param params: the chart's viz form-data (a dict at ``import_chart`` time). + :param viz_type: the chart's viz type (classification + builder input). + :param datasource_id: the importer-resolved datasource id. **Datasource is + taken from this argument only, never from ``params`` (ADR-014 / RISK-T02)** + so the synthesized context names the same real datasource the authz layer + vets at read time. + :param datasource_type: the datasource type — ``"table"`` on import. + :returns: a ``query_context`` payload dict (the shape + ``ChartDataQueryContextSchema`` loads), or ``None`` when the chart is + non-derivable (FR-003): no datasource, a datasource-less viz type, or a + form-data shape the canonical builder produces no query for. Never + returns a fabricated/invalid context. + """ + params = params or {} + + # FR-003 non-derivable classification — return None, importer leaves NULL. + if not datasource_id or viz_type in _NON_DATASOURCE_VIZ: + return None + + # The datasource is the importer-resolved id/type only; overwrite any + # datasource carried in params and never trust it (ADR-014). The exported + # ``slice_id`` is dropped here so the synthesized context — which embeds the + # form data — cannot bind to an unrelated local chart that happens to share + # that id; the local slice is bound by the caller once the row's id is known. + form_data = { + **params, + "datasource": f"{datasource_id}__{datasource_type}", + } + form_data.pop("slice_id", None) + + context = build_query_context_from_form_data( Review Comment: This hands the form data to `build_query_context_from_form_data`, which drops constraints the chart's own `buildQuery` would apply, so the saved context can return more rows than the chart shows. For a `big_number_total` chart with an adhoc `TEMPORAL_RANGE` filter on `ds` for January and no top-level `time_range`, the builder's `query.setdefault("time_range", "No filter")` makes `QueryContextFactory._apply_filters` overwrite the filter's January comparator with `No filter`, so `/data/` counts every date. Charts carrying `extra_filters` or `extra_form_data.filters` (e.g. `country == US`) get `queries[0].filters=[]`, and a non-SIMPLE/non-SQL adhoc filter is skipped rather than rejected. Should synthesis return `None` (leave `NULL`) when the form data carries any of these, rather than persist a broader query? ########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,131 @@ +# 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. +""" +Derive a ``query_context`` payload from a chart's stored ``params``. + +Charts imported via a v1 ZIP bundle persist with ``Slice.query_context = NULL`` +(the importer never synthesizes one), so the first +``GET /api/v1/chart/{pk}/data/`` returns HTTP 400 "Chart has no query context +saved" (Apache Superset #33615). This module builds a valid ``query_context`` +payload from the chart's viz ``params`` + its importer-resolved datasource, so +the imported row becomes queryable on first read — or classifies the chart +non-derivable and returns ``None`` (honest-fail; never a fabricated context). + +The mapping itself is **not** reimplemented here. Instead this delegates to the +canonical, server-side form-data → query-context builder +``superset.common.form_data_query_context.build_query_context_from_form_data`` +— the same one the dashboard Excel export and the MCP chart service already use +to rebuild a context for charts that predate persisted query contexts. Reusing +it (rather than maintaining a second, partial form-data-to-query mapping) means +the synthesized context matches what the chart's own ``buildQuery`` would emit +at read time, so an imported chart returns exactly the rows it defines — no +broadened result set, no dropped filter/ordering/role/post-processing, and no +viz-specific semantics (time axis, pivot axes, server pagination, contribution, +aggregation) silently lost (#33615 review). + +The datasource is taken from the importer-resolved id/type **only**, never from +``params`` (ADR-014 authz/RLS): the synthesized context names the same real +datasource the authz layer vets at read time. +""" + +from __future__ import annotations + +from typing import Any + +from superset.common.form_data_query_context import ( + build_query_context_from_form_data, +) + +# Datasource-less viz types render static/markdown content rather than a +# datasource-backed query; a NULL query_context is the *correct* state for them +# (FR-003), not a bug. Kept as a small, explicit, conservatively-expanded set. +# NOTE: `handlebars` is deliberately NOT here — it renders a template over query +# results and issues a real datasource-backed query, so it must stay on the +# derivable path. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "divider"}) + + +def build_query_context_config( + params: dict[str, Any] | None, + viz_type: str, + datasource_id: int | None, + datasource_type: str = "table", +) -> dict[str, Any] | None: + """ + Map a chart's ``params`` + resolved datasource to a ``query_context`` payload. + + :param params: the chart's viz form-data (a dict at ``import_chart`` time). + :param viz_type: the chart's viz type (classification + builder input). + :param datasource_id: the importer-resolved datasource id. **Datasource is + taken from this argument only, never from ``params`` (ADR-014 / RISK-T02)** + so the synthesized context names the same real datasource the authz layer + vets at read time. + :param datasource_type: the datasource type — ``"table"`` on import. + :returns: a ``query_context`` payload dict (the shape + ``ChartDataQueryContextSchema`` loads), or ``None`` when the chart is + non-derivable (FR-003): no datasource, a datasource-less viz type, or a + form-data shape the canonical builder produces no query for. Never + returns a fabricated/invalid context. + """ + params = params or {} + + # FR-003 non-derivable classification — return None, importer leaves NULL. + if not datasource_id or viz_type in _NON_DATASOURCE_VIZ: + return None + + # The datasource is the importer-resolved id/type only; overwrite any + # datasource carried in params and never trust it (ADR-014). The exported + # ``slice_id`` is dropped here so the synthesized context — which embeds the + # form data — cannot bind to an unrelated local chart that happens to share + # that id; the local slice is bound by the caller once the row's id is known. + form_data = { + **params, + "datasource": f"{datasource_id}__{datasource_type}", + } + form_data.pop("slice_id", None) + + context = build_query_context_from_form_data( + form_data, + {"id": datasource_id, "type": datasource_type}, + viz_type, + ) + + # Honest-fail: if the canonical builder produced nothing to query, leave the + # chart non-derivable rather than persist an empty/invalid context. + if not context or not context.get("queries"): Review Comment: The `context.get("queries")` check only proves the builder emitted a query, not that it mapped this viz type. For `sankey_v2` with `source="origin"`, `target="destination"`, and metric `count` (or `graph_chart` with `source`/`target`), the builder has no alias for those controls, so it persists `columns=[]`: `/data/` returns one overall count instead of edge counts. That context is then saved as if it were valid, and the chart no longer prompts a re-save. Could this allowlist the viz types the builder has an explicit mapping for, and leave the rest `NULL`? -- 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]
