fix(#20232): handle circular relation

Signed-off-by: vdizengremel <victor.dizengremel@octo.com>
2023-10-02 14:09:27 +02:00
parent 88fa5516c3
commit ca3ab66bbe
2 changed files with 88 additions and 62 deletions
@@ -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<Entity> as Entity;
 }
+
+function createGroupRefFromName(name: string): CompoundEntityRef {
+  return {
+    kind: 'Group',
+    namespace: 'default',
+    name: name,
+  };
+}
@@ -81,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',
@@ -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 (