eugeneo17 commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4004774798
########## 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: Good point. My inclination is to fail closed: return non-derivable (None) when any filter cannot be preserved, since a 400 is more honest than a silently broadened result set. That does mean one malformed adhoc filter drops the whole synthesized context. If you agree I will implement it with a test that an unmappable WHERE yields None. ########## 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, + help="Commit every N updated charts.", +) +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_builder import ( + build_query_context_config, + ) + 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 + + 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) + ) + if viz_types: + query = query.filter(Slice.viz_type.in_(viz_types)) + + updated = 0 + non_derivable = 0 + errors = 0 + pending = 0 + + for chart in query.yield_per(batch_size): + try: + 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 + ) + + 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() + pending = 0 + except Exception as ex: # pylint: disable=broad-except + errors += 1 + logger.warning( + "backfill-query-context: chart id=%s failed: %s", chart.id, ex + ) + + if not dry_run and pending: + db.session.commit() + + 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}." + ) Review Comment: Fair. The backfill-query-context command has no tests yet; a proper one needs an app context and a DB session (existing charts with and without a query_context), so I would add it as an integration test rather than a unit test. Planning it as a follow-up unless you would rather it block here. ########## superset/commands/chart/query_context_generator.py: ########## @@ -0,0 +1,176 @@ +# 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. +""" +Faithful ``query_context`` synthesis by running the real frontend ``buildQuery`` +on the backend (Apache Superset #33615, ADR-013 refinement). + +The ``form_data -> query_context`` mapping is per-viz-plugin JavaScript +(``buildQuery.ts``); a generic Python derivation can only approximate it. This +module evaluates a pre-built JS bundle of those ``buildQuery`` functions inside +V8 (``py_mini_racer``) and calls ``generateQueryContext(viz_type, form_data)`` — +producing the exact context the UI would. + +It is best-effort and NON-FATAL: if ``py_mini_racer`` is missing, the bundle has +not been built, or evaluation fails, :meth:`QueryContextGenerator.generate` +returns ``None`` and the caller falls back to the pure-Python generic derivation +(:func:`superset.commands.chart.query_context_builder.build_query_context_config`). + +Build the bundle with ``npm run build:backend-querycontext`` (from +``superset-frontend/``); the artifact lands at +``superset/commands/chart/_bundles/query_context_bundle.js``. +""" + +from __future__ import annotations + +import json +import logging +import os +import threading +from typing import Any, Optional + +logger = logging.getLogger(__name__) + +_BUNDLE_PATH = os.path.join( + os.path.dirname(__file__), "_bundles", "query_context_bundle.js" +) + +# Minimal globals a browser-targeted bundle may touch at load time. Kept as small +# as possible; expand only if a real load error demands it. +_BROWSER_SHIMS = """ +var globalThis = (typeof globalThis !== 'undefined') ? globalThis : this; +var self = globalThis; +var window = globalThis; +var navigator = { userAgent: 'superset-backend' }; +var document = undefined; +""" + +# Sentinels the JS entry returns instead of a context; each means "fall back". +_FALLBACK_SENTINELS = ("__unsupported__", "__error__") + + +class QueryContextGenerator: + """Lazy V8-backed generator. Thread-safe (a lock serializes V8 access).""" + + def __init__(self) -> None: + self._lock = threading.Lock() + self._ctx: Any = None + self._available: Optional[bool] = None # None = not yet initialized + self._logged_unavailable = False + + def _ensure_ctx(self) -> bool: + """Initialize the V8 context once. Returns availability; never raises.""" + if self._available is not None: + return self._available + try: + from py_mini_racer import MiniRacer # pylint: disable=import-outside-toplevel + except Exception as ex: # pylint: disable=broad-except Review Comment: This one is intentional. The generator is documented as best-effort and non-fatal: if the V8 bundle or py_mini_racer is missing, or a single builder throws, synthesis must never abort the import, it falls back to the Python builder. Narrowing the except here would risk a builder-specific error taking down the whole import. Happy to catch a more specific type if you have one in mind. ########## superset/commands/chart/query_context_generator.py: ########## @@ -0,0 +1,176 @@ +# 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. +""" +Faithful ``query_context`` synthesis by running the real frontend ``buildQuery`` +on the backend (Apache Superset #33615, ADR-013 refinement). + +The ``form_data -> query_context`` mapping is per-viz-plugin JavaScript +(``buildQuery.ts``); a generic Python derivation can only approximate it. This +module evaluates a pre-built JS bundle of those ``buildQuery`` functions inside +V8 (``py_mini_racer``) and calls ``generateQueryContext(viz_type, form_data)`` — +producing the exact context the UI would. + +It is best-effort and NON-FATAL: if ``py_mini_racer`` is missing, the bundle has +not been built, or evaluation fails, :meth:`QueryContextGenerator.generate` +returns ``None`` and the caller falls back to the pure-Python generic derivation +(:func:`superset.commands.chart.query_context_builder.build_query_context_config`). + +Build the bundle with ``npm run build:backend-querycontext`` (from +``superset-frontend/``); the artifact lands at +``superset/commands/chart/_bundles/query_context_bundle.js``. +""" + +from __future__ import annotations + +import json +import logging +import os +import threading +from typing import Any, Optional + +logger = logging.getLogger(__name__) + +_BUNDLE_PATH = os.path.join( + os.path.dirname(__file__), "_bundles", "query_context_bundle.js" +) + +# Minimal globals a browser-targeted bundle may touch at load time. Kept as small +# as possible; expand only if a real load error demands it. +_BROWSER_SHIMS = """ +var globalThis = (typeof globalThis !== 'undefined') ? globalThis : this; +var self = globalThis; +var window = globalThis; +var navigator = { userAgent: 'superset-backend' }; +var document = undefined; +""" + +# Sentinels the JS entry returns instead of a context; each means "fall back". +_FALLBACK_SENTINELS = ("__unsupported__", "__error__") + + +class QueryContextGenerator: + """Lazy V8-backed generator. Thread-safe (a lock serializes V8 access).""" + + def __init__(self) -> None: + self._lock = threading.Lock() + self._ctx: Any = None + self._available: Optional[bool] = None # None = not yet initialized + self._logged_unavailable = False + + def _ensure_ctx(self) -> bool: + """Initialize the V8 context once. Returns availability; never raises.""" + if self._available is not None: + return self._available + try: + from py_mini_racer import MiniRacer # pylint: disable=import-outside-toplevel + except Exception as ex: # pylint: disable=broad-except + self._available = False + logger.info( + "Backend query_context generator disabled: py_mini_racer " + "unavailable (%s). Falling back to generic derivation.", + ex, + ) + return False + + if not os.path.exists(_BUNDLE_PATH): + self._available = False + logger.info( + "Backend query_context generator disabled: bundle not built at " + "%s (run `npm run build:backend-querycontext`). Falling back to " + "generic derivation.", + _BUNDLE_PATH, + ) + return False + + try: + with open(_BUNDLE_PATH, encoding="utf-8") as fh: + bundle_src = fh.read() + ctx = MiniRacer() + ctx.eval(_BROWSER_SHIMS) + ctx.eval(bundle_src) + # Smoke-test that the callable is present. + ctx.eval("typeof generateQueryContext === 'function'") + self._ctx = ctx + self._available = True + logger.info("Backend query_context generator ready (V8 buildQuery).") + return True + except Exception as ex: # pylint: disable=broad-except Review Comment: Same as the other broad-except thread: this is a deliberate fail-safe so a single builder error never aborts the import (the path is documented as best-effort and non-fatal, falling back to the Python builder). Open to a narrower exception type if you prefer one. ########## superset/commands/chart/importers/v1/utils.py: ########## @@ -134,6 +141,69 @@ def _prepare_existing_chart_for_import( return None +def _synthesize_query_context_if_absent(config: dict[str, Any]) -> None: + """ + Synthesize a ``query_context`` for an imported chart that arrives 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); mutates ``config`` in place. + + 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 config.get("query_context"): + return + 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 = { Review Comment: The foreign-binding part is fixed: the exported slice_id is dropped before the V8 builder, so the context binds to the resolved datasource only. On your cache_timeout follow-up: you are right that with no local slice_id the reconstructed context cannot resolve the chart's own cache_timeout. Both come from synthesis running before the row has an id. I would like to re-inject the imported chart's own slice_id after import_from_dict assigns it, which fixes both the foreign-binding risk and the cache_timeout gap. Does that match what you had in mind? -- 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]
