gabotorresruiz commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r4094594136
##########
superset/dashboards/api.py:
##########
@@ -1886,64 +1898,204 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
return self.response_403()
# A requester with no email on file (e.g. an embedded/guest session)
- # still gets a usable export: they poll export_xlsx_status/<job_id>/
- # for the download link instead of relying on an email notification.
+ # still gets a usable export: a queued export is polled at
+ # export_xlsx_status/<job_id>/, and a direct download needs no email.
if not dashboard.slices:
- return self.response_400(message="Dashboard has no charts to
export.")
+ return self.response_400(
+ message=gettext("Dashboard has no charts to export.")
+ )
- # Throttle: one concurrent export per user+dashboard. Acquire a shared,
- # atomic distributed lock (Redis when configured, the metadata DB
- # otherwise) so the guard works across the web server and workers and
is
- # not a no-op under the default cache. The task releases it when it
- # settles; the TTL is the backstop if that release is ever lost.
+ active_data_mask = payload.get("active_data_mask", {})
+ mode = payload.get("mode", "data")
+
+ if not queued and mode == EXPORT_MODE_IMAGES:
+ # Webdriver rendering is too slow and unbounded for a web request.
+ return self.response_400(
+ message=gettext(
+ "Exporting images to Excel requires background exports. "
+ "Ask an administrator to enable them, or export the "
+ "dashboard's data instead."
+ )
+ )
+
+ # Allow one export per user and dashboard across web and worker
processes.
+ # The TTL releases the lock if normal cleanup fails.
# A guest/embedded requester has no DB-backed user id (GuestUser
carries
- # no ``id`` attribute at all), so all guests share lock slot 0 for the
- # dashboard; the task reconstructs the guest (with the token's RLS
rules
- # and resource claims) from the token payload passed alongside.
+ # no ``id`` attribute at all), so guests get a stable slot derived from
+ # their token; the task reconstructs the guest (with the token's RLS
+ # rules and resource claims) from the token payload passed alongside.
user_id = get_user_id()
guest_token_payload = (
getattr(g.user, "guest_token", None) if user_id is None else None
)
lock_params = export_lock_params(
user_id or guest_lock_slot(guest_token_payload), dashboard.id
)
- acquire = AcquireDistributedLock(
+ acquire_lock = AcquireDistributedLock(
EXPORT_LOCK_NAMESPACE,
lock_params,
ttl_seconds=EXPORT_LOCK_TTL_SECONDS,
)
try:
- acquire.run()
+ acquire_lock.run()
except LockAlreadyHeldException:
return self.response(
202,
message="An Excel export for this dashboard is already in
progress.",
)
+ # Every release is checked against this acquisition's token, so an
export
+ # that outlives the TTL cannot delete the lock of whoever acquired
next.
+ lock_token = acquire_lock.token
job_id = str(uuid.uuid4())
+ if queued:
+ return self._export_xlsx_queued(
+ dashboard,
+ user_id,
+ guest_token_payload,
+ active_data_mask,
+ mode,
+ job_id,
+ lock_params,
+ lock_token,
+ )
+
+ # 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=gettext(
+ "This dashboard has too much data to download
directly. "
+ "Ask an administrator to enable background exports, or
"
+ "lower the row limits of its charts."
+ )
+ )
+ lock_delegated = True
+ return self._export_xlsx_inline(
+ dashboard,
+ active_data_mask,
+ job_id,
+ lock_params,
+ lock_token,
+ plan,
+ )
+ finally:
+ if not lock_delegated:
+ try:
+ ReleaseDistributedLock(
+ EXPORT_LOCK_NAMESPACE, lock_params, token=lock_token
+ ).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,
+ user_id: int | None,
+ guest_token: GuestToken | None,
+ active_data_mask: dict[str, Any],
+ mode: str,
+ job_id: str,
+ lock_params: dict[str, int],
+ lock_token: str,
+ ) -> WerkzeugResponse:
+ """Queue an export for upload and delivery by email or status
polling."""
try:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
"user_id": 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"),
- "guest_token": guest_token_payload,
- "lock_token": acquire.token,
+ "mode": mode,
+ "guest_token": guest_token,
+ "lock_token": lock_token,
},
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, token=acquire.token
+ EXPORT_LOCK_NAMESPACE, lock_params, token=lock_token
).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],
+ lock_token: str,
+ plan: InlineExportPlan,
+ ) -> 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=plan.query_contexts,
+ skipped_charts=plan.skipped,
+ )
+ # A dashboard may be untitled; fall back the same way the task
does.
+ filename = get_filename(
+ dashboard.dashboard_title or f"Dashboard {dashboard.id}",
+ dashboard.id,
+ skip_id=False,
+ )
+ response = send_file(
Review Comment:
Just a small NIT, not a blocker. This response ships with `send_file`'s
defaults, which I measured as `Cache-Control: no-cache, max-age=0` plus an
`ETag`, while the streamed download at `export_xlsx/download/<uuid>/` goes
through `_never_cache` (`no-store`, `private`, `Pragma: no-cache`). Same
data-bearing file, two policies. It is a POST response so browsers will not
cache it regardless, so this is symmetry rather than a bug, but
`after_this_request(_never_cache)` before returning here would keep the two
aligned, and `test_export_xlsx_200_streams_workbook_without_storage` could pin
`"no-store" in rv.headers["Cache-Control"]`.
##########
superset/dashboards/excel_export/storage.py:
##########
@@ -0,0 +1,33 @@
+# 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.
+"""Check whether dashboard Excel export storage is configured."""
+
+from __future__ import annotations
+
+from flask import current_app
+
+
+def is_export_storage_configured() -> bool:
+ """Return whether exports can be uploaded and shared by link.
+
+ Both a bucket and a backend are required; the task cannot upload without
+ either, so a partial ``EXPORT_STORAGE`` falls back to direct downloads.
+ """
+ storage_config = current_app.config["EXPORT_STORAGE"]
+ return bool(storage_config.get("bucket")) and (
Review Comment:
Not a blocker, cheap insurance. A bucket without a `backend` (or the
reverse) now quietly selects the direct download, where master returned a loud
`501`. `test_export_xlsx_bucket_without_backend_downloads_directly` pins that
and UPDATING documents it, so I am fine with the choice; but the upgrade
scenario UPDATING describes (bucket ported, `backend` forgotten) would only
surface as image exports disappearing and no links arriving. A `logger.warning`
when exactly one of the two keys is set would make that one line in the logs
instead of a support ticket, with a caplog assertion in
`test_excel_export_storage.py`.
##########
superset/dashboards/api.py:
##########
@@ -1886,64 +1898,204 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
return self.response_403()
# A requester with no email on file (e.g. an embedded/guest session)
- # still gets a usable export: they poll export_xlsx_status/<job_id>/
- # for the download link instead of relying on an email notification.
+ # still gets a usable export: a queued export is polled at
+ # export_xlsx_status/<job_id>/, and a direct download needs no email.
if not dashboard.slices:
- return self.response_400(message="Dashboard has no charts to
export.")
+ return self.response_400(
+ message=gettext("Dashboard has no charts to export.")
+ )
- # Throttle: one concurrent export per user+dashboard. Acquire a shared,
- # atomic distributed lock (Redis when configured, the metadata DB
- # otherwise) so the guard works across the web server and workers and
is
- # not a no-op under the default cache. The task releases it when it
- # settles; the TTL is the backstop if that release is ever lost.
+ active_data_mask = payload.get("active_data_mask", {})
+ mode = payload.get("mode", "data")
+
+ if not queued and mode == EXPORT_MODE_IMAGES:
+ # Webdriver rendering is too slow and unbounded for a web request.
+ return self.response_400(
+ message=gettext(
+ "Exporting images to Excel requires background exports. "
+ "Ask an administrator to enable them, or export the "
+ "dashboard's data instead."
+ )
+ )
+
+ # Allow one export per user and dashboard across web and worker
processes.
+ # The TTL releases the lock if normal cleanup fails.
# A guest/embedded requester has no DB-backed user id (GuestUser
carries
- # no ``id`` attribute at all), so all guests share lock slot 0 for the
- # dashboard; the task reconstructs the guest (with the token's RLS
rules
- # and resource claims) from the token payload passed alongside.
+ # no ``id`` attribute at all), so guests get a stable slot derived from
+ # their token; the task reconstructs the guest (with the token's RLS
+ # rules and resource claims) from the token payload passed alongside.
user_id = get_user_id()
guest_token_payload = (
getattr(g.user, "guest_token", None) if user_id is None else None
)
lock_params = export_lock_params(
user_id or guest_lock_slot(guest_token_payload), dashboard.id
)
- acquire = AcquireDistributedLock(
+ acquire_lock = AcquireDistributedLock(
EXPORT_LOCK_NAMESPACE,
lock_params,
ttl_seconds=EXPORT_LOCK_TTL_SECONDS,
)
try:
- acquire.run()
+ acquire_lock.run()
except LockAlreadyHeldException:
return self.response(
202,
message="An Excel export for this dashboard is already in
progress.",
)
+ # Every release is checked against this acquisition's token, so an
export
+ # that outlives the TTL cannot delete the lock of whoever acquired
next.
+ lock_token = acquire_lock.token
job_id = str(uuid.uuid4())
+ if queued:
+ return self._export_xlsx_queued(
+ dashboard,
+ user_id,
+ guest_token_payload,
+ active_data_mask,
+ mode,
+ job_id,
+ lock_params,
+ lock_token,
+ )
+
+ # 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:
Review Comment:
Question, not a blocker. When every chart ends up in `plan.skipped` (a
dashboard of pivot tables with AVG metrics all go to `ERROR_UNBOUNDED`),
`requested_rows` stays `0`, this check passes, `build_workbook` writes only the
"No chart data could be exported." summary sheet, and the UI shows "Dashboard
data exported to Excel". The queued path behaves the same way with its
summary-only workbook, so this may be intentional. Would a `400` with the
unbounded message be friendlier when `plan.query_contexts` has no runnable
chart, or am I misunderstanding the intent here?
--
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]
