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/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 191bd10284..587dc689e3 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,49 +31,35 @@ 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', () => ({ 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[]) => expect.objectContaining({ filter: expect.arrayContaining([ expect.objectContaining({ - 'relations.ownedBy': owners, + 'relations.ownedBy': expect.arrayContaining(owners), }), ]), }); @@ -100,24 +76,132 @@ 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}`, - ), + beforeEach(() => { + getEntitiesByRefsMock.mockImplementation(async ({ entityRefs: [ref] }) => + ref.includes(givenParentGroup) + ? { items: [givenParentGroupEntity] } + : { items: [givenLeafGroupEntity] }, ); }); - 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}`, - ), - ); + afterEach(() => { + getEntityRelationsMock.mockRestore(); + getEntitiesByRefsMock.mockRestore(); + }); + + describe('when given entity is a group', () => { + beforeEach(() => { + getEntityRelationsMock + .mockReturnValueOnce([createGroupRefFromName(givenLeafGroup)]) + .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'], + }); + }); + + describe('when relations are deep (children of children)', () => { + const givenIntermediateGroup = 'intermediate-group'; + const givenIntermediateGroupEntity = createGroupEntityFromName( + givenIntermediateGroup, + ); + + beforeEach(() => { + getEntitiesByRefsMock.mockRestore(); + getEntitiesByRefsMock.mockImplementation( + async ({ entityRefs: [ref] }) => { + if (ref.includes(givenParentGroup)) { + return { items: [givenParentGroupEntity] }; + } + + if (ref.includes(givenIntermediateGroup)) { + return { items: [givenIntermediateGroupEntity] }; + } + + return { items: [givenLeafGroupEntity] }; + }, + ); + }); + + 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([ + createGroupRefFromName(givenLeafGroup), + ]); + + await whenHookIsCalledWith(givenUserEntity); + expect(catalogApiMock.getEntities).toHaveBeenCalledWith( + ownersFilter( + `group:default/${givenLeafGroup}`, + `user:default/${givenUser}`, + ), + ); + }); }); }); @@ -148,3 +232,20 @@ describe('useGetEntities', () => { }); }); }); + +function createGroupEntityFromName(name: string): Entity { + return { + kind: 'Group', + metadata: { + name: name, + }, + } 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 1ff01cd8e1..b6fa9cbfd1 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); @@ -80,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', @@ -87,26 +89,38 @@ 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({ - fields: ['kind', 'metadata.namespace', 'metadata.name'], + fields: ['kind', 'metadata.namespace', 'metadata.name', 'relations'], entityRefs, }); const childGroupEntities = childGroupResponse.items.filter(isEntity); - return ( + 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, entityRef]); } - return [stringifyEntityRef(entity)]; + return [entityRef]; }; const getOwners = async ( @@ -118,23 +132,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 9a356d0351..de2a8d5ea7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8148,6 +8148,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