From 097a3342285199a44d67b191dc360df4d5b06666 Mon Sep 17 00:00:00 2001 From: Jacob Raihle kdm951 Date: Tue, 15 Jul 2025 16:28:52 +0300 Subject: [PATCH 1/4] Add tests to demonstrate a race condition in EntityListProvider Signed-off-by: Jacob Raihle kdm951 --- .../src/hooks/useEntityListProvider.test.tsx | 66 +++++++++++++++++-- .../src/testUtils/MockPromise.test.ts | 31 +++++++++ .../src/testUtils/MockPromise.ts | 47 +++++++++++++ 3 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 plugins/catalog-react/src/testUtils/MockPromise.test.ts create mode 100644 plugins/catalog-react/src/testUtils/MockPromise.ts diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx index de85fa1042..cd92605875 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import { catalogApiMock } from '@backstage/plugin-catalog-react/testUtils'; +import type { GetEntitiesResponse } from '@backstage/catalog-client'; import { Entity } from '@backstage/catalog-model'; import { alertApiRef, @@ -23,7 +23,10 @@ import { identityApiRef, storageApiRef, } from '@backstage/core-plugin-api'; +import { translationApiRef } from '@backstage/core-plugin-api/alpha'; +import { catalogApiMock } from '@backstage/plugin-catalog-react/testUtils'; import { mockApis, TestApiProvider } from '@backstage/test-utils'; +import { useMountEffect } from '@react-hookz/web'; import { act, renderHook, waitFor } from '@testing-library/react'; import qs from 'qs'; import { PropsWithChildren } from 'react'; @@ -37,10 +40,9 @@ import { EntityTypeFilter, EntityUserFilter, } from '../filters'; -import { EntityListProvider, useEntityList } from './useEntityListProvider'; -import { useMountEffect } from '@react-hookz/web'; -import { translationApiRef } from '@backstage/core-plugin-api/alpha'; +import { MockPromise } from '../testUtils/MockPromise'; import { EntityListPagination } from '../types'; +import { EntityListProvider, useEntityList } from './useEntityListProvider'; const entities: Entity[] = [ { @@ -341,6 +343,62 @@ describe('', () => { filter: { kind: 'group' }, }); }); + + it('uses the last applied filter even if an earlier request finishes later', async () => { + const { result } = renderHook(() => useEntityList(), { + wrapper: createWrapper({ pagination }), + }); + + const firstResult = new MockPromise(); + const secondResult = new MockPromise(); + + await waitFor(() => { + expect(result.current.backendEntities.length).toBeGreaterThan(0); + }); + expect(result.current.totalItems).toBe(2); + expect(result.current.backendEntities.length).toBe(2); + expect(mockCatalogApi.getEntities).toHaveBeenCalledTimes(1); + + mockCatalogApi.getEntities!.mockReturnValueOnce(firstResult); + + await act(async () => { + result.current.updateFilters({ + kind: new EntityKindFilter('api', 'API'), + }); + }); + + await waitFor(() => { + expect(mockCatalogApi.getEntities).toHaveBeenNthCalledWith(2, { + filter: { kind: 'api' }, + }); + }); + + mockCatalogApi.getEntities!.mockReturnValueOnce(secondResult); + + await act(async () => { + result.current.updateFilters({ + kind: new EntityKindFilter('system', 'System'), + }); + }); + + await waitFor(() => { + expect(mockCatalogApi.getEntities).toHaveBeenNthCalledWith(3, { + filter: { kind: 'system' }, + }); + }); + + await act(async () => { + secondResult.resolve({ + items: [], + }); + firstResult.resolve({ + items: entities, + }); + }); + + expect(result.current.filters.kind!.value).toBe('system'); + expect(result.current.backendEntities.length).toBe(0); + }); }); describe('', () => { diff --git a/plugins/catalog-react/src/testUtils/MockPromise.test.ts b/plugins/catalog-react/src/testUtils/MockPromise.test.ts new file mode 100644 index 0000000000..6d6789f7a2 --- /dev/null +++ b/plugins/catalog-react/src/testUtils/MockPromise.test.ts @@ -0,0 +1,31 @@ +/* + * Copyright 2025 The Backstage Authors + * + * Licensed 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 { MockPromise } from './MockPromise.ts'; + +describe('MockPromise', () => { + it('is resolved when its resolve function is called', async () => { + const p = new MockPromise(); + p.resolve(12); + return expect(p).resolves.toBe(12); + }); + + it('is rejected when its reject function is called', async () => { + const p = new MockPromise(); + p.reject('Test rejection'); + return expect(p).rejects.toMatch('Test rejection'); + }); +}); diff --git a/plugins/catalog-react/src/testUtils/MockPromise.ts b/plugins/catalog-react/src/testUtils/MockPromise.ts new file mode 100644 index 0000000000..cc46eaafda --- /dev/null +++ b/plugins/catalog-react/src/testUtils/MockPromise.ts @@ -0,0 +1,47 @@ +/* + * Copyright 2025 The Backstage Authors + * + * Licensed 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. + */ +type ResolveFn = (value: T | PromiseLike) => void; +type RejectFn = (reason?: any) => void; +type Executor = (resolve: ResolveFn, reject: RejectFn) => void; + +export class MockPromise extends Promise { + private _resolve!: ResolveFn; + private _reject!: RejectFn; + + constructor(executor?: Executor) { + if (executor !== undefined) { + super(executor); + return; + } + + let res: ResolveFn; + let rej: RejectFn; + super((resolve, reject) => { + res = resolve; + rej = reject; + }); + this._resolve = res!; + this._reject = rej!; + } + + get resolve() { + return this._resolve; + } + + get reject() { + return this._reject; + } +} From 4c98f05f0a435c2386b255b89a6d0b019fd4f167 Mon Sep 17 00:00:00 2001 From: Jacob Raihle kdm951 Date: Tue, 15 Jul 2025 16:29:14 +0300 Subject: [PATCH 2/4] Fix the race condition in EntityListProvider Signed-off-by: Jacob Raihle kdm951 --- .../src/hooks/useEntityListProvider.tsx | 118 ++++++++++-------- 1 file changed, 67 insertions(+), 51 deletions(-) diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx index 56af7e9a73..f424dafb56 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx @@ -14,7 +14,9 @@ * limitations under the License. */ +import { QueryEntitiesResponse } from '@backstage/catalog-client'; import { Entity } from '@backstage/catalog-model'; +import { useApi } from '@backstage/core-plugin-api'; import { compact, isEqual } from 'lodash'; import qs from 'qs'; import { @@ -22,6 +24,7 @@ import { PropsWithChildren, useCallback, useContext, + useEffect, useMemo, useState, } from 'react'; @@ -50,8 +53,6 @@ import { reduceCatalogFilters, reduceEntityFilters, } from '../utils/filters'; -import { useApi } from '@backstage/core-plugin-api'; -import { QueryEntitiesResponse } from '@backstage/catalog-client'; /** @public */ export type DefaultEntityFilters = { @@ -239,7 +240,7 @@ export const EntityListProvider = ( // The main async filter worker. Note that while it has a lot of dependencies // in terms of its implementation, the triggering only happens (debounced) // based on the requested filters changing. - const [{ loading, error }, refresh] = useAsyncFn( + const [{ value: resolvedValue, loading, error }, refresh] = useAsyncFn( async () => { const kindValue = requestedFilters.kind?.value?.toLocaleLowerCase('en-US'); @@ -249,19 +250,6 @@ export const EntityListProvider = ( : requestedFilters; const compacted = compact(Object.values(adjustedFilters)); - const queryParams = Object.keys(requestedFilters).reduce( - (params, key) => { - const filter = requestedFilters[key as keyof EntityFilters] as - | EntityFilter - | undefined; - if (filter?.toQueryValue) { - params[key] = filter.toQueryValue(); - } - return params; - }, - {} as Record, - ); - if (paginationMode !== 'none') { if (cursor) { if (cursor !== outputState.appliedCursor) { @@ -270,14 +258,14 @@ export const EntityListProvider = ( cursor, limit, }); - setOutputState({ + return { appliedFilters: requestedFilters, appliedCursor: cursor, backendEntities: response.items, entities: response.items.filter(entityFilter), pageInfo: response.pageInfo, totalItems: response.totalItems, - }); + }; } } else { const entityFilter = reduceEntityFilters(compacted); @@ -296,7 +284,7 @@ export const EntityListProvider = ( limit, offset, }); - setOutputState({ + return { appliedFilters: requestedFilters, backendEntities: response.items, entities: response.items.filter(entityFilter), @@ -304,7 +292,7 @@ export const EntityListProvider = ( totalItems: response.totalItems, limit, offset, - }); + }; } } } else { @@ -324,43 +312,22 @@ export const EntityListProvider = ( filter: backendFilter, }); const entities = response.items.filter(entityFilter); - setOutputState({ + return { appliedFilters: requestedFilters, backendEntities: response.items, entities, totalItems: entities.length, - }); - } else { - const entities = outputState.backendEntities.filter(entityFilter); - setOutputState({ - appliedFilters: requestedFilters, - backendEntities: outputState.backendEntities, - entities, - totalItems: entities.length, - }); + }; } + const entities = outputState.backendEntities.filter(entityFilter); + return { + appliedFilters: requestedFilters, + backendEntities: outputState.backendEntities, + entities, + totalItems: entities.length, + }; } - - if (isMounted()) { - const oldParams = qs.parse(location.search, { - ignoreQueryPrefix: true, - }); - const newParams = qs.stringify( - { - ...oldParams, - filters: queryParams, - ...(paginationMode === 'none' ? {} : { cursor, limit, offset }), - }, - { addQueryPrefix: true, arrayFormat: 'repeat' }, - ); - const newUrl = `${window.location.pathname}${newParams}`; - // We use direct history manipulation since useSearchParams and - // useNavigate in react-router-dom cause unnecessary extra rerenders. - // Also make sure to replace the state rather than pushing, since we - // don't want there to be back/forward slots for every single filter - // change. - window.history?.replaceState(null, document.title, newUrl); - } + return undefined; }, [ catalogApi, @@ -379,6 +346,55 @@ export const EntityListProvider = ( // filters will be calling this in rapid succession. useDebounce(refresh, 10, [requestedFilters, cursor, limit, offset]); + useEffect(() => { + if (resolvedValue === undefined) { + return; + } + setOutputState(resolvedValue); + if (isMounted()) { + const queryParams = Object.keys(requestedFilters).reduce( + (params, key) => { + const filter = requestedFilters[key as keyof EntityFilters] as + | EntityFilter + | undefined; + if (filter?.toQueryValue) { + params[key] = filter.toQueryValue(); + } + return params; + }, + {} as Record, + ); + + const oldParams = qs.parse(location.search, { + ignoreQueryPrefix: true, + }); + const newParams = qs.stringify( + { + ...oldParams, + filters: queryParams, + ...(paginationMode === 'none' ? {} : { cursor, limit, offset }), + }, + { addQueryPrefix: true, arrayFormat: 'repeat' }, + ); + const newUrl = `${window.location.pathname}${newParams}`; + // We use direct history manipulation since useSearchParams and + // useNavigate in react-router-dom cause unnecessary extra rerenders. + // Also make sure to replace the state rather than pushing, since we + // don't want there to be back/forward slots for every single filter + // change. + window.history?.replaceState(null, document.title, newUrl); + } + }, [ + cursor, + isMounted, + limit, + location.search, + offset, + requestedFilters, + resolvedValue, + paginationMode, + ]); + const updateFilters = useCallback( ( update: From 01747991cb8bd7e728a65b334e051fa32ddd6538 Mon Sep 17 00:00:00 2001 From: Jacob Raihle kdm951 Date: Tue, 15 Jul 2025 16:35:23 +0300 Subject: [PATCH 3/4] Add changeset Signed-off-by: Jacob Raihle kdm951 --- .changeset/cold-donuts-train.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/cold-donuts-train.md diff --git a/.changeset/cold-donuts-train.md b/.changeset/cold-donuts-train.md new file mode 100644 index 0000000000..fe84653601 --- /dev/null +++ b/.changeset/cold-donuts-train.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Fix a potential race condition in EntityListProvider when selecting filters From 629900817a3730d552d2fa6e19c53952e6b99d6f Mon Sep 17 00:00:00 2001 From: Jacob Raihle kdm951 Date: Mon, 1 Sep 2025 20:11:42 +0300 Subject: [PATCH 4/4] Replace MockPromise with existing createDeferred Signed-off-by: Jacob Raihle kdm951 --- .../src/hooks/useEntityListProvider.test.tsx | 6 +-- .../src/testUtils/MockPromise.test.ts | 31 ------------ .../src/testUtils/MockPromise.ts | 47 ------------------- 3 files changed, 3 insertions(+), 81 deletions(-) delete mode 100644 plugins/catalog-react/src/testUtils/MockPromise.test.ts delete mode 100644 plugins/catalog-react/src/testUtils/MockPromise.ts diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx index cd92605875..ef3c83315a 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx @@ -40,7 +40,7 @@ import { EntityTypeFilter, EntityUserFilter, } from '../filters'; -import { MockPromise } from '../testUtils/MockPromise'; +import { createDeferred } from '@backstage/types'; import { EntityListPagination } from '../types'; import { EntityListProvider, useEntityList } from './useEntityListProvider'; @@ -349,8 +349,8 @@ describe('', () => { wrapper: createWrapper({ pagination }), }); - const firstResult = new MockPromise(); - const secondResult = new MockPromise(); + const firstResult = createDeferred(); + const secondResult = createDeferred(); await waitFor(() => { expect(result.current.backendEntities.length).toBeGreaterThan(0); diff --git a/plugins/catalog-react/src/testUtils/MockPromise.test.ts b/plugins/catalog-react/src/testUtils/MockPromise.test.ts deleted file mode 100644 index 6d6789f7a2..0000000000 --- a/plugins/catalog-react/src/testUtils/MockPromise.test.ts +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright 2025 The Backstage Authors - * - * Licensed 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 { MockPromise } from './MockPromise.ts'; - -describe('MockPromise', () => { - it('is resolved when its resolve function is called', async () => { - const p = new MockPromise(); - p.resolve(12); - return expect(p).resolves.toBe(12); - }); - - it('is rejected when its reject function is called', async () => { - const p = new MockPromise(); - p.reject('Test rejection'); - return expect(p).rejects.toMatch('Test rejection'); - }); -}); diff --git a/plugins/catalog-react/src/testUtils/MockPromise.ts b/plugins/catalog-react/src/testUtils/MockPromise.ts deleted file mode 100644 index cc46eaafda..0000000000 --- a/plugins/catalog-react/src/testUtils/MockPromise.ts +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright 2025 The Backstage Authors - * - * Licensed 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. - */ -type ResolveFn = (value: T | PromiseLike) => void; -type RejectFn = (reason?: any) => void; -type Executor = (resolve: ResolveFn, reject: RejectFn) => void; - -export class MockPromise extends Promise { - private _resolve!: ResolveFn; - private _reject!: RejectFn; - - constructor(executor?: Executor) { - if (executor !== undefined) { - super(executor); - return; - } - - let res: ResolveFn; - let rej: RejectFn; - super((resolve, reject) => { - res = resolve; - rej = reject; - }); - this._resolve = res!; - this._reject = rej!; - } - - get resolve() { - return this._resolve; - } - - get reject() { - return this._reject; - } -}