EnxDev commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r4094870127
##########
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:
Done in 8081c98. The direct download now goes through
`after_this_request(_never_cache)`, so both downloads send `no-store`,
`private` and `Pragma: no-cache`, and
`test_export_xlsx_200_streams_workbook_without_storage` asserts `no-store`. I
kept `conditional=False` so a stale `If-None-Match` still can't turn into a 304.
##########
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:
Added in 8081c98. When only one of the two keys is set,
`is_export_storage_configured()` logs a warning naming the missing one. It also
runs on every page load through the bootstrap payload, so the warning goes
through a `functools.cache`d helper and fires once per process per missing key
rather than on every request. `test_excel_export_storage.py` covers both halves
with caplog, and checks that the default and complete configs stay quiet.
UPDATING mentions it next to the upgrade note.
##########
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:
You read it right, and a success toast on a file with no data is misleading.
8081c98 returns a 400 when no chart has a query to run and at least one was
left out as unbounded: "None of this dashboard's charts can be downloaded
directly. Ask an administrator to enable background exports."
I limited it to that case on purpose. If every chart is missing a saved
query context, the queued path would produce the same summary-only workbook, so
sending the user to an admin wouldn't help, and the summary sheet's "re-save in
Explore" note is the useful answer there. The rule is
`InlineExportPlan.needs_background_export`, with unit cases for each mix of
reasons and an integration test for the 400. The docs and UPDATING describe it
too.
--
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]
