feat(dashboard): handle empty chart query context in Excel export - #42284
feat(dashboard): handle empty chart query context in Excel export#42284hughhhh wants to merge 10 commits into
Conversation
… export
The export guard only skipped charts whose query_context was missing
(None/blank). A present-but-unusable context slipped through and failed
mid-export: "null" parsed to None and raised TypeError on item
assignment, while "{}"/{"queries": []} failed schema validation — all
landing in the generic error bucket with a noisy traceback instead of
the "no query context" remediation ("re-save in Explore"), which is the
correct guidance for every one of these cases.
Replace the shallow truthiness check with `_has_empty_query_context`,
which also treats an unparseable, non-object, or query-less context as
empty, and route all of them to ERROR_NO_QUERY_CONTEXT. Add a
parametrized test over blank/null/{}/empty-queries/malformed inputs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l export Charts persist a query_context only once (re-)saved in Explore, so older charts have params (form data) but no context and were skipped entirely by the data export. When the saved context is empty, synthesize one from the chart's form data (columns, metrics, adhoc filters, time range) and run that instead. The rebuild is a generic single-query mapping — it does not reproduce plugin post-processing (pivot, rolling, forecast) or multi-query charts — so it is gated to a conservative viz-type allowlist (EXCEL_EXPORT_REBUILD_VIZ_TYPES: table, big_number_total, big_number, pie). Charts of any other type without a saved context are still skipped and listed for re-save, so no chart exports silently wrong or incomplete data. The form-data -> query-context logic lives in a new shared module (superset.common.form_data_query_context), mirroring the approach the MCP chart compile/preview path already uses. Adds unit tests for the builder and export integration tests for the eligible-rebuild and ineligible-skip paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #f56d0eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42284 +/- ##
==========================================
- Coverage 65.25% 65.04% -0.21%
==========================================
Files 2795 2789 -6
Lines 157639 156909 -730
Branches 36052 35772 -280
==========================================
- Hits 102860 102061 -799
- Misses 52802 52871 +69
Partials 1977 1977
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #4ba2e4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
How Has This Been Tested? (real full stack, Docker)Validated end-to-end against the live stack (Flask + Celery worker + Postgres + Redis + MinIO/S3 + Mailpit/SMTP) serving this branch via bind mount. Bring-up (from workspace root, with the S3/SMTP verify overlay): Test case — the natural one: Superset's example dashboards ship charts that have no saved
Before this PR every one of these 8 charts would have been skipped (empty workbook); now the 3 rebuildable ones export with correct data. Non-visual proof:
Screenshots (in |
Add unit tests for the previously-uncovered branches of the empty query-context handling: x_axis-as-adhoc-dict and the granularity_sqla column fallback in the form-data builder (now 100%), and the eligible-viz skip paths in _resolve_query_context when the saved form data is unparseable, non-object, or has no datasource. All new source lines are now exercised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| 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() |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Good catch — valid latent bug. Fixed in de7dfc1: columns_from_form_data now prefers raw columns only when non-empty, otherwise falls back to groupby, so a stale columns: [] no longer drops the grouping. Added a unit test.
| "columns": columns, | ||
| "metrics": metrics, | ||
| "orderby": form_data.get("orderby") or [], | ||
| "filters": adhoc_filters_to_query_filters(form_data.get("adhoc_filters", [])), |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Fixed in de7dfc1: build_query_context_from_form_data now also honors legacy simple filters (already in {col, op, val} shape) alongside adhoc_filters, dropping malformed entries. Added a unit test. Legacy charts (the target of this feature) are exactly where a top-level filters field can appear.
|
|
||
| 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 |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Fixed in de7dfc1: _rebuild_viz_types now checks is None explicitly instead of or, so an operator can disable the rebuild with an explicit empty set instead of it silently falling back to the default allowlist. Added a parametrized unit test (None → default, empty set → disabled, override → honored).
Code Review Agent Run #54836dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Address review of the empty query-context rebuild:
- columns_from_form_data no longer lets a stale, explicitly-present-but-
empty `columns: []` key shadow the group-by dimensions (which silently
dropped grouping and changed the aggregation). Prefer raw columns only
when non-empty, else fall back to groupby.
- build_query_context_from_form_data now also honors legacy simple
`filters` (already in QueryObject {col, op, val} shape) in addition to
`adhoc_filters`, so legacy charts export the same filtered data they
show; malformed entries are dropped.
- _rebuild_viz_types checks the config for None explicitly instead of
using `or`, so an operator can disable the rebuild with an empty set
instead of it silently falling back to the default allowlist.
Adds unit tests for each case.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #fe22c4Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev's Review Agent — #42284 · HEAD de7dfc1request changes — the rebuilt query context silently drops the chart's time range, ordering and custom-SQL filters, so allowlisted charts export a different dataset than the chart shows — the exact outcome the PR promises never happens. The three earlier bot findings are genuinely fixed at this HEAD (empty- Note: the 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
Findings are code-verified against this HEAD, not runtime-verified (test suite not executed). |
Address EnxDev review — the generic rebuild silently dropped several query aspects, so allowlisted charts could export a different dataset than they show: - Ordering: derive `orderby` from order_by_cols (raw) or the sort metric / first-metric-descending (aggregate), so a `row_limit` returns the chart's top-N instead of an arbitrary N. - Time range: set `granularity` (from granularity/granularity_sqla) so `time_range` is actually applied — but only when there is an active range, so a numeric column saved as granularity_sqla with no range isn't forced through date bucketing (verified against a real export). - Custom SQL filters: map `SQL` adhoc filters + legacy top-level `where` into `extras.where`/`extras.having` by clause instead of dropping them. - Table specifics: carry `percent_metrics` into metrics and pass `time_grain_sqla` through `extras`. - Big Number: only promote granularity_sqla to a grouping column for the trendline viz (`big_number`), never `big_number_total`. Also point the MCP chart compile/preview helpers (adhoc_filters_to_query_filters, _build_query_columns) at this shared module so they stop diverging (preview_utils still had the pre-fix `columns` bug), and address nits: drop the `-O`-stripped assert by restructuring the loop, parse the saved query_context once, and correct the shallow-copy comment. Adds unit tests for every derived field plus an export test asserting the full query body reaches ChartDataQueryContextSchema().load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — all findings addressed in 🔴 Functional
🟡 Should-fix
🔵 Nits — restructured the loop to drop the
|
| 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() |
There was a problem hiding this 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]
Severity Level: Minor 🧹
- ⚠️ Docstring overstates de-duplication actually implemented.
- ⚠️ Duplicate columns unlikely from normal frontend form_data.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| ineligible = _chart(20, "Ineligible", viz_type="mixed_timeseries") | ||
| ineligible.query_context = None | ||
| ineligible.params = json.dumps({"groupby": ["x"], "metrics": ["count"]}) | ||
| ineligible.datasource_id = 5 |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Ineligible viz allowlist gating may remain untested.
- ⚠️ Export skip reason obscured by missing datasource_type.
- ⚠️ Tests may pass while rebuild allowlist misconfigured.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixCo-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #f00e56Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| if form_data.get("time_grain_sqla"): | ||
| extras["time_grain_sqla"] = form_data["time_grain_sqla"] | ||
|
|
||
| time_range = form_data.get("time_range") or "No filter" |
There was a problem hiding this comment.
The rebuild only reads time_range, but older charts may still use since/until. In that case, the export falls back to "No filter" and could export the full history instead of the chart's configured time range.
Should we fall back to since/until when time_range is missing? It would also be worth adding a regression test for this case.
There was a problem hiding this comment.
Fixed in 53350ae — when time_range is absent the rebuild now falls back to since/until ("{since} : {until}") before defaulting to "No filter", so older charts export their configured range. Added a regression test.
| 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 |
There was a problem hiding this comment.
adhoc_filters_to_query_filters now only converts SIMPLE filters with a WHERE clause, while the previous chart_utils implementation converted all SIMPLE filters. This means SIMPLE HAVING filters previously handled by mcp_service/chart/compile.py and preview_utils.py are now silently dropped.
Should we preserve the previous behavior here and handle both WHERE and HAVING filters?
There was a problem hiding this comment.
Reverted in 53350ae — adhoc_filters_to_query_filters again converts all SIMPLE filters regardless of clause, so SIMPLE HAVING filters the MCP compile/preview path relied on are no longer dropped. Added a test pinning that a SIMPLE HAVING filter still converts. (Note: the frontend processFilters does drop SIMPLE-HAVING, but preserving the prior shared behavior here is the safer choice and keeps MCP unchanged.)
| 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() | ||
|
|
||
| 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 |
There was a problem hiding this comment.
columns_from_form_data now checks the truthiness of raw_columns instead of whether "columns" exists in form_data. As a result, columns: [] no longer takes precedence over groupby in the MCP preview/compile path.
This looks like an improvement, but it changes the previous behavior. Could we confirm this is intentional and add an MCP-path test to cover it?
There was a problem hiding this comment.
Confirmed intentional — this fixes a real bug (an earlier reviewer flagged that a stale, present-but-empty columns: [] was silently dropping groupby and changing the aggregation). Now that preview_utils._build_query_columns delegates to the shared columns_from_form_data, the MCP compile/preview path gets the same fix. Added an MCP-path test (test_build_query_columns_empty_columns_key_keeps_groupby) calling preview_utils._build_query_columns({"groupby": ["country"], "columns": []}) and asserting ["country"].
| # Table percent metrics are computed as additional query metrics. | ||
| if viz_type == "table" and form_data.get("percent_metrics"): | ||
| metrics = [*metrics, *form_data["percent_metrics"]] |
There was a problem hiding this comment.
For table charts, percent_metrics are added as regular query metrics without applying the percentage/contribution post-processing.
This means the exported column contains the raw aggregate instead of the "% of total" shown in the chart.
Should we exclude percent_metrics from the rebuild to avoid exporting values that don't match what the user sees in the chart?
There was a problem hiding this comment.
Good call — excluded in 53350ae. Since the rebuild doesn't apply the contribution/percent post-processing, carrying percent_metrics would export raw aggregates under a column the user expects to be a "% of total", i.e. silently-wrong values. Omitting them is the safer choice (consistent with the rest of this rebuild); time_grain_sqla is still carried. Test updated to assert they're excluded.
- Exclude percent_metrics from the rebuild: the chart shows them as a "% of total" via contribution post-processing the rebuild can't apply, so adding them as plain metrics would export raw aggregates that don't match the chart. - Restore converting all SIMPLE adhoc filters (not just WHERE-clause) so SIMPLE HAVING filters the MCP compile/preview path relied on are not silently dropped. - Fall back to legacy since/until when time_range is absent, so older charts export their configured range instead of the full history. - Add an MCP-path test pinning that an explicitly empty `columns: []` no longer shadows `groupby` (intentional behavior change), plus tests for the since/until fallback and SIMPLE HAVING conversion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| if flt.get("expressionType") == "SIMPLE": | ||
| result.append( | ||
| { | ||
| "col": flt.get("subject"), | ||
| "op": flt.get("operator"), | ||
| "val": flt.get("comparator"), | ||
| } |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Aggregate conditions are applied before chart aggregation.
- ❌ Legacy chart Excel sheets can contain incorrect totals.
- ⚠️ Databases may reject aggregate predicates in WHERE.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| if not isinstance(parsed, dict) or not parsed.get("queries"): | ||
| return None | ||
| return parsed |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Affected chart data sheets are omitted from exports.
- ⚠️ Users receive generic errors instead of remediation guidance.
- ⚠️ Malformed saved contexts bypass missing-context handling.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| # Conservative by default: only charts whose data maps faithfully to a single | ||
| # plain query (no post-processing, no multi-query fan-out). Operators can | ||
| # override via ``EXCEL_EXPORT_REBUILD_VIZ_TYPES``. | ||
| REBUILD_VIZ_TYPES = {"table", "big_number_total", "big_number", "pie"} |
There was a problem hiding this comment.
REBUILD_VIZ_TYPES only checks the viz type, but some behaviors that require post-processing or multiple queries depend on the form data.
For example, a table with time_compare requires additional queries/post-processing, and a Big Number with rolling/resampling can also produce values that differ from the raw query result. Rebuilding these directly could therefore export incomplete or incorrect data.
Should we also skip the rebuild when rolling_type, resample_rule, time_compare, or aggregation === "raw" is present?
It would be good to add a regression test for at least the time_compare case.
| granularity = form_data.get("granularity") or form_data.get("granularity_sqla") | ||
| if granularity and time_range != "No filter": | ||
| query["granularity"] = granularity | ||
| if form_data.get("row_limit"): | ||
| query["row_limit"] = form_data["row_limit"] |
There was a problem hiding this comment.
For Big Number trendlines, granularity is only set when time_range != "No filter". However, time_grain_sqla is only applied to the column identified by granularity.
This means a Big Number with no time filter can export rows at the raw timestamp precision instead of the configured time grain.
Should we always set granularity to the promoted time column for Big Number trendlines, regardless of time_range?
It would also be good to add a regression test for a Big Number with time_grain_sqla and no time_range.
| """ | ||
| result: list[dict[str, Any]] = [] | ||
| for flt in adhoc_filters or []: | ||
| if flt.get("expressionType") == "SIMPLE": |
There was a problem hiding this comment.
Restoring support for all SIMPLE filters makes sense for MCP, but it creates a difference between the chart and the export. The chart only applies SIMPLE filters with clause === "WHERE", while the export would also apply HAVING filters, potentially resulting in fewer rows than what the user sees.
Should we filter out HAVING filters in build_query_context_from_form_data while keeping the shared helper unchanged for MCP?
Code Review Agent Run #0a2f4bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Follow-up to the async dashboard "Export Data to Excel" feature to properly handle charts that have no saved
query_context. A chart persists itsquery_contextonly once it has been (re-)saved in Explore, so older charts haveparams(form data) but no context and previously failed mid-export or fell into the generic error bucket. This PR first makes such charts skip cleanly and get listed under the "no query context" remediation (coveringnull/{}/{"queries": []}/malformed values, not just a blank one), then goes further and synthesizes a query context from the chart's saved form data so those charts still export. Because the rebuild is a generic single-query mapping that does not reproduce plugin post-processing (pivot, rolling, forecast) or multi-query charts, it is gated behind a conservative, configurable viz-type allowlist (EXCEL_EXPORT_REBUILD_VIZ_TYPES, defaulttable/big_number_total/big_number/pie); anything else without a saved context is still skipped and listed for re-save, so no chart ever exports silently wrong or incomplete data.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend export-task behavior only.
TESTING INSTRUCTIONS
Run the unit tests:
pytest tests/unit_tests/tasks/test_export_dashboard_excel.py tests/unit_tests/common/test_form_data_query_context.py. They cover the clean-skip cases (blank/null/{}/empty-queries/malformed), the form-data rebuild for an eligible viz type, and the skip-with-notice fallback for an ineligible (multi-query) viz type. End-to-end: export a dashboard containing a legacy chart with no savedquery_context— an eligible chart (e.g. a table) now appears as a data sheet, while an ineligible one is listed in the email as needing a re-save in Explore.ADDITIONAL INFORMATION
🤖 Generated with Claude Code