This is an automated email from the ASF dual-hosted git repository. EnxDev pushed a commit to branch enxdev/feat/dashboard-excel-export-sync-fallback in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3108f41167244ea46598385d0c14cd209484c505 Author: Enzo Martellucci <[email protected]> AuthorDate: Wed Sep 9 15:04:00 2026 +0200 fix(dashboard): keep image exports queued, list skips in the workbook Follow-up to the inline Excel export, addressing three gaps in it: An image export draws every chart through the headless webdriver, one browser session at a time. No row budget bounds that, so serving it inline was exactly the gateway timeout the budget exists to prevent; without export storage it is now refused with a message saying image exports run in the background. The skipped-charts list moves into the workbook, on an "Export Summary" sheet whenever any chart is skipped rather than only when every chart is. The list then travels with the file however it is delivered, which is the only way an export served as a download can report it -- and matches the sheet #43805 adds. Query contexts are resolved once. The row budget already had to resolve them to size the export, and the builder resolved them again to run it: with EXCEL_EXPORT_QUERY_CONTEXT_BUILDER pointed at a service, that is both a doubled cost and a real hazard, since one set of queries could be measured and a different set run. The budget now returns an InlineExportPlan carrying what it resolved, and the builder runs those contexts as-is. The export action also reports progress: while a request is in flight both Excel actions are disabled and the one clicked reads "Preparing export…", restored when the file downloads, the queued message arrives, or it fails. The server's lock prevents duplicate work but told the user nothing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- UPDATING.md | 13 ++- .../using-superset/exporting-dashboard-data.mdx | 26 +++-- .../DownloadMenuItems/DownloadMenuItems.test.tsx | 109 ++++++++++++++++++ .../components/menu/DownloadMenuItems/index.tsx | 22 +++- superset/dashboards/api.py | 89 ++++++++++---- superset/dashboards/excel_export/sync_budget.py | 94 +++++++++------ superset/dashboards/excel_export/workbook.py | 40 +++++-- tests/integration_tests/dashboards/api_tests.py | 64 ++++++++++- .../dashboards/test_excel_export_sync_budget.py | 100 +++++++++------- .../dashboards/test_excel_export_workbook.py | 128 +++++++++++++++++++++ .../tasks/test_export_dashboard_excel.py | 28 +++++ 11 files changed, 578 insertions(+), 135 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index e944b087f43..62a421c0526 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -522,8 +522,9 @@ configuration at all. Because it has to finish inside a single request, the direct-download path is bounded by `EXCEL_EXPORT_SYNC_MAX_ROWS` (default `100_000`): the export sums the `row_limit` of every query it would run and refuses, before running any of them, -when the total is higher or when any query has no finite limit. The refusal is a -`400` naming `EXCEL_EXPORT_S3_BUCKET` as the fix. +when the total is higher or when any query has no finite limit. `mode=images` is +refused there outright, since webdriver rendering is not work a request can wait +on. Both refusals are a `400` naming `EXCEL_EXPORT_S3_BUCKET` as the fix. API clients should note that `POST /api/v1/dashboard/<id>/export_xlsx/` now answers either `202` with a job id (queued) or `200` with the `.xlsx` itself @@ -544,9 +545,11 @@ types (`table`, `big_number_total`, `big_number`, `pie`) the export rebuilds a query context from the chart's saved form data so those charts still export. The rebuild is a single-query mapping and does **not** reproduce plugin post-processing (pivot, rolling, forecast) or multi-query charts, so any chart of -another type without a saved query context is skipped — listed in the email on -the queued path, and simply absent from the workbook on a direct download — for -the user to re-save. To cover those types, set `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER` +another type without a saved query context is skipped and named on an "Export +Summary" worksheet in the workbook, for the user to re-save. That sheet is new: +previously it appeared only when *every* chart was skipped, and it now also +lists partial failures, so the list travels with the file rather than only in +the email. To cover those types, set `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER` to a callable that receives the chart's form data and returns a query-context payload (or `None` to fall back to the built-in rebuild) — for example one backed by a service that runs the chart's real frontend `buildQuery`. diff --git a/docs/docs/using-superset/exporting-dashboard-data.mdx b/docs/docs/using-superset/exporting-dashboard-data.mdx index ce3a8961863..5e6e7420573 100644 --- a/docs/docs/using-superset/exporting-dashboard-data.mdx +++ b/docs/docs/using-superset/exporting-dashboard-data.mdx @@ -19,8 +19,9 @@ export storage configured: email with a time-limited download link. - **Without one** the workbook is built while the request is open and downloads straight to the browser — no worker, bucket or email needed. Because it has to - finish inside a single request, this path only accepts exports below - `EXCEL_EXPORT_SYNC_MAX_ROWS` (see [Prerequisites](#prerequisites)). + finish inside a single request, this path only accepts data exports below + `EXCEL_EXPORT_SYNC_MAX_ROWS`, and never image exports (see + [Prerequisites](#prerequisites)). ## Using the export @@ -35,6 +36,9 @@ rendered image (tables stay tabular) instead of exporting raw data. Because it renders charts through the headless webdriver, this option only appears when the webdriver screenshot feature flags are enabled (see the prerequisites below); which viz types stay tabular is controlled by `EXCEL_EXPORT_TABLE_VIZ_TYPES`. +Rendering is far too slow to hold a request open, so image exports always run in +the background: without an export bucket the action is refused with a message +saying so. Notes on the generated workbook: @@ -47,9 +51,10 @@ Notes on the generated workbook: `big_number_total` or `pie`, by rebuilding the query from the chart's saved form data. Charts of other types — and charts relying on post-processing the rebuild can't reproduce — are skipped; open the chart in Explore and re-save it - to include it next time, or configure `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`. A - queued export lists the skipped charts in its email; a direct download has no - email to list them in, so they are simply absent from the workbook. + to include it next time, or configure `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`. + Skipped charts are named on an **Export Summary** worksheet in the workbook + itself, so the list travels with the file however it was delivered; a queued + export also lists them in its email. - Row counts per sheet are capped the same way as the chart-level CSV/Excel export (`ROW_LIMIT`, bounded by `SQL_MAX_ROW`), and never exceed Excel's per-sheet maximum. @@ -64,7 +69,8 @@ total — or one where any chart has no finite row limit — is refused up front a message pointing here, rather than being started and left to hit the web server's request timeout. Raise the limit only as far as that timeout allows. -To lift the size ceiling, configure the background path. It requires: +To lift the size ceiling — and to enable **Export Images to Excel** at all — +configure the background path. It requires: 1. **The `boto3` dependency.** It is not installed by default; install it with `pip install apache-superset[excel-export]`. Without it, exports fail and the @@ -78,7 +84,8 @@ To lift the size ceiling, configure the background path. It requires: using the same settings as alerts & reports (`SMTP_*`, `EMAIL_REPORTS_SUBJECT_PREFIX`). -**Export Images to Excel** additionally requires a working headless webdriver — +**Export Images to Excel** requires the background path above (it is refused +without an export bucket) plus a working headless webdriver — the same infrastructure scheduled reports and thumbnails use (`WEBDRIVER_*`, plus the `ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS` and `ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT` feature flags). The menu option @@ -94,7 +101,7 @@ will not register. | Key | Default | Description | | ------------------------------- | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `EXCEL_EXPORT_S3_BUCKET` | `None` | Destination bucket. When unset, exports are built during the request and downloaded directly instead of being queued. | -| `EXCEL_EXPORT_SYNC_MAX_ROWS` | `100000` | Largest export served during the request, as the combined `row_limit` of every query it would run. Over this (or with any query lacking a finite limit) the export is refused and asks for a bucket. Ignored once one is configured. | +| `EXCEL_EXPORT_SYNC_MAX_ROWS` | `100000` | Largest data export served during the request, as the combined `row_limit` of every query it would run. Over this (or with any query lacking a finite limit) the export is refused and asks for a bucket. Ignored once one is configured. | | `EXCEL_EXPORT_S3_KEY_PREFIX` | `"dashboard-exports/"` | Key prefix: `{prefix}{dashboard_id}/{job_id}.xlsx`. | | `EXCEL_EXPORT_LINK_TTL_SECONDS` | `86400` | Lifetime of the pre-signed download URL (24h). | | `EXCEL_EXPORT_S3_CLIENT_KWARGS` | `{}` | Extra kwargs for `boto3.client("s3", ...)` — e.g. `region_name`, or `endpoint_url` for MinIO/LocalStack. | @@ -125,5 +132,6 @@ variables, shared config, or instance role) unless overridden via embedded dashboard can still use the export. - The default **Export Data to Excel** mode exports data only (no visual styling). Use **Export Images to Excel** to embed rendered chart images, which - requires the webdriver infrastructure described in the prerequisites. + requires both an export bucket and the webdriver infrastructure described in + the prerequisites — it is never built during the request. - Scheduled/automated exports are not part of this feature. diff --git a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx index 861e588c17c..e1e5ff79083 100644 --- a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx +++ b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadMenuItems.test.tsx @@ -256,6 +256,115 @@ test('Export Data to Excel names the downloaded file from the response', async ( click.mockRestore(); }); +/** A request that stays in flight until the returned resolve is called. */ +const mockPendingResponse = (): { settle: (response: unknown) => void } => { + let settle: (response: unknown) => void = () => {}; + mockSupersetClient.post.mockReturnValue( + new Promise(resolve => { + settle = resolve; + }) as never, + ); + return { settle: response => settle(response) }; +}; + +const menuItemFor = (label: string) => + screen.getByText(label).closest('[role="menuitem"]'); + +test('Export Data to Excel reports progress while the export is running', async () => { + // The request can take a while when the workbook is built inline, and a lock + // stops duplicate work without telling the user anything. + const { settle } = mockPendingResponse(); + + render(<MenuWrapper />, { useRedux: true }); + await userEvent.click(screen.getByText('Export Data to Excel')); + + await waitFor(() => { + expect(screen.getByText('Preparing export…')).toBeInTheDocument(); + }); + expect(screen.queryByText('Export Data to Excel')).not.toBeInTheDocument(); + expect(menuItemFor('Preparing export…')).toHaveAttribute( + 'aria-disabled', + 'true', + ); + + settle({ status: 202, json: jest.fn().mockResolvedValue({ job_id: 'abc' }) }); + + // Once the queued message arrives the action is offered again. + await waitFor(() => { + expect(screen.getByText('Export Data to Excel')).toBeInTheDocument(); + }); + expect(mockAddSuccessToast).toHaveBeenCalledWith( + "Your export is being prepared. You'll receive an email when it's ready.", + ); +}); + +test('Export Data to Excel is offered again once the download starts', async () => { + const { settle } = mockPendingResponse(); + stubObjectUrls(); + + render(<MenuWrapper />, { useRedux: true }); + await userEvent.click(screen.getByText('Export Data to Excel')); + await waitFor(() => { + expect(screen.getByText('Preparing export…')).toBeInTheDocument(); + }); + + settle({ + status: 200, + blob: jest.fn().mockResolvedValue(new Blob(['xlsx'])), + headers: new Headers({ + 'Content-Disposition': 'attachment; filename=dash.xlsx', + }), + }); + + await waitFor(() => { + expect(screen.getByText('Export Data to Excel')).toBeInTheDocument(); + }); + expect(mockAddSuccessToast).toHaveBeenCalledWith( + 'Dashboard data exported to Excel', + ); +}); + +test('Export Data to Excel is offered again after a failure', async () => { + const { settle } = mockPendingResponse(); + mockGetClientErrorObject.mockResolvedValue({ status: 500 }); + + render(<MenuWrapper />, { useRedux: true }); + await userEvent.click(screen.getByText('Export Data to Excel')); + await waitFor(() => { + expect(screen.getByText('Preparing export…')).toBeInTheDocument(); + }); + + // A rejected request must clear the pending state too, or the action would + // stay stuck until the dashboard is reloaded. + settle(Promise.reject(new Error('boom'))); + + await waitFor(() => { + expect(screen.getByText('Export Data to Excel')).toBeInTheDocument(); + }); + expect(mockAddDangerToast).toHaveBeenCalledWith( + 'Sorry, something went wrong. Try again later.', + ); +}); + +test('Export Images to Excel is blocked while a data export is running', async () => { + // One export at a time per dashboard: the server holds a lock, so offering a + // second export would only earn an "already in progress" refusal. + enableWebDriverScreenshot(); + mockPendingResponse(); + + render(<MenuWrapper />, { useRedux: true }); + await userEvent.click(screen.getByText('Export Data to Excel')); + + await waitFor(() => { + expect(menuItemFor('Export Images to Excel')).toHaveAttribute( + 'aria-disabled', + 'true', + ); + }); + await userEvent.click(screen.getByText('Export Images to Excel')); + expect(mockSupersetClient.post).toHaveBeenCalledTimes(1); +}); + test('Export Data to Excel shows an "already in progress" toast when throttled', async () => { // The throttle response is 202 with a message but no job_id. mockQueuedResponse({ diff --git a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx index 8109a5fffc9..555d58b6acb 100644 --- a/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx +++ b/superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { SyntheticEvent } from 'react'; +import { SyntheticEvent, useState } from 'react'; import { useSelector } from 'react-redux'; import { logging } from '@apache-superset/core/utils'; import { t } from '@apache-superset/core/translation'; @@ -72,6 +72,12 @@ export const useDownloadMenuItems = ( const { addDangerToast, addSuccessToast } = useToasts(); const dataMask = useSelector((state: RootState) => state.dataMask); + // The mode whose export is in flight, if any. The server allows one export + // per user and dashboard at a time, so while one runs both actions are + // disabled and the one that was clicked reports its progress. + const [exportingXlsx, setExportingXlsx] = useState<'data' | 'images' | null>( + null, + ); const SCREENSHOT_NODE_SELECTOR = '.dashboard'; const buildActiveDataMask = (): Record<string, { extraFormData: object }> => @@ -177,6 +183,7 @@ export const useDownloadMenuItems = ( }; const onExportXlsx = async (mode: 'data' | 'images') => { + setExportingXlsx(mode); try { const response = await SupersetClient.post({ endpoint: `/api/v1/dashboard/${dashboardId}/export_xlsx/`, @@ -229,6 +236,10 @@ export const useDownloadMenuItems = ( } else { addDangerToast(t('Sorry, something went wrong. Try again later.')); } + } finally { + // However the request ends, the action has to come back: otherwise a + // failure would leave it stuck until the dashboard is reloaded. + setExportingXlsx(null); } }; @@ -276,12 +287,16 @@ export const useDownloadMenuItems = ( }, ]; + const xlsxExportLabel = (mode: 'data' | 'images', text: string) => + exportingXlsx === mode ? t('Preparing export…') : text; + const exportMenuItems: MenuItem[] = [ ...(userCanExport ? [ { key: 'export-xlsx', - label: t('Export Data to Excel'), + label: xlsxExportLabel('data', t('Export Data to Excel')), + disabled: exportingXlsx !== null, onClick: () => onExportXlsx('data'), }, // Image export renders charts through the headless webdriver, so only @@ -292,7 +307,8 @@ export const useDownloadMenuItems = ( ? [ { key: 'export-xlsx-images', - label: t('Export Images to Excel'), + label: xlsxExportLabel('images', t('Export Images to Excel')), + disabled: exportingXlsx !== null, onClick: () => onExportXlsx('images'), }, ] diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 66482c43e44..8d46456b075 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -95,8 +95,16 @@ from superset.commands.purge import PurgeArchivedCommand, SoftDeleteBinding from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.daos.dashboard import DashboardDAO, EmbeddedDashboardDAO from superset.dashboards.excel_export.storage import is_export_storage_configured -from superset.dashboards.excel_export.sync_budget import is_within_sync_row_budget -from superset.dashboards.excel_export.workbook import build_workbook +from superset.dashboards.excel_export.sync_budget import ( + InlineExportPlan, + plan_inline_export, +) +from superset.dashboards.excel_export.workbook import ( + build_workbook, + EXPORT_MODE_DATA, + EXPORT_MODE_IMAGES, + ResolvedQueryContexts, +) from superset.dashboards.filter_scope import derive_json_metadata from superset.dashboards.filters import ( DashboardAccessFilter, @@ -1732,7 +1740,7 @@ class DashboardRestApi( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.export_xlsx", log_to_statsd=False, ) - def export_xlsx(self, pk: int) -> WerkzeugResponse: + def export_xlsx(self, pk: int) -> WerkzeugResponse: # noqa: C901 """Export all of a dashboard's chart data to an Excel workbook. --- post: @@ -1780,6 +1788,10 @@ class DashboardRestApi( 500: $ref: '#/components/responses/500' """ + # C901 above: a linear chain of request guards, each returning its own + # status, ahead of the two export paths. Splitting it would only move the + # guards somewhere less obvious. + # # With storage the export is queued and delivered by link; without it the # workbook is built here and returned as the response. Resolved once, so a # single request cannot take one path's checks and the other's delivery. @@ -1824,17 +1836,32 @@ class DashboardRestApi( mode = payload.get("mode", "data") # An export served as the response has to finish inside this request, so - # refuse an oversized one before doing any of the work rather than letting - # it run into a gateway timeout. Checked ahead of the lock so a refusal + # establish that it can before doing any of the work, rather than letting + # it run into a gateway timeout. Checked ahead of the lock, so a refusal # never leaves a lock to be released. - if not queued and not is_within_sync_row_budget(dashboard, mode): - return self.response_400( - message=( - "This dashboard requests too many rows to export in a single " - "request. Configure EXCEL_EXPORT_S3_BUCKET to export it in the " - "background, or lower the row limits of its charts." + plan: InlineExportPlan | None = None + if not queued: + if mode == EXPORT_MODE_IMAGES: + # Image charts are drawn by the headless webdriver, one browser + # session at a time. That is not work a request can wait on, and + # no row budget bounds it, so it stays queued-only. + return self.response_400( + message=( + "Exporting images to Excel runs in the background. " + "Configure EXCEL_EXPORT_S3_BUCKET to use it, or export " + "the dashboard's data instead." + ) + ) + plan = plan_inline_export(dashboard) + if not plan.fits_row_budget: + return self.response_400( + message=( + "This dashboard requests too many rows to export in a " + "single request. Configure EXCEL_EXPORT_S3_BUCKET to " + "export it in the background, or lower the row limits of " + "its charts." + ) ) - ) # Throttle: one concurrent export per user+dashboard. Acquire a shared, # atomic distributed lock (Redis when configured, the metadata DB @@ -1856,8 +1883,13 @@ class DashboardRestApi( ) job_id = str(uuid.uuid4()) - run_export = self._export_xlsx_queued if queued else self._export_xlsx_inline - return run_export(dashboard, active_data_mask, mode, job_id, lock_params) + if plan is None: + return self._export_xlsx_queued( + dashboard, active_data_mask, mode, job_id, lock_params + ) + return self._export_xlsx_inline( + dashboard, active_data_mask, job_id, lock_params, plan.query_contexts + ) def _export_xlsx_queued( # pylint: disable=too-many-arguments self, @@ -1896,23 +1928,24 @@ class DashboardRestApi( def _export_xlsx_inline( # pylint: disable=too-many-arguments dashboard: Dashboard, active_data_mask: dict[str, Any], - mode: str, job_id: str, lock_params: dict[str, int], + query_contexts: ResolvedQueryContexts, ) -> WerkzeugResponse: """ Build the export during this request and return it as the response. Used where no export storage is configured, so there is nowhere to upload a finished file and nothing to link to in an email. The workbook is the - same one the Celery task builds, from the same builder: only the delivery - differs. It is written to a temp file (the writer streams to disk in - constant memory) and read back once, so the response carries a complete - file and the temp file never outlives the request. - - The charts the export had to skip are not reported here. The queued path - lists them in its email, which this path has no equivalent of; the - workbook itself is identical either way. + same one the Celery task builds, from the same builder — including the + summary sheet naming any charts it had to skip, which is how this path + reports them without an email. It is written to a temp file (the writer + streams to disk in constant memory) and read back once, so the response + carries a complete file and the temp file never outlives the request. + + ``query_contexts`` are the ones the row budget was measured against, so + the queries that run here are exactly the ones that were vouched for. + Always a data export: an image export is refused before this point. """ tmp_path: str | None = None try: @@ -1921,7 +1954,15 @@ class DashboardRestApi( ) os.close(file_descriptor) - build_workbook(tmp_path, dashboard, active_data_mask, job_id, mode, g.user) + build_workbook( + tmp_path, + dashboard, + active_data_mask, + job_id, + EXPORT_MODE_DATA, + g.user, + query_contexts=query_contexts, + ) with open(tmp_path, "rb") as workbook: content = workbook.read() finally: diff --git a/superset/dashboards/excel_export/sync_budget.py b/superset/dashboards/excel_export/sync_budget.py index 34b6f445454..e07408150e9 100644 --- a/superset/dashboards/excel_export/sync_budget.py +++ b/superset/dashboards/excel_export/sync_budget.py @@ -15,33 +15,56 @@ # specific language governing permissions and limitations # under the License. """ -Decide whether a dashboard is small enough to export inline. - -An export served as the HTTP response has to finish inside one request, so the -size of the workbook is settled *before* any query runs, by adding up the rows -the export is allowed to ask for: the ``row_limit`` of every query it would run. -A request/server timeout is the last-resort backstop, not the criterion — a -timed-out export wastes the work already done and tells the user nothing -actionable, whereas an up-front refusal can name the fix. - -The total is deliberately the *requested* row count rather than the delivered -one. It is knowable without touching a database, and it is an upper bound: an -export that clears the budget cannot exceed it once the queries run. +Plan an export that has to be served as the response to one request. + +An inline export has to finish inside its request, so its size is settled +*before* any query runs, by adding up the rows it is allowed to ask for: the +``row_limit`` of every query it would run. A request/server timeout is the +last-resort backstop, not the criterion — a timed-out export wastes the work +already done and tells the user nothing actionable, whereas an up-front refusal +can name the fix. + +Working that total out means resolving each chart's query context, which is what +the export itself runs. The plan therefore hands those contexts back and the +export reuses them, so the queries that run are exactly the ones the budget was +measured against — resolution can be expensive and, through +``EXCEL_EXPORT_QUERY_CONTEXT_BUILDER``, is not guaranteed to be deterministic. """ from __future__ import annotations +from dataclasses import dataclass from typing import Any from flask import current_app from superset.dashboards.excel_export.layout import get_charts_in_layout_order from superset.dashboards.excel_export.workbook import ( - renders_as_image, resolve_query_context, + ResolvedQueryContexts, ) +@dataclass(frozen=True) +class InlineExportPlan: + """What an inline export would run, and whether it is small enough to.""" + + #: Every chart's resolved query context, keyed by chart id. A ``None`` value + #: is an answer, not a gap: that chart cannot be exported and will be listed + #: as skipped. + query_contexts: ResolvedQueryContexts + #: Rows every query is allowed to return, or ``None`` when any query has no + #: finite limit and the size of the export is therefore unknowable. + requested_rows: int | None + #: The configured ceiling this plan was measured against. + max_rows: int + + @property + def fits_row_budget(self) -> bool: + """Whether this export may run inline, as the response to one request.""" + return self.requested_rows is not None and self.requested_rows <= self.max_rows + + def _finite_row_limit(query: Any) -> int | None: """ A query's ``row_limit`` when it bounds the result, else ``None``. @@ -59,28 +82,12 @@ def _finite_row_limit(query: Any) -> int | None: return row_limit if row_limit > 0 else None -def requested_row_total(dashboard: Any, mode: str) -> int | None: - """ - Total rows every query in this export is allowed to return. - - Returns ``None`` when any query the export would run has no finite row limit, - which makes the total — and so the size of the export — indeterminate. - - Charts the export cannot run contribute nothing: one with no usable query - context is skipped by the export itself, and in image mode a non-table chart - is rendered rather than queried. - - Note that this resolves each chart's query context, which the export then - resolves again when it builds the workbook. For a saved context that is a - JSON parse; a deployment using ``EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`` pays - for its hook twice on this path. - """ +def _row_total(query_contexts: ResolvedQueryContexts) -> int | None: + """Rows every resolved query may return, or ``None`` if any is unbounded.""" total = 0 - for chart in get_charts_in_layout_order(dashboard): - if renders_as_image(chart, mode): - continue - query_context = resolve_query_context(chart) + for query_context in query_contexts.values(): if query_context is None: + # Nothing to run: the export skips this chart and lists it instead. continue for query in query_context["queries"]: row_limit = _finite_row_limit(query) @@ -90,9 +97,20 @@ def requested_row_total(dashboard: Any, mode: str) -> int | None: return total -def is_within_sync_row_budget(dashboard: Any, mode: str) -> bool: - """Whether this export may run inline, as the response to one request.""" - total = requested_row_total(dashboard, mode) - return ( - total is not None and total <= current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] +def plan_inline_export(dashboard: Any) -> InlineExportPlan: + """ + Resolve what an inline export of ``dashboard`` would run, and size it. + + Only ever called for a data export: an image export renders charts through + the headless webdriver, which is not work a request can wait on, so it is + refused before it gets here. + """ + query_contexts: ResolvedQueryContexts = { + chart.id: resolve_query_context(chart) + for chart in get_charts_in_layout_order(dashboard) + } + return InlineExportPlan( + query_contexts=query_contexts, + requested_rows=_row_total(query_contexts), + max_rows=current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"], ) diff --git a/superset/dashboards/excel_export/workbook.py b/superset/dashboards/excel_export/workbook.py index 154c1a65b9c..696d8d4d247 100644 --- a/superset/dashboards/excel_export/workbook.py +++ b/superset/dashboards/excel_export/workbook.py @@ -76,6 +76,12 @@ TABLE_VIZ_TYPES = {"table", "pivot_table_v2", "pivot_table"} # saved query context is skipped and listed for the user to re-save in Explore. REBUILD_VIZ_TYPES = {"table", "big_number_total", "big_number", "pie"} +#: Query contexts already resolved for a set of charts, keyed by chart id, as +#: :func:`resolve_query_context` returns them. A ``None`` value is an answer, not +#: a gap: that chart has no usable context and is skipped. A chart absent from +#: the mapping has not been resolved yet. +ResolvedQueryContexts = dict[int, dict[str, Any] | None] + class ChartSkippedError(Exception): """Signals a chart that could not be exported and should be listed as skipped.""" @@ -308,13 +314,14 @@ def _write_chart_sheets( ) -def build_workbook( +def build_workbook( # pylint: disable=too-many-arguments path: str, dashboard: Any, active_data_mask: dict[str, Any], job_id: str, mode: str, user: Any, + query_contexts: ResolvedQueryContexts | None = None, ) -> dict[str, list[str]]: """Build the workbook on disk. @@ -328,8 +335,13 @@ def build_workbook( :param job_id: Correlation id used in log lines :param mode: ``"data"`` or ``"images"`` :param user: The requesting user (used to render images) + :param query_contexts: Contexts a caller has already resolved, used as-is + instead of resolving them again — so an export whose size was measured + up front runs the very queries that were measured. Charts absent from + the mapping are resolved here; pass nothing to resolve them all. """ errored: dict[str, list[str]] = {} + resolved = query_contexts or {} writer = StreamingXlsxWriter(path) try: for chart in get_charts_in_layout_order(dashboard): @@ -342,10 +354,15 @@ def build_workbook( writer, chart, dashboard.id, active_data_mask, user ) else: - # Data charts need a query context: use the saved one, or - # rebuild it from form data for eligible viz types. Skip - # cleanly when none is available rather than failing. - json_body = resolve_query_context(chart) + # Data charts need a query context: use the one the caller + # already resolved, else the saved one or a rebuild from form + # data for eligible viz types. Skip cleanly when none is + # available rather than failing. + json_body = ( + resolved[chart.id] + if chart.id in resolved + else resolve_query_context(chart) + ) if json_body is None: errored.setdefault(email.ERROR_NO_QUERY_CONTEXT, []).append( label @@ -376,12 +393,17 @@ def build_workbook( ) errored.setdefault(email.ERROR_GENERAL, []).append(label) - if writer.sheet_count == 0: + # The workbook itself carries the skipped-charts list, so it travels with + # the file however the file is delivered: an export returned as the + # response to the request has no email to list them in. + if writer.sheet_count == 0 or errored: flat = [label for labels in errored.values() for label in labels] - writer.add_summary_sheet( - "Export Summary", - ["No chart data could be exported.", *flat], + header = ( + "No chart data could be exported." + if writer.sheet_count == 0 + else "Charts that could not be exported:" ) + writer.add_summary_sheet("Export Summary", [header, *flat]) finally: writer.close() return errored diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 1e16a8e6acc..2a49a439d09 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -34,6 +34,7 @@ from sqlalchemy import and_ from superset import db, security_manager # noqa: F401 from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand from superset.daos.dashboard import EmbeddedDashboardDAO +from superset.dashboards.excel_export.sync_budget import InlineExportPlan from superset.exceptions import LockAlreadyHeldException from superset.security.guest_token import GuestTokenResourceType from superset.models.dashboard import Dashboard @@ -3811,12 +3812,15 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa @with_config({"EXCEL_EXPORT_S3_BUCKET": None}) @patch("superset.dashboards.api.AcquireDistributedLock") @patch("superset.dashboards.api.build_workbook") - @patch("superset.dashboards.api.is_within_sync_row_budget", return_value=False) + @patch("superset.dashboards.api.plan_inline_export") def test_export_xlsx_sync_refused_when_over_the_row_budget( - self, mock_budget, mock_build, mock_acquire + self, mock_plan, mock_build, mock_acquire ): """Dashboard API: an export too large to serve inline is refused up front with a message naming the fix, rather than being started and timing out.""" + mock_plan.return_value = InlineExportPlan( + query_contexts={}, requested_rows=250_000, max_rows=100_000 + ) self.login(ADMIN_USERNAME) dashboard = db.session.query(Dashboard).filter_by(slug="world_health").first() @@ -3828,10 +3832,60 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa assert rv.status_code == 400 message = rv.data.decode("utf-8") assert "EXCEL_EXPORT_S3_BUCKET" in message - # The budget is consulted for the dashboard and mode being exported. - mock_budget.assert_called_once() - assert mock_budget.call_args.args[1] == "data" # Refused before any work started, so no lock was taken and no rows read. + mock_plan.assert_called_once() + mock_build.assert_not_called() + mock_acquire.return_value.run.assert_not_called() + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @with_config({"EXCEL_EXPORT_S3_BUCKET": None}) + @patch("superset.dashboards.api.build_workbook") + @patch("superset.dashboards.api.plan_inline_export") + def test_export_xlsx_sync_runs_the_contexts_the_budget_measured( + self, mock_plan, mock_build + ): + """Dashboard API: the export runs the query contexts the row budget was + measured against. Resolving them a second time would risk vouching for one + set of queries and running another, since a deployment's context builder + need not be deterministic.""" + measured = {10: {"queries": [{"row_limit": 5}]}, 20: None} + mock_plan.return_value = InlineExportPlan( + query_contexts=measured, requested_rows=5, max_rows=100_000 + ) + mock_build.side_effect = self._write_stub_workbook + self.login(ADMIN_USERNAME) + dashboard = db.session.query(Dashboard).filter_by(slug="world_health").first() + + rv = self.client.post( + f"api/v1/dashboard/{dashboard.id}/export_xlsx/", + json={"active_data_mask": {}}, + ) + + assert rv.status_code == 200 + assert mock_build.call_args.kwargs["query_contexts"] is measured + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @with_config({"EXCEL_EXPORT_S3_BUCKET": None}) + @with_feature_flags( + ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS=True, + ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT=True, + ) + @patch("superset.dashboards.api.AcquireDistributedLock") + @patch("superset.dashboards.api.build_workbook") + def test_export_xlsx_images_refused_without_storage(self, mock_build, mock_acquire): + """Dashboard API: image export renders every chart through the headless + webdriver, which no row budget bounds and no request should wait on, so it + is refused rather than served inline -- even with the webdriver enabled.""" + self.login(ADMIN_USERNAME) + dashboard = db.session.query(Dashboard).filter_by(slug="world_health").first() + + rv = self.client.post( + f"api/v1/dashboard/{dashboard.id}/export_xlsx/", + json={"active_data_mask": {}, "mode": "images"}, + ) + + assert rv.status_code == 400 + assert "EXCEL_EXPORT_S3_BUCKET" in rv.data.decode("utf-8") mock_build.assert_not_called() mock_acquire.return_value.run.assert_not_called() diff --git a/tests/unit_tests/dashboards/test_excel_export_sync_budget.py b/tests/unit_tests/dashboards/test_excel_export_sync_budget.py index b51855c2704..6127420786d 100644 --- a/tests/unit_tests/dashboards/test_excel_export_sync_budget.py +++ b/tests/unit_tests/dashboards/test_excel_export_sync_budget.py @@ -23,10 +23,7 @@ from unittest import mock import pytest from flask import current_app -from superset.dashboards.excel_export.sync_budget import ( - is_within_sync_row_budget, - requested_row_total, -) +from superset.dashboards.excel_export.sync_budget import plan_inline_export from superset.utils import json MODULE = "superset.dashboards.excel_export.sync_budget" @@ -42,6 +39,14 @@ def _chart(chart_id: int, *queries: dict[str, Any]) -> mock.MagicMock: return chart +def _unexportable_chart(chart_id: int) -> mock.MagicMock: + """A chart the export has to skip: no context, and no rebuilding it.""" + chart = _chart(chart_id) + chart.query_context = None + chart.viz_type = "mixed_timeseries" # outside the rebuild allowlist + return chart + + @pytest.fixture def charts() -> Iterator[mock.MagicMock]: """Patch the layout walk so tests supply the dashboard's charts directly.""" @@ -57,23 +62,23 @@ def restore_config() -> Iterator[None]: current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] = original -def test_row_total_sums_the_row_limit_of_every_chart(charts: mock.MagicMock) -> None: +def test_plan_sums_the_row_limit_of_every_chart(charts: mock.MagicMock) -> None: charts.return_value = [ _chart(10, {"row_limit": 1000}), _chart(20, {"row_limit": 250}), ] - assert requested_row_total(mock.MagicMock(), "data") == 1250 + assert plan_inline_export(mock.MagicMock()).requested_rows == 1250 -def test_row_total_counts_every_query_of_a_multi_query_chart( +def test_plan_counts_every_query_of_a_multi_query_chart( charts: mock.MagicMock, ) -> None: # A mixed-series chart fans out to several queries, each of which becomes its # own sheet and runs its own row_limit worth of rows. charts.return_value = [_chart(10, {"row_limit": 100}, {"row_limit": 400})] - assert requested_row_total(mock.MagicMock(), "data") == 500 + assert plan_inline_export(mock.MagicMock()).requested_rows == 500 @pytest.mark.parametrize( @@ -86,67 +91,78 @@ def test_row_total_counts_every_query_of_a_multi_query_chart( {"row_limit": -5}, ], ) -def test_row_total_is_indeterminate_without_a_finite_row_limit( +def test_plan_row_total_is_indeterminate_without_a_finite_row_limit( charts: mock.MagicMock, query: dict[str, Any] ) -> None: # Without a finite limit on every query the export's size is unknown, so the - # budget cannot vouch for it and the caller must not run it inline. + # plan cannot vouch for it and the caller must not run it inline. charts.return_value = [_chart(10, {"row_limit": 100}), _chart(20, query)] - assert requested_row_total(mock.MagicMock(), "data") is None + plan = plan_inline_export(mock.MagicMock()) + assert plan.requested_rows is None + assert plan.fits_row_budget is False -def test_row_total_ignores_charts_that_cannot_be_exported( - charts: mock.MagicMock, -) -> None: + +def test_plan_ignores_charts_that_cannot_be_exported(charts: mock.MagicMock) -> None: # A chart with no usable query context is skipped by the export itself, so it # runs no query and cannot contribute rows. - skipped = _chart(20) - skipped.query_context = None - skipped.viz_type = "mixed_timeseries" # outside the rebuild allowlist - charts.return_value = [_chart(10, {"row_limit": 100}), skipped] - - assert requested_row_total(mock.MagicMock(), "data") == 100 - + charts.return_value = [_chart(10, {"row_limit": 100}), _unexportable_chart(20)] -def test_row_total_ignores_charts_rendered_as_images(charts: mock.MagicMock) -> None: - # In image mode a non-table chart is rendered through the webdriver instead of - # queried, so its row_limit is not part of the row budget. - rendered = _chart(20, {"row_limit": 999_999}) - rendered.viz_type = "pie" # not a table viz type, so it renders as an image - charts.return_value = [_chart(10, {"row_limit": 100}), rendered] - - assert requested_row_total(mock.MagicMock(), "images") == 100 + assert plan_inline_export(mock.MagicMock()).requested_rows == 100 @pytest.mark.parametrize( - ("row_limit", "within"), + ("row_limit", "fits"), [ (99_999, True), # below the limit (100_000, True), # exactly at the limit (100_001, False), # above the limit ], ) -def test_budget_allows_totals_up_to_and_including_the_limit( - charts: mock.MagicMock, row_limit: int, within: bool +def test_plan_fits_totals_up_to_and_including_the_limit( + charts: mock.MagicMock, row_limit: int, fits: bool ) -> None: charts.return_value = [_chart(10, {"row_limit": row_limit})] - assert is_within_sync_row_budget(mock.MagicMock(), "data") is within - - -def test_budget_refuses_an_indeterminate_total(charts: mock.MagicMock) -> None: - charts.return_value = [_chart(10, {})] + assert plan_inline_export(mock.MagicMock()).fits_row_budget is fits - assert is_within_sync_row_budget(mock.MagicMock(), "data") is False - -def test_budget_honors_the_configured_limit(charts: mock.MagicMock) -> None: +def test_plan_honors_the_configured_limit(charts: mock.MagicMock) -> None: charts.return_value = [_chart(10, {"row_limit": 5_000})] current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] = 1_000 - assert is_within_sync_row_budget(mock.MagicMock(), "data") is False + assert plan_inline_export(mock.MagicMock()).fits_row_budget is False current_app.config["EXCEL_EXPORT_SYNC_MAX_ROWS"] = 10_000 - assert is_within_sync_row_budget(mock.MagicMock(), "data") is True + assert plan_inline_export(mock.MagicMock()).fits_row_budget is True + + +def test_plan_carries_the_resolved_context_of_every_chart( + charts: mock.MagicMock, +) -> None: + # The plan hands back what it resolved so the export runs exactly the queries + # the budget was measured against, rather than resolving a second time. + exportable = _chart(10, {"row_limit": 100}) + charts.return_value = [exportable, _unexportable_chart(20)] + + plan = plan_inline_export(mock.MagicMock()) + + assert plan.query_contexts[10] == {"queries": [{"row_limit": 100}]} + # Present but None: resolved, and resolved to "this chart cannot be exported". + assert 20 in plan.query_contexts + assert plan.query_contexts[20] is None + + +def test_plan_resolves_each_chart_exactly_once(charts: mock.MagicMock) -> None: + # Resolution can be expensive and, through + # EXCEL_EXPORT_QUERY_CONTEXT_BUILDER, need not be deterministic, so the plan + # must not resolve a chart it has already resolved. + charts.return_value = [_chart(10, {"row_limit": 1}), _chart(20, {"row_limit": 2})] + + with mock.patch(f"{MODULE}.resolve_query_context") as resolve: + resolve.return_value = {"queries": [{"row_limit": 1}]} + plan_inline_export(mock.MagicMock()) + + assert resolve.call_count == 2 diff --git a/tests/unit_tests/dashboards/test_excel_export_workbook.py b/tests/unit_tests/dashboards/test_excel_export_workbook.py new file mode 100644 index 00000000000..688d21f2450 --- /dev/null +++ b/tests/unit_tests/dashboards/test_excel_export_workbook.py @@ -0,0 +1,128 @@ +# 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. +"""Tests for passing already-resolved query contexts into the workbook builder. + +The builder's own behavior (sheet naming, skipped charts, filter application) is +covered through the Celery task in +``tests/unit_tests/tasks/test_export_dashboard_excel.py``. +""" + +from __future__ import annotations + +import os +import tempfile +from collections.abc import Iterator +from typing import Any +from unittest import mock + +import pytest + +from superset.dashboards.excel_export.workbook import build_workbook +from superset.utils import json + +MODULE = "superset.dashboards.excel_export.workbook" + + +def _chart(chart_id: int, name: str) -> mock.MagicMock: + chart = mock.MagicMock() + chart.id = chart_id + chart.slice_name = name + chart.viz_type = "table" + chart.query_context = json.dumps({"queries": [{"row_limit": 100}]}) + return chart + + [email protected] +def mocks() -> Iterator[dict[str, Any]]: + """Patch the builder's collaborators; keep the real xlsx writer.""" + with mock.patch.multiple( + MODULE, + get_charts_in_layout_order=mock.DEFAULT, + get_dashboard_filter_context=mock.DEFAULT, + ChartDataQueryContextSchema=mock.DEFAULT, + ChartDataCommand=mock.DEFAULT, + resolve_query_context=mock.DEFAULT, + ) as patched: + patched["get_dashboard_filter_context"].return_value.extra_form_data = {} + patched["ChartDataCommand"].return_value.run.return_value = { + "queries": [{"colnames": ["a"], "data": [{"a": 1}]}] + } + yield patched + + [email protected] +def workbook_path() -> Iterator[str]: + file_descriptor, path = tempfile.mkstemp(suffix=".xlsx") + os.close(file_descriptor) + yield path + if os.path.exists(path): + os.remove(path) + + +def _build(path: str, charts: list[mock.MagicMock], **kwargs: Any) -> Any: + dashboard = mock.MagicMock() + dashboard.id = 1 + return build_workbook( + path, dashboard, {}, "job-1", "data", mock.MagicMock(), **kwargs + ) + + +def test_provided_query_context_is_used_without_resolving_again( + mocks: dict[str, Any], workbook_path: str +) -> None: + # The context the caller measured its row budget against is the one that runs: + # resolving a second time could yield a different query than was vouched for. + chart = _chart(10, "First") + mocks["get_charts_in_layout_order"].return_value = [chart] + provided = {"queries": [{"row_limit": 7, "metrics": ["count"]}]} + + _build(workbook_path, [chart], query_contexts={10: provided}) + + mocks["resolve_query_context"].assert_not_called() + loaded = mocks["ChartDataQueryContextSchema"].return_value.load.call_args.args[0] + assert loaded["queries"] == provided["queries"] + + +def test_a_chart_resolved_to_none_is_skipped_without_resolving_again( + mocks: dict[str, Any], workbook_path: str +) -> None: + # ``None`` in the map is an answer, not a gap: the caller already found this + # chart unexportable, so the builder must not try to resolve it itself. + chart = _chart(20, "Skipped") + mocks["get_charts_in_layout_order"].return_value = [chart] + + errored = _build(workbook_path, [chart], query_contexts={20: None}) + + mocks["resolve_query_context"].assert_not_called() + mocks["ChartDataCommand"].return_value.run.assert_not_called() + assert [label for labels in errored.values() for label in labels] == [ + "20 - Skipped" + ] + + +def test_a_chart_missing_from_the_map_is_resolved_by_the_builder( + mocks: dict[str, Any], workbook_path: str +) -> None: + # The Celery path passes no map at all, and a partial map must not silently + # drop the charts it does not mention. + chart = _chart(30, "Unmapped") + mocks["get_charts_in_layout_order"].return_value = [chart] + mocks["resolve_query_context"].return_value = {"queries": [{"row_limit": 5}]} + + _build(workbook_path, [chart], query_contexts={}) + + mocks["resolve_query_context"].assert_called_once_with(chart) diff --git a/tests/unit_tests/tasks/test_export_dashboard_excel.py b/tests/unit_tests/tasks/test_export_dashboard_excel.py index 042f7f53868..6cc337862d1 100644 --- a/tests/unit_tests/tasks/test_export_dashboard_excel.py +++ b/tests/unit_tests/tasks/test_export_dashboard_excel.py @@ -710,6 +710,34 @@ def test_all_charts_skipped_writes_summary(mocks: dict[str, Any]) -> None: mocks["email"].build_success_email.assert_called_once() +def test_partial_failure_appends_summary_sheet(mocks: dict[str, Any]) -> None: + """When some charts export and others are skipped, the workbook itself lists + the skipped charts: a download served as the response to the request has no + email to list them in.""" + mocks["get_charts_in_layout_order"].return_value = [ + _chart(10, "Good"), + _chart(20, "Bad", has_context=False, viz_type="sunburst"), + ] + mocks["ChartDataCommand"].return_value.run.return_value = { + "queries": [{"colnames": ["a"], "data": [{"a": 1}]}] + } + + uploaded: dict[str, Any] = {} + + def _capture(path: str, bucket: str, key: str) -> None: + uploaded["sheets"] = _read_sheets(path) + + mocks["s3"].upload_file_to_s3.side_effect = _capture + + _run() + + assert "Export Summary" in uploaded["sheets"] + flat = [str(cell) for row in uploaded["sheets"]["Export Summary"] for cell in row] + assert any("20 - Bad" in cell for cell in flat) + # The chart that did export is still there; the summary is additional. + assert "10 - Good" in uploaded["sheets"] + + def test_upload_failure_sends_failure_email_and_cleans_up( mocks: dict[str, Any], ) -> None:
