eugeneo17 commented on code in PR #43303:
URL: https://github.com/apache/superset/pull/43303#discussion_r4004769826
##########
superset-frontend/scripts/gen-qc-fixtures.mjs:
##########
@@ -0,0 +1,108 @@
+// Generates a minimal-but-valid form_data fixture + frontend golden for every
viz
+// type in the generated REGISTRY, by actually running each plugin's
buildQuery under
+// 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 { readFileSync, writeFileSync, mkdirSync } from 'node:fs';
+import { createRequire } from 'node:module';
+
+const require = createRequire(import.meta.url);
+const FEX = 'src/backend-querycontext/__fixtures__';
+
+const res = await build({
+ entryPoints: ['src/backend-querycontext/entry.ts'],
+ bundle: true,
+ format: 'cjs',
+ platform: 'node',
+ target: 'es2020',
+ write: false,
+ legalComments: 'none',
+});
+const shim = (k, v) => {
+ try {
+ if (globalThis[k] === undefined) globalThis[k] = v;
+ } catch {
+ /* read-only builtin */
+ }
+};
+shim('self', globalThis);
+shim('window', globalThis);
+shim('navigator', { userAgent: 'superset-fixtures-node' });
+shim('document', {});
+const mod = { exports: {} };
+new Function('module', 'exports', 'require', res.outputFiles[0].text)(mod,
mod.exports, require);
+const gen = mod.exports.generateQueryContext;
+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) => ({
+ datasource: '1__table',
+ viz_type: viz,
+ metric: 'count',
+ metrics: ['count'],
+ groupby: ['gender'],
+ columns: ['gender'],
+ all_columns: ['gender', 'name'],
+ entity: 'gender',
+ series: 'gender',
+ series_columns: ['gender'],
+ x_axis: 'ds',
+ granularity_sqla: 'ds',
+ time_grain_sqla: 'P1D',
+ time_range: 'No filter',
+ row_limit: 100,
+ adhoc_filters: [],
+ // graph/sankey/tree
+ source: 'gender',
+ target: 'name',
+ source_category: 'gender',
+ target_category: 'name',
+ // heatmap
+ x_axis_column: 'gender',
+ y_axis: 'name',
+ // bubble
+ x: 'count',
+ y: 'count',
+ size: 'count',
+ // histogram
+ column: 'num',
+ // mixed timeseries second query
+ metrics_b: ['count'],
+ groupby_b: ['gender'],
+ adhoc_filters_b: [],
+});
+
+mkdirSync(`${FEX}/formdata`, { recursive: true });
+mkdirSync(`${FEX}/expected`, { recursive: true });
+
+const covered = [];
+const skipped = [];
+for (const viz of VIZ_TYPES) {
+ const fd = baseFor(viz);
+ let out;
+ try {
+ out = JSON.parse(gen(viz, JSON.stringify(fd)));
+ } catch (e) {
+ skipped.push([viz, `threw: ${String(e).slice(0, 120)}`]);
+ continue;
+ }
+ if (!out || out.__unsupported__ || out.__error__) {
+ 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');
+ covered.push(viz);
Review Comment:
Fixed in 065f8b4f5. After generating, both fixture directories are
reconciled to the covered set, so a removed or renamed viz type can no longer
leave a stale fixture failing parity.
##########
superset-frontend/scripts/gen-qc-registry.mjs:
##########
@@ -0,0 +1,187 @@
+// Deterministic, re-runnable codegen: maps every plugin `buildQuery` module
to the
+// viz_type key(s) it is registered under, and emits registry.generated.ts
consumed
+// by entry.ts. Join: buildQuery module <- (index.ts that imports it) ->
plugin class
+// name -> MainPreset `.configure({ key: VizType.X })` -> VizType enum string.
+// A single builder legitimately maps to several keys (e.g. echarts_timeseries
+ _line/_bar/...).
+import { readFileSync, writeFileSync, readdirSync, statSync, existsSync } from
'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const FE = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
+const PLUGINS = path.join(FE, 'plugins');
+const OUT = path.join(FE, 'src', 'backend-querycontext',
'registry.generated.ts');
+
+// --- 1. VizType enum: EnumName -> 'string_value' ---
+const vizTypeSrc = readFileSync(
+ path.join(FE, 'packages/superset-ui-core/src/chart/types/VizType.ts'),
+ 'utf8',
+);
+const VIZ_ENUM = {};
+for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g))
VIZ_ENUM[m[1]] = m[2];
+
+// --- 2. MainPreset: ClassName -> viz string ---
+const mainPreset = readFileSync(
+ path.join(FE, 'src/visualizations/presets/MainPreset.js'),
+ 'utf8',
+);
+// MainPreset renames on import (e.g. `import { PivotTableChartPlugin as
+// PivotTableChartPluginV2 } from '...'`), then `new
PivotTableChartPluginV2()`. Map
+// each local `new X()` name back to the ORIGINAL package export name the
codegen sees.
+const importOrig = {}; // localName -> package-export name
+for (const im of
mainPreset.matchAll(/import\s*\{([^}]*)\}\s*from\s*['"][^'"]+['"]/g)) {
+ for (let spec of im[1].split(',')) {
+ spec = spec.trim();
+ if (!spec) continue;
+ const as = spec.match(/^(\w+)\s+as\s+(\w+)$/);
+ if (as) importOrig[as[2]] = as[1];
+ else if (/^\w+$/.test(spec)) importOrig[spec] = spec;
+ }
+}
+const CLASS_TO_VIZ = {};
+for (const m of mainPreset.matchAll(
+ /new\s+(\w+)\s*\(\s*\)\s*\.configure\(\s*\{\s*key:\s*VizType\.(\w+)/g,
Review Comment:
Fixed in d7ae729487. Cartodiagram (and Chord, which registers its buildQuery
lazily via `loadBuildQuery: () => import('./buildQuery')`) are now mapped
through an explicit override table in the generator, each path asserted to
exist. I kept the structural matcher narrow rather than broadening it to every
constructor-arg plugin, to scope this to the two viz types raised here;
generalizing it is a follow-up.
##########
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,
Review Comment:
Fixed: `batch_size < 1` is rejected before the backfill starts, so a
non-positive value can no longer reach `yield_per` or the commit threshold.
##########
superset/commands/chart/importers/v1/__init__.py:
##########
@@ -103,7 +112,16 @@ def _import(
"datasource_name": dataset.table_name,
}
config = update_chart_config_dataset(config, dataset_dict)
+ # Capture the pre-import context state before `import_chart`
+ # mutates `config` (it synthesizes into
config["query_context"]).
+ had_query_context = bool(config.get("query_context"))
chart = import_chart(config, overwrite=overwrite)
+ if had_query_context:
+ n_preserved += 1
+ elif chart.query_context:
+ n_queryable += 1
+ else:
+ n_non_derivable += 1
Review Comment:
Fixed in 065f8b4f5. The count is now keyed off what this import wrote to the
config (synthesis only sets `config["query_context"]` when it derives one), not
the returned row, so an existing chart returned unchanged under
`overwrite=false` is no longer reported as newly queryable.
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -70,6 +77,64 @@ def import_chart(
filter_chart_annotations(config)
+ # Synthesize a query_context for imported charts that arrive 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).
+ #
+ # 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 not config.get("query_context"):
+ 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 = {
+ **params,
+ "datasource": f"{datasource_id}__{datasource_type}",
+ }
+ query_context_config = get_query_context_generator().generate(
+ viz_type, js_params
+ )
+ if query_context_config is None:
+ query_context_config = build_query_context_config(
+ params, viz_type, datasource_id, datasource_type
Review Comment:
Fixed: synthesis now runs after `migrate_chart`, so legacy viz types (e.g.
dual_line to mixed_timeseries) derive their queries from the migrated viz_type
and params rather than the pre-migration form data.
##########
superset/commands/chart/query_context_builder.py:
##########
@@ -0,0 +1,186 @@
+# 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
+
+# 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"})
+
+# 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]], list[str], list[str]]:
+ """
+ Translate viz ``adhoc_filters`` into simple ``{col, op, val}`` filters.
+
+ - ``expressionType == "SIMPLE"`` → a ``{col, op, val}`` filter.
+ - ``expressionType == "SQL"`` → routed to ``extras.where`` (or
+ ``extras.having`` for a HAVING clause) so a hand-written SQL predicate is
+ preserved rather than lost.
+ - Anything that cannot be mapped cleanly (missing subject/operator,
malformed
+ entry) is dropped **without raising** (RISK-T05) — an imported chart must
+ never abort its bundle over a single unmappable filter.
+
+ Returns ``(filters, where_expressions, having_expressions)``.
+ """
+ filters: list[dict[str, Any]] = []
+ where_expressions: list[str] = []
+ having_expressions: list[str] = []
+ for adhoc_filter in adhoc_filters or []:
+ if not isinstance(adhoc_filter, dict):
+ continue
+ expression_type = adhoc_filter.get("expressionType") or "SIMPLE"
+ if expression_type == "SIMPLE":
+ subject = adhoc_filter.get("subject")
+ operator = adhoc_filter.get("operator")
+ if subject and operator:
+ filters.append(
+ {
+ "col": subject,
+ "op": operator,
+ "val": adhoc_filter.get("comparator"),
+ }
+ )
+ # else: unmappable SIMPLE filter — dropped, no crash (RISK-T05).
+ elif expression_type == "SQL":
+ sql_expression = adhoc_filter.get("sqlExpression")
+ if sql_expression:
+ clause = (adhoc_filter.get("clause") or "WHERE").upper()
+ if clause == "HAVING":
+ having_expressions.append(sql_expression)
+ else:
+ where_expressions.append(sql_expression)
+ return filters, where_expressions, having_expressions
+
+
+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 []
+ # `groupby` is the deprecated alias of `columns`.
+ columns = params.get("columns") or params.get("groupby") or []
+ if not metrics and not columns:
+ return None
+
+ filters, where_expressions, having_expressions = _translate_adhoc_filters(
+ params.get("adhoc_filters", [])
+ )
+
+ query_object = {
+ "time_range": params.get("time_range", " : "),
+ "granularity": params.get("granularity_sqla") or
params.get("granularity"),
+ "filters": filters,
+ "extras": {
+ "time_grain_sqla": params.get("time_grain_sqla"),
+ "having": " AND ".join(having_expressions),
+ "where": " AND ".join(where_expressions),
+ },
+ "applied_time_extras": {},
+ "columns": columns,
+ "metrics": metrics,
+ "orderby": _derive_orderby(params),
+ "annotation_layers": [],
+ "row_limit": params.get("row_limit", _DEFAULT_ROW_LIMIT),
Review Comment:
Fixed in d7ae729487. The hard-coded 5000 is gone; row_limit is left unset
when the chart specifies none, so the read path applies the configured
ROW_LIMIT. Added a unit test.
--
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]
