sadpandajoe commented on code in PR #43303:
URL: https://github.com/apache/superset/pull/43303#discussion_r4195010203
##########
superset-frontend/src/backend-querycontext/__fixtures__/expected/word_cloud.json:
##########
@@ -0,0 +1,63 @@
+{
+ "datasource": {
+ "id": 1,
+ "type": "table"
+ },
+ "force": false,
+ "queries": [
+ {
+ "time_range": "No filter",
+ "granularity": "ds",
+ "filters": [],
+ "extras": {
+ "time_grain_sqla": "P1D",
+ "having": "",
+ "where": ""
+ },
+ "applied_time_extras": {},
+ "columns": ["gender", "name"],
+ "metrics": ["count"],
+ "annotation_layers": [],
+ "row_limit": 100,
+ "series_columns": ["gender"],
+ "series_limit": 0,
+ "order_desc": true,
Review Comment:
This golden has no `group_others_when_limit_reached`, but
`buildQueryObject.ts` always emits `group_others_when_limit_reached: false`,
and none of the goldens contain it. `parity.test.ts` rewrites these files on
every run rather than comparing them, and the Python parity test is skipped in
CI (no `querycontext` extra, no bundle). So the committed goldens are already
stale and nothing detects it; once someone enables the Python parity test,
`actual == expected` fails for word_cloud. Could the goldens be regenerated at
this head and the Python parity test run in a CI job that has the bundle and
extra installed?
##########
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(
+ 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 []
+ # Single-metric viz types (e.g. Big Number) persist the metric under the
+ # singular `metric` key; normalize it into `metrics` so those charts are
not
+ # misclassified as non-derivable (#33615: Big Number left with a NULL
context).
+ if not metrics and params.get("metric"):
+ metrics = [params["metric"]]
+ # `groupby` is the deprecated alias of `columns`.
+ columns = params.get("columns") or params.get("groupby") or []
Review Comment:
Without the V8 bundle, any chart that has a `metric` or `groupby` gets a
context saved, but that context ignores viz-specific semantics. For example,
the bundled `Big_Number_with_Trendline.yaml` has `x_axis: order_date` and
`metric: count`. The fallback ignores `x_axis` and never sets `is_timeseries`,
so the saved-chart data read returns a single aggregate instead of the monthly
trendline series. A `big_number` with `aggregation: LAST_VALUE` returns the
period total instead of the last bucket. Pie with `threshold_for_other` is
missing the contribution post-processing, so the "Other" grouping never
happens. Because the context is persisted, installing the V8 extra later does
not repair it: both synthesis and the backfill only act on a missing context.
Could the fallback return non-derivable for viz types or shapes whose semantics
it cannot reproduce (time axis, aggregation, post-processing), and keep the
generic mapping for the shapes it does cover?
##########
Dockerfile:
##########
@@ -270,6 +270,12 @@ COPY --from=superset-node
/app/superset/static/service-worker.j[s] superset/stat
# TODO, when the next version comes out, use --exclude superset/translations
COPY superset superset
+# Overlay the backend query_context V8 bundle built in the node stage (#33615).
+# Optional glob (`[s]`): the bundle only exists when the frontend was built
+# (DEV_MODE=false); without it the chart importer falls back to the pure-Python
+# generic derivation. Must come after `COPY superset superset` so it is not
+# clobbered by the source tree (which never contains the generated bundle).
+COPY --from=superset-node
/app/superset/commands/chart/_bundles/query_context_bundle.j[s]
superset/commands/chart/_bundles/query_context_bundle.js
Review Comment:
This copies the V8 bundle into the image, but nothing in the image installs
the `querycontext` extra: the lean stage runs `uv pip install -e . --no-deps`
and the default stage installs only `.[postgres,mysql,fastmcp]`.
`_ensure_ctx()` therefore fails its `py_mini_racer` import in every published
image, so the bundle is never used and imports silently use the approximate
Python fallback. Is the intent to also install `.[querycontext]` in the image,
or should the bundle copy be dropped until the runtime dependency ships?
##########
tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py:
##########
@@ -0,0 +1,354 @@
+# 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_singular_metric_is_normalized_and_derivable() -> None:
+ """
+ Single-metric viz types (e.g. Big Number) persist the metric under the
+ singular ``metric`` key. It must be normalized into ``metrics`` so the
chart
+ is derivable rather than classified non-derivable (#33615 review: Big
Number
+ left with a NULL query_context and a 400 data endpoint).
+ """
+ query_context = build_query_context_config(
+ {"metric": "count"}, "big_number", 7, "table"
+ )
+ assert query_context is not None
+ assert query_context["queries"][0]["metrics"] == ["count"]
+
+
+def test_plural_metrics_take_precedence_over_singular() -> None:
+ """When both keys exist, the plural ``metrics`` list wins (no
duplication)."""
+ query_context = build_query_context_config(
+ {"metrics": ["sum__num"], "metric": "count"}, "table", 7, "table"
+ )
+ assert query_context is not None
+ assert query_context["queries"][0]["metrics"] == ["sum__num"]
+
+
+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"]}, "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_handlebars_is_derivable() -> None:
+ """
+ `handlebars` renders a template over query results — it has a real
buildQuery
+ and a parity golden — so the fallback must derive it too. Classifying it
+ datasource-less left imported handlebars charts with a null context that
400s
+ when the V8 bundle is unavailable (#33615 review).
+ """
+ query_context = build_query_context_config(
+ {"metrics": ["count"]}, "handlebars", 7, "table"
+ )
+ assert query_context is not None
+ assert query_context["queries"][0]["metrics"] == ["count"]
+
+
+def test_raw_mode_all_columns_is_derivable() -> None:
+ """
+ A raw-mode table (``query_mode: raw``) carries no metrics/groupby and
selects
+ row-level ``all_columns``. It must be derivable from ``all_columns`` rather
+ than classified non-derivable — the bundled raw ``Table.yaml`` otherwise
kept
+ a null query_context and 400d on its data endpoint (#33615 review).
+ """
+ params = {
+ "query_mode": "raw",
+ "metrics": [],
+ "groupby": [],
+ "all_columns": ["gender", "name"],
+ }
+ query_context = build_query_context_config(params, "table", 7, "table")
+ assert query_context is not None
+ assert query_context["queries"][0]["columns"] == ["gender", "name"]
+ assert query_context["queries"][0]["metrics"] == []
+
+
+def test_row_limit_absent_left_unset_for_runtime_default() -> None:
+ """
+ When the chart specifies no ``row_limit`` the payload must leave it unset
so
+ the read path applies the runtime-configured ``ROW_LIMIT``, rather than
baking
+ in a hard-coded cap that silently truncates imported charts (#33615
review).
+ """
+ query_context = build_query_context_config(
+ {"metrics": ["count"]}, "table", 7, "table"
+ )
+ assert query_context is not None
+ assert query_context["queries"][0]["row_limit"] 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``); non-dict junk is tolerated (dropped, never raised) so a
+ stray serialization artifact does not void an otherwise sound chart
+ (RISK-T05).
+ """
+ params = {
+ "metrics": ["sum__num"],
+ "adhoc_filters": [
+ {"expressionType": "SQL", "sqlExpression": "num > 0", "clause":
"WHERE"},
+ {
+ "expressionType": "SQL",
+ "sqlExpression": "sum(num) > 1",
+ "clause": "HAVING",
+ },
+ "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"]
+ # Composed via the shared splitter: each predicate is parenthesized.
+ assert extras["where"] == "(num > 0)"
+ assert extras["having"] == "(sum(num) > 1)"
+ 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,
+ like ``split_adhoc_filters_into_base_filters`` (#33615 review). Without the
+ parentheses, ``status='a' OR status='b'`` AND ``type='x'`` would bind as
+ ``status='a' OR (status='b' AND type='x')`` and change the result set.
+ """
+ params = {
+ "metrics": ["sum__num"],
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "sqlExpression": "status = 'a' OR status = 'b'",
+ "clause": "WHERE",
+ },
+ {"expressionType": "SQL", "sqlExpression": "type = 'x'", "clause":
"WHERE"},
+ ],
+ }
+ query_context = build_query_context_config(params, "table", 12, "table")
+ assert query_context is not None
+ assert query_context["queries"][0]["extras"]["where"] == (
+ "(status = 'a' OR status = 'b') AND (type = 'x')"
+ )
+
+
+def test_adhoc_sql_trailing_comment_is_neutralized() -> None:
+ """
+ A trailing ``--`` line comment must not swallow predicates joined after it;
+ the shared splitter appends a newline so the following ``AND`` survives.
+ """
+ params = {
+ "metrics": ["sum__num"],
+ "adhoc_filters": [
+ {
+ "expressionType": "SQL",
+ "sqlExpression": "a = 1 -- note",
+ "clause": "WHERE",
+ },
+ {"expressionType": "SQL", "sqlExpression": "b = 2", "clause":
"WHERE"},
+ ],
+ }
+ query_context = build_query_context_config(params, "table", 12, "table")
+ assert query_context is not None
+ where = query_context["queries"][0]["extras"]["where"]
+ # The newline keeps `b = 2` from being commented out.
+ assert "\n) AND (b = 2)" in where
+
+
+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
Review Comment:
The broad `except Exception` turns any failure of
`QueryContextFactory().create(**payload)` into `pytest.skip`. The test uses
datasource id `1` with no resolvable datasource, so a real payload-shape error
(a `TypeError` from an unexpected kwarg, for instance) skips instead of
failing, and the "real end-to-end validity check" can never go red. Could the
test create a datasource with the unit-test session fixture and let
construction errors fail, asserting the resolved datasource and the query
fields?
##########
superset-frontend/scripts/gen-qc-registry.mjs:
##########
@@ -0,0 +1,281 @@
+/**
+ * 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.
+ */
+
+// Deterministic, re-runnable codegen: maps every plugin `buildQuery` module
to the
+// viz_type key(s) it is registered under, and emits registry.generated.ts
consumed
+// by entry.ts. Join: buildQuery module <- (index.ts that imports it) ->
plugin class
+// name -> MainPreset `.configure({ key: VizType.X })` -> VizType enum string.
+// A single builder legitimately maps to several keys (e.g. echarts_timeseries
+ _line/_bar/...).
+import {
+ readFileSync,
+ writeFileSync,
+ readdirSync,
+ statSync,
+ existsSync,
+} from 'node:fs';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const FE = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
+const PLUGINS = path.join(FE, 'plugins');
+const OUT = path.join(
+ FE,
+ 'src',
+ 'backend-querycontext',
+ 'registry.generated.ts',
+);
+
+// --- 1. VizType enum: EnumName -> 'string_value' ---
+const vizTypeSrc = readFileSync(
+ path.join(FE, 'packages/superset-ui-core/src/chart/types/VizType.ts'),
+ 'utf8',
+);
+const VIZ_ENUM = {};
+for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g))
+ VIZ_ENUM[m[1]] = m[2];
+
+// --- 2. MainPreset: ClassName -> viz string ---
+const mainPreset = readFileSync(
+ path.join(FE, 'src/visualizations/presets/MainPreset.ts'),
+ 'utf8',
+);
+// MainPreset renames on import (e.g. `import { PivotTableChartPlugin as
+// PivotTableChartPluginV2 } from '...'`), then `new
PivotTableChartPluginV2()`. Map
+// each local `new X()` name back to the ORIGINAL package export name the
codegen sees.
+const importOrig = {}; // localName -> package-export name
+for (const im of mainPreset.matchAll(
+ /import\s*\{([^}]*)\}\s*from\s*['"][^'"]+['"]/g,
+)) {
+ for (let spec of im[1].split(',')) {
+ spec = spec.trim();
+ if (!spec) continue;
+ const as = spec.match(/^(\w+)\s+as\s+(\w+)$/);
+ if (as) importOrig[as[2]] = as[1];
+ else if (/^\w+$/.test(spec)) importOrig[spec] = spec;
+ }
+}
+const CLASS_TO_VIZ = {};
+for (const m of mainPreset.matchAll(
+ /new\s+(\w+)\s*\(\s*\)\s*\.configure\(\s*\{\s*key:\s*VizType\.(\w+)/g,
+)) {
+ const [, local, enumName] = m;
+ if (!VIZ_ENUM[enumName]) continue;
+ CLASS_TO_VIZ[importOrig[local] ?? local] = VIZ_ENUM[enumName];
+ CLASS_TO_VIZ[local] = VIZ_ENUM[enumName]; // also accept the local alias
+}
+
+// --- collect all index.ts + buildQuery modules under plugins/ ---
+function walk(dir, acc = []) {
+ for (const e of readdirSync(dir)) {
+ const p = path.join(dir, e);
+ const st = statSync(p);
+ if (st.isDirectory()) {
+ if (e === 'node_modules' || e === 'test' || e === '__tests__') continue;
+ walk(p, acc);
+ } else acc.push(p);
+ }
+ return acc;
+}
+const files = walk(PLUGINS);
Review Comment:
Discovery only walks `plugins/`, but `TimeTable` is registered in
`MainPreset` with its builder under
`src/visualizations/TimeTable/config/buildQuery.ts`, so `time_table` never
appears in the generated registry even with V8 available. Importing a TimeTable
chart with `metrics: ["count"]` and a daily `granularity_sqla` falls through to
the Python fallback, which omits `is_timeseries`, so the saved-chart read
returns an aggregate with no time series. Could the generator also scan
`src/visualizations/` (or the registry check fail when a `MainPreset` plugin
with a `buildQuery` has no entry)?
##########
superset/commands/chart/importers/v1/__init__.py:
##########
@@ -181,7 +200,26 @@ def _import(
):
if str(config["uuid"]) in reused_chart_uuids:
continue
+ # Count the query_context synthesis outcome from what this import
did
+ # to the config. Keyed off the pre-import snapshot so a chart that
+ # already had a context is counted as preserved, not as newly
+ # synthesized (#33615 review).
+ if had_query_context.get(str(config["uuid"])):
+ n_preserved += 1
+ elif config.get("query_context"):
Review Comment:
`import_chart()` rebinds its local `config` to the deep-copied result of
`migrate_chart()` and sets `query_context` on that copy, but `import_charts()`
yields the caller's original config. So `config.get("query_context")` here is
falsy for every chart that did not already have one, and the summary logs
successfully synthesized charts as non-derivable (`0 queryable, N
non-derivable`). Could the count be based on the persisted
`chart.query_context` instead?
--
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]
