codeant-ai-for-open-source[bot] commented on code in PR #42284:
URL: https://github.com/apache/superset/pull/42284#discussion_r3640004099
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -182,6 +182,184 @@ def test_chart_without_query_context_is_skipped(mocks:
dict[str, Any]) -> None:
}
[email protected](
+ "raw_context",
+ [
+ "", # blank
+ "null", # parses to None
+ "{}", # object with no queries
+ '{"queries": []}', # object with an empty queries list
+ "not valid json", # unparseable
+ ],
+)
+def test_chart_with_empty_query_context_is_skipped(
+ mocks: dict[str, Any], raw_context: str
+) -> None:
+ # A present-but-empty/unusable query context is treated the same as a
+ # missing one: the chart is listed under "no query context" and the export
+ # continues, rather than raising mid-export and landing in the general
bucket.
+ good = _chart(10, "Good")
+ empty = _chart(20, "Empty")
+ empty.query_context = raw_context
+ mocks["get_charts_in_layout_order"].return_value = [good, empty]
+ mocks["ChartDataCommand"].return_value.run.return_value = {
+ "queries": [{"colnames": ["a"], "data": [{"a": 1}]}]
+ }
+
+ _run()
+
+ _, kwargs = mocks["email"].build_success_email.call_args
+ assert kwargs["errored"] == {mocks["email"].ERROR_NO_QUERY_CONTEXT: ["20 -
Empty"]}
+ # The empty chart is skipped before any query runs; only the good one runs.
+ mocks["ChartDataCommand"].return_value.run.assert_called_once()
+
+
+def test_empty_query_context_rebuilt_from_form_data_for_eligible_viz(
+ mocks: dict[str, Any],
+) -> None:
+ # An eligible viz type (table) with no saved query context is rebuilt from
+ # its form data and exported instead of being skipped.
+ good = _chart(10, "Good")
+ rebuilt = _chart(20, "Rebuilt", viz_type="table")
+ rebuilt.query_context = None
+ rebuilt.params = json.dumps({"groupby": ["country"], "metrics": ["count"]})
+ rebuilt.datasource_id = 5
+ rebuilt.datasource_type = "table"
+ mocks["get_charts_in_layout_order"].return_value = [good, rebuilt]
+ mocks["ChartDataCommand"].return_value.run.return_value = {
+ "queries": [{"colnames": ["a"], "data": [{"a": 1}]}]
+ }
+
+ _run()
+
+ _, kwargs = mocks["email"].build_success_email.call_args
+ assert kwargs["errored"] == {}
+ # Both charts ran a query (the saved one and the rebuilt one).
+ assert mocks["ChartDataCommand"].return_value.run.call_count == 2
+
+
+def test_empty_query_context_ineligible_viz_is_skipped(
+ mocks: dict[str, Any],
+) -> None:
+ # A viz type outside the rebuild allowlist (mixed_timeseries — a
multi-query
+ # chart the generic rebuild can't reproduce) is skipped, not exported
wrong.
+ good = _chart(10, "Good")
+ ineligible = _chart(20, "Ineligible", viz_type="mixed_timeseries")
+ ineligible.query_context = None
+ ineligible.params = json.dumps({"groupby": ["x"], "metrics": ["count"]})
+ ineligible.datasource_id = 5
Review Comment:
**Suggestion:** This test is intended to prove skip behavior is driven by an
ineligible viz type, but it never sets `datasource_type` on the chart. If
rebuild preconditions require a complete datasource, the chart can be skipped
for missing datasource metadata instead, so the test can pass even when
allowlist gating is broken. Set a valid datasource type so the only reason to
skip is viz ineligibility. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Ineligible viz allowlist gating may remain untested.
- ⚠️ Export skip reason obscured by missing datasource_type.
- ⚠️ Tests may pass while rebuild allowlist misconfigured.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/unit_tests/tasks/test_export_dashboard_excel.py` and
focus on
`test_empty_query_context_ineligible_viz_is_skipped` at lines 241–263 in
`tests/unit_tests/tasks/test_export_dashboard_excel.py`.
2. Inside this test, the ineligible chart is set up at lines 247–250:
`ineligible =
_chart(20, "Ineligible", viz_type="mixed_timeseries")`,
`ineligible.query_context = None`,
`ineligible.params = json.dumps({"groupby": ["x"], "metrics": ["count"]})`,
and
`ineligible.datasource_id = 5`; note that `ineligible.datasource_type` is
never set.
3. Compare this setup with the rebuild tests for eligible viz types:
`test_empty_query_context_rebuilt_from_form_data_for_eligible_viz` (lines
217–239) and
`test_eligible_viz_skipped_when_form_data_unusable` (lines 273–295), where
charts
explicitly set both `datasource_id` and `datasource_type = "table"` before
invoking
`_run()`. This indicates the production rebuild logic in
`superset.tasks.export_dashboard_excel` requires complete datasource
metadata (both id and
type) as a precondition.
4. If the allowlist gating in
`superset.tasks.export_dashboard_excel._rebuild_viz_types()`
(tested at lines 344–360) is accidentally broadened to include
`"mixed_timeseries"`, the
ineligible chart in `test_empty_query_context_ineligible_viz_is_skipped` can
still be
skipped solely because `datasource_type` is missing. The test will pass, but
it will be
exercising the “missing datasource metadata” path instead of the intended
“viz type
outside allowlist” path, so a bug in allowlist gating goes undetected.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=69eeeef853814e52b56c376f6faf54ac&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=69eeeef853814e52b56c376f6faf54ac&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:** tests/unit_tests/tasks/test_export_dashboard_excel.py
**Line:** 247:250
**Comment:**
*Logic Error: This test is intended to prove skip behavior is driven by
an ineligible viz type, but it never sets `datasource_type` on the chart. If
rebuild preconditions require a complete datasource, the chart can be skipped
for missing datasource metadata instead, so the test can pass even when
allowlist gating is broken. Set a valid datasource type so the only reason to
skip is viz ineligibility.
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=6aa3ac667173a7e16b8a1ec4aee090965c5fe4bfd2e1eea6b2f2138934948213&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=6aa3ac667173a7e16b8a1ec4aee090965c5fe4bfd2e1eea6b2f2138934948213&reaction=dislike'>👎</a>
##########
superset/common/form_data_query_context.py:
##########
@@ -0,0 +1,228 @@
+# 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}``. Only ``SIMPLE`` WHERE-clause filters are
+ convertible here; 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"
+ and (flt.get("clause") or "WHERE").upper() == "WHERE"
+ ):
+ result.append(
+ {
+ "col": flt.get("subject"),
+ "op": flt.get("operator"),
+ "val": flt.get("comparator"),
+ }
+ )
+ return result
+
+
+def freeform_where_having(form_data: dict[str, Any]) -> dict[str, str]:
+ """
+ Collect free-form SQL predicates into a query ``extras`` mapping.
+
+ Mirrors ``processFilters`` on the frontend: ``SQL`` adhoc filters (and a
+ legacy top-level ``where``) join into ``extras.where`` / ``extras.having``
by
+ clause, so a chart restricted by a custom SQL predicate exports the same
rows
+ it displays instead of the full, unrestricted result.
+ """
+ where: list[str] = []
+ having: list[str] = []
+ if form_data.get("where"):
+ where.append(form_data["where"])
+ for flt in form_data.get("adhoc_filters") or []:
+ if flt.get("expressionType") == "SQL" and flt.get("sqlExpression"):
+ clause = (flt.get("clause") or "WHERE").upper()
+ (having if clause == "HAVING" else
where).append(flt["sqlExpression"])
+
+ extras: dict[str, str] = {}
+ if where:
+ extras["where"] = " AND ".join(f"({clause})" for clause in where)
+ if having:
+ extras["having"] = " AND ".join(f"({clause})" for clause in having)
+ return extras
+
+
+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 []
+ # Prefer explicit raw columns only when they are actually present; a stale
+ # empty ``columns: []`` key must not shadow the group-by dimensions (which
+ # would silently drop the grouping and change the aggregation).
+ columns = raw_columns.copy() if raw_columns else groupby_columns.copy()
Review Comment:
**Suggestion:** The function documentation claims columns are de-duplicated,
but the implementation only prevents duplicate insertion for `x_axis` and does
not de-duplicate existing `groupby`/`columns` entries. This contradiction can
produce duplicate selected/grouped columns and inconsistent query output;
either actually de-duplicate or correct the contract. [docstring mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Docstring overstates de-duplication actually implemented.
- ⚠️ Duplicate columns unlikely from normal frontend form_data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a dashboard Excel export so `_build_workbook`
(`superset/tasks/export_dashboard_excel.py:255`) runs and iterates charts
from
`get_charts_in_layout_order`.
2. For a chart without saved `query_context` but with saved `params`
(`chart.params`),
`_build_workbook` calls `_resolve_query_context`
(`superset/tasks/export_dashboard_excel.py:137`), which parses
`chart.params` and invokes
`build_query_context_from_form_data` in
`superset/common/form_data_query_context.py:162`.
3. `build_query_context_from_form_data` calls `columns_from_form_data`
(`superset/common/form_data_query_context.py:94`), which at lines 107–112
simply copies
`form_data["columns"]` or `form_data["groupby"]` into `columns` without
de-duplicating
within those lists; it only avoids inserting a duplicate `x_axis` later in
the function.
4. In practice, frontend-generated `form_data` for charts does not contain
duplicate
entries in `columns` or `groupby`, and there is no backend code that injects
duplicates,
so any duplication would require malformed or manually edited `params`; the
mismatch is
between the docstring (“de-duplicating while preserving order”) and
implementation rather
than a reproducible bug in normal usage.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf58f290afa3490db9ef3792c118f38e&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=bf58f290afa3490db9ef3792c118f38e&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:** 107:112
**Comment:**
*Docstring Mismatch: The function documentation claims columns are
de-duplicated, but the implementation only prevents duplicate insertion for
`x_axis` and does not de-duplicate existing `groupby`/`columns` entries. This
contradiction can produce duplicate selected/grouped columns and inconsistent
query output; either actually de-duplicate or correct the contract.
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=9242ad889b888972fee1afda8935a091ceef2f0228aa0755024f735cc2ac3ab3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=9242ad889b888972fee1afda8935a091ceef2f0228aa0755024f735cc2ac3ab3&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]
