Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rate limit feature to be more browser freindly #3428

Merged
merged 9 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .all-contributorsrc
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,5 @@
"contributions": ["code"]
}
],
"contributorsPerLine": 26
"contributorsPerLine": 300
}

16 changes: 8 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,28 +38,28 @@
"@auto-it/omit-commits": "^11.0.4",
"@auto-it/omit-release-notes": "^11.0.4",
"@auto-it/released": "^11.0.4",
"@swc/core": "^1.3.100",
"@swc/core": "^1.3.101",
"@swc/jest": "^0.2.29",
"@types/jest": "^29.5.11",
"@typescript-eslint/eslint-plugin": "^6.13.2",
"@typescript-eslint/parser": "^6.13.2",
"@typescript-eslint/eslint-plugin": "^6.15.0",
"@typescript-eslint/parser": "^6.15.0",
"auto": "^11.0.4",
"eslint": "^8.55.0",
"eslint": "^8.56.0",
"eslint-config-airbnb-base": "^15.0.0",
"eslint-config-prettier": "^9.1.0",
"eslint-import-resolver-typescript": "^3.6.1",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-import": "^2.29.1",
"eslint-plugin-jest": "^27.6.0",
"eslint-plugin-jest-extended": "^2.0.0",
"eslint-plugin-prettier": "^5.0.1",
"eslint-plugin-prettier": "^5.1.0",
"husky": "^8.0.3",
"jest": "^29.7.0",
"jest-extended": "^4.0.2",
"jest-junit": "^16.0.0",
"lerna": "^8.0.0",
"lerna": "^8.0.1",
"lint-staged": "^15.2.0",
"nx": "17.2.8",
"prettier": "^3.1.0",
"prettier": "^3.1.1",
"typescript": "^5.3.3"
},
"packageManager": "[email protected]"
Expand Down
15 changes: 10 additions & 5 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,14 @@
"exports": {
"./map.json": "./dist/map.json",
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.mjs",
"require": "./dist/index.js"
"import": {
"types": "./dist/index.d.mts",
"default": "./dist/index.mjs"
},
"require": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
}
}
},
"files": [
Expand Down Expand Up @@ -60,10 +65,10 @@
"xcase": "^2.0.1"
},
"devDependencies": {
"@types/node": "^20.10.4",
"@types/node": "^20.10.5",
"get-param-names": "github:jdalrymple/get-param-names#1-improve-functionality",
"tsup": "^8.0.1",
"tsx": "^4.6.2",
"tsx": "^4.7.0",
"typescript": "^5.3.3"
}
}
18 changes: 8 additions & 10 deletions packages/core/src/infrastructure/RequestHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,22 +92,20 @@ export type GitlabAPIExpandedResponse<T, E extends boolean | void, P> = E extend
: ExpandedResponse<T>
: T;

export type GitlabAPISingleResponse<
T,
C extends boolean | void,
E extends boolean | void,
> = T extends Record<string, unknown>
? GitlabAPIExpandedResponse<CamelizedResponse<T, C>, E, undefined>
: GitlabAPIExpandedResponse<T, E, undefined>;
export type GitlabAPISingleResponse<T, C extends boolean | void, E extends boolean | void> =
T extends Record<string, unknown>
? GitlabAPIExpandedResponse<CamelizedResponse<T, C>, E, undefined>
: GitlabAPIExpandedResponse<T, E, undefined>;

export type GitlabAPIMultiResponse<
T,
C extends boolean | void,
E extends boolean | void,
P extends PaginationTypes | void,
> = T extends Record<string, unknown>
? GitlabAPIExpandedResponse<CamelizedResponse<T, C>[], E, P>
: GitlabAPIExpandedResponse<T[], E, P>;
> =
T extends Record<string, unknown>
? GitlabAPIExpandedResponse<CamelizedResponse<T, C>[], E, P>
: GitlabAPIExpandedResponse<T[], E, P>;

export type GitlabAPIResponse<
T,
Expand Down
17 changes: 11 additions & 6 deletions packages/requester-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@
"types": "./dist/index.d.ts",
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.mjs",
"require": "./dist/index.js"
"import": {
"types": "./dist/index.d.mts",
"default": "./dist/index.mjs"
},
"require": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
}
}
},
"files": [
Expand All @@ -51,13 +56,13 @@
"release": "auto shipit"
},
"dependencies": {
"async-sema": "^3.1.1",
"micromatch": "^4.0.5",
"picomatch-browser": "^2.2.6",
"qs": "^6.11.2",
"rate-limiter-flexible": "^4.0.0",
"xcase": "^2.0.1"
},
"devDependencies": {
"@types/node": "^20.10.4",
"@types/node": "^20.10.5",
"tsup": "^8.0.1",
"typescript": "^5.3.3"
}
Expand Down
40 changes: 24 additions & 16 deletions packages/requester-utils/src/RequesterUtils.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { stringify } from 'qs';
import { decamelizeKeys } from 'xcase';
import { RateLimit } from 'async-sema';
import micromatch from 'micromatch';
import { RateLimiterMemory, RateLimiterQueue } from 'rate-limiter-flexible';
import Picomatch from 'picomatch-browser';

const { isMatch: isGlobMatch } = Picomatch;

// Types
export type RateLimiters = Record<
string,
ReturnType<typeof RateLimit> | { method: string; limit: ReturnType<typeof RateLimit> }
>;
export type RateLimiterFn = () => Promise<number>;
export type RateLimiters = Record<string, RateLimiterFn | { method: string; limit: RateLimiterFn }>;
export type RateLimitOptions = Record<string, number | { method: string; limit: number }>;

export type ResponseBodyTypes =
Expand Down Expand Up @@ -57,7 +57,7 @@ export type RequestOptions = {
body?: string | FormData;
asStream?: boolean;
signal?: AbortSignal;
rateLimiters?: Record<string, ReturnType<typeof RateLimit>>;
rateLimiters?: Record<string, RateLimiterFn>;
};

export interface RequesterType {
Expand Down Expand Up @@ -89,6 +89,14 @@ export type RequestHandlerFn<T extends ResponseBodyTypes = ResponseBodyTypes> =
) => Promise<FormattedResponse<T>>;

// Utility methods
export function generateRateLimiterFn(limit: number, interval: number) {
const limiter = new RateLimiterQueue(
new RateLimiterMemory({ points: limit, duration: interval }),
);

return () => limiter.removeTokens(1);
}

export function formatQuery(params: Record<string, unknown> = {}): string {
const decamelized = decamelizeKeys(params);

Expand Down Expand Up @@ -152,11 +160,11 @@ export function createRateLimiters(rateLimitOptions: RateLimitOptions = {}) {
const rateLimiters: RateLimiters = {};

Object.entries(rateLimitOptions).forEach(([key, config]) => {
if (typeof config === 'number') rateLimiters[key] = RateLimit(config, { timeUnit: 60000 });
if (typeof config === 'number') rateLimiters[key] = generateRateLimiterFn(config, 60);
else
rateLimiters[key] = {
method: config.method.toUpperCase(),
limit: RateLimit(config.limit, { timeUnit: 60000 }),
limit: generateRateLimiterFn(config.limit, 60),
};
});

Expand Down Expand Up @@ -219,16 +227,16 @@ export function getMatchingRateLimiter(
endpoint: string,
rateLimiters: RateLimiters = {},
method: string = 'GET',
): () => Promise<void> {
): RateLimiterFn {
const sortedEndpoints = Object.keys(rateLimiters).sort().reverse();
const match = sortedEndpoints.find((ep) => micromatch.isMatch(endpoint, ep));
const match = sortedEndpoints.find((ep) => isGlobMatch(endpoint, ep));
const rateLimitConfig = match && rateLimiters[match];

if (rateLimitConfig && typeof rateLimitConfig !== 'object') {
return rateLimitConfig;
}
if (rateLimitConfig && rateLimitConfig.method.toUpperCase() === method.toUpperCase()) {
if (typeof rateLimitConfig === 'function') return rateLimitConfig;

if (rateLimitConfig && rateLimitConfig?.method?.toUpperCase() === method.toUpperCase()) {
return rateLimitConfig.limit;
}
return RateLimit(3000, { timeUnit: 60000 });

return generateRateLimiterFn(3000, 60);
}
23 changes: 7 additions & 16 deletions packages/requester-utils/test/unit/RequesterUtils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as AsyncSema from 'async-sema';
import { RateLimiterMemory } from 'rate-limiter-flexible';
import {
RequestOptions,
ResourceOptions,
Expand All @@ -10,6 +10,8 @@ import {
presetResourceArguments,
} from '../../src/RequesterUtils';

jest.mock('rate-limiter-flexible');

const methods = ['get', 'put', 'patch', 'delete', 'post'];

describe('defaultOptionsHandler', () => {
Expand Down Expand Up @@ -193,8 +195,6 @@ describe('createInstance', () => {
});

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,
Expand All @@ -212,9 +212,6 @@ describe('createInstance', () => {

await requester.get(testEndpoint, {});

expect(rateLimitSpy).toHaveBeenCalledWith(10, { timeUnit: 60000 });
expect(rateLimitSpy).toHaveBeenCalledWith(40, { timeUnit: 60000 });

expect(requestHandler).toHaveBeenCalledWith(testEndpoint, {
rateLimiters: {
'*': expect.toBeFunction(),
Expand All @@ -229,8 +226,6 @@ describe('createInstance', () => {

describe('createRateLimiters', () => {
it('should create rate limiter functions when configured', () => {
const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit');

const limiters = createRateLimiters({
'*': 40,
'projects/*/test': {
Expand All @@ -239,8 +234,8 @@ describe('createRateLimiters', () => {
},
});

expect(rateLimitSpy).toHaveBeenCalledWith(10, { timeUnit: 60000 });
expect(rateLimitSpy).toHaveBeenCalledWith(40, { timeUnit: 60000 });
expect(RateLimiterMemory).toHaveBeenCalledWith({ points: 10, duration: 60 });
expect(RateLimiterMemory).toHaveBeenCalledWith({ points: 40, duration: 60 });

expect(limiters).toStrictEqual({
'*': expect.toBeFunction(),
Expand Down Expand Up @@ -338,11 +333,9 @@ describe('getMatchingRateLimiter', () => {
});

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(3000, { timeUnit: 60000 });
expect(RateLimiterMemory).toHaveBeenCalledWith({ points: 3000, duration: 60 });
});

it('should return the most specific rate limit', async () => {
Expand All @@ -358,11 +351,9 @@ describe('getMatchingRateLimiter', () => {
});

it('should return a default rate limit of 3000 rpm if nothing matches', () => {
const rateLimitSpy = jest.spyOn(AsyncSema, 'RateLimit');

getMatchingRateLimiter('endpoint', { someurl: jest.fn() });

expect(rateLimitSpy).toHaveBeenCalledWith(3000, { timeUnit: 60000 });
expect(RateLimiterMemory).toHaveBeenCalledWith({ points: 3000, duration: 60 });
});

it('should handle expanded rate limit options with a particular method and limit', async () => {
Expand Down
13 changes: 9 additions & 4 deletions packages/rest/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,14 @@
"types": "./dist/index.d.ts",
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.mjs",
"require": "./dist/index.js"
"import": {
"types": "./dist/index.d.mts",
"default": "./dist/index.mjs"
},
"require": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
}
}
},
"files": [
Expand Down Expand Up @@ -61,7 +66,7 @@
},
"devDependencies": {
"@playwright/test": "^1.40.1",
"@types/node": "^20.10.4",
"@types/node": "^20.10.5",
"tsup": "^8.0.1",
"typescript": "^5.3.3"
}
Expand Down
6 changes: 3 additions & 3 deletions packages/rest/src/Requester.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export async function defaultOptionsHandler(
resourceOptions.rejectUnauthorized != null &&
resourceOptions.rejectUnauthorized === false
) {
if (typeof window !== 'object') {
if (typeof window === 'undefined') {
const { Agent } = await import('https');

options.agent = new Agent({
Expand Down Expand Up @@ -103,7 +103,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 = getMatchingRateLimiter(endpoint, rateLimiters, method);
const rateLimit = getMatchingRateLimiter(endpoint, rateLimiters, method);
let baseUrl: string | undefined;

if (prefixUrl) baseUrl = prefixUrl.endsWith('/') ? prefixUrl : `${prefixUrl}/`;
Expand All @@ -119,7 +119,7 @@ export async function defaultRequestHandler(endpoint: string, options?: RequestO
for (let i = 0; i < maxRetries; i += 1) {
const request = new Request(url, { ...opts, method, mode });

await endpointRateLimit();
await rateLimit();

const response = await fetch(request).catch((e) => {
if (e.name === 'TimeoutError' || e.name === 'AbortError') {
Expand Down
4 changes: 2 additions & 2 deletions packages/rest/test/e2e/browser/assets/test-import.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +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",
"rate-limiter-flexible": "https://esm.sh/rate-limiter-flexible?min",
"picomatch-browser": "https://esm.sh/picomatch-browser?min",
"@gitbeaker/requester-utils": "../../../../../requester-utils/dist/index.mjs",
"@gitbeaker/core": "../../../../../core/dist/index.mjs"
}
Expand Down
Loading