sadpandajoe commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4129093426
########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,224 @@ +# 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 function is **pure** (no DB / network / analytical-DB access). Its output, +when passed to ``QueryContextFactory.create(**payload)``, constructs a valid +``QueryContext``. See ADR-013 (synthesize-at-import) and ADR-014 +(datasource authz/RLS preservation). +""" + +from __future__ import annotations + +from typing import Any + +from superset.utils.core import split_adhoc_filters_into_base_filters + +# 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 ships a real buildQuery (its generated registry entry and golden +# fixture build a datasource-backed query), so it must stay on the derivable path. +# Classifying it datasource-less left imported handlebars charts with a NULL +# context that 400s on the data endpoint whenever the V8 bundle is unavailable. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "divider"}) + + +def _is_mappable_adhoc_filter(adhoc_filter: dict[str, Any]) -> bool: + """ + Whether ``split_adhoc_filters_into_base_filters`` will actually carry this + adhoc filter into the synthesized context. + + The shared splitter preserves only SIMPLE ``WHERE`` clauses (a subject + + operator) and SQL ``WHERE`` / ``HAVING`` clauses (a non-empty expression). + Anything else — a SIMPLE ``HAVING``, an unknown ``expressionType``, or a + SIMPLE filter missing its subject/operator — is silently discarded, which + would broaden the imported chart's result set. Such filters are treated as + unmappable so the caller can fail closed instead of querying more rows than + the chart defines. + """ + expression_type = adhoc_filter.get("expressionType") + clause = adhoc_filter.get("clause") + if expression_type == "SIMPLE": + return ( + clause == "WHERE" + and bool(adhoc_filter.get("subject")) + and bool(adhoc_filter.get("operator")) + ) + if expression_type == "SQL": + return clause in ("WHERE", "HAVING") and bool(adhoc_filter.get("sqlExpression")) + return False + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], str, str] | None: + """ + Translate viz ``adhoc_filters`` into base filters via the shared splitter. + + Delegates to ``split_adhoc_filters_into_base_filters`` — the same helper the + read path uses — so SQL predicates are composed identically rather than by a + bare ``" AND ".join``: each clause is wrapped in parentheses (preserving + ``OR`` precedence) and a trailing ``--`` line comment is prevented from + swallowing predicates joined after it. + + Fails closed (#33615 review): if any adhoc filter would be silently dropped + by the splitter, the synthesized context would query a broader row set than + the chart defines. Returning ``None`` here makes the caller classify the + chart non-derivable and leave the ``query_context`` NULL — an honest 400 + until backfilled is safer than a wrong, broadened result. Non-dict junk is + still tolerated (dropped, never raised) so a stray serialization artifact + does not by itself void an otherwise sound chart (RISK-T05). + + Returns ``(filters, where, having)`` where ``where``/``having`` are the + parenthesized, comment-safe SQL strings ready for ``extras``, or ``None`` + when a real filter could not be preserved. + """ + # Drop non-dict junk up front (RISK-T05): the shared splitter calls + # ``.get`` on each entry and would raise on a stray non-dict item. + sanitized = [f for f in (adhoc_filters or []) if isinstance(f, dict)] + if any(not _is_mappable_adhoc_filter(f) for f in sanitized): + return None + form_data: dict[str, Any] = {"adhoc_filters": sanitized} + split_adhoc_filters_into_base_filters(form_data) + return ( + form_data.get("filters") or [], + form_data.get("where") or "", + form_data.get("having") or "", + ) + + +def _derive_orderby(params: dict[str, Any]) -> list[list[Any]]: + """ + Best-effort ordering from ``params`` (FR-002). + + Handles an explicit ``orderby`` list (either ``[[col, asc_bool], ...]`` or a + flat list of expressions) and a single sort metric + (``timeseries_limit_metric`` / ``sort_by_metric``). Falls back to no + ordering when nothing is derivable — the query builder supplies defaults. + """ + order_asc = not params.get("order_desc", True) + + orderby = params.get("orderby") + if isinstance(orderby, list) and orderby: + normalized: list[list[Any]] = [] + for entry in orderby: + if isinstance(entry, (list, tuple)) and len(entry) == 2: + normalized.append([entry[0], bool(entry[1])]) + else: + normalized.append([entry, order_asc]) + return normalized + + sort_metric = params.get("timeseries_limit_metric") or params.get("sort_by_metric") + if sort_metric: Review Comment: `sort_by_metric` is a boolean toggle on charts like Pie/Funnel (their `buildQuery` only adds `orderby: [[metric, false]]` when `sort_by_metric` is truthy), not a metric reference — but this falls back to it as the sort value itself. When `timeseries_limit_metric` is absent and `sort_by_metric` is `true`, the synthesized context gets `orderby: [[true, ...]]` instead of ordering by the chart's actual metric. Could this branch use the chart's own `metric`/`metrics[0]` instead of the raw `sort_by_metric` flag? ########## superset/cli/charts.py: ########## @@ -0,0 +1,170 @@ +# 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. +"""CLI commands for charts (Apache Superset #33615).""" + +from __future__ import annotations + +import logging +from typing import Any, Optional + +import click +from flask.cli import with_appcontext + +logger = logging.getLogger(__name__) + + [email protected]() +def charts() -> None: + """Chart-related maintenance commands.""" + + +def _derive_query_context(chart: Any, generator: Any) -> Optional[dict[str, Any]]: + """ + Derive a ``query_context`` config for ``chart``, or ``None`` if non-derivable. + + Prefers the authoritative frontend ``buildQuery`` (V8) and falls back to the + pure-Python generic derivation; the datasource is taken from the chart's own + resolved id/type, never from ``params`` (authz-preserving). + """ + from superset.commands.chart.query_context_builder import ( + build_query_context_config, + ) + from superset.utils import json + + params = json.loads(chart.params) if chart.params else {} + if not isinstance(params, dict): + params = {} + datasource_id = chart.datasource_id + datasource_type = chart.datasource_type or "table" + + context = None + if datasource_id: + js_params = { + **params, + "datasource": f"{datasource_id}__{datasource_type}", Review Comment: Unlike the import path's `_synthesize_query_context_if_absent` (which pops `slice_id` from `js_params` before calling the generator), this spreads `params` unchanged into `js_params`. A chart whose stored `params` still carries an exported/foreign `slice_id` (for example one imported before slice_id-stripping existed) gets that id passed straight to `generator.generate()`, reintroducing the foreign-chart-binding risk already fixed on the import path. This path also never re-injects `chart.cache_timeout` as `custom_cache_timeout`, so a backfilled chart's saved-chart reads fall back past its own cache_timeout to the datasource default. Could this mirror both fixes from the import path? ########## superset/commands/chart/importers/v1/utils.py: ########## @@ -134,6 +141,69 @@ def _prepare_existing_chart_for_import( return None +def _synthesize_query_context_if_absent(config: dict[str, Any]) -> None: + """ + Synthesize a ``query_context`` for an imported chart that arrives without + one, so the first ``GET /api/v1/chart/{pk}/data/`` returns data instead of + HTTP 400 "Chart has no query context saved" (issue #33615, ADR-013). Guarded + on an ABSENT context so an existing/remapped one is never overwritten + (FR-006); mutates ``config`` in place. + + Two-tier derivation: + 1. AUTHORITATIVE — run the chart's real frontend ``buildQuery`` in V8 + (QueryContextGenerator) for byte-faithful parity with the UI. + 2. FALLBACK — a pure-Python generic derivation + (``build_query_context_config``) when the V8 bundle / py_mini_racer is + unavailable or the viz type is not (yet) covered by the bundle. + Either way the datasource is taken from the importer-resolved id/type ONLY, + never a value carried in params (ADR-014 authz/RLS). A per-chart derivation + error must never abort the bundle (RISK-T03 / FR-004). + """ + if config.get("query_context"): + return + try: + params = config.get("params") or {} + viz_type = config["viz_type"] + datasource_id = config.get("datasource_id") + datasource_type = config.get("datasource_type", "table") + + query_context_config = None + if datasource_id: + # form_data for the JS builder: the datasource is the + # importer-resolved id/type only (overwrite any incoming + # params.datasource — never trust it; ADR-014). + js_params = { Review Comment: The landed fix took a different path than re-injecting the local slice_id: it bakes the chart's cache_timeout into the synthesized context as `custom_cache_timeout` instead. That closes the original gap, but `custom_cache_timeout` takes precedence over the chart's own cache_timeout in `get_cache_timeout()`'s priority order, and it's set once at synthesis time. If the chart's cache_timeout is edited later without re-triggering synthesis, saved-chart reads keep using the frozen import-time value instead of the new setting. Would binding to the actual local slice (once its id is known) still be worth doing instead, so cache-timeout edits take effect the same way they do for a chart that wasn't imported? ########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,177 @@ +# 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 function is **pure** (no DB / network / analytical-DB access). Its output, +when passed to ``QueryContextFactory.create(**payload)``, constructs a valid +``QueryContext``. See ADR-013 (synthesize-at-import) and ADR-014 +(datasource authz/RLS preservation). +""" + +from __future__ import annotations + +from typing import Any + +from superset.utils.core import split_adhoc_filters_into_base_filters + +# 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 ships a real buildQuery (its generated registry entry and golden +# fixture build a datasource-backed query), so it must stay on the derivable path. +# Classifying it datasource-less left imported handlebars charts with a NULL +# context that 400s on the data endpoint whenever the V8 bundle is unavailable. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "divider"}) + +# Default row limit mirrors the query-object default used across the read path. +_DEFAULT_ROW_LIMIT = 5000 + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], str, str]: + """ + Translate viz ``adhoc_filters`` into base filters via the shared splitter. + + Delegates to ``split_adhoc_filters_into_base_filters`` — the same helper the + read path uses — so SQL predicates are composed identically rather than by a + bare ``" AND ".join``: each clause is wrapped in parentheses (preserving + ``OR`` precedence) and a trailing ``--`` line comment is prevented from + swallowing predicates joined after it. Malformed entries are dropped by the + shared splitter rather than raising (RISK-T05), so an imported chart never + aborts its bundle over a single unmappable filter. + + Returns ``(filters, where, having)`` where ``where``/``having`` are the + parenthesized, comment-safe SQL strings ready for ``extras``. + """ + # Drop non-dict junk up front (RISK-T05): the shared splitter calls + # ``.get`` on each entry and would raise on a stray non-dict item. + sanitized = [f for f in (adhoc_filters or []) if isinstance(f, dict)] + form_data: dict[str, Any] = {"adhoc_filters": sanitized} + split_adhoc_filters_into_base_filters(form_data) + return ( + form_data.get("filters") or [], + form_data.get("where") or "", + form_data.get("having") or "", + ) + + +def _derive_orderby(params: dict[str, Any]) -> list[list[Any]]: + """ + Best-effort ordering from ``params`` (FR-002). + + Handles an explicit ``orderby`` list (either ``[[col, asc_bool], ...]`` or a + flat list of expressions) and a single sort metric + (``timeseries_limit_metric`` / ``sort_by_metric``). Falls back to no + ordering when nothing is derivable — the query builder supplies defaults. + """ + order_asc = not params.get("order_desc", True) + + orderby = params.get("orderby") + if isinstance(orderby, list) and orderby: + normalized: list[list[Any]] = [] + for entry in orderby: + if isinstance(entry, (list, tuple)) and len(entry) == 2: + normalized.append([entry[0], bool(entry[1])]) + else: + normalized.append([entry, order_asc]) + return normalized + + sort_metric = params.get("timeseries_limit_metric") or params.get("sort_by_metric") + if sort_metric: + return [[sort_metric, order_asc]] + return [] + + +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 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 whose keys are the kwargs of + ``QueryContextFactory.create``, or ``None`` when the chart is + non-derivable (FR-003): no datasource, nothing to query, or a + datasource-less viz type. 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 + metrics = params.get("metrics") or [] + # Single-metric viz types (e.g. Big Number) persist the metric under the + # singular `metric` key; normalize it into `metrics` so those charts are not + # misclassified as non-derivable (#33615: Big Number left with a NULL context). + if not metrics and params.get("metric"): + metrics = [params["metric"]] + # `groupby` is the deprecated alias of `columns`. + columns = params.get("columns") or params.get("groupby") or [] Review Comment: This still only reaches `all_columns` when `columns` and `groupby` are both empty. A chart with `query_mode: raw` that also has a leftover non-empty `groupby`/aggregate `metrics` (for example from before the chart was switched to raw mode) skips the `all_columns` branch entirely and gets a grouped/aggregated query instead of the raw column selection the frontend would produce for that viz. Could the `query_mode == "raw"` check take priority regardless of what's left in `columns`/`groupby`? ########## superset/cli/charts.py: ########## @@ -0,0 +1,161 @@ +# 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. +"""CLI commands for charts (Apache Superset #33615).""" + +from __future__ import annotations + +import logging +from typing import Any, Optional + +import click +from flask.cli import with_appcontext + +logger = logging.getLogger(__name__) + + [email protected]() +def charts() -> None: + """Chart-related maintenance commands.""" + + +def _derive_query_context(chart: Any, generator: Any) -> Optional[dict[str, Any]]: + """ + Derive a ``query_context`` config for ``chart``, or ``None`` if non-derivable. + + Prefers the authoritative frontend ``buildQuery`` (V8) and falls back to the + pure-Python generic derivation; the datasource is taken from the chart's own + resolved id/type, never from ``params`` (authz-preserving). + """ + from superset.commands.chart.query_context_builder import ( + build_query_context_config, + ) + from superset.utils import json + + params = json.loads(chart.params) if chart.params else {} + if not isinstance(params, dict): + params = {} + datasource_id = chart.datasource_id + datasource_type = chart.datasource_type or "table" + + context = None + if datasource_id: + js_params = { + **params, + "datasource": f"{datasource_id}__{datasource_type}", + } + context = generator.generate(chart.viz_type, js_params) + if context is None: + context = build_query_context_config( + params, chart.viz_type, datasource_id, datasource_type + ) + return context + + [email protected]("backfill-query-context") +@with_appcontext [email protected]( + "--dry-run", + is_flag=True, + default=False, + help="Report what would change without writing.", +) [email protected]( + "--viz-type", + "viz_types", + multiple=True, + help="Restrict to these viz types (repeatable). Default: all.", +) [email protected]( + "--batch-size", + type=int, + default=200, + show_default=True, + help="Number of charts to load, process, and commit per batch.", +) +def backfill_query_context( + dry_run: bool, viz_types: tuple[str, ...], batch_size: int +) -> None: + """ + Backfill a synthesized ``query_context`` on saved charts that have none. + + Repairs charts imported before the import-time synthesis landed (issue + #33615): each chart with ``query_context IS NULL`` gets a context derived + from its ``params`` + datasource — authoritatively via the frontend + ``buildQuery`` (V8) when available, else the pure-Python generic derivation. + Non-derivable charts are left untouched (never a fabricated context). + """ + # Imported lazily so the module imports cleanly without an app context. + from superset.commands.chart.query_context_generator import ( + get_query_context_generator, + ) + from superset.extensions import db + from superset.models.slice import Slice + from superset.utils import json + + if batch_size < 1: + raise click.BadParameter( + "must be a positive integer", param_hint="'--batch-size'" + ) + + generator = get_query_context_generator() + + # Snapshot the candidate primary keys up front (a light id-only query), then + # process them in stable-id pages. We must NOT stream with ``yield_per`` and + # commit mid-iteration: on PostgreSQL the commit closes the server-side cursor, + # so the next fetch fails and a backfill spanning more than one batch leaves the + # remaining charts untouched (#33615 review). Paging by id means each batch is + # its own query, so committing between batches is safe. + id_query = db.session.query(Slice.id).filter(Slice.query_context.is_(None)) + if viz_types: + id_query = id_query.filter(Slice.viz_type.in_(viz_types)) + chart_ids = [row[0] for row in id_query.all()] + + updated = 0 + non_derivable = 0 + errors = 0 + Review Comment: The reload-time NULL check only closes the snapshot-to-reload window; the write itself is still unconditional. If a chart is saved with a new query_context after this backfill's batch reloads it but before the batch commits, the commit overwrites that save with the context derived from the pre-save params. Could the write use a conditional update (e.g. `UPDATE ... WHERE id = ? AND query_context IS NULL`) instead of an unconditional ORM assignment, so a mid-batch save always wins? -- 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]
