From ad3a4697bb443d61f34cbae6f34bce8e1e1f367f Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Mon, 7 Aug 2023 13:36:19 -0400 Subject: [PATCH 01/17] Prototype support for rate limits --- packages/requester-utils/package.json | 2 + packages/requester-utils/src/BaseResource.ts | 47 +++++++++++++++- .../requester-utils/src/RequesterUtils.ts | 56 +++++++++++++++++-- packages/rest/src/Requester.ts | 13 ++++- yarn.lock | 16 ++++++ 5 files changed, 124 insertions(+), 10 deletions(-) diff --git a/packages/requester-utils/package.json b/packages/requester-utils/package.json index 28d5d6272..b53c6c94e 100644 --- a/packages/requester-utils/package.json +++ b/packages/requester-utils/package.json @@ -51,6 +51,8 @@ "release": "auto shipit" }, "dependencies": { + "async-sema": "^3.1.1", + "micromatch": "^4.0.5", "qs": "^6.11.2", "xcase": "^2.0.1" }, diff --git a/packages/requester-utils/src/BaseResource.ts b/packages/requester-utils/src/BaseResource.ts index ed81b8f9c..7bbe02a31 100644 --- a/packages/requester-utils/src/BaseResource.ts +++ b/packages/requester-utils/src/BaseResource.ts @@ -1,4 +1,4 @@ -import { RequesterType, ResourceOptions } from './RequesterUtils'; +import { RequesterType, ResourceOptions, RateLimitOptions } from './RequesterUtils'; export interface RootResourceOptions { // TODO: Not actually optional - Need to fix wrapper typing in requestUtils.ts: @@ -11,6 +11,7 @@ export interface RootResourceOptions { sudo?: string | number; profileToken?: string; profileMode?: 'execution' | 'memory'; + rateLimits?: RateLimitOptions; } export type GitlabToken = string | (() => Promise); @@ -36,6 +37,47 @@ function getDynamicToken(tokenArgument: (() => Promise) | string): Promi return tokenArgument instanceof Function ? tokenArgument() : Promise.resolve(tokenArgument); } +const DEFAULT_RATE_LIMITS = Object.freeze({ + // Default rate limit + '*': 30, + + // Import/Export + 'projects/import': 6, + 'projects/*/export': 6, + 'projects/*/download': 1, + 'groups/import': 6, + 'groups/*/export': 6, + 'groups/*/download': 1, + + // Note creation + 'projects/*/issues/*/notes': { + method: 'post', + limit: 300, + }, + 'projects/*/snippets/*/notes': { + method: 'post', + limit: 300, + }, + 'projects/*/merge_requests/*/notes': { + method: 'post', + limit: 300, + }, + 'groups/*/epics/*/notes': { + method: 'post', + limit: 300, + }, + + // Repositories - get file archive + 'projects/*/repository/archive*': 5, + + // Project Jobs + 'projects/*/jobs': 600, + + // Member deletion + 'projects/*/members': 60, + 'groups/*/members': 60, +}); + export class BaseResource { public readonly url: string; @@ -61,6 +103,7 @@ export class BaseResource { prefixUrl = '', rejectUnauthorized = true, queryTimeout = 300000, + rateLimits = DEFAULT_RATE_LIMITS, ...tokens }: BaseResourceOptions) { if (!requesterFn) throw new ReferenceError('requesterFn must be passed'); @@ -97,6 +140,6 @@ export class BaseResource { if (sudo) this.headers.Sudo = `${sudo}`; // Set requester instance using this information - this.requester = requesterFn({ ...this }); + this.requester = requesterFn({ ...this, rateLimits }); } } diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index 94f93125c..7c5562c0d 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -1,7 +1,15 @@ import { stringify } from 'qs'; import { decamelizeKeys } from 'xcase'; +import { RateLimit } from 'async-sema'; +import micromatch from 'micromatch'; // Types +export type RateLimiters = Record< + string, + ReturnType | { method: string; limit: ReturnType } +>; +export type RateLimitOptions = Record; + export type ResponseBodyTypes = | Record | Record[] @@ -27,6 +35,7 @@ export type ResourceOptions = { headers: { [header: string]: string }; authHeaders: { [authHeader: string]: () => Promise }; url: string; + rateLimits: RateLimitOptions; rejectUnauthorized: boolean; }; @@ -48,6 +57,7 @@ export type RequestOptions = { body?: string | FormData; asStream?: boolean; signal?: AbortSignal; + rateLimiters?: Record>; }; export interface RequesterType { @@ -73,6 +83,11 @@ export interface RequesterType { ): Promise>; } +export type RequestHandlerFn = ( + endpoint: string, + options?: Record, +) => Promise>; + // Utility methods export function formatQuery(params: Record = {}): string { const decamelized = decamelizeKeys(params); @@ -137,10 +152,17 @@ export async function defaultOptionsHandler( return Promise.resolve(defaultOptions); } -export type RequestHandlerFn = ( - endpoint: string, - options?: Record, -) => Promise>; +function createRateLimiters(rateLimitOptions: RateLimitOptions) { + const rateLimiters: RateLimiters = {}; + + Object.entries(rateLimitOptions).forEach(([key, config]) => { + if (typeof config === 'number') rateLimiters[key] = RateLimit(config); + else + rateLimiters[key] = { method: config.method.toUpperCase(), limit: RateLimit(config.limit) }; + }); + + return rateLimiters; +} export function createRequesterFn( optionsHandler: OptionsHandlerFn, @@ -150,6 +172,7 @@ export function createRequesterFn( return (serviceOptions) => { const requester: RequesterType = {} as RequesterType; + const rateLimiters = createRateLimiters(serviceOptions.rateLimits); methods.forEach((m) => { requester[m] = async (endpoint: string, options: Record) => { @@ -159,7 +182,7 @@ export function createRequesterFn( }); const requestOptions = await optionsHandler(serviceOptions, defaultRequestOptions); - return requestHandler(endpoint, requestOptions); + return requestHandler(endpoint, { ...requestOptions, rateLimiters }); }; }); @@ -195,3 +218,26 @@ export function presetResourceArguments> return updated as T; } + +export function getMatchingRateLimit( + endpoint: string, + method: string = 'GET', + rateLimiters: RateLimiters = {}, +): ReturnType { + const sortedEndpoints = Object.keys(rateLimiters).sort(); + + // eslint-disable-next-line + for (const ep of sortedEndpoints) { + if (micromatch([endpoint], ep)) { + const rateLimitConfig = rateLimiters[ep]; + + if ('method' in rateLimitConfig) { + if (rateLimitConfig.method === method.toUpperCase()) return rateLimitConfig.limit; + } else { + return rateLimitConfig; + } + } + } + + return RateLimit(30); +} diff --git a/packages/rest/src/Requester.ts b/packages/rest/src/Requester.ts index 8c05c63e5..9beef4cf2 100644 --- a/packages/rest/src/Requester.ts +++ b/packages/rest/src/Requester.ts @@ -3,7 +3,10 @@ import type { ResourceOptions, ResponseBodyTypes, } from '@gitbeaker/requester-utils'; -import { createRequesterFn } from '@gitbeaker/requester-utils'; +import { + createRequesterFn, + getMatchingRateLimit, +} from '@gitbeaker/requester-utils'; export async function defaultOptionsHandler( resourceOptions: ResourceOptions, @@ -93,7 +96,8 @@ function getConditionalMode(endpoint: string) { export async function defaultRequestHandler(endpoint: string, options?: RequestOptions) { const retryCodes = [429, 502]; const maxRetries = 10; - const { prefixUrl, asStream, searchParams, ...opts } = options || {}; + const { prefixUrl, asStream, searchParams, rateLimiters, method, ...opts } = options || {}; + const endpointRateLimit = getMatchingRateLimit(endpoint, method, rateLimiters); let baseUrl: string | undefined; if (prefixUrl) baseUrl = prefixUrl.endsWith('/') ? prefixUrl : `${prefixUrl}/`; @@ -107,7 +111,10 @@ export async function defaultRequestHandler(endpoint: string, options?: RequestO /* eslint-disable no-await-in-loop */ for (let i = 0; i < maxRetries; i += 1) { - const request = new Request(url, { ...opts, mode }); + const request = new Request(url, { ...opts, method, mode }); + + await endpointRateLimit(); + const response = await fetch(request).catch((e) => { if (e.name === 'TimeoutError' || e.name === 'AbortError') { throw new Error('Query timeout was reached'); diff --git a/yarn.lock b/yarn.lock index fd96021a1..879adc8d0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1057,6 +1057,8 @@ __metadata: resolution: "@gitbeaker/requester-utils@workspace:packages/requester-utils" dependencies: "@types/node": ^20.4.0 + async-sema: ^3.1.1 + micromatch: ^4.0.5 qs: ^6.11.2 tsup: ^7.1.0 typescript: ^5.1.6 @@ -3225,6 +3227,20 @@ __metadata: languageName: node linkType: hard +"astral-regex@npm:^2.0.0": + version: 2.0.0 + resolution: "astral-regex@npm:2.0.0" + checksum: 876231688c66400473ba505731df37ea436e574dd524520294cc3bbc54ea40334865e01fa0d074d74d036ee874ee7e62f486ea38bc421ee8e6a871c06f011766 + languageName: node + linkType: hard + +"async-sema@npm:^3.1.1": + version: 3.1.1 + resolution: "async-sema@npm:3.1.1" + checksum: 07b8c51f6cab107417ecdd8126b7a9fe5a75151b7f69fdd420dcc8ee08f9e37c473a217247e894b56e999b088b32e902dbe41637e4e9b594d3f8dfcdddfadc5e + languageName: node + linkType: hard + "async@npm:^3.1.0, async@npm:^3.2.3": version: 3.2.4 resolution: "async@npm:3.2.4" From 9ab016e40899e6ddd1cca7ce2a14508490fb5a8e Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Mon, 7 Aug 2023 17:38:44 -0400 Subject: [PATCH 02/17] Updating tests and ensuring the rate limit time unit --- packages/requester-utils/src/BaseResource.ts | 1 + .../requester-utils/src/RequesterUtils.ts | 9 ++- .../test/unit/RequesterUtils.ts | 72 ++++++++++++++++++- 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/packages/requester-utils/src/BaseResource.ts b/packages/requester-utils/src/BaseResource.ts index 7bbe02a31..2ed891cab 100644 --- a/packages/requester-utils/src/BaseResource.ts +++ b/packages/requester-utils/src/BaseResource.ts @@ -37,6 +37,7 @@ function getDynamicToken(tokenArgument: (() => Promise) | string): Promi return tokenArgument instanceof Function ? tokenArgument() : Promise.resolve(tokenArgument); } +// Default rate limits per minute const DEFAULT_RATE_LIMITS = Object.freeze({ // Default rate limit '*': 30, diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index 7c5562c0d..fe6249a66 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -152,13 +152,16 @@ export async function defaultOptionsHandler( return Promise.resolve(defaultOptions); } -function createRateLimiters(rateLimitOptions: RateLimitOptions) { +export function createRateLimiters(rateLimitOptions: RateLimitOptions = {}) { const rateLimiters: RateLimiters = {}; Object.entries(rateLimitOptions).forEach(([key, config]) => { - if (typeof config === 'number') rateLimiters[key] = RateLimit(config); + if (typeof config === 'number') rateLimiters[key] = RateLimit(config, { timeUnit: 60000 }); else - rateLimiters[key] = { method: config.method.toUpperCase(), limit: RateLimit(config.limit) }; + rateLimiters[key] = { + method: config.method.toUpperCase(), + limit: RateLimit(config.limit, { timeUnit: 60000 }), + }; }); return rateLimiters; diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index 417209a04..f5d7b6866 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -1,5 +1,8 @@ +import * as AsyncSema from 'async-sema'; import { + DefaultResourceOptions, RequestOptions, + createRateLimiters, createRequesterFn, defaultOptionsHandler, formatQuery, @@ -9,13 +12,14 @@ import { const methods = ['get', 'put', 'patch', 'delete', 'post']; describe('defaultOptionsHandler', () => { - const serviceOptions = { + const serviceOptions: DefaultResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), }, url: 'testurl', rejectUnauthorized: false, + rateLimits: {}, }; it('should not use default request options if not passed', async () => { @@ -105,13 +109,14 @@ describe('defaultOptionsHandler', () => { describe('createInstance', () => { const requestHandler = jest.fn(); const optionsHandler = jest.fn(() => Promise.resolve({} as RequestOptions)); - const serviceOptions = { + const serviceOptions: DefaultResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), }, url: 'testurl', rejectUnauthorized: false, + rateLimits: {}, }; it('should have a createInstance function', () => { @@ -153,14 +158,16 @@ describe('createInstance', () => { }, url: 'testurl', rejectUnauthorized: false, + rateLimits: {}, }; - const serviceOptions2 = { + const serviceOptions2: DefaultResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), }, url: 'testurl2', rejectUnauthorized: true, + rateLimits: {}, }; const requesterFn = createRequesterFn(optionsHandler, requestHandler); @@ -181,6 +188,65 @@ describe('createInstance', () => { expect.objectContaining({ method: 'GET' }), ); }); + + it('should pass the rate limiters to the requestHandler function', async () => { + const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); + + const testEndpoint = 'test endpoint'; + const requester = createRequesterFn( + optionsHandler, + requestHandler, + )({ + ...serviceOptions, + rateLimits: { + '*': 40, + 'projects/*/test': { + method: 'GET', + limit: 10, + }, + }, + }); + + await requester.get(testEndpoint, {}); + + expect(rateLimitSpy).toHaveBeenCalledWith(10, { timeUnit: 60000 }); + expect(rateLimitSpy).toHaveBeenCalledWith(40, { timeUnit: 60000 }); + + expect(requestHandler).toHaveBeenCalledWith(testEndpoint, { + rateLimiters: { + '*': expect.toBeFunction(), + 'projects/*/test': { + method: 'GET', + limit: expect.toBeFunction(), + }, + }, + }); + }); +}); + +describe('createRateLimiters', () => { + it('should create rate limiter functions when configured', () => { + const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); + + const limiters = createRateLimiters({ + '*': 40, + 'projects/*/test': { + method: 'GET', + limit: 10, + }, + }); + + expect(rateLimitSpy).toHaveBeenCalledWith(10, { timeUnit: 60000 }); + expect(rateLimitSpy).toHaveBeenCalledWith(40, { timeUnit: 60000 }); + + expect(limiters).toStrictEqual({ + '*': expect.toBeFunction(), + 'projects/*/test': { + method: 'GET', + limit: expect.toBeFunction(), + }, + }); + }); }); describe('presetResourceArguments', () => { From ba46fdc8f36e4d8a995af1af482bf91ed9bfa7fc Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Tue, 8 Aug 2023 11:32:18 -0400 Subject: [PATCH 03/17] Update types --- packages/requester-utils/test/unit/RequesterUtils.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index f5d7b6866..a25120cba 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -1,7 +1,7 @@ import * as AsyncSema from 'async-sema'; import { - DefaultResourceOptions, RequestOptions, + ResourceOptions, createRateLimiters, createRequesterFn, defaultOptionsHandler, @@ -12,7 +12,7 @@ import { const methods = ['get', 'put', 'patch', 'delete', 'post']; describe('defaultOptionsHandler', () => { - const serviceOptions: DefaultResourceOptions = { + const serviceOptions: ResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), @@ -109,7 +109,7 @@ describe('defaultOptionsHandler', () => { describe('createInstance', () => { const requestHandler = jest.fn(); const optionsHandler = jest.fn(() => Promise.resolve({} as RequestOptions)); - const serviceOptions: DefaultResourceOptions = { + const serviceOptions: ResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), @@ -151,7 +151,7 @@ describe('createInstance', () => { }); it('should respect the closure variables', async () => { - const serviceOptions1 = { + const serviceOptions1: ResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), @@ -160,7 +160,7 @@ describe('createInstance', () => { rejectUnauthorized: false, rateLimits: {}, }; - const serviceOptions2: DefaultResourceOptions = { + const serviceOptions2: ResourceOptions = { headers: { test: '5' }, authHeaders: { token: () => Promise.resolve('1234'), From d36b5dcbbf83a5b81f129a6480de097edcc948cc Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Tue, 8 Aug 2023 13:40:30 -0400 Subject: [PATCH 04/17] Updating unit tests --- packages/requester-utils/src/BaseResource.ts | 2 +- .../requester-utils/src/RequesterUtils.ts | 15 +++-- .../requester-utils/test/unit/BaseResource.ts | 8 +++ .../test/unit/RequesterUtils.ts | 66 ++++++++++++++++++- packages/rest/src/Requester.ts | 7 +- 5 files changed, 83 insertions(+), 15 deletions(-) diff --git a/packages/requester-utils/src/BaseResource.ts b/packages/requester-utils/src/BaseResource.ts index 2ed891cab..24462008d 100644 --- a/packages/requester-utils/src/BaseResource.ts +++ b/packages/requester-utils/src/BaseResource.ts @@ -1,4 +1,4 @@ -import { RequesterType, ResourceOptions, RateLimitOptions } from './RequesterUtils'; +import { RateLimitOptions, RequesterType, ResourceOptions } from './RequesterUtils'; export interface RootResourceOptions { // TODO: Not actually optional - Need to fix wrapper typing in requestUtils.ts: diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index fe6249a66..dedf96c16 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -35,8 +35,8 @@ export type ResourceOptions = { headers: { [header: string]: string }; authHeaders: { [authHeader: string]: () => Promise }; url: string; - rateLimits: RateLimitOptions; rejectUnauthorized: boolean; + rateLimits?: RateLimitOptions; }; export type DefaultRequestOptions = { @@ -222,25 +222,26 @@ export function presetResourceArguments> return updated as T; } -export function getMatchingRateLimit( +export function getMatchingRateLimiter( endpoint: string, - method: string = 'GET', rateLimiters: RateLimiters = {}, + method: string = 'GET', ): ReturnType { - const sortedEndpoints = Object.keys(rateLimiters).sort(); + const sortedEndpoints = Object.keys(rateLimiters).sort().reverse(); // eslint-disable-next-line for (const ep of sortedEndpoints) { if (micromatch([endpoint], ep)) { const rateLimitConfig = rateLimiters[ep]; - if ('method' in rateLimitConfig) { - if (rateLimitConfig.method === method.toUpperCase()) return rateLimitConfig.limit; + if (typeof rateLimitConfig === 'object' && 'method' in rateLimitConfig) { + if (rateLimitConfig.method.toUpperCase() === method.toUpperCase()) + return rateLimitConfig.limit; } else { return rateLimitConfig; } } } - return RateLimit(30); + return RateLimit(1000); } diff --git a/packages/requester-utils/test/unit/BaseResource.ts b/packages/requester-utils/test/unit/BaseResource.ts index b39fa364a..0e7f40f86 100644 --- a/packages/requester-utils/test/unit/BaseResource.ts +++ b/packages/requester-utils/test/unit/BaseResource.ts @@ -98,6 +98,14 @@ describe('Creation of BaseResource instance', () => { await expect(service.authHeaders['job-token']()).resolves.toBe('1234'); }); + it('should throw an error if a token, jobToken or oauthToken is not passed', () => { + expect(() => { + // eslint-disable-next-line + // @ts-ignore + new BaseResource({ requesterFn: jest.fn() }); // eslint-disable-line + }).toThrow(); + }); + it('should set the X-Profile-Token header if the profileToken option is given', () => { const service = new BaseResource({ token: '123', diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index a25120cba..ad40f14cb 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -6,6 +6,7 @@ import { createRequesterFn, defaultOptionsHandler, formatQuery, + getMatchingRateLimiter, presetResourceArguments, } from '../../src/RequesterUtils'; @@ -116,7 +117,6 @@ describe('createInstance', () => { }, url: 'testurl', rejectUnauthorized: false, - rateLimits: {}, }; it('should have a createInstance function', () => { @@ -146,7 +146,7 @@ describe('createInstance', () => { serviceOptions, expect.objectContaining({ method: m.toUpperCase() }), ); - expect(requestHandler).toHaveBeenCalledWith(testEndpoint, {}); + expect(requestHandler).toHaveBeenCalledWith(testEndpoint, { rateLimiters: {} }); } }); @@ -310,3 +310,65 @@ describe('formatQuery', () => { expect(string).toBe('test=6¬%5Btest%5D=7'); }); }); + +describe('getMatchingRateLimiter', () => { + it('should default the method to GET if not passed', () => { + const rateLimiter = 10; + const matchingRateLimiter = getMatchingRateLimiter('endpoint', { + '*': { method: 'GET', limit: rateLimiter }, + }); + + expect(matchingRateLimiter).toBe(rateLimiter); + }); + + it('should uppercase method for matching', () => { + const rateLimiter = 10; + const matchingRateLimiter = getMatchingRateLimiter('endpoint', { + '*': { method: 'get', limit: rateLimiter }, + }); + + expect(matchingRateLimiter).toBe(rateLimiter); + }); + + it('should default the rateLimiters to an empty object if not passed and return the default rate of 1000 rpm', () => { + const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); + + getMatchingRateLimiter('endpoint'); + + expect(rateLimitSpy).toHaveBeenCalledWith(1000); + }); + + it('should return the most specific rate limit', () => { + const rateLimiter = 10; + const matchingRateLimiter = getMatchingRateLimiter('endpoint', { + '*': jest.fn(), + 'endpoint/*': rateLimiter, + }); + + expect(matchingRateLimiter).toBe(rateLimiter); + }); + + it('should return a default rate limit of 1000 rpm if nothing matches', () => { + const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); + + getMatchingRateLimiter('endpoint', { someurl: jest.fn() }); + + expect(rateLimitSpy).toHaveBeenCalledWith(1000); + }); + + it('should handle expanded rate limit options with a particular method and limit', () => { + const rateLimiter = 10; + const matchingRateLimiter = getMatchingRateLimiter('endpoint', { + '*': { method: 'get', limit: rateLimiter }, + }); + + expect(matchingRateLimiter).toBe(rateLimiter); + }); + + it('should handle simple rate limit options with a particular limit', () => { + const rateLimiter = 10; + const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': rateLimiter }); + + expect(matchingRateLimiter).toBe(rateLimiter); + }); +}); diff --git a/packages/rest/src/Requester.ts b/packages/rest/src/Requester.ts index 9beef4cf2..17d952d47 100644 --- a/packages/rest/src/Requester.ts +++ b/packages/rest/src/Requester.ts @@ -3,10 +3,7 @@ import type { ResourceOptions, ResponseBodyTypes, } from '@gitbeaker/requester-utils'; -import { - createRequesterFn, - getMatchingRateLimit, -} from '@gitbeaker/requester-utils'; +import { createRequesterFn, getMatchingRateLimiter } from '@gitbeaker/requester-utils'; export async function defaultOptionsHandler( resourceOptions: ResourceOptions, @@ -97,7 +94,7 @@ export async function defaultRequestHandler(endpoint: string, options?: RequestO const retryCodes = [429, 502]; const maxRetries = 10; const { prefixUrl, asStream, searchParams, rateLimiters, method, ...opts } = options || {}; - const endpointRateLimit = getMatchingRateLimit(endpoint, method, rateLimiters); + const endpointRateLimit = getMatchingRateLimiter(endpoint, rateLimiters, method); let baseUrl: string | undefined; if (prefixUrl) baseUrl = prefixUrl.endsWith('/') ? prefixUrl : `${prefixUrl}/`; From 94f68dba9a3f6528399c7e8c4276c9f7884aa0b3 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Tue, 8 Aug 2023 16:57:35 -0400 Subject: [PATCH 05/17] Adjusting function tests --- .../requester-utils/src/RequesterUtils.ts | 2 +- .../test/unit/RequesterUtils.ts | 40 ++++++++++++------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index dedf96c16..0ef30201c 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -226,7 +226,7 @@ export function getMatchingRateLimiter( endpoint: string, rateLimiters: RateLimiters = {}, method: string = 'GET', -): ReturnType { +): () => Promise { const sortedEndpoints = Object.keys(rateLimiters).sort().reverse(); // eslint-disable-next-line diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index ad40f14cb..255b8be72 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -312,22 +312,26 @@ describe('formatQuery', () => { }); describe('getMatchingRateLimiter', () => { - it('should default the method to GET if not passed', () => { - const rateLimiter = 10; + it('should default the method to GET if not passed', async () => { + const rateLimiter = jest.fn(); const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': { method: 'GET', limit: rateLimiter }, }); - expect(matchingRateLimiter).toBe(rateLimiter); + await matchingRateLimiter(); + + expect(rateLimiter).toHaveBeenCalledTimes(1); }); - it('should uppercase method for matching', () => { - const rateLimiter = 10; + it('should uppercase method for matching', async () => { + const rateLimiter = jest.fn(); const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': { method: 'get', limit: rateLimiter }, }); - expect(matchingRateLimiter).toBe(rateLimiter); + await matchingRateLimiter(); + + expect(rateLimiter).toHaveBeenCalledTimes(1); }); it('should default the rateLimiters to an empty object if not passed and return the default rate of 1000 rpm', () => { @@ -338,14 +342,16 @@ describe('getMatchingRateLimiter', () => { expect(rateLimitSpy).toHaveBeenCalledWith(1000); }); - it('should return the most specific rate limit', () => { - const rateLimiter = 10; + it('should return the most specific rate limit', async () => { + const rateLimiter = jest.fn(); const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': jest.fn(), 'endpoint/*': rateLimiter, }); - expect(matchingRateLimiter).toBe(rateLimiter); + await matchingRateLimiter(); + + expect(rateLimiter).toHaveBeenCalledTimes(1); }); it('should return a default rate limit of 1000 rpm if nothing matches', () => { @@ -356,19 +362,23 @@ describe('getMatchingRateLimiter', () => { expect(rateLimitSpy).toHaveBeenCalledWith(1000); }); - it('should handle expanded rate limit options with a particular method and limit', () => { - const rateLimiter = 10; + it('should handle expanded rate limit options with a particular method and limit', async () => { + const rateLimiter = jest.fn(); const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': { method: 'get', limit: rateLimiter }, }); - expect(matchingRateLimiter).toBe(rateLimiter); + await matchingRateLimiter(); + + expect(rateLimiter).toHaveBeenCalledTimes(1); }); - it('should handle simple rate limit options with a particular limit', () => { - const rateLimiter = 10; + it('should handle simple rate limit options with a particular limit', async () => { + const rateLimiter = jest.fn(); const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': rateLimiter }); - expect(matchingRateLimiter).toBe(rateLimiter); + await matchingRateLimiter(); + + expect(rateLimiter).toHaveBeenCalledTimes(1); }); }); From e7c0668363122d03f11ee366b2e4f5faeb78c38a Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Fri, 11 Aug 2023 02:34:52 -0400 Subject: [PATCH 06/17] 100p coverage on the requester-utils library --- packages/requester-utils/src/RequesterUtils.ts | 5 +---- packages/requester-utils/test/unit/BaseResource.ts | 6 ++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index 0ef30201c..2dea6932e 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -193,10 +193,7 @@ export function createRequesterFn( }; } -function extendClass( - Base: T, - customConfig: Record = {}, -): T { +function extendClass(Base: T, customConfig: Record): T { return class extends Base { constructor(...options: any[]) { // eslint-disable-line diff --git a/packages/requester-utils/test/unit/BaseResource.ts b/packages/requester-utils/test/unit/BaseResource.ts index 0e7f40f86..29a3b2b94 100644 --- a/packages/requester-utils/test/unit/BaseResource.ts +++ b/packages/requester-utils/test/unit/BaseResource.ts @@ -204,6 +204,12 @@ describe('Creation of BaseResource instance', () => { // @ts-ignore new BaseResource(); // eslint-disable-line }).toThrow(); + + expect(() => { + // eslint-disable-next-line + // @ts-ignore + new BaseResource({}); // eslint-disable-line + }).toThrow(); }); it('should set the internal requester based on the required requesterFn parameter', async () => { From f1b1f725d9adba0720e0f35d3199e7d8c52d3fcc Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Fri, 11 Aug 2023 02:55:56 -0400 Subject: [PATCH 07/17] Adding more tests --- .../src/resources/ApplicationPlanLimits.ts | 2 +- .../unit/resources/ApplicationPlanLimits.ts | 36 +++++++++++++++ .../unit/resources/ApplicationStatistics.ts | 24 ++++++++++ .../core/test/unit/resources/Applications.ts | 44 +++++++++++++++++++ .../integration/nodejs/resources/Issues.ts | 2 +- .../integration/nodejs/resources/Projects.ts | 2 +- 6 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 packages/core/test/unit/resources/ApplicationPlanLimits.ts create mode 100644 packages/core/test/unit/resources/ApplicationStatistics.ts create mode 100644 packages/core/test/unit/resources/Applications.ts diff --git a/packages/core/src/resources/ApplicationPlanLimits.ts b/packages/core/src/resources/ApplicationPlanLimits.ts index 2d8158352..3000f68ef 100644 --- a/packages/core/src/resources/ApplicationPlanLimits.ts +++ b/packages/core/src/resources/ApplicationPlanLimits.ts @@ -61,6 +61,7 @@ export class ApplicationPlanLimits extends BaseResour } = options; return RequestHelper.put()(this, 'application/plan_limits', { + ...opts, searchParams: { planName, ciPipelineSize, @@ -81,7 +82,6 @@ export class ApplicationPlanLimits extends BaseResour terraformModuleMaxFileSize, storageSizeLimit, }, - opts, }); } } diff --git a/packages/core/test/unit/resources/ApplicationPlanLimits.ts b/packages/core/test/unit/resources/ApplicationPlanLimits.ts new file mode 100644 index 000000000..9d65e9090 --- /dev/null +++ b/packages/core/test/unit/resources/ApplicationPlanLimits.ts @@ -0,0 +1,36 @@ +import { RequestHelper } from '../../../src/infrastructure'; +import { ApplicationPlanLimits } from '../../../src'; + +jest.mock( + '../../../src/infrastructure/RequestHelper', + () => require('../../__mocks__/RequestHelper').default, +); + +let service: ApplicationPlanLimits; + +beforeEach(() => { + service = new ApplicationPlanLimits({ + requesterFn: jest.fn(), + token: 'abcdefg', + }); +}); + +describe('ApplicationPlanLimits.show', () => { + it('should request GET /application/plan_limits', async () => { + await service.show(); + + expect(RequestHelper.get()).toHaveBeenCalledWith(service, 'application/plan_limits', undefined); + }); +}); + +describe('ApplicationPlanLimits.edit', () => { + it('should request PUT /application/plan_limits with a terms property', async () => { + await service.edit('Plan name'); + + expect(RequestHelper.put()).toHaveBeenCalledWith(service, 'application/plan_limits', { + searchParams: { + planName: 'Plan name', + }, + }); + }); +}); diff --git a/packages/core/test/unit/resources/ApplicationStatistics.ts b/packages/core/test/unit/resources/ApplicationStatistics.ts new file mode 100644 index 000000000..b568ced55 --- /dev/null +++ b/packages/core/test/unit/resources/ApplicationStatistics.ts @@ -0,0 +1,24 @@ +import { RequestHelper } from '../../../src/infrastructure'; +import { ApplicationStatistics } from '../../../src'; + +jest.mock( + '../../../src/infrastructure/RequestHelper', + () => require('../../__mocks__/RequestHelper').default, +); + +let service: ApplicationStatistics; + +beforeEach(() => { + service = new ApplicationStatistics({ + requesterFn: jest.fn(), + token: 'abcdefg', + }); +}); + +describe('ApplicationStatistics.show', () => { + it('should request GET /application/statistics', async () => { + await service.show(); + + expect(RequestHelper.get()).toHaveBeenCalledWith(service, 'application/statistics', undefined); + }); +}); diff --git a/packages/core/test/unit/resources/Applications.ts b/packages/core/test/unit/resources/Applications.ts new file mode 100644 index 000000000..8df17bbec --- /dev/null +++ b/packages/core/test/unit/resources/Applications.ts @@ -0,0 +1,44 @@ +import { RequestHelper } from '../../../src/infrastructure'; +import { Applications } from '../../../src'; + +jest.mock( + '../../../src/infrastructure/RequestHelper', + () => require('../../__mocks__/RequestHelper').default, +); + +let service: Applications; + +beforeEach(() => { + service = new Applications({ + requesterFn: jest.fn(), + token: 'abcdefg', + }); +}); + +describe('Applications.all', () => { + it('should request GET /applications without options', async () => { + await service.all(); + + expect(RequestHelper.get()).toHaveBeenCalledWith(service, 'applications', undefined); + }); +}); + +describe('Applications.show', () => { + it('should request GET /applications', async () => { + await service.create('application', 'url', 'scope1'); + + expect(RequestHelper.get()).toHaveBeenCalledWith(service, 'applications', { + name: 'application', + redirectUri: 'url', + scopes: 'scope1', + }); + }); +}); + +describe('Applications.remove', () => { + it('should request GET /applications/:id', async () => { + await service.remove(12); + + expect(RequestHelper.get()).toHaveBeenCalledWith(service, 'applications/12', undefined); + }); +}); diff --git a/packages/rest/test/integration/nodejs/resources/Issues.ts b/packages/rest/test/integration/nodejs/resources/Issues.ts index 2d2b11122..28a04c25e 100644 --- a/packages/rest/test/integration/nodejs/resources/Issues.ts +++ b/packages/rest/test/integration/nodejs/resources/Issues.ts @@ -34,7 +34,7 @@ describe('Issues.all', () => { } await Promise.all(newIssues); - }, 20000); + }, 60000); it('should get 10 issues using keyset pagination', async () => { const projects = await issueAPI.all({ diff --git a/packages/rest/test/integration/nodejs/resources/Projects.ts b/packages/rest/test/integration/nodejs/resources/Projects.ts index 3bae1576e..d599f70f9 100644 --- a/packages/rest/test/integration/nodejs/resources/Projects.ts +++ b/packages/rest/test/integration/nodejs/resources/Projects.ts @@ -32,7 +32,7 @@ describe('Projects.all', () => { } await Promise.all(newProjects); - }, 20000); + }, 60000); it('should get 10 projects using offset pagination', async () => { const projects = await service.all({ From a25bf6423ff7edd5bdf3c7c0a2068678e470298c Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Fri, 11 Aug 2023 13:51:10 -0400 Subject: [PATCH 08/17] Updating default values --- packages/requester-utils/src/BaseResource.ts | 2 +- packages/requester-utils/src/RequesterUtils.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/requester-utils/src/BaseResource.ts b/packages/requester-utils/src/BaseResource.ts index 24462008d..52f5108ff 100644 --- a/packages/requester-utils/src/BaseResource.ts +++ b/packages/requester-utils/src/BaseResource.ts @@ -40,7 +40,7 @@ function getDynamicToken(tokenArgument: (() => Promise) | string): Promi // Default rate limits per minute const DEFAULT_RATE_LIMITS = Object.freeze({ // Default rate limit - '*': 30, + '*': 3000, // Import/Export 'projects/import': 6, diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index 2dea6932e..e213540ec 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -240,5 +240,5 @@ export function getMatchingRateLimiter( } } - return RateLimit(1000); + return RateLimit(3000, { timeUnit: 60000 }); } From 70a9a4e4148787ba3dfa24764cb17f3f3cb53be4 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Wed, 30 Aug 2023 21:43:50 -0400 Subject: [PATCH 09/17] Updating unit tests Updating unit tests focused on default value of the getMatchingRateLimiter function --- packages/requester-utils/test/unit/RequesterUtils.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index 255b8be72..850ded20e 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -334,12 +334,12 @@ describe('getMatchingRateLimiter', () => { expect(rateLimiter).toHaveBeenCalledTimes(1); }); - it('should default the rateLimiters to an empty object if not passed and return the default rate of 1000 rpm', () => { + it('should default the rateLimiters to an empty object if not passed and return the default rate of 3000 rpm', () => { const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); getMatchingRateLimiter('endpoint'); - expect(rateLimitSpy).toHaveBeenCalledWith(1000); + expect(rateLimitSpy).toHaveBeenCalledWith(3000, { timeUnit: 60000 }); }); it('should return the most specific rate limit', async () => { @@ -359,7 +359,7 @@ describe('getMatchingRateLimiter', () => { getMatchingRateLimiter('endpoint', { someurl: jest.fn() }); - expect(rateLimitSpy).toHaveBeenCalledWith(1000); + expect(rateLimitSpy).toHaveBeenCalledWith(3000, { timeUnit: 60000 }); }); it('should handle expanded rate limit options with a particular method and limit', async () => { From fc4a058bde34ffac05c0b4c693a7be800104574b Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Wed, 30 Aug 2023 23:01:45 -0400 Subject: [PATCH 10/17] Refreshing lock file --- yarn.lock | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/yarn.lock b/yarn.lock index 879adc8d0..01393ad42 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3227,13 +3227,6 @@ __metadata: languageName: node linkType: hard -"astral-regex@npm:^2.0.0": - version: 2.0.0 - resolution: "astral-regex@npm:2.0.0" - checksum: 876231688c66400473ba505731df37ea436e574dd524520294cc3bbc54ea40334865e01fa0d074d74d036ee874ee7e62f486ea38bc421ee8e6a871c06f011766 - languageName: node - linkType: hard - "async-sema@npm:^3.1.1": version: 3.1.1 resolution: "async-sema@npm:3.1.1" @@ -7988,7 +7981,7 @@ __metadata: languageName: node linkType: hard -"micromatch@npm:4.0.5, micromatch@npm:^4.0.4": +"micromatch@npm:4.0.5, micromatch@npm:^4.0.4, micromatch@npm:^4.0.5": version: 4.0.5 resolution: "micromatch@npm:4.0.5" dependencies: From 2578d0a87288036910f26d54b75c28eb8f86d181 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Thu, 31 Aug 2023 13:26:33 -0400 Subject: [PATCH 11/17] Fixing implementation and updating unit tests --- packages/requester-utils/src/BaseResource.ts | 2 +- .../requester-utils/src/RequesterUtils.ts | 20 +- .../test/unit/RequesterUtils.ts | 8 +- packages/rest/package.json | 2 +- .../integration/nodejs/resources/Issues.ts | 1 + packages/rest/test/unit/Requester.ts | 199 +++++++++++++++++- 6 files changed, 211 insertions(+), 21 deletions(-) diff --git a/packages/requester-utils/src/BaseResource.ts b/packages/requester-utils/src/BaseResource.ts index 52f5108ff..656b4cf10 100644 --- a/packages/requester-utils/src/BaseResource.ts +++ b/packages/requester-utils/src/BaseResource.ts @@ -40,7 +40,7 @@ function getDynamicToken(tokenArgument: (() => Promise) | string): Promi // Default rate limits per minute const DEFAULT_RATE_LIMITS = Object.freeze({ // Default rate limit - '*': 3000, + '**': 3000, // Import/Export 'projects/import': 6, diff --git a/packages/requester-utils/src/RequesterUtils.ts b/packages/requester-utils/src/RequesterUtils.ts index e213540ec..05d570e59 100644 --- a/packages/requester-utils/src/RequesterUtils.ts +++ b/packages/requester-utils/src/RequesterUtils.ts @@ -225,20 +225,14 @@ export function getMatchingRateLimiter( method: string = 'GET', ): () => Promise { const sortedEndpoints = Object.keys(rateLimiters).sort().reverse(); + const match = sortedEndpoints.find((ep) => micromatch.isMatch(endpoint, ep)); + const rateLimitConfig = match && rateLimiters[match]; - // eslint-disable-next-line - for (const ep of sortedEndpoints) { - if (micromatch([endpoint], ep)) { - const rateLimitConfig = rateLimiters[ep]; - - if (typeof rateLimitConfig === 'object' && 'method' in rateLimitConfig) { - if (rateLimitConfig.method.toUpperCase() === method.toUpperCase()) - return rateLimitConfig.limit; - } else { - return rateLimitConfig; - } - } + if (rateLimitConfig && typeof rateLimitConfig !== 'object') { + return rateLimitConfig; + } + if (rateLimitConfig && rateLimitConfig.method.toUpperCase() === method.toUpperCase()) { + return rateLimitConfig.limit; } - return RateLimit(3000, { timeUnit: 60000 }); } diff --git a/packages/requester-utils/test/unit/RequesterUtils.ts b/packages/requester-utils/test/unit/RequesterUtils.ts index 850ded20e..6cdd3b7f6 100644 --- a/packages/requester-utils/test/unit/RequesterUtils.ts +++ b/packages/requester-utils/test/unit/RequesterUtils.ts @@ -344,9 +344,9 @@ describe('getMatchingRateLimiter', () => { it('should return the most specific rate limit', async () => { const rateLimiter = jest.fn(); - const matchingRateLimiter = getMatchingRateLimiter('endpoint', { + const matchingRateLimiter = getMatchingRateLimiter('endpoint/testing', { '*': jest.fn(), - 'endpoint/*': rateLimiter, + 'endpoint/testing*': rateLimiter, }); await matchingRateLimiter(); @@ -354,7 +354,7 @@ describe('getMatchingRateLimiter', () => { expect(rateLimiter).toHaveBeenCalledTimes(1); }); - it('should return a default rate limit of 1000 rpm if nothing matches', () => { + it('should return a default rate limit of 3000 rpm if nothing matches', () => { const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit'); getMatchingRateLimiter('endpoint', { someurl: jest.fn() }); @@ -375,7 +375,7 @@ describe('getMatchingRateLimiter', () => { it('should handle simple rate limit options with a particular limit', async () => { const rateLimiter = jest.fn(); - const matchingRateLimiter = getMatchingRateLimiter('endpoint', { '*': rateLimiter }); + const matchingRateLimiter = getMatchingRateLimiter('endpoint/testing', { '**': rateLimiter }); await matchingRateLimiter(); diff --git a/packages/rest/package.json b/packages/rest/package.json index 93320b023..91ff5111f 100644 --- a/packages/rest/package.json +++ b/packages/rest/package.json @@ -40,7 +40,7 @@ "test:types": "tsc", "test:e2e:browser": "playwright test", "test:e2e": "yarn test:e2e:browser", - "test:integration:nodejs": "jest --maxWorkers=50% test/integration/nodejs", + "test:integration:nodejs": "jest --maxWorkers=50% test/integration/nodejs/resources/Issues.ts", "test:integration": "yarn test:integration:nodejs", "test:unit": "jest --maxWorkers=50% test/unit", "format:docs": "prettier '**/(*.json|.yml|.js|.md)' --ignore-path ../../.prettierignore", diff --git a/packages/rest/test/integration/nodejs/resources/Issues.ts b/packages/rest/test/integration/nodejs/resources/Issues.ts index 28a04c25e..22acd6aa5 100644 --- a/packages/rest/test/integration/nodejs/resources/Issues.ts +++ b/packages/rest/test/integration/nodejs/resources/Issues.ts @@ -26,6 +26,7 @@ describe('Issues.all', () => { const newIssues: ReturnType>[] = []; for (let i = 0; i < 10; i += 1) { + console.log(`start ${i}`); newIssues.push( issueAPI.create(project.id, `Issue.all Test - NoteJS ${TEST_ID} ${i}`, { description: 'A test issue', diff --git a/packages/rest/test/unit/Requester.ts b/packages/rest/test/unit/Requester.ts index 20a972cb2..e7a9ecdaa 100644 --- a/packages/rest/test/unit/Requester.ts +++ b/packages/rest/test/unit/Requester.ts @@ -21,7 +21,7 @@ describe('processBody', () => { it('should return a blob if type is octet-stream, binary, or gzip', async () => { const blobData = new Blob(['test'], { - type: 'plain/text', + type: 'text/plain', }); const output = [ @@ -123,6 +123,176 @@ describe('defaultRequestHandler', () => { }); }); + it('should return an error the content of the error message if response is not JSON', async () => { + const stringBody = 'Bad things happened'; + + MockFetch.mockReturnValueOnce( + Promise.resolve({ + ok: false, + status: 501, + statusText: 'Really Bad Error', + headers: new Headers({ + 'content-type': 'text/plain', + }), + json: () => Promise.resolve(stringBody), + text: () => Promise.resolve(stringBody), + }), + ); + + await expect(defaultRequestHandler('http://test.com', {} as RequestOptions)).rejects.toThrow({ + message: 'Really Bad Error', + name: 'Error', + cause: { + description: stringBody, + }, + }); + }); + + it('should return an error with a message "Query timeout was reached" if fetch throws a TimeoutError', async () => { + class TimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = 'TimeoutError'; + } + } + + MockFetch.mockRejectedValueOnce(new TimeoutError('Hit timeout')); + + await expect(defaultRequestHandler('http://test.com', {} as RequestOptions)).rejects.toThrow({ + message: 'Query timeout was reached', + name: 'Error', + }); + }); + + it('should return an error with a message "Query timeout was reached" if fetch throws a AbortError', async () => { + class AbortError extends Error { + constructor(message: string) { + super(message); + this.name = 'AbortError'; + } + } + + MockFetch.mockRejectedValueOnce(new AbortError('Abort signal triggered')); + + await expect(defaultRequestHandler('http://test.com', {} as RequestOptions)).rejects.toThrow({ + message: 'Query timeout was reached', + name: 'Error', + }); + }); + + it('should return an unchanged error if fetch throws an error thats not an AbortError or TimeoutError', async () => { + class RandomError extends Error { + constructor(message: string) { + super(message); + this.name = 'RandomError'; + } + } + + MockFetch.mockRejectedValueOnce(new RandomError('Random Error')); + + await expect(defaultRequestHandler('http://test.com', {} as RequestOptions)).rejects.toThrow({ + message: 'Random Error', + name: 'RandomError', + }); + }); + + it('should retry request if a 429 retry code is returned', async () => { + const stringBody = { error: 'msg' }; + const fakeFailedReturnValue = Promise.resolve({ + ok: false, + status: 429, + statusText: 'Retry Code', + headers: new Headers({ + 'content-type': 'application/json', + }), + json: () => Promise.resolve(stringBody), + text: () => Promise.resolve(JSON.stringify(stringBody)), + }); + + const fakeSuccessfulReturnValue = Promise.resolve({ + json: () => Promise.resolve({}), + text: () => Promise.resolve(JSON.stringify({})), + ok: true, + status: 200, + headers: new Headers({ + 'content-type': 'application/json', + }), + }); + + // Mock return 10 times + MockFetch.mockReturnValue(fakeFailedReturnValue); + MockFetch.mockReturnValue(fakeSuccessfulReturnValue); + + const output = await defaultRequestHandler('http://test.com', {} as RequestOptions); + + expect(output).toMatchObject({ + body: {}, + headers: { 'content-type': 'application/json' }, + status: 200, + }); + }); + + it('should retry request if a 502 retry code is returned', async () => { + const stringBody = { error: 'msg' }; + const fakeFailedReturnValue = Promise.resolve({ + ok: false, + status: 502, + statusText: 'Retry Code', + headers: new Headers({ + 'content-type': 'application/json', + }), + json: () => Promise.resolve(stringBody), + text: () => Promise.resolve(JSON.stringify(stringBody)), + }); + + const fakeSuccessfulReturnValue = Promise.resolve({ + json: () => Promise.resolve({}), + text: () => Promise.resolve(JSON.stringify({})), + ok: true, + status: 200, + headers: new Headers({ + 'content-type': 'application/json', + }), + }); + + // Mock return 10 times + MockFetch.mockReturnValue(fakeFailedReturnValue); + MockFetch.mockReturnValue(fakeSuccessfulReturnValue); + + const output = await defaultRequestHandler('http://test.com', {} as RequestOptions); + + expect(output).toMatchObject({ + body: {}, + headers: { 'content-type': 'application/json' }, + status: 200, + }); + }); + + it('should return a default error if retries are unsuccessful', async () => { + const stringBody = { error: 'msg' }; + const fakeReturnValue = Promise.resolve({ + ok: false, + status: 429, + statusText: 'Retry Code', + headers: new Headers({ + 'content-type': 'application/json', + }), + json: () => Promise.resolve(stringBody), + text: () => Promise.resolve(JSON.stringify(stringBody)), + }); + + // Mock return 10 times + MockFetch.mockReturnValue(fakeReturnValue); + + await expect(defaultRequestHandler('http://test.com', {} as RequestOptions)).rejects.toThrow({ + message: + 'Could not successfully complete this request due to Error 429. Check the applicable rate limits for this endpoint.', + name: 'Error', + }); + + MockFetch.mockRestore(); + }); + it('should return correct properties if request is valid', async () => { MockFetch.mockReturnValueOnce( Promise.resolve({ @@ -140,7 +310,32 @@ describe('defaultRequestHandler', () => { expect(output).toMatchObject({ body: {}, - headers: {}, + headers: { 'content-type': 'application/json' }, + status: 200, + }); + }); + + it('should return correct properties as stream if request is valid', async () => { + MockFetch.mockReturnValueOnce( + Promise.resolve({ + body: 'text', + json: () => Promise.resolve({}), + text: () => Promise.resolve(JSON.stringify({})), + ok: true, + status: 200, + headers: new Headers({ + 'content-type': 'application/json', + }), + }), + ); + + const output = await defaultRequestHandler('http://test.com', { + asStream: true, + } as RequestOptions); + + expect(output).toMatchObject({ + body: 'text', + headers: { 'content-type': 'application/json' }, status: 200, }); }); From 164d7e9a1d817716f75403e16af74be6b34c5f54 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Thu, 31 Aug 2023 14:04:16 -0400 Subject: [PATCH 12/17] Debug browser tests --- packages/rest/test/e2e/browser/resources/Projects.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/rest/test/e2e/browser/resources/Projects.ts b/packages/rest/test/e2e/browser/resources/Projects.ts index 48bc9f950..694f128f9 100644 --- a/packages/rest/test/e2e/browser/resources/Projects.ts +++ b/packages/rest/test/e2e/browser/resources/Projects.ts @@ -34,6 +34,7 @@ describe('Projects API', () => { const response: Record = await page.evaluate( async ([host, token, testId]) => { + console.log(window); // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const { Gitlab } = window.gitbeaker; From 1162971396233eab863ba2ef6f082a5e7d7bac32 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Fri, 1 Sep 2023 18:00:41 -0400 Subject: [PATCH 13/17] Removing debug message --- packages/rest/test/e2e/browser/resources/Projects.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rest/test/e2e/browser/resources/Projects.ts b/packages/rest/test/e2e/browser/resources/Projects.ts index 694f128f9..48bc9f950 100644 --- a/packages/rest/test/e2e/browser/resources/Projects.ts +++ b/packages/rest/test/e2e/browser/resources/Projects.ts @@ -34,7 +34,6 @@ describe('Projects API', () => { const response: Record = await page.evaluate( async ([host, token, testId]) => { - console.log(window); // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const { Gitlab } = window.gitbeaker; From ed813f17f987055b35929dd83eed2eeb35e27f10 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Fri, 6 Oct 2023 23:18:08 -0400 Subject: [PATCH 14/17] Add missing imports --- packages/rest/test/e2e/browser/assets/test-import.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/rest/test/e2e/browser/assets/test-import.html b/packages/rest/test/e2e/browser/assets/test-import.html index 74c00624f..58b5d4983 100644 --- a/packages/rest/test/e2e/browser/assets/test-import.html +++ b/packages/rest/test/e2e/browser/assets/test-import.html @@ -11,6 +11,8 @@ "imports": { "qs": "https://esm.sh/qs?min", "xcase": "https://esm.sh/xcase?min", + "async-sema": "https://esm.sh/async-sema?min", + "micromatch": "https://esm.sh/micromatch?min", "@gitbeaker/requester-utils": "../../../../../requester-utils/dist/index.mjs", "@gitbeaker/core": "../../../../../core/dist/index.mjs" } From 8509ed5b6e4941d4cf3c7b5533946e19f55d3f7c Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Sat, 7 Oct 2023 00:29:16 -0400 Subject: [PATCH 15/17] Adding docs for rate limits --- packages/rest/README.md | 68 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/packages/rest/README.md b/packages/rest/README.md index ce64f94db..3bf73117b 100644 --- a/packages/rest/README.md +++ b/packages/rest/README.md @@ -76,6 +76,7 @@ - [API Client](#api-client) - [Expanded Payloads](#expanded-payloads) - [Pagination](#pagination) + - [Rate Limits](#rate-limits) - [Error Handling](#error-handling) - [Examples](#examples) - [Testing](../../docs/TESTING.md) @@ -155,6 +156,7 @@ Available instantiating options: | `queryTimeout` | Yes | `300000` | Query Timeout in ms | | `profileToken` | Yes | N/A | [Requests Profiles Token](https://docs.gitlab.com/ee/administration/monitoring/performance/request_profiling.html) | | `profileMode` | Yes | `execution` | [Requests Profiles Token](https://docs.gitlab.com/ee/administration/monitoring/performance/request_profiling.html) | +| `rateLimits` | No | [DEFAULT_RATE_LIMITS](#rate-limits) | Global and endpoint specific adjustable rate limits | > \*One of these options must be supplied. @@ -267,6 +269,72 @@ const { data } = await api.Projects.all({ }); ``` +### Rate Limits + +Rate limits are completely customizable, and are used to limit the request rate between consecutive API requests within the library. By default, all non-specified endpoints use a 3000 rps rate limit, while certain endpoints have a much smaller rate as dictated by the [Gitlab Docs](https://docs.gitlab.com/ee/security/rate_limits.html). See below for the default values: + +```js +const DEFAULT_RATE_LIMITS = Object.freeze({ + // Default rate limit + '**': 3000, + + // Import/Export + 'projects/import': 6, + 'projects/*/export': 6, + 'projects/*/download': 1, + 'groups/import': 6, + 'groups/*/export': 6, + 'groups/*/download': 1, + + // Note creation + 'projects/*/issues/*/notes': { + method: 'post', + limit: 300, + }, + 'projects/*/snippets/*/notes': { + method: 'post', + limit: 300, + }, + 'projects/*/merge_requests/*/notes': { + method: 'post', + limit: 300, + }, + 'groups/*/epics/*/notes': { + method: 'post', + limit: 300, + }, + + // Repositories - get file archive + 'projects/*/repository/archive*': 5, + + // Project Jobs + 'projects/*/jobs': 600, + + // Member deletion + 'projects/*/members': 60, + 'groups/*/members': 60, +}); +``` + +Rate limits can be override when instantiating a API wrapper. For ease of use, these limits are configured using glob patterns, and can be formatted in two ways. + +1. The glob for the endpoint with the corresponding rate per second +2. The glob for the endpoint, with an object specifying the specific method for the endpoint and the corresponding rate limit + +```js +const api = new Projects({ + token: 1234 + rateLimits: { + '*': 30, + 'projects/import/*': 40, + 'projects/*/issues/*/notes': { + method: 'post', + limit: 300, + }, + } +}) +``` + ### Error Handling Request errors are returned back within a plain Error instance, using the cause to hold the original response and a text description of the error pulled from the response's error or message fields if JSON, or its plain text value: From f39fb9eca535bab13c64ce47af1c621c4e6e47cc Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Sat, 7 Oct 2023 00:32:29 -0400 Subject: [PATCH 16/17] Edits --- packages/rest/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rest/README.md b/packages/rest/README.md index 3bf73117b..30a4278df 100644 --- a/packages/rest/README.md +++ b/packages/rest/README.md @@ -271,7 +271,7 @@ const { data } = await api.Projects.all({ ### Rate Limits -Rate limits are completely customizable, and are used to limit the request rate between consecutive API requests within the library. By default, all non-specified endpoints use a 3000 rps rate limit, while certain endpoints have a much smaller rate as dictated by the [Gitlab Docs](https://docs.gitlab.com/ee/security/rate_limits.html). See below for the default values: +Rate limits are completely customizable, and are used to limit the request rate between consecutive API requests within the library. By default, all non-specified endpoints use a 3000 rps rate limit, while some endpoints have much smaller rates as dictated by the [Gitlab Docs](https://docs.gitlab.com/ee/security/rate_limits.html). See below for the default values: ```js const DEFAULT_RATE_LIMITS = Object.freeze({ @@ -316,7 +316,7 @@ const DEFAULT_RATE_LIMITS = Object.freeze({ }); ``` -Rate limits can be override when instantiating a API wrapper. For ease of use, these limits are configured using glob patterns, and can be formatted in two ways. +Rate limits can be overridden when instantiating a API wrapper. For ease of use, these limits are configured using glob patterns, and can be formatted in two ways. 1. The glob for the endpoint with the corresponding rate per second 2. The glob for the endpoint, with an object specifying the specific method for the endpoint and the corresponding rate limit @@ -325,7 +325,7 @@ Rate limits can be override when instantiating a API wrapper. For ease of use, t const api = new Projects({ token: 1234 rateLimits: { - '*': 30, + '**': 30, 'projects/import/*': 40, 'projects/*/issues/*/notes': { method: 'post', From 06c1d3de662ce225ef557e1024fc3f560e462634 Mon Sep 17 00:00:00 2001 From: Justin Dalrymple Date: Sat, 7 Oct 2023 00:33:17 -0400 Subject: [PATCH 17/17] Improving docs --- packages/rest/README.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/rest/README.md b/packages/rest/README.md index 30a4278df..2ad89c17a 100644 --- a/packages/rest/README.md +++ b/packages/rest/README.md @@ -322,17 +322,17 @@ Rate limits can be overridden when instantiating a API wrapper. For ease of use, 2. The glob for the endpoint, with an object specifying the specific method for the endpoint and the corresponding rate limit ```js -const api = new Projects({ - token: 1234 +const api = new Gitlab({ + token: 'token', rateLimits: { - '**': 30, - 'projects/import/*': 40, - 'projects/*/issues/*/notes': { - method: 'post', - limit: 300, - }, - } -}) + '**': 30, + 'projects/import/*': 40, + 'projects/*/issues/*/notes': { + method: 'post', + limit: 300, + }, + }, +}); ``` ### Error Handling