This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch change/33615-chart-data-query-context in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1e72277f1850fa8e7cd178cfb365fcfabf698858 Author: Eugene Olkhovski <[email protected]> AuthorDate: Mon Aug 31 12:32:50 2026 -0400 fix(charts): address review on handlebars, backfill paging, slice_id leak (#33615) - query_context_builder: drop `handlebars` from the datasource-less set so the Python fallback derives it too (its registry entry and golden build a datasource-backed query); it otherwise kept a null context and 400'd when the V8 bundle was unavailable - cli/charts backfill: stop streaming with `yield_per` and committing mid-iteration (the commit closes the PostgreSQL server-side cursor, so a multi-batch run fails on the next fetch). Snapshot candidate ids and process them in stable-id pages, committing per page; guard `--batch-size` as positive - importers/v1/utils: drop the exported `slice_id` before V8 synthesis so QueryContextFactory can't resolve an unrelated destination chart sharing that id - tests: handlebars derivable, foreign slice_id dropped from generator params Co-Authored-By: Claude Opus 4.8 <[email protected]> --- superset/cli/charts.py | 77 +++++++++++----------- superset/commands/chart/importers/v1/utils.py | 6 ++ superset/commands/chart/query_context_builder.py | 7 +- .../charts/commands/importers/v1/import_test.py | 32 +++++++++ .../importers/v1/query_context_builder_test.py | 15 ++++- 5 files changed, 98 insertions(+), 39 deletions(-) diff --git a/superset/cli/charts.py b/superset/cli/charts.py index b1458e37d80..3252b5570dd 100644 --- a/superset/cli/charts.py +++ b/superset/cli/charts.py @@ -84,7 +84,7 @@ def _derive_query_context(chart: Any, generator: Any) -> Optional[dict[str, Any] type=int, default=200, show_default=True, - help="Commit every N updated charts.", + 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 @@ -106,50 +106,53 @@ def backfill_query_context( 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() - # `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) - ) + # 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: - query = query.filter(Slice.viz_type.in_(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 - pending = 0 - - for chart in query.yield_per(batch_size): - try: - context = _derive_query_context(chart, generator) - except Exception as ex: # pylint: disable=broad-except - errors += 1 - logger.warning( - "backfill-query-context: chart id=%s failed: %s", chart.id, ex - ) - continue - - 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() # pylint: disable=consider-using-transaction - pending = 0 - if not dry_run and pending: - db.session.commit() # pylint: disable=consider-using-transaction + for start in range(0, len(chart_ids), batch_size): + batch_ids = chart_ids[start : start + batch_size] + for chart in db.session.query(Slice).filter(Slice.id.in_(batch_ids)): + try: + context = _derive_query_context(chart, generator) + except Exception as ex: # pylint: disable=broad-except + errors += 1 + logger.warning( + "backfill-query-context: chart id=%s failed: %s", chart.id, ex + ) + continue + + if context is None: + non_derivable += 1 + continue + + updated += 1 + if not dry_run: + chart.query_context = json.dumps(context) + + if not dry_run: + db.session.commit() # pylint: disable=consider-using-transaction + # Keep the session (and memory) bounded across a large backfill; the next + # page loads its own rows by id. + db.session.expunge_all() prefix = "[dry-run] would update" if dry_run else "updated" click.echo( diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index 8c171798490..407347ea189 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -188,6 +188,12 @@ def _synthesize_query_context_if_absent(config: dict[str, Any]) -> None: **params, "datasource": f"{datasource_id}__{datasource_type}", } + # Drop the source chart's exported `slice_id`: passing it to the V8 + # buildQuery would let QueryContextFactory resolve an unrelated chart + # that happens to share that id in this installation, changing that + # chart's cache/guest-access behavior. The synthesized context must be + # bound to the datasource only, never a foreign slice. + js_params.pop("slice_id", None) query_context_config = get_query_context_generator().generate( viz_type, js_params ) diff --git a/superset/commands/chart/query_context_builder.py b/superset/commands/chart/query_context_builder.py index d68f727d0a0..ce377a40ec7 100644 --- a/superset/commands/chart/query_context_builder.py +++ b/superset/commands/chart/query_context_builder.py @@ -40,7 +40,12 @@ 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. -_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "handlebars", "divider"}) +# 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 diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py index 2cf8e3407d6..76ea920f051 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -557,6 +557,38 @@ def test_synthesis_runs_after_migration_for_legacy_viz( assert passed_viz_type == "mixed_timeseries" +def test_synthesis_drops_source_slice_id_from_generator_params( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + #33615 review: the exported ``slice_id`` must not reach the V8 buildQuery form + data. If that id belongs to an unrelated chart in the destination, + ``QueryContextFactory`` would resolve that chart from the synthesized context, + changing its cache/guest-access behavior. It is dropped before generation. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + generator = mocker.patch( + "superset.commands.chart.importers.v1.utils.get_query_context_generator" + ).return_value + generator.generate.return_value = None # force fallback; we inspect the call + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + config["viz_type"] = "table" + config["params"]["viz_type"] = "table" + config["params"]["metrics"] = ["count"] + config["params"]["slice_id"] = 999999 # a foreign id from the source export + config.pop("query_context", None) + + import_chart(config) + + # The V8 generator was called with form data that no longer carries slice_id. + assert generator.generate.called + passed_params = generator.generate.call_args.args[1] + assert "slice_id" not in passed_params + + def test_import_non_derivable_chart_leaves_query_context_null( mocker: MockerFixture, session_with_schema: Session ) -> None: diff --git a/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py index 82937422bd5..002af842e83 100644 --- a/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py @@ -126,7 +126,6 @@ def test_datasource_taken_from_argument_not_params() -> None: ({}, "table", 12), # no metrics AND no columns/groupby ({"metrics": [], "groupby": []}, "table", 12), # empty query intent ({"metrics": ["x"]}, "markup", 12), # datasource-less viz - ({"metrics": ["x"]}, "handlebars", 12), # datasource-less viz ({"metrics": ["x"]}, "divider", 12), # datasource-less viz ], ) @@ -138,6 +137,20 @@ def test_non_derivable_returns_none( assert build_query_context_config(params, viz_type, datasource_id, "table") is None +def test_handlebars_is_derivable() -> None: + """ + `handlebars` renders a template over query results — it has a real buildQuery + and a parity golden — so the fallback must derive it too. Classifying it + datasource-less left imported handlebars charts with a null context that 400s + when the V8 bundle is unavailable (#33615 review). + """ + query_context = build_query_context_config( + {"metrics": ["count"]}, "handlebars", 7, "table" + ) + assert query_context is not None + assert query_context["queries"][0]["metrics"] == ["count"] + + def test_adhoc_simple_filter_translation() -> None: """SIMPLE adhoc filters → simple {col, op, val} filters.""" params = {
