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 6a200efe589108e7e5905b9872fd70f03295ec9d Author: Eugene Olkhovski <[email protected]> AuthorDate: Tue Sep 22 09:14:32 2026 -0400 fix(charts): fail closed on unmappable filters, preserve cache_timeout, test backfill (#33615) Addresses the remaining query_context import review threads: - Fail closed on an adhoc filter the shared splitter would silently drop (a SIMPLE HAVING, an unknown expressionType, or a SIMPLE filter missing its subject/operator). Such a filter would broaden the synthesized context beyond what the chart defines, so the chart is now classified non-derivable and left with a NULL query_context (an honest 400 until backfilled) rather than a wrong, broadened result. Non-dict junk stays tolerated (dropped, never raised). - Re-inject the imported chart's own cache_timeout as custom_cache_timeout on the synthesized context. The context is bound to the datasource (the exported slice_id is dropped), so get_cache_timeout() cannot fall back to the chart's Slice.cache_timeout; without this the caching behavior silently reverts to the datasource/database default. - Add an integration test for the backfill-query-context CLI command (fills only NULL contexts, binds to the chart's own datasource, leaves existing ones). Verified locally: ruff check + ruff format, py_compile. (Superset pytest env not available on this machine; relying on CI to run the suite.) Co-Authored-By: Claude Opus 4.8 <[email protected]> --- superset/commands/chart/importers/v1/utils.py | 14 +++++ superset/commands/chart/query_context_builder.py | 52 ++++++++++++++-- tests/integration_tests/cli_tests.py | 71 ++++++++++++++++++++++ .../charts/commands/importers/v1/import_test.py | 32 ++++++++++ .../importers/v1/query_context_builder_test.py | 33 +++++++++- 5 files changed, 194 insertions(+), 8 deletions(-) diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index 407347ea189..727bf0b69e9 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -203,6 +203,20 @@ def _synthesize_query_context_if_absent(config: dict[str, Any]) -> None: ) if query_context_config is not None: + # The synthesized context is bound to the datasource, not a slice + # (the exported slice_id is dropped above), so at read time + # QueryContext.get_cache_timeout() cannot fall back to the chart's + # own Slice.cache_timeout. Re-inject the imported chart's + # cache_timeout as an explicit custom override so its caching + # behavior survives the import rather than silently reverting to the + # datasource/database default (#33615 review). Never overwrite a + # value a builder already supplied. + cache_timeout = config.get("cache_timeout") + if ( + isinstance(cache_timeout, int) + and "custom_cache_timeout" not in query_context_config + ): + query_context_config["custom_cache_timeout"] = cache_timeout config["query_context"] = json.dumps(query_context_config) logger.info( "Synthesized query_context for imported chart %s (queryable)", diff --git a/superset/commands/chart/query_context_builder.py b/superset/commands/chart/query_context_builder.py index 99b3561a5c8..635faaaa32c 100644 --- a/superset/commands/chart/query_context_builder.py +++ b/superset/commands/chart/query_context_builder.py @@ -48,9 +48,35 @@ from superset.utils.core import split_adhoc_filters_into_base_filters _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]: +) -> tuple[list[dict[str, Any]], str, str] | None: """ Translate viz ``adhoc_filters`` into base filters via the shared splitter. @@ -58,16 +84,25 @@ def _translate_adhoc_filters( 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. Malformed entries are dropped by the - shared splitter rather than raising (RISK-T05), so an imported chart never - aborts its bundle over a single unmappable filter. + 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``. + 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 ( @@ -147,7 +182,12 @@ def build_query_context_config( if not metrics and not columns: return None - filters, where, having = _translate_adhoc_filters(params.get("adhoc_filters", [])) + translated = _translate_adhoc_filters(params.get("adhoc_filters", [])) + # Fail closed: an adhoc filter could not be preserved, so a synthesized + # context would silently query a broader row set than the chart defines. + if translated is None: + return None + filters, where, having = translated query_object = { "time_range": params.get("time_range", " : "), diff --git a/tests/integration_tests/cli_tests.py b/tests/integration_tests/cli_tests.py index 85252cccdfd..2961d5fb46c 100644 --- a/tests/integration_tests/cli_tests.py +++ b/tests/integration_tests/cli_tests.py @@ -582,3 +582,74 @@ def test_failing_import_directory( ) assert_cli_fails_properly(response, caplog) + + [email protected]("load_birth_names_dashboard_with_slices") +def test_backfill_query_context_fills_only_null_contexts(app_context): + """ + ``backfill-query-context`` synthesizes a context for charts that have none, + binds it to the chart's own datasource, and leaves charts that already have + a context untouched (#33615). The V8 generator is stubbed to None so the test + exercises the deterministic Python fallback regardless of bundle availability. + """ + import superset.cli.charts # noqa: F811 + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + from superset.utils import json + + table = db.session.query(SqlaTable).filter_by(table_name="birth_names").one() + + null_chart = Slice( + slice_name="qc-backfill-null", + viz_type="table", + datasource_type="table", + datasource_id=table.id, + params=json.dumps( + { + "viz_type": "table", + "metrics": ["count"], + "datasource": f"{table.id}__table", + } + ), + query_context=None, + ) + kept_chart = Slice( + slice_name="qc-backfill-existing", + viz_type="table", + datasource_type="table", + datasource_id=table.id, + params=json.dumps({"viz_type": "table", "metrics": ["count"]}), + query_context='{"existing": true}', + ) + db.session.add_all([null_chart, kept_chart]) + db.session.commit() + + try: + with mock.patch( + "superset.commands.chart.query_context_generator." + "get_query_context_generator" + ) as get_generator: + get_generator.return_value.generate.return_value = None + runner = current_app.test_cli_runner() + response = runner.invoke(superset.cli.charts.backfill_query_context, ()) + + assert response.exit_code == 0, response.output + + db.session.refresh(null_chart) + db.session.refresh(kept_chart) + + # The empty chart gained a context bound to its own datasource ... + assert null_chart.query_context is not None + datasource = json.loads(null_chart.query_context)["datasource"] + datasource_id = ( + datasource["id"] + if isinstance(datasource, dict) + else int(str(datasource).split("__")[0]) + ) + assert datasource_id == table.id + # ... and the chart that already had a context is left untouched. + assert kept_chart.query_context == '{"existing": true}' + finally: + db.session.delete(null_chart) + db.session.delete(kept_chart) + db.session.commit() 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 76ea920f051..4ee6e47e076 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -589,6 +589,38 @@ def test_synthesis_drops_source_slice_id_from_generator_params( assert "slice_id" not in passed_params +def test_synthesis_reinjects_chart_cache_timeout( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + #33615 review: the synthesized context is bound to the datasource, not a + slice (the exported slice_id is dropped), so at read time + ``QueryContext.get_cache_timeout()`` cannot fall back to the chart's own + ``Slice.cache_timeout``. The imported chart's cache_timeout is re-injected as + an explicit ``custom_cache_timeout`` so its caching behavior survives import. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + # Force the pure-Python fallback so a real payload dict is produced. + mocker.patch( + "superset.commands.chart.importers.v1.utils.get_query_context_generator" + ).return_value.generate.return_value = None + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + config["viz_type"] = "table" + config["params"]["viz_type"] = "table" + config["params"]["metrics"] = ["count"] + config["cache_timeout"] = 3600 + config.pop("query_context", None) + + chart = import_chart(config) + + assert chart.query_context is not None + query_context = json.loads(chart.query_context) + assert query_context["custom_cache_timeout"] == 3600 + + def test_import_non_derivable_chart_leaves_query_context_null( mocker: MockerFixture, session_with_schema: Session ) -> None: 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 index 668461b3755..10bb6b95e20 100644 --- 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 @@ -207,7 +207,8 @@ def test_adhoc_simple_filter_translation() -> None: 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 + ``extras.having``); non-dict junk is tolerated (dropped, never raised) so a + stray serialization artifact does not void an otherwise sound chart (RISK-T05). """ params = { @@ -219,7 +220,6 @@ def test_adhoc_sql_filter_routed_to_extras_and_no_crash() -> None: "sqlExpression": "sum(num) > 1", "clause": "HAVING", }, - {"expressionType": "SIMPLE"}, # unmappable → dropped, no crash "not-a-dict", # junk → skipped, no crash ], } @@ -232,6 +232,35 @@ def test_adhoc_sql_filter_routed_to_extras_and_no_crash() -> None: assert query_context["queries"][0]["filters"] == [] +def test_unmappable_adhoc_filter_yields_none() -> None: + """ + Fail closed (#33615 review): an adhoc filter the shared splitter would + silently drop (here a SIMPLE filter with no subject/operator, and a SIMPLE + ``HAVING`` the splitter does not carry) makes the chart non-derivable rather + than synthesizing a context that queries a broader row set than defined. + """ + unmappable_cases = [ + [{"expressionType": "SIMPLE"}], # missing subject/operator + [ + { + "expressionType": "SIMPLE", + "subject": "num", + "operator": ">", + "comparator": 0, + "clause": "HAVING", # SIMPLE HAVING is dropped by the splitter + } + ], + [{"expressionType": "SQL", "sqlExpression": "", "clause": "WHERE"}], # empty + [ + {"expressionType": "SQL", "sqlExpression": "num > 0", "clause": "WHERE"}, + {"expressionType": "SIMPLE"}, # one bad filter voids the whole context + ], + ] + for adhoc_filters in unmappable_cases: + params = {"metrics": ["sum__num"], "adhoc_filters": adhoc_filters} + assert build_query_context_config(params, "table", 12, "table") is None + + def test_adhoc_sql_or_predicate_is_parenthesized() -> None: """ Multiple SQL WHERE predicates must be parenthesized before being AND-joined,
