fix(catalog): msgraph to use only first parent for groups
the order of the groups from msgraph is random, thus the parent group for groups having multiple parents can be random. this leads to situation where the grpup parent is changing over the msgraph provider runs leading to unnecessary stitching. other possibility for the fix would be to change the model to allow multiple parents for groups (like mentioned in the TODO) but that could have some weird consequences in apps. Signed-off-by: Hellgren Heikki <heikki.hellgren@op.fi>
This commit is contained in:
@@ -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
|
||||
@@ -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']),
|
||||
);
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user