sadpandajoe commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r4152682944
########## 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 [] + # Raw (non-aggregated) tables carry no metrics/groupby and select row-level + # ``all_columns`` instead. Without this the bundled raw ``Table.yaml`` + # (query_mode: raw, empty metrics/groupby, populated all_columns) is + # misclassified non-derivable and its data endpoint 400s (#33615 review). + if not columns and params.get("query_mode") == "raw": + columns = params.get("all_columns") or [] + if not metrics and not columns: + return None + + translated = _translate_adhoc_filters(params.get("adhoc_filters", [])) Review Comment: When V8 is unavailable, a chart with `where: "region = 'EMEA'"` persists `extras.where: ""`, and an `extra_filters` region restriction is also dropped, so the saved-chart data endpoint returns a broader result set than the chart defines. Could the fallback preserve these supported filter sources, or leave the chart non-derivable when it cannot? ########## 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`. Review Comment: With the optional V8 runtime absent, a `pivot_table_v2` chart using `groupbyRows` and `groupbyColumns` but no generic `columns`/`groupby` is saved as a single aggregate with `columns: []`, losing both pivot axes. Could unsupported visualization-specific shapes remain non-derivable rather than persisting a context that returns the wrong data? ########## 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 [] + # Raw (non-aggregated) tables carry no metrics/groupby and select row-level + # ``all_columns`` instead. Without this the bundled raw ``Table.yaml`` + # (query_mode: raw, empty metrics/groupby, populated all_columns) is + # misclassified non-derivable and its data endpoint 400s (#33615 review). + if not columns and params.get("query_mode") == "raw": + columns = params.get("all_columns") or [] + if not metrics and not columns: + return None + + 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", " : "), + "granularity": params.get("granularity_sqla") or params.get("granularity"), + "filters": filters, + "extras": { + "time_grain_sqla": params.get("time_grain_sqla"), + "having": having, + "where": where, + }, + "applied_time_extras": {}, + "columns": columns, + "metrics": metrics, + "orderby": _derive_orderby(params), + "annotation_layers": [], + # Leave ``row_limit`` unset when the chart does not specify one so the + # read path applies the runtime-configured ``ROW_LIMIT`` (50,000 by + # default), rather than baking in a hard-coded cap that would silently + # truncate imported charts to fewer rows than the UI shows (#33615 review). + "row_limit": params.get("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}, Review Comment: This fallback omits `form_data`, which `QueryContextFactory` reads to enable server pagination; a saved table with `server_pagination: true` is therefore reconstructed with pagination disabled and its requested row limit is clamped to the lower `SQL_MAX_ROW` cap. Could the persisted context retain the necessary form data with destination-local identity, and cover this through the query-context schema? ########## 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); +const indexFiles = files.filter(f => /[/\\]index\.ts$/.test(f)); +const buildQueryFiles = files.filter(f => /[/\\]buildQuery\.(ts|js)$/.test(f)); + +// resolve an import specifier from a file to an absolute module file (ts/js/index) +function resolveImport(fromFile, spec) { + const base = path.resolve(path.dirname(fromFile), spec); + for (const c of [ + base, + `${base}.ts`, + `${base}.js`, + path.join(base, 'index.ts'), + path.join(base, 'index.js'), + ]) { + if (existsSync(c) && statSync(c).isFile()) return path.resolve(c); + } + return null; +} + +// --- re-export graph: node "NAME@file" or "DEF@file"; edge to the module it forwards to. +// Lets us find every alias under which a subdir's DEFAULT export (a plugin class) is +// visible in the package barrels MainPreset imports from (handles `default as Alias` +// renames + multi-hop named passthrough like BigNumber). --- +const edges = new Map(); // node -> node it forwards to +const key = (name, file) => `${name}@${file}`; +for (const idx of indexFiles) { + const src = readFileSync(idx, 'utf8'); + for (const m of src.matchAll( + /export\s*\{([^}]*)\}\s*from\s*['"]([^'"]+)['"]/g, + )) { + const target = resolveImport(idx, m[2]); + if (!target) continue; + for (let spec of m[1].split(',')) { + spec = spec.trim(); + if (!spec) continue; + let dm; + if ((dm = spec.match(/^default\s+as\s+(\w+)$/))) + edges.set(key(dm[1], idx), key('DEF', target)); // Alias = default of target + else if ((dm = spec.match(/^(\w+)\s+as\s+(\w+)$/))) + edges.set(key(dm[2], idx), key(dm[1], target)); // Y = target's X + else if ((dm = spec.match(/^(\w+)$/))) + edges.set(key(dm[1], idx), key(dm[1], target)); // named passthrough + } + } +} +// reverse reachability: all alias NAMEs (anywhere) that resolve to DEF@indexFile +function aliasesForDefault(indexFile) { + const goal = key('DEF', path.resolve(indexFile)); + const out = new Set(); + for (const [from, to] of edges) { + // does `from` reach goal? + let cur = to; + const seen = new Set([from]); + while (cur && !seen.has(cur)) { + if (cur === goal) { + out.add(from.split('@')[0]); + break; + } + seen.add(cur); + cur = edges.get(cur); + } + } + return out; +} + +// --- 3+4. For each buildQuery, find index.ts importing it -> plugin class alias -> viz key --- +const REGISTRY = {}; // viz -> buildQuery abs path +const unmapped = []; // buildQuery with no resolvable viz key +for (const bq of buildQueryFiles) { + const bqAbs = path.resolve(bq); + const owningIndexes = new Set(); + for (const idx of indexFiles) { + const src = readFileSync(idx, 'utf8'); + for (const m of src.matchAll( + /import\s+\w+\s+from\s+['"]([^'"]+buildQuery)['"]/g, + )) { + if ( + resolveImport(idx, m[1]) === bqAbs && + /export\s+default\s+class\s+\w+/.test(src) + ) + owningIndexes.add(path.resolve(idx)); + } + } + const aliases = new Set(); + for (const idx of owningIndexes) { + const src = readFileSync(idx, 'utf8'); + const localCls = src.match(/export\s+default\s+class\s+(\w+)/); + if (localCls) aliases.add(localCls[1]); // direct (unrenamed) registration + for (const a of aliasesForDefault(idx)) aliases.add(a); // renamed re-exports + } + const vizKeys = [ + ...new Set([...aliases].map(c => CLASS_TO_VIZ[c]).filter(Boolean)), + ]; + if (vizKeys.length === 0) + unmapped.push({ bq: path.relative(FE, bq), classes: [...aliases] }); + else for (const v of vizKeys) REGISTRY[v] = bqAbs; +} + +// --- 4b. Explicit overrides for plugins the structural matchers above do not +// cover yet: Chord registers its buildQuery lazily +// (`loadBuildQuery: () => import('./buildQuery')`) rather than as a static +// default import, and Cartodiagram is constructed with arguments +// (`new CartodiagramPlugin({...}).configure(...)`) rather than the empty-paren +// form. Mapped explicitly so this change stays scoped to the two viz types +// raised in review (#33615); generalizing the matchers to every such plugin is +// left as a follow-up. Each path is asserted to exist so a plugin move surfaces +// here instead of silently dropping the entry. +const EXPLICIT_BUILD_QUERY = { + chord: 'plugins/plugin-chart-chord/src/buildQuery.ts', + cartodiagram: 'plugins/plugin-chart-cartodiagram/src/plugin/buildQuery.ts', Review Comment: Cartodiagram's builder calls `getChartBuildQueryRegistry().get(selectedChart.viz_type)`, but this standalone bundle never registers the imported builders in that singleton, so selecting a Pie chart throws and returns `__error__` despite this mapping. Could the bundle initialize the nested-builder registry and test Cartodiagram through the generated entry without test-side registration? -- 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]
