fix(embedded): add guest token to streaming exports#40712
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Scans 1–19 (Python): N/A — TypeScript-only PR. TypeScript checks clean. Security checks clean (backend reads req.form.get("guest_token") at security/manager.py:3464 — established mechanism; raise_for_access() applies same RLS/permission checks as all guest requests; expired token yields 401 before stream opens). CI: sharded-jest, lint-frontend, pre-commit, CodeQL all pass. DB test failures are infra-class ("Set up job" step failed); Playwright failures are infra-class ("Build embedded SDK" step failed before any tests ran). Both unrelated to this frontend-only change.
| expect(request.body.get('form_data')).toBe( | ||
| JSON.stringify({ datasource: '1__table', viz_type: 'table' }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
NIT: The new test covers the token-present path, but no test (new or existing) explicitly asserts that guest_token is absent from the form body when getGuestToken() returns undefined. beforeEach resets the mock to undefined so all prior tests implicitly run without a token, but none assert the absence. A companion test or a single added assertion (e.g. expect(request.body.get('guest_token')).toBeNull() in an existing chart-export test) would guard against accidental token injection in a future refactor and make the boundary explicit.
|
The reviewer's suggestion to explicitly assert the absence of |
richardfogaca
force-pushed
the
rch-118-csv-export-with-row-limit-issue
branch
from
4244b4a to
3a6fc56
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40712 +/- ##
=======================================
Coverage 64.07% 64.07%
=======================================
Files 2664 2664
Lines 143831 143834 +3
Branches 33084 33085 +1
=======================================
+ Hits 92160 92168 +8
+ Misses 50062 50059 -3
+ Partials 1609 1607 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #3c0f3bActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Resolved conflicts: - UPDATING.md: kept both — PR #39925 subdirectory-deployment block and master's Intl.DurationFormat block are independent "Next" entries. - superset/translations/messages.pot + 26 .po files: took master's baseline (--theirs) and re-extracted via babel_update.sh so PR-introduced strings are folded into master's updated catalog. Inbound master highlights: - Intl.DurationFormat replaces pretty-ms (#39330) - Streaming-export guest-token plumbing (#40712) - ChartRenderer/Chart/DrillByChart converted to function components - Routine dep bumps (react-map-gl, @ant-design/icons, dayjs, etc.) Rebuilt superset-ui-core .d.ts via tsc -b so the new createDurationFormatter locale option is visible to pre-commit type-checking. Verified Slice 8 navigateTo/navigateWithState edits in dashboardState.ts and SupersetClient routing edits in useStreamingExport.ts survived auto-merge.
SUMMARY
This fixes embedded dashboard streaming CSV exports for table-like charts whose row limit enters the large-export path.
Previously, the streaming export hook built a native
fetchform request with chart export fields but did not include the active embedded guest token. Embedded users could load the dashboard, but the CSV export POST reached the chart data API without guest-auth material and failed before returning the CSV.The change adds the configured guest token to the streaming export form body when one is present. Backend guest authentication remains the authority; the frontend only transports the existing token on the native streaming request path. The regression test covers the token-bearing case while existing hook coverage keeps the no-token and adjacent streaming behaviors intact.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - no public screenshot or recording is attached to this PR. The verified behavior is described in the testing instructions below.
TESTING INSTRUCTIONS
row_limit=100000, and observed the CSV Export modal reach the successful state with a Download button. Regression condition: the previous export failure/401 state did not recur./api/v1/chart/datafrom that embedded export scenario and observedguest_tokenandexpected_rows=100000in the submitted form, plus a200 OKtext/csvresponse with CSV rows. Regression condition: the request no longer omitted embedded auth material or returned the prior unauthorized export failure.npm run test -- src/components/StreamingExportModal/useStreamingExport.test.tsADDITIONAL INFORMATION
Reviewer focus: this is intentionally limited to the streaming export request builder and its tests; it should not change backend authorization rules, export thresholds, or provider-specific SQL execution.