sadpandajoe commented on code in PR #43303:
URL: https://github.com/apache/superset/pull/43303#discussion_r4445238869
##########
tests/unit_tests/charts/commands/importers/v1/import_test.py:
##########
@@ -546,6 +546,144 @@ 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)
+
+
Review Comment:
This preservation test uses the default `chart_config` (`deck_path`), which
the new allowlist treats as non-derivable, so synthesis returns nothing for it
whether or not the "existing context" guard in
`_synthesize_query_context_if_absent` is there. Deleting that guard would leave
this test green. Could this use a derivable viz (e.g. `table` with
metrics/groupby) carrying a deliberately different `query_context`, and assert
its queries survive unchanged?
##########
MANIFEST.in:
##########
@@ -24,4 +24,8 @@ recursive-include superset/migrations *
recursive-include superset/templates *
recursive-include superset/translations *
recursive-include superset/static *
+# Backend query_context parity bundle (#33615): a build artifact produced by
+# `npm run build` (postbuild). Ship it so deployments get exact frontend parity
+# by default instead of silently falling back to the generic derivation.
+recursive-include superset/commands/chart/_bundles *.js
Review Comment:
This ships `superset/commands/chart/_bundles/*.js`, but that directory and
the `npm run build` postbuild step that produced it no longer exist at this
head (synthesis now goes through the Python builder only). The same leftovers
are the matching `.gitignore` entry and the `backfill-query-context` docstring
in `superset/cli/charts.py`, which still tells operators `--help` that the
frontend `buildQuery` runs via V8 when available. Operators reading that will
expect frontend-exact parity that this code can't provide — should these three
leftovers be removed so the packaging and help text match the Python-only
behavior?
##########
tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py:
##########
@@ -0,0 +1,123 @@
+# 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.
+"""
+Pure-classification tests for ``query_context_builder`` (Apache Superset
#33615).
+
+These cover the honest-fail gates that run *before* the canonical builder is
+invoked (so they need no app context): the viz-type allowlist, the
+dashboard-context-filter guard, and the adhoc ``TEMPORAL_RANGE`` preservation
+that stops a builder-injected ``"No filter"`` from broadening the result.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+import pytest
+
+from superset.commands.chart.query_context_builder import (
+ _preserve_adhoc_temporal_range,
+ bind_slice_id,
+ build_query_context_config,
+)
+
+
[email protected]("viz_type", ["sankey_v2", "graph_chart", "sankey",
"nope"])
+def test_non_mapped_viz_type_is_non_derivable(viz_type: str) -> None:
+ """
+ A viz type the canonical builder has no explicit adapter for is left NULL.
+
+ The generic top-N branch would emit a plausible-looking but wrong context
+ (one aggregate row instead of edges), so synthesis must honest-fail rather
+ than persist it (#33615 review). Returns None *before* the builder runs, so
+ no app context is required.
+ """
+ params = {"viz_type": viz_type, "metric": "count", "groupby": ["source"]}
+ assert build_query_context_config(params, viz_type, 1, "table") is None
+
+
[email protected](
+ "params",
+ [
+ {"viz_type": "table", "extra_filters": [{"col": "country", "val":
"US"}]},
+ {
+ "viz_type": "table",
+ "extra_form_data": {"filters": [{"col": "country", "val": "US"}]},
+ },
+ {"viz_type": "table", "extra_form_data": {"time_range": "Last week"}},
+ ],
+)
+def test_dashboard_context_filters_are_non_derivable(params: dict[str, Any])
-> None:
+ """Charts carrying ``extra_filters`` / ``extra_form_data`` are left
NULL."""
+ assert build_query_context_config(params, "table", 1, "table") is None
+
+
+def test_missing_datasource_and_datasourceless_viz_are_non_derivable() -> None:
+ assert build_query_context_config({"viz_type": "table"}, "table", None) is
None
+ assert build_query_context_config({"viz_type": "markup"}, "markup", 1) is
None
+
+
+def test_preserve_adhoc_temporal_range_drops_injected_no_filter() -> None:
+ """A bound TEMPORAL_RANGE filter must survive the injected ``"No
filter"``."""
+ context = {
+ "queries": [
+ {
+ "time_range": "No filter",
+ "filters": [
+ {
+ "col": "ds",
+ "op": "TEMPORAL_RANGE",
+ "val": "2020-01-01 : 2020-02-01",
+ }
+ ],
+ }
+ ]
+ }
+ _preserve_adhoc_temporal_range(context)
Review Comment:
These tests call `_preserve_adhoc_temporal_range` directly, so removing its
call from `build_query_context_config` would still pass them. A chart with a
bound `TEMPORAL_RANGE` adhoc filter and no top-level `time_range` would then
get the builder's injected `"No filter"` back and be read as unbounded. Could
one test run `build_query_context_config` on such form data and assert the
filter's range survives with no query-level `time_range`, plus a control with
an explicit `time_range`?
--
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]
