sadpandajoe commented on code in PR #43303:
URL: https://github.com/apache/superset/pull/43303#discussion_r4451405639
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -49,6 +53,8 @@
logger = logging.getLogger(__name__)
+logger = logging.getLogger(__name__)
Review Comment:
`logger = logging.getLogger(__name__)` is now defined twice, here and at
line 54. Is the second one left over from a merge and safe to drop?
##########
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(
Review Comment:
The only backfill test runs the command for real, so `--dry-run` is never
exercised: deleting both `if not dry_run` guards in `backfill_query_context`
would still pass, and a dry run would silently write contexts. Could a test
invoke the command with `--dry-run` on a derivable NULL-context chart, assert
the "[dry-run] would update 1" output, then reload the chart and assert
`query_context` is still NULL?
##########
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"):
Review Comment:
This guard only looks at the primary `extra_filters` and at
`filters`/`adhoc_filters`/`time_range` inside `extra_form_data`, but the
canonical builder applies none of the other dashboard overrides. Two ways a
derivable chart still gets a context that returns different rows than the live
chart: an `extra_form_data` carrying `time_grain_sqla` (e.g. `P1M` over a chart
saved with `P1D`) is persisted with daily buckets, and a `mixed_timeseries`
chart with only `extra_filters_b` (or `extra_form_data_b.filters`) has its
second query synthesized unfiltered. Should any non-empty `extra_form_data` /
`extra_filters`, including the `_b` variants, be treated as non-derivable so
these charts stay 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]
