eugeneo17 commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4445055615
########## 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: Added `test_backfill_query_context_fills_only_null_contexts` in `tests/integration_tests/cli_tests.py`: it covers dry-run vs write, a multi-batch commit (`--batch-size 1` over 3 null charts), the already-has-context skip, and `slice_id` binding to each chart's own id. The broad `except` is intentional — a single chart's synthesis failure must not abort the backfill; it is logged with the chart id and counted as an error. ########## tests/integration_tests/charts/data/api_tests.py: ########## @@ -1269,6 +1270,161 @@ def test_chart_data_get(self): assert data["result"][0]["status"] == "success" assert data["result"][0]["rowcount"] == 2 + # ------------------------------------------------------------------ # + # #33615 — synthesized query_context read-path verification. + # + # These exercise the REAL read/authz/RLS path (get_data → + # ChartDataCommand.validate() → raise_for_access) on a chart whose + # query_context was produced by `build_query_context_config` — the same + # helper the importer runs (ADR-013/014). The verifier is NOT mocked + # (SECURITY-CRITICAL discipline). They are the api-side counterpart of the + # importer round-trip (F3-T1), baseline-equivalence (F3-T2), and security + # (F3-T3) tasks. The existing 68 tests in this module stand unmodified as + # the NFR-COMPAT-01 regression guard. + # ------------------------------------------------------------------ # + + #: params equivalent to the "Genders" chart's saved viz form-data. + _GENDERS_PARAMS = { + "metrics": ["sum__num"], + "groupby": ["gender"], + "time_range": "1900-01-01T00:00:00 : 2000-01-01T00:00:00", + "granularity_sqla": "ds", + "row_limit": 50000, + "order_desc": True, + } + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_synthesized_query_context_returns_data(self): Review Comment: That class-level `@pytest.mark.skip` is pre-existing on master (the DuckDB/`birth_names` fixture conflict), not introduced by this PR. I kept the read-path assertions here so they run once that skip is lifted, and added import round-trip coverage that does execute: `commands_tests.py` asserts a derivable chart persists a context while `markup`/raising charts stay NULL. I'm happy to move the four read-path tests to a module that executes — do you have a preferred home for them? ########## tests/integration_tests/charts/commands_tests.py: ########## @@ -326,6 +328,153 @@ def test_import_v1_chart_multiple(self, mock_add_permissions, sm_g): db.session.delete(database) db.session.commit() + @patch("superset.utils.core.g") + @patch("superset.security.manager.g") + @patch("superset.commands.database.importers.v1.utils.add_permissions") + def test_import_v1_chart_synthesizes_query_context( + self, mock_add_permissions, sm_g, utils_g + ) -> None: + """ + #33615 / F3-T1 (import side): a derivable chart imported WITHOUT a + query_context persists a synthesized one naming the resolved datasource. + """ + sm_g.user = utils_g.user = security_manager.find_user("admin") + + derivable = copy.deepcopy(chart_config) + derivable["uuid"] = "aaaaaaaa-0000-0000-0000-000000000001" + derivable["slice_name"] = "Derivable imported chart" + derivable["viz_type"] = "table" + derivable["params"]["viz_type"] = "table" + derivable["params"]["metrics"] = ["count"] + derivable["params"]["groupby"] = ["cnt"] + derivable.pop("query_context", None) + + contents = { + "metadata.yaml": yaml.safe_dump(chart_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), + "charts/imported_chart.yaml": yaml.safe_dump(derivable), + } + ImportChartsCommand(contents, overwrite=True).run() + + chart: Slice = db.session.query(Slice).filter_by(uuid=derivable["uuid"]).one() + # --- RED anchor: synthesis persisted a non-null context (FR-001) --- + assert chart.query_context is not None + query_context = json.loads(chart.query_context) + # --- RED anchor: datasource fidelity — resolved id, not params (RISK-T02) --- + assert query_context["datasource"] == { + "id": chart.datasource_id, + "type": "table", + } + assert query_context["queries"][0]["metrics"] == ["count"] + + dataset = chart.datasource + database = dataset.database if dataset else None + db.session.delete(chart) + if dataset: + db.session.delete(dataset) + if database: + db.session.delete(database) + db.session.commit() + + @patch("superset.utils.core.g") + @patch("superset.security.manager.g") + @patch("superset.commands.database.importers.v1.utils.add_permissions") + def test_import_v1_chart_preserves_existing_query_context( + self, mock_add_permissions, sm_g, utils_g + ) -> None: + """ + FR-006 / INV-3: an imported chart that already has a query_context keeps + it (only the datasource is remapped) — synthesis never clobbers it. + """ + sm_g.user = utils_g.user = security_manager.find_user("admin") + + contents = { + "metadata.yaml": yaml.safe_dump(chart_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), + "charts/imported_chart.yaml": yaml.safe_dump(chart_config), + } + ImportChartsCommand(contents, overwrite=True).run() + + chart: Slice = ( + db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one() + ) + # --- RED anchor: context still present; datasource remapped, not resynthesized + assert chart.query_context is not None + query_context = json.loads(chart.query_context) + assert query_context["datasource"]["id"] == chart.datasource_id + assert query_context["result_type"] == "full" + + dataset = chart.datasource + database = dataset.database if dataset else None + db.session.delete(chart) + if dataset: + db.session.delete(dataset) + if database: + db.session.delete(database) + db.session.commit() + + @patch("superset.utils.core.g") + @patch("superset.security.manager.g") + @patch("superset.commands.database.importers.v1.utils.add_permissions") + def test_import_mixed_derivable_and_non_derivable_bundle( + self, mock_add_permissions, sm_g, utils_g + ) -> None: + """ + F3-T4 / FR-003 / FR-004 / RISK-T03: a bundle mixing a derivable chart and + a non-derivable (markup) chart imports fully — the derivable one becomes + queryable, the markup one keeps a NULL context, and the bundle does not + abort. + """ + sm_g.user = utils_g.user = security_manager.find_user("admin") + + derivable = copy.deepcopy(chart_config) + derivable["uuid"] = "aaaaaaaa-0000-0000-0000-000000000010" + derivable["slice_name"] = "Derivable table chart" + derivable["viz_type"] = "table" + derivable["params"]["viz_type"] = "table" + derivable["params"]["metrics"] = ["count"] + derivable["params"]["groupby"] = ["cnt"] + derivable.pop("query_context", None) + + markup = copy.deepcopy(chart_config) + markup["uuid"] = "aaaaaaaa-0000-0000-0000-000000000011" + markup["slice_name"] = "Markup chart" + markup["viz_type"] = "markup" + markup["params"]["viz_type"] = "markup" + markup.pop("query_context", None) + + contents = { + "metadata.yaml": yaml.safe_dump(chart_metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset_config), + "charts/derivable_chart.yaml": yaml.safe_dump(derivable), + "charts/markup_chart.yaml": yaml.safe_dump(markup), + } + # --- RED anchor: mixed bundle imports without raising (FR-004) --- + ImportChartsCommand(contents, overwrite=True).run() + + derivable_chart: Slice = ( + db.session.query(Slice).filter_by(uuid=derivable["uuid"]).one() + ) + markup_chart: Slice = ( + db.session.query(Slice).filter_by(uuid=markup["uuid"]).one() + ) + # --- RED anchor: derivable → context; non-derivable → NULL (FR-003) --- + assert derivable_chart.query_context is not None + assert markup_chart.query_context is None Review Comment: Done — the isolation test now includes a chart whose synthesis actually raises (`big_number_total` with `adhoc_filters: 123`, a scalar rather than a list) and asserts the bundle still imports, both charts persist, and only the failing one keeps a NULL `query_context`, so the per-chart `except` is exercised. ########## tests/unit_tests/charts/commands/importers/v1/import_test.py: ########## @@ -500,6 +499,179 @@ def test_import_existing_active_chart_overwrite_without_can_write_returns_existi assert result.deleted_at is None +def test_import_chart_synthesizes_query_context( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + #33615 / F1-T2: importing a derivable chart that arrives WITHOUT a + query_context persists a synthesized one naming the resolved datasource. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + # Make the chart derivable and strip its persisted context. + config["viz_type"] = "table" + config["params"]["viz_type"] = "table" + config["params"]["metrics"] = ["count"] + config["params"]["groupby"] = ["gender"] + config.pop("query_context", None) + + chart = import_chart(config) + + # --- RED anchor: a synthesized context is persisted (FR-001) --- + assert chart.query_context is not None + query_context = json.loads(chart.query_context) + # --- RED anchor: datasource taken from resolved id, not params (RISK-T02) --- + assert query_context["datasource"] == {"id": 1, "type": "table"} + assert query_context["queries"][0]["metrics"] == ["count"] Review Comment: The generator/V8 path is gone in the current revision, so synthesis has a single path through the canonical `build_query_context_from_form_data` — there is no fallback to diverge from. `import_chart` persists `context["queries"]` verbatim (plus the local `slice_id` bind). A multi-query viz such as `mixed_timeseries` is emitted by the builder itself (it is on the allowlist) and its two queries are saved unchanged. ########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,224 @@ +# 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"}) + + +def _is_mappable_adhoc_filter(adhoc_filter: dict[str, Any]) -> bool: + """ + Whether ``split_adhoc_filters_into_base_filters`` will actually carry this + adhoc filter into the synthesized context. + + The shared splitter preserves only SIMPLE ``WHERE`` clauses (a subject + + operator) and SQL ``WHERE`` / ``HAVING`` clauses (a non-empty expression). + Anything else — a SIMPLE ``HAVING``, an unknown ``expressionType``, or a + SIMPLE filter missing its subject/operator — is silently discarded, which + would broaden the imported chart's result set. Such filters are treated as + unmappable so the caller can fail closed instead of querying more rows than + the chart defines. + """ + expression_type = adhoc_filter.get("expressionType") + clause = adhoc_filter.get("clause") + if expression_type == "SIMPLE": + return ( + clause == "WHERE" + and bool(adhoc_filter.get("subject")) + and bool(adhoc_filter.get("operator")) + ) + if expression_type == "SQL": + return clause in ("WHERE", "HAVING") and bool(adhoc_filter.get("sqlExpression")) + return False + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], str, str] | None: + """ + 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. + + Fails closed (#33615 review): if any adhoc filter would be silently dropped + by the splitter, the synthesized context would query a broader row set than + the chart defines. Returning ``None`` here makes the caller classify the + chart non-derivable and leave the ``query_context`` NULL — an honest 400 + until backfilled is safer than a wrong, broadened result. Non-dict junk is + still tolerated (dropped, never raised) so a stray serialization artifact + does not by itself void an otherwise sound chart (RISK-T05). + + Returns ``(filters, where, having)`` where ``where``/``having`` are the + parenthesized, comment-safe SQL strings ready for ``extras``, or ``None`` + when a real filter could not be preserved. + """ + # 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)] + if any(not _is_mappable_adhoc_filter(f) for f in sanitized): + return None + 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( Review Comment: Done in the rework — this module no longer reimplements a mapping; it delegates to `superset.common.form_data_query_context.build_query_context_from_form_data` (the one Excel export / MCP use). The World Map `secondary_metric` case you cited is handled by that builder's `_entity_map_query` adapter. -- 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]
