codeant-ai-for-open-source[bot] commented on code in PR #42284: URL: https://github.com/apache/superset/pull/42284#discussion_r3630927752
########## superset/common/form_data_query_context.py: ########## @@ -0,0 +1,129 @@ +# 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. +""" +Synthesize a query context from a chart's saved form data (``params``). + +A chart's ``query_context`` is normally generated client-side by each viz +plugin's ``buildQuery`` and only persisted when the chart is (re-)saved in +Explore. Charts that predate that behavior keep their ``params`` (form data) but +carry no ``query_context``, so server-side consumers that need to run the query +(e.g. the dashboard Excel export) have nothing to execute. + +This module rebuilds a best-effort query context from the form data. The same +approach is used by the MCP chart compile/preview path +(``superset.mcp_service.chart.compile``); it is a *generic single-query* +mapping (columns, metrics, filters, time range) and intentionally does **not** +reproduce plugin-specific ``buildQuery`` logic β post-processing pipelines +(pivot, rolling, contribution, forecast) or multi-query fan-out (e.g. mixed +time-series). Callers must therefore restrict it to viz types whose data maps +faithfully to a single plain query; anything else risks silently wrong or +incomplete results. +""" + +from __future__ import annotations + +from typing import Any + + +def adhoc_filters_to_query_filters( + adhoc_filters: list[dict[str, Any]], +) -> list[dict[str, Any]]: + """ + Convert saved adhoc filters into QueryObject filter clauses. + + Adhoc filters use ``{subject, operator, comparator}`` while a query object + expects ``{col, op, val}``. Only ``SIMPLE`` filters are convertible; custom + SQL filters have no equivalent here and are dropped. + """ + result: list[dict[str, Any]] = [] + for flt in adhoc_filters or []: + if flt.get("expressionType") == "SIMPLE": + result.append( + { + "col": flt.get("subject"), + "op": flt.get("operator"), + "val": flt.get("comparator"), + } + ) + return result + + +def columns_from_form_data(form_data: dict[str, Any]) -> list[Any]: + """ + Derive the query's grouping/raw columns from form data. + + Handles raw-mode tables (``all_columns``/``columns``), an ``x_axis`` (string + or adhoc column), and ``groupby`` dimensions, de-duplicating while preserving + order. + """ + if form_data.get("query_mode") == "raw" and ( + form_data.get("all_columns") or form_data.get("columns") + ): + return list(form_data.get("all_columns") or form_data.get("columns") or []) + + groupby_columns: list[Any] = form_data.get("groupby") or [] + raw_columns: list[Any] = form_data.get("columns") or [] + columns = raw_columns.copy() if "columns" in form_data else groupby_columns.copy() + + x_axis = form_data.get("x_axis") + if isinstance(x_axis, str) and x_axis and x_axis not in columns: + columns.insert(0, x_axis) + elif isinstance(x_axis, dict): + col_name = x_axis.get("column_name") + if col_name and col_name not in columns: + columns.insert(0, col_name) + return columns + + +def build_query_context_from_form_data( + form_data: dict[str, Any], + datasource: dict[str, Any], +) -> dict[str, Any]: + """ + Build a query-context payload (the JSON shape ``ChartDataQueryContextSchema`` + loads) from a chart's form data and datasource reference. + + :param form_data: The chart's saved ``params`` parsed to a dict. + :param datasource: ``{"id": <int>, "type": "table"}`` datasource reference. + :returns: A single-query query-context dict. + """ + metrics = form_data.get("metrics") or [] + # Single-metric charts (e.g. Big Number) store ``metric`` rather than + # ``metrics``. + if not metrics and form_data.get("metric"): + metrics = [form_data["metric"]] + + columns = columns_from_form_data(form_data) + # Big Number with a trendline uses ``granularity_sqla`` as its time column. + if not columns and form_data.get("granularity_sqla"): + columns = [form_data["granularity_sqla"]] + + query: dict[str, Any] = { + "columns": columns, + "metrics": metrics, + "orderby": form_data.get("orderby") or [], + "filters": adhoc_filters_to_query_filters(form_data.get("adhoc_filters", [])), Review Comment: **Suggestion:** The rebuild only maps `adhoc_filters` and ignores legacy/simple `filters` stored directly in form data. Charts that still rely on `filters` will export unfiltered or partially filtered data, producing incorrect Excel output. [logic error] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - Excel exports ignore legacy form_data filters. - Dashboard exports include rows that charts filter out. - Data consumers see mismatched filtered versus exported data. - Affects legacy charts without saved query_context. - Undermines trust in dashboard Excel export accuracy. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Locate or create a legacy chart (for example viz_type "table" or "pie") whose saved form data uses the older `filters` field instead of `adhoc_filters` (for example `form_data["filters"] = [{"col": "country", "op": "==", "val": "US"}]`) and has no persisted `query_context`; this form data is what `chart.params` contains and is parsed in `_resolve_query_context()` at `superset/tasks/export_dashboard_excel.py:145-148`. 2. Add this chart to a dashboard and trigger a dashboard Excel export so that the Celery task in `superset/tasks/export_dashboard_excel.py` runs `_build_workbook()` at line 244 and iterates charts via `get_charts_in_layout_order(dashboard)` at line 261. 3. For this chart, `_build_workbook()` calls `_resolve_query_context()` at line 269; because `chart.query_context` is empty, `_resolve_query_context()` falls back to `build_query_context_from_form_data()` in `superset/common/form_data_query_context.py:92-125`, passing the parsed `form_data` dictionary. 4. Inside `build_query_context_from_form_data()`, the query payload is constructed at lines 115β121; the `filters` entry is set solely from `adhoc_filters_to_query_filters(form_data.get("adhoc_filters", []))` (line 119), and the legacy `form_data["filters"]` is never read, so the rebuilt query context has an empty `filters` list, causing `ChartDataCommand` (invoked downstream in `_write_chart_sheets()` at `superset/tasks/export_dashboard_excel.py:192-213`) to execute an unfiltered query; the resulting Excel sheet includes all rows instead of only those matching the original `filters`, silently diverging from what users see in Explore. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f3588690220c4bc2be1c93d4fe2998bf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f3588690220c4bc2be1c93d4fe2998bf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/common/form_data_query_context.py **Line:** 119:119 **Comment:** *Logic Error: The rebuild only maps `adhoc_filters` and ignores legacy/simple `filters` stored directly in form data. Charts that still rely on `filters` will export unfiltered or partially filtered data, producing incorrect Excel output. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=e89de93850a4ba2b58887faebe11b3026e66cd534d16e65a59691a29b71894bb&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=e89de93850a4ba2b58887faebe11b3026e66cd534d16e65a59691a29b71894bb&reaction=dislike'>π</a> ########## superset/tasks/export_dashboard_excel.py: ########## @@ -96,6 +103,59 @@ def _chart_label(chart: Any) -> str: return f"{chart.id} - {chart.slice_name or ''}".strip() +def _has_empty_query_context(raw: Any) -> bool: + """ + Whether a chart has no usable saved query context to export data from. + + Covers a missing/blank value, a string that does not parse as JSON, a value + that parses to something other than an object (e.g. ``"null"``), and an + object with no queries (e.g. ``"{}"`` or ``{"queries": []}``). All of these + are treated the same as a missing context: the chart is skipped and listed + under the "no query context" remediation rather than raising mid-export. + """ + if not raw: + return True + try: + parsed = json.loads(raw) + except (TypeError, ValueError): + return True + return not isinstance(parsed, dict) or not parsed.get("queries") + + +def _rebuild_viz_types() -> set[str]: + """Viz types eligible for form-data query-context rebuild (config or default).""" + return current_app.config.get("EXCEL_EXPORT_REBUILD_VIZ_TYPES") or REBUILD_VIZ_TYPES Review Comment: **Suggestion:** The config fallback logic treats an explicitly configured empty set as falsy and silently re-enables the default rebuild allowlist. This makes it impossible to disable rebuilds via configuration. Check for `None` explicitly instead of using `or` so an empty set is respected. [logic error] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx - Cannot fully disable query-context rebuild via config. - Operatorsβ explicit empty allowlist is silently ignored. - Charts still export with synthesized contexts against expectations. - Potentially reintroduces silently incomplete or approximate results. - Reduces trust in Excel export configuration controls. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. In the Superset configuration file `superset/config.py`, set `EXCEL_EXPORT_REBUILD_VIZ_TYPES = set()` (or an empty list) to explicitly disable form-data-based query-context rebuilds for Excel export while keeping other defaults unchanged; this value is read through `current_app.config` by `_rebuild_viz_types()` in `superset/tasks/export_dashboard_excel.py:125-127`. 2. Start the Superset application and Celery workers so that the dashboard Excel export task defined in `superset/tasks/export_dashboard_excel.py` is available and using the configured `EXCEL_EXPORT_REBUILD_VIZ_TYPES`. 3. Export a dashboard containing a legacy chart (for example viz_type "table") with no saved `query_context`; during `_build_workbook()` at `superset/tasks/export_dashboard_excel.py:244-273`, this chart is processed on the data path (not as an image) so `_resolve_query_context()` at line 130 is invoked. 4. Inside `_resolve_query_context()`, `_rebuild_viz_types()` is called at line 144; because `_rebuild_viz_types()` currently returns `current_app.config.get("EXCEL_EXPORT_REBUILD_VIZ_TYPES") or REBUILD_VIZ_TYPES` (line 127), the empty set from the config is treated as falsy and the function returns the default `REBUILD_VIZ_TYPES` instead, so the chartβs viz_type (for example "table") is still considered rebuild-eligible and its query context is synthesized from form data, even though the operator explicitly configured an empty allowlist to disable rebuilds. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8fa6b34d935f418b8747e30483a12de4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8fa6b34d935f418b8747e30483a12de4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/tasks/export_dashboard_excel.py **Line:** 127:127 **Comment:** *Logic Error: The config fallback logic treats an explicitly configured empty set as falsy and silently re-enables the default rebuild allowlist. This makes it impossible to disable rebuilds via configuration. Check for `None` explicitly instead of using `or` so an empty set is respected. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=7d71c1dfd6131671222be7eb582ea2d9c26e1f8192caec64c6036c4603602425&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=7d71c1dfd6131671222be7eb582ea2d9c26e1f8192caec64c6036c4603602425&reaction=dislike'>π</a> ########## superset/common/form_data_query_context.py: ########## @@ -0,0 +1,129 @@ +# 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. +""" +Synthesize a query context from a chart's saved form data (``params``). + +A chart's ``query_context`` is normally generated client-side by each viz +plugin's ``buildQuery`` and only persisted when the chart is (re-)saved in +Explore. Charts that predate that behavior keep their ``params`` (form data) but +carry no ``query_context``, so server-side consumers that need to run the query +(e.g. the dashboard Excel export) have nothing to execute. + +This module rebuilds a best-effort query context from the form data. The same +approach is used by the MCP chart compile/preview path +(``superset.mcp_service.chart.compile``); it is a *generic single-query* +mapping (columns, metrics, filters, time range) and intentionally does **not** +reproduce plugin-specific ``buildQuery`` logic β post-processing pipelines +(pivot, rolling, contribution, forecast) or multi-query fan-out (e.g. mixed +time-series). Callers must therefore restrict it to viz types whose data maps +faithfully to a single plain query; anything else risks silently wrong or +incomplete results. +""" + +from __future__ import annotations + +from typing import Any + + +def adhoc_filters_to_query_filters( + adhoc_filters: list[dict[str, Any]], +) -> list[dict[str, Any]]: + """ + Convert saved adhoc filters into QueryObject filter clauses. + + Adhoc filters use ``{subject, operator, comparator}`` while a query object + expects ``{col, op, val}``. Only ``SIMPLE`` filters are convertible; custom + SQL filters have no equivalent here and are dropped. + """ + result: list[dict[str, Any]] = [] + for flt in adhoc_filters or []: + if flt.get("expressionType") == "SIMPLE": + result.append( + { + "col": flt.get("subject"), + "op": flt.get("operator"), + "val": flt.get("comparator"), + } + ) + return result + + +def columns_from_form_data(form_data: dict[str, Any]) -> list[Any]: + """ + Derive the query's grouping/raw columns from form data. + + Handles raw-mode tables (``all_columns``/``columns``), an ``x_axis`` (string + or adhoc column), and ``groupby`` dimensions, de-duplicating while preserving + order. + """ + if form_data.get("query_mode") == "raw" and ( + form_data.get("all_columns") or form_data.get("columns") + ): + return list(form_data.get("all_columns") or form_data.get("columns") or []) + + groupby_columns: list[Any] = form_data.get("groupby") or [] + raw_columns: list[Any] = form_data.get("columns") or [] + columns = raw_columns.copy() if "columns" in form_data else groupby_columns.copy() Review Comment: **Suggestion:** This selection logic drops `groupby` dimensions whenever the `columns` key exists, even if `columns` is an empty list. Many saved form-data payloads include empty `columns` by default, so exports can run with missing grouping columns and incorrect results. [logic error] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - Exported table sheets lose group-by columns. - Aggregations no longer grouped as in original charts. - Dashboard Excel exports show semantically different data. - Issue affects legacy charts without saved query_context. - Impacts REBUILD_VIZ_TYPES such as table exports. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Create or locate a legacy chart with viz_type "table" whose saved form data (stored in `Slice.params`) contains a non-empty `groupby` (for example `["country"]`) and an explicitly present but empty `columns` list (this is the payload read in `superset/tasks/export_dashboard_excel.py:145` when `_resolve_query_context()` parses `chart.params`). 2. Add this chart to a dashboard and ensure it has no saved `query_context` (the column is NULL or an empty/invalid JSON string), so that `_resolve_query_context()` at `superset/tasks/export_dashboard_excel.py:130-156` will execute the form-data rebuild path instead of using a saved context. 3. Trigger a dashboard Excel export (the Flask route enqueues the Celery task in `superset/tasks/export_dashboard_excel.py`, which calls `_build_workbook()` at `superset/tasks/export_dashboard_excel.py:244` and iterates charts via `get_charts_in_layout_order(dashboard)` at line 261). 4. For this table chart, `_build_workbook()` calls `_resolve_query_context()` (line 269), which calls `build_query_context_from_form_data()` at `superset/common/form_data_query_context.py:92`; inside it, `columns_from_form_data()` at line 65 executes the logic at lines 78β80: because the `columns` key is present (even though it is an empty list), `columns = raw_columns.copy()` sets `columns` to `[]` and discards the non-empty `groupby_columns`, so the rebuilt query context has no grouping columns, and the exported Excel sheet aggregates metrics without the expected group-by dimension, producing different results than the chart in Explore. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e8902e9121584570900b77c76d63164c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e8902e9121584570900b77c76d63164c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/common/form_data_query_context.py **Line:** 78:80 **Comment:** *Logic Error: This selection logic drops `groupby` dimensions whenever the `columns` key exists, even if `columns` is an empty list. Many saved form-data payloads include empty `columns` by default, so exports can run with missing grouping columns and incorrect results. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=1b985045c25af740e320122a11c832960d66fd2d8a12b0683b8c713a8f0b87cf&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=1b985045c25af740e320122a11c832960d66fd2d8a12b0683b8c713a8f0b87cf&reaction=dislike'>π</a> -- 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]
