diff --git a/README.md b/README.md index 26359238..5f0f8513 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ [![@latest](https://img.shields.io/npm/v/@octokit/plugin-throttling.svg)](https://www.npmjs.com/package/@octokit/plugin-throttling) [![Build Status](https://github.com/octokit/plugin-throttling.js/workflows/Test/badge.svg)](https://github.com/octokit/plugin-throttling.js/actions?workflow=Test) -Implements all [recommended best practices](https://docs.github.com/en/rest/guides/best-practices-for-integrators) to prevent hitting abuse rate limits. +Implements all [recommended best practices](https://docs.github.com/en/rest/guides/best-practices-for-integrators) to prevent hitting secondary rate limits. ## Usage @@ -44,7 +44,7 @@ const { throttling } = require("@octokit/plugin-throttling"); The code below creates a "Hello, world!" issue on every repository in a given organization. Without the throttling plugin it would send many requests in parallel and would hit rate limits very quickly. But the `@octokit/plugin-throttling` slows down your requests according to the official guidelines, so you don't get blocked before your quota is exhausted. -The `throttle.onAbuseLimit` and `throttle.onRateLimit` options are required. Return `true` to automatically retry the request after `retryAfter` seconds. +The `throttle.onSecondaryRateLimit` and `throttle.onRateLimit` options are required. Return `true` to automatically retry the request after `retryAfter` seconds. ```js const MyOctokit = Octokit.plugin(throttling); @@ -63,10 +63,10 @@ const octokit = new MyOctokit({ return true; } }, - onAbuseLimit: (retryAfter, options, octokit) => { + onSecondaryRateLimit: (retryAfter, options, octokit) => { // does not retry, only logs a warning octokit.log.warn( - `Abuse detected for request ${options.method} ${options.url}` + `SecondaryRateLimit detected for request ${options.method} ${options.url}` ); }, }, @@ -119,7 +119,7 @@ connection.on("error", err => console.error(err)); const octokit = new MyOctokit({ auth: 'secret123' throttle: { - onAbuseLimit: (retryAfter, options, octokit) => { + onSecondaryRateLimit: (retryAfter, options, octokit) => { /* ... */ }, onRateLimit: (retryAfter, options, octokit) => { diff --git a/src/index.ts b/src/index.ts index bd4bbfb8..1674ee94 100644 --- a/src/index.ts +++ b/src/index.ts @@ -68,7 +68,7 @@ export function throttling(octokit: Octokit, octokitOptions = {}) { { clustering: connection != null, triggersNotification, - minimumAbuseRetryAfter: 5, + minimumSecondaryRateRetryAfter: 5, retryAfterBaseValue: 1000, retryLimiter: new Bottleneck(), id, @@ -79,16 +79,17 @@ export function throttling(octokit: Octokit, octokitOptions = {}) { ); if ( - typeof state.onAbuseLimit !== "function" || + (typeof state.onSecondaryRateLimit !== "function" && + typeof state.onAbuseLimit !== "function") || typeof state.onRateLimit !== "function" ) { throw new Error(`octokit/plugin-throttling error: - You must pass the onAbuseLimit and onRateLimit error handlers. + You must pass the onSecondaryRateLimit and onRateLimit error handlers. See https://github.com/octokit/rest.js#throttling const octokit = new Octokit({ throttle: { - onAbuseLimit: (retryAfter, options) => {/* ... */}, + onSecondaryRateLimit: (retryAfter, options) => {/* ... */}, onRateLimit: (retryAfter, options) => {/* ... */} } }) @@ -98,7 +99,16 @@ export function throttling(octokit: Octokit, octokitOptions = {}) { const events = {}; const emitter = new Bottleneck.Events(events); // @ts-ignore - events.on("abuse-limit", state.onAbuseLimit); + events.on( + "secondary-limit", + state.onSecondaryRateLimit || + function (...args: any[]) { + console.warn( + "[@octokit/plugin-throttling] `onAbuseLimit()` is deprecated and will be removed in a future release of `@octokit/plugin-throttling`, please use the `onSecondaryRateLimit` handler instead" + ); + return state.onAbuseLimit(...args); + } + ); // @ts-ignore events.on("rate-limit", state.onRateLimit); // @ts-ignore @@ -122,17 +132,17 @@ export function throttling(octokit: Octokit, octokitOptions = {}) { const { wantRetry, retryAfter } = await (async function () { if (/\bsecondary rate\b/i.test(error.message)) { - // The user has hit the abuse rate limit. (REST and GraphQL) - // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits + // The user has hit the secondary rate limit. (REST and GraphQL) + // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits - // The Retry-After header can sometimes be blank when hitting an abuse limit, + // The Retry-After header can sometimes be blank when hitting a secondary rate limit, // but is always present after 2-3s, so make sure to set `retryAfter` to at least 5s by default. const retryAfter = Math.max( ~~error.response.headers["retry-after"], - state.minimumAbuseRetryAfter + state.minimumSecondaryRateRetryAfter ); const wantRetry = await emitter.trigger( - "abuse-limit", + "secondary-limit", retryAfter, options, octokit diff --git a/test/events.test.ts b/test/events.test.ts index 3043e52e..e6cb7742 100644 --- a/test/events.test.ts +++ b/test/events.test.ts @@ -3,7 +3,7 @@ import { TestOctokit } from "./octokit"; describe("Events", function () { it("Should support non-limit 403s", async function () { const octokit = new TestOctokit({ - throttle: { onAbuseLimit: () => 1, onRateLimit: () => 1 }, + throttle: { onSecondaryRateLimit: () => 1, onRateLimit: () => 1 }, }); let caught = false; @@ -32,144 +32,148 @@ describe("Events", function () { ]); }); - describe("'abuse-limit'", function () { - it("Should detect abuse limit and broadcast event", async function () { - let eventCount = 0; - const octokit = new TestOctokit({ - throttle: { - onAbuseLimit: ( - retryAfter: number, - options: object, - octokitFromOptions: InstanceType - ) => { - expect(octokit).toBe(octokitFromOptions); - expect(retryAfter).toEqual(60); - expect(options).toMatchObject({ - method: "GET", - url: "/route2", - request: { retryCount: 0 }, - }); - eventCount++; + describe.each(["onSecondaryRateLimit", "onAbuseLimit"])( + "secondary-limit", + function (eventHandlerName) { + it("Should detect SecondaryRate limit and broadcast event", async function () { + let eventCount = 0; + + const octokit = new TestOctokit({ + throttle: { + [eventHandlerName]: ( + retryAfter: number, + options: object, + octokitFromOptions: InstanceType + ) => { + expect(octokit).toBe(octokitFromOptions); + expect(retryAfter).toEqual(60); + expect(options).toMatchObject({ + method: "GET", + url: "/route2", + request: { retryCount: 0 }, + }); + eventCount++; + }, + onRateLimit: () => 1, }, - onRateLimit: () => 1, - }, - }); + }); - await octokit.request("GET /route1", { - request: { - responses: [{ status: 201, headers: {}, data: {} }], - }, - }); - try { - await octokit.request("GET /route2", { + await octokit.request("GET /route1", { request: { - responses: [ - { - status: 403, - headers: { "retry-after": "60" }, - data: { - message: "You have exceeded a secondary rate limit", - }, - }, - ], + responses: [{ status: 201, headers: {}, data: {} }], }, }); - throw new Error("Should not reach this point"); - } catch (error: any) { - expect(error.status).toEqual(403); - } - - expect(eventCount).toEqual(1); - }); + try { + await octokit.request("GET /route2", { + request: { + responses: [ + { + status: 403, + headers: { "retry-after": "60" }, + data: { + message: "You have exceeded a secondary rate limit", + }, + }, + ], + }, + }); + throw new Error("Should not reach this point"); + } catch (error: any) { + expect(error.status).toEqual(403); + } + + expect(eventCount).toEqual(1); + }); - it("Should ensure retryAfter is a minimum of 5s", async function () { - let eventCount = 0; - const octokit = new TestOctokit({ - throttle: { - onAbuseLimit: (retryAfter: number, options: object) => { - expect(retryAfter).toEqual(5); - expect(options).toMatchObject({ - method: "GET", - url: "/route2", - request: { retryCount: 0 }, - }); - eventCount++; + it("Should ensure retryAfter is a minimum of 5s", async function () { + let eventCount = 0; + const octokit = new TestOctokit({ + throttle: { + [eventHandlerName]: (retryAfter: number, options: object) => { + expect(retryAfter).toEqual(5); + expect(options).toMatchObject({ + method: "GET", + url: "/route2", + request: { retryCount: 0 }, + }); + eventCount++; + }, + onRateLimit: () => 1, }, - onRateLimit: () => 1, - }, - }); + }); - await octokit.request("GET /route1", { - request: { - responses: [{ status: 201, headers: {}, data: {} }], - }, - }); - try { - await octokit.request("GET /route2", { + await octokit.request("GET /route1", { request: { - responses: [ - { - status: 403, - headers: { "retry-after": "2" }, - data: { - message: "You have exceeded a secondary rate limit", - }, - }, - ], + responses: [{ status: 201, headers: {}, data: {} }], }, }); - throw new Error("Should not reach this point"); - } catch (error: any) { - expect(error.status).toEqual(403); - } - - expect(eventCount).toEqual(1); - }); + try { + await octokit.request("GET /route2", { + request: { + responses: [ + { + status: 403, + headers: { "retry-after": "2" }, + data: { + message: "You have exceeded a secondary rate limit", + }, + }, + ], + }, + }); + throw new Error("Should not reach this point"); + } catch (error: any) { + expect(error.status).toEqual(403); + } + + expect(eventCount).toEqual(1); + }); - it("Should broadcast retryAfter of 5s even when the header is missing", async function () { - let eventCount = 0; - const octokit = new TestOctokit({ - throttle: { - onAbuseLimit: (retryAfter: number, options: object) => { - expect(retryAfter).toEqual(5); - expect(options).toMatchObject({ - method: "GET", - url: "/route2", - request: { retryCount: 0 }, - }); - eventCount++; + it("Should broadcast retryAfter of 5s even when the header is missing", async function () { + let eventCount = 0; + const octokit = new TestOctokit({ + throttle: { + [eventHandlerName]: (retryAfter: number, options: object) => { + expect(retryAfter).toEqual(5); + expect(options).toMatchObject({ + method: "GET", + url: "/route2", + request: { retryCount: 0 }, + }); + eventCount++; + }, + onRateLimit: () => 1, }, - onRateLimit: () => 1, - }, - }); + }); - await octokit.request("GET /route1", { - request: { - responses: [{ status: 201, headers: {}, data: {} }], - }, - }); - try { - await octokit.request("GET /route2", { + await octokit.request("GET /route1", { request: { - responses: [ - { - status: 403, - headers: {}, - data: { - message: "You have exceeded a secondary rate limit", - }, - }, - ], + responses: [{ status: 201, headers: {}, data: {} }], }, }); - throw new Error("Should not reach this point"); - } catch (error: any) { - expect(error.status).toEqual(403); - } - - expect(eventCount).toEqual(1); - }); - }); + try { + await octokit.request("GET /route2", { + request: { + responses: [ + { + status: 403, + headers: {}, + data: { + message: "You have exceeded a secondary rate limit", + }, + }, + ], + }, + }); + throw new Error("Should not reach this point"); + } catch (error: any) { + expect(error.status).toEqual(403); + } + + expect(eventCount).toEqual(1); + }); + } + ); describe("'rate-limit'", function () { it("Should detect rate limit exceeded and broadcast event", async function () { @@ -191,7 +195,7 @@ describe("Events", function () { }); eventCount++; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); const t0 = Date.now(); diff --git a/test/plugin.test.ts b/test/plugin.test.ts index 8fa1d03e..8858a89c 100644 --- a/test/plugin.test.ts +++ b/test/plugin.test.ts @@ -37,19 +37,24 @@ describe("General", function () { it("Should require the user to pass both limit handlers", function () { const message = - "You must pass the onAbuseLimit and onRateLimit error handlers"; + "You must pass the onSecondaryRateLimit and onRateLimit error handlers"; expect(() => new TestOctokit()).toThrow(message); expect(() => new TestOctokit({ throttle: {} })).toThrow(message); expect( - () => new TestOctokit({ throttle: { onAbuseLimit: 5, onRateLimit: 5 } }) + () => + new TestOctokit({ + throttle: { onSecondaryRateLimit: 5, onRateLimit: 5 }, + }) ).toThrow(message); expect( () => - new TestOctokit({ throttle: { onAbuseLimit: 5, onRateLimit: () => 1 } }) + new TestOctokit({ + throttle: { onSecondaryRateLimit: 5, onRateLimit: () => 1 }, + }) ).toThrow(message); expect( - () => new TestOctokit({ throttle: { onAbuseLimit: () => 1 } }) + () => new TestOctokit({ throttle: { onSecondaryRateLimit: () => 1 } }) ).toThrow(message); expect( () => new TestOctokit({ throttle: { onRateLimit: () => 1 } }) @@ -57,7 +62,7 @@ describe("General", function () { expect( () => new TestOctokit({ - throttle: { onAbuseLimit: () => 1, onRateLimit: () => 1 }, + throttle: { onSecondaryRateLimit: () => 1, onRateLimit: () => 1 }, }) ).not.toThrow(); }); @@ -66,7 +71,7 @@ describe("General", function () { describe("Github API best practices", function () { it("Should linearize requests", async function () { const octokit = new TestOctokit({ - throttle: { onAbuseLimit: () => 1, onRateLimit: () => 1 }, + throttle: { onSecondaryRateLimit: () => 1, onRateLimit: () => 1 }, }); const req1 = octokit.request("GET /route1", { request: { @@ -101,7 +106,7 @@ describe("Github API best practices", function () { const octokit = new TestOctokit({ throttle: { write: new Bottleneck.Group({ minTime: 50 }), - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, onRateLimit: () => 1, }, }); @@ -151,7 +156,7 @@ describe("Github API best practices", function () { throttle: { write: new Bottleneck.Group({ minTime: 50 }), notifications: new Bottleneck.Group({ minTime: 100 }), - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, onRateLimit: () => 1, }, }); @@ -227,7 +232,7 @@ describe("Github API best practices", function () { const octokit = new TestOctokit({ throttle: { search: new Bottleneck.Group({ minTime: 50 }), - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, onRateLimit: () => 1, }, }); @@ -281,7 +286,7 @@ describe("Github API best practices", function () { throttle: { write: new Bottleneck.Group({ minTime: 50 }), notifications: new Bottleneck.Group({ minTime: 150 }), - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, onRateLimit: () => 1, }, }); diff --git a/test/retry.test.ts b/test/retry.test.ts index 89fb1655..7643da62 100644 --- a/test/retry.test.ts +++ b/test/retry.test.ts @@ -3,13 +3,13 @@ import { TestOctokit } from "./octokit"; describe("Retry", function () { describe("REST", function () { - it("Should retry 'abuse-limit' and succeed", async function () { + it("Should retry 'secondary-limit' and succeed", async function () { let eventCount = 0; const octokit = new TestOctokit({ throttle: { - minimumAbuseRetryAfter: 0, + minimumSecondaryRateRetryAfter: 0, retryAfterBaseValue: 50, - onAbuseLimit: (retryAfter: number, options: object) => { + onSecondaryRateLimit: (retryAfter: number, options: object) => { expect(options).toMatchObject({ method: "GET", url: "/route", @@ -49,13 +49,13 @@ describe("Retry", function () { expect(ms).toBeGreaterThan(20); }); - it("Should retry 'abuse-limit' twice and fail", async function () { + it("Should retry 'secondary-limit' twice and fail", async function () { let eventCount = 0; const octokit = new TestOctokit({ throttle: { - minimumAbuseRetryAfter: 0, + minimumSecondaryRateRetryAfter: 0, retryAfterBaseValue: 50, - onAbuseLimit: (retryAfter: number, options: object) => { + onSecondaryRateLimit: (retryAfter: number, options: object) => { expect(options).toMatchObject({ method: "GET", url: "/route", @@ -128,7 +128,7 @@ describe("Retry", function () { eventCount++; return true; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); @@ -178,7 +178,7 @@ describe("Retry", function () { eventCount++; return true; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); @@ -228,7 +228,7 @@ describe("Retry", function () { eventCount++; return true; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); @@ -272,7 +272,7 @@ describe("Retry", function () { eventCount++; return true; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); @@ -310,7 +310,7 @@ describe("Retry", function () { eventCount++; return true; }, - onAbuseLimit: () => 1, + onSecondaryRateLimit: () => 1, }, }); @@ -342,17 +342,17 @@ describe("Retry", function () { expect(octokit.__requestLog).toStrictEqual(["START POST /graphql"]); }); - it("Should retry 403 Forbidden errors on abuse limit", async function () { + it("Should retry 403 Forbidden errors on SecondaryRate limit", async function () { let eventCount = 0; const octokit = new TestOctokit({ throttle: { write: new Bottleneck.Group({ minTime: 50 }), - onAbuseLimit: (retryAfter: number, options: object) => { + onSecondaryRateLimit: (retryAfter: number, options: object) => { eventCount++; return true; }, onRateLimit: () => 1, - minimumAbuseRetryAfter: 0, + minimumSecondaryRateRetryAfter: 0, retryAfterBaseValue: 50, }, }); @@ -368,7 +368,7 @@ describe("Retry", function () { message: "You have exceeded a secondary rate limit. Please wait a few minutes before you try again.", documentation_url: - "https://developer.github.com/v3/#abuse-rate-limits", + "https://developer.github.com/v3/#secondary-rate-limits", }, }, { status: 200, headers: {}, data: { message: "Success!" } },