fix: address Copilot review comments on msgraph-incremental provider

- Use undefined instead of null for MSGraphCursor.nextLink (repo style)
- Avoid mutating caller-provided query object in requestOnePage
- Improve error handling for non-JSON Graph API error responses
- Build entity spec immutably instead of mutating entity.spec directly
- Log debug message when a sparse group member cannot be transformed
- Warn when schedule.frequency is a cron expression (restLength falls back to 8h)
- Use proper MSGraphContext generic instead of unknown in addProvider

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

plugins/catalog-backend-module-msgraph-incremental/report.api.md

@@ -91,6 +91,6 @@ export type MSGraphContext = {
 // @public
 export type MSGraphCursor = {
   phase: 'users' | 'groups';
-  nextLink: string | null;
+  nextLink?: string;
 };
 ```

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

@@ -320,7 +320,7 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
 
       expect(result.done).toBe(false);
       expect((result.cursor as MSGraphCursor).phase).toBe('groups');
-      expect((result.cursor as MSGraphCursor).nextLink).toBeNull();
+      expect((result.cursor as MSGraphCursor).nextLink).toBeUndefined();
     });
 
     it('skips users where the transformer returns undefined', async () => {
@@ -450,7 +450,7 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
   });
 
   describe('next — groups phase', () => {
-    const groupsCursor: MSGraphCursor = { phase: 'groups', nextLink: null };
+    const groupsCursor: MSGraphCursor = { phase: 'groups' };
 
     it('emits the tenant root group on the first groups page', async () => {
       (mockClient.getOrganization as jest.Mock).mockResolvedValue({

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

@@ -86,13 +86,13 @@ function withLocations(providerId: string, entity: Entity): Entity {
  *
  * The `nextLink` field holds the `@odata.nextLink` URL returned by the
  * Microsoft Graph API, which encodes all state needed to resume a paged
- * request. A `null` value means the current phase is starting fresh.
+ * request. An absent value means the current phase is starting fresh.
  *
  * @public
  */
 export type MSGraphCursor = {
   phase: 'users' | 'groups';
-  nextLink: string | null;
+  nextLink?: string;
 };
 
 /**
@@ -279,7 +279,7 @@ export class MicrosoftGraphIncrementalEntityProvider
     cursor?: MSGraphCursor,
   ): Promise<EntityIteratorResult<MSGraphCursor>> {
     const phase = cursor?.phase ?? 'users';
-    const nextLink = cursor?.nextLink ?? null;
+    const nextLink = cursor?.nextLink;
 
     if (phase === 'users') {
       return this.readUsersPage(client, provider, nextLink);
@@ -290,7 +290,7 @@ export class MicrosoftGraphIncrementalEntityProvider
   private async readUsersPage(
     client: MicrosoftGraphClient,
     provider: MicrosoftGraphProviderConfig,
-    nextLink: string | null,
+    nextLink: string | undefined,
   ): Promise<EntityIteratorResult<MSGraphCursor>> {
     const { items: rawUsers, nextLink: newNextLink } =
       await requestOnePage<MicrosoftGraph.User>(
@@ -304,7 +304,7 @@ export class MicrosoftGraphIncrementalEntityProvider
             top: PAGE_SIZE,
           },
           queryMode: provider.queryMode,
-          nextLink: nextLink ?? undefined,
+          nextLink,
         },
       );
 
@@ -352,14 +352,14 @@ export class MicrosoftGraphIncrementalEntityProvider
     return {
       done: false,
       entities,
-      cursor: { phase: 'groups', nextLink: null },
+      cursor: { phase: 'groups' },
     };
   }
 
   private async readGroupsPage(
     client: MicrosoftGraphClient,
     provider: MicrosoftGraphProviderConfig,
-    nextLink: string | null,
+    nextLink: string | undefined,
   ): Promise<EntityIteratorResult<MSGraphCursor>> {
     const { items: rawGroups, nextLink: newNextLink } =
       await requestOnePage<MicrosoftGraph.Group>(
@@ -374,7 +374,7 @@ export class MicrosoftGraphIncrementalEntityProvider
             top: PAGE_SIZE,
           },
           queryMode: provider.queryMode,
-          nextLink: nextLink ?? undefined,
+          nextLink,
         },
       );
 
@@ -431,6 +431,12 @@ export class MicrosoftGraphIncrementalEntityProvider
                   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`,
+                );
               }
             } else if (member['@odata.type'] === '#microsoft.graph.group') {
               const childEntity = await groupTransformer(
@@ -445,12 +451,12 @@ export class MicrosoftGraphIncrementalEntityProvider
             }
           }
 
-          entity.spec.members = userRefs;
-          entity.spec.children = childRefs;
-
           entities.push({
             locationKey: `msgraph-org-provider:${this.options.id}`,
-            entity: withLocations(this.options.id, entity),
+            entity: withLocations(this.options.id, {
+              ...entity,
+              spec: { ...entity.spec, members: userRefs, children: childRefs },
+            }),
           });
         }),
       ),

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

@@ -44,24 +44,33 @@ export async function requestOnePage<T>(
 
   // Microsoft Graph requires $count=true whenever ConsistencyLevel: eventual is set,
   // including plain listing requests with no $filter or $search.
-  if (appliedQueryMode === 'advanced' && query) {
-    query.count = true;
-  }
+  const finalQuery =
+    appliedQueryMode === 'advanced' && query
+      ? { ...query, count: true }
+      : query;
 
   const headers: Record<string, string> =
     appliedQueryMode === 'advanced' ? { ConsistencyLevel: 'eventual' } : {};
 
   const response = nextLink
     ? await client.requestRaw(nextLink, headers, 2, signal)
-    : await client.requestApi(path, query, headers, signal);
+    : await client.requestApi(path, finalQuery, headers, signal);
 
   if (response.status !== 200) {
-    const body = await response.json();
-    const err = body?.error;
+    let message = `HTTP ${response.status}`;
+    try {
+      const body = await response.json();
+      const err = body?.error;
+      if (err?.code || err?.message) {
+        message = `${err.code} - ${err.message}`;
+      }
+    } catch {
+      // Response body is not JSON; fall back to HTTP status above
+    }
     throw new Error(
-      `Error while reading ${nextLink ?? path} from Microsoft Graph: ${
-        err?.code
-      } - ${err?.message}`,
+      `Error while reading ${
+        nextLink ?? path
+      } from Microsoft Graph: ${message}`,
     );
   }
 

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

@@ -18,6 +18,7 @@ import {
   coreServices,
   createBackendModule,
   createExtensionPoint,
+  LoggerService,
 } from '@backstage/backend-plugin-api';
 import { incrementalIngestionProvidersExtensionPoint } from '@backstage/plugin-catalog-backend-module-incremental-ingestion';
 import {
@@ -30,6 +31,7 @@ import {
 import { HumanDuration } from '@backstage/types';
 import {
   MicrosoftGraphIncrementalEntityProvider,
+  MSGraphContext,
   MSGraphCursor,
 } from '../MicrosoftGraphIncrementalEntityProvider';
 
@@ -192,9 +194,9 @@ export const catalogModuleMicrosoftGraphIncrementalEntityProvider =
               ),
             });
 
-            const restLength = deriveRestLength(providerConfig);
+            const restLength = deriveRestLength(providerConfig, logger);
 
-            incremental.addProvider<MSGraphCursor, unknown>({
+            incremental.addProvider<MSGraphCursor, MSGraphContext>({
               provider,
               options: {
                 burstInterval: { seconds: 3 },
@@ -226,10 +228,18 @@ function resolveTransformer<T extends Function>(
 
 function deriveRestLength(
   providerConfig: ReturnType<typeof readProviderConfigs>[number],
+  logger: LoggerService,
 ): HumanDuration {
   const freq = providerConfig.schedule?.frequency;
   if (freq && typeof freq === 'object' && !('cron' in freq)) {
     return freq as HumanDuration;
   }
+  if (freq) {
+    logger.warn(
+      `MicrosoftGraphIncrementalEntityProvider:${providerConfig.id}: ` +
+        `schedule.frequency is a cron expression; cannot derive restLength from it. ` +
+        `Defaulting restLength to 8 hours.`,
+    );
+  }
   return { hours: 8 };
 }