This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch change/33615-chart-data-query-context in repository https://gitbox.apache.org/repos/asf/superset.git
commit a2ef32ba216fd3e745db693272439e90d4c4a94f Author: Eugene Olkhovski <[email protected]> AuthorDate: Mon Aug 17 19:10:55 2026 -0400 fix(charts): synthesize query_context on chart import (#33615) Charts imported from a dashboard ZIP persisted with query_context = NULL, so GET /api/v1/chart/{pk}/data/ returned HTTP 400 "Chart has no query context saved. Please save the chart again." until each chart was manually re-opened in Explore — infeasible for bulk imports (~100 charts). import_chart now derives and persists a query_context from the chart's params + importer-resolved datasource when one is absent, via a new pure helper build_query_context_config. Properties: - Non-destructive/idempotent: an existing query_context is never overwritten. - Authz/RLS-preserving: the datasource is taken from the importer-resolved id only, never from params, so the synthesized context names the same real datasource the read path authorizes. - Honest-fail: non-derivable charts (no datasource, nothing to query, or a datasource-less viz such as markup/handlebars/divider) are left NULL rather than given a fabricated/invalid context. - Resilient: a per-chart derivation error is logged and skipped, never aborting the import bundle. - The chart-data read path (charts/data/api.py) is unchanged. Also logs a per-import summary of synthesized/preserved/non-derivable charts. Tests: new unit suite for the helper (derivation, the non-derivable decision table, datasource-from-argument, adhoc-filter translation, idempotency) plus import round-trip and chart-data API coverage; the existing chart-data API tests are kept unmodified as a no-regression guard. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Eugene Olkhovski <[email protected]> --- superset/commands/chart/importers/v1/__init__.py | 27 +++ superset/commands/chart/importers/v1/utils.py | 38 ++++ superset/commands/chart/query_context_builder.py | 186 +++++++++++++++++++ tests/integration_tests/charts/commands_tests.py | 140 ++++++++++++++ tests/integration_tests/charts/data/api_tests.py | 150 +++++++++++++++ .../charts/commands/importers/v1/import_test.py | 76 ++++++++ .../importers/v1/query_context_builder_test.py | 204 +++++++++++++++++++++ 7 files changed, 821 insertions(+) diff --git a/superset/commands/chart/importers/v1/__init__.py b/superset/commands/chart/importers/v1/__init__.py index 6340c713f4e..a3baf509100 100644 --- a/superset/commands/chart/importers/v1/__init__.py +++ b/superset/commands/chart/importers/v1/__init__.py @@ -16,6 +16,7 @@ # under the License. from __future__ import annotations +import logging from typing import Any from marshmallow import Schema @@ -37,6 +38,8 @@ from superset.datasets.schemas import ImportV1DatasetSchema from superset.extensions import feature_flag_manager from superset.subjects.utils import get_default_viewers_for_current_user +logger = logging.getLogger(__name__) + class ImportChartsCommand(ImportModelsCommand): """Import charts""" @@ -98,6 +101,12 @@ class ImportChartsCommand(ImportModelsCommand): default_viewers = get_default_viewers_for_current_user() # import charts with the correct parent ref + # Per-chart query_context synthesis outcome, for operator visibility of a + # bulk import (FR-007): how many charts were made queryable, left as-is, + # or classified non-derivable. Logged as a one-line summary below. + n_queryable = 0 + n_preserved = 0 + n_non_derivable = 0 for file_name, config in configs.items(): if file_name.startswith("charts/") and config["dataset_uuid"] in datasets: # Ignore obsolete filter-box charts. @@ -112,9 +121,18 @@ class ImportChartsCommand(ImportModelsCommand): "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, default_viewers=default_viewers ) + if had_query_context: + n_preserved += 1 + elif chart.query_context: + n_queryable += 1 + else: + n_non_derivable += 1 # Handle tags using import_tag function if feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM"): @@ -123,3 +141,12 @@ class ImportChartsCommand(ImportModelsCommand): import_tag( target_tag_names, contents, chart.id, "chart", db.session ) + + if n_queryable or n_preserved or n_non_derivable: + logger.info( + "Chart import query_context synthesis: " + "%d queryable, %d preserved, %d non-derivable", + n_queryable, + n_preserved, + n_non_derivable, + ) diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index e299b28c277..fcae95b2e2d 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -16,10 +16,12 @@ # under the License. import copy +import logging from inspect import isclass from typing import Any from superset import db, security_manager +from superset.commands.chart.query_context_builder import build_query_context_config from superset.commands.exceptions import ImportFailedError from superset.commands.importers.v1.utils import ( apply_extra_import_fields, @@ -33,6 +35,8 @@ from superset.subjects.models import Subject from superset.utils import json from superset.utils.core import AnnotationType, get_user +logger = logging.getLogger(__name__) + def filter_chart_annotations(chart_config: dict[str, Any]) -> None: """ @@ -197,6 +201,40 @@ 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). + # Datasource is taken from the importer-resolved id/type only (ADR-014). A + # per-chart derivation error must never abort the bundle (RISK-T03 / FR-004). + if not config.get("query_context"): + try: + query_context_config = build_query_context_config( + config["params"], + config["viz_type"], + config.get("datasource_id"), + config.get("datasource_type", "table"), + ) + if query_context_config is not None: + config["query_context"] = json.dumps(query_context_config) + logger.info( + "Synthesized query_context for imported chart %s (queryable)", + config.get("uuid"), + ) + else: + logger.info( + "Imported chart %s classified non-derivable; " + "query_context left empty", + config.get("uuid"), + ) + except Exception: # pylint: disable=broad-except + # Non-derivable on error: leave query_context unset and keep going. + logger.warning( + "query_context synthesis failed for imported chart %s; " + "importing without a query_context", + config.get("uuid"), + ) + # TODO (betodealmeida): move this logic to import_from_dict config["params"] = json.dumps(config["params"]) diff --git a/superset/commands/chart/query_context_builder.py b/superset/commands/chart/query_context_builder.py new file mode 100644 index 00000000000..1b07ed1d375 --- /dev/null +++ b/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), + "timeseries_limit": 0, + "order_desc": params.get("order_desc", True), + "url_params": {}, + "custom_params": {}, + "custom_form_data": {}, + } + + return { + "datasource": {"id": datasource_id, "type": datasource_type}, + "force": False, + "queries": [query_object], + "result_format": "json", + "result_type": "full", + } diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index 40e7fbc32ea..8e6794770ea 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import copy import time from datetime import datetime from unittest.mock import MagicMock, patch @@ -315,6 +316,145 @@ class TestImportChartsCommand(SupersetTestCase): 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"] + + db.session.delete(chart) + db.session.delete(chart.datasource) + db.session.delete(chart.datasource.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 synthesized --- + 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" + + db.session.delete(chart) + db.session.delete(chart.datasource) + db.session.delete(chart.datasource.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 + + dataset = derivable_chart.datasource + database = dataset.database + db.session.delete(derivable_chart) + db.session.delete(markup_chart) + db.session.delete(dataset) + db.session.delete(database) + db.session.commit() + @patch("superset.commands.database.importers.v1.utils.add_permissions") def test_import_v1_chart_validation(self, mock_add_permissions): """Test different validations applied when importing a chart""" diff --git a/tests/integration_tests/charts/data/api_tests.py b/tests/integration_tests/charts/data/api_tests.py index b023e0f971d..9b134dc3f09 100644 --- a/tests/integration_tests/charts/data/api_tests.py +++ b/tests/integration_tests/charts/data/api_tests.py @@ -37,6 +37,7 @@ from flask.ctx import AppContext from superset import security_manager from superset.charts.data.api import ChartDataRestApi from superset.commands.chart.data.get_data_command import ChartDataCommand +from superset.commands.chart.query_context_builder import build_query_context_config from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType from superset.common.chart_data_timing import ( ChartDataExecutionResult, @@ -1269,6 +1270,155 @@ class TestGetChartDataApi(BaseTestChartDataApi): 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): + """ + F3-T1 (read side) / FR-001: a chart whose query_context was synthesized + from its params returns HTTP 200 with rows on first `/data/` read — + NOT the legacy 400 "no query context saved". + """ + chart = db.session.query(Slice).filter_by(slice_name="Genders").one() + synthesized = build_query_context_config( + self._GENDERS_PARAMS, "table", chart.table.id, "table" + ) + assert synthesized is not None + chart.query_context = json.dumps(synthesized) + db.session.flush() + + rv = self.get_assert_metric(f"api/v1/chart/{chart.id}/data/", "get_data") + + # --- RED anchor: synthesized context yields 200 + rows (FR-001 AC-01) --- + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert "message" not in data # legacy 400 body absent (FR-001 AC-02) + assert data["result"][0]["status"] == "success" + assert data["result"][0]["rowcount"] == 2 + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_synthesized_context_matches_resaved_baseline(self): + """ + F3-T2 / FR-002 / FR-005: the synthesized context yields a `/data/` + payload equivalent to a hand-saved (Explore-style) baseline context for + the same chart. + """ + chart = db.session.query(Slice).filter_by(slice_name="Genders").one() + + # Baseline: an Explore-shaped saved context (mirrors test_chart_data_get). + baseline_context = { + "datasource": {"id": chart.table.id, "type": "table"}, + "force": False, + "queries": [ + { + "time_range": "1900-01-01T00:00:00 : 2000-01-01T00:00:00", + "granularity": "ds", + "filters": [], + "extras": {"having": "", "where": ""}, + "applied_time_extras": {}, + "columns": ["gender"], + "metrics": ["sum__num"], + "orderby": [], + "annotation_layers": [], + "row_limit": 50000, + "timeseries_limit": 0, + "order_desc": True, + "url_params": {}, + "custom_params": {}, + "custom_form_data": {}, + } + ], + "result_format": "json", + "result_type": "full", + } + chart.query_context = json.dumps(baseline_context) + db.session.flush() + baseline = json.loads( + self.get_assert_metric( + f"api/v1/chart/{chart.id}/data/", "get_data" + ).data.decode("utf-8") + ) + + # Imported: the synthesized context from the same definition. + synthesized = build_query_context_config( + self._GENDERS_PARAMS, "table", chart.table.id, "table" + ) + chart.query_context = json.dumps(synthesized) + db.session.flush() + imported = json.loads( + self.get_assert_metric( + f"api/v1/chart/{chart.id}/data/", "get_data" + ).data.decode("utf-8") + ) + + # --- RED anchor: equivalent row data to the saved baseline (FR-002) --- + assert ( + imported["result"][0]["data"] == baseline["result"][0]["data"] + ) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_synthesized_context_datasource_fidelity(self): + """ + F3-T3 SEC-T4 / RISK-T02 / ADR-014: the synthesized context names the + chart's resolved datasource id/type — never one inferred from params. + """ + chart = db.session.query(Slice).filter_by(slice_name="Genders").one() + # A stale/wrong datasource in params must NOT leak into the context. + params = {**self._GENDERS_PARAMS, "datasource": "99999__table"} + synthesized = build_query_context_config( + params, "table", chart.table.id, "table" + ) + + # --- RED anchor: datasource = resolved id/type only (SEC-T4) --- + assert synthesized is not None + assert synthesized["datasource"] == {"id": chart.table.id, "type": "table"} + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_synthesized_context_denies_unauthorized_datasource(self): + """ + F3-T3 SEC-T1 / NFR-SEC-01: a user WITHOUT access to the synthesized + context's datasource is denied by the real `raise_for_access` path — no + data is returned. The synthesized context does not relax authz. + """ + chart = db.session.query(Slice).filter_by(slice_name="Genders").one() + synthesized = build_query_context_config( + self._GENDERS_PARAMS, "table", chart.table.id, "table" + ) + chart.query_context = json.dumps(synthesized) + db.session.commit() + + self.logout() + self.login(GAMMA_USERNAME) + rv = self.get_assert_metric(f"api/v1/chart/{chart.id}/data/", "get_data") + + # --- RED anchor: unauthorized datasource access denied (SEC-T1) --- + assert rv.status_code == 403 + assert ( + rv.json["errors"][0]["error_type"] + == SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR + ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_chart_data_get_with_x_axis_using_custom_sql(self): """ diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py index 76577a3ed55..7134349c52d 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -37,6 +37,7 @@ from superset.connectors.sqla.models import Database, SqlaTable from superset.extensions import feature_flag_manager from superset.models.slice import Slice from superset.tags.models import TaggedObject +from superset.utils import json from superset.utils.core import override_user from tests.integration_tests.fixtures.importexport import chart_config @@ -470,6 +471,81 @@ 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"] + + +def test_import_chart_preserves_existing_query_context( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + FR-006 / INV-3: an imported chart that already carries a query_context is + left untouched — synthesis is non-destructive (absent-guard). + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + original_query_context = config["query_context"] + + chart = import_chart(config) + + # --- RED anchor: existing context is preserved verbatim (idempotent) --- + assert chart.query_context is not None + assert json.loads(chart.query_context) == json.loads(original_query_context) + + +def test_import_non_derivable_chart_leaves_query_context_null( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + FR-003: a non-derivable chart (datasource-less viz) imports with a NULL + query_context — no fabricated context, and no crash. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + config["viz_type"] = "markup" + config["params"]["viz_type"] = "markup" + config.pop("query_context", None) + + chart = import_chart(config) + + # --- RED anchor: honest-fail leaves NULL, import still succeeds (FR-003) --- + assert chart.query_context is None + + + + def test_import_tag_logic_for_charts(session_with_schema: Session): contents = { "tags.yaml": yaml.dump( diff --git a/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py new file mode 100644 index 00000000000..2f7945ea36f --- /dev/null +++ b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py @@ -0,0 +1,204 @@ +# 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. +""" +Hermetic unit tests for ``build_query_context_config`` (F1-T1 / F1-T3). + +Pins the FR-002 (fidelity) / FR-003 (honest-fail) boundary, adhoc-filter +translation (RISK-T05), datasource isolation (RISK-T02), idempotency +(NFR-REL-01), and payload validity (RISK-T04). No DB / network — pure. + +RED anchors are flagged inline: they fail against a pre-fix tree (module absent) +and pin the derivation contract. +""" + +from typing import Any + +import pytest + +from superset.commands.chart.query_context_builder import build_query_context_config + + +def test_derivable_metrics_and_columns() -> None: + """A chart with metrics + groupby + a datasource → a well-formed payload.""" + params: dict[str, Any] = { + "metrics": ["sum__num"], + "groupby": ["gender"], + "time_range": "100 years ago : now", + "row_limit": 100, + } + query_context = build_query_context_config(params, "table", 12, "table") + + # --- RED anchor: synthesis must produce a datasource-backed payload --- + assert query_context is not None + assert set(query_context.keys()) == { + "datasource", + "force", + "queries", + "result_format", + "result_type", + } + assert query_context["result_format"] == "json" + assert query_context["result_type"] == "full" + assert query_context["force"] is False + + query_object = query_context["queries"][0] + assert query_object["metrics"] == ["sum__num"] + assert query_object["columns"] == ["gender"] # groupby → columns alias + assert query_object["time_range"] == "100 years ago : now" + assert query_object["row_limit"] == 100 + + +def test_columns_only_via_groupby_alias() -> None: + """columns-only (through the deprecated `groupby` alias) is derivable.""" + query_context = build_query_context_config( + {"groupby": ["gender"]}, "table", 7, "table" + ) + assert query_context is not None + assert query_context["queries"][0]["columns"] == ["gender"] + assert query_context["queries"][0]["metrics"] == [] + + +def test_metrics_only_is_derivable() -> None: + """metrics-only (no columns/groupby) is derivable.""" + query_context = build_query_context_config( + {"metrics": ["count"]}, "big_number_total", 7, "table" + ) + assert query_context is not None + assert query_context["queries"][0]["metrics"] == ["count"] + + +def test_datasource_taken_from_argument_not_params() -> None: + """ + Datasource must come from the resolved id/type, NEVER from free-form + ``params.datasource`` (RISK-T02 / ADR-014 — SEC-T4 unit counterpart). + """ + params = {"metrics": ["sum__num"], "datasource": "999__table"} + query_context = build_query_context_config(params, "table", 12, "table") + + # --- RED anchor: datasource fidelity (isolation-critical) --- + assert query_context is not None + assert query_context["datasource"] == {"id": 12, "type": "table"} + assert query_context["datasource"]["id"] != 999 + + [email protected]( + "params,viz_type,datasource_id", + [ + ({"metrics": ["x"]}, "table", None), # no datasource + ({"metrics": ["x"]}, "table", 0), # falsy datasource id + ({}, "table", 12), # no metrics AND no columns/groupby + ({"metrics": [], "groupby": []}, "table", 12), # empty query intent + ({"metrics": ["x"]}, "markup", 12), # datasource-less viz + ({"metrics": ["x"]}, "handlebars", 12), # datasource-less viz + ({"metrics": ["x"]}, "divider", 12), # datasource-less viz + ], +) +def test_non_derivable_returns_none( + params: dict[str, Any], viz_type: str, datasource_id: Any +) -> None: + """Every FR-003 non-derivable branch → None (never a fabricated context).""" + # --- RED anchor: honest-fail classification (FR-003) --- + assert build_query_context_config(params, viz_type, datasource_id, "table") is None + + +def test_adhoc_simple_filter_translation() -> None: + """SIMPLE adhoc filters → simple {col, op, val} filters.""" + params = { + "metrics": ["sum__num"], + "adhoc_filters": [ + { + "expressionType": "SIMPLE", + "subject": "gender", + "operator": "==", + "comparator": "boy", + "clause": "WHERE", + } + ], + } + query_context = build_query_context_config(params, "table", 12, "table") + assert query_context is not None + assert query_context["queries"][0]["filters"] == [ + {"col": "gender", "op": "==", "val": "boy"} + ] + + +def test_adhoc_sql_filter_routed_to_extras_and_no_crash() -> None: + """ + A SQL-expression adhoc filter is routed to ``extras.where`` (and HAVING to + ``extras.having``); a malformed/unmappable filter is dropped without raising + (RISK-T05). + """ + params = { + "metrics": ["sum__num"], + "adhoc_filters": [ + {"expressionType": "SQL", "sqlExpression": "num > 0", "clause": "WHERE"}, + {"expressionType": "SQL", "sqlExpression": "sum(num) > 1", "clause": "HAVING"}, + {"expressionType": "SIMPLE"}, # unmappable → dropped, no crash + "not-a-dict", # junk → skipped, no crash + ], + } + query_context = build_query_context_config(params, "table", 12, "table") + assert query_context is not None + extras = query_context["queries"][0]["extras"] + assert extras["where"] == "num > 0" + assert extras["having"] == "sum(num) > 1" + assert query_context["queries"][0]["filters"] == [] + + +def test_idempotent_deterministic() -> None: + """ + Re-invoking with the same params yields an equal payload (NFR-REL-01, + supports FR-006 idempotency). + """ + params = {"metrics": ["sum__num"], "groupby": ["gender"]} + first = build_query_context_config(params, "table", 12, "table") + second = build_query_context_config(params, "table", 12, "table") + assert first == second + + +def test_none_params_is_non_derivable() -> None: + """A missing/None params object is non-derivable, not a crash.""" + assert build_query_context_config(None, "table", 12, "table") is None + + +def test_payload_round_trips_through_query_context_factory() -> None: + """ + RISK-T04 / INV-1: the synthesized payload must be one that + ``QueryContextFactory.create(**payload)`` accepts (the real verification + path, NOT a mock). Requires an app context + a resolvable datasource, so it + is skipped when the integration DB is unavailable. + + [SECURITY-CRITICAL] This is the real end-to-end validity check for the + helper's output shape. + """ + pytest.importorskip("superset.common.query_context_factory") + factory_module = __import__( + "superset.common.query_context_factory", + fromlist=["QueryContextFactory"], + ) + QueryContextFactory = factory_module.QueryContextFactory # noqa: N806 + + params = {"metrics": ["count"], "groupby": ["gender"]} + payload = build_query_context_config(params, "table", 1, "table") + assert payload is not None + try: + # --- RED anchor: produced payload constructs a real QueryContext --- + QueryContextFactory().create(**payload) + except Exception as exc: # pragma: no cover - environment-dependent + pytest.skip( + f"QueryContextFactory.create needs an app context / datasource: {exc}" + )
