sadpandajoe commented on code in PR #43303:
URL: https://github.com/apache/superset/pull/43303#discussion_r4220342143
##########
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:
The `markup` chart simply yields no derivation (it returns `None`), so this
passes even if the per-chart `except` in `_synthesize_query_context_if_absent`
is removed. The bundle-isolation guarantee (one chart's synthesis raising must
not abort the import) is never exercised. Could this add a chart whose
synthesis actually raises, for example `adhoc_filters: 123` on a
`big_number_total`, and assert the import still succeeds, both charts persist,
and only the failing one keeps a NULL `query_context`?
##########
superset-frontend/scripts/gen-qc-goldens.mjs:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.
+ */
+
+// Records frontend query_context goldens by running the SAME buildQuery entry
+// under Node's V8 (independent of the backend mini-racer runtime). Pure
buildQuery
+// logic → identical output to the browser. Writes
__fixtures__/expected/<stem>.json.
+import { build } from 'esbuild';
+import { readFileSync, writeFileSync, readdirSync, mkdirSync } from 'node:fs';
+import { createRequire } from 'node:module';
+
+const require = createRequire(import.meta.url);
+const FEX = 'src/backend-querycontext/__fixtures__';
+
+const res = await build({
Review Comment:
This esbuild call has no `loader` map, but `build-backend-querycontext.mjs`
needed `.css`/`.svg`/font/image entries mapped to `empty` because the
`entry.ts` barrel imports transitively pull in theme and UI assets. As written,
running `node scripts/gen-qc-goldens.mjs` (and `gen-qc-fixtures.mjs:31`, which
has the same config) should fail while bundling, so the goldens and fixtures
can't be refreshed when a `buildQuery` changes. Could these scripts share the
production script's loader config?
##########
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:
This test only checks `queries[0]` against a chart that falls back to the
Python derivation when the V8 runtime isn't installed, and the other synthesis
tests force the generator to return `None`. No import test has the generator
return a real payload, so an importer that rewrote or dropped authoritative
output (for example a `mixed_timeseries` with two queries) would still pass.
Could one test mock `generate()` to return a two-query context and assert
`import_chart` persists it unchanged without calling the fallback?
--
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]
