gabotorresruiz commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r4062846314
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -167,6 +210,101 @@ export const useDownloadMenuItems = (
}
};
+ const triggerExportDownload = (downloadUrl: string) => {
+ // Stream the file straight to disk via a hidden iframe. The endpoint sends
+ // Content-Disposition: attachment, so the browser saves it without
+ // navigating the dashboard away (fatal inside an embedded iframe) and
+ // without buffering the whole workbook in tab memory the way
fetch().blob()
+ // would. The status endpoint already confirmed the link is ready and
+ // backend-matched, so the only failure left is the narrow race where the
+ // object is removed between that check and this click; such an error
+ // response loads invisibly in the iframe and leaves the page untouched.
+ const iframe = document.createElement('iframe');
+ iframe.style.display = 'none';
+ iframe.src = downloadUrl;
+ document.body.appendChild(iframe);
+ // Remove the iframe after the browser has taken over the download; removal
+ // does not cancel a download already handed off to the browser.
+ setTimeout(() => {
+ if (iframe.parentNode) {
+ iframe.parentNode.removeChild(iframe);
+ }
+ }, EXPORT_STATUS_POLL_INTERVAL_MS);
Review Comment:
Fixed in bbc371d08dcf3f4cfecdad0039a1a7d04e7db8b8. You were right, and the
comment I had left there was wrong: removal only fails to cancel once the
response has committed, and nothing guarantees that has happened by the time
the timer fires. Reusing `EXPORT_STATUS_POLL_INTERVAL_MS` as a download handoff
deadline was the mistake, since time to first byte is unbounded.
Each frame is now held until the component unmounts and cleared in the
existing cleanup rather than on a timer. They are appended to `document.body`,
outside the React tree, so React would otherwise leave them behind.
I rebuilt your setup outside Superset to confirm both halves: an attachment
endpoint that stalls 4.5s before sending headers, and a page starting two
downloads, one whose iframe is removed at 3s and one held. Server side, in
Chromium:
```
old_removed_at_3s : CANCELLED: client closed before headers
new_held : DELIVERED: download reached the browser
```
Only the held one produced a download at all, which matches what you saw.
Added two jest tests for the lifetime contract, that the frame survives well
past the poll interval and that unmount clears it. Both fail without the change.
On the browser test with delayed attachment headers: agreed that is the test
that would actually catch a regression here, since jest cannot model the
cancellation. I have not added a Playwright case in this PR. Happy to do that
as a follow-up if you think it earns the runtime.
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -299,6 +346,13 @@ def _write_chart_sheets(
json_body["result_type"] = ChartDataResultType.FULL
json_body.pop("force", None)
+ # Guest authorization links a chart to its dashboard through
+ # ``form_data.dashboardId`` (raise_for_access); saved contexts don't carry
+ # it, so stamp it the way the browser does on interactive requests.
+ form_data = dict(json_body.get("form_data") or {})
+ form_data["dashboardId"] = dashboard_id
+ json_body["form_data"] = form_data
Review Comment:
Good find, fixed in c17e2d3026822c276e4414b52c805376b87a1c36, stamped right
next to the `dashboardId` stamp since it is the same class of gap for the same
reason.
I traced the mechanism to be sure of it. `Slice.form_data` does
`form_data.update({"slice_id": self.id, ...})`, so an interactive request
silently corrects a stale id, while the export takes
`_saved_query_context(chart.query_context)` and replays the stored JSON
verbatim. `query_context_modified` then compares `form_data["slice_id"]`
against `query_context.slice_` and treats the mismatch as a tampered payload.
When no `current_slice` is passed, `QueryContextFactory.create` instead
resolves the slice from that same stale id, which fails the dashboard to chart
linkage check. Both routes end with the copy in the skipped charts summary
while it renders normally in the dashboard.
Driving the real `query_context_modified` with the payload the task builds:
```
BEFORE: slice_id=7 -> modified=True (logs "Guest chart payload rejected
for slice 10")
AFTER: slice_id=10 -> modified=False
```
Added the copied chart case you asked for, alongside the `dashboardId`
regression test: `test_query_context_is_stamped_with_the_current_slice_id`. It
fails without the stamp.
--
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]
