codeant-ai-for-open-source[bot] commented on code in PR #42284: URL: https://github.com/apache/superset/pull/42284#discussion_r3658006907
########## superset/common/form_data_query_context.py: ########## @@ -0,0 +1,231 @@ +# 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 — columns, +metrics, filters (including free-form SQL and the time range), ordering and time +grain — mirroring the shared parts of the viz plugins' ``buildQuery``. It does +**not** reproduce plugin post-processing (pivot, contribution/percent +transforms, rolling/forecast) or multi-query fan-out, so callers must restrict it +to viz types whose data maps faithfully to a single plain query. +""" + +from __future__ import annotations + +from typing import Any + +from superset.utils import json + + +def adhoc_filters_to_query_filters( + adhoc_filters: list[dict[str, Any]], +) -> list[dict[str, Any]]: + """ + Convert ``SIMPLE`` adhoc filters into QueryObject filter clauses. + + Adhoc filters use ``{subject, operator, comparator}`` while a query object + expects ``{col, op, val}``. All ``SIMPLE`` filters are converted (matching the + behavior the MCP compile/preview path relied on); free-form ``SQL`` filters + have no ``{col, op, val}`` equivalent and are handled separately (see + :func:`freeform_where_having`). + """ + 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"), + } Review Comment: **Suggestion:** A SIMPLE adhoc filter marked with clause HAVING is always appended to `filters`, which applies it before aggregation as a WHERE predicate. Aggregate conditions such as `COUNT(*) > 5` will therefore produce incorrect results or fail in the database. Route HAVING filters to the query's having expression instead of treating every SIMPLE filter as a row-level filter. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Aggregate conditions are applied before chart aggregation. - ❌ Legacy chart Excel sheets can contain incorrect totals. - ⚠️ Databases may reject aggregate predicates in WHERE. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or use a legacy chart whose saved `params` contain a `SIMPLE` adhoc filter with `clause: "HAVING"` and an aggregate condition, such as a metric count greater than five. 2. Export a dashboard containing that chart through the Excel export task in `superset/tasks/export_dashboard_excel.py`; `_build_workbook()` at lines 272-291 resolves the missing context and invokes `_write_chart_sheets()`. 3. `_resolve_query_context()` at lines 148-164 calls `build_query_context_from_form_data()`, which calls `adhoc_filters_to_query_filters()` at line 192. 4. `adhoc_filters_to_query_filters()` at lines 55-61 converts the filter into the query's `filters` list without inspecting its HAVING clause, while `freeform_where_having()` at lines 79-88 only handles SQL filters. The generated query therefore applies the aggregate predicate as a row-level WHERE filter, producing incorrect results or a database validation error. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=23aa6f0a48b944c0aa44664325e8e714&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=23aa6f0a48b944c0aa44664325e8e714&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:** 55:61 **Comment:** *Logic Error: A SIMPLE adhoc filter marked with clause HAVING is always appended to `filters`, which applies it before aggregation as a WHERE predicate. Aggregate conditions such as `COUNT(*) > 5` will therefore produce incorrect results or fail in the database. Route HAVING filters to the query's having expression instead of treating every SIMPLE filter as a row-level filter. 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=a36c8e450affb6f05dbc803ff43b76f72d1673709f7a562547317ab62ec0fdc5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=a36c8e450affb6f05dbc803ff43b76f72d1673709f7a562547317ab62ec0fdc5&reaction=dislike'>👎</a> ########## superset/tasks/export_dashboard_excel.py: ########## @@ -96,6 +103,67 @@ def _chart_label(chart: Any) -> str: return f"{chart.id} - {chart.slice_name or ''}".strip() +def _saved_query_context(raw: Any) -> dict[str, Any] | None: + """ + The chart's saved query context parsed to a dict, or ``None`` when it is + missing or unusable. + + Returns ``None`` for a 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 treated + the same as a missing context. + """ + if not raw: + return None + try: + parsed = json.loads(raw) + except (TypeError, ValueError): + return None + if not isinstance(parsed, dict) or not parsed.get("queries"): + return None + return parsed Review Comment: **Suggestion:** `_saved_query_context` only checks that `queries` is truthy, so malformed contexts such as `{"queries": "invalid"}` or other non-list values are accepted and passed to `ChartDataCommand`. This causes validation or iteration failures to be reported as a generic chart export error instead of being classified as missing query context and listed for remediation. Require `queries` to be a non-empty list with the expected query-object structure before returning the parsed context. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Affected chart data sheets are omitted from exports. - ⚠️ Users receive generic errors instead of remediation guidance. - ⚠️ Malformed saved contexts bypass missing-context handling. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Persist a chart with `query_context` containing valid JSON such as `{"queries": "invalid"}`; this is accepted by `json.loads()` at line 119 and produces a truthy `queries` value. 2. Include that chart in a data-mode dashboard Excel export; `_build_workbook()` at lines 272-291 calls `_resolve_query_context()` for the chart. 3. `_saved_query_context()` at lines 122-124 returns the parsed dictionary because it checks only that `queries` is truthy, not that it is a non-empty list of query objects. 4. `_write_chart_sheets()` receives the malformed payload at line 291 and passes it into the downstream chart-data execution path, where schema validation or query iteration fails. The exception is handled as a generic chart export failure instead of being classified under `email.ERROR_NO_QUERY_CONTEXT` for re-saving. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=97e08063b0114f4da5da48eb585e4715&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=97e08063b0114f4da5da48eb585e4715&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:** 122:124 **Comment:** *Possible Bug: `_saved_query_context` only checks that `queries` is truthy, so malformed contexts such as `{"queries": "invalid"}` or other non-list values are accepted and passed to `ChartDataCommand`. This causes validation or iteration failures to be reported as a generic chart export error instead of being classified as missing query context and listed for remediation. Require `queries` to be a non-empty list with the expected query-object structure before returning the parsed context. 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=a783dd45d0c570421663b1480006712286c8a8303d8729ad0e6d57f4805e7163&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=a783dd45d0c570421663b1480006712286c8a8303d8729ad0e6d57f4805e7163&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]
