diff --git a/.changeset/fluffy-owls-act.md b/.changeset/fluffy-owls-act.md new file mode 100644 index 0000000000..03ae42fd99 --- /dev/null +++ b/.changeset/fluffy-owls-act.md @@ -0,0 +1,7 @@ +--- +'@backstage/backend-defaults': patch +'@backstage/integration': minor +'@backstage/plugin-catalog-backend-module-gitlab': patch +--- + +Add configurable throttling and retry mechanism for GitLab integration. diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.test.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.test.ts index 4bae21a3ef..d7513a22d6 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.test.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.test.ts @@ -78,9 +78,6 @@ describe('GitlabUrlReader', () => { describe('read', () => { beforeEach(() => { worker.use( - rest.get('*/api/v4/projects/:name', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), rest.get('*', (req, res, ctx) => res( ctx.status(200), @@ -107,14 +104,14 @@ describe('GitlabUrlReader', () => { url: 'https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/my/path/to/file.yaml', config: createConfig(), response: expect.objectContaining({ - url: 'https://gitlab.com/api/v4/projects/12345/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', + url: 'https://gitlab.com/api/v4/projects/groupA%2Fteams%2FteamA%2FsubgroupA%2FrepoA/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', }), }, { url: 'https://gitlab.example.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/my/path/to/file.yaml', config: createConfig('0123456789'), response: expect.objectContaining({ - url: 'https://gitlab.example.com/api/v4/projects/12345/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', + url: 'https://gitlab.example.com/api/v4/projects/groupA%2Fteams%2FteamA%2FsubgroupA%2FrepoA/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', headers: expect.objectContaining({ authorization: 'Bearer 0123456789', }), @@ -124,7 +121,7 @@ describe('GitlabUrlReader', () => { url: 'https://gitlab.com/groupA/teams/teamA/repoA/-/blob/branch/my/path/to/file.yaml', // Repo not in subgroup config: createConfig(), response: expect.objectContaining({ - url: 'https://gitlab.com/api/v4/projects/12345/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', + url: 'https://gitlab.com/api/v4/projects/groupA%2Fteams%2FteamA%2FrepoA/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', }), }, @@ -133,7 +130,7 @@ describe('GitlabUrlReader', () => { url: 'https://gitlab.example.com/a/b/blob/master/c.yaml', config: createConfig(), response: expect.objectContaining({ - url: 'https://gitlab.example.com/api/v4/projects/12345/repository/files/c.yaml/raw?ref=master', + url: 'https://gitlab.example.com/api/v4/projects/a%2Fb/repository/files/c.yaml/raw?ref=master', }), }, ])('should handle happy path %#', async ({ url, config, response }) => { @@ -177,9 +174,6 @@ describe('GitlabUrlReader', () => { it('should throw NotModified on HTTP 304 from etag', async () => { worker.use( - rest.get('*/api/v4/projects/:name', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), rest.get('*', (req, res, ctx) => { expect(req.headers.get('If-None-Match')).toBe('999'); return res(ctx.status(304)); @@ -198,9 +192,6 @@ describe('GitlabUrlReader', () => { it('should throw NotModified on HTTP 304 from lastModifiedAt', async () => { worker.use( - rest.get('*/api/v4/projects/:name', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), rest.get('*', (req, res, ctx) => { expect(req.headers.get('If-Modified-Since')).toBe( new Date('2019 12 31 23:59:59 GMT').toUTCString(), @@ -221,9 +212,6 @@ describe('GitlabUrlReader', () => { it('should return etag and last-modified in response', async () => { worker.use( - rest.get('*/api/v4/projects/:name', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), rest.get('*', (_req, res, ctx) => { return res( ctx.status(200), @@ -248,15 +236,6 @@ describe('GitlabUrlReader', () => { it('should return the file when using a user token', async () => { worker.use( - rest.get('*/api/v4/projects/user%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-user-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), rest.get('*', (_req, res, ctx) => { return res(ctx.status(200), ctx.body('foo')); }), @@ -566,18 +545,6 @@ describe('GitlabUrlReader', () => { }); it('should return the file when using a user token', async () => { - worker.use( - rest.get('*/api/v4/projects/user%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-user-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), - ); - const response = await gitlabProcessor.readTree( 'https://gitlab.com/user/project/tree/main', { token: 'gl-user-token' }, @@ -738,30 +705,13 @@ describe('GitlabUrlReader', () => { }); describe('getGitlabFetchUrl', () => { - beforeEach(() => { - worker.use( - rest.get( - '*/api/v4/projects/group%2Fsubgroup%2Fproject', - (_, res, ctx) => res(ctx.status(200), ctx.json({ id: 12345 })), - ), - rest.get('*/api/v4/projects/user%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-user-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), - ); - }); it('should fall back to getGitLabFileFetchUrl for blob urls', async () => { await expect( (gitlabProcessor as any).getGitlabFetchUrl( 'https://gitlab.com/group/subgroup/project/-/blob/branch/my/path/to/file.yaml', ), ).resolves.toEqual( - 'https://gitlab.com/api/v4/projects/12345/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', + 'https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', ); }); it('should work for job artifact urls', async () => { @@ -770,7 +720,7 @@ describe('GitlabUrlReader', () => { 'https://gitlab.com/group/subgroup/project/-/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', ), ).resolves.toEqual( - 'https://gitlab.com/api/v4/projects/12345/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', + 'https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', ); }); it('should fail on unfamiliar or non-Gitlab urls', async () => { @@ -779,7 +729,7 @@ describe('GitlabUrlReader', () => { 'https://gitlab.com/some/random/endpoint', ), ).rejects.toThrow( - 'Failed converting /some/random/endpoint to a project id. Url path must include /blob/.', + 'Failed extracting project path from /some/random/endpoint. Url path must include /blob/.', ); }); it('should resolve the project path using a user token', async () => { @@ -789,39 +739,18 @@ describe('GitlabUrlReader', () => { 'gl-user-token', ), ).resolves.toEqual( - 'https://gitlab.com/api/v4/projects/12345/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', + 'https://gitlab.com/api/v4/projects/user%2Fproject/repository/files/my%2Fpath%2Fto%2Ffile.yaml/raw?ref=branch', ); }); }); describe('getGitlabArtifactFetchUrl', () => { - beforeEach(() => { - worker.use( - rest.get( - '*/api/v4/projects/group%2Fsubgroup%2Fproject', - (_, res, ctx) => res(ctx.status(200), ctx.json({ id: 12345 })), - ), - rest.get( - '*/api/v4/projects/groupA%2Fsubgroup%2Fproject', - (_, res, ctx) => res(ctx.status(404)), - ), - rest.get('*/api/v4/projects/user%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-user-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), - ); - }); - it('should reject urls that are not for the job artifacts API', async () => { - await expect( + it('should reject urls that are not for the job artifacts API', () => { + expect(() => (gitlabProcessor as any).getGitlabArtifactFetchUrl( new URL('https://gitlab.com/some/url'), ), - ).rejects.toThrow('Unable to process url as an GitLab artifact'); + ).toThrow('Unable to process url as an GitLab artifact'); }); it('should work for job artifact urls', async () => { await expect( @@ -832,18 +761,22 @@ describe('GitlabUrlReader', () => { ), ).resolves.toEqual( new URL( - 'https://gitlab.com/api/v4/projects/12345/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', + 'https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', ), ); }); - it('errors in mapping the project ID should be captured', async () => { + it('should work for job artifact urls with any project path', async () => { await expect( (gitlabProcessor as any).getGitlabArtifactFetchUrl( new URL( 'https://gitlab.com/groupA/subgroup/project/-/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', ), ), - ).rejects.toThrow(/^Unable to translate GitLab artifact URL:/); + ).resolves.toEqual( + new URL( + 'https://gitlab.com/api/v4/projects/groupA%2Fsubgroup%2Fproject/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', + ), + ); }); it('should resolve the project path using a user token', async () => { await expect( @@ -855,51 +788,9 @@ describe('GitlabUrlReader', () => { ), ).resolves.toEqual( new URL( - 'https://gitlab.com/api/v4/projects/12345/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', + 'https://gitlab.com/api/v4/projects/user%2Fproject/jobs/artifacts/branch/raw/my/path/to/file.yaml?job=myJob', ), ); }); }); - - describe('resolveProjectToId', () => { - beforeEach(() => { - worker.use( - rest.get('*/api/v4/projects/group%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-dummy-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), - rest.get('*/api/v4/projects/user%2Fproject', (req, res, ctx) => { - if (req.headers.get('authorization') !== 'Bearer gl-user-token') { - return res( - ctx.status(401), - ctx.json({ message: '401 Unauthorized' }), - ); - } - return res(ctx.status(200), ctx.json({ id: 12345 })); - }), - ); - }); - - it('should resolve the project path to a valid project id', async () => { - await expect( - (gitlabProcessor as any).resolveProjectToId( - new URL('https://gitlab.com/group/project'), - ), - ).resolves.toEqual(12345); - }); - - it('should resolve the project path to a valid project id using a user token', async () => { - await expect( - (gitlabProcessor as any).resolveProjectToId( - new URL('https://gitlab.com/user/project'), - 'gl-user-token', - ), - ).resolves.toEqual(12345); - }); - }); }); diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.ts index 182dc2cf28..f386df768d 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/GitlabUrlReader.ts @@ -14,9 +14,6 @@ * limitations under the License. */ -// NOTE(freben): Intentionally uses node-fetch because of https://github.com/backstage/backstage/issues/28190 -import fetch, { Response } from 'node-fetch'; - import { UrlReaderService, UrlReaderServiceReadTreeOptions, @@ -41,10 +38,8 @@ import { import parseGitUrl from 'git-url-parse'; import { trimEnd, trimStart } from 'lodash'; import { Minimatch } from 'minimatch'; -import { Readable } from 'node:stream'; import { ReadUrlResponseFactory } from './ReadUrlResponseFactory'; import { ReaderFactory, ReadTreeResponseFactory } from './types'; -import { parseLastModified } from './util'; /** * Implements a {@link @backstage/backend-plugin-api#UrlReaderService} for files on GitLab. @@ -89,14 +84,14 @@ export class GitlabUrlReader implements UrlReaderService { let response: Response; try { - response = await fetch(builtUrl, { + response = await this.integration.fetch(builtUrl, { headers: { ...getGitLabRequestOptions(this.integration.config, token).headers, ...(etag && !isArtifact && { 'If-None-Match': etag }), ...(lastModifiedAfter && !isArtifact && { - 'If-Modified-Since': lastModifiedAfter.toUTCString(), - }), + 'If-Modified-Since': lastModifiedAfter.toUTCString(), + }), }, // TODO(freben): The signal cast is there because pre-3.x versions of // node-fetch have a very slightly deviating AbortSignal type signature. @@ -115,12 +110,7 @@ export class GitlabUrlReader implements UrlReaderService { } if (response.ok) { - return ReadUrlResponseFactory.fromNodeJSReadable(response.body, { - etag: response.headers.get('ETag') ?? undefined, - lastModifiedAt: parseLastModified( - response.headers.get('Last-Modified'), - ), - }); + return ReadUrlResponseFactory.fromResponse(response); } const message = `${url} could not be read as ${builtUrl}, ${response.status} ${response.statusText}`; @@ -155,7 +145,7 @@ export class GitlabUrlReader implements UrlReaderService { // Use GitLab API to get the default branch // encodeURIComponent is required for GitLab API // https://docs.gitlab.com/ee/api/README.html#namespaced-path-encoding - const projectGitlabResponse = await fetch( + const projectGitlabResponse = await this.integration.fetch( new URL( `${this.integration.config.apiBaseUrl}/projects/${encodeURIComponent( repoFullName, @@ -182,7 +172,7 @@ export class GitlabUrlReader implements UrlReaderService { if (!!filepath) { commitsReqParams.set('path', filepath); } - const commitsGitlabResponse = await fetch( + const commitsGitlabResponse = await this.integration.fetch( new URL( `${this.integration.config.apiBaseUrl}/projects/${encodeURIComponent( repoFullName, @@ -218,23 +208,22 @@ export class GitlabUrlReader implements UrlReaderService { archiveReqParams.set('path', filepath); } // https://docs.gitlab.com/ee/api/repositories.html#get-file-archive - const archiveGitLabResponse = await fetch( - `${this.integration.config.apiBaseUrl}/projects/${encodeURIComponent( + const reqUrl = `${this.integration.config.apiBaseUrl + }/projects/${encodeURIComponent( repoFullName, - )}/repository/archive?${archiveReqParams.toString()}`, - { - ...getGitLabRequestOptions(this.integration.config, token), - // TODO(freben): The signal cast is there because pre-3.x versions of - // node-fetch have a very slightly deviating AbortSignal type signature. - // The difference does not affect us in practice however. The cast can - // be removed after we support ESM for CLI dependencies and migrate to - // version 3 of node-fetch. - // https://github.com/backstage/backstage/issues/8242 - ...(signal && { signal: signal as any }), - }, - ); + )}/repository/archive?${archiveReqParams.toString()}`; + const archiveGitLabResponse = await this.integration.fetch(reqUrl, { + ...getGitLabRequestOptions(this.integration.config, token), + // TODO(freben): The signal cast is there because pre-3.x versions of + // node-fetch have a very slightly deviating AbortSignal type signature. + // The difference does not affect us in practice however. The cast can + // be removed after we support ESM for CLI dependencies and migrate to + // version 3 of node-fetch. + // https://github.com/backstage/backstage/issues/8242 + ...(signal && { signal: signal as any }), + }); if (!archiveGitLabResponse.ok) { - const message = `Failed to read tree (archive) from ${url}, ${archiveGitLabResponse.status} ${archiveGitLabResponse.statusText}`; + const message = `Failed to read tree (archive) from ${url}, ${reqUrl}, ${archiveGitLabResponse.status} ${archiveGitLabResponse.statusText}`; if (archiveGitLabResponse.status === 404) { throw new NotFoundError(message); } @@ -242,7 +231,7 @@ export class GitlabUrlReader implements UrlReaderService { } return await this.deps.treeResponseFactory.fromTarArchive({ - stream: Readable.from(archiveGitLabResponse.body), + response: archiveGitLabResponse, subpath: filepath, etag: commitSha, filter: options?.filter, @@ -337,74 +326,48 @@ export class GitlabUrlReader implements UrlReaderService { // If the target is for a job artifact then go down that path const targetUrl = new URL(target); if (targetUrl.pathname.includes('/-/jobs/artifacts/')) { - return this.getGitlabArtifactFetchUrl(targetUrl, token).then(value => + return this.getGitlabArtifactFetchUrl(targetUrl).then(value => value.toString(), ); } - // Default to the old behavior of assuming the url is for a file + // Default to the optimized behavior - no API call needed for file URLs return getGitLabFileFetchUrl(target, this.integration.config, token); } // convert urls of the form: // https://example.com///-/jobs/artifacts//raw/?job= // to urls of the form: - // https://example.com/api/v4/projects/:id/jobs/artifacts/:ref_name/raw/*artifact_path?job= - private async getGitlabArtifactFetchUrl( - target: URL, - token?: string, - ): Promise { + // https://example.com/api/v4/projects/namespace%2Fproject/jobs/artifacts/:ref_name/raw/*artifact_path?job= + private getGitlabArtifactFetchUrl(target: URL): Promise { if (!target.pathname.includes('/-/jobs/artifacts/')) { throw new Error('Unable to process url as an GitLab artifact'); } try { const [namespaceAndProject, ref] = target.pathname.split('/-/jobs/artifacts/'); - const projectPath = new URL(target); - projectPath.pathname = namespaceAndProject; - const projectId = await this.resolveProjectToId(projectPath, token); + + // Extract project path directly instead of making API call const relativePath = getGitLabIntegrationRelativePath( this.integration.config, ); + + let projectPath = namespaceAndProject; + // Check relative path exist and remove it + if (relativePath) { + projectPath = trimStart(projectPath, relativePath); + } + // Trim an initial / if it exists + projectPath = projectPath.replace(/^\//, ''); + const newUrl = new URL(target); - newUrl.pathname = `${relativePath}/api/v4/projects/${projectId}/jobs/artifacts/${ref}`; - return newUrl; + newUrl.pathname = `${relativePath}/api/v4/projects/${encodeURIComponent( + projectPath, + )}/jobs/artifacts/${ref}`; + return Promise.resolve(newUrl); } catch (e) { throw new Error( `Unable to translate GitLab artifact URL: ${target}, ${e}`, ); } } - - private async resolveProjectToId( - pathToProject: URL, - token?: string, - ): Promise { - let project = pathToProject.pathname; - // Check relative path exist and remove it if so - const relativePath = getGitLabIntegrationRelativePath( - this.integration.config, - ); - if (relativePath) { - project = project.replace(relativePath, ''); - } - // Trim an initial / if it exists - project = project.replace(/^\//, ''); - const result = await fetch( - `${ - pathToProject.origin - }${relativePath}/api/v4/projects/${encodeURIComponent(project)}`, - getGitLabRequestOptions(this.integration.config, token), - ); - const data = await result.json(); - if (!result.ok) { - if (result.status === 401) { - throw new Error( - 'GitLab Error: 401 - Unauthorized. The access token used is either expired, or does not have permission to read the project', - ); - } - - throw new Error(`Gitlab error: ${data.error}, ${data.error_description}`); - } - return Number(data.id); - } } diff --git a/packages/integration/config.d.ts b/packages/integration/config.d.ts index 4278a8fcfe..add237a6b9 100644 --- a/packages/integration/config.d.ts +++ b/packages/integration/config.d.ts @@ -386,6 +386,28 @@ export interface Config { * @visibility secret */ commitSigningKey?: string; + + /** + * Retry configuration for requests. + * @visibility frontend + */ + retry?: { + /** + * Maximum number of retries for failed requests. + * @visibility frontend + */ + maxRetries?: number; + /** + * HTTP status codes that should trigger a retry. + * @visibility frontend + */ + retryStatusCodes?: number[]; + /** + * Maximum number of API requests allowed per minute. Set to -1 to disable rate limiting. + * @visibility frontend + */ + maxApiRequestsPerMinute?: number; + }; }>; /** Integration configuration for Google Cloud Storage */ diff --git a/packages/integration/package.json b/packages/integration/package.json index 24eb80018f..74902d3c3e 100644 --- a/packages/integration/package.json +++ b/packages/integration/package.json @@ -46,7 +46,8 @@ "cross-fetch": "^4.0.0", "git-url-parse": "^15.0.0", "lodash": "^4.17.21", - "luxon": "^3.0.0" + "luxon": "^3.0.0", + "p-throttle": "^4.1.1" }, "devDependencies": { "@backstage/cli": "workspace:^", diff --git a/packages/integration/report.api.md b/packages/integration/report.api.md index abcdb8898b..83c75db4d1 100644 --- a/packages/integration/report.api.md +++ b/packages/integration/report.api.md @@ -596,7 +596,7 @@ export function getGitilesAuthenticationUrl( export function getGitLabFileFetchUrl( url: string, config: GitLabIntegrationConfig, - token?: string, + _token?: string, ): Promise; // @public @@ -759,6 +759,8 @@ export class GitLabIntegration implements ScmIntegration { // (undocumented) static factory: ScmIntegrationsFactory; // (undocumented) + fetch(input: RequestInfo | URL, init?: RequestInit): Promise; + // (undocumented) resolveEditUrl(url: string): string; // (undocumented) resolveUrl(options: { @@ -779,6 +781,11 @@ export type GitLabIntegrationConfig = { token?: string; baseUrl: string; commitSigningKey?: string; + retry?: { + maxRetries?: number; + retryStatusCodes?: number[]; + maxApiRequestsPerMinute?: number; + }; }; // @public diff --git a/packages/integration/src/gitlab/GitLabIntegration.test.ts b/packages/integration/src/gitlab/GitLabIntegration.test.ts index 575ce75308..7489badb8c 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.test.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.test.ts @@ -14,8 +14,20 @@ * limitations under the License. */ +import { setupServer } from 'msw/node'; +import { rest } from 'msw'; import { ConfigReader } from '@backstage/config'; -import { GitLabIntegration, replaceGitLabUrlType } from './GitLabIntegration'; +import { + GitLabIntegration, + replaceGitLabUrlType, + sleep, +} from './GitLabIntegration'; +import { registerMswTestHooks } from '../helpers'; + +// Mock pThrottle to make testing easier +jest.mock('p-throttle', () => { + return jest.fn(() => (fn: any) => fn); +}); describe('GitLabIntegration', () => { it('has a working factory', () => { @@ -45,7 +57,11 @@ describe('GitLabIntegration', () => { }); it('resolve edit URL', () => { - const integration = new GitLabIntegration({ host: 'h.com' } as any); + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + }); expect( integration.resolveEditUrl( @@ -53,6 +69,349 @@ describe('GitLabIntegration', () => { ), ).toBe('https://gitlab.com/my-org/my-project/-/edit/develop/README.md'); }); + + describe('fetch strategy', () => { + const worker = setupServer(); + registerMswTestHooks(worker); + + beforeAll(() => { + jest.useFakeTimers(); + }); + afterAll(() => { + jest.useRealTimers(); + }); + beforeEach(() => { + jest.clearAllTimers(); + }); + + it('uses plain fetch when no throttling or retries configured', async () => { + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + }); + + let calledUrl: string | undefined; + worker.use( + rest.get('https://h.com/api/v4', (req, res, ctx) => { + calledUrl = req.url.href; + return res(ctx.status(200)); + }), + ); + + await integration.fetch('https://h.com/api/v4'); + expect(calledUrl).toBe('https://h.com/api/v4'); + }); + + it('applies retry logic when maxRetries > 0', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + if (callCount === 1) { + return res(ctx.status(429), ctx.json({})); + } + return res(ctx.status(200), ctx.json({})); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [429], + }, + }); + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(100); + await jest.advanceTimersByTimeAsync(200); + await jest.advanceTimersByTimeAsync(400); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(callCount).toBe(2); + }); + + it('does not retry when status code is not in retryStatusCodes', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + return res(ctx.status(404)); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [429, 500], + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(1000); + const response = await responsePromise; + + expect(response.status).toBe(404); + expect(callCount).toBe(1); + }); + + it('stops retrying after maxRetries attempts', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + return res(ctx.status(429)); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 2, + retryStatusCodes: [429], + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(100); + await jest.advanceTimersByTimeAsync(200); + await jest.advanceTimersByTimeAsync(400); + const response = await responsePromise; + + expect(response.status).toBe(429); + expect(callCount).toBe(3); // initial + 2 retries + }); + + it('applies throttling when limitPerMinute > 0', async () => { + const pThrottle = require('p-throttle'); + const throttleMock = jest.fn(() => (fn: any) => fn); + pThrottle.mockReturnValue(throttleMock); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxApiRequestsPerMinute: 60, + }, + }); + + await integration.fetch('https://h.com/api/v4'); + + expect(pThrottle).toHaveBeenCalledWith({ + limit: 60, + interval: 60_000, + }); + expect(throttleMock).toHaveBeenCalled(); + }); + + it('applies both throttling and retry when both are configured', async () => { + const pThrottle = require('p-throttle'); + + const throttleMock = jest.fn((fn: any) => fn); + pThrottle.mockReturnValue(throttleMock); + + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + if (callCount === 1) { + return res(ctx.status(429), ctx.json({})); + } + return res(ctx.status(200), ctx.json({})); + }), + ); + + const integration = new GitLabIntegration({ + apiBaseUrl: 'https://h.com/api/v4', + host: 'h.com', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [429], + maxApiRequestsPerMinute: 60, + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(100); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(pThrottle).toHaveBeenCalledWith({ + limit: 60, + interval: 60_000, + }); + expect(callCount).toBe(2); + }); + + it('retries based on configured status codes', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + if (callCount === 1) { + return res( + ctx.status(429), + ctx.set('Retry-After', '1'), + ctx.json({}), + ); + } + return res(ctx.status(200), ctx.json({})); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [429], + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(1000); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(callCount).toBe(2); + }); + + it('retries multiple times for persistent failures', async () => { + let callCount = 0; + worker.use( + rest.get('https://h.com/api/v4', (_req, res, ctx) => { + callCount += 1; + if (callCount < 3) { + return res(ctx.status(500), ctx.json({})); + } + return res(ctx.status(200), ctx.json({})); + }), + ); + + const integration = new GitLabIntegration({ + host: 'h.com', + apiBaseUrl: 'https://h.com/api/v4', + baseUrl: 'https://h.com', + retry: { + maxRetries: 3, + retryStatusCodes: [500], + }, + }); + + const responsePromise = integration.fetch('https://h.com/api/v4'); + await jest.advanceTimersByTimeAsync(100); + await jest.advanceTimersByTimeAsync(200); + await jest.advanceTimersByTimeAsync(400); + const response = await responsePromise; + + expect(response.status).toBe(200); + expect(callCount).toBe(3); + }); + }); +}); + +describe('sleep', () => { + beforeAll(() => { + jest.useFakeTimers(); + }); + afterAll(() => { + jest.useRealTimers(); + }); + beforeEach(() => { + jest.clearAllTimers(); + }); + + it('should resolve after the specified duration when not aborted', async () => { + const duration = 1000; + const sleepPromise = sleep(duration, null); + + // Fast-forward timers to trigger the timeout + jest.advanceTimersByTimeAsync(duration); + + await expect(sleepPromise).resolves.toBeUndefined(); + }); + + it('should resolve immediately if abortSignal is already aborted', async () => { + const abortController = new AbortController(); + abortController.abort(); + + const sleepPromise = sleep(5000, abortController.signal); + + // Should resolve immediately without needing to advance timers + await expect(sleepPromise).resolves.toBeUndefined(); + }); + + it('should resolve when aborted during wait', async () => { + const abortController = new AbortController(); + const duration = 5000; + + const sleepPromise = sleep(duration, abortController.signal); + + // Abort the signal after starting the sleep + abortController.abort(); + + // Should resolve immediately when aborted + await expect(sleepPromise).resolves.toBeUndefined(); + }); + + it('should handle undefined abortSignal gracefully', async () => { + const duration = 500; + const sleepPromise = sleep(duration, undefined); + + // Fast-forward timers to trigger the timeout + jest.advanceTimersByTimeAsync(duration); + + await expect(sleepPromise).resolves.toBeUndefined(); + }); + + it('should clean up timeout when aborted', async () => { + const abortController = new AbortController(); + const duration = 10000; + + const sleepPromise = sleep(duration, abortController.signal); + + // Check that a timer was set + expect(jest.getTimerCount()).toBe(1); + + // Abort the signal + abortController.abort(); + + // Wait for the promise to resolve + await sleepPromise; + + // Timer should be cleaned up + expect(jest.getTimerCount()).toBe(0); + }); + + it('should clean up abort event listener when timeout completes', async () => { + const abortController = new AbortController(); + const duration = 1000; + + const sleepPromise = sleep(duration, abortController.signal); + + // Fast-forward timers to complete the timeout + jest.advanceTimersByTimeAsync(duration); + + // Wait for the promise to complete and verify it resolves properly + await expect(sleepPromise).resolves.toBeUndefined(); + + // Event listener should be cleaned up - aborting after completion should not cause issues + abortController.abort(); // This should not affect anything since the sleep is already done + + // Verify the sleep function handled cleanup properly + expect(jest.getTimerCount()).toBe(0); + }); }); describe('replaceGitLabUrlType', () => { diff --git a/packages/integration/src/gitlab/GitLabIntegration.ts b/packages/integration/src/gitlab/GitLabIntegration.ts index 0c52799599..68afb6f63e 100644 --- a/packages/integration/src/gitlab/GitLabIntegration.ts +++ b/packages/integration/src/gitlab/GitLabIntegration.ts @@ -13,13 +13,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - import { basicIntegrations, defaultScmResolveUrl } from '../helpers'; import { ScmIntegration, ScmIntegrationsFactory } from '../types'; import { GitLabIntegrationConfig, readGitLabIntegrationConfigs, } from './config'; +import pThrottle from 'p-throttle'; + +type FetchFunction = typeof fetch; /** * A GitLab based integration. @@ -37,7 +39,12 @@ export class GitLabIntegration implements ScmIntegration { ); }; - constructor(private readonly integrationConfig: GitLabIntegrationConfig) {} + private readonly fetchImpl: FetchFunction; + + constructor(private readonly integrationConfig: GitLabIntegrationConfig) { + // Configure fetch strategy based on configuration + this.fetchImpl = this.createFetchStrategy(); + } get type(): string { return 'gitlab'; @@ -62,6 +69,97 @@ export class GitLabIntegration implements ScmIntegration { resolveEditUrl(url: string): string { return replaceGitLabUrlType(url, 'edit'); } + + fetch(input: RequestInfo | URL, init?: RequestInit): Promise { + return this.fetchImpl(input, init); + } + + private createFetchStrategy(): FetchFunction { + let fetchFn: FetchFunction = async (url, options) => { + return fetch(url, { ...options, mode: 'same-origin' }); + }; + + const retryConfig = this.integrationConfig.retry; + if (retryConfig) { + // Apply retry wrapper if configured + fetchFn = this.withRetry(fetchFn, retryConfig); + + // Apply throttling wrapper if configured + if ( + retryConfig.maxApiRequestsPerMinute && + retryConfig.maxApiRequestsPerMinute > 0 + ) { + fetchFn = pThrottle({ + limit: retryConfig.maxApiRequestsPerMinute, + interval: 60_000, + })(fetchFn); + } + } + + return fetchFn; + } + + private withRetry( + fetchFn: FetchFunction, + retryConfig: { maxRetries?: number; retryStatusCodes?: number[] }, + ): FetchFunction { + const maxRetries = retryConfig?.maxRetries ?? 0; + const retryStatusCodes = retryConfig?.retryStatusCodes ?? []; + if (maxRetries <= 0 || retryStatusCodes.length === 0) { + return fetchFn; + } + + return async (url, options) => { + const abortSignal = options?.signal; + let response: Response; + let attempt = 0; + for (;;) { + response = await fetchFn(url, options); + // If response is not retryable, return immediately + if (!retryStatusCodes.includes(response.status)) { + break; + } + + // If this was the last allowed attempt, return response + if (attempt++ >= maxRetries) { + break; + } + // Determine delay from Retry-After header if present, otherwise exponential backoff + const retryAfter = response.headers.get('Retry-After'); + const delay = retryAfter + ? parseInt(retryAfter, 10) * 1000 + : Math.min(100 * Math.pow(2, attempt - 1), 10000); // Exponential backoff, cap at 10 seconds + + await sleep(delay, abortSignal); + } + + return response; + }; + } +} + +export async function sleep( + durationMs: number, + abortSignal: AbortSignal | null | undefined, +): Promise { + if (abortSignal?.aborted) { + return; + } + + await new Promise(resolve => { + let timeoutHandle: NodeJS.Timeout | undefined = undefined; + + const done = () => { + if (timeoutHandle) { + clearTimeout(timeoutHandle); + } + abortSignal?.removeEventListener('abort', done); + resolve(); + }; + + timeoutHandle = setTimeout(done, durationMs); + abortSignal?.addEventListener('abort', done); + }); } /** diff --git a/packages/integration/src/gitlab/config.test.ts b/packages/integration/src/gitlab/config.test.ts index f1d17a6d7e..693e31e2e4 100644 --- a/packages/integration/src/gitlab/config.test.ts +++ b/packages/integration/src/gitlab/config.test.ts @@ -58,6 +58,11 @@ describe('readGitLabIntegrationConfig', () => { token: ' t\n', apiBaseUrl: 'https://a.com', baseUrl: 'https://baseurl.for.me/gitlab', + retry: { + maxRetries: 3, + maxApiRequestsPerMinute: 1000, + retryStatusCodes: [429], + }, }), ); @@ -66,6 +71,12 @@ describe('readGitLabIntegrationConfig', () => { token: 't', apiBaseUrl: 'https://a.com', baseUrl: 'https://baseurl.for.me/gitlab', + commitSigningKey: undefined, + retry: { + maxRetries: 3, + maxApiRequestsPerMinute: 1000, + retryStatusCodes: [429], + }, }); }); @@ -77,6 +88,8 @@ describe('readGitLabIntegrationConfig', () => { host: 'gitlab.com', apiBaseUrl: 'https://gitlab.com/api/v4', baseUrl: 'https://gitlab.com', + commitSigningKey: undefined, + retry: undefined, }); }); @@ -89,6 +102,7 @@ describe('readGitLabIntegrationConfig', () => { host: 'gitlab.com', baseUrl: 'https://gitlab.com', apiBaseUrl: 'https://gitlab.com/api/v4', + retry: undefined, }); }); @@ -119,6 +133,9 @@ describe('readGitLabIntegrationConfig', () => { host: 'a.com', apiBaseUrl: 'https://a.com/api', baseUrl: 'https://a.com', + token: undefined, // token is filtered out on frontend + commitSigningKey: undefined, + retry: undefined, }); }); }); @@ -144,6 +161,7 @@ describe('readGitLabIntegrationConfigs', () => { token: 't', apiBaseUrl: 'https://a.com/api/v4', baseUrl: 'https://a.com', + retry: undefined, }); }); diff --git a/packages/integration/src/gitlab/config.ts b/packages/integration/src/gitlab/config.ts index 636919291c..344d054904 100644 --- a/packages/integration/src/gitlab/config.ts +++ b/packages/integration/src/gitlab/config.ts @@ -21,6 +21,32 @@ import { isValidHost, isValidUrl } from '../helpers'; const GITLAB_HOST = 'gitlab.com'; const GITLAB_API_BASE_URL = 'https://gitlab.com/api/v4'; +/** + * Reads an optional number array from config + */ +function readOptionalNumberArray( + config: Config, + key: string, +): number[] | undefined { + const value = config.getOptional(key); + if (value === undefined) { + return undefined; + } + if (!Array.isArray(value)) { + throw new Error( + `Invalid ${key} config: expected an array, got ${typeof value}`, + ); + } + return value.map((item, index) => { + if (typeof item !== 'number') { + throw new Error( + `Invalid ${key} config: all values must be numbers, got ${typeof item} at index ${index}`, + ); + } + return item; + }); +} + /** * The configuration parameters for a single GitLab integration. * @@ -59,6 +85,29 @@ export type GitLabIntegrationConfig = { * Signing key to sign commits */ commitSigningKey?: string; + + /** + * Retry configuration for failed requests. + */ + retry?: { + /** + * Maximum number of retries for failed requests + * @defaultValue 0 + */ + maxRetries?: number; + + /** + * HTTP status codes that should trigger a retry + * @defaultValue [] + */ + retryStatusCodes?: number[]; + + /** + * Rate limit for requests per minute + * @defaultValue -1 + */ + maxApiRequestsPerMinute?: number; + }; }; /** @@ -100,12 +149,25 @@ export function readGitLabIntegrationConfig( ); } + const retryConfig = config.getOptionalConfig('retry'); + + const retry = retryConfig + ? { + maxRetries: retryConfig.getOptionalNumber('maxRetries') ?? 0, + retryStatusCodes: + readOptionalNumberArray(retryConfig, 'retryStatusCodes') ?? [], + maxApiRequestsPerMinute: + retryConfig.getOptionalNumber('maxApiRequestsPerMinute') ?? -1, + } + : undefined; + return { host, token, apiBaseUrl, baseUrl, commitSigningKey: config.getOptionalString('commitSigningKey'), + retry, }; } diff --git a/packages/integration/src/gitlab/core.test.ts b/packages/integration/src/gitlab/core.test.ts index d514dba755..58fcbe34a5 100644 --- a/packages/integration/src/gitlab/core.test.ts +++ b/packages/integration/src/gitlab/core.test.ts @@ -14,29 +14,14 @@ * limitations under the License. */ -import { rest } from 'msw'; -import { setupServer } from 'msw/node'; import { GitLabIntegrationConfig } from './config'; -import { getGitLabFileFetchUrl, getGitLabRequestOptions } from './core'; - -const worker = setupServer(); +import { + getGitLabFileFetchUrl, + getGitLabRequestOptions, + extractProjectPath, +} from './core'; describe('gitlab core', () => { - beforeAll(() => worker.listen({ onUnhandledRequest: 'error' })); - afterAll(() => worker.close()); - afterEach(() => worker.resetHandlers()); - - beforeEach(() => { - worker.use( - rest.get('*/api/v4/projects/group%2Fproject', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), - rest.get('*/api/v4/projects/group%2Fsubgroup%2Fproject', (_, res, ctx) => - res(ctx.status(200), ctx.json({ id: 12345 })), - ), - ); - }); - const configWithNoToken: GitLabIntegrationConfig = { host: 'gitlab.com', apiBaseUrl: '', @@ -63,7 +48,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/-/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -73,7 +58,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/-/blob/branch/blob/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/blob%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/blob%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -83,7 +68,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/subgroup/project/-/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -93,7 +78,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/-/blob/branch/folder/file.yml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -103,7 +88,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/-/blob/branch/folder/file with spaces.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -114,7 +99,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.mycompany.com/group/project/-/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.mycompany.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.mycompany.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configSelfHostedWithoutRelativePath), ).resolves.toBe(fetchUrl); @@ -124,7 +109,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.mycompany.com/group/project/-/blob/branch/folder/file with spaces.yaml'; const fetchUrl = - 'https://gitlab.mycompany.com/api/v4/projects/12345/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; + 'https://gitlab.mycompany.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configSelfHostedWithoutRelativePath), ).resolves.toBe(fetchUrl); @@ -135,7 +120,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.mycompany.com/gitlab/group/project/-/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.mycompany.com/gitlab/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.mycompany.com/gitlab/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configSelfHosteWithRelativePath), ).resolves.toBe(fetchUrl); @@ -145,7 +130,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.mycompany.com/gitlab/group/project/-/blob/branch/folder/file with spaces.yaml'; const fetchUrl = - 'https://gitlab.mycompany.com/gitlab/api/v4/projects/12345/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; + 'https://gitlab.mycompany.com/gitlab/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile%20with%20spaces.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configSelfHosteWithRelativePath), ).resolves.toBe(fetchUrl); @@ -159,7 +144,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -169,7 +154,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/subgroup/project/blob/branch/folder/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=branch'; + 'https://gitlab.com/api/v4/projects/group%2Fsubgroup%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=branch'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -179,7 +164,7 @@ describe('gitlab core', () => { const target = 'https://gitlab.com/group/project/blob/blob/folder/file.yaml'; const fetchUrl = - 'https://gitlab.com/api/v4/projects/12345/repository/files/folder%2Ffile.yaml/raw?ref=blob'; + 'https://gitlab.com/api/v4/projects/group%2Fproject/repository/files/folder%2Ffile.yaml/raw?ref=blob'; await expect( getGitLabFileFetchUrl(target, configWithNoToken), ).resolves.toBe(fetchUrl); @@ -187,8 +172,49 @@ describe('gitlab core', () => { }); }); + describe('extractProjectPath', () => { + it('extracts project path from scoped route', () => { + const target = + 'https://gitlab.com/group/project/-/blob/branch/folder/file.yaml'; + expect(extractProjectPath(target, configWithNoToken)).toBe( + 'group/project', + ); + }); + + it('extracts project path from subgroup', () => { + const target = + 'https://gitlab.com/group/subgroup/project/-/blob/branch/folder/file.yaml'; + expect(extractProjectPath(target, configWithNoToken)).toBe( + 'group/subgroup/project', + ); + }); + + it('extracts project path from unscoped route', () => { + const target = + 'https://gitlab.com/group/project/blob/branch/folder/file.yaml'; + expect(extractProjectPath(target, configWithNoToken)).toBe( + 'group/project', + ); + }); + + it('extracts project path from self-hosted gitlab with relative path', () => { + const target = + 'https://gitlab.mycompany.com/gitlab/group/project/-/blob/branch/folder/file.yaml'; + expect(extractProjectPath(target, configSelfHosteWithRelativePath)).toBe( + 'group/project', + ); + }); + + it('throws error for invalid URLs without blob path', () => { + const target = 'https://gitlab.com/some/random/endpoint'; + expect(() => extractProjectPath(target, configWithNoToken)).toThrow( + 'Failed extracting project path from /some/random/endpoint. Url path must include /blob/.', + ); + }); + }); + describe('getGitLabRequestOptions', () => { - it('should return Authorization bearer header when a token is provided', () => { + it('should return Authorization bearer header when a token is provided', async () => { const token = '1234567890'; const result = getGitLabRequestOptions( configSelfHosteWithRelativePath, @@ -202,7 +228,7 @@ describe('gitlab core', () => { }); }); - it('should return Authorization bearer header using the config token when no token is provided', () => { + it('should return Authorization bearer header using the config token when no token is provided', async () => { const result = getGitLabRequestOptions(configSelfHosteWithRelativePath); expect(result).toEqual({ diff --git a/packages/integration/src/gitlab/core.ts b/packages/integration/src/gitlab/core.ts index 65360840b5..435e5a8a31 100644 --- a/packages/integration/src/gitlab/core.ts +++ b/packages/integration/src/gitlab/core.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import fetch from 'cross-fetch'; import { getGitLabIntegrationRelativePath, GitLabIntegrationConfig, @@ -28,22 +27,25 @@ import { * * Converts * from: https://gitlab.example.com/a/b/blob/master/c.yaml - * to: https://gitlab.com/api/v4/projects/projectId/repository/c.yaml?ref=master + * to: https://gitlab.com/api/v4/projects/a%2Fb/repository/files/c.yaml/raw?ref=master * -or- * from: https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/filepath - * to: https://gitlab.com/api/v4/projects/projectId/repository/files/filepath?ref=branch + * to: https://gitlab.com/api/v4/projects/groupA%2Fteams%2FteamA%2FsubgroupA%2FrepoA/repository/files/filepath/raw?ref=branch * * @param url - A URL pointing to a file * @param config - The relevant provider config + * @param token - An optional auth token (not used in path extraction, kept for compatibility) * @public */ -export async function getGitLabFileFetchUrl( +export function getGitLabFileFetchUrl( url: string, config: GitLabIntegrationConfig, - token?: string, + _token?: string, ): Promise { - const projectID = await getProjectId(url, config, token); - return buildProjectUrl(url, projectID, config).toString(); + // Use project path directly instead of making an API call to get project ID + // Note: _token parameter kept for backward compatibility but not used for path extraction + const projectPath = extractProjectPath(url, config); + return Promise.resolve(buildProjectUrl(url, projectPath, config).toString()); } /** @@ -72,10 +74,10 @@ export function getGitLabRequestOptions( // Converts // from: https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/filepath -// to: https://gitlab.com/api/v4/projects/projectId/repository/files/filepath?ref=branch +// to: https://gitlab.com/api/v4/projects/groupA%2Fteams%2FteamA%2FsubgroupA%2FrepoA/repository/files/filepath/raw?ref=branch export function buildProjectUrl( target: string, - projectID: Number, + projectPathOrID: string | Number, config: GitLabIntegrationConfig, ): URL { try { @@ -88,10 +90,12 @@ export function buildProjectUrl( const [branch, ...filePath] = branchAndFilePath.split('/'); const relativePath = getGitLabIntegrationRelativePath(config); + const projectIdentifier = encodeURIComponent(String(projectPathOrID)); + url.pathname = [ ...(relativePath ? [relativePath] : []), 'api/v4/projects', - projectID, + projectIdentifier, 'repository/files', encodeURIComponent(decodeURIComponent(filePath.join('/'))), 'raw', @@ -105,62 +109,33 @@ export function buildProjectUrl( } } -// Convert -// from: https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/filepath -// to: The project ID that corresponds to the URL -export async function getProjectId( +/** + * Extracts the project path from a GitLab URL + * from: https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/filepath + * to: groupA/teams/teamA/subgroupA/repoA + */ +export function extractProjectPath( target: string, config: GitLabIntegrationConfig, - token?: string, -): Promise { +): string { const url = new URL(target); if (!url.pathname.includes('/blob/')) { throw new Error( - `Failed converting ${url.pathname} to a project id. Url path must include /blob/.`, + `Failed extracting project path from ${url.pathname}. Url path must include /blob/.`, ); } - try { - let repo = url.pathname.split('/-/blob/')[0].split('/blob/')[0]; + let repo = url.pathname.split('/-/blob/')[0].split('/blob/')[0]; - // Get gitlab relative path - const relativePath = getGitLabIntegrationRelativePath(config); + // Get gitlab relative path + const relativePath = getGitLabIntegrationRelativePath(config); - // Check relative path exist and replace it if it's the case. - if (relativePath) { - repo = repo.replace(relativePath, ''); - } - - // Convert - // to: https://gitlab.com/api/v4/projects/groupA%2Fteams%2FsubgroupA%2FteamA%2Frepo - const repoIDLookup = new URL( - `${url.origin}${relativePath}/api/v4/projects/${encodeURIComponent( - repo.replace(/^\//, ''), - )}`, - ); - - const response = await fetch( - repoIDLookup.toString(), - getGitLabRequestOptions(config, token), - ); - - const data = await response.json(); - - if (!response.ok) { - if (response.status === 401) { - throw new Error( - 'GitLab Error: 401 - Unauthorized. The access token used is either expired, or does not have permission to read the project', - ); - } - - throw new Error( - `GitLab Error '${data.error}', ${data.error_description}`, - ); - } - - return Number(data.id); - } catch (e) { - throw new Error(`Could not get GitLab project ID for: ${target}, ${e}`); + // Check relative path exist and replace it if it's the case. + if (relativePath) { + repo = repo.replace(relativePath, ''); } + + // Remove leading slash + return repo.replace(/^\//, ''); } diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 5ae71dda06..eda2bca1ab 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -102,7 +102,7 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { } const client = new GitLabClient({ - config: integration.config, + integration: integration, logger: this.logger, }); this.logger.debug(`Reading GitLab projects from ${location.target}`); 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 5e10fbffd6..adda756b94 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/client.test.ts @@ -19,7 +19,10 @@ import { registerMswTestHooks, } from '@backstage/backend-test-utils'; import { ConfigReader } from '@backstage/config'; -import { readGitLabIntegrationConfig } from '@backstage/integration'; +import { + GitLabIntegration, + readGitLabIntegrationConfig, +} from '@backstage/integration'; import { setupServer } from 'msw/node'; import { handlers } from '../__testUtils__/handlers'; import * as mock from '../__testUtils__/mocks'; @@ -33,8 +36,10 @@ describe('GitLabClient', () => { describe('isSelfManaged', () => { it('returns true if self managed instance', () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -42,7 +47,9 @@ describe('GitLabClient', () => { }); it('returns false if gitlab.com', () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + ), logger: mockServices.logger.mock(), }); expect(client.isSelfManaged()).toBeFalsy(); @@ -52,8 +59,10 @@ describe('GitLabClient', () => { describe('pagedRequest', () => { it('should provide immediate items within the page', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -65,8 +74,10 @@ describe('GitLabClient', () => { it('should request items for a given page number', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -86,8 +97,10 @@ describe('GitLabClient', () => { it('should not have a next page if at the end', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -105,8 +118,10 @@ describe('GitLabClient', () => { it('should throw if response is not okay', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -121,8 +136,10 @@ describe('GitLabClient', () => { describe('listProjects', () => { it('should get projects for a given group', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -145,8 +162,10 @@ describe('GitLabClient', () => { it('should get not archived projects', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -170,8 +189,10 @@ describe('GitLabClient', () => { it('should get all projects for an instance', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -190,8 +211,10 @@ describe('GitLabClient', () => { it('should pass simple parameter to API when provided', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -212,8 +235,10 @@ describe('GitLabClient', () => { it('should pass simple parameter to group projects API when provided', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -241,8 +266,10 @@ describe('GitLabClient', () => { describe('listUsers', () => { it('listUsers gets all users in the instance', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -262,8 +289,10 @@ describe('GitLabClient', () => { describe('listGroups', () => { it('listGroups gets all groups in the instance', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -344,7 +373,9 @@ describe('GitLabClient', () => { describe('get gitlab.com users', () => { it('gets all users under group', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + ), logger: mockServices.logger.mock(), }); const saasMembers = ( @@ -358,7 +389,9 @@ describe('GitLabClient', () => { }); it('gets all users with token without full permissions', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + ), logger: mockServices.logger.mock(), }); const saasMembers = ( @@ -368,7 +401,9 @@ describe('GitLabClient', () => { }); it('rejects when GraphQL returns errors', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_saas)), + ), logger: mockServices.logger.mock(), }); await expect(() => @@ -379,8 +414,10 @@ describe('GitLabClient', () => { }); it('traverses multi-page results', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -397,8 +434,10 @@ describe('GitLabClient', () => { describe('listDescendantGroups', () => { it('gets all groups under root', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -412,8 +451,10 @@ describe('GitLabClient', () => { it('gets all descendant groups with token without full permissions', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -427,8 +468,10 @@ describe('GitLabClient', () => { it('rejects when GraphQL returns errors', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -441,8 +484,10 @@ describe('GitLabClient', () => { }); it('traverses multi-page results', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -458,8 +503,10 @@ describe('GitLabClient', () => { describe('getGroupMembers', () => { it('gets member IDs', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -481,8 +528,10 @@ describe('GitLabClient', () => { it('gets member IDs with token without full permissions', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -497,8 +546,10 @@ describe('GitLabClient', () => { // TODO: is this one really necessary? it('rejects when GraphQL returns errors', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -512,8 +563,10 @@ describe('GitLabClient', () => { it('traverses multi-page results', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -528,8 +581,10 @@ describe('GitLabClient', () => { describe('getGroupById', () => { it('should return group details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -540,8 +595,10 @@ describe('GitLabClient', () => { it('should handle errors when fetching group details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -555,8 +612,10 @@ describe('GitLabClient', () => { describe('getProjectById', () => { it('should return project details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -567,8 +626,10 @@ describe('GitLabClient', () => { it('should handle errors when fetching project details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -582,8 +643,10 @@ describe('GitLabClient', () => { describe('getUserById', () => { it('should return user details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -594,8 +657,10 @@ describe('GitLabClient', () => { it('should handle errors when fetching user details by ID', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig( + new ConfigReader(mock.config_self_managed), + ), ), logger: mockServices.logger.mock(), }); @@ -610,8 +675,8 @@ describe('GitLabClient', () => { describe('paginated', () => { it('should iterate through the pages until exhausted', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); @@ -634,8 +699,8 @@ describe('hasFile', () => { beforeEach(() => { client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); @@ -655,8 +720,8 @@ describe('hasFile', () => { describe('pagedRequest search params', () => { it('no search params provided', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); @@ -673,8 +738,8 @@ describe('pagedRequest search params', () => { it('defined numeric search params', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); @@ -694,8 +759,8 @@ describe('pagedRequest search params', () => { it('defined string search params', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); @@ -715,8 +780,8 @@ describe('pagedRequest search params', () => { it('defined boolean search params', async () => { const client = new GitLabClient({ - config: readGitLabIntegrationConfig( - new ConfigReader(mock.config_self_managed), + integration: new GitLabIntegration( + readGitLabIntegrationConfig(new ConfigReader(mock.config_self_managed)), ), logger: mockServices.logger.mock(), }); diff --git a/plugins/catalog-backend-module-gitlab/src/lib/client.ts b/plugins/catalog-backend-module-gitlab/src/lib/client.ts index c04585142d..8515eeaad4 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/client.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/client.ts @@ -14,11 +14,9 @@ * limitations under the License. */ -// NOTE(freben): Intentionally uses node-fetch because of https://github.com/backstage/backstage/issues/28190 -import fetch from 'node-fetch'; - import { getGitLabRequestOptions, + GitLabIntegration, GitLabIntegrationConfig, } from '@backstage/integration'; import { LoggerService } from '@backstage/backend-plugin-api'; @@ -44,7 +42,7 @@ interface ListProjectOptions extends CommonListOptions { group?: string; membership?: boolean; topics?: string; - simple?: boolean; + last_activity_after?: string; } interface ListFilesOptions extends CommonListOptions { @@ -59,13 +57,15 @@ interface UserListOptions extends CommonListOptions { export class GitLabClient { private readonly config: GitLabIntegrationConfig; + private readonly integration: GitLabIntegration; private readonly logger: LoggerService; constructor(options: { - config: GitLabIntegrationConfig; + integration: GitLabIntegration; logger: LoggerService; }) { - this.config = options.config; + this.config = options.integration.config; + this.integration = options.integration; this.logger = options.logger; } @@ -125,6 +125,25 @@ export class GitLabClient { return response; } + async getProjectCommits( + projectId: number | string, + options?: { + since?: string; + until?: string; + ref_name?: string; + path?: string; + per_page?: number; + page?: number; + }, + ): Promise> { + const encodedProjectId = + typeof projectId === 'string' ? encodeURIComponent(projectId) : projectId; + return this.pagedRequest( + `/projects/${encodedProjectId}/repository/commits`, + options, + ); + } + async listGroupMembers( groupPath: string, options?: CommonListOptions, @@ -215,8 +234,8 @@ export class GitLabClient { let endCursor: string | null = null; do { - const response: GitLabDescendantGroupsResponse = - await this.fetchWithRetry(`${this.config.baseUrl}/api/graphql`, { + const response: GitLabDescendantGroupsResponse = await this.integration + .fetch(`${this.config.baseUrl}/api/graphql`, { method: 'POST', headers: { ...getGitLabRequestOptions(this.config).headers, @@ -247,7 +266,8 @@ export class GitLabClient { } `, }), - }).then(r => r.json()); + }) + .then(r => r.json()); if (response.errors) { throw new Error(`GraphQL errors: ${JSON.stringify(response.errors)}`); } @@ -289,9 +309,8 @@ export class GitLabClient { let hasNextPage: boolean = false; let endCursor: string | null = null; do { - const response: GitLabGroupMembersResponse = await this.fetchWithRetry( - `${this.config.baseUrl}/api/graphql`, - { + const response: GitLabGroupMembersResponse = await this.integration + .fetch(`${this.config.baseUrl}/api/graphql`, { method: 'POST', headers: { ...getGitLabRequestOptions(this.config).headers, @@ -331,8 +350,8 @@ export class GitLabClient { } `, }), - }, - ).then(r => r.json()); + }) + .then(r => r.json()); if (response.errors) { throw new Error(`GraphQL errors: ${JSON.stringify(response.errors)}`); } @@ -383,7 +402,7 @@ export class GitLabClient { const request = new URL(`${this.config.apiBaseUrl}${endpoint}`); request.searchParams.append('ref', branch); - const response = await this.fetchWithRetry(request.toString(), { + const response = await this.integration.fetch(request.toString(), { headers: getGitLabRequestOptions(this.config).headers, method: 'HEAD', }); @@ -402,6 +421,69 @@ export class GitLabClient { return true; } + async getCommitsTouchingFile( + projectPath: string, + filePath: string, + since?: string, + ): Promise<{ + added: boolean; + modified: boolean; + deleted: boolean; + }> { + const options: any = { + path: filePath, + per_page: 100, + }; + + if (since) { + options.since = since; + } + + try { + const commits = await this.getProjectCommits(projectPath, options); + + if (commits.items.length === 0) { + return { added: false, modified: false, deleted: false }; + } + + // Check if file exists now + const hasFileNow = await this.hasFile(projectPath, 'HEAD', filePath); + + if (hasFileNow) { + // File exists now - check if it existed before the 'since' date + // by getting all commits for this file and checking if the first one is after 'since' + const allCommitsForFile = await this.getProjectCommits(projectPath, { + path: filePath, + per_page: 1, + }); + + if (allCommitsForFile.items.length > 0 && since) { + const firstCommitDate = new Date( + allCommitsForFile.items[0].created_at, + ); + const sinceDate = new Date(since); + + if (firstCommitDate >= sinceDate) { + // First commit for this file is after our 'since' date -> file was added + return { added: true, modified: false, deleted: false }; + } + } + + // File existed before and exists now -> modified + return { added: false, modified: true, deleted: false }; + } + + // File doesn't exist now but we have commits -> deleted + return { added: false, modified: false, deleted: true }; + } catch (error) { + this.logger.warn( + `Error checking commits for file ${filePath} in project ${projectPath}: ${error}`, + ); + // On error, assume modified to be safe + return { added: false, modified: true, deleted: false }; + } + } + /** * Performs a request against a given paginated GitLab endpoint. * @@ -430,7 +512,7 @@ export class GitLabClient { } this.logger.debug(`Fetching: ${request.toString()}`); - const response = await this.fetchWithRetry( + const response = await this.integration.fetch( request.toString(), getGitLabRequestOptions(this.config), ); @@ -468,7 +550,7 @@ export class GitLabClient { } } - const response = await this.fetchWithRetry( + const response = await this.integration.fetch( request.toString(), getGitLabRequestOptions(this.config), ); @@ -483,48 +565,6 @@ export class GitLabClient { return response.json(); } - - /** - * Performs a fetch request with retry logic for rate limiting (429 errors) - * @param url - The URL to fetch - * @param options - Fetch options - * @param retries - Maximum number of retries - * @param initialBackoff - Initial backoff time in ms - */ - async fetchWithRetry( - url: string, - options: fetch.RequestInit, - retries = 5, - initialBackoff = 100, - ): Promise { - let currentRetry = 0; - let backoff = initialBackoff; - - for (;;) { - const response = await fetch(url, options); - - if (response.status !== 429 || currentRetry >= retries) { - return response; - } - - // Get retry-after header if available, or use exponential backoff - const retryAfter = response.headers.get('Retry-After'); - const waitTime = retryAfter ? parseInt(retryAfter, 10) * 1000 : backoff; - - this.logger.warn( - `GitLab API rate limit exceeded, retrying in ${waitTime}ms (retry ${ - currentRetry + 1 - }/${retries})`, - ); - - // Wait before retrying - await new Promise(resolve => setTimeout(resolve, waitTime)); - - // Exponential backoff with jitter - backoff = backoff * 2 * (0.8 + Math.random() * 0.4); - currentRetry++; - } - } } /** diff --git a/plugins/catalog-backend-module-gitlab/src/lib/index.ts b/plugins/catalog-backend-module-gitlab/src/lib/index.ts index cc82bd6a0b..7985d94b69 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/index.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/index.ts @@ -17,6 +17,7 @@ export { readGitlabConfigs } from '../providers/config'; export { GitLabClient, paginated } from './client'; export type { + GitLabCommit, GitLabGroup, GitLabGroupSamlIdentity, GitLabProject, diff --git a/plugins/catalog-backend-module-gitlab/src/lib/types.ts b/plugins/catalog-backend-module-gitlab/src/lib/types.ts index 0d00c956e5..c4defd9fcf 100644 --- a/plugins/catalog-backend-module-gitlab/src/lib/types.ts +++ b/plugins/catalog-backend-module-gitlab/src/lib/types.ts @@ -45,6 +45,21 @@ export type GitLabProject = { forked_from_project?: GitlabProjectForkedFrom; }; +export type GitLabCommit = { + id: string; + short_id: string; + title: string; + message: string; + created_at: string; + author_name: string; + author_email: string; + authored_date: string; + committed_date: string; + committer_name: string; + committer_email: string; + web_url: string; +}; + /** * Representation of a GitLab user in the GitLab API * diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts index 06b3f8c158..40936a4d77 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts @@ -135,7 +135,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { this.scheduleFn = this.createScheduleFn(options.taskRunner); this.events = options.events; this.gitLabClient = new GitLabClient({ - config: this.integration.config, + integration: this.integration, logger: this.logger, }); } diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts index 3b8720b5f2..37b446fd95 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts @@ -204,7 +204,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { : [this.config.groupPattern]; this.gitLabClient = new GitLabClient({ - config: this.integration.config, + integration: this.integration, logger: this.logger, }); } diff --git a/yarn.lock b/yarn.lock index 97a438ad8a..1e5cbb9910 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4079,6 +4079,7 @@ __metadata: lodash: "npm:^4.17.21" luxon: "npm:^3.0.0" msw: "npm:^1.0.0" + p-throttle: "npm:^4.1.1" languageName: unknown linkType: soft