EnxDev commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r3986719735
##########
docs/docs/using-superset/exporting-dashboard-data.mdx:
##########
@@ -88,18 +106,19 @@ variables, shared config, or instance role) unless
overridden via
## Security considerations
- The emailed link is a **pre-signed S3 URL**: anyone who holds it can download
- the workbook until it expires. Keep the bucket **private**, enable
- encryption, and consider a lifecycle rule to delete objects after a few days.
+ the workbook until it expires. Direct downloads are not stored or linked.
+ Keep the bucket **private**, enable encryption, and consider a lifecycle rule
+ to delete objects after a few days.
Lower `EXCEL_EXPORT_LINK_TTL_SECONDS` if 24 hours is too long for your data.
- The export runs with the requesting user's permissions; each chart's query is
access-checked, so users only ever receive data they are entitled to.
## Limitations
- **Embedded dashboards / guest tokens are not supported** in this version,
- because guest users have no email address to deliver the link to. Logged-in
- users viewing an embedded dashboard can still use the export.
+ including direct downloads. Logged-in users viewing an embedded dashboard can
Review Comment:
Agreed, this requirement should be explicit. I kept the existing behavior
consistent across both delivery paths and updated the limitations section to
say that a signed-in, non-guest account with an email address is required,
including for direct downloads. Fixed in `81c73c6e77`.
##########
superset/views/base.py:
##########
@@ -556,6 +556,9 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
# should not expose API TOKEN to frontend
frontend_config = {k: _get_frontend_config_value(k) for k in
FRONTEND_CONF_KEYS}
+ frontend_config["EXCEL_EXPORT_STORAGE_CONFIGURED"] = bool(
Review Comment:
Good call. I switched the bootstrap flag to `is_export_storage_configured()`
so the UI and endpoint use the same capability check. I also changed the test
to call the uncached function and mock the helper directly, avoiding cache-key
coupling. Fixed in `81c73c6e77`.
##########
superset/dashboards/excel_export/sync_budget.py:
##########
@@ -0,0 +1,85 @@
+# 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.
+"""Plan and size dashboard Excel exports served in the HTTP response."""
+
+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 (
+ resolve_query_context,
+ ResolvedQueryContexts,
+)
+
+
+@dataclass(frozen=True)
+class InlineExportPlan:
+ """Queries planned for a direct download and their row budget."""
+
+ #: Resolved query contexts by chart id. ``None`` marks a skipped chart.
+ query_contexts: ResolvedQueryContexts
+ #: Combined row limit, or ``None`` when any query has no finite limit.
+ requested_rows: int | None
+ #: Configured limit for direct downloads.
+ max_rows: int
+
+ @property
+ def fits_row_budget(self) -> bool:
+ """Return whether the export can run during the request."""
+ return self.requested_rows is not None and self.requested_rows <=
self.max_rows
+
+
+def _finite_row_limit(query: Any) -> int | None:
+ """Return the limit; missing, null, and zero use ``ROW_LIMIT``."""
+ if not isinstance(query, dict):
+ return None
+ row_limit = query.get("row_limit") or current_app.config["ROW_LIMIT"]
Review Comment:
Good point. Charging the full `ROW_LIMIT` for an ungrouped metric query made
small KPI dashboards fail the default budget too quickly. The planner now
counts a non-timeseries metric query with no grouping columns as one row.
Timeseries queries still use their normal row limit, and both cases have tests.
Fixed in `c8001036a1`.
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -244,23 +255,25 @@ 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')),
Review Comment:
Thanks, you’re right about the dropdown lifecycle. I added a persistent
progress toast outside the menu and remove it as soon as the request settles.
The menu state still prevents duplicate exports, and there is now a
Header-level test covering the closed-dropdown case. Fixed in `81c73c6e77`.
##########
superset/dashboards/excel_export/sync_budget.py:
##########
@@ -0,0 +1,85 @@
+# 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.
+"""Plan and size dashboard Excel exports served in the HTTP response."""
+
+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 (
+ resolve_query_context,
+ ResolvedQueryContexts,
+)
+
+
+@dataclass(frozen=True)
+class InlineExportPlan:
+ """Queries planned for a direct download and their row budget."""
+
+ #: Resolved query contexts by chart id. ``None`` marks a skipped chart.
+ query_contexts: ResolvedQueryContexts
+ #: Combined row limit, or ``None`` when any query has no finite limit.
+ requested_rows: int | None
+ #: Configured limit for direct downloads.
+ max_rows: int
+
+ @property
+ def fits_row_budget(self) -> bool:
+ """Return whether the export can run during the request."""
+ return self.requested_rows is not None and self.requested_rows <=
self.max_rows
+
+
+def _finite_row_limit(query: Any) -> int | None:
+ """Return the limit; missing, null, and zero use ``ROW_LIMIT``."""
+ if not isinstance(query, dict):
+ return None
+ row_limit = query.get("row_limit") or current_app.config["ROW_LIMIT"]
+ if isinstance(row_limit, bool) or not isinstance(row_limit, int):
+ return None
+ return row_limit if row_limit > 0 else None
+
+
+def _row_total(query_contexts: ResolvedQueryContexts) -> int | None:
+ """Rows every resolved query may return, or ``None`` if any is
unbounded."""
+ total = 0
+ for query_context in query_contexts.values():
+ if query_context is None:
+ # Skipped charts do not add to the row budget.
+ continue
+ for query in query_context["queries"]:
+ row_limit = _finite_row_limit(query)
Review Comment:
You are right—the declared `row_limit` is not a safe bound once grouping
sets are involved. The native path also skips that limit, while fallback
execution can fan out across levels. The planner now treats any non-empty
`grouping_sets` context as unbounded and requires the background path. I added
regression coverage for this case. Fixed in `c8001036a1`.
##########
superset/dashboards/api.py:
##########
@@ -1825,25 +1848,131 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
)
job_id = str(uuid.uuid4())
+ if queued:
+ return self._export_xlsx_queued(
+ dashboard, active_data_mask, mode, job_id, lock_params
+ )
+
+ # Plan after locking because query-context resolution can be expensive.
+ # Release here unless the inline exporter takes over cleanup.
+ lock_delegated = False
+ try:
+ plan: InlineExportPlan = 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."
+ )
+ )
+ lock_delegated = True
+ return self._export_xlsx_inline(
+ dashboard,
+ active_data_mask,
+ job_id,
+ lock_params,
+ plan.query_contexts,
+ )
+ finally:
+ if not lock_delegated:
+ try:
+ ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE,
lock_params).run()
+ except Exception: # pylint: disable=broad-except
+ # The TTL is the fallback if release fails.
+ logger.exception(
+ "Failed to release in-flight export lock for dashboard
%s",
+ dashboard.id,
+ )
+
+ def _export_xlsx_queued( # pylint: disable=too-many-arguments
+ self,
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ mode: str,
+ job_id: str,
+ lock_params: dict[str, int],
+ ) -> WerkzeugResponse:
+ """Queue an export for upload and email delivery."""
try:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
"user_id": g.user.id,
- "active_data_mask": payload.get("active_data_mask", {}),
+ "active_data_mask": active_data_mask,
"job_id": job_id,
- "mode": payload.get("mode", "data"),
+ "mode": mode,
},
task_id=job_id,
)
except Exception:
- # If enqueuing fails (e.g. broker down) the task will never run to
- # release the lock, so free it now rather than block exports until
- # the TTL expires.
+ # No task will release the lock if enqueueing fails.
ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE, lock_params).run()
raise
return self.response(202, job_id=job_id)
+ @staticmethod
+ def _export_xlsx_inline( # pylint: disable=too-many-arguments
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ job_id: str,
+ lock_params: dict[str, int],
+ query_contexts: ResolvedQueryContexts,
+ ) -> WerkzeugResponse:
+ """Build a planned data export and return it in the response."""
+ tmp_path: str | None = None
+ try:
+ file_descriptor, tmp_path = tempfile.mkstemp(
+ suffix=".xlsx", prefix=f"dash-export-{job_id}-"
+ )
+ os.close(file_descriptor)
+
+ build_workbook(
+ tmp_path,
+ dashboard,
+ active_data_mask,
+ job_id,
+ EXPORT_MODE_DATA,
+ g.user,
+ query_contexts=query_contexts,
+ )
+ filename = get_filename(
+ dashboard.dashboard_title, dashboard.id, skip_id=False
Review Comment:
Confirmed — `dashboard_title` is nullable and `get_filename` sanitizes with
a regex that rejects `None`, so the workbook was built and only then did the
request 500. The inline path now uses the queued path's `Dashboard {id}`
fallback, with an integration test asserting an untitled dashboard downloads
under that name. Fixed in 8db370e6b5.
--
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]
