From 159a862ec340a0cdff06f5ab560008a9119aee50 Mon Sep 17 00:00:00 2001 From: pillaris Date: Mon, 27 Apr 2026 12:15:34 +0530 Subject: [PATCH] fix: address second round of Copilot review comments - getUserPhotoGated now throws for non-404 errors (401/403/5xx) so callers can distinguish a missing photo from a real API failure - Log a debug message when photo loading fails instead of swallowing the error silently - Merge transformer-pre-populated spec.members/spec.children with fetched Graph membership rather than overwriting them - deriveRestLength now also excludes manual-trigger schedule objects (not just cron expressions) from being cast as HumanDuration Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: pillaris --- ...softGraphIncrementalEntityProvider.test.ts | 58 ++++++++++++++++++- ...MicrosoftGraphIncrementalEntityProvider.ts | 24 +++++++- .../src/clientHelpers.test.ts | 13 ++++- .../src/clientHelpers.ts | 10 +++- ...MicrosoftGraphIncrementalEntityProvider.ts | 11 +++- 5 files changed, 108 insertions(+), 8 deletions(-) diff --git a/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts b/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts index ffb7793dae..c384debb71 100644 --- a/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts +++ b/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts @@ -416,7 +416,7 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => { expect(mockGetUserPhotoGated).toHaveBeenCalledWith(mockClient, 'u1', 120); }); - it('continues processing remaining users when a photo load fails', async () => { + it('continues processing remaining users when a photo load fails and logs the error', async () => { mockRequestOnePage.mockResolvedValueOnce({ items: [ { @@ -446,6 +446,10 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => { // Both users should still be emitted despite the photo failure expect(result.entities!).toHaveLength(2); + expect(logger.debug).toHaveBeenCalledWith( + expect.stringContaining('failed to load photo for user u1'), + expect.anything(), + ); }); }); @@ -731,6 +735,58 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => { ); }); + it('merges transformer-pre-populated members with fetched membership', 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: 'u2', + displayName: 'Bob', + userPrincipalName: 'bob@example.com', + }), + ); + + const provider = new MicrosoftGraphIncrementalEntityProvider({ + id: 'default', + provider: baseProviderConfig as any, + logger, + groupTransformer: async group => { + const base = await import( + '@backstage/plugin-catalog-backend-module-msgraph' + ).then(m => m.defaultGroupTransformer(group)); + if (!base) return undefined; + // Transformer pre-populates an extra member + base.spec = { ...base.spec, members: ['user:default/extra-user'] }; + return base; + }, + }); + + const result = await provider.next(makeContext(), groupsCursor); + + const groupEntity = result.entities!.find( + e => + e.entity.metadata.annotations?.[ + MICROSOFT_GRAPH_GROUP_ID_ANNOTATION + ] === 'grp-1', + ); + // Should contain both the transformer-set member and the fetched one + expect(groupEntity!.entity.spec?.members).toContain( + 'user:default/extra-user', + ); + expect(groupEntity!.entity.spec?.members).toContain( + 'user:default/bob_example.com', + ); + }); + it('passes group filter and search from provider config', async () => { const providerWithFilter = { ...baseProviderConfig, diff --git a/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts b/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts index de7f57d1f5..8b0835e08d 100644 --- a/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts +++ b/plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts @@ -319,8 +319,13 @@ export class MicrosoftGraphIncrementalEntityProvider if (provider.loadUserPhotos !== false) { try { userPhoto = await getUserPhotoGated(client, user.id!, 120); - } catch { - // Photo load failures are non-fatal + } catch (e) { + this.options.logger.debug( + `${this.getProviderName()}: failed to load photo for user ${ + user.id + }`, + { error: e }, + ); } } @@ -451,11 +456,24 @@ export class MicrosoftGraphIncrementalEntityProvider } } + // Merge fetched membership with any members/children the transformer + // may have pre-populated, so custom transformers can augment the list. + const existingMembers = Array.isArray(entity.spec?.members) + ? (entity.spec.members as string[]) + : []; + const existingChildren = Array.isArray(entity.spec?.children) + ? (entity.spec.children as string[]) + : []; + entities.push({ locationKey: `msgraph-org-provider:${this.options.id}`, entity: withLocations(this.options.id, { ...entity, - spec: { ...entity.spec, members: userRefs, children: childRefs }, + spec: { + ...entity.spec, + members: [...new Set([...existingMembers, ...userRefs])], + children: [...new Set([...existingChildren, ...childRefs])], + }, }), }); }), diff --git a/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts b/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts index 1819492be8..fb8b28d8ec 100644 --- a/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts +++ b/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts @@ -225,7 +225,7 @@ describe('getUserPhotoGated', () => { afterEach(() => jest.resetAllMocks()); - it('returns undefined when the photo check returns non-200', async () => { + it('returns undefined when the photo check returns 404', async () => { (client.requestApi as jest.Mock).mockResolvedValue({ status: 404, } as Response); @@ -236,6 +236,17 @@ describe('getUserPhotoGated', () => { expect(client.getUserPhotoWithSizeLimit).not.toHaveBeenCalled(); }); + it('throws for non-404 error responses so callers can distinguish real errors', async () => { + (client.requestApi as jest.Mock).mockResolvedValue({ + status: 403, + } as Response); + + await expect(getUserPhotoGated(client, 'user-id', 120)).rejects.toThrow( + 'Unexpected status 403 when checking photo for user user-id', + ); + expect(client.getUserPhotoWithSizeLimit).not.toHaveBeenCalled(); + }); + it('returns the photo data URI when the check passes', async () => { (client.requestApi as jest.Mock).mockResolvedValue({ status: 200, diff --git a/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts b/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts index 79fd4d3487..7a0cce38c8 100644 --- a/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts +++ b/plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts @@ -85,6 +85,9 @@ export async function requestOnePage( * Like `getUserPhotoWithSizeLimit` but skips the size-listing call for users * with no photo. For users without a photo: 1 fast check call. For users with * a photo: 1 check + the normal size-limited fetch (2 more calls). + * + * Returns `undefined` only for 404 (no photo assigned). Throws for any other + * non-200 status so callers can distinguish "no photo" from real errors. */ export async function getUserPhotoGated( client: MicrosoftGraphClient, @@ -92,6 +95,11 @@ export async function getUserPhotoGated( maxSize: number, ): Promise { const check = await client.requestApi(`users/${userId}/photo`); - if (check.status !== 200) return undefined; + if (check.status === 404) return undefined; + if (check.status !== 200) { + throw new Error( + `Unexpected status ${check.status} when checking photo for user ${userId}`, + ); + } return await client.getUserPhotoWithSizeLimit(userId, maxSize); } diff --git a/plugins/catalog-backend-module-msgraph-incremental/src/module/catalogModuleMicrosoftGraphIncrementalEntityProvider.ts b/plugins/catalog-backend-module-msgraph-incremental/src/module/catalogModuleMicrosoftGraphIncrementalEntityProvider.ts index dc96fde43a..674ce7a009 100644 --- a/plugins/catalog-backend-module-msgraph-incremental/src/module/catalogModuleMicrosoftGraphIncrementalEntityProvider.ts +++ b/plugins/catalog-backend-module-msgraph-incremental/src/module/catalogModuleMicrosoftGraphIncrementalEntityProvider.ts @@ -231,13 +231,20 @@ function deriveRestLength( logger: LoggerService, ): HumanDuration { const freq = providerConfig.schedule?.frequency; - if (freq && typeof freq === 'object' && !('cron' in freq)) { + // Only treat plain duration objects as restLength — exclude cron expressions + // and any other non-duration schedule types (e.g. manual triggers). + if ( + freq && + typeof freq === 'object' && + !('cron' in freq) && + !('trigger' in freq) + ) { return freq as HumanDuration; } if (freq) { logger.warn( `MicrosoftGraphIncrementalEntityProvider:${providerConfig.id}: ` + - `schedule.frequency is a cron expression; cannot derive restLength from it. ` + + `schedule.frequency is not a duration-based schedule; cannot derive restLength from it. ` + `Defaulting restLength to 8 hours.`, ); }