From 64dd0b8699bcb5832e2c6367c6a174d07be17568 Mon Sep 17 00:00:00 2001 From: sonikro Date: Tue, 10 Dec 2024 20:28:29 +0000 Subject: [PATCH] feat(GitHubOrgEntityProvider): adds throttling and rate limit to GithubOrgEntityProvider Signed-off-by: sonikro --- .changeset/spicy-trainers-search.md | 5 + .../package.json | 4 +- .../src/lib/github.test.ts | 92 ++++++++++++++++++- .../src/lib/github.ts | 62 ++++++++++++- .../providers/GithubOrgEntityProvider.test.ts | 15 ++- .../src/providers/GithubOrgEntityProvider.ts | 15 ++- yarn.lock | 82 +++++++++-------- 7 files changed, 222 insertions(+), 53 deletions(-) create mode 100644 .changeset/spicy-trainers-search.md diff --git a/.changeset/spicy-trainers-search.md b/.changeset/spicy-trainers-search.md new file mode 100644 index 0000000000..9c6131bb33 --- /dev/null +++ b/.changeset/spicy-trainers-search.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-github': patch +--- + +fixes GithubOrgEntityProvider not handling throttling and rate limit errors correctly diff --git a/plugins/catalog-backend-module-github/package.json b/plugins/catalog-backend-module-github/package.json index ce731cd8c9..f43129b0ad 100644 --- a/plugins/catalog-backend-module-github/package.json +++ b/plugins/catalog-backend-module-github/package.json @@ -61,7 +61,9 @@ "@backstage/plugin-catalog-common": "workspace:^", "@backstage/plugin-catalog-node": "workspace:^", "@backstage/plugin-events-node": "workspace:^", - "@octokit/graphql": "^5.0.0", + "@octokit/core": "^5.2.0", + "@octokit/graphql": "^7.0.2", + "@octokit/plugin-throttling": "^8.1.3", "@octokit/rest": "^19.0.3", "git-url-parse": "^15.0.0", "lodash": "^4.17.21", diff --git a/plugins/catalog-backend-module-github/src/lib/github.test.ts b/plugins/catalog-backend-module-github/src/lib/github.test.ts index cbe8d8c525..ef52007f50 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.test.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.test.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { registerMswTestHooks } from '@backstage/backend-test-utils'; +import { + mockServices, + registerMswTestHooks, +} from '@backstage/backend-test-utils'; import { GroupEntity, UserEntity } from '@backstage/catalog-model'; import { graphql as graphqlOctokit } from '@octokit/graphql'; import { graphql as graphqlMsw, HttpResponse } from 'msw'; @@ -32,7 +35,17 @@ import { createAddEntitiesOperation, createRemoveEntitiesOperation, createReplaceEntitiesOperation, + createGraphqlClient, } from './github'; +import { Octokit } from '@octokit/core'; +import { throttling } from '@octokit/plugin-throttling'; + +jest.mock('@octokit/core', () => ({ + ...jest.requireActual('@octokit/core'), + Octokit: { + plugin: jest.fn().mockReturnValue({ defaults: jest.fn() }), + }, +})); describe('github', () => { const server = setupServer(); @@ -699,4 +712,81 @@ describe('github', () => { }); }); }); + + describe('createGraphqlClient', () => { + const headers = {}; + + const baseUrl = 'https://api.github.com'; + + const logger = mockServices.rootLogger(); + + const mockClient = jest.fn().mockImplementation(); + + const graphqlDefaults = jest.fn().mockReturnValue(mockClient); + const mockedOctokit = jest.fn().mockImplementation(() => ({ + graphql: { + defaults: graphqlDefaults, + }, + })); + (Octokit.plugin as jest.Mock).mockReturnValue(mockedOctokit); + + const rateLimitOptions = { + method: 'POST', + url: '/graphql', + }; + const client = createGraphqlClient({ + headers, + baseUrl, + logger, + }); + it('should return a graphql client with throttling', async () => { + expect(client).toBeDefined(); + expect(Octokit.plugin).toHaveBeenCalledWith(throttling); + }); + + it('should return a graphql client with the correct options', async () => { + expect(graphqlDefaults).toHaveBeenCalledWith({ + baseUrl, + headers, + }); + }); + + describe('onRateLimit', () => { + it.each([ + { retryCount: 0, expectedResult: true }, + { retryCount: 1, expectedResult: true }, + { retryCount: 2, expectedResult: false }, + ])('should return %s', async ({ retryCount, expectedResult }) => { + const throttleOptions = mockedOctokit.mock.calls[0][0].throttle; + + const result = throttleOptions.onRateLimit( + 60, + rateLimitOptions, + undefined, + retryCount, + ); + + expect(result).toBe(expectedResult); + }); + }); + + describe('onSecondaryRateLimit', () => { + it.each([ + { retryCount: 0, expectedResult: true }, + { retryCount: 1, expectedResult: true }, + { retryCount: 2, expectedResult: false }, + ])('should return %s', async ({ retryCount, expectedResult }) => { + const throttleOptions = mockedOctokit.mock.calls[0][0].throttle; + + const result = throttleOptions.onSecondaryRateLimit( + 60, + rateLimitOptions, + undefined, + retryCount, + ); + + expect(result).toBe(expectedResult); + }); + }); + }); }); diff --git a/plugins/catalog-backend-module-github/src/lib/github.ts b/plugins/catalog-backend-module-github/src/lib/github.ts index cba65e522e..bb30da136c 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.ts @@ -27,7 +27,9 @@ import { import { withLocations } from './withLocations'; import { DeferredEntity } from '@backstage/plugin-catalog-node'; - +import { Octokit } from '@octokit/core'; +import { LoggerService } from '@backstage/backend-plugin-api'; +import { throttling } from '@octokit/plugin-throttling'; // Graphql types export type QueryResponse = { @@ -710,3 +712,61 @@ export const createReplaceEntitiesOperation = added: entitiesToReplace, }; }; + +/** + * Creates a GraphQL Client with Throttling + */ +export const createGraphqlClient = (args: { + headers: + | { + [name: string]: string; + } + | undefined; + baseUrl: string; + logger: LoggerService; +}): typeof graphql => { + const { headers, baseUrl, logger } = args; + const MyOctokit = Octokit.plugin(throttling); + const octokit = new MyOctokit({ + throttle: { + onRateLimit: (retryAfter, rateLimitOptions, _, retryCount) => { + const rateLimitData = rateLimitOptions as any; + logger.warn( + `Request quota exhausted for request ${rateLimitData?.method} ${rateLimitData?.url}`, + ); + + if (retryCount < 2) { + logger.warn( + `Retrying after ${retryAfter} seconds for the ${retryCount} time due to Rate Limit!`, + ); + return true; + } + + return false; + }, + onSecondaryRateLimit: (retryAfter, rateLimitOptions, _, retryCount) => { + const rateLimitData = rateLimitOptions as any; + logger.warn( + `Secondary Rate Limit Exhausted for request ${rateLimitData?.method} ${rateLimitData?.url}`, + ); + + if (retryCount < 2) { + logger.warn( + `Retrying after ${retryAfter} seconds for the ${retryCount} time due to Secondary Rate Limit!`, + ); + return true; + } + + return false; + }, + }, + }); + + const client = octokit.graphql.defaults({ + headers, + baseUrl, + }); + + // Since the project has multiple versions of @octokit/graphql, we need to cast the client to the correct type + return client as unknown as typeof graphql; +}; diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts index 52dc2b6d14..17fa00b137 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts @@ -14,32 +14,37 @@ * limitations under the License. */ +import { mockServices } from '@backstage/backend-test-utils'; import { GroupEntity, UserEntity } from '@backstage/catalog-model'; import { GithubCredentialsProvider, GithubIntegrationConfig, } from '@backstage/integration'; import { EntityProviderConnection } from '@backstage/plugin-catalog-node'; -import { graphql } from '@octokit/graphql'; import { DefaultEventsService, EventParams, } from '@backstage/plugin-events-node'; -import { GithubOrgEntityProvider } from './GithubOrgEntityProvider'; +import { graphql } from '@octokit/graphql'; +import { createGraphqlClient } from '../lib/github'; import { withLocations } from '../lib/withLocations'; -import { mockServices } from '@backstage/backend-test-utils'; +import { GithubOrgEntityProvider } from './GithubOrgEntityProvider'; jest.mock('@octokit/graphql'); +jest.mock('../lib/github', () => ({ + ...jest.requireActual('../lib/github'), + createGraphqlClient: jest.fn(), +})); describe('GithubOrgEntityProvider', () => { describe('read', () => { - let mockClient; + let mockClient: any; let entityProviderConnection: EntityProviderConnection; let entityProvider: GithubOrgEntityProvider; const setupMocks = (response: ((...args: any) => any) | undefined) => { mockClient = jest.fn().mockImplementation(response); - (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + (createGraphqlClient as jest.Mock).mockReturnValue(mockClient); }; beforeEach(() => { diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts index 6e83b8d65f..a8dc045567 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { SchedulerServiceTaskRunner } from '@backstage/backend-plugin-api'; +import { + LoggerService, + SchedulerServiceTaskRunner, +} from '@backstage/backend-plugin-api'; import { Entity, isGroupEntity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { @@ -47,6 +50,7 @@ import { } from '../lib/defaultTransformers'; import { createAddEntitiesOperation, + createGraphqlClient, createRemoveEntitiesOperation, createReplaceEntitiesOperation, DeferredEntitiesBuilder, @@ -56,11 +60,10 @@ import { getOrganizationUsers, GithubTeam, } from '../lib/github'; +import { areGroupEntities, areUserEntities } from '../lib/guards'; import { assignGroupsToUsers, buildOrgHierarchy } from '../lib/org'; import { parseGithubOrgUrl } from '../lib/util'; import { withLocations } from '../lib/withLocations'; -import { areGroupEntities, areUserEntities } from '../lib/guards'; -import { LoggerService } from '@backstage/backend-plugin-api'; const EVENT_TOPICS = [ 'github.membership', @@ -220,9 +223,11 @@ export class GithubOrgEntityProvider implements EntityProvider { await this.credentialsProvider.getCredentials({ url: this.options.orgUrl, }); - const client = graphql.defaults({ - baseUrl: this.options.gitHubConfig.apiBaseUrl, + + const client = createGraphqlClient({ headers, + baseUrl: this.options.gitHubConfig.apiBaseUrl!, + logger, }); const { org } = parseGithubOrgUrl(this.options.orgUrl); diff --git a/yarn.lock b/yarn.lock index f40ec341af..d6e9f09dad 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6006,7 +6006,9 @@ __metadata: "@backstage/plugin-catalog-common": "workspace:^" "@backstage/plugin-catalog-node": "workspace:^" "@backstage/plugin-events-node": "workspace:^" - "@octokit/graphql": ^5.0.0 + "@octokit/core": ^5.2.0 + "@octokit/graphql": ^7.0.2 + "@octokit/plugin-throttling": ^8.1.3 "@octokit/rest": ^19.0.3 "@types/lodash": ^4.14.151 git-url-parse: ^15.0.0 @@ -13017,18 +13019,18 @@ __metadata: languageName: node linkType: hard -"@octokit/core@npm:^5.0.0": - version: 5.0.2 - resolution: "@octokit/core@npm:5.0.2" +"@octokit/core@npm:^5.0.0, @octokit/core@npm:^5.2.0": + version: 5.2.0 + resolution: "@octokit/core@npm:5.2.0" dependencies: "@octokit/auth-token": ^4.0.0 - "@octokit/graphql": ^7.0.0 - "@octokit/request": ^8.0.2 - "@octokit/request-error": ^5.0.0 - "@octokit/types": ^12.0.0 + "@octokit/graphql": ^7.1.0 + "@octokit/request": ^8.3.1 + "@octokit/request-error": ^5.1.0 + "@octokit/types": ^13.0.0 before-after-hook: ^2.2.0 universal-user-agent: ^6.0.0 - checksum: 9ce060d61577f6805901ae5c33b2764a441db119ae0cca09104adf37b119cce68b656220de56c0c5004c9c9c1c892a7fdfbe9c0b1f5e398cb359dfd39c57eca8 + checksum: 57d5f02b759b569323dcb76cc72bf94ea7d0de58638c118ee14ec3e37d303c505893137dd72918328794844f35c74b3cd16999319c4b40d410a310d44a9b7566 languageName: node linkType: hard @@ -13054,13 +13056,13 @@ __metadata: languageName: node linkType: hard -"@octokit/endpoint@npm:^9.0.0": - version: 9.0.4 - resolution: "@octokit/endpoint@npm:9.0.4" +"@octokit/endpoint@npm:^9.0.1": + version: 9.0.5 + resolution: "@octokit/endpoint@npm:9.0.5" dependencies: - "@octokit/types": ^12.0.0 + "@octokit/types": ^13.1.0 universal-user-agent: ^6.0.0 - checksum: ed1b64a448f478e5951a043ef816d634a5a1f584519cbf2f374ceac058f82a16e52f078f156aa8b8cbcab7b0590348d94294fc83c9b4eebd42a820a5f10db81c + checksum: d5cc2df9bd4603844c163eea05eec89c677cfe699c6f065fe86b83123e34554ec16d429e8142dec1e2b4cf56591ef0ce5b1763f250c87bc8e7bf6c74ba59ae82 languageName: node linkType: hard @@ -13085,14 +13087,14 @@ __metadata: languageName: node linkType: hard -"@octokit/graphql@npm:^7.0.0": - version: 7.0.2 - resolution: "@octokit/graphql@npm:7.0.2" +"@octokit/graphql@npm:^7.0.2, @octokit/graphql@npm:^7.1.0": + version: 7.1.0 + resolution: "@octokit/graphql@npm:7.1.0" dependencies: - "@octokit/request": ^8.0.1 - "@octokit/types": ^12.0.0 + "@octokit/request": ^8.3.0 + "@octokit/types": ^13.0.0 universal-user-agent: ^6.0.0 - checksum: 05a752c4c2d84fc2900d8e32e1c2d1ee98a5a14349e651cb1109d0741e821e7417a048b1bb40918534ed90a472314aabbda35688868016f248098925f82a3bfa + checksum: 7b2706796e0269fc033ed149ea211117bcacf53115fd142c1eeafc06ebc5b6290e4e48c03d6276c210d72e3695e8598f83caac556cd00714fc1f8e4707d77448 languageName: node linkType: hard @@ -13319,15 +13321,15 @@ __metadata: languageName: node linkType: hard -"@octokit/plugin-throttling@npm:^8.0.0": - version: 8.1.3 - resolution: "@octokit/plugin-throttling@npm:8.1.3" +"@octokit/plugin-throttling@npm:^8.0.0, @octokit/plugin-throttling@npm:^8.1.3": + version: 8.2.0 + resolution: "@octokit/plugin-throttling@npm:8.2.0" dependencies: "@octokit/types": ^12.2.0 bottleneck: ^2.15.3 peerDependencies: "@octokit/core": ^5.0.0 - checksum: 98963ef2eab825015702b1ca1ef4ccbda0c009242e93001144e51014d3b53d3ecb1282b67488680c7f5f4e805d0c3af4020ad673079fff92c6353f04903a9a64 + checksum: 12c357175783bcd0feea454ece57f033928948a0555dc97c79675b56d2cc79043d2a5e28a7554d3531f1de13583634df3b48fb9609f79e8bb3adad92820bd807 languageName: node linkType: hard @@ -13353,14 +13355,14 @@ __metadata: languageName: node linkType: hard -"@octokit/request-error@npm:^5.0.0": - version: 5.0.1 - resolution: "@octokit/request-error@npm:5.0.1" +"@octokit/request-error@npm:^5.0.0, @octokit/request-error@npm:^5.1.0": + version: 5.1.0 + resolution: "@octokit/request-error@npm:5.1.0" dependencies: - "@octokit/types": ^12.0.0 + "@octokit/types": ^13.1.0 deprecation: ^2.0.0 once: ^1.4.0 - checksum: a681341e43b4da7a8acb19e1a6ba0355b1af146fa0191f2554a98950cf85f898af6ae3ab0b0287d6c871f5465ec57cb38363b96b5019f9f77ba6f30eca39ede5 + checksum: 2cdbb8e44072323b5e1c8c385727af6700e3e492d55bc1e8d0549c4a3d9026914f915866323d371b1f1772326d6e902341c872679cc05c417ffc15cadf5f4a4e languageName: node linkType: hard @@ -13392,15 +13394,15 @@ __metadata: languageName: node linkType: hard -"@octokit/request@npm:^8.0.0, @octokit/request@npm:^8.0.1, @octokit/request@npm:^8.0.2": - version: 8.1.6 - resolution: "@octokit/request@npm:8.1.6" +"@octokit/request@npm:^8.0.0, @octokit/request@npm:^8.0.2, @octokit/request@npm:^8.3.0, @octokit/request@npm:^8.3.1": + version: 8.4.0 + resolution: "@octokit/request@npm:8.4.0" dependencies: - "@octokit/endpoint": ^9.0.0 - "@octokit/request-error": ^5.0.0 - "@octokit/types": ^12.0.0 + "@octokit/endpoint": ^9.0.1 + "@octokit/request-error": ^5.1.0 + "@octokit/types": ^13.1.0 universal-user-agent: ^6.0.0 - checksum: df90204586ee7db5adf69c3007c5d9c0a866de488c9ba8756f98083208726ed360d5a541e68204c413fa10e6f17e171dc9868b18768b9799df0003bc84c59cf2 + checksum: 3d937e817a85c0adf447ab46b428ccd702c31b2091e47adec90583ec2242bd64666306fe8188628fb139aa4752e19400eb7652b0f5ca33cd9e77bbb2c60b202a languageName: node linkType: hard @@ -13432,12 +13434,12 @@ __metadata: languageName: node linkType: hard -"@octokit/types@npm:^13.0.0, @octokit/types@npm:^13.5.0": - version: 13.5.0 - resolution: "@octokit/types@npm:13.5.0" +"@octokit/types@npm:^13.0.0, @octokit/types@npm:^13.1.0, @octokit/types@npm:^13.5.0": + version: 13.6.2 + resolution: "@octokit/types@npm:13.6.2" dependencies: "@octokit/openapi-types": ^22.2.0 - checksum: 8e92f2b145b3c28a35312f93714245824a7b6b7353caa88edfdc85fc2ed4108321ed0c3988001ea53449fbb212febe0e8e9582744e85c3574dabe9d0441af5a0 + checksum: c4e51da1ccabc028b70fc90ca58d65091862a7fe3a65ec2692681f473a0aee4c355abfd15fd547bce3a9800d07c1592603fe84b4223e7beea0d493e4fcc1a65e languageName: node linkType: hard