fix: address third round of Copilot review comments

- Wrap userTransformer call for group members in try/catch so a sparse
  object that causes the transformer to throw logs a warning and skips
  that member rather than failing the entire groups page
- Split PAGE_SIZE into USER_PAGE_SIZE (999) and GROUP_PAGE_SIZE (100)
  so each burst stays within its time budget despite per-group member
  fetching in the groups phase
- Always send $count=true in advanced mode even when no query object is
  provided (use empty object spread instead of short-circuit on query)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: pillaris <pillaris@adobe.com>

plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts

@@ -231,7 +231,7 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
         mockClient,
         'users',
         expect.objectContaining({
-          query: expect.objectContaining({ top: 999 }),
+          query: expect.objectContaining({ top: 999 }), // USER_PAGE_SIZE
         }),
       );
       // No users → advances straight to groups phase
@@ -735,6 +735,62 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
       );
     });
 
+    it('logs a warning and skips a group member when the transformer throws', async () => {
+      (mockClient.getOrganization as jest.Mock).mockResolvedValue({
+        id: 'org-id',
+        displayName: 'My Org',
+      });
+      mockRequestOnePage.mockResolvedValueOnce({
+        items: [
+          { id: 'grp-1', displayName: 'Engineering', mail: 'eng@example.com' },
+        ],
+        nextLink: undefined,
+      });
+      (mockClient.getGroupMembers as jest.Mock).mockReturnValue(
+        asyncYield(
+          {
+            '@odata.type': '#microsoft.graph.user',
+            id: 'u-bad',
+            // sparse — no userPrincipalName, transformer will throw
+          },
+          {
+            '@odata.type': '#microsoft.graph.user',
+            id: 'u-good',
+            displayName: 'Alice',
+            userPrincipalName: 'alice@example.com',
+          },
+        ),
+      );
+
+      const provider = new MicrosoftGraphIncrementalEntityProvider({
+        id: 'default',
+        provider: baseProviderConfig as any,
+        logger,
+        userTransformer: async user => {
+          if (!user.userPrincipalName) throw new Error('Missing UPN');
+          const { defaultUserTransformer } = await import(
+            '@backstage/plugin-catalog-backend-module-msgraph'
+          );
+          return defaultUserTransformer(user);
+        },
+      });
+
+      const result = await provider.next(makeContext(), groupsCursor);
+
+      expect(logger.warn).toHaveBeenCalledWith(
+        expect.stringContaining('group member user u-bad failed to transform'),
+        expect.anything(),
+      );
+      const groupEntity = result.entities!.find(
+        e =>
+          e.entity.metadata.annotations?.[
+            MICROSOFT_GRAPH_GROUP_ID_ANNOTATION
+          ] === 'grp-1',
+      );
+      // Only the good member should appear
+      expect(groupEntity!.entity.spec?.members).toHaveLength(1);
+    });
+
     it('merges transformer-pre-populated members with fetched membership', async () => {
       (mockClient.getOrganization as jest.Mock).mockResolvedValue({
         id: 'org-id',

plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts

@@ -47,7 +47,10 @@ import {
 import { LoggerService } from '@backstage/backend-plugin-api';
 import { getUserPhotoGated, requestOnePage } from './clientHelpers';
 
-const PAGE_SIZE = 999;
+const USER_PAGE_SIZE = 999;
+// Groups phase fetches members for every group on the page, so a smaller page
+// size keeps each burst within its time budget.
+const GROUP_PAGE_SIZE = 100;
 
 /**
  * Backstage entity names must be ≤63 chars ([a-zA-Z0-9] separated by [-_.]).
@@ -301,7 +304,7 @@ export class MicrosoftGraphIncrementalEntityProvider
             filter: provider.userFilter,
             expand: provider.userExpand,
             select: provider.userSelect,
-            top: PAGE_SIZE,
+            top: USER_PAGE_SIZE,
           },
           queryMode: provider.queryMode,
           nextLink,
@@ -376,7 +379,7 @@ export class MicrosoftGraphIncrementalEntityProvider
             search: provider.groupSearch,
             expand: provider.groupExpand,
             select: provider.groupSelect,
-            top: PAGE_SIZE,
+            top: GROUP_PAGE_SIZE,
           },
           queryMode: provider.queryMode,
           nextLink,
@@ -425,22 +428,31 @@ export class MicrosoftGraphIncrementalEntityProvider
           const childRefs: string[] = [];
 
           for await (const member of client.getGroupMembers(group.id!, {
-            top: PAGE_SIZE,
+            top: GROUP_PAGE_SIZE,
           })) {
             if (member['@odata.type'] === '#microsoft.graph.user') {
-              const userEntity = await userTransformer(
-                member as MicrosoftGraph.User,
-              );
-              if (userEntity) {
-                userEntity.metadata.name = capEntityName(
-                  userEntity.metadata.name,
+              try {
+                const userEntity = await userTransformer(
+                  member as MicrosoftGraph.User,
                 );
-                userRefs.push(stringifyEntityRef(userEntity));
-              } else {
-                this.options.logger.debug(
+                if (userEntity) {
+                  userEntity.metadata.name = capEntityName(
+                    userEntity.metadata.name,
+                  );
+                  userRefs.push(stringifyEntityRef(userEntity));
+                } else {
+                  this.options.logger.debug(
+                    `${this.getProviderName()}: group member user ${
+                      member.id
+                    } could not be transformed (sparse object?), skipping`,
+                  );
+                }
+              } catch (e) {
+                this.options.logger.warn(
                   `${this.getProviderName()}: group member user ${
                     member.id
-                  } could not be transformed (sparse object?), skipping`,
+                  } failed to transform, skipping`,
+                  { error: e },
                 );
               }
             } else if (member['@odata.type'] === '#microsoft.graph.group') {

plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts

@@ -110,6 +110,21 @@ describe('requestOnePage', () => {
     );
   });
 
+  it('sends $count=true in advanced mode even when no query is provided', async () => {
+    (client.requestApi as jest.Mock).mockResolvedValue(
+      makeResponse(200, { value: [] }),
+    );
+
+    await requestOnePage(client, 'users', { queryMode: 'advanced' });
+
+    expect(client.requestApi).toHaveBeenCalledWith(
+      'users',
+      expect.objectContaining({ count: true }),
+      { ConsistencyLevel: 'eventual' },
+      undefined,
+    );
+  });
+
   it('does not set $count for basic mode', async () => {
     (client.requestApi as jest.Mock).mockResolvedValue(
       makeResponse(200, { value: [] }),

plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts

@@ -45,9 +45,7 @@ export async function requestOnePage<T>(
   // Microsoft Graph requires $count=true whenever ConsistencyLevel: eventual is set,
   // including plain listing requests with no $filter or $search.
   const finalQuery =
-    appliedQueryMode === 'advanced' && query
-      ? { ...query, count: true }
-      : query;
+    appliedQueryMode === 'advanced' ? { ...(query ?? {}), count: true } : query;
 
   const headers: Record<string, string> =
     appliedQueryMode === 'advanced' ? { ConsistencyLevel: 'eventual' } : {};