EnxDev commented on code in PR #41133: URL: https://github.com/apache/superset/pull/41133#discussion_r3554763774
########## superset/dashboards/excel_export/screenshot.py: ########## @@ -0,0 +1,98 @@ +# 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. +""" +Render a single dashboard chart to a PNG for the image-mode Excel export. + +This reuses the same headless render path scheduled reports use +(:class:`~superset.utils.screenshots.ChartScreenshot`), but points it at an +Explore URL whose ``form_data`` carries the live dashboard filter state — so an +embedded image reflects the same filters the data path applies, rather than the +chart's default saved state. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from flask import current_app + +from superset.charts.data.dashboard_filter_context import ( + get_dashboard_filter_context, +) +from superset.utils import json +from superset.utils.screenshots import ChartScreenshot +from superset.utils.urls import get_url_path + +logger = logging.getLogger(__name__) + + +def render_chart_image( + chart: Any, + dashboard_id: int, + active_data_mask: dict[str, Any], + user: Any, +) -> bytes | None: + """ + Render ``chart`` (as seen on ``dashboard_id``) to PNG bytes. + + The chart is rendered through Explore in standalone mode with the live + dashboard filter state injected as ``extra_form_data`` — the same object the + data path merges into the query context — so the image and the data stay + consistent. + + :param chart: The ``Slice`` to render + :param dashboard_id: The dashboard the chart is displayed on (for filter scope) + :param active_data_mask: Live dashboard filter state keyed by native filter id + :param user: The requesting user; the render runs with their permissions + :returns: PNG bytes, or ``None`` if the render failed (the caller skips and + notes the chart) + """ + try: + filter_context = get_dashboard_filter_context( + dashboard_id=dashboard_id, + chart_id=chart.id, + active_data_mask=active_data_mask, + ) + + # Start from the chart's saved form data and force the slice id, then + # layer the live filters on top so the render matches the data path. + form_data: dict[str, Any] = json.loads(chart.params or "{}") + form_data["slice_id"] = chart.id + if filter_context.extra_form_data: + form_data["extra_form_data"] = filter_context.extra_form_data + + url = get_url_path( + "ExploreView.root", + form_data=json.dumps(form_data), + ) + + window_size = current_app.config["WEBDRIVER_WINDOW"]["slice"] + screenshot = ChartScreenshot( + url, + chart.digest, + window_size=window_size, + thumb_size=window_size, + ) + return screenshot.get_screenshot(user=user) + except Exception: # pylint: disable=broad-except + logger.exception( + "Failed to render image for chart %s in dashboard %s", + getattr(chart, "id", "?"), + dashboard_id, + ) + return None Review Comment: The broad except Exception also catches `SoftTimeLimitExceeded` (since it subclasses Exception). In image export mode this converts a timeout into None, which is later reported as a generic chart failure instead of aborting the export. This also contributes to the timeout handling issue described above. It would be safer to explicitly except SoftTimeLimitExceeded: raise before the generic handler. A regression test ensuring the exception propagates would be useful. ########## superset/utils/s3.py: ########## @@ -0,0 +1,76 @@ +# 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. +""" +Minimal S3 helpers for uploading export artifacts and minting pre-signed URLs. + +Credentials and region come from the standard boto3 resolution chain (env vars, +shared config, instance role). Operators can override client construction via +the ``EXCEL_EXPORT_S3_CLIENT_KWARGS`` config (e.g. ``region_name`` or an +``endpoint_url`` for S3-compatible stores such as MinIO/LocalStack). +""" + +from __future__ import annotations + +import logging +from typing import Any + +from flask import current_app + +logger = logging.getLogger(__name__) + + +def _get_s3_client() -> Any: + """Build an S3 client using operator-provided client kwargs (if any).""" + # boto3 is imported lazily so that importing this module (which happens at + # app startup via the dashboard API) does not require boto3 to be installed. + # The dependency is only needed when an export actually runs. + import boto3 # pylint: disable=import-outside-toplevel Review Comment: boto3 is currently only declared under [project.optional-dependencies].development in pyproject.toml, while this feature imports it in production code. As a result, production installs without the development dependencies will fail with ModuleNotFoundError, causing dashboard exports to fail and users to receive the generic "could not be completed" email. This also addresses Beto's unresolved review comment. It would be better to move boto3 into the main dependencies (or introduce a dedicated optional extra with clear installation requirements). A regression test importing superset.utils.s3 under a non-development install would help prevent regressions. ########## superset/tasks/export_dashboard_excel.py: ########## @@ -0,0 +1,345 @@ +# 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. +""" +Celery task that exports every chart on a dashboard to a single multi-sheet +``.xlsx`` file, uploads it to S3, and emails the requesting user a pre-signed +download link. + +In ``"data"`` mode the task re-runs each chart's saved query context under the +requesting user, applies the live dashboard filter state, and streams the results +row-by-row into a constant-memory workbook so large dashboards never load all +data at once. In ``"images"`` mode non-table charts are instead rendered to +images (through the same headless path as scheduled reports, reflecting the live +filters) and embedded, while table-like charts stay tabular. +""" + +from __future__ import annotations + +import logging +import os +import tempfile +from datetime import datetime, timedelta, timezone +from typing import Any + +from celery.exceptions import SoftTimeLimitExceeded +from flask import current_app, g + +from superset import db, security_manager +from superset.charts.data.dashboard_filter_context import ( + apply_dashboard_filter_context, + get_dashboard_filter_context, +) +from superset.charts.schemas import ChartDataQueryContextSchema +from superset.commands.chart.data.get_data_command import ChartDataCommand +from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType +from superset.dashboards.excel_export import email +from superset.dashboards.excel_export.layout import get_charts_in_layout_order +from superset.dashboards.excel_export.screenshot import render_chart_image +from superset.extensions import cache_manager, celery_app +from superset.utils import json, s3 +from superset.utils.core import override_user +from superset.utils.excel_streaming import StreamingXlsxWriter + +logger = logging.getLogger(__name__) + +# Export modes: "data" streams every chart's tabular result (the default, +# unchanged behavior); "images" embeds non-table charts as rendered images and +# keeps only table-like charts tabular. +EXPORT_MODE_DATA = "data" +EXPORT_MODE_IMAGES = "images" + +# Viz types kept as tabular data in image mode; everything else is rendered as an +# image. Operators can override the set via ``EXCEL_EXPORT_TABLE_VIZ_TYPES``. +TABLE_VIZ_TYPES = {"table", "pivot_table_v2", "pivot_table"} + +EXPORT_SOFT_TIME_LIMIT = 600 +EXPORT_HARD_TIME_LIMIT = 660 +# TTL for the per-user+dashboard in-flight lock set by the API before enqueue. +# It outlives the hard time limit so a worker killed at that limit (which skips +# the ``finally`` cleanup) cannot hold the lock forever; the delete in +# ``finally`` is the fast path that frees it as soon as the task settles. +EXPORT_INFLIGHT_CACHE_TTL = EXPORT_HARD_TIME_LIMIT + 60 + + +class _ChartSkippedError(Exception): + """Signals a chart that could not be exported and should be listed as skipped.""" + + +def _chart_label(chart: Any) -> str: + """Human-readable label for a chart in the skipped-charts list.""" + return f"{chart.id} - {chart.slice_name or ''}".strip() + + +def _record_to_row(record: dict[str, Any], colnames: list[str]) -> list[Any]: + return [record.get(col) for col in colnames] + + +def _table_viz_types() -> set[str]: + """Viz types kept tabular in image mode (config override or built-in default).""" + return current_app.config.get("EXCEL_EXPORT_TABLE_VIZ_TYPES") or TABLE_VIZ_TYPES + + +def _renders_as_image(chart: Any, mode: str) -> bool: + """Whether this chart is embedded as an image rather than streamed as data.""" + return mode == EXPORT_MODE_IMAGES and chart.viz_type not in _table_viz_types() + + +def _write_chart_image_sheet( + writer: StreamingXlsxWriter, + chart: Any, + dashboard_id: int, + active_data_mask: dict[str, Any], + user: Any, +) -> None: + """ + Render a single chart to an image and embed it as its own sheet. + + :raises _ChartSkippedError: if the chart could not be rendered + """ + image = render_chart_image(chart, dashboard_id, active_data_mask, user) + if image is None: + raise _ChartSkippedError + writer.add_image_sheet(_chart_label(chart), image) + + +def _write_chart_sheets( + writer: StreamingXlsxWriter, + chart: Any, + dashboard_id: int, + active_data_mask: dict[str, Any], +) -> None: + """ + Run a single chart's query and stream its result(s) into the workbook. + + Charts may yield more than one query (e.g. mixed-series charts); each becomes + its own sheet. Raises if the chart cannot be exported, so the caller can skip + it and note it in the email. + """ + json_body = json.loads(chart.query_context) + # Override any stale saved values: we always want full JSON results. + json_body["result_format"] = ChartDataResultFormat.JSON + json_body["result_type"] = ChartDataResultType.FULL + json_body.pop("force", None) + + filter_context = get_dashboard_filter_context( + dashboard_id=dashboard_id, + chart_id=chart.id, + active_data_mask=active_data_mask, + ) + if filter_context.extra_form_data: + apply_dashboard_filter_context(json_body, filter_context.extra_form_data) + + # Jinja macros resolve form data from g.form_data; expose the saved context. + g.form_data = json_body + + query_context = ChartDataQueryContextSchema().load(json_body) + command = ChartDataCommand(query_context) + command.validate() + result = command.run() + + for index, query in enumerate(result["queries"]): + colnames = query.get("colnames") or [] + data = query.get("data") or [] + if index == 0: + name = f"{chart.id} - {chart.slice_name or ''}" + else: + name = f"{chart.id}.{index} - {chart.slice_name or ''}" + writer.add_sheet( + name, + colnames, + (_record_to_row(record, colnames) for record in data), + ) + + +def _build_workbook( + path: str, + dashboard: Any, + active_data_mask: dict[str, Any], + job_id: str, + mode: str, + user: Any, +) -> dict[str, list[str]]: + """Build the workbook on disk. + + Return the charts that could not be exported, grouped by the reason they + were omitted (see the ``email.ERROR_*`` reason keys), so the notification + can explain each group separately. + """ + errored: dict[str, list[str]] = {} + writer = StreamingXlsxWriter(path) + try: + for chart in get_charts_in_layout_order(dashboard): + label = _chart_label(chart) + as_image = _renders_as_image(chart, mode) + # Image charts render from their saved params and don't need a query + # context; data (and table) charts still do. + if not as_image and not chart.query_context: + errored.setdefault(email.ERROR_NO_QUERY_CONTEXT, []).append(label) + continue + try: + if as_image: + _write_chart_image_sheet( + writer, chart, dashboard.id, active_data_mask, user + ) + else: + _write_chart_sheets(writer, chart, dashboard.id, active_data_mask) + except SoftTimeLimitExceeded: + logger.warning( + "Chart %s timed out in dashboard export %s", chart.id, job_id + ) + errored.setdefault(email.ERROR_TIMEOUT, []).append(label) Review Comment: `SoftTimeLimitExceeded ` is raised once by Celery. Catching it per chart and continuing allows the task to keep running until the hard time limit (time_limit=660) terminates the worker. In that case, the cleanup logic never executes, no failure email is sent, temporary files may remain, and the in-flight lock is only released after its TTL expires. The outer handler (`SoftTimeLimitExceeded`) is therefore unlikely to be reached in the normal timeout scenario. Consider re-raising the exception after recording the failed chart, or tracking the remaining execution time and exiting the loop early. A regression test that raises SoftTimeLimitExceeded during the second chart and verifies build_failure_email() is invoked would cover this case. ########## superset/dashboards/api.py: ########## @@ -1501,6 +1514,109 @@ def export_as_example(self, pk: int) -> Response: response.set_cookie(token, "done", max_age=600) return response + @expose("/<pk>/export_xlsx/", methods=("POST",)) + @protect() + @safe + @permission_name("export") + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.export_xlsx", + log_to_statsd=False, + ) + def export_xlsx(self, pk: int) -> WerkzeugResponse: + """Export all of a dashboard's chart data to an Excel workbook (async). + --- + post: + summary: Export dashboard chart data to Excel + description: >- + Enqueues an async task that writes each chart's data to its own + worksheet, uploads the .xlsx to S3, and emails the requesting user a + pre-signed download link. Returns immediately with a job id. + parameters: + - in: path + schema: + type: integer + name: pk + description: The dashboard id + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxPostSchema' + responses: + 202: + description: Export task accepted + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxResponseSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + 501: + description: Excel export is not configured on this server + """ + if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]: + return self.response( + 501, message="Excel export is not configured on this server." + ) + try: + # Tolerate an empty/non-JSON body (e.g. a POST with no Content-Type); + # request.json would otherwise raise 415. + payload = DashboardExportXlsxPostSchema().load( + request.get_json(silent=True) or {} + ) + except ValidationError as error: + return self.response_400(message=error.messages) + + dashboard = cast(Dashboard, self.datamodel.get(pk, self._base_filters)) + if not dashboard: + return self.response_404() + try: + security_manager.raise_for_access(dashboard=dashboard) + except SupersetSecurityException: + return self.response_403() + + # Email delivery is the only result channel, so an account with an email + # address is required; embedded guest users are excluded in this version. + if isinstance(g.user, GuestUser) or not getattr(g.user, "email", None): + return self.response_400( + message="Excel export requires an account with an email address." + ) + if not dashboard.slices: + return self.response_400(message="Dashboard has no charts to export.") + + # Throttle: one concurrent export per user+dashboard. The lock is set + # here and cleared by the task; its TTL guards against a lost cleanup. + inflight_key = f"excel-export-inflight:{g.user.id}:{dashboard.id}" + if cache_manager.cache.get(inflight_key): + return self.response( + 202, + message="An Excel export for this dashboard is already in progress.", + ) Review Comment: The concurrency guard currently relies on `cache_manager.cache`, which uses `CACHE_CONFIG`. Since the default configuration is `NullCache`, the lock is effectively disabled out of the box (get() always returns None, set() is a no-op), allowing unlimited concurrent exports. Additionally, with SimpleCache the lock is process-local, so it doesn't synchronize between the web server and Celery workers. Since this is intended to act as the DDoS protection discussed in previous reviews, it would likely be more robust to use a shared backend (similar to the SupersetMetastoreCache pattern used by FILTER_STATE_CACHE_CONFIG) and perform the lock acquisition atomically rather than via separate get()/set() calls. A regression test using the default CACHE_CONFIG could verify that a second request is rejected. ########## superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx: ########## @@ -198,6 +237,20 @@ export const useDownloadMenuItems = ( ]; const exportMenuItems: MenuItem[] = [ + ...(userCanExport + ? [ + { + key: 'export-xlsx', + label: t('Export Data to Excel'), + onClick: () => onExportXlsx('data'), + }, + { + key: 'export-xlsx-images', + label: t('Export Images to Excel'), + onClick: () => onExportXlsx('images'), Review Comment: "Export Images to Excel" is exposed to every `can_export` user, but the current documentation still states that only data is exported (no chart images), the PR description doesn't introduce an image-export mode, and the feature isn't gated on screenshot/webdriver availability. On installations without that infrastructure, non-table charts fail and users still receive a success email containing only the summary sheet. It may be worth gating this option behind the appropriate configuration or deferring it to a follow-up PR. It would also be good to document `EXCEL_EXPORT_TABLE_VIZ_TYPES` in both the configuration docs and `UPDATING.md.` ########## superset/dashboards/api.py: ########## @@ -1501,6 +1514,109 @@ def export_as_example(self, pk: int) -> Response: response.set_cookie(token, "done", max_age=600) return response + @expose("/<pk>/export_xlsx/", methods=("POST",)) + @protect() + @safe + @permission_name("export") + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.export_xlsx", + log_to_statsd=False, + ) + def export_xlsx(self, pk: int) -> WerkzeugResponse: + """Export all of a dashboard's chart data to an Excel workbook (async). + --- + post: + summary: Export dashboard chart data to Excel + description: >- + Enqueues an async task that writes each chart's data to its own + worksheet, uploads the .xlsx to S3, and emails the requesting user a + pre-signed download link. Returns immediately with a job id. + parameters: + - in: path + schema: + type: integer + name: pk + description: The dashboard id + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxPostSchema' + responses: + 202: + description: Export task accepted + content: + application/json: + schema: + $ref: '#/components/schemas/DashboardExportXlsxResponseSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + 501: + description: Excel export is not configured on this server + """ + if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]: + return self.response( + 501, message="Excel export is not configured on this server." + ) + try: + # Tolerate an empty/non-JSON body (e.g. a POST with no Content-Type); + # request.json would otherwise raise 415. + payload = DashboardExportXlsxPostSchema().load( + request.get_json(silent=True) or {} + ) + except ValidationError as error: + return self.response_400(message=error.messages) + + dashboard = cast(Dashboard, self.datamodel.get(pk, self._base_filters)) + if not dashboard: + return self.response_404() + try: + security_manager.raise_for_access(dashboard=dashboard) + except SupersetSecurityException: + return self.response_403() + + # Email delivery is the only result channel, so an account with an email + # address is required; embedded guest users are excluded in this version. + if isinstance(g.user, GuestUser) or not getattr(g.user, "email", None): + return self.response_400( + message="Excel export requires an account with an email address." + ) + if not dashboard.slices: + return self.response_400(message="Dashboard has no charts to export.") + + # Throttle: one concurrent export per user+dashboard. The lock is set + # here and cleared by the task; its TTL guards against a lost cleanup. + inflight_key = f"excel-export-inflight:{g.user.id}:{dashboard.id}" + if cache_manager.cache.get(inflight_key): + return self.response( + 202, + message="An Excel export for this dashboard is already in progress.", + ) + + job_id = str(uuid.uuid4()) + cache_manager.cache.set(inflight_key, job_id, timeout=EXPORT_INFLIGHT_CACHE_TTL) Review Comment: The in-flight lock is written before `apply_async()`. If the broker is unavailable, `apply_async()` raises while the lock remains in place, preventing further exports until `EXPORT_INFLIGHT_CACHE_TTL` expires. It may be safer to either enqueue the task before acquiring the lock or ensure the lock is cleaned up if task submission fail. -- 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]
