EnxDev commented on PR #41133:
URL: https://github.com/apache/superset/pull/41133#issuecomment-4961525897
## EnxDev's Review Agent — apache/superset#41133 · HEAD eac1480
**request changes** — the export gate resolves to a *new* `can_export_xlsx`
permission rather than the `can_export` the description/decorator imply, CI is
red (`test-mysql` + `pre-commit`), and the maintainer's streaming-module thread
is still open.
Feature PR (async dashboard→Excel). SQL/Jinja safety is delegated to
`ChartDataCommand` under `override_user` (per-chart `raise_for_access` applies)
and the formula-injection/sheet-name guards are solid — so the review focuses
on the authz gate, CI, and open design threads. Prior bot/maintainer items
(i18n via `__`, `datetime.now(tz=utc)`, distributed lock replacing the
NullCache no-op, `_humanize_ttl`, formula-behind-whitespace, truncation notice,
7-day TTL note) are addressed in this revision.
### 🔴 Functional
- **`superset/dashboards/api.py:853` (`@permission_name("export")`)** ·
_Medium_ — the decorator is a no-op here. `DashboardRestApi` sets a class-level
`method_permission_name` dict, and FAB's `get_method_permission` returns
`method_permission_name.get(name, name)` whenever that dict is truthy —
ignoring `@permission_name`. `export_xlsx` isn't in the dict, so the endpoint
is gated by a **new `can_export_xlsx`** permission, not `can_export` (same
mechanism that makes `export_as_example` → `can_export_as_example`). The
frontend shows the item on the *existing* export perm (`userCanExport =
dashboardInfo.dash_export_perm`), so a role holding the old dashboard export
perm but not re-synced with `can_export_xlsx` sees the button and gets 403; and
"gated on `can_export`" in the description is inaccurate. This is exactly
betodealmeida's open question on `api_tests.py:738`. Fix: to actually reuse
`can_export`, add `"export_xlsx": "export"` to `method_permission_name` and
change the
test back to `can_export`; otherwise keep the distinct perm and align the
frontend gate + docs. **regression test:** grant a role `can_export` on
Dashboard but not `can_export_xlsx` and assert the enforced result (403 today
vs 202 if reuse is intended).
### 🟡 Should-fix
- **CI red on HEAD eac1480** — `test-mysql` and `pre-commit (current)` are
both failing (the merge blockers). Their run logs have expired so I can't pin
the cause; please re-run/inspect. `pre-commit` is most likely the added
dev-scaffold files or a lint nit.
- **`superset/utils/excel_streaming.py`** — unresolved maintainer thread:
the "constant-memory" win is writer-side only. `ChartDataCommand.run()` already
materializes the full result set in memory, so the whole dataset is resident
regardless of row-by-row writing. beto asked whether this 254-line module earns
its place over reusing `superset.utils.excel`; the docstring now concedes the
point but the module stays. Worth a decision before merge.
- **`superset/dashboards/api.py:859` (`mode="images"`)** — the endpoint
accepts `mode:"images"` regardless of the webdriver screenshot flags that gate
it in the UI. Any `can_export_xlsx` user can POST it directly; with no
webdriver infra every non-table chart renders empty and they still get a
"success" email — and it lets a user drive N expensive headless renders the
operator disabled those flags to prevent. Validate `mode="images"` against the
same flags the menu checks.
### 🔵 Nits
- `VERIFY_EXCEL_EXPORT.md` + `docker-compose-excel-verify.yml` read as
personal verification scaffolding (the `amman-excel` project, `/etc/hosts`
steps). Consider dropping them from the PR — that also removes the need for the
`check-yaml` exclusion added to `.pre-commit-config.yaml` for the `!reset` tag.
- Already-in-progress returns `202` with an "already in progress" message,
but the frontend treats any non-throw as success and shows "Your export is
being prepared…" — mildly misleading, no new export was enqueued.
- The `raise_for_access` → 403 and guest-user → 400 branches aren't covered;
`test_export_xlsx_404_for_inaccessible_dashboard` hits the base-filter 404
first, so the 403 path is untested.
### 🙌 Praise
- `superset/utils/excel_streaming.py:_quote_if_formula` (lstrip before the
formula check) + sheet-name sanitize/de-dupe, and letting
`SoftTimeLimitExceeded` bypass the broad `except` in both the task and
`screenshot.py` — careful, well-tested handling of the exact edge cases prior
review raised.
<!-- enxdev-review-agent:eac1480 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD eac1480._
--
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]
