This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-dynamic-search-dataset in repository https://gitbox.apache.org/repos/asf/superset.git
commit d8577a967664c932a021303c5ce52bf58c75f157 Author: Joe Li <[email protected]> AuthorDate: Tue Mar 3 23:10:21 2026 -0800 fix(roles): suppress duplicate toasts on hydration failure and add cache key test When fetchPaginatedData fails, it toasts the error but never calls setData, leaving rolePermissions/roleGroups empty. The reconciliation useEffect then fires a second "shown as IDs" toast. Track fetch success via refs so the fallback toast only fires when the fetch succeeded but returned partial data. Also adds a test asserting different-case queries use distinct cache keys, fixes no-use-before-define order, and adds type annotations for strictness. Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../src/features/roles/RoleListEditModal.test.tsx | 53 +++++++ .../src/features/roles/RoleListEditModal.tsx | 22 ++- superset-frontend/src/features/roles/utils.test.ts | 153 ++++++++++++++++++++- superset-frontend/src/features/roles/utils.ts | 79 ++++++++--- 4 files changed, 279 insertions(+), 28 deletions(-) diff --git a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx b/superset-frontend/src/features/roles/RoleListEditModal.test.tsx index 79aefc72a1b..7c8723ef713 100644 --- a/superset-frontend/src/features/roles/RoleListEditModal.test.tsx +++ b/superset-frontend/src/features/roles/RoleListEditModal.test.tsx @@ -272,6 +272,59 @@ describe('RoleListEditModal', () => { }); }); + test('does not fire fallback toast when hydration fetch fails', async () => { + mockToasts.addDangerToast.mockClear(); + const mockGet = SupersetClient.get as jest.Mock; + mockGet.mockImplementation(({ endpoint }) => { + if (endpoint?.includes('/api/v1/security/permissions-resources/')) { + return Promise.reject(new Error('network error')); + } + if (endpoint?.includes('/api/v1/security/groups/')) { + return Promise.reject(new Error('network error')); + } + return Promise.resolve({ json: { count: 0, result: [] } }); + }); + + render(<RoleListEditModal {...mockProps} />); + + await waitFor(() => { + // fetchPaginatedData fires the error toasts + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'There was an error loading permissions.', + ); + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'There was an error loading groups.', + ); + }); + + // The fallback "shown as IDs" toasts should NOT have fired + expect(mockToasts.addDangerToast).not.toHaveBeenCalledWith( + 'Some permissions could not be resolved and are shown as IDs.', + ); + expect(mockToasts.addDangerToast).not.toHaveBeenCalledWith( + 'Some groups could not be resolved and are shown as IDs.', + ); + }); + + test('fires warning toast when hydration returns zero rows but IDs were expected', async () => { + const mockGet = SupersetClient.get as jest.Mock; + mockGet.mockImplementation(({ endpoint }) => + Promise.resolve({ json: { count: 0, result: [] } }), + ); + + render(<RoleListEditModal {...mockProps} />); + + await waitFor(() => { + // Both warnings should fire because IDs were expected but none resolved + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'Some permissions could not be resolved and are shown as IDs.', + ); + expect(mockToasts.addDangerToast).toHaveBeenCalledWith( + 'Some groups could not be resolved and are shown as IDs.', + ); + }); + }); + test('fetches permissions and groups by id for hydration', async () => { const mockGet = SupersetClient.get as jest.Mock; mockGet.mockResolvedValue({ diff --git a/superset-frontend/src/features/roles/RoleListEditModal.tsx b/superset-frontend/src/features/roles/RoleListEditModal.tsx index ce7a42a7d85..8713faf4dbf 100644 --- a/superset-frontend/src/features/roles/RoleListEditModal.tsx +++ b/superset-frontend/src/features/roles/RoleListEditModal.tsx @@ -120,6 +120,8 @@ function RoleListEditModal({ const [loadingRolePermissions, setLoadingRolePermissions] = useState(true); const [loadingRoleGroups, setLoadingRoleGroups] = useState(true); const formRef = useRef<FormInstance | null>(null); + const permissionFetchSucceeded = useRef(false); + const groupFetchSucceeded = useRef(false); useEffect(() => { const filters = [{ col: 'roles', opr: 'rel_m_m', value: id }]; @@ -151,12 +153,16 @@ function RoleListEditModal({ } setLoadingRolePermissions(true); + permissionFetchSucceeded.current = false; const filters = [{ col: 'id', opr: 'in', value: stablePermissionIds }]; fetchPaginatedData({ endpoint: `/api/v1/security/permissions-resources/`, pageSize: 100, - setData: setRolePermissions, + setData: (data: SelectOption[]) => { + permissionFetchSucceeded.current = true; + setRolePermissions(data); + }, filters, setLoadingState: (loading: boolean) => setLoadingRolePermissions(loading), loadingKey: 'rolePermissions', @@ -184,12 +190,16 @@ function RoleListEditModal({ } setLoadingRoleGroups(true); + groupFetchSucceeded.current = false; const filters = [{ col: 'id', opr: 'in', value: stableGroupIds }]; fetchPaginatedData({ endpoint: `/api/v1/security/groups/`, pageSize: 100, - setData: setRoleGroups, + setData: (data: SelectOption[]) => { + groupFetchSucceeded.current = true; + setRoleGroups(data); + }, filters, setLoadingState: (loading: boolean) => setLoadingRoleGroups(loading), loadingKey: 'roleGroups', @@ -219,7 +229,7 @@ function RoleListEditModal({ if ( !loadingRolePermissions && formRef.current && - rolePermissions.length > 0 + stablePermissionIds.length > 0 ) { const fetchedIds = new Set(rolePermissions.map(p => p.value)); const missingIds = stablePermissionIds.filter(id => !fetchedIds.has(id)); @@ -227,7 +237,7 @@ function RoleListEditModal({ ...rolePermissions, ...missingIds.map(id => ({ value: id, label: String(id) })), ]; - if (missingIds.length > 0) { + if (missingIds.length > 0 && permissionFetchSucceeded.current) { addDangerToast( t('Some permissions could not be resolved and are shown as IDs.'), ); @@ -239,14 +249,14 @@ function RoleListEditModal({ }, [loadingRolePermissions, rolePermissions, stablePermissionIds, addDangerToast]); useEffect(() => { - if (!loadingRoleGroups && formRef.current && roleGroups.length > 0) { + if (!loadingRoleGroups && formRef.current && stableGroupIds.length > 0) { const fetchedIds = new Set(roleGroups.map(g => g.value)); const missingIds = stableGroupIds.filter(id => !fetchedIds.has(id)); const allGroups = [ ...roleGroups, ...missingIds.map(id => ({ value: id, label: String(id) })), ]; - if (missingIds.length > 0) { + if (missingIds.length > 0 && groupFetchSucceeded.current) { addDangerToast( t('Some groups could not be resolved and are shown as IDs.'), ); diff --git a/superset-frontend/src/features/roles/utils.test.ts b/superset-frontend/src/features/roles/utils.test.ts index fa70c3549bb..b8d9455e502 100644 --- a/superset-frontend/src/features/roles/utils.test.ts +++ b/superset-frontend/src/features/roles/utils.test.ts @@ -58,12 +58,12 @@ test('fetchPermissionOptions fetches all results on page 0 with large page_size' expect(queries).toContainEqual({ page: 0, - page_size: 5000, + page_size: 1000, filters: [{ col: 'view_menu.name', opr: 'ct', value: 'dataset' }], }); expect(queries).toContainEqual({ page: 0, - page_size: 5000, + page_size: 1000, filters: [{ col: 'permission.name', opr: 'ct', value: 'dataset' }], }); @@ -269,3 +269,152 @@ test('fetchGroupOptions returns empty options on error', async () => { ); expect(result).toEqual({ data: [], totalCount: 0 }); }); + +test('fetchPermissionOptions fetches multiple pages when results exceed PAGE_SIZE', async () => { + const PAGE_SIZE = 1000; + const totalCount = 1500; + const page0Items = Array.from({ length: PAGE_SIZE }, (_, i) => ({ + id: i + 1, + permission: { name: `perm_${i}` }, + view_menu: { name: `view_${i}` }, + })); + const page1Items = Array.from({ length: totalCount - PAGE_SIZE }, (_, i) => ({ + id: PAGE_SIZE + i + 1, + permission: { name: `perm_${PAGE_SIZE + i}` }, + view_menu: { name: `view_${PAGE_SIZE + i}` }, + })); + + getMock.mockImplementation(({ endpoint }: { endpoint: string }) => { + const query = rison.decode(endpoint.split('?q=')[1]) as Record< + string, + unknown + >; + if (query.page === 0) { + return Promise.resolve({ + json: { count: totalCount, result: page0Items }, + } as any); + } + if (query.page === 1) { + return Promise.resolve({ + json: { count: totalCount, result: page1Items }, + } as any); + } + return Promise.resolve({ json: { count: 0, result: [] } } as any); + }); + + const addDangerToast = jest.fn(); + const result = await fetchPermissionOptions('multi', 0, 50, addDangerToast); + + // Two search branches (view_menu + permission), each needing 2 pages = 4 calls + expect(getMock).toHaveBeenCalledTimes(4); + // Deduplicated: both branches return identical ids, so total is 1500 + expect(result.totalCount).toBe(totalCount); +}); + +test('fetchPermissionOptions handles backend capping page_size below requested', async () => { + const BACKEND_CAP = 500; + const totalCount = 1200; + const makeItems = (start: number, count: number) => + Array.from({ length: count }, (_, i) => ({ + id: start + i + 1, + permission: { name: `perm_${start + i}` }, + view_menu: { name: `view_${start + i}` }, + })); + + getMock.mockImplementation(({ endpoint }: { endpoint: string }) => { + const query = rison.decode(endpoint.split('?q=')[1]) as Record< + string, + unknown + >; + const page = query.page as number; + let items: ReturnType<typeof makeItems>; + if (page === 0) { + items = makeItems(0, BACKEND_CAP); + } else if (page === 1) { + items = makeItems(BACKEND_CAP, BACKEND_CAP); + } else { + items = makeItems(BACKEND_CAP * 2, totalCount - BACKEND_CAP * 2); + } + return Promise.resolve({ + json: { count: totalCount, result: items }, + } as any); + }); + + const addDangerToast = jest.fn(); + const result = await fetchPermissionOptions('cap', 0, 50, addDangerToast); + + // Two search branches, each needing 3 pages (500+500+200) = 6 calls + expect(getMock).toHaveBeenCalledTimes(6); + // Both branches return identical ids, so deduplicated total is 1200 + expect(result.totalCount).toBe(totalCount); + expect(result.data).toHaveLength(50); // first page of client-side pagination +}); + +test('fetchPermissionOptions treats different-case queries as distinct cache keys', async () => { + getMock.mockResolvedValue({ + json: { + count: 1, + result: [ + { + id: 10, + permission: { name: 'can_access' }, + view_menu: { name: 'dataset_one' }, + }, + ], + }, + } as any); + const addDangerToast = jest.fn(); + + await fetchPermissionOptions('Dataset', 0, 50, addDangerToast); + expect(getMock).toHaveBeenCalledTimes(2); + + // Same letters, different case should be a cache miss (separate key) + const result = await fetchPermissionOptions('dataset', 0, 50, addDangerToast); + expect(getMock).toHaveBeenCalledTimes(4); + expect(result).toEqual({ + data: [{ value: 10, label: 'can access dataset one' }], + totalCount: 1, + }); +}); + +test('fetchPermissionOptions evicts oldest cache entry when MAX_CACHE_ENTRIES is reached', async () => { + getMock.mockImplementation(({ endpoint }: { endpoint: string }) => { + const query = rison.decode(endpoint.split('?q=')[1]) as Record<string, any>; + const searchVal = query.filters?.[0]?.value || 'unknown'; + return Promise.resolve({ + json: { + count: 1, + result: [ + { + id: Number(searchVal.replace('term', '')), + permission: { name: searchVal }, + view_menu: { name: 'view' }, + }, + ], + }, + } as any); + }); + + const addDangerToast = jest.fn(); + + // Fill cache with 20 entries (MAX_CACHE_ENTRIES) + for (let i = 0; i < 20; i += 1) { + await fetchPermissionOptions(`term${i}`, 0, 50, addDangerToast); + } + + getMock.mockClear(); + + // Adding the 21st entry should evict the oldest (term0) + await fetchPermissionOptions('term20', 0, 50, addDangerToast); + + // term0 should have been evicted — re-fetching it should trigger API calls + getMock.mockClear(); + await fetchPermissionOptions('term0', 0, 50, addDangerToast); + expect(getMock).toHaveBeenCalled(); + + // term2 should still be cached — no API calls + // (term1 was evicted when term0 was re-added as the 21st entry) + getMock.mockClear(); + await fetchPermissionOptions('term2', 0, 50, addDangerToast); + expect(getMock).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/features/roles/utils.ts b/superset-frontend/src/features/roles/utils.ts index e654e0bfc62..d67e4014f3a 100644 --- a/superset-frontend/src/features/roles/utils.ts +++ b/superset-frontend/src/features/roles/utils.ts @@ -72,16 +72,16 @@ const mapPermissionResults = (results: PermissionResult[]) => label: formatPermissionLabel(item.permission.name, item.view_menu.name), })); -const MAX_PERMISSION_SEARCH_SIZE = 5000; +const PAGE_SIZE = 1000; +const CONCURRENCY_LIMIT = 3; +const MAX_CACHE_ENTRIES = 20; const permissionSearchCache = new Map<string, SelectOption[]>(); export const clearPermissionSearchCache = () => { permissionSearchCache.clear(); }; -const fetchPermissionPageRaw = async ( - queryParams: Record<string, unknown>, -) => { +const fetchPermissionPageRaw = async (queryParams: Record<string, unknown>) => { const response = await SupersetClient.get({ endpoint: `/api/v1/security/permissions-resources/?q=${rison.encode(queryParams)}`, }); @@ -91,6 +91,45 @@ const fetchPermissionPageRaw = async ( }; }; +const fetchAllPermissionPages = async ( + filters: Record<string, unknown>[], +): Promise<SelectOption[]> => { + const page0 = await fetchPermissionPageRaw({ + page: 0, + page_size: PAGE_SIZE, + filters, + }); + if (page0.data.length === 0 || page0.data.length >= page0.totalCount) { + return page0.data; + } + + // Use actual returned size — backend may cap below PAGE_SIZE + const actualPageSize = page0.data.length; + const totalPages = Math.ceil(page0.totalCount / actualPageSize); + const allResults = [...page0.data]; + + // Fetch remaining pages in batches of CONCURRENCY_LIMIT + for (let batch = 1; batch < totalPages; batch += CONCURRENCY_LIMIT) { + const batchEnd = Math.min(batch + CONCURRENCY_LIMIT, totalPages); + const batchResults = await Promise.all( + Array.from({ length: batchEnd - batch }, (_, i) => + fetchPermissionPageRaw({ + page: batch + i, + page_size: PAGE_SIZE, + filters, + }), + ), + ); + for (const r of batchResults) { + allResults.push(...r.data); + if (r.data.length === 0) return allResults; + } + if (allResults.length >= page0.totalCount) break; + } + + return allResults; +}; + export const fetchPermissionOptions = async ( filterValue: string, page: number, @@ -108,31 +147,31 @@ export const fetchPermissionOptions = async ( } try { - let cached = permissionSearchCache.get(filterValue); + const cacheKey = filterValue; + let cached = permissionSearchCache.get(cacheKey); if (!cached) { - const searchQuery = { page: 0, page_size: MAX_PERMISSION_SEARCH_SIZE }; const [byViewMenu, byPermission] = await Promise.all([ - fetchPermissionPageRaw({ - ...searchQuery, - filters: [ - { col: 'view_menu.name', opr: 'ct', value: filterValue }, - ], - }), - fetchPermissionPageRaw({ - ...searchQuery, - filters: [ - { col: 'permission.name', opr: 'ct', value: filterValue }, - ], - }), + fetchAllPermissionPages([ + { col: 'view_menu.name', opr: 'ct', value: filterValue }, + ]), + fetchAllPermissionPages([ + { col: 'permission.name', opr: 'ct', value: filterValue }, + ]), ]); const seen = new Set<number>(); - cached = [...byViewMenu.data, ...byPermission.data].filter(item => { + cached = [...byViewMenu, ...byPermission].filter(item => { if (seen.has(item.value)) return false; seen.add(item.value); return true; }); - permissionSearchCache.set(filterValue, cached); + if (permissionSearchCache.size >= MAX_CACHE_ENTRIES) { + const oldestKey = permissionSearchCache.keys().next().value; + if (oldestKey !== undefined) { + permissionSearchCache.delete(oldestKey); + } + } + permissionSearchCache.set(cacheKey, cached); } const start = page * pageSize;
