eugeneo17 commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4483352641
########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,261 @@ +# 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 +that, for the viz types that builder has an explicit adapter for, 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 dropped +filter/ordering/role/post-processing and no viz-specific semantics (time axis, +pivot axes, server pagination, contribution, aggregation) silently lost. + +That builder is a **generic** mapper, not the chart's per-viz ``buildQuery``, so +two things are guarded here so synthesis never persists a context that returns +*more* rows than the chart shows (#33615 review): + +* **Allowlist (honest-fail).** Only viz types the builder maps explicitly are + synthesized; any other viz type (e.g. ``sankey_v2``, ``graph_chart``) falls + through the builder's generic top-N branch, which has no alias for its + controls and would emit a single aggregate query that *looks* valid. Those are + classified non-derivable and left ``NULL`` so the chart keeps prompting a + re-save rather than silently returning wrong data. +* **Adhoc time range.** The canonical builder injects ``time_range="No filter"`` + when a chart carries no top-level ``time_range``. At read time + ``QueryContextFactory._apply_filters`` would then overwrite a bound + ``TEMPORAL_RANGE`` adhoc filter's value with ``"No filter"`` and count every + date, so the injected sentinel is dropped when it would clobber a real range. + +Charts carrying dashboard-context filters (``extra_filters`` / +``extra_form_data``) are also left ``NULL`` — the generic builder does not apply +those the way the live chart does. + +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, +) +from superset.utils.date_parser import is_no_time_range + +# 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"}) + +# Viz types the canonical ``build_query_objects_from_form_data`` has an explicit +# adapter for (mirrors its ``effective_viz`` branches). Only these are +# synthesized; everything else falls through that builder's generic top-N branch, +# which cannot be trusted to map the viz faithfully, so we honest-fail to NULL. +# Keep in sync with ``superset.common.form_data_query_context``. +_BUILDER_MAPPED_VIZ: frozenset[str] = frozenset( + { + "table", + "ag-grid-table", + "ag-grid-pivot-table", + "histogram_v2", + "box_plot", + "pivot_table_v2", + "waterfall", + "gantt", + "gantt_chart", + "echarts_area", + "mixed_timeseries", + "big_number", + "big_number_total", + "handlebars", + "pie", + "gauge_chart", + "treemap_v2", + "sunburst_v2", + "bubble", + "bubble_v2", + "world_map", + "country_map", + } +) + + +def _is_builder_mapped(viz_type: str) -> bool: + """Return True if the canonical builder maps ``viz_type`` explicitly.""" + return viz_type in _BUILDER_MAPPED_VIZ or viz_type.startswith("echarts_timeseries") + + +def _has_dashboard_context_filters(params: dict[str, Any]) -> bool: + """ + Return True if ``params`` carries dashboard-context filters. + + ``extra_filters`` / ``extra_form_data`` are applied by the live chart (native + dashboard filters, URL params) in ways the generic builder does not reproduce, + so a chart carrying them is left non-derivable rather than synthesized with a + context that would drop those constraints (#33615 review). + """ + if params.get("extra_filters"): + return True + extra_form_data = params.get("extra_form_data") + if isinstance(extra_form_data, dict) and ( + extra_form_data.get("filters") + or extra_form_data.get("adhoc_filters") + or extra_form_data.get("time_range") + ): + return True + return False + + +def _preserve_adhoc_temporal_range(context: dict[str, Any]) -> None: + """ + Drop a builder-injected ``time_range="No filter"`` that would clobber a range. + + The canonical builder ``setdefault``s ``time_range="No filter"`` on every + query when the chart has no top-level ``time_range``. For a chart whose date + constraint lives in a bound ``TEMPORAL_RANGE`` adhoc filter, + ``QueryContextFactory._apply_filters`` would overwrite that filter's value + with ``"No filter"`` at read time and count every date. Removing the injected + sentinel (only when a bound ``TEMPORAL_RANGE`` filter is present) leaves the + adhoc filter's own range intact. Called only when ``params`` had no explicit + top-level time range, so a genuinely unbounded chart is never touched. + """ + for query in context.get("queries") or []: + if query.get("time_range") != "No filter": + continue + for flt in query.get("filters") or []: + if not isinstance(flt, dict) or flt.get("op") != "TEMPORAL_RANGE": + continue + val = flt.get("val") + bound = val is not None and not ( + isinstance(val, str) and is_no_time_range(val) + ) + if bound: + query.pop("time_range", None) + break + + +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, a viz + type the canonical builder has no explicit adapter for, a chart carrying + dashboard-context filters, 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 + + # Honest-fail for viz types the canonical builder has no explicit adapter + # for: its generic top-N branch would emit a plausible-looking but wrong + # context (e.g. ``sankey_v2``/``graph_chart`` -> one aggregate row instead of + # edges). Leave those NULL so the chart keeps prompting a re-save (#33615). + if not _is_builder_mapped(viz_type): + return None + + # Dashboard-context filters are applied by the live chart in ways the generic + # builder does not reproduce; synthesizing would drop them, so leave NULL. + if _has_dashboard_context_filters(params): + return None + + # Whether the chart itself carries a top-level time range. If not, any + # ``time_range`` on the built query was injected by the canonical builder and + # must not clobber a bound ``TEMPORAL_RANGE`` adhoc filter (see below). + had_explicit_time_range = bool( + params.get("time_range") or params.get("since") or params.get("until") + ) + + # 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"): + return None + + if not had_explicit_time_range: Review Comment: Fixed in 5bb17ea213. Preservation is now decided per query, not per chart: I dropped the chart-wide `had_explicit_time_range` gate and always call `_preserve_adhoc_temporal_range`, whose per-query guard only drops `time_range='No filter'` on a query that still carries a bound `TEMPORAL_RANGE` filter of its own. So a `mixed_timeseries` where only query A has a top-level range no longer leaves query B's bound `_b` range to be overwritten with `No filter` by `_apply_filters` at read time. Added `test_build_config_preserves_temporal_range_per_query_not_per_chart` (stubs the builder to a two-query context: A explicit range, B sentinel over a bound range, asserts A kept and B's sentinel dropped) plus a per-query helper test. ########## tests/integration_tests/cli_tests.py: ########## @@ -582,3 +582,96 @@ def test_failing_import_directory( ) assert_cli_fails_properly(response, caplog) + + [email protected]("load_birth_names_dashboard_with_slices") +def test_backfill_query_context_fills_only_null_contexts(app_context): + """ + ``backfill-query-context`` synthesizes a context for charts that have none, + binds it to the chart's own id/datasource, and leaves charts that already + have a context untouched (#33615). + + Runs with ``--batch-size 1`` across THREE null-context charts so the + id-snapshot paging and per-batch commit (which replaced the PostgreSQL-unsafe + ``yield_per`` streaming) are exercised across more than one batch (#33615 + review). + """ + import superset.cli.charts # noqa: F811 + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + from superset.utils import json + + table = db.session.query(SqlaTable).filter_by(table_name="birth_names").one() + # Capture the id now: the command commits (expiring instances) and expunges, + # so ``table`` is detached by the time the assertions run. + table_id = table.id + + null_names = [f"qc-backfill-null-{i}" for i in range(3)] + null_charts = [ + Slice( + slice_name=name, + viz_type="table", + datasource_type="table", + datasource_id=table.id, + params=json.dumps( + { + "viz_type": "table", + "metrics": ["count"], + "datasource": f"{table.id}__table", + } + ), + query_context=None, + ) + for name in null_names + ] + kept_chart = Slice( + slice_name="qc-backfill-existing", + viz_type="table", + datasource_type="table", + datasource_id=table.id, + params=json.dumps({"viz_type": "table", "metrics": ["count"]}), + query_context='{"existing": true}', + ) + db.session.add_all([*null_charts, kept_chart]) + db.session.commit() + + try: + runner = current_app.test_cli_runner() + response = runner.invoke( + superset.cli.charts.backfill_query_context, ("--batch-size", "1") + ) + + assert response.exit_code == 0, response.output Review Comment: Added `test_backfill_query_context_isolates_per_chart_errors` in 5bb17ea213: a chart with invalid-JSON `params` ahead of a valid NULL-context chart, run with `--batch-size 1` so they land in different batches. It asserts exit code 0, the bad chart still NULL, the valid one filled, and `errors 1` in the summary. Removing the per-chart `except` now aborts the run on the bad chart and fails the test. ########## superset/cli/charts.py: ########## @@ -0,0 +1,142 @@ +# 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 + +import click +from flask.cli import with_appcontext + +logger = logging.getLogger(__name__) + + [email protected]() +def charts() -> None: + """Chart-related maintenance commands.""" + + [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="Commit every N updated charts.", +) +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_builder import ( + build_query_context_config, + ) + 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 + + generator = get_query_context_generator() + + # `enable_eagerloads(False)` is required for `yield_per`: Slice has eager + # (joined) collection relationships that otherwise raise + # "Can't use yield_per with eager loaders that require uniquing/buffering". + query = ( + db.session.query(Slice) + .filter(Slice.query_context.is_(None)) + .enable_eagerloads(False) + ) + if viz_types: + query = query.filter(Slice.viz_type.in_(viz_types)) + + updated = 0 + non_derivable = 0 + errors = 0 + pending = 0 + + for chart in query.yield_per(batch_size): + try: + 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 + ) + + if context is None: + non_derivable += 1 + continue + + updated += 1 + if dry_run: + continue + + chart.query_context = json.dumps(context) + pending += 1 + if pending >= batch_size: + db.session.commit() + pending = 0 + except Exception as ex: # pylint: disable=broad-except + errors += 1 + logger.warning( + "backfill-query-context: chart id=%s failed: %s", chart.id, ex + ) + + if not dry_run and pending: + db.session.commit() + + prefix = "[dry-run] would update" if dry_run else "updated" + click.echo( + f"backfill-query-context: {prefix} {updated}, " + f"non-derivable (left null) {non_derivable}, errors {errors}." + ) Review Comment: Added CLI coverage in 5bb17ea213 on top of the existing `test_backfill_query_context_fills_only_null_contexts` (which exercises batch commits across `--batch-size 1`): a `--dry-run` test (asserts nothing is written) and a per-chart error-isolation test (malformed `params` counted as an error, backfill continues and still fills the valid chart). On the broad `except Exception`: it is deliberate here. The backfill must isolate a single bad chart rather than abort the whole run, so it counts the error, logs `chart id=... failed`, and continues; the new error-isolation test exercises exactly that path. -- 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]
