bito-code-review[bot] commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4004776573
########## 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: <!-- Bito Reply --> Adding an integration test is a solid approach for verifying the CLI command, as it allows you to properly mock or set up the application context and database session. Since this command interacts directly with the database and relies on the app context, an integration test is more appropriate than a unit test to ensure the `backfill-query-context` logic behaves correctly in a real-world scenario. Proceeding with this as a follow-up is reasonable, provided it is tracked to ensure the coverage gap is addressed. ########## 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: <!-- Bito Reply --> The approach of catching a broad exception is acceptable here given the requirement that the generator must be non-fatal and best-effort. Since the goal is to prevent any builder-specific error from aborting the entire import process, catching a broad exception ensures the fallback mechanism is triggered reliably. If you prefer more granularity, you could catch `ImportError` specifically for the `py_mini_racer` import, but keeping the broader catch for the execution logic remains a valid pattern for non-fatal, best-effort components. **superset/commands/chart/query_context_generator.py** ``` try: from py_mini_racer import MiniRacer # pylint: disable=import-outside-toplevel except Exception as ex: # pylint: disable=broad-except ``` ########## 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: <!-- Bito Reply --> The use of a broad exception here is acceptable given the context that this is a non-fatal, best-effort fail-safe mechanism. Since the goal is to ensure that a single builder error does not abort the import process, catching the general `Exception` is a standard pattern for this type of defensive programming. If you prefer a narrower approach, you could catch specific exceptions related to the `py_mini_racer` initialization, such as `ImportError` or `RuntimeError`, but the current implementation is appropriate for its intended purpose. **superset/commands/chart/query_context_generator.py** ``` try: from py_mini_racer import MiniRacer # pylint: disable=import-outside-toplevel except Exception as ex: # pylint: disable=broad-except self._available = False ``` -- 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]
