Skip to content

Commit

Permalink
fix: use octokit.hook.wrap rather than Proxies
Browse files Browse the repository at this point in the history
  • Loading branch information
@pvdlg
pvdlg committed Nov 24, 2018
1 parent a753ef8 commit 681ff6a
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 124 deletions.
101 changes: 19 additions & 82 deletions lib/get-client.js
View file
Expand Up @@ -7,7 +7,6 @@ const urljoin = require('url-join');
const HttpProxyAgent = require('http-proxy-agent');
const HttpsProxyAgent = require('https-proxy-agent');

const GH_ROUTES = require('@octokit/rest/plugins/rest-api-endpoints/routes');
const {RETRY_CONF, RATE_LIMITS, GLOBAL_RATE_LIMIT} = require('./definitions/rate-limit');

/**
Expand All @@ -28,98 +27,36 @@ 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} limitKey The key to find the limit rate for the API endpoint and method.
* @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 = (limitKey, 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[limitKey][access] && access;
};

/**
* Get the limiter identifier associated with a client API call.
*
* @param {String} limitKey The key to find the limit rate for the API endpoint and method.
* @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 = (limitKey, endpoint, command) => {
return limitKey
? [limitKey, RATE_LIMITS[limitKey] && getAccess(limitKey, endpoint, command)].filter(Boolean).join('.')
: RATE_LIMITS[command]
? command
: 'core';
};
module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy}) => {
const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
const globalThrottler = new Bottleneck({minTime: GLOBAL_RATE_LIMIT});
const github = new Octokit({
baseUrl,
agent: proxy
? baseUrl && url.parse(baseUrl).protocol.replace(':', '') === 'http'
? new HttpProxyAgent(proxy)
: new HttpsProxyAgent(proxy)
: undefined,
});

/**
* 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 {Throttler} globalThrottler The throller function for the global rate limit.
* @param {String} limitKey The key to find the limit rate for the API endpoint and method.
* @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
*
* @return {Function} The `handler` for a `Proxy` wrapping an Octokit instance.
*/
const handler = (globalThrottler, limitKey, endpoint) => ({
/**
* 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(globalThrottler, getLimitKey(limitKey, endpoint, name), endpoint || name))
: Reflect.get(target, name, receiver),
github.hook.wrap('request', (request, options) => {
const access = options.method === 'GET' ? 'read' : 'write';
const rateCategory = options.url.startsWith('/search') ? 'search' : 'core';
const limitKey = [rateCategory, RATE_LIMITS[rateCategory][access] && access].filter(Boolean).join('.');

/**
* Create a throttled version of the called function then call it and retry it if the call fails with certain error code.
*
* @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(limitKey, globalThrottler);
return pRetry(async () => {
try {
return await throttler.wrap(func)(...args);
return await getThrottler(limitKey, globalThrottler).wrap(request)(options);
} catch (error) {
if (SKIP_RETRY_CODES.includes(error.status)) {
throw new pRetry.AbortError(error);
}
throw error;
}
}, RETRY_CONF);
},
});

module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy} = {}) => {
const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
const github = new Octokit({
baseUrl,
agent: proxy
? baseUrl && url.parse(baseUrl).protocol.replace(':', '') === 'http'
? new HttpProxyAgent(proxy)
: new HttpsProxyAgent(proxy)
: undefined,
});

github.authenticate({type: 'token', token: githubToken});
return new Proxy(github, handler(new Bottleneck({minTime: GLOBAL_RATE_LIMIT})));

return github;
};
67 changes: 25 additions & 42 deletions test/get-client.test.js
View file
Expand Up @@ -4,11 +4,12 @@ import https from 'https';
import {promisify} from 'util';
import {readFile} from 'fs-extra';
import test from 'ava';
import {isFunction, isPlainObject, inRange} from 'lodash';
import {inRange} from 'lodash';
import {stub, spy} from 'sinon';
import proxyquire from 'proxyquire';
import Proxy from 'proxy';
import serverDestroy from 'server-destroy';
import Octokit from '@octokit/rest';
import rateLimit from './helpers/rate-limit';

const getClient = proxyquire('../lib/get-client', {'./definitions/rate-limit': rateLimit});
Expand Down Expand Up @@ -85,32 +86,15 @@ test.serial('Use a https proxy', async t => {
await promisify(server.destroy).bind(server)();
});

test('Wrap Octokit in a proxy', t => {
const github = getClient({githubToken: 'github_token'});

t.true(Reflect.apply(Object.prototype.hasOwnProperty, github, ['repos']));
t.true(isPlainObject(github.repos));
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github.repos, ['createRelease']));
t.true(isFunction(github.repos.createRelease));

t.true(Reflect.apply(Object.prototype.hasOwnProperty, github, ['search']));
t.true(isPlainObject(github.search));
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github.search, ['issues']));
t.true(isFunction(github.search.issues));

t.falsy(github.unknown);
});

test('Use the global throttler for all endpoints', async t => {
const createRelease = stub().callsFake(() => Date.now());
const createComment = stub().callsFake(() => Date.now());
const issues = stub().callsFake(() => Date.now());
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
const rate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': stub().returns(octokit),
'./definitions/rate-limit': {RATE_LIMITS: {search: 1, core: 1}, GLOBAL_RATE_LIMIT: rate},
})();
})({githubToken: 'token'});

const a = await github.repos.createRelease();
const b = await github.issues.createComment();
Expand All @@ -132,16 +116,15 @@ test('Use the global throttler for all endpoints', async t => {
});

test('Use the same throttler for endpoints in the same rate limit group', async t => {
const createRelease = stub().callsFake(() => Date.now());
const createComment = stub().callsFake(() => Date.now());
const issues = stub().callsFake(() => Date.now());
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
const searchRate = 300;
const coreRate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': stub().returns(octokit),
'./definitions/rate-limit': {RATE_LIMITS: {search: searchRate, core: coreRate}, GLOBAL_RATE_LIMIT: 1},
})();
})({githubToken: 'token'});

const a = await github.repos.createRelease();
const b = await github.issues.createComment();
Expand All @@ -164,15 +147,15 @@ test('Use the same throttler for endpoints in the same rate limit group', async
});

test('Use different throttler for read and write endpoints', async t => {
const createRelease = stub().callsFake(() => Date.now());
const get = stub().callsFake(() => Date.now());
const octokit = {repos: {createRelease, get}, authenticate: stub()};
const writeRate = 300;
const readRate = 150;

const octokit = new Octokit();
octokit.hook.wrap('request', () => Date.now());
const github = proxyquire('../lib/get-client', {
'@octokit/rest': stub().returns(octokit),
'./definitions/rate-limit': {RATE_LIMITS: {core: {write: writeRate, read: readRate}}, GLOBAL_RATE_LIMIT: 1},
})();
})({githubToken: 'token'});

const a = await github.repos.get();
const b = await github.repos.get();
Expand All @@ -186,32 +169,32 @@ test('Use different throttler for read and write endpoints', async t => {
});

test('Use the same throttler when retrying', async t => {
const coreRate = 200;

// eslint-disable-next-line require-await
const createRelease = stub().callsFake(async () => {
const request = stub().callsFake(async () => {
const err = new Error();
err.time = Date.now();
err.code = 404;
err.status = 404;
throw err;
});

const octokit = {repos: {createRelease}, authenticate: stub()};
const coreRate = 200;
const octokit = new Octokit();
octokit.hook.wrap('request', request);
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,
},
})();
})({githubToken: 'token'});

await t.throws(github.repos.createRelease());
t.is(createRelease.callCount, 4);

const {time: a} = await t.throws(createRelease.getCall(0).returnValue);
const {time: b} = await t.throws(createRelease.getCall(1).returnValue);
const {time: c} = await t.throws(createRelease.getCall(2).returnValue);
const {time: d} = await t.throws(createRelease.getCall(3).returnValue);
const {time: a} = await t.throws(request.getCall(0).returnValue);
const {time: b} = await t.throws(request.getCall(1).returnValue);
const {time: c} = await t.throws(request.getCall(2).returnValue);
const {time: d} = await t.throws(request.getCall(3).returnValue);

// Each retry should be done after `coreRate` ms
t.true(inRange(b - a, coreRate - 50, coreRate + 50));
Expand Down

0 comments on commit 681ff6a

Please sign in to comment.