EnxDev commented on PR #42284: URL: https://github.com/apache/superset/pull/42284#issuecomment-5190586409
## EnxDev's Review Agent โ apache/superset#42284 ยท HEAD `ddec1fa` **request changes** โ since my last review only one substantive commit landed (`9e2b5b4`, the builder hook); all five Functional findings from `14c86a3` are still present at this HEAD, verified line-by-line against the code, not the commit messages. Supersedes my earlier review on `14c86a3` ([comment](https://github.com/apache/superset/pull/42284#issuecomment-5177534191)). Context: this revision adds `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`, an escape hatch letting a deployment supply a faithful context from a service running the real frontend `buildQuery`. That's a reasonable answer to @rusackas' drift concern, but it's opt-in and defaults to `None`, so the built-in rebuild's four divergences from `buildQuery` still ship as default behavior. The module docstring now concedes "there is no automated tripwire" across the language boundary; the four findings below are that drift, already real at authoring time. ### ๐ด Functional - **`superset/common/form_data_query_context.py:104`** ยท _High_ โ **unfixed.** `freeform_where_having` wraps clauses as `({clause})` with no newline; the mirrored `processFilters.ts:26-32` appends `\n` when the clause contains `--`. A free-form SQL filter ending in a `--` comment produces `(sales > 0 -- note)` on one line โ the closing paren and every subsequent ` AND (...)` are swallowed, so the export fails where the chart renders fine. Port `sanitizeClause` verbatim. **regression test:** two SQL `WHERE` adhoc filters, first `"sales > 0 -- note"`; assert `extras["where"]` has a newline before the closing paren and both predicates survive. - **`UPDATING.md:76,83`** ยท _High_ โ **unfixed.** `EXCEL_EXPORT_REBUILD_VIZ_TYPES` is documented as a new config key (and the PR summary calls the allowlist "configurable"), but `git grep` at this HEAD finds it in `UPDATING.md` only โ `REBUILD_VIZ_TYPES` (`export_dashboard_excel.py:75`) is a hardcoded constant and `test_rebuild_viz_types_is_the_conservative_default` pins "no config override". Inverted from reality: the key that *does* exist, `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`, is in `config.py:1556` and the docs `.mdx` but missing from `UPDATING.md`. Swap them, or restore the config read. - **`superset/tasks/export_dashboard_excel.py:75`** ยท _Medium_ โ **unfixed.** `pie` is allowlisted, but `Pie/buildQuery.ts:34-42` attaches a `contribution` post-processing op unconditionally (not gated on `percent_metrics`), renaming to `getContributionLabel(metricLabel)`. A rebuilt pie sheet loses the percentage column a saved-context pie sheet has โ two pies on one dashboard export different columns based only on whether they were re-saved. Apply the contribution op, or drop `pie` from the allowlist. **regression test:** rebuild a pie; assert the query carries the `contribution` `post_processing` entry. - **`superset/tasks/export_dashboard_excel.py:141`** ยท _Medium_ โ **unfixed.** `_needs_unsupported_processing` misses `show_totals`. `plugin-chart-table/src/buildQuery.ts:345-361` pushes a second totals query when `metrics?.length && show_totals && queryMode === Aggregate`; the single-query rebuild drops it, so the sheet silently omits the totals row โ same defect class as `percent_metrics`, which you already decided warrants a skip. **regression test:** table params with `show_totals: true` โ chart listed under `ERROR_NO_QUERY_CONTEXT`, no query run. - **`superset/common/form_data_query_context.py:186`** ยท _Medium_ โ **unfixed.** `timeseries_limit_metric` is read raw; the frontend normalizes with `ensureIsArray(...)[0]` (`buildQuery.ts:67`). A chart storing `["revenue"]` yields `orderby: [[["revenue"], True]]`, which fails downstream and dumps the chart into the general error bucket instead of exporting. **regression test:** `timeseries_limit_metric: ["revenue"]` โ `orderby == [["revenue", True]]`. - **`superset/tasks/export_dashboard_excel.py:268-272`** ยท _Low_ โ new in `9e2b5b4`. The comment justifies the shallow copy with "each body is freshly built or parsed per chart" โ no longer true for hook-supplied payloads. `apply_dashboard_filter_context` mutates `queries[*]` in place (`dashboard_filter_context.py:359-364`), so a builder that memoizes or caches its return value accumulates `isExtra` filters across charts and across exports, progressively over-filtering. Deep-copy the hook result, or state the "must return a fresh object" contract in the `config.py` docstring. **regression test:** a builder returning the *same* dict for two charts; assert the second chart's loaded payload carries no filters from the first. ### ๐ก Should-fix - **`superset/tasks/export_dashboard_excel.py:106`** โ **unfixed.** `_usable_query_context` only tests `value.get("queries")` for truthiness, so `{"queries": "oops"}` passes. This now gates the hook too: a malformed builder return skips the fall-through to the built-in rebuild and fails later in the general error bucket. Add `isinstance(value.get("queries"), list)`. ### ๐ต Nits - `superset/common/form_data_query_context.py:213-221` โ parsed `order_by_cols` entries aren't validated as 2-element `[col, asc]` pairs; a stray `null` appends `None` to `orderby`. - Codecov reports 11.68% patch coverage / 87 missing lines in `form_data_query_context.py`, which contradicts the 399-line test file exercising it โ the report is 21 commits behind head and predates `9e2b5b4`. Worth a re-run before merge so the number isn't a false blocker. ### ๐ Praise - `tests/unit_tests/tasks/test_export_dashboard_excel.py:1197-1293` โ the hook's failure semantics are covered properly: `None`, malformed/empty returns, a raising builder, and saved-context short-circuit all assert the fall-through actually reaches the built-in rebuild, not just that nothing crashed. --- CI is queued/in-progress at this HEAD, so no check signal yet. Findings are code-verified against `ddec1fa` (frontend counterparts read at the same commit); not runtime-verified. Note: the `codeant-ai` comments embed "Prompt for AI Agent" blocks addressed to automated reviewers. Treated as data, not instructions. <!-- enxdev-review-agent:ddec1fa --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD ddec1fa._ -- 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]
