From 5132d28818568a0c45132f9f49b49cba138a6b08 Mon Sep 17 00:00:00 2001 From: Beth Griggs Date: Fri, 21 Jun 2024 18:35:30 +0100 Subject: [PATCH 1/2] fix(plugin-org): implement batching in useGetEntities to handle large headers The useGetEntities hook can result in requests to /api/catalog/entities where the headers exceed the default maximum Node.js header size of 16KB. This commit implements batching in the useGetEntities hook to handle cases where this is likely to occur. Refs: https://github.com/backstage/backstage/issues/22139 Signed-off-by: Beth Griggs --- .changeset/fast-pens-smell.md | 5 + .../OwnershipCard/useGetEntities.test.ts | 104 ++++++++++++++++++ .../Cards/OwnershipCard/useGetEntities.ts | 50 +++++---- 3 files changed, 140 insertions(+), 19 deletions(-) create mode 100644 .changeset/fast-pens-smell.md diff --git a/.changeset/fast-pens-smell.md b/.changeset/fast-pens-smell.md new file mode 100644 index 0000000000..6d909a397c --- /dev/null +++ b/.changeset/fast-pens-smell.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-org': patch +--- + +The `useGetEntities` hook could result in requests to `/api/catalog/entities` where the headers exceed the default maximum Node.js header size of 16KB. The hook logic has been adjusted to batch the requests. diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index eac5a171ba..2c223c3f4f 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -231,6 +231,110 @@ describe('useGetEntities', () => { ); }); }); + + describe('useGetEntities with 500+ relations', () => { + const manyGroups = Array.from({ length: 555 }, (_, i) => + createGroupEntityFromName(`group${i + 1}`), + ); + + const givenUserWithManyGroups = { + kind: 'User', + metadata: { + name: givenUser, + }, + spec: { + memberOf: manyGroups.map( + group => `group:default/${group.metadata.name}`, + ), + }, + } as Partial as Entity; + + beforeEach(() => { + getEntityRelationsMock.mockImplementation(entity => + entity?.kind === 'User' + ? manyGroups.map(group => createGroupRefFromName(group.metadata.name)) + : [], + ); + (catalogApiMock.getEntities as jest.Mock).mockClear(); + }); + + it('should handle 500+ relations without exceeding URL length limits', async () => { + const { result } = renderHook( + ({ entity }) => useGetEntities(entity, 'aggregated'), + { + initialProps: { entity: givenUserWithManyGroups }, + }, + ); + + await waitFor(() => expect(result.current.loading).toBe(false)); + + const callArgs = (catalogApiMock.getEntities as jest.Mock).mock + .calls[0][0]; + + expect( + callArgs.filter[0]['relations.ownedBy'].length, + ).toBeLessThanOrEqual(100); + + const owners = callArgs.filter[0]['relations.ownedBy']; + + expect(Array.isArray(owners)).toBeTruthy(); + expect(owners.length).toBeLessThanOrEqual(100); + }); + }); + + describe('useGetEntities exceeding default 16384 bytes header size', () => { + const largeNumberOfGroups = Array.from({ length: 600 }, (_, i) => + createGroupEntityFromName(`very-long-group-name-${i + 1}`), + ); + + const givenUserWithLargeNumberOfGroups = { + kind: 'User', + metadata: { + name: givenUser, + }, + spec: { + memberOf: largeNumberOfGroups.map( + group => `group:default/${group.metadata.name}`, + ), + }, + } as Partial as Entity; + + beforeEach(() => { + getEntityRelationsMock.mockImplementation(entity => + entity?.kind === 'User' + ? largeNumberOfGroups.map(group => + createGroupRefFromName(group.metadata.name), + ) + : [], + ); + (catalogApiMock.getEntities as jest.Mock).mockClear(); + }); + + it('should batch the request to avoid exceeding header size limits', async () => { + const { result } = renderHook( + ({ entity }) => useGetEntities(entity, 'aggregated'), + { + initialProps: { entity: givenUserWithLargeNumberOfGroups }, + }, + ); + + await waitFor(() => expect(result.current.loading).toBe(false)); + const callArgs = (catalogApiMock.getEntities as jest.Mock).mock + .calls[0][0]; + + const url = new URL( + `http://localhost/api/catalog/entities?${new URLSearchParams( + callArgs as any, + )}`, + ); + const headerSize = url.href.length; + expect(headerSize).toBeLessThanOrEqual(16384); + + const owners = callArgs.filter[0]['relations.ownedBy']; + expect(Array.isArray(owners)).toBeTruthy(); + expect(owners.length).toBeLessThanOrEqual(100); + }); + }); }); function createGroupEntityFromName(name: string): Entity { diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts index 5a708bcfee..d919e1d7c5 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts @@ -143,26 +143,38 @@ const getOwners = async ( return [stringifyEntityRef(entity)]; }; -const getOwnedEntitiesByOwners = ( +const batchGetOwnedEntitiesByOwners = async ( owners: string[], kinds: string[], catalogApi: CatalogApi, -) => - catalogApi.getEntities({ - filter: [ - { - kind: kinds, - 'relations.ownedBy': owners, - }, - ], - fields: [ - 'kind', - 'metadata.name', - 'metadata.namespace', - 'spec.type', - 'relations', - ], - }); + batchSize: number = 100, +) => { + const batches = []; + + for (let i = 0; i < owners.length; i += batchSize) { + const batch = owners.slice(i, i + batchSize); + batches.push( + catalogApi.getEntities({ + filter: [ + { + kind: kinds, + 'relations.ownedBy': batch, + }, + ], + fields: [ + 'kind', + 'metadata.name', + 'metadata.namespace', + 'spec.type', + 'relations', + ], + }), + ); + } + + const results = await Promise.all(batches); + return results.flatMap(result => result.items); +}; export function useGetEntities( entity: Entity, @@ -191,13 +203,13 @@ export function useGetEntities( } = useAsync(async () => { const owners = await getOwners(entity, relations, catalogApi); - const ownedEntitiesList = await getOwnedEntitiesByOwners( + const ownedEntitiesList = await batchGetOwnedEntitiesByOwners( owners, kinds, catalogApi, ); - const counts = ownedEntitiesList.items.reduce( + const counts = ownedEntitiesList.reduce( (acc: EntityTypeProps[], ownedEntity) => { const match = acc.find( x => x.kind === ownedEntity.kind && x.type === ownedEntity.spec?.type, From 9a85f1cdaf0fb28e65bdf2958276e528ccad4af5 Mon Sep 17 00:00:00 2001 From: Beth Griggs Date: Mon, 1 Jul 2024 15:41:24 +0100 Subject: [PATCH 2/2] chore: add delay to useGetEntities batches and dedupe Signed-off-by: Beth Griggs --- .../OwnershipCard/useGetEntities.test.ts | 8 +++- .../Cards/OwnershipCard/useGetEntities.ts | 46 ++++++++++--------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index 2c223c3f4f..207b01099e 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -266,7 +266,9 @@ describe('useGetEntities', () => { }, ); - await waitFor(() => expect(result.current.loading).toBe(false)); + await waitFor(() => expect(result.current.loading).toBe(false), { + timeout: 5000, + }); const callArgs = (catalogApiMock.getEntities as jest.Mock).mock .calls[0][0]; @@ -318,7 +320,9 @@ describe('useGetEntities', () => { }, ); - await waitFor(() => expect(result.current.loading).toBe(false)); + await waitFor(() => expect(result.current.loading).toBe(false), { + timeout: 5000, + }); const callArgs = (catalogApiMock.getEntities as jest.Mock).mock .calls[0][0]; diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts index d919e1d7c5..27fe3019e2 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts @@ -32,7 +32,7 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsync from 'react-use/esm/useAsync'; import qs from 'qs'; import { EntityRelationAggregation } from '../types'; -import { uniq } from 'lodash'; +import { uniq, uniqBy } from 'lodash'; const limiter = limiterFactory(5); @@ -143,37 +143,41 @@ const getOwners = async ( return [stringifyEntityRef(entity)]; }; +const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); + const batchGetOwnedEntitiesByOwners = async ( owners: string[], kinds: string[], catalogApi: CatalogApi, batchSize: number = 100, + delayMs: number = 100, ) => { - const batches = []; + const results = []; for (let i = 0; i < owners.length; i += batchSize) { const batch = owners.slice(i, i + batchSize); - batches.push( - catalogApi.getEntities({ - filter: [ - { - kind: kinds, - 'relations.ownedBy': batch, - }, - ], - fields: [ - 'kind', - 'metadata.name', - 'metadata.namespace', - 'spec.type', - 'relations', - ], - }), - ); + const response = await catalogApi.getEntities({ + filter: [ + { + kind: kinds, + 'relations.ownedBy': batch, + }, + ], + fields: [ + 'kind', + 'metadata.name', + 'metadata.namespace', + 'spec.type', + 'relations', + ], + }); + + results.push(...response.items); + + if (i + batchSize < owners.length) await delay(delayMs); } - const results = await Promise.all(batches); - return results.flatMap(result => result.items); + return uniqBy(results, stringifyEntityRef); }; export function useGetEntities(