From 75f1f7bd45a396787366e8614df4ed4ca6fc00aa Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Fri, 3 Jun 2022 10:14:19 +0200 Subject: [PATCH 1/7] fix: do not send empty timestamp The `last_activity_after` timestamp was being sent as current time when the cache is empty. Now it should not be sent at all and as we assert in the test. Fixes #10399 Signed-off-by: Zee V. (Philip) --- .../src/GitLabDiscoveryProcessor.test.ts | 59 ++++++++++++------- .../src/GitLabDiscoveryProcessor.ts | 26 +++++--- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts index 278efe8036..1b52fea7b5 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts @@ -17,7 +17,7 @@ import { getVoidLogger } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; import { LocationSpec } from '@backstage/plugin-catalog-backend'; -import { rest } from 'msw'; +import { rest, RestRequest } from 'msw'; import { setupServer } from 'msw/node'; import { GitLabDiscoveryProcessor, parseUrl } from './GitLabDiscoveryProcessor'; import { GitLabProject } from './lib'; @@ -48,6 +48,7 @@ const GROUP_LOCATION_CUSTOM_BRANCH: LocationSpec = { type: 'gitlab-discovery', target: `${SERVER_URL}/group/subgroup/blob/test/catalog-info.yaml`, }; +const SERVER_TIME = '2001-01-01T12:34:56.000Z'; function setupFakeServer( url: string, @@ -58,9 +59,15 @@ function setupFakeServer( data: GitLabProject[]; nextPage?: number; }, + assertion?: (r: RestRequest) => any, ) { server.use( rest.get(url, (req, res, ctx) => { + // Send the request to the assertion to give the test an opportunity to inspect the parameters. + if (assertion !== undefined) { + assertion(req) + } + if (req.headers.get('private-token') !== 'test-token') { return res(ctx.status(401), ctx.json({})); } @@ -145,7 +152,7 @@ describe('GitlabDiscoveryProcessor', () => { beforeAll(() => { server.listen(); jest.useFakeTimers('modern'); - jest.setSystemTime(new Date('2001-01-01T12:34:56Z')); + jest.setSystemTime(new Date(SERVER_TIME)); }); afterEach(() => server.resetHandlers()); afterAll(() => { @@ -394,33 +401,35 @@ describe('GitlabDiscoveryProcessor', () => { }); it('uses the previous scan timestamp to filter', async () => { + const payload = { + data: [ + { + id: 1, + archived: false, + default_branch: 'main', + last_activity_at: '2000-01-01T00:00:00Z', + web_url: 'https://gitlab.fake/1', + }, + { + id: 2, + archived: false, + default_branch: 'main', + last_activity_at: '2002-01-01T00:00:00Z', + web_url: 'https://gitlab.fake/2', + }, + ], + }; const processor = getProcessor(); setupFakeServer(PROJECTS_URL, request => { switch (request.page) { case 1: - return { - data: [ - { - id: 1, - archived: false, - default_branch: 'main', - last_activity_at: '2000-01-01T00:00:00Z', - web_url: 'https://gitlab.fake/1', - path_with_namespace: '1', - }, - { - id: 2, - archived: false, - default_branch: 'main', - last_activity_at: '2002-01-01T00:00:00Z', - web_url: 'https://gitlab.fake/2', - path_with_namespace: '2', - }, - ], - }; + return payload; default: throw new Error('Invalid request'); } + }, request => { + // We assert that the last activity timestamp is not being sent to the GitLab API as we expect the cache to be empty. + expect(request.url.searchParams.get('last_activity_after')).toBeNull() }); const result: any[] = []; @@ -433,6 +442,12 @@ describe('GitlabDiscoveryProcessor', () => { // Second scan should have used the mocked Date to set the last scanned time to 2001 // This should result in only the second repo being scanned, since that has a timestamp of 2002 + setupFakeServer(PROJECTS_URL, _ => { + return payload; + }, request => { + // We assert that the last activity timestamp is being sent to the GitLab API since we expect it to be in the cache. + expect(request.url.searchParams.get('last_activity_after')).toMatch(SERVER_TIME) + }); const result2: any[] = []; await processor.readLocation(PROJECT_LOCATION, false, e => { result2.push(e); diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 73b8c52358..2c94826b88 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -100,11 +100,22 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { const startTimestamp = Date.now(); this.logger.debug(`Reading GitLab projects from ${location.target}`); - const projects = paginated(options => client.listProjects(options), { + + const opts = { group, - last_activity_after: await this.updateLastActivity(), page: 1, - }); + } as { + group: string + page: number + last_activity_after: string | undefined + } + + const lastActivity = await this.cache.get(this.getCacheKey()) as string; + if (lastActivity !== "") { + opts.last_activity_after = lastActivity + } + + const projects = paginated(options => client.listProjects(options), opts); const res: Result = { scanned: 0, @@ -156,6 +167,8 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { ); } + await this.cache.set(this.getCacheKey(), new Date().toISOString()); + const duration = ((Date.now() - startTimestamp) / 1000).toFixed(1); this.logger.debug( `Read ${res.scanned} GitLab repositories in ${duration} seconds`, @@ -164,11 +177,8 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { return true; } - private async updateLastActivity(): Promise { - const cacheKey = `processors/${this.getProcessorName()}/last-activity`; - const lastActivity = await this.cache.get(cacheKey); - await this.cache.set(cacheKey, new Date().toISOString()); - return lastActivity as string | undefined; + private getCacheKey(): string { + return `processors/${this.getProcessorName()}/last-activity`; } } From 3bf6fa5fa840cb6275bf25e2ead36a13a202151e Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Fri, 3 Jun 2022 10:15:49 +0200 Subject: [PATCH 2/7] chore: run prettier to tidy up Signed-off-by: Zee V. (Philip) --- .../src/GitLabDiscoveryProcessor.test.ts | 51 ++++++++++++------- .../src/GitLabDiscoveryProcessor.ts | 17 +++---- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts index 1b52fea7b5..341167f279 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.test.ts @@ -65,9 +65,9 @@ function setupFakeServer( rest.get(url, (req, res, ctx) => { // Send the request to the assertion to give the test an opportunity to inspect the parameters. if (assertion !== undefined) { - assertion(req) + assertion(req); } - + if (req.headers.get('private-token') !== 'test-token') { return res(ctx.status(401), ctx.json({})); } @@ -420,17 +420,24 @@ describe('GitlabDiscoveryProcessor', () => { ], }; const processor = getProcessor(); - setupFakeServer(PROJECTS_URL, request => { - switch (request.page) { - case 1: - return payload; - default: - throw new Error('Invalid request'); - } - }, request => { - // We assert that the last activity timestamp is not being sent to the GitLab API as we expect the cache to be empty. - expect(request.url.searchParams.get('last_activity_after')).toBeNull() - }); + + setupFakeServer( + PROJECTS_URL, + request => { + switch (request.page) { + case 1: + return payload; + default: + throw new Error('Invalid request'); + } + }, + request => { + // We assert that the last activity timestamp is not being sent to the GitLab API as we expect the cache to be empty. + expect( + request.url.searchParams.get('last_activity_after'), + ).toBeNull(); + }, + ); const result: any[] = []; @@ -442,12 +449,18 @@ describe('GitlabDiscoveryProcessor', () => { // Second scan should have used the mocked Date to set the last scanned time to 2001 // This should result in only the second repo being scanned, since that has a timestamp of 2002 - setupFakeServer(PROJECTS_URL, _ => { - return payload; - }, request => { - // We assert that the last activity timestamp is being sent to the GitLab API since we expect it to be in the cache. - expect(request.url.searchParams.get('last_activity_after')).toMatch(SERVER_TIME) - }); + setupFakeServer( + PROJECTS_URL, + _ => { + return payload; + }, + request => { + // We assert that the last activity timestamp is being sent to the GitLab API since we expect it to be in the cache. + expect(request.url.searchParams.get('last_activity_after')).toMatch( + SERVER_TIME, + ); + }, + ); const result2: any[] = []; await processor.readLocation(PROJECT_LOCATION, false, e => { result2.push(e); diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 2c94826b88..8fb6fbe4da 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -100,19 +100,18 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { const startTimestamp = Date.now(); this.logger.debug(`Reading GitLab projects from ${location.target}`); - const opts = { group, page: 1, } as { - group: string - page: number - last_activity_after: string | undefined - } - - const lastActivity = await this.cache.get(this.getCacheKey()) as string; - if (lastActivity !== "") { - opts.last_activity_after = lastActivity + group: string; + page: number; + last_activity_after: string | undefined; + }; + + const lastActivity = (await this.cache.get(this.getCacheKey())) as string; + if (lastActivity !== '') { + opts.last_activity_after = lastActivity; } const projects = paginated(options => client.listProjects(options), opts); From bad907d794ff5c34323f6cfe241bd1de9d071198 Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Tue, 3 May 2022 13:44:01 +0200 Subject: [PATCH 3/7] chore: include a changeset Signed-off-by: Zee V. (Philip) --- .changeset/long-spiders-wave.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/long-spiders-wave.md diff --git a/.changeset/long-spiders-wave.md b/.changeset/long-spiders-wave.md new file mode 100644 index 0000000000..8c836621db --- /dev/null +++ b/.changeset/long-spiders-wave.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-gitlab': patch +--- + +The `last_activity_after` timestamp is now being omitted when querying the GitLab API for the first time. From 7819f995debfbc71843a4ef831a6e4c4e298cdd0 Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Mon, 16 May 2022 15:53:36 +0200 Subject: [PATCH 4/7] =?UTF-8?q?fix:=20check=20if=20lastActivity=20is=20?= =?UTF-8?q?=E2=80=9Ctruthy=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do this rather than checking that it’s not an empty string to make sure we capture the case where it might be undefined. Signed-off-by: Zee V. (Philip) --- .../src/GitLabDiscoveryProcessor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 8fb6fbe4da..7f81683408 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -110,7 +110,7 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { }; const lastActivity = (await this.cache.get(this.getCacheKey())) as string; - if (lastActivity !== '') { + if (lastActivity) { opts.last_activity_after = lastActivity; } From f8d0c432bb9b003c352c7139aff9c564d5e4edbd Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Fri, 3 Jun 2022 10:09:38 +0200 Subject: [PATCH 5/7] fix: ensure we do not have a race condition We achieve this by setting the start time prior to making the request. Signed-off-by: Zee V. (Philip) --- .../src/GitLabDiscoveryProcessor.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 7f81683408..a23d1f2803 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -84,6 +84,7 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { return false; } + const startTime = new Date(); const { group, host, branch, catalogPath } = parseUrl(location.target); const integration = this.integrations.gitlab.byUrl(`https://${host}`); @@ -97,7 +98,6 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { config: integration.config, logger: this.logger, }); - const startTimestamp = Date.now(); this.logger.debug(`Reading GitLab projects from ${location.target}`); const opts = { @@ -110,6 +110,8 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { }; const lastActivity = (await this.cache.get(this.getCacheKey())) as string; + // We check for the existence of lastActivity and only set it if it's present to ensure + // that the options doesn't include the key so that the API doesn't receive an empty query parameter. if (lastActivity) { opts.last_activity_after = lastActivity; } @@ -166,9 +168,10 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { ); } - await this.cache.set(this.getCacheKey(), new Date().toISOString()); + // Save an ISO formatted string in the cache as that's what GitLab expects in the API request. + await this.cache.set(this.getCacheKey(), startTime.toISOString()); - const duration = ((Date.now() - startTimestamp) / 1000).toFixed(1); + const duration = ((Date.now() - startTime.getTime()) / 1000).toFixed(1); this.logger.debug( `Read ${res.scanned} GitLab repositories in ${duration} seconds`, ); From 4e2028456654f6b1c9805e5f6a3d8f57b60e7037 Mon Sep 17 00:00:00 2001 From: "Zee V. (Philip)" Date: Tue, 7 Jun 2022 10:34:42 +0200 Subject: [PATCH 6/7] refactor: clean up construction of opts Signed-off-by: Zee V. (Philip) --- .../src/GitLabDiscoveryProcessor.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index a23d1f2803..25240d2d14 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -100,20 +100,13 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { }); this.logger.debug(`Reading GitLab projects from ${location.target}`); + const lastActivity = (await this.cache.get(this.getCacheKey())) as string; const opts = { group, page: 1, - } as { - group: string; - page: number; - last_activity_after: string | undefined; - }; - - const lastActivity = (await this.cache.get(this.getCacheKey())) as string; - // We check for the existence of lastActivity and only set it if it's present to ensure - // that the options doesn't include the key so that the API doesn't receive an empty query parameter. - if (lastActivity) { - opts.last_activity_after = lastActivity; + // We check for the existence of lastActivity and only set it if it's present to ensure + // that the options doesn't include the key so that the API doesn't receive an empty query parameter. + ...(lastActivity && { last_activity_after: lastActivity }), } const projects = paginated(options => client.listProjects(options), opts); From 32807d5a8e6d30ace8f9913138e14c9d839ee807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 8 Jun 2022 17:09:48 +0200 Subject: [PATCH 7/7] fixup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .../src/GitLabDiscoveryProcessor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts index 25240d2d14..b03c665ebc 100644 --- a/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts +++ b/plugins/catalog-backend-module-gitlab/src/GitLabDiscoveryProcessor.ts @@ -107,7 +107,7 @@ export class GitLabDiscoveryProcessor implements CatalogProcessor { // We check for the existence of lastActivity and only set it if it's present to ensure // that the options doesn't include the key so that the API doesn't receive an empty query parameter. ...(lastActivity && { last_activity_after: lastActivity }), - } + }; const projects = paginated(options => client.listProjects(options), opts);