From 9d2dcf219804950f8085bc92ca3c6f5bc90ad32f Mon Sep 17 00:00:00 2001 From: yousoph Date: Wed, 29 Jul 2026 10:20:15 -0700 Subject: [PATCH] fix(oauth2): clear schema/catalog auth banner after OAuth2 redirect refetch (#41913) Co-authored-by: Claude Opus 4.8 --- .../TableExploreTree.test.tsx | 71 +++++- .../TableExploreTree/useTreeData.ts | 99 +++++++- .../DatabaseSelector.test.tsx | 52 ++++ .../src/hooks/apiResources/catalogs.test.ts | 231 ++++++++++++++++++ .../src/hooks/apiResources/catalogs.ts | 66 +++-- .../src/hooks/apiResources/schemas.test.ts | 142 ++++++++++- .../src/hooks/apiResources/schemas.ts | 74 +++--- .../src/hooks/apiResources/tables.test.ts | 61 +++++ .../src/hooks/apiResources/tables.ts | 41 ++-- 9 files changed, 754 insertions(+), 83 deletions(-) create mode 100644 superset-frontend/src/hooks/apiResources/catalogs.test.ts diff --git a/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx b/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx index 585fa09128b..6766c04384d 100644 --- a/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx +++ b/superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx @@ -18,8 +18,16 @@ */ import type { ReactChild } from 'react'; import fetchMock from 'fetch-mock'; -import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import { + act, + createStore, + render, + screen, + waitFor, +} from 'spec/helpers/testing-library'; +import reducerIndex from 'spec/helpers/reducerIndex'; import userEvent from '@testing-library/user-event'; +import { api } from 'src/hooks/apiResources/queryApi'; import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; import { ViewLocations } from 'src/SqlLab/contributions'; @@ -346,3 +354,64 @@ test('closes a schema while searchTerm is active and keeps it closed', async () // The schema node itself remains visible as a matching ancestor (just collapsed) expect(screen.getByText('public')).toBeInTheDocument(); }); + +test('clears the OAuth error banner after a Tables invalidateTags refetch', async () => { + // Regression test for the OAuth2 crud symptom (follow-up to PR #41101). + // Expanding a schema lazily fetches its tables; when that fails with an + // OAuth2 auth error the banner is held in local reducer state and, before + // this fix, only cleared when a table list loaded via manual re-expansion. + // After the OAuth2 redirect, OAuth2RedirectMessage dispatches + // invalidateTags(['Tables']); the errored node's subscribed tables query must + // now refetch automatically and clear the banner. + fetchMock.removeRoutes().clearHistory(); + fetchMock.get('glob:*/api/v1/database/1/schemas/?*', { + count: mockSchemas.length, + result: mockSchemas, + }); + let tablesShouldFail = true; + fetchMock.get('glob:*/api/v1/database/1/tables/*', () => + tablesShouldFail + ? { + status: 500, + body: { + errors: [ + { + error_type: 'GENERIC_DB_ENGINE_ERROR', + level: 'error', + message: 'Tables could not be loaded', + extra: {}, + }, + ], + }, + } + : { count: mockTables.length, result: mockTables }, + ); + + const store = createStore(getInitialState(), reducerIndex); + render(, { + useRedux: true, + store, + }); + + await waitFor(() => { + expect(screen.getByText('public')).toBeInTheDocument(); + }); + + // Expand the schema node → its table fetch rejects → the banner appears. + await userEvent.click(screen.getByText('public')); + expect(await screen.findByText('Unexpected error')).toBeInTheDocument(); + + // The OAuth2 redirect completes: the stored token makes the next fetch + // succeed, and the redirect handler invalidates the Tables cache. + tablesShouldFail = false; + act(() => { + store.dispatch(api.util.invalidateTags(['Tables'])); + }); + + // The subscribed tables query refetches, the tables load, and the banner is + // cleared without the user manually re-expanding the node. + expect(await screen.findByText('users')).toBeInTheDocument(); + await waitFor(() => + expect(screen.queryByText('Unexpected error')).not.toBeInTheDocument(), + ); +}); diff --git a/superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts b/superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts index 6dd01a5279c..c26830b3897 100644 --- a/superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts +++ b/superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useMemo, useReducer, useCallback } from 'react'; +import { useMemo, useReducer, useCallback, useEffect, useRef } from 'react'; import { useAppDispatch } from 'src/SqlLab/hooks/useAppDispatch'; import { t } from '@apache-superset/core/translation'; import { @@ -24,21 +24,33 @@ import { type TableMetaData, useSchemas, useLazyTablesQuery, + useTablesQuery, useLazyTableMetadataQuery, useLazyTableExtendedMetadataQuery, } from 'src/hooks/apiResources'; import { addDangerToast } from 'src/SqlLab/actions/sqlLab'; import type { TreeNodeData } from './types'; -import { SupersetError } from '@superset-ui/core'; +import { ClientErrorObject, SupersetError } from '@superset-ui/core'; export const EMPTY_NODE_ID_PREFIX = 'empty:'; +// Identifies the schema node whose table list failed to load, so the tree can +// automatically recover once the underlying Tables cache is refetched (e.g. +// after the OAuth2 redirect dispatches invalidateTags(['Tables'])). +interface ErroredNode { + schemaKey: string; + dbId: number; + catalog: string | null | undefined; + schema: string; +} + // Reducer state and actions interface TreeDataState { tableData: Record; tableSchemaData: Record; loadingNodes: Record; errorPayload: SupersetError | null; + erroredNode: ErroredNode | null; } type TreeDataAction = @@ -46,13 +58,18 @@ type TreeDataAction = | { type: 'SET_TABLE_SCHEMA_DATA'; key: string; data: TableMetaData } | { type: 'CLEAR_TABLE_SCHEMA_DATA'; key: string } | { type: 'SET_LOADING_NODE'; nodeId: string; loading: boolean } - | { type: 'SET_ERROR'; errorPayload: SupersetError | null }; + | { + type: 'SET_ERROR'; + errorPayload: SupersetError | null; + erroredNode: ErroredNode | null; + }; const initialState: TreeDataState = { tableData: {}, tableSchemaData: {}, loadingNodes: {}, errorPayload: null, + erroredNode: null, }; function treeDataReducer( @@ -64,6 +81,7 @@ function treeDataReducer( return { ...state, errorPayload: null, + erroredNode: null, tableData: { ...state.tableData, [action.key]: action.data }, }; case 'SET_TABLE_SCHEMA_DATA': @@ -90,6 +108,7 @@ function treeDataReducer( return { ...state, errorPayload: action.errorPayload, + erroredNode: action.erroredNode, }; default: @@ -144,7 +163,67 @@ const useTreeData = ({ // Combined state for table data, schema data, loading nodes, and data version const [state, dispatch] = useReducer(treeDataReducer, initialState); - const { tableData, tableSchemaData, loadingNodes, errorPayload } = state; + const { + tableData, + tableSchemaData, + loadingNodes, + errorPayload, + erroredNode, + } = state; + + // Tables are loaded lazily on node toggle, so a schema whose table list fails + // (e.g. an OAuth2 auth error) has no active subscription that would recover on + // cache invalidation. Subscribe to the tables query for the single errored + // node so that when OAuth2RedirectMessage dispatches invalidateTags(['Tables']) + // after the redirect, this query refetches automatically. The subscribed + // entry shares the cache key (dbId + schema) that the lazy fetch already + // populated with the error, so this reflects that error and does not trigger + // an eager refetch of its own. + const erroredTablesResult = useTablesQuery( + { + dbId: erroredNode?.dbId, + catalog: erroredNode?.catalog, + schema: erroredNode?.schema, + forceRefresh: false, + }, + { skip: !erroredNode }, + ); + const wasFetchingErroredRef = useRef(false); + + useEffect(() => { + // Recover the errored schema node when its subscribed tables query finishes + // a fetch (driven by the Tables cache invalidation). Keying off the + // isFetching true->false transition avoids acting on the initial rejected + // state and on unrelated re-renders. On success, SET_TABLE_DATA repopulates + // the node and clears the banner; on renewed failure the banner is re-armed. + if (!erroredNode) { + wasFetchingErroredRef.current = erroredTablesResult.isFetching; + return; + } + const { isSuccess, isError, isFetching, currentData, error } = + erroredTablesResult; + const nodeId = `schema:${erroredNode.dbId}:${erroredNode.schema}`; + if (isFetching && !wasFetchingErroredRef.current) { + dispatch({ type: 'SET_LOADING_NODE', nodeId, loading: true }); + } + if (!isFetching && wasFetchingErroredRef.current) { + if (isSuccess && currentData) { + dispatch({ + type: 'SET_TABLE_DATA', + key: erroredNode.schemaKey, + data: currentData, + }); + } else if (isError) { + dispatch({ + type: 'SET_ERROR', + errorPayload: (error as ClientErrorObject)?.errors?.[0] ?? null, + erroredNode, + }); + } + dispatch({ type: 'SET_LOADING_NODE', nodeId, loading: false }); + } + wasFetchingErroredRef.current = isFetching; + }, [erroredTablesResult, erroredNode]); // Shared helper: fetch table metadata + extended metadata and store in state. // preferCacheValue=true on initial open (use cached data if available), @@ -233,6 +312,12 @@ const useTreeData = ({ dispatch({ type: 'SET_ERROR', errorPayload: error?.errors?.[0] ?? null, + erroredNode: { + schemaKey, + dbId: parsedDbId, + catalog, + schema, + }, }); }) .finally(() => { @@ -296,6 +381,12 @@ const useTreeData = ({ dispatch({ type: 'SET_ERROR', errorPayload: error?.errors?.[0] ?? null, + erroredNode: { + schemaKey, + dbId: refreshDbId, + catalog: refreshCatalog, + schema, + }, }); }) .finally(() => { diff --git a/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx b/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx index a1a4a56b35d..cd8382c323c 100644 --- a/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx +++ b/superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx @@ -367,6 +367,58 @@ test('Sends the correct db when changing the database', async () => { ); }); +test('clears the schema error banner after an invalidateTags-driven refetch', async () => { + // Regression test for the OAuth2 crud symptom (follow-up to PR #41101). + // The schema fetch fails and shows an error banner held in DatabaseSelector's + // local `errorPayload` state. After the OAuth2 redirect completes, + // OAuth2RedirectMessage dispatches invalidateTags, which refetches the + // subscribed schemas query. The banner must disappear once the refetch + // succeeds — previously it lingered because useSchemas only fired onSuccess + // through its lazy trigger, not on the subscribed refetch. + fetchMock.removeRoutes().clearHistory(); + fetchMock.get(databaseApiRoute, fakeDatabaseApiResult, { + name: databaseApiRoute, + }); + fetchMock.get(catalogApiRoute, fakeCatalogApiResult); + fetchMock.get(tablesApiRoute, fakeFunctionNamesApiResult); + let failSchemas = true; + fetchMock.get(schemaApiRoute, () => + failSchemas + ? { + status: 500, + body: { + errors: [ + { + error_type: 'GENERIC_DB_ENGINE_ERROR', + level: 'error', + message: 'Schemas could not be loaded', + extra: {}, + }, + ], + }, + } + : fakeSchemaApiResult, + ); + + const props = createProps(); + render(, { useRedux: true, store }); + + // The error banner appears once the initial schemas fetch fails. + expect(await screen.findByText('Unexpected error')).toBeInTheDocument(); + + // Simulate the OAuth2 redirect completing: the stored token makes the next + // fetch succeed, and the redirect handler invalidates the Schemas tag. + failSchemas = false; + act(() => { + store.dispatch(api.util.invalidateTags([{ type: 'Schemas', id: 'LIST' }])); + }); + + // The subscribed query refetches successfully and the banner is cleared. + await waitFor(() => + expect(screen.queryByText('Unexpected error')).not.toBeInTheDocument(), + ); +}); + test('Sends the correct schema when changing the schema', async () => { const props = createProps(); const { rerender } = render(, { diff --git a/superset-frontend/src/hooks/apiResources/catalogs.test.ts b/superset-frontend/src/hooks/apiResources/catalogs.test.ts new file mode 100644 index 00000000000..e7cbce0a4e8 --- /dev/null +++ b/superset-frontend/src/hooks/apiResources/catalogs.test.ts @@ -0,0 +1,231 @@ +/** + * 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. + */ +import rison from 'rison'; +import fetchMock from 'fetch-mock'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { + createWrapper, + defaultStore as store, +} from 'spec/helpers/testing-library'; +import { api } from 'src/hooks/apiResources/queryApi'; +import { useCatalogs } from './catalogs'; + +const fakeApiResult = { + result: ['test catalog 1', 'test catalog b'], +}; + +const expectedResult = fakeApiResult.result.map((value: string) => ({ + value, + label: value, + title: value, +})); + +// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks +describe('useCatalogs hook', () => { + beforeEach(() => { + fetchMock.clearHistory().removeRoutes(); + store.dispatch(api.util.resetApiState()); + }); + + test('returns api response mapping json result', async () => { + const expectDbId = 'db1'; + const forceRefresh = false; + const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`; + fetchMock.get(catalogApiRoute, fakeApiResult); + const onSuccess = jest.fn(); + const { result } = renderHook( + () => + useCatalogs({ + dbId: expectDbId, + onSuccess, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + await waitFor(() => + expect(fetchMock.callHistory.calls(catalogApiRoute).length).toBe(1), + ); + expect(result.current.data).toEqual(expectedResult); + expect( + fetchMock.callHistory.calls( + `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({ + force: forceRefresh, + })}`, + ).length, + ).toBe(1); + expect(onSuccess).toHaveBeenCalledTimes(1); + act(() => { + result.current.refetch(); + }); + await waitFor(() => + expect(fetchMock.callHistory.calls(catalogApiRoute).length).toBe(2), + ); + expect( + fetchMock.callHistory.calls( + `end:/api/v1/database/${expectDbId}/catalogs/?q=${rison.encode({ + force: true, + })}`, + ).length, + ).toBe(1); + expect(onSuccess).toHaveBeenCalledTimes(2); + expect(result.current.data).toEqual(expectedResult); + }); + + test('fires onSuccess when the subscribed query refetches after invalidateTags', async () => { + // Regression test for the OAuth2 retry-after-redirect path (PR #41101). + // The redirect handler dispatches invalidateTags, which refetches the + // SUBSCRIBED query (not the lazy trigger). onSuccess must still fire so + // consumers holding local state (e.g. an auth error banner) get cleared. + const expectDbId = 'db1'; + const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`; + fetchMock.get(catalogApiRoute, fakeApiResult); + const onSuccess = jest.fn(); + const { result } = renderHook( + () => + useCatalogs({ + dbId: expectDbId, + onSuccess, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + await waitFor(() => + expect(fetchMock.callHistory.calls(catalogApiRoute).length).toBe(1), + ); + expect(result.current.currentData).toEqual(expectedResult); + expect(onSuccess).toHaveBeenCalledTimes(1); + + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Catalogs', id: 'LIST' }]), + ); + }); + + await waitFor(() => + expect(fetchMock.callHistory.calls(catalogApiRoute).length).toBe(2), + ); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + // isRefetched must be false so the selectors don't emit a "List refreshed" + // toast for an automatic refetch the user did not request. + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); + + test('recovers from an error when the subscribed query refetches (OAuth2 retry)', async () => { + const expectDbId = 'db1'; + const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`; + let shouldFail = true; + fetchMock.get(catalogApiRoute, () => + shouldFail ? { status: 500, body: {} } : fakeApiResult, + ); + const onSuccess = jest.fn(); + const onError = jest.fn(); + renderHook( + () => + useCatalogs({ + dbId: expectDbId, + onSuccess, + onError, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + await waitFor(() => expect(onError).toHaveBeenCalledTimes(1)); + expect(onSuccess).not.toHaveBeenCalled(); + + // The OAuth2 redirect completes and the token is stored: the next fetch + // succeeds, and onSuccess must fire to clear the error banner. + shouldFail = false; + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Catalogs', id: 'LIST' }]), + ); + }); + + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); + + test('fires callbacks on invalidation refetch even after a failed force refresh (OAuth2 refresh button)', async () => { + // Reviewer regression: serializeQueryArgs strips forceRefresh, so the + // subscribed query and the lazy force-refresh trigger share one cache entry. + // A failed refresh-button click (forceRefresh:true) leaves the entry's + // originalArgs.forceRefresh sticky-true. The old `!originalArgs.forceRefresh` + // guard then suppressed onSuccess/onError on the later invalidation refetch, + // so the banner stayed stuck. The ref-based flag must fire the callbacks. + const expectDbId = 'db1'; + const catalogApiRoute = `glob:*/api/v1/database/${expectDbId}/catalogs/*`; + let mode: 'ok' | 'fail' = 'ok'; + fetchMock.get(catalogApiRoute, () => + mode === 'fail' ? { status: 500, body: {} } : fakeApiResult, + ); + const onSuccess = jest.fn(); + const onError = jest.fn(); + const { result } = renderHook( + () => + useCatalogs({ + dbId: expectDbId, + onSuccess, + onError, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Initial subscribed load succeeds (not a manual refresh: isRefetched=false). + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + + // User clicks the refresh button (force refresh) and hits the OAuth2 wall. + // This makes the shared entry's originalArgs.forceRefresh sticky-true. + mode = 'fail'; + act(() => { + result.current.refetch(); + }); + await waitFor(() => expect(onError).toHaveBeenCalledTimes(1)); + + // User authorizes: invalidateTags refetches the subscribed query, which now + // succeeds. onSuccess must fire (isRefetched=false: not a manual refresh). + mode = 'ok'; + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Catalogs', id: 'LIST' }]), + ); + }); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); +}); diff --git a/superset-frontend/src/hooks/apiResources/catalogs.ts b/superset-frontend/src/hooks/apiResources/catalogs.ts index 26f56b1dd63..1f118c883b9 100644 --- a/superset-frontend/src/hooks/apiResources/catalogs.ts +++ b/superset-frontend/src/hooks/apiResources/catalogs.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { ClientErrorObject } from '@superset-ui/core'; import useEffectEvent from 'src/hooks/useEffectEvent'; import { api, JsonResponse } from './queryApi'; @@ -70,42 +70,60 @@ export const EMPTY_CATALOGS = [] as CatalogOption[]; export function useCatalogs(options: Params) { const { dbId, onSuccess, onError } = options || {}; - const [trigger] = useLazyCatalogsQuery(); + const wasFetchingRef = useRef(false); + const isRefreshingRef = useRef(false); const result = useCatalogsQuery( { dbId, forceRefresh: false }, { skip: !dbId, }, ); + const [trigger] = useLazyCatalogsQuery(); - useEffect(() => { - if (result.isError) { - onError?.(result.error as ClientErrorObject); - } - }, [result.isError, result.error, onError]); - - const fetchData = useEffectEvent( - (dbId: FetchCatalogsQueryParams['dbId'], forceRefresh = false) => { - if (dbId && (!result.currentData || forceRefresh)) { - trigger({ dbId, forceRefresh }).then(({ isSuccess, isError, data }) => { - if (isSuccess) { - onSuccess?.(data || EMPTY_CATALOGS, forceRefresh); - } - if (isError) { - onError?.(result.error as ClientErrorObject); - } - }); - } + const handleOnSuccess = useEffectEvent( + (data: CatalogOption[], isRefetched: boolean) => { + onSuccess?.(data, isRefetched); }, ); + const handleOnError = useEffectEvent((error: ClientErrorObject) => { + onError?.(error); + }); + const refetch = useCallback(() => { - fetchData(dbId, true); - }, [dbId, fetchData]); + if (dbId) { + // Force a real server refresh. The success/error callbacks are fired by + // the subscribed effect below (the single source of truth), which + // observes the shared cache entry's isFetching transition. isRefreshingRef + // flags that completion as a user-requested refresh (isRefetched=true). + isRefreshingRef.current = true; + trigger({ dbId, forceRefresh: true }); + } + }, [dbId, trigger]); useEffect(() => { - fetchData(dbId, false); - }, [dbId, fetchData]); + // Fire the success/error callbacks whenever the subscribed query finishes a + // fetch, not just when data is loaded through the lazy `trigger` path. This + // covers refetches driven by cache invalidation (e.g. after an OAuth2 + // redirect) so consumers holding local state such as an auth error banner + // are notified. Keying off the isFetching true->false transition avoids + // re-firing on cache-hit re-renders, which would spuriously re-run + // auto-select logic in the selectors. isRefreshingRef (not the sticky + // originalArgs.forceRefresh, which the shared cache entry never resets) + // distinguishes a user-requested refresh from other fetches. + const { isSuccess, isError, isFetching, currentData, error } = result; + if (wasFetchingRef.current && !isFetching) { + const isRefetched = isRefreshingRef.current; + if (isSuccess && currentData) { + handleOnSuccess(currentData, isRefetched); + } + if (isError) { + handleOnError(error as ClientErrorObject); + } + isRefreshingRef.current = false; + } + wasFetchingRef.current = isFetching; + }, [result, handleOnSuccess, handleOnError]); return { ...result, diff --git a/superset-frontend/src/hooks/apiResources/schemas.test.ts b/superset-frontend/src/hooks/apiResources/schemas.test.ts index e09c74fa597..29ff80c2598 100644 --- a/superset-frontend/src/hooks/apiResources/schemas.test.ts +++ b/superset-frontend/src/hooks/apiResources/schemas.test.ts @@ -177,8 +177,10 @@ describe('useSchemas hook', () => { store.dispatch(api.util.invalidateTags(['Schemas'])); }); + // Only the currently subscribed query (expectDbId) is refetched on + // invalidation; the previously visited db2 entry is no longer subscribed. await waitFor(() => - expect(fetchMock.callHistory.calls(schemaApiRoute).length).toBe(4), + expect(fetchMock.callHistory.calls(schemaApiRoute).length).toBe(3), ); expect(fetchMock.callHistory.calls(schemaApiRoute)[2].url).toContain( expectDbId, @@ -188,6 +190,144 @@ describe('useSchemas hook', () => { ); }); + test('fires onSuccess when the subscribed query refetches after invalidateTags', async () => { + // Regression test for the OAuth2 retry-after-redirect path (PR #41101). + // The redirect handler dispatches invalidateTags, which refetches the + // SUBSCRIBED query (not the lazy trigger). onSuccess must still fire so + // consumers holding local state (e.g. an auth error banner) get cleared. + const expectDbId = 'db1'; + const schemaApiRoute = `glob:*/api/v1/database/${expectDbId}/schemas/*`; + fetchMock.get(schemaApiRoute, fakeApiResult); + const onSuccess = jest.fn(); + const { result } = renderHook( + () => + useSchemas({ + dbId: expectDbId, + onSuccess, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + await waitFor(() => + expect(fetchMock.callHistory.calls(schemaApiRoute).length).toBe(1), + ); + expect(result.current.currentData).toEqual(expectedResult); + expect(onSuccess).toHaveBeenCalledTimes(1); + + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Schemas', id: 'LIST' }]), + ); + }); + + await waitFor(() => + expect(fetchMock.callHistory.calls(schemaApiRoute).length).toBe(2), + ); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + // isRefetched must be false so the selectors don't emit a "List refreshed" + // toast for an automatic refetch the user did not request. + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); + + test('recovers from an error when the subscribed query refetches (OAuth2 retry)', async () => { + const expectDbId = 'db1'; + const schemaApiRoute = `glob:*/api/v1/database/${expectDbId}/schemas/*`; + let shouldFail = true; + fetchMock.get(schemaApiRoute, () => + shouldFail ? { status: 500, body: {} } : fakeApiResult, + ); + const onSuccess = jest.fn(); + const onError = jest.fn(); + renderHook( + () => + useSchemas({ + dbId: expectDbId, + onSuccess, + onError, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + await waitFor(() => expect(onError).toHaveBeenCalledTimes(1)); + expect(onSuccess).not.toHaveBeenCalled(); + + // The OAuth2 redirect completes and the token is stored: the next fetch + // succeeds, and onSuccess must fire to clear the error banner. + shouldFail = false; + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Schemas', id: 'LIST' }]), + ); + }); + + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); + + test('fires callbacks on invalidation refetch even after a failed force refresh (OAuth2 refresh button)', async () => { + // Reviewer regression: serializeQueryArgs strips forceRefresh, so the + // subscribed query and the lazy force-refresh trigger share one cache entry. + // A failed refresh-button click (forceRefresh:true) leaves the entry's + // originalArgs.forceRefresh sticky-true. The old `!originalArgs.forceRefresh` + // guard then suppressed onSuccess/onError on the later invalidation refetch, + // so the banner stayed stuck. The ref-based flag must fire the callbacks. + const expectDbId = 'db1'; + const schemaApiRoute = `glob:*/api/v1/database/${expectDbId}/schemas/*`; + let mode: 'ok' | 'fail' = 'ok'; + fetchMock.get(schemaApiRoute, () => + mode === 'fail' ? { status: 500, body: {} } : fakeApiResult, + ); + const onSuccess = jest.fn(); + const onError = jest.fn(); + const { result } = renderHook( + () => + useSchemas({ + dbId: expectDbId, + onSuccess, + onError, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Initial subscribed load succeeds (not a manual refresh: isRefetched=false). + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + + // User clicks the refresh button (force refresh) and hits the OAuth2 wall. + // This makes the shared entry's originalArgs.forceRefresh sticky-true. + mode = 'fail'; + act(() => { + result.current.refetch(); + }); + await waitFor(() => expect(onError).toHaveBeenCalledTimes(1)); + + // User authorizes: invalidateTags refetches the subscribed query, which now + // succeeds. onSuccess must fire (isRefetched=false: not a manual refresh). + mode = 'ok'; + act(() => { + store.dispatch( + api.util.invalidateTags([{ type: 'Schemas', id: 'LIST' }]), + ); + }); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedResult, false); + }); + test('returns correct schema list by a catalog', async () => { const dbId = '1'; const expectCatalog = 'catalog3'; diff --git a/superset-frontend/src/hooks/apiResources/schemas.ts b/superset-frontend/src/hooks/apiResources/schemas.ts index 4439b894cfa..912023dda69 100644 --- a/superset-frontend/src/hooks/apiResources/schemas.ts +++ b/superset-frontend/src/hooks/apiResources/schemas.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { ClientErrorObject } from '@superset-ui/core'; import useEffectEvent from 'src/hooks/useEffectEvent'; import { api, JsonResponse } from './queryApi'; @@ -74,48 +74,60 @@ export const EMPTY_SCHEMAS = [] as SchemaOption[]; export function useSchemas(options: Params) { const { dbId, catalog, onSuccess, onError } = options || {}; - const [trigger] = useLazySchemasQuery(); + const wasFetchingRef = useRef(false); + const isRefreshingRef = useRef(false); const result = useSchemasQuery( { dbId, catalog: catalog || undefined, forceRefresh: false }, { skip: !dbId, }, ); + const [trigger] = useLazySchemasQuery(); - useEffect(() => { - if (result.isError) { - onError?.(result.error as ClientErrorObject); - } - }, [result.isError, result.error, onError]); - - const fetchData = useEffectEvent( - ( - dbId: FetchSchemasQueryParams['dbId'], - catalog: FetchSchemasQueryParams['catalog'], - forceRefresh = false, - ) => { - if (dbId && (!result.currentData || forceRefresh)) { - trigger({ dbId, catalog, forceRefresh }).then( - ({ isSuccess, isError, data }) => { - if (isSuccess) { - onSuccess?.(data || EMPTY_SCHEMAS, forceRefresh); - } - if (isError) { - onError?.(result.error as ClientErrorObject); - } - }, - ); - } + const handleOnSuccess = useEffectEvent( + (data: SchemaOption[], isRefetched: boolean) => { + onSuccess?.(data, isRefetched); }, ); - useEffect(() => { - fetchData(dbId, catalog, false); - }, [dbId, catalog, fetchData]); + const handleOnError = useEffectEvent((error: ClientErrorObject) => { + onError?.(error); + }); const refetch = useCallback(() => { - fetchData(dbId, catalog, true); - }, [dbId, catalog, fetchData]); + if (dbId) { + // Force a real server refresh. The success/error callbacks are fired by + // the subscribed effect below (the single source of truth), which + // observes the shared cache entry's isFetching transition. isRefreshingRef + // flags that completion as a user-requested refresh (isRefetched=true). + isRefreshingRef.current = true; + trigger({ dbId, catalog, forceRefresh: true }); + } + }, [dbId, catalog, trigger]); + + useEffect(() => { + // Fire the success/error callbacks whenever the subscribed query finishes a + // fetch, not just when data is loaded through the lazy `trigger` path. This + // covers refetches driven by cache invalidation (e.g. after an OAuth2 + // redirect) so consumers holding local state such as an auth error banner + // are notified. Keying off the isFetching true->false transition avoids + // re-firing on cache-hit re-renders, which would spuriously re-run + // auto-select logic in the selectors. isRefreshingRef (not the sticky + // originalArgs.forceRefresh, which the shared cache entry never resets) + // distinguishes a user-requested refresh from other fetches. + const { isSuccess, isError, isFetching, currentData, error } = result; + if (wasFetchingRef.current && !isFetching) { + const isRefetched = isRefreshingRef.current; + if (isSuccess && currentData) { + handleOnSuccess(currentData, isRefetched); + } + if (isError) { + handleOnError(error as ClientErrorObject); + } + isRefreshingRef.current = false; + } + wasFetchingRef.current = isFetching; + }, [result, handleOnSuccess, handleOnError]); return { ...result, diff --git a/superset-frontend/src/hooks/apiResources/tables.test.ts b/superset-frontend/src/hooks/apiResources/tables.test.ts index 4e7abc4fd79..55fb45d3ccc 100644 --- a/superset-frontend/src/hooks/apiResources/tables.test.ts +++ b/superset-frontend/src/hooks/apiResources/tables.test.ts @@ -334,4 +334,65 @@ describe('useTables hook', () => { await waitFor(() => expect(result.current.data).toEqual(expectedData)); expect(fetchMock.callHistory.calls(tableApiRoute).length).toBe(4); }); + + test('fires callbacks on invalidation refetch even after a failed force refresh (OAuth2 refresh button)', async () => { + // Reviewer regression: serializeQueryArgs strips forceRefresh, so the + // subscribed query and the lazy force-refresh trigger share one cache entry. + // A failed refresh-button click (forceRefresh:true) leaves the entry's + // originalArgs.forceRefresh sticky-true. The old `!originalArgs.forceRefresh` + // guard then suppressed onSuccess/onError on the later invalidation refetch, + // so TableSelector's banner stayed stuck. The ref-based flag must fire them. + const expectDbId = 'db1'; + const expectedSchema = 'schema1'; + const tableApiRoute = `glob:*/api/v1/database/${expectDbId}/tables/?q=*`; + let mode: 'ok' | 'fail' = 'ok'; + fetchMock.get(tableApiRoute, () => + mode === 'fail' ? { status: 500, body: {} } : fakeApiResult, + ); + fetchMock.get(`glob:*/api/v1/database/${expectDbId}/catalogs/*`, { + count: 0, + result: [], + }); + fetchMock.get(`glob:*/api/v1/database/${expectDbId}/schemas/*`, { + result: fakeSchemaApiResult, + }); + const onSuccess = jest.fn(); + const onError = jest.fn(); + const { result } = renderHook( + () => + useTables({ + dbId: expectDbId, + schema: expectedSchema, + onSuccess, + onError, + }), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Initial subscribed load succeeds (not a manual refresh: isRefetched=false). + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedData, false); + + // User clicks the refresh button (force refresh) and hits the OAuth2 wall. + // This makes the shared entry's originalArgs.forceRefresh sticky-true. + mode = 'fail'; + act(() => { + result.current.refetch(); + }); + await waitFor(() => expect(onError).toHaveBeenCalledTimes(1)); + + // User authorizes: invalidateTags refetches the subscribed query, which now + // succeeds. onSuccess must fire (isRefetched=false: not a manual refresh). + mode = 'ok'; + act(() => { + store.dispatch(api.util.invalidateTags(['Tables'])); + }); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + expect(onSuccess).toHaveBeenLastCalledWith(expectedData, false); + }); }); diff --git a/superset-frontend/src/hooks/apiResources/tables.ts b/superset-frontend/src/hooks/apiResources/tables.ts index e8b59326604..c3c07db9218 100644 --- a/superset-frontend/src/hooks/apiResources/tables.ts +++ b/superset-frontend/src/hooks/apiResources/tables.ts @@ -177,6 +177,7 @@ export function useTables(options: Params) { onError, } = options || {}; const isMountedRef = useRef(false); + const isRefreshingRef = useRef(false); const { currentData: schemaOptions, isFetching } = useSchemas({ dbId, catalog: catalog || undefined, @@ -208,37 +209,33 @@ export function useTables(options: Params) { const refetch = useCallback(() => { if (enabled) { - trigger({ dbId, catalog, schema, forceRefresh: true }).then( - ({ isSuccess, isError, data, error }) => { - if (isSuccess && data) { - handleOnSuccess(data, true); - } - if (isError) { - handleOnError(error as ClientErrorObject); - } - }, - ); + // Force a real server refresh. The success/error callbacks are fired by + // the subscribed effect below (the single source of truth), which observes + // the shared cache entry's completion. isRefreshingRef flags that + // completion as a user-requested refresh (isRefetched=true). + isRefreshingRef.current = true; + trigger({ dbId, catalog, schema, forceRefresh: true }); } - }, [dbId, catalog, schema, enabled, handleOnSuccess, handleOnError, trigger]); + }, [dbId, catalog, schema, enabled, trigger]); useEffect(() => { if (isMountedRef.current) { - const { - requestId, - isSuccess, - isError, - isFetching, - currentData, - error, - originalArgs, - } = result; - if (!originalArgs?.forceRefresh && requestId && !isFetching) { + const { requestId, isSuccess, isError, isFetching, currentData, error } = + result; + // Fire once per completed fetch. isRefreshingRef (not the sticky + // originalArgs.forceRefresh, which the shared cache entry never resets) + // distinguishes a user-requested refresh from other fetches, so a failed + // force refresh no longer suppresses the callback on a later + // invalidation-driven refetch (e.g. after an OAuth2 redirect). + if (requestId && !isFetching) { + const isRefetched = isRefreshingRef.current; if (isSuccess && currentData) { - handleOnSuccess(currentData, false); + handleOnSuccess(currentData, isRefetched); } if (isError) { handleOnError(error as ClientErrorObject); } + isRefreshingRef.current = false; } } else { isMountedRef.current = true;