From 5001de90886a151932a7cf5e3301e6a190e6bb29 Mon Sep 17 00:00:00 2001 From: "Chongyang Adrian, Ke" Date: Thu, 6 May 2021 12:42:00 +0800 Subject: [PATCH 1/2] GitlabUrlReader compare SHA commits for filepath if given Signed-off-by: Chongyang Adrian, Ke --- .changeset/short-ears-attend.md | 5 + .../src/reading/GitlabUrlReader.test.ts | 127 +++++++++++++----- .../src/reading/GitlabUrlReader.ts | 19 +-- 3 files changed, 109 insertions(+), 42 deletions(-) create mode 100644 .changeset/short-ears-attend.md diff --git a/.changeset/short-ears-attend.md b/.changeset/short-ears-attend.md new file mode 100644 index 0000000000..45115b7c8c --- /dev/null +++ b/.changeset/short-ears-attend.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Change GitlabUrlReader to SHA timestamp compare using only commits that modify given file path, if file path given diff --git a/packages/backend-common/src/reading/GitlabUrlReader.test.ts b/packages/backend-common/src/reading/GitlabUrlReader.test.ts index 9dcc92a4a6..105a0db159 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.test.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.test.ts @@ -190,11 +190,17 @@ describe('GitlabUrlReader', () => { default_branch: 'main', }; - const branchGitlabApiResponse = { - commit: { + const commitsGitlabApiResponse = [ + { id: 'sha123abc', }, - }; + ]; + + const specificPathCommitsGitlabApiResponse = [ + { + id: 'sha456def', + }, + ]; beforeEach(() => { worker.use( @@ -221,17 +227,29 @@ describe('GitlabUrlReader', () => { ), ), rest.get( - 'https://gitlab.com/api/v4/projects/backstage%2Fmock/repository/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(branchGitlabApiResponse), - ), - ), - rest.get( - 'https://gitlab.com/api/v4/projects/backstage%2Fmock/repository/branches/branchDoesNotExist', - (_, res, ctx) => res(ctx.status(404)), + 'https://gitlab.com/api/v4/projects/backstage%2Fmock/repository/commits', + (req, res, ctx) => { + const refName = req.url.searchParams.get('ref_name'); + if (refName === 'main') { + const filepath = req.url.searchParams.get('path'); + if (filepath === 'testFilepath') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(specificPathCommitsGitlabApiResponse), + ); + } + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(commitsGitlabApiResponse), + ); + } + if (refName === 'branchDoesNotExist') { + return res(ctx.status(404)); + } + return res(); + }, ), rest.get( 'https://gitlab.mycompany.com/api/v4/projects/backstage%2Fmock', @@ -243,13 +261,26 @@ describe('GitlabUrlReader', () => { ), ), rest.get( - 'https://gitlab.mycompany.com/api/v4/projects/backstage%2Fmock/repository/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(branchGitlabApiResponse), - ), + 'https://gitlab.mycompany.com/api/v4/projects/backstage%2Fmock/repository/commits', + (req, res, ctx) => { + const refName = req.url.searchParams.get('ref_name'); + if (refName === 'main') { + const filepath = req.url.searchParams.get('path'); + if (filepath === 'testFilepath') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(specificPathCommitsGitlabApiResponse), + ); + } + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(commitsGitlabApiResponse), + ); + } + return res(); + }, ), rest.get( 'https://gitlab.mycompany.com/api/v4/projects/backstage%2Fmock/repository/archive.zip?sha=main', @@ -351,7 +382,7 @@ describe('GitlabUrlReader', () => { ).resolves.toBe('# Test\n'); }); - it('throws a NotModifiedError when given a etag in options', async () => { + it('throws a NotModifiedError when given a etag in options matching last commit', async () => { const fnGitlab = async () => { await gitlabProcessor.readTree('https://gitlab.com/backstage/mock', { etag: 'sha123abc', @@ -371,6 +402,29 @@ describe('GitlabUrlReader', () => { await expect(fnHostedGitlab).rejects.toThrow(NotModifiedError); }); + it('throws a NotModifiedError when given a etag in options matching last commit affecting specified filepath', async () => { + const fnGitlab = async () => { + await gitlabProcessor.readTree( + 'https://gitlab.com/backstage/mock/blob/main/testFilepath', + { + etag: 'sha456def', + }, + ); + }; + + const fnHostedGitlab = async () => { + await hostedGitlabProcessor.readTree( + 'https://gitlab.mycompany.com/backstage/mock/blob/main/testFilepath', + { + etag: 'sha456def', + }, + ); + }; + + await expect(fnGitlab).rejects.toThrow(NotModifiedError); + await expect(fnHostedGitlab).rejects.toThrow(NotModifiedError); + }); + it('should not throw error when given an outdated etag in options', async () => { const response = await gitlabProcessor.readTree( 'https://gitlab.com/backstage/mock/tree/main', @@ -389,12 +443,12 @@ describe('GitlabUrlReader', () => { }); it('should throw error on missing branch', async () => { - const fnGithub = async () => { + const fnGitlab = async () => { await gitlabProcessor.readTree( 'https://gitlab.com/backstage/mock/tree/branchDoesNotExist', ); }; - await expect(fnGithub).rejects.toThrow(NotFoundError); + await expect(fnGitlab).rejects.toThrow(NotFoundError); }); }); @@ -408,11 +462,11 @@ describe('GitlabUrlReader', () => { default_branch: 'main', }; - const branchGitlabApiResponse = { - commit: { + const commitsGitlabApiResponse = [ + { id: 'sha123abc', }, - }; + ]; beforeEach(() => { worker.use( @@ -439,13 +493,18 @@ describe('GitlabUrlReader', () => { ), ), rest.get( - 'https://gitlab.com/api/v4/projects/backstage%2Fmock/repository/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(branchGitlabApiResponse), - ), + 'https://gitlab.com/api/v4/projects/backstage%2Fmock/repository/commits', + (req, res, ctx) => { + const refName = req.url.searchParams.get('ref_name'); + if (refName === 'main') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(commitsGitlabApiResponse), + ); + } + return res(); + }, ), ); }); diff --git a/packages/backend-common/src/reading/GitlabUrlReader.ts b/packages/backend-common/src/reading/GitlabUrlReader.ts index c93103b2da..a9365e1b09 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.ts @@ -106,25 +106,28 @@ export class GitlabUrlReader implements UrlReader { // ref is an empty string if no branch is set in provided url to readTree. const branch = ref || projectGitlabResponseJson.default_branch; - // Fetch the latest commit in the provided or default branch to compare against - // the provided sha. - const branchGitlabResponse = await fetch( + // Fetch the latest commit that modifies the the filepath in the provided or default branch + // to compare against the provided sha. + const branchQuery = `ref_name=${branch}`; + const pathQuery = !!filepath ? `&path=${filepath}` : ''; + + const commitsGitlabResponse = await fetch( new URL( `${this.integration.config.apiBaseUrl}/projects/${encodeURIComponent( full_name, - )}/repository/branches/${branch}`, + )}/repository/commits?${branchQuery}${pathQuery}`, ).toString(), getGitLabRequestOptions(this.integration.config), ); - if (!branchGitlabResponse.ok) { - const message = `Failed to read tree (branch) from ${url}, ${branchGitlabResponse.status} ${branchGitlabResponse.statusText}`; - if (branchGitlabResponse.status === 404) { + if (!commitsGitlabResponse.ok) { + const message = `Failed to read tree (branch) from ${url}, ${commitsGitlabResponse.status} ${commitsGitlabResponse.statusText}`; + if (commitsGitlabResponse.status === 404) { throw new NotFoundError(message); } throw new Error(message); } - const commitSha = (await branchGitlabResponse.json()).commit.id; + const commitSha = (await commitsGitlabResponse.json())[0].id; if (options?.etag && options.etag === commitSha) { throw new NotModifiedError(); From b57db403ac95eaa7a67f0a346af712368e6450d7 Mon Sep 17 00:00:00 2001 From: "Chongyang Adrian, Ke" Date: Tue, 18 May 2021 10:32:38 +0800 Subject: [PATCH 2/2] use UrlSearchParams Signed-off-by: Chongyang Adrian, Ke --- packages/backend-common/src/reading/GitlabUrlReader.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/backend-common/src/reading/GitlabUrlReader.ts b/packages/backend-common/src/reading/GitlabUrlReader.ts index a9365e1b09..9dcbd5783c 100644 --- a/packages/backend-common/src/reading/GitlabUrlReader.ts +++ b/packages/backend-common/src/reading/GitlabUrlReader.ts @@ -108,14 +108,16 @@ export class GitlabUrlReader implements UrlReader { // Fetch the latest commit that modifies the the filepath in the provided or default branch // to compare against the provided sha. - const branchQuery = `ref_name=${branch}`; - const pathQuery = !!filepath ? `&path=${filepath}` : ''; - + const commitsReqParams = new URLSearchParams(); + commitsReqParams.set('ref_name', branch); + if (!!filepath) { + commitsReqParams.set('path', filepath); + } const commitsGitlabResponse = await fetch( new URL( `${this.integration.config.apiBaseUrl}/projects/${encodeURIComponent( full_name, - )}/repository/commits?${branchQuery}${pathQuery}`, + )}/repository/commits?${commitsReqParams.toString()}`, ).toString(), getGitLabRequestOptions(this.integration.config), );