sadpandajoe commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r3994499826
########## superset-frontend/scripts/gen-qc-registry.mjs: ########## @@ -0,0 +1,233 @@ +/** + * 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. + */ + +// 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.ts'), + '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, +)) { + const [, local, enumName] = m; + if (!VIZ_ENUM[enumName]) continue; + CLASS_TO_VIZ[importOrig[local] ?? local] = VIZ_ENUM[enumName]; + CLASS_TO_VIZ[local] = VIZ_ENUM[enumName]; // also accept the local alias +} + +// --- collect all index.ts + buildQuery modules under plugins/ --- +function walk(dir, acc = []) { + for (const e of readdirSync(dir)) { + const p = path.join(dir, e); + const st = statSync(p); + if (st.isDirectory()) { + if (e === 'node_modules' || e === 'test' || e === '__tests__') continue; + walk(p, acc); + } else acc.push(p); + } + return acc; +} +const files = walk(PLUGINS); +const indexFiles = files.filter((f) => /[/\\]index\.ts$/.test(f)); +const buildQueryFiles = files.filter((f) => /[/\\]buildQuery\.(ts|js)$/.test(f)); + +// resolve an import specifier from a file to an absolute module file (ts/js/index) +function resolveImport(fromFile, spec) { + const base = path.resolve(path.dirname(fromFile), spec); + for (const c of [ + base, + `${base}.ts`, + `${base}.js`, + path.join(base, 'index.ts'), + path.join(base, 'index.js'), + ]) { + if (existsSync(c) && statSync(c).isFile()) return path.resolve(c); + } + return null; +} + +// --- re-export graph: node "NAME@file" or "DEF@file"; edge to the module it forwards to. +// Lets us find every alias under which a subdir's DEFAULT export (a plugin class) is +// visible in the package barrels MainPreset imports from (handles `default as Alias` +// renames + multi-hop named passthrough like BigNumber). --- +const edges = new Map(); // node -> node it forwards to +const key = (name, file) => `${name}@${file}`; +for (const idx of indexFiles) { + const src = readFileSync(idx, 'utf8'); + for (const m of src.matchAll(/export\s*\{([^}]*)\}\s*from\s*['"]([^'"]+)['"]/g)) { + const target = resolveImport(idx, m[2]); + if (!target) continue; + for (let spec of m[1].split(',')) { + spec = spec.trim(); + if (!spec) continue; + let dm; + if ((dm = spec.match(/^default\s+as\s+(\w+)$/))) + edges.set(key(dm[1], idx), key('DEF', target)); // Alias = default of target + else if ((dm = spec.match(/^(\w+)\s+as\s+(\w+)$/))) + edges.set(key(dm[2], idx), key(dm[1], target)); // Y = target's X + else if ((dm = spec.match(/^(\w+)$/))) + edges.set(key(dm[1], idx), key(dm[1], target)); // named passthrough + } + } +} +// reverse reachability: all alias NAMEs (anywhere) that resolve to DEF@indexFile +function aliasesForDefault(indexFile) { + const goal = key('DEF', path.resolve(indexFile)); + const out = new Set(); + for (const [from, to] of edges) { + // does `from` reach goal? + let cur = to; + const seen = new Set([from]); + while (cur && !seen.has(cur)) { + if (cur === goal) { + out.add(from.split('@')[0]); + break; + } + seen.add(cur); + cur = edges.get(cur); + } + } + return out; +} + +// --- 3+4. For each buildQuery, find index.ts importing it -> plugin class alias -> viz key --- +const REGISTRY = {}; // viz -> buildQuery abs path +const unmapped = []; // buildQuery with no resolvable viz key +for (const bq of buildQueryFiles) { + const bqAbs = path.resolve(bq); + const owningIndexes = new Set(); + for (const idx of indexFiles) { + const src = readFileSync(idx, 'utf8'); + for (const m of src.matchAll(/import\s+\w+\s+from\s+['"]([^'"]+buildQuery)['"]/g)) { Review Comment: This matcher only recognizes static `import ... from './buildQuery'`, but plugins such as Chord register the normal `loadBuildQuery: () => import('./buildQuery')` path, so they are omitted from the generated registry. An imported Chord chart then falls back to the generic builder and drops its source `groupby` dimension; could the generator follow lazy registrations and add a Chord parity case? ########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,177 @@ +# 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 + +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. +# 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 + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], str, str]: + """ + Translate viz ``adhoc_filters`` into base filters via the shared splitter. + + Delegates to ``split_adhoc_filters_into_base_filters`` — the same helper the + read path uses — so SQL predicates are composed identically rather than by a + bare ``" AND ".join``: each clause is wrapped in parentheses (preserving + ``OR`` precedence) and a trailing ``--`` line comment is prevented from + swallowing predicates joined after it. Malformed entries are dropped by the + shared splitter rather than raising (RISK-T05), so an imported chart never + aborts its bundle over a single unmappable filter. + + Returns ``(filters, where, having)`` where ``where``/``having`` are the + parenthesized, comment-safe SQL strings ready for ``extras``. + """ + # Drop non-dict junk up front (RISK-T05): the shared splitter calls + # ``.get`` on each entry and would raise on a stray non-dict item. + sanitized = [f for f in (adhoc_filters or []) if isinstance(f, dict)] + form_data: dict[str, Any] = {"adhoc_filters": sanitized} + split_adhoc_filters_into_base_filters(form_data) + return ( + form_data.get("filters") or [], + form_data.get("where") or "", + form_data.get("having") or "", + ) + + +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 [] + # Single-metric viz types (e.g. Big Number) persist the metric under the + # singular `metric` key; normalize it into `metrics` so those charts are not + # misclassified as non-derivable (#33615: Big Number left with a NULL context). + if not metrics and params.get("metric"): + metrics = [params["metric"]] + # `groupby` is the deprecated alias of `columns`. + columns = params.get("columns") or params.get("groupby") or [] Review Comment: The fallback only treats `columns` or `groupby` as table dimensions and never reads raw-mode `all_columns`. Without the optional JavaScript runtime, importing the bundled `Table.yaml` (`query_mode: raw`, empty metrics/groupby, five `all_columns`) returns `None`, so its saved-chart data endpoint still responds 400; could raw-table semantics be preserved with a regression for this fixture? ########## superset/cli/charts.py: ########## @@ -0,0 +1,161 @@ +# 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 +from typing import Any, Optional + +import click +from flask.cli import with_appcontext + +logger = logging.getLogger(__name__) + + [email protected]() +def charts() -> None: + """Chart-related maintenance commands.""" + + +def _derive_query_context(chart: Any, generator: Any) -> Optional[dict[str, Any]]: + """ + Derive a ``query_context`` config for ``chart``, or ``None`` if non-derivable. + + Prefers the authoritative frontend ``buildQuery`` (V8) and falls back to the + pure-Python generic derivation; the datasource is taken from the chart's own + resolved id/type, never from ``params`` (authz-preserving). + """ + from superset.commands.chart.query_context_builder import ( + build_query_context_config, + ) + from superset.utils import json + + 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 + ) + return context + + [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="Number of charts to load, process, and commit per batch.", +) +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_generator import ( + get_query_context_generator, + ) + from superset.extensions import db + 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() + + # 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: + 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 + Review Comment: The initial NULL-ID snapshot is not rechecked when each batch reloads rows, so a chart saved with a fresh `query_context` after the snapshot can be overwritten by the backfill's approximate result. Could each write use a guarded update or row lock and skip rows whose context is no longer null, with a concurrent-save regression? ########## 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: Agreed—the generator never removes fixtures for visualization types that were removed or became unsupported, so stale form-data files keep failing parity against the current registry. Could this reconcile both fixture directories to the covered set? ########## 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: Agreed—`CartodiagramPlugin` still passes constructor options, so this matcher omits its real `buildQuery`; the generic fallback cannot derive the query intent nested under `selected_chart`. Could this accept constructor arguments and add a Cartodiagram fixture? ########## 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: Agreed—when `overwrite` is false, `import_chart` can return an existing row before synthesis and this counter reports its old context as newly queryable. Could the import result distinguish unchanged existing rows so the operator summary stays accurate? ########## 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) Review Comment: Agreed—dropping an unmappable filter while still persisting a context turns an invalid imported chart into a broader query than its form data requested. Could the fallback return non-derivable when any filter cannot be preserved? ########## 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: Agreed—the fallback persists 5,000 when `row_limit` is absent, while `QueryObjectFactory` applies the configured `ROW_LIMIT` (50,000 by default), so imported charts can silently truncate results. Could this leave the field unset or use the runtime-configured limit? ########## superset-frontend/package.json: ########## @@ -39,6 +39,7 @@ "scripts": { "_format": "oxfmt './{src,spec,cypress-base,plugins,packages,.storybook}/**/*{.js,.jsx,.ts,.tsx,.css,.scss,.sass,.json}'", "build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production BABEL_ENV=\"${BABEL_ENV:=production}\" webpack --color --mode production", + "build:backend-querycontext": "node scripts/build-backend-querycontext.mjs", Review Comment: The extra now requires `py-mini-racer>=0.12.4`, but that distribution stops at 0.6.0 (0.12.4 is published as `mini-racer`), and the Docker final stage never copies the generated bundle. Could this use the published package and transfer the bundle into the final image so the documented path can run? -- 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]
