This is an automated email from the ASF dual-hosted git repository.
yousoph pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 9d2dcf21980 fix(oauth2): clear schema/catalog auth banner after OAuth2
redirect refetch (#41913)
9d2dcf21980 is described below
commit 9d2dcf219804950f8085bc92ca3c6f5bc90ad32f
Author: yousoph <[email protected]>
AuthorDate: Wed Jul 29 10:20:15 2026 -0700
fix(oauth2): clear schema/catalog auth banner after OAuth2 redirect refetch
(#41913)
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
.../TableExploreTree/TableExploreTree.test.tsx | 71 ++++++-
.../components/TableExploreTree/useTreeData.ts | 99 ++++++++-
.../DatabaseSelector/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 ++++++
superset-frontend/src/hooks/apiResources/tables.ts | 41 ++--
9 files changed, 754 insertions(+), 83 deletions(-)
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(<TableExploreTree queryEditorId={mockedQueryEditorId} />, {
+ 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<string, { options: Table[] }>;
tableSchemaData: Record<string, TableMetaData>;
loadingNodes: Record<string, boolean>;
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(<DatabaseSelector {...props} />, { 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(<DatabaseSelector {...props} db={null} />, {
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;
