Merge pull request #20267 from vdizengremel/fix/deep-nested-relations-ownership-card

fix(#20232): load relations to get children of children

.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.

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",

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<Entity> as Entity;
-const givenLeafGroupEntity = {
-  kind: 'Group',
-  metadata: {
-    name: givenLeafGroup,
-  },
-} as Partial<Entity> as Entity;
+const givenParentGroupEntity = createGroupEntityFromName(givenParentGroup);
+const givenLeafGroupEntity = createGroupEntityFromName(givenLeafGroup);
 const givenUserEntity = {
   kind: 'User',
   metadata: {
@@ -41,49 +31,35 @@ const givenUserEntity = {
   },
 } as Partial<Entity> as Entity;
 
+const getEntitiesByRefsMock = jest.fn();
 const catalogApiMock: Pick<CatalogApi, 'getEntities' | 'getEntitiesByRefs'> = {
   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<Entity> as Entity;
+}
+
+function createGroupRefFromName(name: string): CompoundEntityRef {
+  return {
+    kind: 'Group',
+    namespace: 'default',
+    name: name,
+  };
+}

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<string[]> => {
   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 = (

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