-
Notifications
You must be signed in to change notification settings - Fork 129
fix(superset): renew guest token on expiry #3816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0de73a9
716680f
8f626f5
fef561f
ca1e1b6
4e13832
33ee87d
0a32a43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||
| import { gql } from '@apollo/client' | ||||||||||||||
| import { gql, useApolloClient } from '@apollo/client' | ||||||||||||||
| import { captureException } from '@sentry/react' | ||||||||||||||
| import { embedDashboard, EmbeddedDashboard } from '@superset-ui/embedded-sdk' | ||||||||||||||
| import { debounce } from 'lodash' | ||||||||||||||
| import { useEffect, useMemo, useRef } from 'react' | ||||||||||||||
|
|
@@ -15,6 +16,7 @@ import { useSupersetDashboardsQuery } from '~/generated/graphql' | |||||||||||||
| import { useInternationalization } from '~/hooks/core/useInternationalization' | ||||||||||||||
| import { useCurrentUser } from '~/hooks/useCurrentUser' | ||||||||||||||
| import '~/main.css' | ||||||||||||||
| import { createFetchSupersetGuestToken } from '~/pages/dashboards/fetchSupersetGuestToken' | ||||||||||||||
| import ErrorImage from '~/public/images/maneki/error.svg' | ||||||||||||||
| import { PageHeader } from '~/styles' | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -42,6 +44,7 @@ export type DashboardProps = { | |||||||||||||
| const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: DashboardProps) => { | ||||||||||||||
| const { translate } = useInternationalization() | ||||||||||||||
| const { currentMembership } = useCurrentUser() | ||||||||||||||
| const client = useApolloClient() | ||||||||||||||
|
|
||||||||||||||
| const dashboardRef = useRef<string>('') | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -63,6 +66,7 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| let embedded: null | EmbeddedDashboard = null | ||||||||||||||
| let disposed = false | ||||||||||||||
|
|
||||||||||||||
| const persistFilters = isFeatureFlagActive(FeatureFlags.SUPERSET_PERSISTENT_FILTERS) | ||||||||||||||
| // Filter persistence key is scoped to the org from the URL slug (resolved | ||||||||||||||
|
|
@@ -87,6 +91,12 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash | |||||||||||||
| }, 500) | ||||||||||||||
| : null | ||||||||||||||
|
|
||||||||||||||
| const fetchGuestToken = createFetchSupersetGuestToken( | ||||||||||||||
| client, | ||||||||||||||
| dashboard.id, | ||||||||||||||
| dashboard.guestToken, | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| const mount = async () => { | ||||||||||||||
| const mountPoint = document.getElementById(mountId) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -107,7 +117,7 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash | |||||||||||||
| id: dashboard.embeddedId, | ||||||||||||||
| supersetDomain: lagoSupersetUrl, | ||||||||||||||
| mountPoint, | ||||||||||||||
| fetchGuestToken: async () => dashboard?.guestToken, | ||||||||||||||
| fetchGuestToken, | ||||||||||||||
| dashboardUiConfig: { | ||||||||||||||
| hideTitle: true, | ||||||||||||||
| emitDataMasks: persistFilters, | ||||||||||||||
|
|
@@ -119,21 +129,38 @@ const Dashboard = ({ contentTitle, dashboardTitle, dashboardTitleTestKey }: Dash | |||||||||||||
| iframeSandboxExtras: ['allow-top-navigation', 'allow-popups-to-escape-sandbox'], | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| // The SDK mounts its iframe before `embedDashboard` resolves, so cleanup that | ||||||||||||||
| // ran while this was in flight had no `embedded` to unmount. | ||||||||||||||
| if (disposed) { | ||||||||||||||
| embedded.unmount() | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intent here is right, but function unmount() {
mountPoint.replaceChildren(); // lib/index.js
}a blind wipe of whatever the mount node currently holds. And Concretely: the effect re-runs while embed #1 is still awaiting its mint ( Note that Worth spelling out what the branch buys in each case, because only one of the three needs it:
(The refresh chain itself is already fully handled by Suggested fix, a generation guard so only the run that still owns the node may clear it: const runIdRef = useRef(0)
useEffect(() => {
if (!dashboard || dashboard?.id === dashboardRef?.current) {
return
}
runIdRef.current += 1
const runId = runIdRef.current
// ...
// `unmount()` is `mountPoint.replaceChildren()` on the shared node, so only
// clear it if no later run has taken it over.
if (disposed) {
if (runIdRef.current === runId) {
embedded.unmount()
}
return
}The increment has to sit after the early return, so case 3 (new run bails on The alternative is giving each run its own child container so the node is never shared, which is immune by construction, but it changes the DOM structure under |
||||||||||||||
|
|
||||||||||||||
| return | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (debouncedSaveFilters) { | ||||||||||||||
| embedded.observeDataMask(debouncedSaveFilters) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| dashboardRef.current = dashboard.id | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| mount() | ||||||||||||||
| // Nothing else observes this promise: uncaught, a bad token from Superset is an | ||||||||||||||
| // unhandled rejection and a silently blank dashboard. | ||||||||||||||
| mount().catch((mountError) => { | ||||||||||||||
| captureException(mountError, { | ||||||||||||||
| tags: { errorType: 'SupersetDashboardMountError', component: 'Dashboard' }, | ||||||||||||||
| extra: { dashboardId: dashboard.id }, | ||||||||||||||
| }) | ||||||||||||||
| }) | ||||||||||||||
|
|
||||||||||||||
| return () => { | ||||||||||||||
| disposed = true | ||||||||||||||
| fetchGuestToken.cancel() | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The SDK does
The orphan race pre-exists ( Suggested fix — let // factory: only the `if (cancelled)` entry guard halts.
// Post-await, return `lastToken` regardless so embedDashboard can finish.
// Dashboard.tsx
let disposed = false
const mount = async () => {
embedded = await embedDashboard({ /* … */ })
if (disposed) {
embedded.unmount()
return
}
// …
}
return () => {
disposed = true
fetchGuestToken.cancel()
embedded?.unmount()
// …
}Note this inverts the |
||||||||||||||
| debouncedSaveFilters?.cancel() | ||||||||||||||
| embedded?.unmount() | ||||||||||||||
|
ancorcruz marked this conversation as resolved.
|
||||||||||||||
| dashboardRef.current = '' | ||||||||||||||
| } | ||||||||||||||
| }, [dashboard, currentMembership?.organization.id, dashboardTitle, mountId]) | ||||||||||||||
| }, [dashboard, currentMembership?.organization.id, client, dashboardTitle, mountId]) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depending on the whole The effect only needs }, [
dashboard?.id,
dashboard?.embeddedId,
currentMembership?.organization.id,
client,
dashboardTitle,
mountId,
])The |
||||||||||||||
|
|
||||||||||||||
| return ( | ||||||||||||||
| <> | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { screen, waitFor } from '@testing-library/react' | |
|
|
||
| import { GENERIC_PLACEHOLDER_TEST_ID } from '~/components/designSystem/GenericPlaceholder' | ||
| import { getItemFromLS, setItemFromLS } from '~/core/utils/localStorage' | ||
| import { SupersetDashboardsDocument } from '~/generated/graphql' | ||
| import { CreateSupersetGuestTokenDocument, SupersetDashboardsDocument } from '~/generated/graphql' | ||
| import { render, TestMocksType } from '~/test-utils' | ||
|
|
||
| import Dashboard, { DASHBOARD_MOUNT_TEST_ID } from '../Dashboard' | ||
|
|
@@ -79,6 +79,14 @@ const dashboardsData = { | |
|
|
||
| const successMock: TestMocksType = [ | ||
| { request: { query: SupersetDashboardsDocument }, result: { data: dashboardsData } }, | ||
| { | ||
| request: { | ||
| query: CreateSupersetGuestTokenDocument, | ||
| variables: { input: { dashboardId: 'dash-1' } }, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion can't fail. The mutation mock returns Return Also missing: a case asserting |
||
| // Must differ from the seed above, or the assertion passes on the fallback too. | ||
| result: { data: { createSupersetGuestToken: { guestToken: 'refreshed-token' } } }, | ||
| }, | ||
| ] | ||
|
|
||
| const errorMock: TestMocksType = [ | ||
|
|
@@ -105,6 +113,26 @@ const renderRevenue = (mocks: TestMocksType = successMock) => | |
| { mocks }, | ||
| ) | ||
|
|
||
| // Whether `promise` has settled once microtasks and 0ms timers have flushed. | ||
| const hasSettled = async (promise: Promise<unknown>): Promise<boolean> => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper is byte-identical to the one in |
||
| let settled = false | ||
|
|
||
| promise.then( | ||
| () => { | ||
| settled = true | ||
| }, | ||
| () => { | ||
| settled = true | ||
| }, | ||
| ) | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| setTimeout(resolve, 0) | ||
| }) | ||
|
|
||
| return settled | ||
| } | ||
|
|
||
| describe('Dashboard', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
|
|
@@ -136,7 +164,7 @@ describe('Dashboard', () => { | |
| expect(config.supersetDomain).toBe('https://localhost:8089') | ||
| expect(config.mountPoint).toBe(document.getElementById('superset-lago-dashboard')) | ||
| expect(config.dashboardUiConfig.hideTitle).toBe(true) | ||
| await expect(config.fetchGuestToken()).resolves.toBe('token-1') | ||
| await expect(config.fetchGuestToken()).resolves.toBe('refreshed-token') | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -228,5 +256,44 @@ describe('Dashboard', () => { | |
|
|
||
| expect(mockUnmount).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| // The SDK's `unmount()` clears the iframe but not its refresh `setTimeout` | ||
| // (`index.js:159-163`), so cleanup has to cancel the fetcher itself. | ||
| it('THEN cancels the guest token fetcher so the refresh chain stops', async () => { | ||
| const { unmount } = renderAnalytics() | ||
|
|
||
| await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1)) | ||
|
|
||
| const { fetchGuestToken } = mockEmbedDashboard.mock.calls[0][0] | ||
|
|
||
| unmount() | ||
|
|
||
| // A cancelled fetcher never settles; the mutation mock would have resolved it. | ||
| expect(await hasSettled(fetchGuestToken())).toBe(false) | ||
| }) | ||
|
|
||
| // The SDK mounts its iframe before `embedDashboard` resolves, so an embed still | ||
| // in flight at cleanup time is orphaned unless it is unmounted once it lands. | ||
| it('THEN tears down an embed that only resolves after cleanup ran', async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test only covers the full-component-unmount path, where React removes the Rough shape, driving the re-run through it('THEN does not clear the mount node when a later run already took it over', async () => {
let resolveFirstEmbed: (value: unknown) => void = () => {}
const firstUnmount = jest.fn()
mockEmbedDashboard
.mockReturnValueOnce(
new Promise((resolve) => {
resolveFirstEmbed = resolve
}),
)
.mockResolvedValue({ unmount: mockUnmount, observeDataMask: mockObserveDataMask })
const { rerender } = renderAnalytics()
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1))
// Effect re-run: the second embed takes over the mount node.
mockCurrentMembership.mockReturnValue({ organization: { id: 'org-2' } })
rerender(/* same element */)
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(2))
resolveFirstEmbed({ unmount: firstUnmount, observeDataMask: jest.fn() })
await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(2))
// The stale embed must not clear the node the live embed now owns.
expect(firstUnmount).not.toHaveBeenCalled()
})Using a distinct |
||
| let resolveEmbed: (value: unknown) => void = () => {} | ||
|
|
||
| mockEmbedDashboard.mockReturnValue( | ||
| new Promise((resolve) => { | ||
| resolveEmbed = resolve | ||
| }), | ||
| ) | ||
|
|
||
| const { unmount } = renderAnalytics() | ||
|
|
||
| await waitFor(() => expect(mockEmbedDashboard).toHaveBeenCalledTimes(1)) | ||
|
|
||
| unmount() | ||
| expect(mockUnmount).not.toHaveBeenCalled() | ||
|
|
||
| resolveEmbed({ unmount: mockUnmount, observeDataMask: mockObserveDataMask }) | ||
|
|
||
| await waitFor(() => expect(mockUnmount).toHaveBeenCalledTimes(1)) | ||
| expect(mockObserveDataMask).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things about the seed token.
First, the description says the
no-cachepolicy keeps the token out of a cache the app persists to IndexedDB. That is true of the mutation, butsupersetDashboardsstill selectsguestTokenandSupersetDashboardhas anid, so the token is normalized andsetupCachePersistor(src/core/apolloClient/cachePersistor.ts:47) writes it to IndexedDB anyway. Theno-cachepolicy is still worth having, but the claim in the description does not hold as written.Second,
initialTokenis currently only a fallback. The SDK doesawait Promise.all([fetchGuestToken(), mountIframe()])before emitting to the iframe, so the very first render now blocks on a network mutation even though a valid token is already in hand.init.ts:38setsTIMEOUT = 300000andtimeoutLinksits afterretryLink, so each link-level attempt gets its own five-minute budget; multiplied by this fetcher's three attempts, a hung backend can leave the analytics page blank for a very long time where the old code rendered instantly.Returning
initialTokendirectly on the first invocation and minting only from the first refresh onward keeps the fix and takes a round trip off first paint.