diff --git a/.changeset/cuddly-lemons-cheat.md b/.changeset/cuddly-lemons-cheat.md new file mode 100644 index 0000000000..aa4ee786ff --- /dev/null +++ b/.changeset/cuddly-lemons-cheat.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-gitlab': patch +--- + +When possible, requests a more limited set of results from the Gitlab projects API, which can reduce the amount of network traffic required to sync with Gitlab. diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts index d75d0e224e..21cbd24232 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts @@ -59,6 +59,7 @@ function setupFakeServer( page: number; include_subgroups: boolean; archived: boolean; + simple?: boolean; }) => { data: GitLabProject[]; nextPage?: number; @@ -78,10 +79,12 @@ function setupFakeServer( const page = req.url.searchParams.get('page'); const include_subgroups = req.url.searchParams.get('include_subgroups'); const archived = req.url.searchParams.get('archived'); + const simple = req.url.searchParams.get('simple'); const response = listProjectsCallback({ page: parseInt(page!, 10), include_subgroups: include_subgroups === 'true', archived: archived === 'true', + simple: simple === 'true', }); // Filter the fake results based on the `last_activity_after` parameter @@ -471,6 +474,110 @@ describe('GitlabDiscoveryProcessor', () => { }); expect(result2).toHaveLength(1); }); + + it('sets simple=true when skipForkedRepos is false', async () => { + const processor = getProcessor({ + options: { skipForkedRepos: false }, + }); + setupFakeServer( + PROJECTS_URL, + request => { + return { + data: [ + { + id: 1, + archived: false, + default_branch: 'main', + last_activity_at: '2021-08-05T11:03:05.774Z', + web_url: 'https://gitlab.fake/1', + path_with_namespace: '1', + }, + ], + }; + }, + request => { + // Verify that simple=true is set in the request + expect(request.url.searchParams.get('simple')).toBe('true'); + }, + ); + + const result: any[] = []; + await processor.readLocation(PROJECT_LOCATION, false, e => { + result.push(e); + }); + expect(result).toHaveLength(1); + }); + + it('does not set simple when skipForkedRepos is true', async () => { + const processor = getProcessor({ + options: { skipForkedRepos: true }, + }); + setupFakeServer( + PROJECTS_URL, + request => { + return { + data: [ + { + id: 1, + archived: false, + default_branch: 'main', + last_activity_at: '2021-08-05T11:03:05.774Z', + web_url: 'https://gitlab.fake/1', + path_with_namespace: '1', + // Include forked_from_project to test fork filtering + forked_from_project: { + id: 100, + name: 'original-project', + }, + }, + ], + }; + }, + request => { + // Verify that simple parameter is not set + expect(request.url.searchParams.get('simple')).toBeNull(); + }, + ); + + const result: any[] = []; + await processor.readLocation(PROJECT_LOCATION, false, e => { + result.push(e); + }); + // Should be empty because forked repo is skipped + expect(result).toHaveLength(0); + }); + + it('sets default parameters correctly (archived=false, simple=true)', async () => { + const processor = getProcessor(); // Uses defaults: skipForkedRepos=false, includeArchivedRepos=false + setupFakeServer( + PROJECTS_URL, + request => { + return { + data: [ + { + id: 1, + archived: false, + default_branch: 'main', + last_activity_at: '2021-08-05T11:03:05.774Z', + web_url: 'https://gitlab.fake/1', + path_with_namespace: '1', + }, + ], + }; + }, + request => { + // Verify default parameters: archived=false and simple=true + expect(request.url.searchParams.get('archived')).toBe('false'); + expect(request.url.searchParams.get('simple')).toBe('true'); + }, + ); + + const result: any[] = []; + await processor.readLocation(PROJECT_LOCATION, false, e => { + result.push(e); + }); + expect(result).toHaveLength(1); + }); }); describe('handles failure', () => { diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index ab31a44bbc..126bb07f03 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -115,6 +115,11 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { // that the options doesn't include the key so that the API doesn't receive an empty query parameter. ...(lastActivity && { last_activity_after: lastActivity }), ...(!this.includeArchivedRepos && { archived: false }), + // Only use simple=true when we don't need to skip forked repos. + // The simple=true parameter reduces response size by returning fewer fields, + // but it excludes the 'forked_from_project' field which is required for fork detection. + // Therefore, we can only optimize with simple=true when skipForkedRepos is false. + ...(!this.skipForkedRepos && { simple: true }), }; const projects = paginated(options => client.listProjects(options), opts); diff --git a/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts b/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts index daea71060f..9b3bac6399 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts @@ -187,6 +187,55 @@ describe('GitLabClient', () => { expect(allProjects).toHaveLength(mock.all_projects_response.length); }); + + it('should pass simple parameter to API when provided', async () => { + const client = new GitLabClient({ + config: readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), + logger: mockServices.logger.mock(), + }); + + // Mock the pagedRequest method to verify parameters + const mockPagedRequest = jest.fn().mockResolvedValue({ + items: [], + nextPage: undefined, + }); + (client as any).pagedRequest = mockPagedRequest; + + await client.listProjects({ simple: true }); + + expect(mockPagedRequest).toHaveBeenCalledWith('/projects', { + simple: true, + }); + }); + + it('should pass simple parameter to group projects API when provided', async () => { + const client = new GitLabClient({ + config: readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), + logger: mockServices.logger.mock(), + }); + + // Mock the pagedRequest method to verify parameters + const mockPagedRequest = jest.fn().mockResolvedValue({ + items: [], + nextPage: undefined, + }); + (client as any).pagedRequest = mockPagedRequest; + + await client.listProjects({ group: 'test-group', simple: true }); + + expect(mockPagedRequest).toHaveBeenCalledWith( + '/groups/test-group/projects', + { + group: 'test-group', + simple: true, + include_subgroups: true, + }, + ); + }); }); describe('listUsers', () => { diff --git a/plugins/catalog-backend-module-gitlab/src/lib/client.ts b/plugins/catalog-backend-module-gitlab/src/lib/client.ts index e58227839e..3178e2c0d1 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/client.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/client.ts @@ -43,6 +43,7 @@ interface ListProjectOptions extends CommonListOptions { group?: string; membership?: boolean; topics?: string; + simple?: boolean; } interface UserListOptions extends CommonListOptions { diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts index dd7a4f0d6d..b03f2c4b5b 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts @@ -147,6 +147,21 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { 'GitlabDiscoveryEntityProvider:test-id', ); + // Mock the GitLabClient listProjects method to verify default parameters + const originalListProjects = (provider as any).gitLabClient.listProjects; + const mockListProjects = jest.fn().mockImplementation(async options => { + // Verify default parameters: archived=false and simple=true (since skipForkedRepos=false by default) + expect(options).toMatchObject({ + group: 'group1', + per_page: 50, + archived: false, + simple: true, // Should be set since skipForkedRepos defaults to false + }); + // Call the original method to maintain test behavior + return originalListProjects.call((provider as any).gitLabClient, options); + }); + (provider as any).gitLabClient.listProjects = mockListProjects; + await provider.connect(entityProviderConnection); const taskDef = schedule.getTasks()[0]; @@ -559,3 +574,115 @@ describe('GitlabDiscoveryEntityProvider - events', () => { }); // EventSupportChange: stop add tests >>> }); + +describe('GitlabDiscoveryEntityProvider - simple parameter', () => { + it('should pass simple=true when skipForkedRepos is false', async () => { + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'example.com', + apiBaseUrl: 'https://example.com/api/v4', + token: 'test-token', + }, + ], + }, + catalog: { + providers: { + gitlab: { + 'test-id': { + host: 'example.com', + group: 'test-group', + skipForkedRepos: false, + }, + }, + }, + }, + }); + + const schedule = new PersistingTaskRunner(); + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const provider = GitlabDiscoveryEntityProvider.fromConfig(config, { + logger, + schedule, + })[0]; + + // Mock the GitLabClient listProjects method to verify parameters + const mockListProjects = jest.fn().mockResolvedValue({ + items: [], + nextPage: undefined, + }); + + (provider as any).gitLabClient.listProjects = mockListProjects; + + await provider.connect(entityProviderConnection); + await provider.refresh(logger); + + expect(mockListProjects).toHaveBeenCalledWith({ + group: 'test-group', + page: undefined, + per_page: 50, + archived: false, + simple: true, // Should be set when skipForkedRepos is false + }); + }); + + it('should not pass simple when skipForkedRepos is true', async () => { + const config = new ConfigReader({ + integrations: { + gitlab: [ + { + host: 'example.com', + apiBaseUrl: 'https://example.com/api/v4', + token: 'test-token', + }, + ], + }, + catalog: { + providers: { + gitlab: { + 'test-id': { + host: 'example.com', + group: 'test-group', + skipForkedRepos: true, + }, + }, + }, + }, + }); + + const schedule = new PersistingTaskRunner(); + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const provider = GitlabDiscoveryEntityProvider.fromConfig(config, { + logger, + schedule, + })[0]; + + // Mock the GitLabClient listProjects method to verify parameters + const mockListProjects = jest.fn().mockResolvedValue({ + items: [], + nextPage: undefined, + }); + + (provider as any).gitLabClient.listProjects = mockListProjects; + + await provider.connect(entityProviderConnection); + await provider.refresh(logger); + + expect(mockListProjects).toHaveBeenCalledWith({ + group: 'test-group', + page: undefined, + per_page: 50, + archived: false, + // simple should not be present when skipForkedRepos is true + }); + }); +}); diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts index 8bf749c275..243ffd5287 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts @@ -216,6 +216,11 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { ...(!this.config.includeArchivedRepos && { archived: false }), ...(this.config.membership && { membership: true }), ...(this.config.topics && { topics: this.config.topics }), + // Only use simple=true when we don't need to skip forked repos. + // The simple=true parameter reduces response size by returning fewer fields, + // but it excludes the 'forked_from_project' field which is required for fork detection. + // Therefore, we can only optimize with simple=true when skipForkedRepos is false. + ...(!this.config.skipForkedRepos && { simple: true }), }, );