EnxDev commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r4079900111
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +447,121 @@ def _build_workbook(
)
errored.setdefault(email.ERROR_GENERAL, []).append(label)
- if writer.sheet_count == 0:
+ # The workbook itself carries the skipped-charts list: sessions with
+ # no email (guests, Public role) have no other way to learn that part
+ # of the requested workbook was omitted.
+ if writer.sheet_count == 0 or errored:
flat = [label for labels in errored.values() for label in labels]
- writer.add_summary_sheet(
- "Export Summary",
- ["No chart data could be exported.", *flat],
+ header = (
+ "No chart data could be exported."
+ if writer.sheet_count == 0
+ else "Charts that could not be exported:"
)
+ writer.add_summary_sheet("Export Summary", [header, *flat])
Review Comment:
`flat` collapses every reason group into one list, so this sheet gives bare
chart names, while the email renders a per-reason note through
`_errored_section` ("open each chart in Explore and re-save", etc).
Guest and Public sessions are exactly the ones who only ever see this sheet,
so they get the least actionable version. Worth walking `errored.items()` and
keeping each group's heading?
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -466,16 +629,22 @@ def export_dashboard_excel(
tmp_path, dashboard, active_data_mask, job_id, mode, user
)
- bucket = current_app.config["EXCEL_EXPORT_S3_BUCKET"]
- key = (
- f"{current_app.config['EXCEL_EXPORT_S3_KEY_PREFIX']}"
- f"{dashboard_id}/{job_id}.xlsx"
+ storage_backend, bucket, key =
_resolve_export_storage(dashboard_id, job_id)
+ storage_backend.upload_file(tmp_path, bucket, key)
+ # Naive local time to match KeyValueEntry.is_expired()'s naive
+ # datetime.now() comparison. Guests hold the link only for the
+ # polling window (no email fallback to revisit later), so their
+ # links need not outlive the short credential that authorized them.
+ link_ttl = min(ttl, GUEST_LINK_TTL_SECONDS) if guest_token else ttl
Review Comment:
An anonymous (Public-role) requester carries no `guest_token`, so they get
the full 24 hour link, even though the reasoning in the comment above applies
to them just as well: no email to revisit a link from, retrieval happens inside
the polling window.
Could the clamp key off `user_id is None` rather than `guest_token`?
##########
superset/dashboards/api.py:
##########
@@ -1623,13 +1663,20 @@ def export(self, **kwargs: Any) -> Response:
$ref: '#/components/responses/400'
401:
$ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
404:
$ref: '#/components/responses/404'
422:
$ref: '#/components/responses/422'
500:
$ref: '#/components/responses/500'
"""
+ # A bundle carries dataset SQL and database metadata the embedded view
+ # never exposes, and no guest flow consumes this endpoint.
+ if security_manager.is_guest_user():
Review Comment:
`is_guest_user()` is false for an anonymous session, so this covers embedded
guests but not the Public role.
Enabling anonymous Excel export means granting `can_export on Dashboard` to
Public, and FAB's `is_item_public` short-circuits `@protect()` before any auth
check, so `/api/v1/dashboard/export/` and `export_as_example` then answer fully
unauthenticated requests with the dataset SQL and database metadata this guard
exists to withhold. Could the predicate be `get_user_id() is None` instead, the
same one you already use for `mode=images`?
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +447,121 @@ def _build_workbook(
)
errored.setdefault(email.ERROR_GENERAL, []).append(label)
- if writer.sheet_count == 0:
+ # The workbook itself carries the skipped-charts list: sessions with
+ # no email (guests, Public role) have no other way to learn that part
+ # of the requested workbook was omitted.
+ if writer.sheet_count == 0 or errored:
flat = [label for labels in errored.values() for label in labels]
- writer.add_summary_sheet(
- "Export Summary",
- ["No chart data could be exported.", *flat],
+ header = (
+ "No chart data could be exported."
+ if writer.sheet_count == 0
+ else "Charts that could not be exported:"
)
+ writer.add_summary_sheet("Export Summary", [header, *flat])
finally:
writer.close()
return errored
-def _send_failure_email(
- user: Any, dashboard_title: str, requested_at: datetime
+_GENERIC_FAILURE_MESSAGE = (
+ "An error occurred while generating the file. Please try again, or "
+ "contact your administrator if the problem persists."
+)
+
+
+def _handle_export_failure(
+ user: Any, dashboard_title: str, requested_at: datetime, job_id: str, ttl:
int
) -> None:
- if not (user and getattr(user, "email", None)):
- return
+ """Notify the requester their export failed: email them if they have an
+ address on file, and record a pollable failure status either way (a
+ session with no email, e.g. an embedded/guest dashboard, has no other way
+ to learn the export failed than polling ``export_xlsx/status/<job_id>/``).
+ """
+ # Status first: on a soft timeout only 60s remain before the hard kill,
+ # and a slow SMTP send must not cost pollers the failure status.
try:
- email.send_export_email(
- user.email,
- email.build_subject(dashboard_title, success=False),
- email.build_failure_email(dashboard_title, requested_at),
+ # Naive local time: KeyValueEntry.is_expired() compares against naive
+ # datetime.now(), like every other writer into this store.
+ mark_export_failed(
Review Comment:
This write is unconditional and `_sweep_and_upsert` replaces the whole
value, so a `SoftTimeLimitExceeded` landing after `create_download_link` turns
a ready record into an error one and takes the bucket/key with it. The file
uploaded fine, but the poller is told it failed and the emailed link 410s from
then on.
The window is narrow, but the loss is permanent and there is no second link
to fall back on. Could this skip the write when `get_export_status(job_id)`
already reports ready?
--
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]
