sadpandajoe commented on code in PR #43340:
URL: https://github.com/apache/superset/pull/43340#discussion_r3878094994
##########
superset/config.py:
##########
@@ -1522,22 +1523,47 @@ def sync_theme_logo_href(
# note: index option should not be overridden
EXCEL_EXPORT: dict[str, Any] = {}
+
# ---------------------------------------------------
-# Dashboard "Export Data to Excel" (async, S3-backed)
+# Dashboard "Export Data to Excel" (async, object-storage-backed)
# ---------------------------------------------------
-# Destination S3 bucket for generated dashboard .xlsx exports. The feature is
-# disabled until this is set: the export endpoint returns 501 when it is None.
-EXCEL_EXPORT_S3_BUCKET: str | None = None
-# Key prefix for export objects: {prefix}{dashboard_id}/{job_id}.xlsx
-EXCEL_EXPORT_S3_KEY_PREFIX = "dashboard-exports/"
-# Lifetime (seconds) of the pre-signed download URL emailed to the user (24h).
-# Note: AWS S3 caps pre-signed URL lifetime at 7 days (604800 seconds); larger
-# values are rejected by S3, so keep this at or below that when using AWS.
+class ExportStorageConfig(TypedDict, total=False):
+ """Where generated export artifacts (dashboard Excel exports, and
+ potentially other export file types) are uploaded, and how the download
+ redirect resolves them back to a fresh URL. See EXPORT_STORAGE."""
+
+ # Destination bucket for generated export artifacts. The export feature is
+ # disabled until this is set: the export endpoint returns 501 while absent.
+ bucket: str
+ # Key/blob prefix for export objects: {prefix}{dashboard_id}/{job_id}.xlsx
+ # A callable is invoked per export, for deployments where the prefix is
+ # only known in request/task context (e.g. a multi-tenant installation
+ # scoping a shared bucket per tenant).
+ key_prefix: str | Callable[[], str]
+ # The storage backend (an instance implementing
+ # superset.utils.export_storage.ExportStorage), the same pattern as
+ # RESULTS_BACKEND or CUSTOM_SECURITY_MANAGER. There is no implicit
+ # default; the feature is disabled (the export endpoint returns 501)
+ # until one is set explicitly, matching the bucket's provider:
+ # from superset.utils.s3 import S3ExportStorage # AWS S3
+ # from superset.utils.gcs import GCSExportStorage # Google Cloud
Storage
+ # EXPORT_STORAGE["backend"] = S3ExportStorage()
+ # S3ExportStorage accepts client_kwargs for boto3.client("s3", ...)
+ # overrides (region_name, or an endpoint_url for S3-compatible stores
+ # such as MinIO/LocalStack); credentials otherwise resolve through each
+ # SDK's standard chain.
+ backend: ExportStorage
+
+
+EXPORT_STORAGE: ExportStorageConfig = {
Review Comment:
This replaces the supported `EXCEL_EXPORT_S3_*` configuration with an empty
`EXPORT_STORAGE` without a compatibility path. An existing S3 deployment that
upgrades unchanged will now receive `501` for every export. Could the legacy
settings be preserved as a deprecated fallback or migrated explicitly?
##########
superset/dashboards/api.py:
##########
@@ -1922,17 +1946,32 @@ def download_xlsx(self, job_id: uuid.UUID) ->
WerkzeugResponse:
description: The job_id from the export_xlsx response
responses:
302:
- description: Redirect to a pre-signed S3 download URL
+ description: Redirect to a signed storage download URL
410:
description: The link is unknown, expired, or the export failed
+ 501:
+ description: Excel export is not configured on this server
"""
resolved = resolve_download_link(job_id)
if resolved is None:
return self.response(410, message="This download link has
expired.")
- bucket, key = resolved
- return redirect(
- s3.generate_presigned_url(bucket, key, PRESIGNED_URL_TTL_SECONDS)
+ bucket, key, uploaded_backend = resolved
+ storage_backend = current_app.config["EXPORT_STORAGE"].get("backend")
+ if storage_backend is None:
+ # A link can only exist if a backend was configured when the export
+ # ran, so reaching this means the config was cleared since then.
+ return self.response(
+ 501, message="Excel export is not configured on this server."
+ )
+ backend_cls = type(storage_backend)
+ configured_backend =
f"{backend_cls.__module__}.{backend_cls.__qualname__}"
+ if uploaded_backend is not None and uploaded_backend !=
configured_backend:
Review Comment:
This only compares the backend class, not the backend configuration. If an
export is uploaded through `S3ExportStorage(endpoint_url=old-minio)` and the
web tier switches to another `S3ExportStorage` endpoint, the check passes and
the redirect signs the stored key against the wrong service. Could the stored
link bind enough backend identity to reject this same-class migration too?
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -190,46 +208,53 @@ export const useDownloadMenuItems = (
download_url: downloadUrl,
message,
} = json as ExportStatusResponse;
- if (status === "ready") {
+ if (status === 'ready') {
if (downloadUrl) {
- window.location.href = downloadUrl;
+ redirect(downloadUrl);
Review Comment:
The no-email flow now depends on this URL, but the status endpoint builds it
from `WEBDRIVER_BASEURL_USER_FRIENDLY`, whose default is
`http://0.0.0.0:8080/`. In a normal deployment that has not configured the
webdriver/email base URL, a guest export completes but redirects the browser to
an unreachable address with no email fallback. Could the polling response use a
same-origin download path or make this configuration an explicit prerequisite?
##########
superset/dashboards/api.py:
##########
@@ -1832,10 +1848,15 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
- "user_id": g.user.id,
+ "user_id": user_id,
"active_data_mask": payload.get("active_data_mask", {}),
"job_id": job_id,
"mode": payload.get("mode", "data"),
+ "guest_token": (
Review Comment:
During a web-before-worker rollout, older workers still registered without
`guest_token` reject this message before the task can record failure or release
the lock. That leaves the export pending until its TTL expires. Could this be
made rollout-compatible (for example by staging the task signature change or
versioning the task)?
--
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]
