From 58d9145eef563e72685bbc5fa04fd5c64de5c6f6 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Sun, 22 Jan 2023 21:05:12 +0000 Subject: [PATCH 1/4] Added page size support to MSGraph client Signed-off-by: Alex Crome --- .../src/microsoftGraph/client.test.ts | 5 +++-- .../src/microsoftGraph/client.ts | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts index 05693149d8..783c16fe6f 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts @@ -70,7 +70,7 @@ describe('MicrosoftGraphClient', () => { expect(await response.json()).toEqual({ value: 'example' }); }); - it('should perform api request with filter, select and expand', async () => { + it('should perform api request with filter, select, expand and top', async () => { worker.use( rest.get('https://example.com/users', (req, res, ctx) => res(ctx.status(200), ctx.json({ queryString: req.url.search })), @@ -81,12 +81,13 @@ describe('MicrosoftGraphClient', () => { filter: 'test eq true', expand: 'children', select: ['id', 'children'], + top: 471, }); expect(response.status).toBe(200); expect(await response.json()).toEqual({ queryString: - '?$filter=test%20eq%20true&$select=id,children&$expand=children', + '?$filter=test%20eq%20true&$select=id,children&$expand=children&$top=471', }); }); diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts index 26da0cb64b..a1fc77b4fd 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts @@ -52,6 +52,10 @@ export type ODataQuery = { * Retrieves the total count of matching resources. */ count?: boolean; + /** + * Maximum number of records to receive in one batch. + */ + top?: number; }; /** @@ -188,6 +192,7 @@ export class MicrosoftGraphClient { $select: query?.select?.join(','), $expand: query?.expand, $count: query?.count, + $top: query?.top, }, { addQueryPrefix: true, From c5b119ad9cbb1d35e5357a6319237320cda0637d Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Sun, 22 Jan 2023 21:56:39 +0000 Subject: [PATCH 2/4] Updated MS Graph plugin to use larger page sizes Signed-off-by: Alex Crome --- .changeset/young-points-drop.md | 5 +++ .../src/microsoftGraph/client.ts | 12 ++++- .../src/microsoftGraph/read.test.ts | 45 ++++++++++++++++--- .../src/microsoftGraph/read.ts | 13 +++++- 4 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 .changeset/young-points-drop.md diff --git a/.changeset/young-points-drop.md b/.changeset/young-points-drop.md new file mode 100644 index 0000000000..4309d54c2c --- /dev/null +++ b/.changeset/young-points-drop.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-msgraph': patch +--- + +Increased default page size to 999 (from 100) to reduce the number of calls made to the Microsoft Graph API. diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts index a1fc77b4fd..fd640e70ba 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts @@ -348,8 +348,16 @@ export class MicrosoftGraphClient { * @param groupId - The unique identifier for the `Group` resource * */ - async *getGroupMembers(groupId: string): AsyncIterable { - yield* this.requestCollection(`groups/${groupId}/members`); + async *getGroupMembers( + groupId: string, + query?: ODataQuery, + queryMode?: 'basic' | 'advanced', + ): AsyncIterable { + yield* this.requestCollection( + `groups/${groupId}/members`, + query, + queryMode, + ); } /** diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index bafb2ecaad..0f9ddb0c13 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -140,6 +140,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + top: 999, }, undefined, ); @@ -186,6 +187,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: 'accountEnabled eq true', + top: 999, }, 'advanced', ); @@ -228,6 +230,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + top: 999, }, undefined, ); @@ -278,11 +281,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq true', + top: 999, }, undefined, ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); expect(client.getUserProfile).toHaveBeenCalledTimes(1); expect(client.getUserProfile).toHaveBeenCalledWith('userid', { @@ -338,11 +344,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq true', + top: 999, }, 'advanced', ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); expect(client.getUserProfile).toHaveBeenCalledTimes(1); expect(client.getUserProfile).toHaveBeenCalledWith('userid', { @@ -390,11 +399,14 @@ describe('read microsoft graph', () => { { expand: 'member', filter: 'securityEnabled eq true', + top: 999, }, undefined, ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); expect(client.getUserProfile).toHaveBeenCalledTimes(1); expect(client.getUserProfile).toHaveBeenCalledWith('userid', { @@ -527,11 +539,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq false', + top: 999, }, undefined, ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); // TODO: Loading groups photos doesn't work right now as Microsoft Graph // doesn't allows this yet // expect(client.getGroupPhotoWithSizeLimit).toBeCalledTimes(1); @@ -603,11 +618,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq false', + top: 999, }, 'advanced', ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); // TODO: Loading groups photos doesn't work right now as Microsoft Graph // doesn't allows this yet // expect(client.getGroupPhotoWithSizeLimit).toBeCalledTimes(1); @@ -680,11 +698,14 @@ describe('read microsoft graph', () => { { expand: 'member', filter: 'securityEnabled eq false', + top: 999, }, undefined, ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); // TODO: Loading groups photos doesn't work right now as Microsoft Graph // doesn't allows this yet // expect(client.getGroupPhotoWithSizeLimit).toBeCalledTimes(1); @@ -750,11 +771,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq true', + top: 999, }, undefined, ); expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid'); + expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { + top: 999, + }); }); }); @@ -887,6 +911,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { filter: undefined, + top: 999, }, undefined, ); @@ -894,6 +919,7 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq false', + top: 999, }, undefined, ); @@ -925,6 +951,7 @@ describe('read microsoft graph', () => { { expand: 'manager', filter: 'accountEnabled eq true', + top: 999, }, undefined, ); @@ -932,6 +959,7 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq false', + top: 999, }, undefined, ); @@ -960,6 +988,7 @@ describe('read microsoft graph', () => { expect(client.getUsers).toHaveBeenCalledWith( { select: ['mail'], + top: 999, }, undefined, ); @@ -994,12 +1023,14 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'name eq backstage-group', + top: 999, }, undefined, ); expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq false', + top: 999, }, undefined, ); diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index 0ed411bfea..2541416777 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -37,6 +37,8 @@ import { UserTransformer, } from './types'; +const PAGE_SIZE = 999; + /** * The default implementation of the transformation from a graph user entry to * a User entity. @@ -107,6 +109,7 @@ export async function readMicrosoftGraphUsers( filter: options.userFilter, expand: options.userExpand, select: options.userSelect, + top: PAGE_SIZE, }, options.queryMode, )) { @@ -172,13 +175,16 @@ export async function readMicrosoftGraphUsersInGroups( expand: options.groupExpand, search: options.userGroupMemberSearch, filter: options.userGroupMemberFilter, + top: PAGE_SIZE, }, options.queryMode, )) { // Process all groups in parallel, otherwise it can take quite some time userGroupMemberPromises.push( limiter(async () => { - for await (const member of client.getGroupMembers(group.id!)) { + for await (const member of client.getGroupMembers(group.id!, { + top: PAGE_SIZE, + })) { if (!member.id) { continue; } @@ -380,6 +386,7 @@ export async function readMicrosoftGraphGroups( search: options?.groupSearch, filter: options?.groupFilter, select: options?.groupSelect, + top: PAGE_SIZE, }, options?.queryMode, )) { @@ -401,7 +408,9 @@ export async function readMicrosoftGraphGroups( return; } - for await (const member of client.getGroupMembers(group.id!)) { + for await (const member of client.getGroupMembers(group.id!, { + top: PAGE_SIZE, + })) { if (!member.id) { continue; } From fb568e2683697a64aef7c9ffe1d140d6ed2eb314 Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Sun, 22 Jan 2023 23:23:26 +0000 Subject: [PATCH 3/4] Optimised loading of users via group membership Signed-off-by: Alex Crome --- .changeset/slow-carrots-sniff.md | 8 + .../api-report.md | 16 +- .../src/microsoftGraph/client.test.ts | 48 ++--- .../src/microsoftGraph/client.ts | 43 ++-- .../src/microsoftGraph/read.test.ts | 78 ++++---- .../src/microsoftGraph/read.ts | 183 +++++++++--------- 6 files changed, 185 insertions(+), 191 deletions(-) create mode 100644 .changeset/slow-carrots-sniff.md diff --git a/.changeset/slow-carrots-sniff.md b/.changeset/slow-carrots-sniff.md new file mode 100644 index 0000000000..d32c2a281e --- /dev/null +++ b/.changeset/slow-carrots-sniff.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-catalog-backend-module-msgraph': minor +--- + +Improve performance when loading users via group membership. +Users data is now loaded from a paged query, rather than having to make an extra call per user to load each user's profiles. + +Note, there are still additional per user calls made to load user avatars diff --git a/plugins/catalog-backend-module-msgraph/api-report.md b/plugins/catalog-backend-module-msgraph/api-report.md index a5abdc17c6..09770614b4 100644 --- a/plugins/catalog-backend-module-msgraph/api-report.md +++ b/plugins/catalog-backend-module-msgraph/api-report.md @@ -70,7 +70,11 @@ export const MICROSOFT_GRAPH_USER_ID_ANNOTATION = 'graph.microsoft.com/user-id'; export class MicrosoftGraphClient { constructor(baseUrl: string, tokenCredential: TokenCredential); static create(config: MicrosoftGraphProviderConfig): MicrosoftGraphClient; - getGroupMembers(groupId: string): AsyncIterable; + getGroupMembers( + groupId: string, + query?: ODataQuery, + queryMode?: 'basic' | 'advanced', + ): AsyncIterable; // (undocumented) getGroupPhoto(groupId: string, sizeId?: string): Promise; getGroupPhotoWithSizeLimit( @@ -81,6 +85,11 @@ export class MicrosoftGraphClient { query?: ODataQuery, queryMode?: 'basic' | 'advanced', ): AsyncIterable; + getGroupUserMembers( + groupId: string, + query?: ODataQuery, + queryMode?: 'basic' | 'advanced', + ): AsyncIterable; getOrganization(tenantId: string): Promise; // (undocumented) getUserPhoto(userId: string, sizeId?: string): Promise; @@ -88,10 +97,6 @@ export class MicrosoftGraphClient { userId: string, maxSize: number, ): Promise; - getUserProfile( - userId: string, - query?: ODataQuery, - ): Promise; getUsers( query?: ODataQuery, queryMode?: 'basic' | 'advanced', @@ -231,6 +236,7 @@ export type ODataQuery = { expand?: string; select?: string[]; count?: boolean; + top?: number; }; // @public diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts index 783c16fe6f..f621b4d924 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.test.ts @@ -135,33 +135,6 @@ describe('MicrosoftGraphClient', () => { expect(values).toEqual(['first', 'second']); }); - it('should load user profile', async () => { - worker.use( - rest.get('https://example.com/users/user-id', (_, res, ctx) => - res( - ctx.status(200), - ctx.json({ - surname: 'Example', - }), - ), - ), - ); - - const userProfile = await client.getUserProfile('user-id'); - - expect(userProfile).toEqual({ surname: 'Example' }); - }); - - it('should throw exception if load user profile fails', async () => { - worker.use( - rest.get('https://example.com/users/user-id', (_, res, ctx) => - res(ctx.status(404)), - ), - ); - - await expect(() => client.getUserProfile('user-id')).rejects.toThrow(); - }); - it('should load user profile photo with max size of 120', async () => { worker.use( rest.get('https://example.com/users/user-id/photos', (_, res, ctx) => @@ -330,6 +303,27 @@ describe('MicrosoftGraphClient', () => { ]); }); + it('should load user group members', async () => { + worker.use( + rest.get( + 'https://example.com/groups/group-id/members/microsoft.graph.user', + (_, res, ctx) => + res( + ctx.status(200), + ctx.json({ + value: [{ id: '12345' }, { id: '67890' }], + }), + ), + ), + ); + + const values = await collectAsyncIterable( + client.getGroupUserMembers('group-id'), + ); + + expect(values).toEqual([{ id: '12345' }, { id: '67890' }]); + }); + it('should load organization', async () => { worker.use( rest.get('https://example.com/organization/tentant-id', (_, res, ctx) => diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts index fd640e70ba..c644e545d6 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/client.ts @@ -234,28 +234,6 @@ export class MicrosoftGraphClient { }); } - /** - * Get {@link https://docs.microsoft.com/en-us/graph/api/resources/user | User} - * from Graph API - * - * @public - * @param userId - The unique identifier for the `User` resource - * @param query - OData Query {@link ODataQuery} - * - */ - async getUserProfile( - userId: string, - query?: ODataQuery, - ): Promise { - const response = await this.requestApi(`users/${userId}`, query); - - if (response.status !== 200) { - await this.handleError('user profile', response); - } - - return await response.json(); - } - /** * Get {@link https://docs.microsoft.com/en-us/graph/api/resources/profilephoto | profilePhoto} * of `User` from Graph API with size limit @@ -360,6 +338,27 @@ export class MicrosoftGraphClient { ); } + /** + * Get a collection of + * {@link https://docs.microsoft.com/en-us/graph/api/resources/user | User} + * belonging to a `Group` from Graph API and return as `AsyncIterable` + * @public + * @param groupId - The unique identifier for the `Group` resource + * @param query - OData Query {@link ODataQuery} + * @param queryMode - Mode to use while querying. Some features are only available at "advanced". + */ + async *getGroupUserMembers( + groupId: string, + query?: ODataQuery, + queryMode?: 'basic' | 'advanced', + ): AsyncIterable { + yield* this.requestCollection( + `groups/${groupId}/members/microsoft.graph.user/`, + query, + queryMode, + ); + } + /** * Get {@link https://docs.microsoft.com/en-us/graph/api/resources/organization | Organization} * from Graph API diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts index 0f9ddb0c13..b1dfc5a148 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.test.ts @@ -61,9 +61,9 @@ function group(data: Partial): GroupEntity { describe('read microsoft graph', () => { const client: jest.Mocked = { getUsers: jest.fn(), - getUserProfile: jest.fn(), getGroups: jest.fn(), getGroupMembers: jest.fn(), + getGroupUserMembers: jest.fn(), getUserPhotoWithSizeLimit: jest.fn(), getGroupPhotoWithSizeLimit: jest.fn(), getOrganization: jest.fn(), @@ -78,13 +78,6 @@ describe('read microsoft graph', () => { mail: 'user.name@example.com', }; } - async function getExampleUserProfile() { - return { - id: 'userid', - displayName: 'User Name', - mail: 'user.name@example.com', - }; - } async function* getExampleGroups() { yield { id: 'groupid', @@ -245,9 +238,8 @@ describe('read microsoft graph', () => { describe('readMicrosoftGraphUsersInGroups', () => { it('should read users from Groups', async () => { client.getGroups.mockImplementation(getExampleGroups); - client.getGroupMembers.mockImplementation(getExampleGroupMembers); + client.getGroupUserMembers.mockImplementation(getExampleUsers); - client.getUserProfile.mockResolvedValue(getExampleUserProfile()); client.getUserPhotoWithSizeLimit.mockResolvedValue( 'data:image/jpeg;base64,...', ); @@ -281,19 +273,19 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq true', + select: ['id', 'displayName'], top: 999, }, undefined, ); - expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { - top: 999, - }); - expect(client.getUserProfile).toHaveBeenCalledTimes(1); - expect(client.getUserProfile).toHaveBeenCalledWith('userid', { - expand: undefined, - }); + expect(client.getGroupUserMembers).toHaveBeenCalledTimes(1); + expect(client.getGroupUserMembers).toHaveBeenCalledWith( + 'groupid', + { top: 999 }, + undefined, + ); + expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledTimes(1); expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledWith( 'userid', @@ -303,13 +295,8 @@ describe('read microsoft graph', () => { it('should read users from Groups with advanced query mode', async () => { client.getGroups.mockImplementation(getExampleGroups); - client.getGroupMembers.mockImplementation(getExampleGroupMembers); + client.getGroupUserMembers.mockImplementation(getExampleUsers); - client.getUserProfile.mockResolvedValue({ - id: 'userid', - displayName: 'User Name', - mail: 'user.name@example.com', - }); client.getUserPhotoWithSizeLimit.mockResolvedValue( 'data:image/jpeg;base64,...', ); @@ -344,19 +331,19 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'securityEnabled eq true', + select: ['id', 'displayName'], top: 999, }, 'advanced', ); - expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { - top: 999, - }); - expect(client.getUserProfile).toHaveBeenCalledTimes(1); - expect(client.getUserProfile).toHaveBeenCalledWith('userid', { - expand: undefined, - }); + expect(client.getGroupUserMembers).toHaveBeenCalledTimes(1); + expect(client.getGroupUserMembers).toHaveBeenCalledWith( + 'groupid', + { top: 999 }, + 'advanced', + ); + expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledTimes(1); expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledWith( 'userid', @@ -366,8 +353,8 @@ describe('read microsoft graph', () => { it('should read users with userExpand, groupExpand and custom transformer', async () => { client.getGroups.mockImplementation(getExampleGroups); - client.getGroupMembers.mockImplementation(getExampleGroupMembers); - client.getUserProfile.mockResolvedValue(getExampleUserProfile()); + client.getGroupUserMembers.mockImplementation(getExampleUsers); + client.getUserPhotoWithSizeLimit.mockResolvedValue( 'data:image/jpeg;base64,...', ); @@ -399,19 +386,22 @@ describe('read microsoft graph', () => { { expand: 'member', filter: 'securityEnabled eq true', + select: ['id', 'displayName'], + top: 999, + }, + undefined, + ); + + expect(client.getGroupUserMembers).toHaveBeenCalledTimes(1); + expect(client.getGroupUserMembers).toHaveBeenCalledWith( + 'groupid', + { + expand: 'manager', top: 999, }, undefined, ); - expect(client.getGroupMembers).toHaveBeenCalledTimes(1); - expect(client.getGroupMembers).toHaveBeenCalledWith('groupid', { - top: 999, - }); - expect(client.getUserProfile).toHaveBeenCalledTimes(1); - expect(client.getUserProfile).toHaveBeenCalledWith('userid', { - expand: 'manager', - }); expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledTimes(1); expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledWith( 'userid', @@ -1001,13 +991,13 @@ describe('read microsoft graph', () => { }); client.getUsers.mockImplementation(getExampleUsers); - client.getUserProfile.mockImplementation(getExampleUserProfile); client.getUserPhotoWithSizeLimit.mockResolvedValue( 'data:image/jpeg;base64,...', ); client.getGroups.mockImplementation(getExampleGroups); client.getGroupMembers.mockImplementation(getExampleGroupMembers); + client.getGroupUserMembers.mockImplementation(getExampleUsers); client.getGroupPhotoWithSizeLimit.mockResolvedValue( 'data:image/jpeg;base64,...', ); @@ -1023,6 +1013,7 @@ describe('read microsoft graph', () => { expect(client.getGroups).toHaveBeenCalledWith( { filter: 'name eq backstage-group', + select: ['id', 'displayName'], top: 999, }, undefined, @@ -1034,7 +1025,6 @@ describe('read microsoft graph', () => { }, undefined, ); - expect(client.getUserProfile).toHaveBeenCalledTimes(1); expect(client.getUserPhotoWithSizeLimit).toHaveBeenCalledTimes(1); }); }); diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index 2541416777..d0c09d6267 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -98,13 +98,7 @@ export async function readMicrosoftGraphUsers( ): Promise<{ users: UserEntity[]; // With all relations empty }> { - const users: UserEntity[] = []; - const limiter = limiterFactory(10); - - const transformer = options.transformer ?? defaultUserTransformer; - const promises: Promise[] = []; - - for await (const user of client.getUsers( + const users = client.getUsers( { filter: options.userFilter, expand: options.userExpand, @@ -112,37 +106,16 @@ export async function readMicrosoftGraphUsers( top: PAGE_SIZE, }, options.queryMode, - )) { - // Process all users in parallel, otherwise it can take quite some time - promises.push( - limiter(async () => { - let userPhoto; - try { - userPhoto = await client.getUserPhotoWithSizeLimit( - user.id!, - // We are limiting the photo size, as users with full resolution photos - // can make the Backstage API slow - 120, - ); - } catch (e) { - options.logger.warn(`Unable to load photo for ${user.id}, ${e}`); - } + ); - const entity = await transformer(user, userPhoto); - - if (!entity) { - return; - } - - users.push(entity); - }), - ); - } - - // Wait for all users and photos to be downloaded - await Promise.all(promises); - - return { users }; + return { + users: await transformUsers( + client, + users, + options.logger, + options.transformer, + ), + }; } export async function readMicrosoftGraphUsersInGroups( @@ -160,21 +133,17 @@ export async function readMicrosoftGraphUsersInGroups( ): Promise<{ users: UserEntity[]; // With all relations empty }> { - const users: UserEntity[] = []; - const limiter = limiterFactory(10); - const transformer = options.transformer ?? defaultUserTransformer; const userGroupMemberPromises: Promise[] = []; - const userPromises: Promise[] = []; - - const groupMemberUsers: Set = new Set(); + const userGroupMembers = new Map(); for await (const group of client.getGroups( { expand: options.groupExpand, search: options.userGroupMemberSearch, filter: options.userGroupMemberFilter, + select: ['id', 'displayName'], top: PAGE_SIZE, }, options.queryMode, @@ -182,17 +151,23 @@ export async function readMicrosoftGraphUsersInGroups( // Process all groups in parallel, otherwise it can take quite some time userGroupMemberPromises.push( limiter(async () => { - for await (const member of client.getGroupMembers(group.id!, { - top: PAGE_SIZE, - })) { - if (!member.id) { - continue; - } - - if (member['@odata.type'] === '#microsoft.graph.user') { - groupMemberUsers.add(member.id); - } + let groupMemberCount = 0; + for await (const user of client.getGroupUserMembers( + group.id!, + { + expand: options.userExpand, + top: PAGE_SIZE, + }, + options.queryMode, + )) { + userGroupMembers.set(user.id!, user); + groupMemberCount++; } + options.logger.debug('Read users from group', { + groupId: group.id, + groupName: group.displayName, + memberCount: groupMemberCount, + }); }), ); } @@ -200,47 +175,19 @@ export async function readMicrosoftGraphUsersInGroups( // Wait for all group members await Promise.all(userGroupMemberPromises); - options.logger.info(`groupMemberUsers ${groupMemberUsers.size}`); - for (const userId of groupMemberUsers) { - // Process all users in parallel, otherwise it can take quite some time - userPromises.push( - limiter(async () => { - let user; - let userPhoto; - try { - user = await client.getUserProfile(userId, { - expand: options.userExpand, - }); - } catch (e) { - options.logger.warn(`Unable to load user for ${userId}, ${e}`); - } - if (user) { - try { - userPhoto = await client.getUserPhotoWithSizeLimit( - user.id!, - // We are limiting the photo size, as users with full resolution photos - // can make the Backstage API slow - 120, - ); - } catch (e) { - options.logger.warn(`Unable to load userphoto for ${userId}, ${e}`); - } + options.logger.info('Read users from group membership', { + groupCount: userGroupMemberPromises.length, + userCount: userGroupMembers.size, + }); - const entity = await transformer(user, userPhoto); - - if (!entity) { - return; - } - users.push(entity); - } - }), - ); - } - - // Wait for all users and photos to be downloaded - await Promise.all(userPromises); - - return { users }; + return { + users: await transformUsers( + client, + userGroupMembers.values(), + options.logger, + options.transformer, + ), + }; } /** @@ -602,6 +549,56 @@ export async function readMicrosoftGraphOrg( return { users, groups }; } +async function transformUsers( + client: MicrosoftGraphClient, + users: Iterable | AsyncIterable, + logger: Logger, + transformer?: UserTransformer, +) { + const limiter = limiterFactory(10); + + const resolvedTransformer = transformer ?? defaultUserTransformer; + const promises: Promise[] = []; + const entities: UserEntity[] = []; + + // Process all users in parallel, otherwise it can take quite some time + for await (const user of users) { + promises.push( + limiter(async () => { + let userPhoto; + try { + userPhoto = await client.getUserPhotoWithSizeLimit( + user.id!, + // We are limiting the photo size, as users with full resolution photos + // can make the Backstage API slow + 120, + ); + } catch (e) { + logger.warn(`Unable to load user photo for`, { + user: user.id, + error: e, + }); + } + + const entity = await resolvedTransformer(user, userPhoto); + + if (entity) { + entities.push(entity); + } + }), + ); + } + + // Wait for all users and photos to be downloaded + await Promise.all(promises); + + logger.debug('Finished transforming users', { + microsoftUserCount: promises.length, + backstageUserCount: entities.length, + }); + return entities; +} + function ensureItem( target: Map>, key: string, From 6197146926d9aa7a39eb7173fb5f71b5d292529a Mon Sep 17 00:00:00 2001 From: Alex Crome Date: Tue, 31 Jan 2023 18:56:20 +0000 Subject: [PATCH 4/4] Fix bad merge Signed-off-by: Alex Crome --- .../catalog-backend-module-msgraph/src/microsoftGraph/read.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts index a060df5ea3..003448a3e8 100644 --- a/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts +++ b/plugins/catalog-backend-module-msgraph/src/microsoftGraph/read.ts @@ -38,6 +38,7 @@ import { defaultOrganizationTransformer, defaultUserTransformer, } from './defaultTransformers'; +import * as MicrosoftGraph from '@microsoft/microsoft-graph-types'; const PAGE_SIZE = 999;