fix: rate limit write function further

pvdlg · pvdlg · commit 3a91322ec96c · 2018-07-14T17:31:34.000-04:00
diff --git a/lib/definitions/rate-limit.js b/lib/definitions/rate-limit.js
@@ -0,0 +1,27 @@
+/**
+ * Default exponential backoff configuration for retries.
+ */
+const RETRY_CONF = {retries: 3, factor: 2, minTimeout: 1000};
+
+/**
+ * Rate limit per API endpoints.
+ *
+ * See {@link https://developer.github.com/v3/search/#rate-limit|Search API rate limit}.
+ * See {@link https://developer.github.com/v3/#rate-limiting|Rate limiting}.
+ */
+const RATE_LIMITS = {
+  search: ((60 * 1000) / 30) * 1.1, // 30 calls per minutes => 1 call every 2s + 10% safety margin
+  core: {
+    read: ((60 * 60 * 1000) / 5000) * 1.1, // 5000 calls per hour => 1 call per 720ms + 10% safety margin
+    write: 3000, // 1 call every 3 seconds
+  },
+};
+
+/**
+ * Global rate limit to prevent abuse.
+ *
+ * See {@link https://developer.github.com/v3/guides/best-practices-for-integrators/#dealing-with-abuse-rate-limits|Dealing with abuse rate limits}
+ */
+const GLOBAL_RATE_LIMIT = 1000;
+
+module.exports = {RETRY_CONF, RATE_LIMITS, GLOBAL_RATE_LIMIT};
diff --git a/lib/get-client.js b/lib/get-client.js
@@ -1,34 +1,14 @@
 const url = require('url');
-const {memoize} = require('lodash');
+const {memoize, get} = require('lodash');
 const Octokit = require('@octokit/rest');
 const pRetry = require('p-retry');
 const Bottleneck = require('bottleneck');
 const urljoin = require('url-join');
 const HttpProxyAgent = require('http-proxy-agent');
 const HttpsProxyAgent = require('https-proxy-agent');
 
-/**
- * Default exponential backoff configuration for retries.
- */
-const DEFAULT_RETRY = {retries: 3, factor: 2, minTimeout: 1000};
-
-/**
- * Rate limit per API endpoints.
- *
- * See {@link https://developer.github.com/v3/search/#rate-limit|Search API rate limit}.
- * See {@link https://developer.github.com/v3/#rate-limiting|Rate limiting}.
- */
-const RATE_LIMITS = {
-  search: (60 * 1000) / 30, // 30 calls per minutes => 1 call per 2s
-  core: (60 * 60 * 1000) / 5000, // 5000 calls per hour => 1 call per 720ms
-};
-
-/**
- * Global rate limit to prevent abuse.
- *
- * See {@link https://developer.github.com/v3/guides/best-practices-for-integrators/#dealing-with-abuse-rate-limits|Dealing with abuse rate limits}
- */
-const GLOBAL_RATE_LIMIT = 1000;
+const GH_ROUTES = require('@octokit/rest/lib/routes');
+const {RETRY_CONF, RATE_LIMITS, GLOBAL_RATE_LIMIT} = require('./definitions/rate-limit');
 
 /**
  * Http error codes for which to not retry.
@@ -41,35 +21,66 @@ const SKIP_RETRY_CODES = [400, 401, 403];
  * @param {Array} rate The rate limit group.
  * @param {String} limit The rate limits per API endpoints.
  * @param {Bottleneck} globalThrottler The global throttler.
+ *
  * @return {Bottleneck} The throller function for the given rate limit group.
  */
-const getThrottler = memoize((rate, limit, globalThrottler) =>
-  new Bottleneck({minTime: limit[rate]}).chain(globalThrottler)
+const getThrottler = memoize((rate, globalThrottler) =>
+  new Bottleneck({minTime: get(RATE_LIMITS, rate)}).chain(globalThrottler)
 );
 
+/**
+ * Determine if a call to a client function will trigger a read (`GET`) or a write (`POST`, `PATCH`, etc...) request.
+ *
+ * @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
+ * @param {String} command The client API command (for example the command for a call to `github.repos.get` is `get`).
+ *
+ * @return {String} `write` or `read` if there is rate limit configuration for this `endpoint` and `command`, `undefined` otherwise.
+ */
+const getAccess = (endpoint, command) => {
+  const method = GH_ROUTES[endpoint] && GH_ROUTES[endpoint][command] && GH_ROUTES[endpoint][command].method;
+  const access = method && method === 'GET' ? 'read' : 'write';
+  return RATE_LIMITS[endpoint][access] && access;
+};
+
+/**
+ * Get the limiter identifier associated with a client API call.
+ *
+ * @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
+ * @param {String} command The client API command (for example the command for a call to `github.repos.get` is `get`).
+ *
+ * @return {String} A string identifying the limiter to use for this `endpoint` and `command` (e.g. `search` or `core.write`).
+ */
+const getLimitKey = (endpoint, command) => {
+  return endpoint
+    ? [endpoint, RATE_LIMITS[endpoint] && getAccess(endpoint, command)].filter(Boolean).join('.')
+    : RATE_LIMITS[command]
+      ? command
+      : 'core';
+};
+
 /**
  * Create a`handler` for a `Proxy` wrapping an Octokit instance to:
  * - Recursively wrap the child objects of the Octokit instance in a `Proxy`
  * - Throttle and retry the Octokit instance functions
  *
- * @param {Object} retry The configuration to pass to `p-retry`.
- * @param {Array} limit The rate limits per API endpoints.
  * @param {Throttler} globalThrottler The throller function for the global rate limit.
- * @param {String} endpoint The API endpoint to handle.
+ * @param {String} limitKey The key to find the limit rate for the  API endpoint and method.
+ *
  * @return {Function} The `handler` for a `Proxy` wrapping an Octokit instance.
  */
-const handler = (retry, limit, globalThrottler, endpoint) => ({
+const handler = (globalThrottler, limitKey) => ({
   /**
    * If the target has the property as own, determine the rate limit based on the property name and recursively wrap the value in a `Proxy`. Otherwise returns the property value.
    *
    * @param {Object} target The target object.
    * @param {String} name The name of the property to get.
    * @param {Any} receiver The `Proxy` object.
+   *
    * @return {Any} The property value or a `Proxy` of the property value.
    */
   get: (target, name, receiver) =>
     Reflect.apply(Object.prototype.hasOwnProperty, target, [name])
-      ? new Proxy(target[name], handler(retry, limit, globalThrottler, endpoint || name))
+      ? new Proxy(target[name], handler(globalThrottler, getLimitKey(limitKey, name)))
       : Reflect.get(target, name, receiver),
 
   /**
@@ -78,11 +89,11 @@ const handler = (retry, limit, globalThrottler, endpoint) => ({
    * @param {Function} func The target function.
    * @param {Any} that The this argument for the call.
    * @param {Array} args The list of arguments for the call.
+   *
    * @return {Promise<Any>} The result of the function called.
    */
   apply: (func, that, args) => {
-    const throttler = getThrottler(limit[endpoint] ? endpoint : 'core', limit, globalThrottler);
-
+    const throttler = getThrottler(limitKey, globalThrottler);
     return pRetry(async () => {
       try {
         return await throttler.wrap(func)(...args);
@@ -92,19 +103,11 @@ const handler = (retry, limit, globalThrottler, endpoint) => ({
         }
         throw err;
       }
-    }, retry);
+    }, RETRY_CONF);
   },
 });
 
-module.exports = ({
-  githubToken,
-  githubUrl,
-  githubApiPathPrefix,
-  proxy,
-  retry = DEFAULT_RETRY,
-  limit = RATE_LIMITS,
-  globalLimit = GLOBAL_RATE_LIMIT,
-}) => {
+module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy} = {}) => {
   const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
   const github = new Octokit({
     baseUrl,
@@ -115,5 +118,5 @@ module.exports = ({
       : undefined,
   });
   github.authenticate({type: 'token', token: githubToken});
-  return new Proxy(github, handler(retry, limit, new Bottleneck({minTime: globalLimit})));
+  return new Proxy(github, handler(new Bottleneck({minTime: GLOBAL_RATE_LIMIT})));
 };
diff --git a/test/fail.test.js b/test/fail.test.js
@@ -5,17 +5,13 @@ import {stub} from 'sinon';
 import proxyquire from 'proxyquire';
 import SemanticReleaseError from '@semantic-release/error';
 import ISSUE_ID from '../lib/definitions/sr-issue-id';
-import getClient from '../lib/get-client';
 import {authenticate} from './helpers/mock-github';
+import rateLimit from './helpers/rate-limit';
 
 /* eslint camelcase: ["error", {properties: "never"}] */
 
 const fail = proxyquire('../lib/fail', {
-  './get-client': conf =>
-    getClient({
-      ...conf,
-      ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
-    }),
+  './get-client': proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit}),
 });
 
 // Save the current process.env
diff --git a/test/find-sr-issue.test.js b/test/find-sr-issue.test.js
@@ -2,22 +2,16 @@ import {escape} from 'querystring';
 import test from 'ava';
 import nock from 'nock';
 import {stub} from 'sinon';
+import proxyquire from 'proxyquire';
 import ISSUE_ID from '../lib/definitions/sr-issue-id';
 import findSRIssues from '../lib/find-sr-issues';
-import getClient from '../lib/get-client';
 import {authenticate} from './helpers/mock-github';
-
-/* eslint camelcase: ["error", {properties: "never"}] */
+import rateLimit from './helpers/rate-limit';
 
 // Save the current process.env
 const envBackup = Object.assign({}, process.env);
 const githubToken = 'github_token';
-const client = getClient({
-  githubToken,
-  retry: {retries: 3, factor: 2, minTimeout: 1, maxTimeout: 1},
-  globalLimit: 1,
-  limit: {search: 1, core: 1},
-});
+const client = proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit})({githubToken});
 
 test.beforeEach(t => {
   // Delete env variables in case they are on the machine running the tests
diff --git a/test/get-client.test.js b/test/get-client.test.js
@@ -9,7 +9,9 @@ import {stub, spy} from 'sinon';
 import proxyquire from 'proxyquire';
 import Proxy from 'proxy';
 import serverDestroy from 'server-destroy';
-import getClient from '../lib/get-client';
+import rateLimit from './helpers/rate-limit';
+
+const getClient = proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit});
 
 test.serial('Use a http proxy', async t => {
   const server = http.createServer();
@@ -33,7 +35,6 @@ test.serial('Use a http proxy', async t => {
     githubUrl: `http://localhost:${serverPort}`,
     githubApiPathPrefix: '',
     proxy: `http://localhost:${proxyPort}`,
-    retry: {retries: 1, factor: 1, minTimeout: 1, maxTimeout: 1},
   });
 
   await github.repos.get({repo: 'repo', owner: 'owner'});
@@ -72,7 +73,6 @@ test.serial('Use a https proxy', async t => {
     githubUrl: `https://localhost:${serverPort}`,
     githubApiPathPrefix: '',
     proxy: {host: 'localhost', port: proxyPort, rejectUnauthorized: false, headers: {foo: 'bar'}},
-    retry: {retries: 1, factor: 1, minTimeout: 1, maxTimeout: 1},
   });
 
   await github.repos.get({repo: 'repo', owner: 'owner'});
@@ -107,10 +107,10 @@ test('Use the global throttler for all endpoints', async t => {
   const issues = stub().callsFake(async () => Date.now());
   const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
   const rate = 150;
-  const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
-    limit: {search: 1, core: 1},
-    globalLimit: rate,
-  });
+  const github = proxyquire('../lib/get-client', {
+    '@octokit/rest': stub().returns(octokit),
+    './definitions/rate-limit': {RATE_LIMITS: {search: 1, core: 1}, GLOBAL_RATE_LIMIT: rate},
+  })();
 
   const a = await github.repos.createRelease();
   const b = await github.issues.createComment();
@@ -138,10 +138,10 @@ test('Use the same throttler for endpoints in the same rate limit group', async
   const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
   const searchRate = 300;
   const coreRate = 150;
-  const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
-    limit: {search: searchRate, core: coreRate},
-    globalLimit: 1,
-  });
+  const github = proxyquire('../lib/get-client', {
+    '@octokit/rest': stub().returns(octokit),
+    './definitions/rate-limit': {RATE_LIMITS: {search: searchRate, core: coreRate}, GLOBAL_RATE_LIMIT: 1},
+  })();
 
   const a = await github.repos.createRelease();
   const b = await github.issues.createComment();
@@ -173,11 +173,14 @@ test('Use the same throttler when retrying', async t => {
 
   const octokit = {repos: {createRelease}, authenticate: stub()};
   const coreRate = 200;
-  const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
-    limit: {core: coreRate},
-    retry: {retries: 3, factor: 1, minTimeout: 1},
-    globalLimit: 1,
-  });
+  const github = proxyquire('../lib/get-client', {
+    '@octokit/rest': stub().returns(octokit),
+    './definitions/rate-limit': {
+      RETRY_CONF: {retries: 3, factor: 1, minTimeout: 1},
+      RATE_LIMITS: {core: coreRate},
+      GLOBAL_RATE_LIMIT: 1,
+    },
+  })();
 
   await t.throws(github.repos.createRelease());
   t.is(createRelease.callCount, 4);
diff --git a/test/helpers/rate-limit.js b/test/helpers/rate-limit.js
@@ -0,0 +1,7 @@
+const RETRY_CONF = {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1};
+
+const RATE_LIMITS = {search: 1, core: {read: 1, write: 1}};
+
+const GLOBAL_RATE_LIMIT = 1;
+
+export default {RETRY_CONF, RATE_LIMITS, GLOBAL_RATE_LIMIT};
diff --git a/test/integration.test.js b/test/integration.test.js
@@ -3,14 +3,17 @@ import test from 'ava';
 import {stat} from 'fs-extra';
 import nock from 'nock';
 import {stub} from 'sinon';
+import proxyquire from 'proxyquire';
 import clearModule from 'clear-module';
 import SemanticReleaseError from '@semantic-release/error';
 import {authenticate, upload} from './helpers/mock-github';
+import rateLimit from './helpers/rate-limit';
 
 /* eslint camelcase: ["error", {properties: "never"}] */
 
 // Save the current process.env
 const envBackup = Object.assign({}, process.env);
+const client = proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit});
 
 test.beforeEach(t => {
   // Delete env variables in case they are on the machine running the tests
@@ -22,7 +25,12 @@ test.beforeEach(t => {
   delete process.env.GITHUB_PREFIX;
   // Clear npm cache to refresh the module state
   clearModule('..');
-  t.context.m = require('..');
+  t.context.m = proxyquire('..', {
+    './lib/verify': proxyquire('../lib/verify', {'./get-client': client}),
+    './lib/publish': proxyquire('../lib/publish', {'./get-client': client}),
+    './lib/success': proxyquire('../lib/success', {'./get-client': client}),
+    './lib/fail': proxyquire('../lib/fail', {'./get-client': client}),
+  });
   // Stub the logger
   t.context.log = stub();
   t.context.error = stub();
diff --git a/test/publish.test.js b/test/publish.test.js
@@ -5,17 +5,13 @@ import nock from 'nock';
 import {stub} from 'sinon';
 import proxyquire from 'proxyquire';
 import tempy from 'tempy';
-import getClient from '../lib/get-client';
 import {authenticate, upload} from './helpers/mock-github';
+import rateLimit from './helpers/rate-limit';
 
 /* eslint camelcase: ["error", {properties: "never"}] */
 
 const publish = proxyquire('../lib/publish', {
-  './get-client': conf =>
-    getClient({
-      ...conf,
-      ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
-    }),
+  './get-client': proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit}),
 });
 
 // Save the current process.env
diff --git a/test/success.test.js b/test/success.test.js
@@ -5,17 +5,13 @@ import nock from 'nock';
 import {stub} from 'sinon';
 import proxyquire from 'proxyquire';
 import ISSUE_ID from '../lib/definitions/sr-issue-id';
-import getClient from '../lib/get-client';
 import {authenticate} from './helpers/mock-github';
+import rateLimit from './helpers/rate-limit';
 
 /* eslint camelcase: ["error", {properties: "never"}] */
 
 const success = proxyquire('../lib/success', {
-  './get-client': conf =>
-    getClient({
-      ...conf,
-      ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
-    }),
+  './get-client': proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit}),
 });
 
 // Save the current process.env
diff --git a/test/verify.test.js b/test/verify.test.js
