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 a4ae889f48c8f01e3920ead687a97f0a58df79e3 Author: Eugene Olkhovski <[email protected]> AuthorDate: Fri Aug 28 06:26:41 2026 -0400 fix(charts): address review on import query_context synthesis (#33615) - gen-qc-registry: read MainPreset.ts (was .js -> ENOENT, registry never regenerated); regenerate registry so newly discovered viz types are tracked - importers/v1/utils: synthesize query_context AFTER migrate_chart so legacy viz types (e.g. dual_line -> mixed_timeseries) derive from migrated form data - query_context_builder: normalize singular `metric` into `metrics` so single-metric charts (Big Number) are derivable instead of NULL/400 - query_context_builder: compose adhoc SQL filters via the shared split_adhoc_filters_into_base_filters (parenthesized, comment-safe) instead of a bare " AND ".join, preserving OR precedence - tests: cover singular metric, OR-precedence + trailing-comment filters, and synthesis-after-migration ordering Co-Authored-By: Claude Opus 4.8 <[email protected]> --- superset-frontend/scripts/gen-qc-registry.mjs | 2 +- .../src/backend-querycontext/registry.generated.ts | 114 +++++++++++---------- superset/commands/chart/importers/v1/utils.py | 18 +++- superset/commands/chart/query_context_builder.py | 78 ++++++-------- .../charts/commands/importers/v1/import_test.py | 37 +++++++ .../importers/v1/query_context_builder_test.py | 76 +++++++++++++- 6 files changed, 223 insertions(+), 102 deletions(-) diff --git a/superset-frontend/scripts/gen-qc-registry.mjs b/superset-frontend/scripts/gen-qc-registry.mjs index 5fad639e24c..5a6e5bd4364 100644 --- a/superset-frontend/scripts/gen-qc-registry.mjs +++ b/superset-frontend/scripts/gen-qc-registry.mjs @@ -40,7 +40,7 @@ for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g)) VIZ_ENUM[m // --- 2. MainPreset: ClassName -> viz string --- const mainPreset = readFileSync( - path.join(FE, 'src/visualizations/presets/MainPreset.js'), + path.join(FE, 'src/visualizations/presets/MainPreset.ts'), 'utf8', ); // MainPreset renames on import (e.g. `import { PivotTableChartPlugin as diff --git a/superset-frontend/src/backend-querycontext/registry.generated.ts b/superset-frontend/src/backend-querycontext/registry.generated.ts index fc4f0796773..192c6a1d456 100644 --- a/superset-frontend/src/backend-querycontext/registry.generated.ts +++ b/superset-frontend/src/backend-querycontext/registry.generated.ts @@ -15,60 +15,70 @@ // GENERATED by superset-frontend/scripts/gen-qc-registry.mjs — DO NOT EDIT. // Re-run: node scripts/gen-qc-registry.mjs -import bq0 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery'; -import bq1 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/buildQuery'; -import bq2 from '../../plugins/plugin-chart-echarts/src/BoxPlot/buildQuery'; -import bq3 from '../../plugins/plugin-chart-echarts/src/Bubble/buildQuery'; -import bq4 from '../../plugins/plugin-chart-echarts/src/Timeseries/buildQuery'; -import bq5 from '../../plugins/plugin-chart-echarts/src/Funnel/buildQuery'; -import bq6 from '../../plugins/plugin-chart-echarts/src/Gauge/buildQuery'; -import bq7 from '../../plugins/plugin-chart-echarts/src/Graph/buildQuery'; -import bq8 from '../../plugins/plugin-chart-handlebars/src/plugin/buildQuery'; -import bq9 from '../../plugins/plugin-chart-echarts/src/Heatmap/buildQuery'; -import bq10 from '../../plugins/plugin-chart-echarts/src/Histogram/buildQuery'; -import bq11 from '../../plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery'; -import bq12 from '../../plugins/plugin-chart-echarts/src/Pie/buildQuery'; -import bq13 from '../../plugins/plugin-chart-pivot-table/src/plugin/buildQuery'; -import bq14 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery'; -import bq15 from '../../plugins/plugin-chart-echarts/src/Radar/buildQuery'; -import bq16 from '../../plugins/plugin-chart-echarts/src/Sankey/buildQuery'; -import bq17 from '../../plugins/plugin-chart-echarts/src/Sunburst/buildQuery'; -import bq18 from '../../plugins/plugin-chart-table/src/buildQuery'; -import bq19 from '../../plugins/plugin-chart-echarts/src/Tree/buildQuery'; -import bq20 from '../../plugins/plugin-chart-echarts/src/Treemap/buildQuery'; -import bq21 from '../../plugins/plugin-chart-echarts/src/Waterfall/buildQuery'; -import bq22 from '../../plugins/plugin-chart-word-cloud/src/plugin/buildQuery'; +import bq0 from '../../plugins/plugin-chart-ag-grid-table/src/buildQuery'; +import bq1 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery'; +import bq2 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/buildQuery'; +import bq3 from '../../plugins/plugin-chart-echarts/src/BoxPlot/buildQuery'; +import bq4 from '../../plugins/plugin-chart-echarts/src/Bubble/buildQuery'; +import bq5 from '../../plugins/plugin-chart-echarts/src/Bullet/buildQuery'; +import bq6 from '../../plugins/plugin-chart-echarts/src/Timeseries/buildQuery'; +import bq7 from '../../plugins/plugin-chart-echarts/src/Funnel/buildQuery'; +import bq8 from '../../plugins/plugin-chart-echarts/src/Gantt/buildQuery'; +import bq9 from '../../plugins/plugin-chart-echarts/src/Gauge/buildQuery'; +import bq10 from '../../plugins/plugin-chart-echarts/src/Graph/buildQuery'; +import bq11 from '../../plugins/plugin-chart-handlebars/src/plugin/buildQuery'; +import bq12 from '../../plugins/plugin-chart-echarts/src/Heatmap/buildQuery'; +import bq13 from '../../plugins/plugin-chart-echarts/src/Histogram/buildQuery'; +import bq14 from '../../plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery'; +import bq15 from '../../plugins/plugin-chart-echarts/src/Pie/buildQuery'; +import bq16 from '../../plugins/plugin-chart-pivot-table/src/plugin/buildQuery'; +import bq17 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery'; +import bq18 from '../../plugins/plugin-chart-echarts/src/Radar/buildQuery'; +import bq19 from '../../plugins/plugin-chart-echarts/src/Rose/buildQuery'; +import bq20 from '../../plugins/plugin-chart-echarts/src/Sankey/buildQuery'; +import bq21 from '../../plugins/plugin-chart-echarts/src/Sunburst/buildQuery'; +import bq22 from '../../plugins/plugin-chart-table/src/buildQuery'; +import bq23 from '../../plugins/plugin-chart-echarts/src/TimePivot/buildQuery'; +import bq24 from '../../plugins/plugin-chart-echarts/src/Tree/buildQuery'; +import bq25 from '../../plugins/plugin-chart-echarts/src/Treemap/buildQuery'; +import bq26 from '../../plugins/plugin-chart-echarts/src/Waterfall/buildQuery'; +import bq27 from '../../plugins/plugin-chart-word-cloud/src/plugin/buildQuery'; export const REGISTRY: Record<string, (fd: any) => any> = { - big_number: bq0 as (fd: any) => any, - big_number_total: bq1 as (fd: any) => any, - box_plot: bq2 as (fd: any) => any, - bubble_v2: bq3 as (fd: any) => any, - echarts_area: bq4 as (fd: any) => any, - echarts_timeseries: bq4 as (fd: any) => any, - echarts_timeseries_bar: bq4 as (fd: any) => any, - echarts_timeseries_line: bq4 as (fd: any) => any, - echarts_timeseries_scatter: bq4 as (fd: any) => any, - echarts_timeseries_smooth: bq4 as (fd: any) => any, - echarts_timeseries_step: bq4 as (fd: any) => any, - funnel: bq5 as (fd: any) => any, - gauge_chart: bq6 as (fd: any) => any, - graph_chart: bq7 as (fd: any) => any, - handlebars: bq8 as (fd: any) => any, - heatmap_v2: bq9 as (fd: any) => any, - histogram_v2: bq10 as (fd: any) => any, - mixed_timeseries: bq11 as (fd: any) => any, - pie: bq12 as (fd: any) => any, - pivot_table_v2: bq13 as (fd: any) => any, - pop_kpi: bq14 as (fd: any) => any, - radar: bq15 as (fd: any) => any, - sankey_v2: bq16 as (fd: any) => any, - sunburst_v2: bq17 as (fd: any) => any, - table: bq18 as (fd: any) => any, - tree_chart: bq19 as (fd: any) => any, - treemap_v2: bq20 as (fd: any) => any, - waterfall: bq21 as (fd: any) => any, - word_cloud: bq22 as (fd: any) => any, + 'ag-grid-table': bq0 as (fd: any) => any, + 'big_number': bq1 as (fd: any) => any, + 'big_number_total': bq2 as (fd: any) => any, + 'box_plot': bq3 as (fd: any) => any, + 'bubble_v2': bq4 as (fd: any) => any, + 'bullet': bq5 as (fd: any) => any, + 'echarts_area': bq6 as (fd: any) => any, + 'echarts_timeseries': bq6 as (fd: any) => any, + 'echarts_timeseries_bar': bq6 as (fd: any) => any, + 'echarts_timeseries_line': bq6 as (fd: any) => any, + 'echarts_timeseries_scatter': bq6 as (fd: any) => any, + 'echarts_timeseries_smooth': bq6 as (fd: any) => any, + 'echarts_timeseries_step': bq6 as (fd: any) => any, + 'funnel': bq7 as (fd: any) => any, + 'gantt_chart': bq8 as (fd: any) => any, + 'gauge_chart': bq9 as (fd: any) => any, + 'graph_chart': bq10 as (fd: any) => any, + 'handlebars': bq11 as (fd: any) => any, + 'heatmap_v2': bq12 as (fd: any) => any, + 'histogram_v2': bq13 as (fd: any) => any, + 'mixed_timeseries': bq14 as (fd: any) => any, + 'pie': bq15 as (fd: any) => any, + 'pivot_table_v2': bq16 as (fd: any) => any, + 'pop_kpi': bq17 as (fd: any) => any, + 'radar': bq18 as (fd: any) => any, + 'rose': bq19 as (fd: any) => any, + 'sankey_v2': bq20 as (fd: any) => any, + 'sunburst_v2': bq21 as (fd: any) => any, + 'table': bq22 as (fd: any) => any, + 'time_pivot': bq23 as (fd: any) => any, + 'tree_chart': bq24 as (fd: any) => any, + 'treemap_v2': bq25 as (fd: any) => any, + 'waterfall': bq26 as (fd: any) => any, + 'word_cloud': bq27 as (fd: any) => any, }; export const VIZ_TYPES: string[] = Object.keys(REGISTRY); diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index a4c53b529ea..8c171798490 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -167,6 +167,14 @@ def _synthesize_query_context_if_absent(config: dict[str, Any]) -> None: return try: params = config.get("params") or {} + # ``params`` is a dict early in the import flow but a JSON string once + # serialized ahead of migration; accept either so synthesis can run + # after ``migrate_chart`` on the migrated form data. + if isinstance(params, str): + try: + params = json.loads(params) or {} + except (json.JSONDecodeError, TypeError): + params = {} viz_type = config["viz_type"] datasource_id = config.get("datasource_id") datasource_type = config.get("datasource_type", "table") @@ -267,8 +275,6 @@ def import_chart( filter_chart_annotations(config) - _synthesize_query_context_if_absent(config) - # TODO (betodealmeida): move this logic to import_from_dict config["params"] = json.dumps(config["params"]) @@ -276,6 +282,14 @@ def import_chart( config = migrate_chart(config) extra = config.pop("extra", None) + + # Synthesize the query_context AFTER migration so legacy viz types (e.g. + # dual_line -> mixed_timeseries) derive their queries from the migrated + # viz_type and params rather than the pre-migration form data (#33615 + # review). Guarded on an absent context, so a chart that shipped its own + # query_context (already patched by migrate_chart) is never overwritten. + _synthesize_query_context_if_absent(config) + chart = Slice.import_from_dict(config, recursive=False, allow_reparenting=True) if chart.id is None: db.session.flush() diff --git a/superset/commands/chart/query_context_builder.py b/superset/commands/chart/query_context_builder.py index c764787c0f7..d68f727d0a0 100644 --- a/superset/commands/chart/query_context_builder.py +++ b/superset/commands/chart/query_context_builder.py @@ -35,6 +35,8 @@ 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. @@ -46,48 +48,31 @@ _DEFAULT_ROW_LIMIT = 5000 def _translate_adhoc_filters( adhoc_filters: list[Any] | None, -) -> tuple[list[dict[str, Any]], list[str], list[str]]: +) -> tuple[list[dict[str, Any]], str, str]: """ - Translate viz ``adhoc_filters`` into simple ``{col, op, val}`` filters. - - - ``expressionType == "SIMPLE"`` → a ``{col, op, val}`` filter. - - ``expressionType == "SQL"`` → routed to ``extras.where`` (or - ``extras.having`` for a HAVING clause) so a hand-written SQL predicate is - preserved rather than lost. - - Anything that cannot be mapped cleanly (missing subject/operator, malformed - entry) is dropped **without raising** (RISK-T05) — an imported chart must - never abort its bundle over a single unmappable filter. - - Returns ``(filters, where_expressions, having_expressions)``. + 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. 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. + + Returns ``(filters, where, having)`` where ``where``/``having`` are the + parenthesized, comment-safe SQL strings ready for ``extras``. """ - filters: list[dict[str, Any]] = [] - where_expressions: list[str] = [] - having_expressions: list[str] = [] - for adhoc_filter in adhoc_filters or []: - if not isinstance(adhoc_filter, dict): - continue - expression_type = adhoc_filter.get("expressionType") or "SIMPLE" - if expression_type == "SIMPLE": - subject = adhoc_filter.get("subject") - operator = adhoc_filter.get("operator") - if subject and operator: - filters.append( - { - "col": subject, - "op": operator, - "val": adhoc_filter.get("comparator"), - } - ) - # else: unmappable SIMPLE filter — dropped, no crash (RISK-T05). - elif expression_type == "SQL": - sql_expression = adhoc_filter.get("sqlExpression") - if sql_expression: - clause = (adhoc_filter.get("clause") or "WHERE").upper() - if clause == "HAVING": - having_expressions.append(sql_expression) - else: - where_expressions.append(sql_expression) - return filters, where_expressions, having_expressions + # 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)] + 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]]: @@ -144,14 +129,17 @@ def build_query_context_config( 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 [] if not metrics and not columns: return None - filters, where_expressions, having_expressions = _translate_adhoc_filters( - params.get("adhoc_filters", []) - ) + filters, where, having = _translate_adhoc_filters(params.get("adhoc_filters", [])) query_object = { "time_range": params.get("time_range", " : "), @@ -159,8 +147,8 @@ def build_query_context_config( "filters": filters, "extras": { "time_grain_sqla": params.get("time_grain_sqla"), - "having": " AND ".join(having_expressions), - "where": " AND ".join(where_expressions), + "having": having, + "where": where, }, "applied_time_extras": {}, "columns": columns, 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 20b3c5d047e..2cf8e3407d6 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -520,6 +520,43 @@ def test_import_chart_preserves_existing_query_context( assert json.loads(chart.query_context) == json.loads(original_query_context) +def test_synthesis_runs_after_migration_for_legacy_viz( + mocker: MockerFixture, session_with_schema: Session +) -> None: + """ + #33615 review: query_context synthesis must run AFTER ``migrate_chart`` so a + legacy viz type (``dual_line`` -> ``mixed_timeseries``) derives its context + from the MIGRATED viz_type/params, not the pre-migration form data. Otherwise + the persisted context describes a chart shape that no longer exists. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + # Force the generic fallback and capture the viz_type it is handed. + fallback = mocker.patch( + "superset.commands.chart.importers.v1.utils.build_query_context_config", + return_value=None, + ) + # Make the V8 generator a no-op so the (spied) fallback is exercised. + 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"] = "dual_line" + config["params"]["viz_type"] = "dual_line" + config.pop("query_context", None) + + chart = import_chart(config) + + # The chart is migrated to the modern viz type ... + assert chart.viz_type == "mixed_timeseries" + # ... and synthesis saw the MIGRATED viz type, never "dual_line". + assert fallback.call_args is not None + passed_viz_type = fallback.call_args.args[1] + assert passed_viz_type == "mixed_timeseries" + + 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 538cd728cf4..82937422bd5 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 @@ -81,6 +81,29 @@ def test_metrics_only_is_derivable() -> 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 @@ -158,11 +181,60 @@ def test_adhoc_sql_filter_routed_to_extras_and_no_crash() -> None: query_context = build_query_context_config(params, "table", 12, "table") assert query_context is not None extras = query_context["queries"][0]["extras"] - assert extras["where"] == "num > 0" - assert extras["having"] == "sum(num) > 1" + # 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_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,
