bito-code-review[bot] commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r3805191347
########## 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: <div> <div id="suggestion"> <div id="issue"><b>Missing test coverage for CLI command</b></div> <div id="fix"> The `backfill-query-context` command has zero test coverage — no unit or integration tests were found in the test suite. Other CLI commands in `cli_tests.py` use `app.test_cli_runner()` for coverage. A test should verify dry-run output, batch commits, and non-derivable chart handling. </div> </div> <div id="suggestion"> <div id="issue"><b>Catching generic Exception is blind</b></div> <div id="fix"> Catching broad `Exception` may hide important errors. Catch specific exceptions or log and re-raise to avoid silent failures. </div> </div> <small><i>Code Review Run #c84a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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 {} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>TypeError on string params in query_context synthesis</b></div> <div id="fix"> On line 96, `config.get("params") or {}` returns the raw JSON string for exported charts whose `params` field is a string (standard import format). Since a non-empty string is truthy, `or {}` evaluates to the string. Lines 106-107 then call `**params` on a string, raising `TypeError: expected a mapping`. The correct pattern is shown in `cli/charts.py:101` which calls `json.loads(chart.params)` before use. Apply the same `json.loads`-with-fallback pattern here. </div> </div> <small><i>Code Review Run #c84a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Catch specific exceptions instead</b></div> <div id="fix"> Catching broad `Exception` on line 80 may mask important errors. Similar issue on lines 111 and 135. Consider catching specific exceptions like `ImportError`. </div> </div> <small><i>Code Review Run #c84a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Catch specific exceptions instead</b></div> <div id="fix"> Catching broad `Exception` on line 111 may mask important errors. Consider catching specific exceptions. </div> </div> <small><i>Code Review Run #c84a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Regex excludes constructor-arg plugins</b></div> <div id="fix"> The regex only matches `new ClassName()` (zero arguments), but `CartodiagramPlugin` in `MainPreset.js` is instantiated with constructor options `new CartodiagramPlugin({ defaultLayers: [...] })`. The codegen silently skips it, leaving `plugins/plugin-chart-cartodiagram/src/plugin/buildQuery.ts` unmapped and `cartodiagram` absent from `registry.generated.ts`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion for (const m of mainPreset.matchAll( /new\s+(\w+)\s*\([^)]*\)\s*\.configure\(\s*\{\s*key:\s*VizType\.(\w+)/g, )) { ```` </div> </details> </div> <small><i>Code Review Run #c84a60</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
