diff --git a/.changeset/fair-mirrors-hammer.md b/.changeset/fair-mirrors-hammer.md new file mode 100644 index 0000000000..3884835d11 --- /dev/null +++ b/.changeset/fair-mirrors-hammer.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-graph': patch +--- + +use `CatalogClient.getEntitiesByRefs()` to reduce the number of backend requests from plugin `catalog-graph` diff --git a/plugins/catalog-graph/src/components/CatalogGraphCard/CatalogGraphCard.test.tsx b/plugins/catalog-graph/src/components/CatalogGraphCard/CatalogGraphCard.test.tsx index cdd55e300f..1f6c77ccf3 100644 --- a/plugins/catalog-graph/src/components/CatalogGraphCard/CatalogGraphCard.test.tsx +++ b/plugins/catalog-graph/src/components/CatalogGraphCard/CatalogGraphCard.test.tsx @@ -40,6 +40,7 @@ describe('', () => { const catalog = { getEntities: jest.fn(), getEntityByRef: jest.fn(), + getEntitiesByRefs: jest.fn(), removeEntityByUid: jest.fn(), getLocationById: jest.fn(), getLocationByRef: jest.fn(), @@ -75,9 +76,13 @@ describe('', () => { }); test('renders without exploding', async () => { - catalog.getEntityByRef.mockImplementation(async _ => ({ - ...entity, - relations: [], + catalog.getEntitiesByRefs.mockImplementation(async _ => ({ + items: [ + { + ...entity, + relations: [], + }, + ], })); await renderInTestApp(wrapper, { @@ -89,13 +94,20 @@ describe('', () => { expect(await screen.findByText('b:d/c')).toBeInTheDocument(); expect(await screen.findAllByTestId('node')).toHaveLength(1); - expect(catalog.getEntityByRef).toHaveBeenCalledTimes(1); + expect(catalog.getEntitiesByRefs).toHaveBeenCalledTimes(1); + expect(catalog.getEntitiesByRefs).toHaveBeenCalledWith( + expect.objectContaining({ entityRefs: ['b:d/c'] }), + ); }); test('renders with custom title', async () => { - catalog.getEntityByRef.mockImplementation(async _ => ({ - ...entity, - relations: [], + catalog.getEntitiesByRefs.mockImplementation(async _ => ({ + items: [ + { + ...entity, + relations: [], + }, + ], })); await renderInTestApp( @@ -116,9 +128,13 @@ describe('', () => { }); test('renders link to standalone viewer', async () => { - catalog.getEntityByRef.mockImplementation(async _ => ({ - ...entity, - relations: [], + catalog.getEntitiesByRefs.mockImplementation(async _ => ({ + items: [ + { + ...entity, + relations: [], + }, + ], })); await renderInTestApp(wrapper, { @@ -138,6 +154,15 @@ describe('', () => { }); test('renders link to standalone viewer with custom config', async () => { + catalog.getEntitiesByRefs.mockImplementation(async _ => ({ + items: [ + { + ...entity, + relations: [], + }, + ], + })); + await renderInTestApp( @@ -162,9 +187,13 @@ describe('', () => { }); test('captures analytics event on click', async () => { - catalog.getEntityByRef.mockImplementation(async _ => ({ - ...entity, - relations: [], + catalog.getEntitiesByRefs.mockImplementation(async _ => ({ + items: [ + { + ...entity, + relations: [], + }, + ], })); const analyticsSpy = new MockAnalyticsApi(); diff --git a/plugins/catalog-graph/src/components/CatalogGraphPage/CatalogGraphPage.test.tsx b/plugins/catalog-graph/src/components/CatalogGraphPage/CatalogGraphPage.test.tsx index f2360b678d..e188dcc7ff 100644 --- a/plugins/catalog-graph/src/components/CatalogGraphPage/CatalogGraphPage.test.tsx +++ b/plugins/catalog-graph/src/components/CatalogGraphPage/CatalogGraphPage.test.tsx @@ -26,6 +26,7 @@ import { screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { CatalogGraphPage } from './CatalogGraphPage'; +import { GetEntitiesByRefsRequest } from '@backstage/catalog-client'; const navigate = jest.fn(); @@ -111,9 +112,14 @@ describe.skip('', () => { }, ], }; + const allEntities: Record = { + 'b:d/c': entityC, + 'b:d/e': entityE, + }; const catalog = { getEntities: jest.fn(), getEntityByRef: jest.fn(), + getEntitiesByRefs: jest.fn(), removeEntityByUid: jest.fn(), getLocationById: jest.fn(), getLocationByRef: jest.fn(), @@ -142,8 +148,10 @@ describe.skip('', () => { afterEach(() => jest.resetAllMocks()); test('should render without exploding', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); await renderInTestApp(wrapper, { @@ -156,12 +164,14 @@ describe.skip('', () => { await expect(screen.findByText('b:d/c')).resolves.toBeInTheDocument(); await expect(screen.findByText('b:d/e')).resolves.toBeInTheDocument(); await expect(screen.findAllByTestId('node')).resolves.toHaveLength(2); - expect(catalog.getEntityByRef).toHaveBeenCalledTimes(2); + expect(catalog.getEntitiesByRefs).toHaveBeenCalledTimes(2); }); test('should toggle filters', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); await renderInTestApp(wrapper, { @@ -178,8 +188,10 @@ describe.skip('', () => { }); test('should select other entity', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); await renderInTestApp(wrapper, { @@ -196,8 +208,10 @@ describe.skip('', () => { }); test('should navigate to entity', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); await renderInTestApp(wrapper, { @@ -215,8 +229,10 @@ describe.skip('', () => { }); test('should capture analytics event when selecting other entity', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); const analyticsSpy = new MockAnalyticsApi(); @@ -242,8 +258,10 @@ describe.skip('', () => { }); test('should capture analytics event when navigating to entity', async () => { - catalog.getEntityByRef.mockImplementation(async (n: any) => - n === 'b:d/e' ? entityE : entityC, + catalog.getEntitiesByRefs.mockImplementation( + async ({ entityRefs }: GetEntitiesByRefsRequest) => ({ + items: entityRefs.map(ref => allEntities[ref]), + }), ); const analyticsSpy = new MockAnalyticsApi(); diff --git a/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.test.ts b/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.test.ts index 0d75fabc3b..786c37ca7e 100644 --- a/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.test.ts +++ b/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.test.ts @@ -27,6 +27,7 @@ describe('useEntityStore', () => { const catalogApi = { getEntities: jest.fn(), getEntityByRef: jest.fn(), + getEntitiesByRefs: jest.fn(), removeEntityByUid: jest.fn(), getLocationById: jest.fn(), getLocationByRef: jest.fn(), @@ -63,7 +64,7 @@ describe('useEntityStore', () => { }, }; - catalogApi.getEntityByRef.mockResolvedValue(entity); + catalogApi.getEntitiesByRefs.mockResolvedValue({ items: [entity] }); const { result } = renderHook(() => useEntityStore()); @@ -83,7 +84,7 @@ describe('useEntityStore', () => { test('handles request failures', async () => { const err = new Error('Hello World'); - catalogApi.getEntityByRef.mockRejectedValue(err); + catalogApi.getEntitiesByRefs.mockRejectedValue(err); const { result } = renderHook(() => useEntityStore()); @@ -100,7 +101,7 @@ describe('useEntityStore', () => { }); test('handles loading', async () => { - catalogApi.getEntityByRef.mockReturnValue(new Promise(() => {})); + catalogApi.getEntitiesByRefs.mockReturnValue(new Promise(() => {})); const { result } = renderHook(() => useEntityStore()); @@ -131,8 +132,16 @@ describe('useEntityStore', () => { name: 'name2', }, }; + const entity3: Entity = { + apiVersion: 'v1', + kind: 'kind', + metadata: { + namespace: 'namespace', + name: 'name3', + }, + }; - catalogApi.getEntityByRef.mockResolvedValue(entity1); + catalogApi.getEntitiesByRefs.mockResolvedValue({ items: [entity1] }); const { result } = renderHook(() => useEntityStore()); @@ -149,12 +158,15 @@ describe('useEntityStore', () => { }); }); - catalogApi.getEntityByRef.mockResolvedValue(entity2); + catalogApi.getEntitiesByRefs.mockResolvedValue({ + items: [entity2, entity3], + }); act(() => { result.current.requestEntities([ 'kind:namespace/name1', 'kind:namespace/name2', + 'kind:namespace/name3', ]); }); @@ -165,6 +177,7 @@ describe('useEntityStore', () => { expect(entities).toEqual({ 'kind:namespace/name1': entity1, 'kind:namespace/name2': entity2, + 'kind:namespace/name3': entity3, }); }); }); @@ -186,13 +199,26 @@ describe('useEntityStore', () => { name: 'name2', }, }; + const entity3: Entity = { + apiVersion: 'v1', + kind: 'kind', + metadata: { + namespace: 'namespace', + name: 'name3', + }, + }; - catalogApi.getEntityByRef.mockResolvedValue(entity1); + catalogApi.getEntitiesByRefs.mockResolvedValue({ + items: [entity1, entity2], + }); const { result } = renderHook(() => useEntityStore()); act(() => { - result.current.requestEntities(['kind:namespace/name1']); + result.current.requestEntities([ + 'kind:namespace/name1', + 'kind:namespace/name2', + ]); }); await waitFor(() => { @@ -201,13 +227,22 @@ describe('useEntityStore', () => { expect(error).toBeUndefined(); expect(entities).toEqual({ 'kind:namespace/name1': entity1, + 'kind:namespace/name2': entity2, }); }); - catalogApi.getEntityByRef.mockResolvedValue(entity2); + expect(catalogApi.getEntitiesByRefs).toHaveBeenCalledTimes(1); + expect(catalogApi.getEntitiesByRefs).toHaveBeenLastCalledWith({ + entityRefs: ['kind:namespace/name1', 'kind:namespace/name2'], + }); + + catalogApi.getEntitiesByRefs.mockResolvedValue({ items: [entity3] }); act(() => { - result.current.requestEntities(['kind:namespace/name2']); + result.current.requestEntities([ + 'kind:namespace/name2', + 'kind:namespace/name3', + ]); }); await waitFor(() => { @@ -216,9 +251,15 @@ describe('useEntityStore', () => { expect(error).toBeUndefined(); expect(entities).toEqual({ 'kind:namespace/name2': entity2, + 'kind:namespace/name3': entity3, }); }); + expect(catalogApi.getEntitiesByRefs).toHaveBeenCalledTimes(2); + expect(catalogApi.getEntitiesByRefs).toHaveBeenLastCalledWith({ + entityRefs: ['kind:namespace/name3'], + }); + act(() => { result.current.requestEntities(['kind:namespace/name1']); }); @@ -232,6 +273,6 @@ describe('useEntityStore', () => { }); }); - expect(catalogApi.getEntityByRef).toHaveBeenCalledTimes(2); + expect(catalogApi.getEntitiesByRefs).toHaveBeenCalledTimes(2); }); }); diff --git a/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.ts b/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.ts index 843b308281..5c33f8053d 100644 --- a/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.ts +++ b/plugins/catalog-graph/src/components/EntityRelationsGraph/useEntityStore.ts @@ -13,18 +13,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { Entity } from '@backstage/catalog-model'; +import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { useApi } from '@backstage/core-plugin-api'; import { catalogApiRef } from '@backstage/plugin-catalog-react'; -import limiterFactory from 'p-limit'; import { Dispatch, useCallback, useRef, useState } from 'react'; import useAsyncFn from 'react-use/lib/useAsyncFn'; // TODO: This is a good use case for a graphql API, once it is available in the // future. -const limiter = limiterFactory(10); - /** * Ensures that a set of requested entities is loaded. */ @@ -58,38 +55,24 @@ export function useEntityStore(): { }, [state, setEntities]); const [asyncState, fetch] = useAsyncFn(async () => { - const { requestedEntities, outstandingEntities, cachedEntities } = - state.current; - - await Promise.all( - Array.from(requestedEntities).map(entityRef => - limiter(async () => { - if (cachedEntities.has(entityRef)) { - return; - } - - if (outstandingEntities.has(entityRef)) { - await outstandingEntities.get(entityRef); - return; - } - - const promise = catalogClient.getEntityByRef(entityRef); - - outstandingEntities.set(entityRef, promise); - - try { - const entity = await promise; - - if (entity) { - cachedEntities.set(entityRef, entity); - updateEntities(); - } - } finally { - outstandingEntities.delete(entityRef); - } - }), - ), + const { requestedEntities, cachedEntities } = state.current; + const entityRefs: string[] = Array.from(requestedEntities).filter( + entityRef => !cachedEntities.has(entityRef), ); + if (entityRefs.length === 0) { + updateEntities(); + return; + } + + const { items } = await catalogClient.getEntitiesByRefs({ entityRefs }); + items.forEach(ent => { + if (ent) { + const entityRef = stringifyEntityRef(ent); + cachedEntities.set(entityRef, ent); + } + }); + + updateEntities(); }, [state, updateEntities]); const { loading, error } = asyncState;