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 a434d44a677f7e74e835e8a1fb1391f9457b917a Author: Eugene Olkhovski <[email protected]> AuthorDate: Mon Sep 14 07:28:36 2026 -0400 fix(charts): address remaining query_context import review items (#33615) - cli/charts.py: guard the backfill against a race with the id snapshot. A chart can gain a query_context between the snapshot and the batch reload, so skip rows whose context is no longer NULL instead of overwriting it; report the skipped count. - importers/v1/__init__.py: count the synthesis outcome from what this import did to the config, not from the returned row. import_chart returns an existing chart unchanged (before synthesis) when overwrite is false, so the previous check reported that row's pre-existing context as newly synthesized. - pyproject.toml: the querycontext extra pinned py-mini-racer>=0.12.4, but that distribution stops at 0.6.0; the maintained package for 0.12.x is mini-racer (still imports as py_mini_racer). Switched the extra accordingly. - Dockerfile: copy the backend query_context V8 bundle built in the node stage into the final image (optional glob, only present when the frontend is built), so the exact-parity path actually ships instead of always falling back to the pure-Python builder. - gen-qc-fixtures.mjs: reconcile both fixture directories to the covered set so a removed/renamed viz type cannot leave a stale fixture failing parity. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- Dockerfile | 6 ++++ pyproject.toml | 4 ++- superset-frontend/scripts/gen-qc-fixtures.mjs | 38 ++++++++++++++++++++---- superset/cli/charts.py | 11 ++++++- superset/commands/chart/importers/v1/__init__.py | 10 ++++++- 5 files changed, 61 insertions(+), 8 deletions(-) diff --git a/Dockerfile b/Dockerfile index e301dad8bac..31f7223be5f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -272,6 +272,12 @@ COPY --from=superset-node /app/superset/static/service-worker.j[s] superset/stat # TODO, when the next version comes out, use --exclude superset/translations COPY superset superset +# Overlay the backend query_context V8 bundle built in the node stage (#33615). +# Optional glob (`[s]`): the bundle only exists when the frontend was built +# (DEV_MODE=false); without it the chart importer falls back to the pure-Python +# generic derivation. Must come after `COPY superset superset` so it is not +# clobbered by the source tree (which never contains the generated bundle). +COPY --from=superset-node /app/superset/commands/chart/_bundles/query_context_bundle.j[s] superset/commands/chart/_bundles/query_context_bundle.js # TODO in the meantime, remove the .po files RUN rm superset/translations/*/*/*.po diff --git a/pyproject.toml b/pyproject.toml index 32aae99c2a6..4e0b02a2bea 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -258,7 +258,9 @@ prophet = ["prophet>=1.4.0, <2"] # the real `buildQuery` bundle in V8 via py_mini_racer. Optional — without it the # importer falls back to the pure-Python generic derivation. Install this extra # (and ship the bundle, built by `npm run build`) to make exact parity the default. -querycontext = ["py-mini-racer>=0.12.4, <1.0"] +# NB: the maintained distribution is `mini-racer` (the original `py-mini-racer` +# stops at 0.6.0); it still imports as `py_mini_racer`. +querycontext = ["mini-racer>=0.12.4, <1.0"] # sqlalchemy-redshift cuts hard from SQLAlchemy 1.4-only (0.8.x) to 2.0-only # (>=1.0.0) with no dual-compat release. Bumped now that Superset's own # SQLAlchemy 2.0 core bump has landed (discussion #40273). diff --git a/superset-frontend/scripts/gen-qc-fixtures.mjs b/superset-frontend/scripts/gen-qc-fixtures.mjs index cf449b551bc..f4211f84b97 100644 --- a/superset-frontend/scripts/gen-qc-fixtures.mjs +++ b/superset-frontend/scripts/gen-qc-fixtures.mjs @@ -22,7 +22,7 @@ // Node's V8. A viz is COVERED if its builder returns a real query_context on the base // form_data; otherwise it's honestly SKIPPED (reason recorded) — never faked. import { build } from 'esbuild'; -import { writeFileSync, mkdirSync } from 'node:fs'; +import { writeFileSync, mkdirSync, readdirSync, rmSync } from 'node:fs'; import { createRequire } from 'node:module'; const require = createRequire(import.meta.url); @@ -63,7 +63,7 @@ const VIZ_TYPES = mod.exports.VIZ_TYPES || globalThis.SUPERSET_QC_VIZ_TYPES; // A deliberately broad base form_data — each plugin's buildQuery reads the fields it // needs and ignores the rest. Family-specific fields are all populated so most // builders find what they require without per-viz hand-tuning. -const baseFor = (viz) => ({ +const baseFor = viz => ({ datasource: '1__table', viz_type: viz, metric: 'count', @@ -115,20 +115,48 @@ for (const viz of VIZ_TYPES) { continue; } if (!out || out.__unsupported__ || out.__error__) { - skipped.push([viz, out && out.__error__ ? `builder error: ${String(out.__error__).slice(0, 140)}` : 'unsupported']); + skipped.push([ + viz, + out && out.__error__ + ? `builder error: ${String(out.__error__).slice(0, 140)}` + : 'unsupported', + ]); continue; } if (!Array.isArray(out.queries) || out.queries.length === 0) { skipped.push([viz, 'no queries produced']); continue; } - writeFileSync(`${FEX}/formdata/${viz}.json`, `${JSON.stringify(fd, null, 2)}\n`); - writeFileSync(`${FEX}/expected/${viz}.json`, `${JSON.stringify(out, null, 2)}\n`); + writeFileSync( + `${FEX}/formdata/${viz}.json`, + `${JSON.stringify(fd, null, 2)}\n`, + ); + writeFileSync( + `${FEX}/expected/${viz}.json`, + `${JSON.stringify(out, null, 2)}\n`, + ); covered.push(viz); } +// Reconcile: drop stale fixtures for viz types no longer covered (a plugin +// removed/renamed, or one that stopped producing a query_context). Otherwise an +// obsolete formdata/<viz>.json keeps failing the parity test's VIZ_TYPES +// membership check, or an expected/<viz>.json compares against an outdated golden. +const keep = new Set(covered.map(v => `${v}.json`)); +const removed = []; +for (const dir of [`${FEX}/formdata`, `${FEX}/expected`]) { + for (const f of readdirSync(dir)) { + if (f.endsWith('.json') && !keep.has(f)) { + rmSync(`${dir}/${f}`); + removed.push(`${dir}/${f}`); + } + } +} + console.log(`REGISTRY viz types: ${VIZ_TYPES.length}`); console.log(`COVERED (fixture+golden written): ${covered.length}`); console.log(` ${covered.join(', ')}`); console.log(`SKIPPED (base form_data insufficient): ${skipped.length}`); for (const [v, r] of skipped) console.log(` - ${v}: ${r}`); +console.log(`REMOVED (stale fixtures reconciled): ${removed.length}`); +for (const f of removed) console.log(` - ${f}`); diff --git a/superset/cli/charts.py b/superset/cli/charts.py index 3252b5570dd..a6a009e0a13 100644 --- a/superset/cli/charts.py +++ b/superset/cli/charts.py @@ -126,11 +126,19 @@ def backfill_query_context( updated = 0 non_derivable = 0 + skipped = 0 errors = 0 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)): + # Guard against a race with the id snapshot: a chart can gain a + # query_context (an import, an edit) between the snapshot above and + # this batch reload. The backfill only fills genuinely empty ones, so + # never overwrite a context that now exists (#33615 review). + if chart.query_context is not None: + skipped += 1 + continue try: context = _derive_query_context(chart, generator) except Exception as ex: # pylint: disable=broad-except @@ -157,5 +165,6 @@ def backfill_query_context( 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}." + f"non-derivable (left null) {non_derivable}, " + f"skipped (already had context) {skipped}, errors {errors}." ) diff --git a/superset/commands/chart/importers/v1/__init__.py b/superset/commands/chart/importers/v1/__init__.py index cc4d6b0e495..2e72b32c797 100644 --- a/superset/commands/chart/importers/v1/__init__.py +++ b/superset/commands/chart/importers/v1/__init__.py @@ -128,9 +128,17 @@ class ImportChartsCommand(ImportModelsCommand): chart = import_chart( config, overwrite=overwrite, default_viewers=default_viewers ) + # Count the synthesis outcome from what THIS import did to the + # config, not from the returned row. `import_chart` returns an + # existing chart unchanged (before synthesis runs) when + # `overwrite` is false, so keying off `chart.query_context` would + # report that row's pre-existing context as newly synthesized by + # this import (#33615 review). `_synthesize_query_context_if_absent` + # only writes `config["query_context"]` when it actually derives + # one, so the config is the honest signal. if had_query_context: n_preserved += 1 - elif chart.query_context: + elif config.get("query_context"): n_queryable += 1 else: n_non_derivable += 1
