Merge pull request #31004 from drodil/msgraph_parent_group_order

fix(catalog): msgraph to use only first parent for groups

.changeset/loose-memes-bathe.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend-module-msgraph': patch
+---
+
+Ensure that msgraph parent group stays same in case the group has multiple parents

plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts

@@ -1102,6 +1102,14 @@ describe('read microsoft graph', () => {
           name: 'c',
         },
       });
+      const groupD = group({
+        metadata: {
+          annotations: {
+            'graph.microsoft.com/group-id': 'group-id-d',
+          },
+          name: 'd',
+        },
+      });
       const user1 = user({
         metadata: {
           annotations: {
@@ -1118,16 +1126,17 @@ describe('read microsoft graph', () => {
           name: 'user2',
         },
       });
-      const groups = [rootGroup, groupA, groupB, groupC];
+      const groups = [rootGroup, groupA, groupB, groupC, groupD];
       const users = [user1, user2];
       const groupMember = new Map<string, Set<string>>();
       groupMember.set('group-id-b', new Set(['group-id-c']));
+      groupMember.set('group-id-d', new Set(['group-id-c']));
       const groupMemberOf = new Map<string, Set<string>>();
       groupMemberOf.set('user-id-1', new Set(['group-id-a']));
       groupMemberOf.set('user-id-2', new Set(['group-id-c']));
 
       // We have a root groups
-      // We have three groups: a, b, c. c is child of b
+      // We have three groups: a, b, c. c is child of b and d, b should be picked as it's first
       // we have two users: u1, u2. u1 is member of a, u2 is member of c
       resolveRelations(rootGroup, groups, users, groupMember, groupMemberOf);
 
@@ -1147,6 +1156,11 @@ describe('read microsoft graph', () => {
       expect(groupC.spec.parent).toEqual('group:default/b');
       expect(groupC.spec.children).toEqual(expect.arrayContaining([]));
 
+      expect(groupD.spec.parent).toEqual('group:default/root');
+      expect(groupD.spec.children).toEqual(
+        expect.arrayContaining(['group:default/c']),
+      );
+
       expect(user1.spec.memberOf).toEqual(
         expect.arrayContaining(['group:default/a']),
       );

plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts

@@ -359,7 +359,14 @@ export function resolveRelations(
       }
     });
 
+    // TODO: Until we have better support for multiple parents in the model,
+    //       the order of the parents is important as changing it causes
+    //       unnecessary entity stitching randomly.
     retrieveItems(parentGroups, id).forEach(p => {
+      // Only set the parent if it doesn't exist yet
+      if (group.spec.parent) {
+        return;
+      }
       const parentGroup = groupMap.get(p);
       if (parentGroup) {
         // TODO: Only having a single parent group might not match every companies model, but fine for now.
@@ -546,5 +553,5 @@ function retrieveItems(
   target: Map<string, Set<string>>,
   key: string,
 ): Set<string> {
-  return target.get(key) ?? new Set();
+  return new Set([...(target.get(key) ?? [])].sort());
 }