From dc5b6b971badbf8135b6f1ff288999d710edfc92 Mon Sep 17 00:00:00 2001 From: vdizengremel Date: Fri, 29 Sep 2023 13:10:02 +0200 Subject: [PATCH 1/4] fix(#20232): load relations to get children of children and fix the display of OwnershipCard with aggregated relations. Signed-off-by: vdizengremel --- .changeset/cold-bees-jam.md | 6 ++++++ .../components/Cards/OwnershipCard/useGetEntities.test.ts | 8 ++++++++ .../src/components/Cards/OwnershipCard/useGetEntities.ts | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 .changeset/cold-bees-jam.md diff --git a/.changeset/cold-bees-jam.md b/.changeset/cold-bees-jam.md new file mode 100644 index 0000000000..3c6e652d2b --- /dev/null +++ b/.changeset/cold-bees-jam.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-org': patch +--- + +Fixed the display of OwnershipCard with aggregated relations by loading relations when getting children of entity. +This allows the already existing recursive method to work properly when children of entity have children themselves. diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index 191bd10284..4b9389a7d4 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -110,6 +110,14 @@ describe('useGetEntities', () => { ); }); + it('given group entity should retrieve child with their relations', async () => { + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntitiesByRefs).toHaveBeenCalledWith({ + entityRefs: [`group:default/${givenLeafGroup}`], + fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], + }); + }); + it('given user entity should aggregate parent ownership and direct', async () => { await whenHookIsCalledWith(givenUserEntity); expect(catalogApiMock.getEntities).toHaveBeenCalledWith( diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts index 1ff01cd8e1..337e926971 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts @@ -90,7 +90,7 @@ const getChildOwnershipEntityRefs = async ( if (hasChildGroups) { const entityRefs = childGroups.map(r => stringifyEntityRef(r)); const childGroupResponse = await catalogApi.getEntitiesByRefs({ - fields: ['kind', 'metadata.namespace', 'metadata.name'], + fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], entityRefs, }); const childGroupEntities = childGroupResponse.items.filter(isEntity); From ad97403fcac6a2dbdf419f07ba15196ce79bdde7 Mon Sep 17 00:00:00 2001 From: vdizengremel Date: Mon, 2 Oct 2023 11:24:16 +0200 Subject: [PATCH 2/4] fix(#20232): refacto test to facilitate maintenance and the add of new cases Signed-off-by: vdizengremel --- .../OwnershipCard/useGetEntities.test.ts | 105 ++++++++++-------- 1 file changed, 61 insertions(+), 44 deletions(-) diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index 4b9389a7d4..27a6ef96ba 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -53,30 +53,19 @@ const catalogApiMock: Pick = { jest.mock('@backstage/core-plugin-api', () => ({ useApi: jest.fn(() => catalogApiMock), })); -jest.mock('@backstage/plugin-catalog-react', () => ({ - catalogApiRef: {}, - getEntityRelations: jest.fn(entity => { - if (entity?.metadata.name === givenParentGroup) { - return [ - { - kind: 'Group', - namespace: 'default', - name: givenLeafGroup, - } as CompoundEntityRef, - ]; - } else if (entity?.kind === 'User') { - return [ - { - kind: 'Group', - namespace: 'default', - name: givenLeafGroup, - } as CompoundEntityRef, - ]; - } - return []; - }) as typeof getEntityRelations, -})); +const getEntityRelationsMock: jest.Mock< + CompoundEntityRef[], + [Entity | undefined] +> = jest.fn(); +jest.mock('@backstage/plugin-catalog-react', () => { + return { + catalogApiRef: {}, + getEntityRelations: jest.fn(entity => { + return getEntityRelationsMock(entity); + }) as typeof getEntityRelations, + }; +}); describe('useGetEntities', () => { const ownersFilter = (...owners: string[]) => @@ -100,32 +89,60 @@ describe('useGetEntities', () => { await hook.waitForNextUpdate(); }; - it('given group entity should aggregate child ownership', async () => { - await whenHookIsCalledWith(givenParentGroupEntity); - expect(catalogApiMock.getEntities).toHaveBeenCalledWith( - ownersFilter( - `group:default/${givenParentGroup}`, - `group:default/${givenLeafGroup}`, - ), - ); + afterEach(() => { + getEntityRelationsMock.mockRestore(); }); - it('given group entity should retrieve child with their relations', async () => { - await whenHookIsCalledWith(givenParentGroupEntity); - expect(catalogApiMock.getEntitiesByRefs).toHaveBeenCalledWith({ - entityRefs: [`group:default/${givenLeafGroup}`], - fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], + describe('when given entity is a group', () => { + beforeEach(() => { + getEntityRelationsMock + .mockReturnValueOnce([ + { + kind: 'Group', + namespace: 'default', + name: givenLeafGroup, + } as CompoundEntityRef, + ]) + .mockReturnValue([]); + }); + + it('should aggregate child ownership', async () => { + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenParentGroup}`, + `group:default/${givenLeafGroup}`, + ), + ); + }); + + it('should retrieve child with their relations', async () => { + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntitiesByRefs).toHaveBeenCalledWith({ + entityRefs: [`group:default/${givenLeafGroup}`], + fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], + }); }); }); - it('given user entity should aggregate parent ownership and direct', async () => { - await whenHookIsCalledWith(givenUserEntity); - expect(catalogApiMock.getEntities).toHaveBeenCalledWith( - ownersFilter( - `group:default/${givenLeafGroup}`, - `user:default/${givenUser}`, - ), - ); + describe('when given entity is a user', () => { + it('should aggregate parent ownership and direct', async () => { + getEntityRelationsMock.mockReturnValue([ + { + kind: 'Group', + namespace: 'default', + name: givenLeafGroup, + } as CompoundEntityRef, + ]); + + await whenHookIsCalledWith(givenUserEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenLeafGroup}`, + `user:default/${givenUser}`, + ), + ); + }); }); }); From 88fa5516c30bbaaf73f2befad6edb105ec748161 Mon Sep 17 00:00:00 2001 From: vdizengremel Date: Mon, 2 Oct 2023 12:22:10 +0200 Subject: [PATCH 3/4] fix(#20232): handle intermediate groups Signed-off-by: vdizengremel --- plugins/org/package.json | 1 + .../OwnershipCard/useGetEntities.test.ts | 97 +++++++++++++++---- .../Cards/OwnershipCard/useGetEntities.ts | 27 +++--- yarn.lock | 1 + 4 files changed, 92 insertions(+), 34 deletions(-) diff --git a/plugins/org/package.json b/plugins/org/package.json index 7d931c4797..c68352b579 100644 --- a/plugins/org/package.json +++ b/plugins/org/package.json @@ -39,6 +39,7 @@ "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.61", "@types/react": "^16.13.1 || ^17.0.0", + "lodash": "^4.17.21", "p-limit": "^3.1.0", "pluralize": "^8.0.0", "qs": "^6.10.1", diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index 27a6ef96ba..189aa4db30 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -22,18 +22,8 @@ import { getEntityRelations } from '@backstage/plugin-catalog-react'; const givenParentGroup = 'team.squad1'; const givenLeafGroup = 'team.squad2'; const givenUser = 'user.john'; -const givenParentGroupEntity = { - kind: 'Group', - metadata: { - name: givenParentGroup, - }, -} as Partial as Entity; -const givenLeafGroupEntity = { - kind: 'Group', - metadata: { - name: givenLeafGroup, - }, -} as Partial as Entity; +const givenParentGroupEntity = createGroupEntityFromName(givenParentGroup); +const givenLeafGroupEntity = createGroupEntityFromName(givenLeafGroup); const givenUserEntity = { kind: 'User', metadata: { @@ -41,13 +31,10 @@ const givenUserEntity = { }, } as Partial as Entity; +const getEntitiesByRefsMock = jest.fn(); const catalogApiMock: Pick = { getEntities: jest.fn(async () => Promise.resolve({ items: [] })), - getEntitiesByRefs: jest.fn(async ({ entityRefs: [ref] }) => - ref.includes(givenParentGroup) - ? { items: [givenParentGroupEntity] } - : { items: [givenLeafGroupEntity] }, - ), + getEntitiesByRefs: getEntitiesByRefsMock, }; jest.mock('@backstage/core-plugin-api', () => ({ @@ -72,7 +59,7 @@ describe('useGetEntities', () => { expect.objectContaining({ filter: expect.arrayContaining([ expect.objectContaining({ - 'relations.ownedBy': owners, + 'relations.ownedBy': expect.arrayContaining(owners), }), ]), }); @@ -89,8 +76,17 @@ describe('useGetEntities', () => { await hook.waitForNextUpdate(); }; + beforeEach(() => { + getEntitiesByRefsMock.mockImplementation(async ({ entityRefs: [ref] }) => + ref.includes(givenParentGroup) + ? { items: [givenParentGroupEntity] } + : { items: [givenLeafGroupEntity] }, + ); + }); + afterEach(() => { getEntityRelationsMock.mockRestore(); + getEntitiesByRefsMock.mockRestore(); }); describe('when given entity is a group', () => { @@ -123,6 +119,62 @@ describe('useGetEntities', () => { fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], }); }); + + it('should retrieve entities owned by any children', async () => { + const givenIntermediateGroup = 'intermediate-group'; + const givenIntermediateGroupEntity = createGroupEntityFromName( + givenIntermediateGroup, + ); + + getEntitiesByRefsMock.mockRestore(); + getEntitiesByRefsMock.mockImplementation( + async ({ entityRefs: [ref] }) => { + if (ref.includes(givenParentGroup)) { + return { items: [givenParentGroupEntity] }; + } + + if (ref.includes(givenIntermediateGroup)) { + return { items: [givenIntermediateGroupEntity] }; + } + + return { items: [givenLeafGroupEntity] }; + }, + ); + + getEntityRelationsMock.mockRestore(); + getEntityRelationsMock.mockImplementation(entity => { + if (entity?.metadata.name === givenParentGroup) { + return [ + { + kind: 'Group', + namespace: 'default', + name: givenIntermediateGroup, + }, + ]; + } + + if (entity?.metadata.name === givenIntermediateGroup) { + return [ + { + kind: 'Group', + namespace: 'default', + name: givenLeafGroup, + }, + ]; + } + + return []; + }); + + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenParentGroup}`, + `group:default/${givenIntermediateGroup}`, + `group:default/${givenLeafGroup}`, + ), + ); + }); }); describe('when given entity is a user', () => { @@ -173,3 +225,12 @@ describe('useGetEntities', () => { }); }); }); + +function createGroupEntityFromName(name: String): Entity { + return { + kind: 'Group', + metadata: { + name: name, + }, + } as Partial as Entity; +} diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts index 337e926971..cc2bb78a23 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts @@ -16,9 +16,9 @@ import { Entity, + parseEntityRef, RELATION_MEMBER_OF, RELATION_PARENT_OF, - parseEntityRef, stringifyEntityRef, } from '@backstage/catalog-model'; import { @@ -32,6 +32,7 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsync from 'react-use/lib/useAsync'; import qs from 'qs'; import { EntityRelationAggregation as EntityRelationsAggregation } from './types'; +import { uniq } from 'lodash'; const limiter = limiterFactory(10); @@ -95,7 +96,7 @@ const getChildOwnershipEntityRefs = async ( }); const childGroupEntities = childGroupResponse.items.filter(isEntity); - return ( + const childrenRefs = ( await Promise.all( childGroupEntities.map(childGroupEntity => limiter(() => @@ -104,6 +105,8 @@ const getChildOwnershipEntityRefs = async ( ), ) ).flatMap(aggregated => aggregated); + + return uniq([...childrenRefs, stringifyEntityRef(entity)]); } return [stringifyEntityRef(entity)]; @@ -118,23 +121,15 @@ const getOwners = async ( const isAggregated = relations === 'aggregated'; const isUserEntity = entity.kind === 'User'; - const owners: string[] = []; - if (isAggregated && isGroup) { - const childEntityRefs = await getChildOwnershipEntityRefs( - entity, - catalogApi, - ); - owners.push(stringifyEntityRef(entity)); - owners.push.apply(owners, childEntityRefs); - } else if (isAggregated && isUserEntity) { - const parentEntityRefs = getMemberOfEntityRefs(entity); - owners.push.apply(owners, parentEntityRefs); - } else { - owners.push(stringifyEntityRef(entity)); + return getChildOwnershipEntityRefs(entity, catalogApi); } - return owners; + if (isAggregated && isUserEntity) { + return getMemberOfEntityRefs(entity); + } + + return [stringifyEntityRef(entity)]; }; const getOwnedEntitiesByOwners = ( diff --git a/yarn.lock b/yarn.lock index 02c4184f9c..a4c817ce7f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8139,6 +8139,7 @@ __metadata: "@testing-library/react-hooks": ^8.0.1 "@testing-library/user-event": ^14.0.0 "@types/react": ^16.13.1 || ^17.0.0 + lodash: ^4.17.21 msw: ^1.0.0 p-limit: ^3.1.0 pluralize: ^8.0.0 From ca3ab66bbeb93ff2cd39bea8b99f2260b916316f Mon Sep 17 00:00:00 2001 From: vdizengremel Date: Mon, 2 Oct 2023 14:09:27 +0200 Subject: [PATCH 4/4] fix(#20232): handle circular relation Signed-off-by: vdizengremel --- .../OwnershipCard/useGetEntities.test.ts | 131 ++++++++++-------- .../Cards/OwnershipCard/useGetEntities.ts | 19 ++- 2 files changed, 88 insertions(+), 62 deletions(-) diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts index 189aa4db30..587dc689e3 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.test.ts @@ -92,13 +92,7 @@ describe('useGetEntities', () => { describe('when given entity is a group', () => { beforeEach(() => { getEntityRelationsMock - .mockReturnValueOnce([ - { - kind: 'Group', - namespace: 'default', - name: givenLeafGroup, - } as CompoundEntityRef, - ]) + .mockReturnValueOnce([createGroupRefFromName(givenLeafGroup)]) .mockReturnValue([]); }); @@ -120,71 +114,84 @@ describe('useGetEntities', () => { }); }); - it('should retrieve entities owned by any children', async () => { + describe('when relations are deep (children of children)', () => { const givenIntermediateGroup = 'intermediate-group'; const givenIntermediateGroupEntity = createGroupEntityFromName( givenIntermediateGroup, ); - getEntitiesByRefsMock.mockRestore(); - getEntitiesByRefsMock.mockImplementation( - async ({ entityRefs: [ref] }) => { - if (ref.includes(givenParentGroup)) { - return { items: [givenParentGroupEntity] }; - } + beforeEach(() => { + getEntitiesByRefsMock.mockRestore(); + getEntitiesByRefsMock.mockImplementation( + async ({ entityRefs: [ref] }) => { + if (ref.includes(givenParentGroup)) { + return { items: [givenParentGroupEntity] }; + } - if (ref.includes(givenIntermediateGroup)) { - return { items: [givenIntermediateGroupEntity] }; - } + if (ref.includes(givenIntermediateGroup)) { + return { items: [givenIntermediateGroupEntity] }; + } - return { items: [givenLeafGroupEntity] }; - }, - ); - - getEntityRelationsMock.mockRestore(); - getEntityRelationsMock.mockImplementation(entity => { - if (entity?.metadata.name === givenParentGroup) { - return [ - { - kind: 'Group', - namespace: 'default', - name: givenIntermediateGroup, - }, - ]; - } - - if (entity?.metadata.name === givenIntermediateGroup) { - return [ - { - kind: 'Group', - namespace: 'default', - name: givenLeafGroup, - }, - ]; - } - - return []; + return { items: [givenLeafGroupEntity] }; + }, + ); }); - await whenHookIsCalledWith(givenParentGroupEntity); - expect(catalogApiMock.getEntities).toHaveBeenCalledWith( - ownersFilter( - `group:default/${givenParentGroup}`, - `group:default/${givenIntermediateGroup}`, - `group:default/${givenLeafGroup}`, - ), - ); + it('should retrieve entities owned by any children', async () => { + getEntityRelationsMock.mockRestore(); + getEntityRelationsMock.mockImplementation(entity => { + if (entity?.metadata.name === givenParentGroup) { + return [createGroupRefFromName(givenIntermediateGroup)]; + } + + if (entity?.metadata.name === givenIntermediateGroup) { + return [createGroupRefFromName(givenLeafGroup)]; + } + + return []; + }); + + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenParentGroup}`, + `group:default/${givenIntermediateGroup}`, + `group:default/${givenLeafGroup}`, + ), + ); + }); + + it('should retrieve entities owned by any children when circular relation', async () => { + getEntityRelationsMock.mockRestore(); + getEntityRelationsMock.mockImplementation(entity => { + if (entity?.metadata.name === givenParentGroup) { + return [createGroupRefFromName(givenIntermediateGroup)]; + } + + if (entity?.metadata.name === givenIntermediateGroup) { + return [createGroupRefFromName(givenLeafGroup)]; + } + + // returns parent by default so givenLeafGroup will have the givenParentGroup as child + return [createGroupRefFromName(givenParentGroup)]; + }); + + await whenHookIsCalledWith(givenParentGroupEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenParentGroup}`, + `group:default/${givenIntermediateGroup}`, + `group:default/${givenLeafGroup}`, + ), + ); + }); }); }); describe('when given entity is a user', () => { it('should aggregate parent ownership and direct', async () => { getEntityRelationsMock.mockReturnValue([ - { - kind: 'Group', - namespace: 'default', - name: givenLeafGroup, - } as CompoundEntityRef, + createGroupRefFromName(givenLeafGroup), ]); await whenHookIsCalledWith(givenUserEntity); @@ -226,7 +233,7 @@ describe('useGetEntities', () => { }); }); -function createGroupEntityFromName(name: String): Entity { +function createGroupEntityFromName(name: string): Entity { return { kind: 'Group', metadata: { @@ -234,3 +241,11 @@ function createGroupEntityFromName(name: String): Entity { }, } as Partial as Entity; } + +function createGroupRefFromName(name: string): CompoundEntityRef { + return { + kind: 'Group', + namespace: 'default', + name: name, + }; +} diff --git a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts index cc2bb78a23..b6fa9cbfd1 100644 --- a/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts +++ b/plugins/org/src/components/Cards/OwnershipCard/useGetEntities.ts @@ -81,6 +81,7 @@ const isEntity = (entity: Entity | undefined): entity is Entity => const getChildOwnershipEntityRefs = async ( entity: Entity, catalogApi: CatalogApi, + alreadyRetrievedParentRefs: string[] = [], ): Promise => { const childGroups = getEntityRelations(entity, RELATION_PARENT_OF, { kind: 'Group', @@ -88,6 +89,7 @@ const getChildOwnershipEntityRefs = async ( const hasChildGroups = childGroups.length > 0; + const entityRef = stringifyEntityRef(entity); if (hasChildGroups) { const entityRefs = childGroups.map(r => stringifyEntityRef(r)); const childGroupResponse = await catalogApi.getEntitiesByRefs({ @@ -96,20 +98,29 @@ const getChildOwnershipEntityRefs = async ( }); const childGroupEntities = childGroupResponse.items.filter(isEntity); + const unknownChildren = childGroupEntities.filter( + childGroupEntity => + !alreadyRetrievedParentRefs.includes( + stringifyEntityRef(childGroupEntity), + ), + ); const childrenRefs = ( await Promise.all( - childGroupEntities.map(childGroupEntity => + unknownChildren.map(childGroupEntity => limiter(() => - getChildOwnershipEntityRefs(childGroupEntity, catalogApi), + getChildOwnershipEntityRefs(childGroupEntity, catalogApi, [ + ...alreadyRetrievedParentRefs, + entityRef, + ]), ), ), ) ).flatMap(aggregated => aggregated); - return uniq([...childrenRefs, stringifyEntityRef(entity)]); + return uniq([...childrenRefs, entityRef]); } - return [stringifyEntityRef(entity)]; + return [entityRef]; }; const getOwners = async (