8000 fix: use `octokit.hook.wrap` rather than Proxies · coderbyheart/github@681ff6a · GitHub
[go: up one dir, main page]
Skip to content

Commit 681ff6a

Browse files
pvdlgpvdlg
committed
fix: use octokit.hook.wrap rather than Proxies
1 parent a753ef8 commit 681ff6a

File tree

2 files changed

+44
-124
lines changed

2 files changed

+44
-124
lines changed

‎lib/get-client.js

Lines changed: 19 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ const urljoin = require('url-join');
77
const HttpProxyAgent = require('http-proxy-agent');
88
const HttpsProxyAgent = require('https-proxy-agent');
99

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

1312
/**
@@ -28,98 +27,36 @@ const getThrottler = memoize((rate, globalThrottler) =>
2827
new Bottleneck({minTime: get(RATE_LIMITS, rate)}).chain(globalThrottler)
2928
);
3029

31-
/**
32-
* Determine if a call to a client function will trigger a read (`GET`) or a write (`POST`, `PATCH`, etc...) request.
33-
*
34-
* @param {String} limitKey The key to find the limit rate for the API endpoint and method.
35-
* @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
36-
* @param {String} command The client API command (for example the command for a call to `github.repos.get` is `get`).
37-
*
38-
* @return {String} `write` or `read` if there is rate limit configuration for this `endpoint` and `command`, `undefined` otherwise.
39-
*/
40-
const getAccess = (limitKey, endpoint, command) => {
41-
const method = GH_ROUTES[endpoint] && GH_ROUTES[endpoint][command] && GH_ROUTES[endpoint][command].method;
42-
const access = method && method === 'GET' ? 'read' : 'write';
43-
return RATE_LIMITS[limitKey][access] && access;
44-
};
45-
46-
/**
47-
* Get the limiter identifier associated with a client API call.
48-
*
49-
* @param {String} limitKey The key to find the limit rate for the API endpoint and method.
50-
* @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
51-
* @param {String} command The client API command (for example the command for a call to `github.repos.get` is `get`).
52-
*
53-
* @return {String} A string identifying the limiter to use for this `endpoint` and `command` (e.g. `search` or `core.write`).
54-
*/
55-
const getLimitKey = (limitKey, endpoint, command) => {
56-
return limitKey
57-
? [limitKey, RATE_LIMITS[limitKey] && getAccess(limitKey, endpoint, command)].filter(Boolean).join('.')
58-
: RATE_LIMITS[command]
59-
? command
60-
: 'core';
61-
};
30+
module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy}) => {
31+
const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
32+
const globalThrottler = new Bottleneck({minTime: GLOBAL_RATE_LIMIT});
33+
const github = new Octokit({
34+
baseUrl,
35+
agent: proxy
36+
? baseUrl && url.parse(baseUrl).protocol.replace(':', '') === 'http'
37+
? new HttpProxyAgent(proxy)
38+
: new HttpsProxyAgent(proxy)
39+
: undefined,
40+
});
6241

63-
/**
64-
* Create a`handler` for a `Proxy` wrapping an Octokit instance to:
65-
* - Recursively wrap the child objects of the Octokit instance in a `Proxy`
66-
* - Throttle and retry the Octokit instance functions
67-
*
68-
* @param {Throttler} globalThrottler The throller function for the global rate limit.
69-
* @param {String} limitKey The key to find the limit rate for the API endpoint and method.
70-
* @param {String} endpoint The client API enpoint (for example the endpoint for a call to `github.repos.get` is `repos`).
71-
*
72-
* @return {Function} The `handler` for a `Proxy` wrapping an Octokit instance.
73-
*/
74-
const handler = (globalThrottler, limitKey, endpoint) => ({
75-
/**
76-
* 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.
77-
*
78-
* @param {Object} target The target object.
79-
* @param {String} name The name of the property to get.
80-
* @param {Any} receiver The `Proxy` object.
81-
*
82-
* @return {Any} The property value or a `Proxy` of the property value.
83-
*/
84-
get: (target, name, receiver) =>
85-
Reflect.apply(Object.prototype.hasOwnProperty, target, [name])
86-
? new Proxy(target[name], handler(globalThrottler, getLimitKey(limitKey, endpoint, name), endpoint || name))
87-
: Reflect.get(target, F438 name, receiver),
42+
github.hook.wrap('request', (request, options) => {
43+
const access = options.method === 'GET' ? 'read' : 'write';
44+
const rateCategory = options.url.startsWith('/search') ? 'search' : 'core';
45+
const limitKey = [rateCategory, RATE_LIMITS[rateCategory][access] && access].filter(Boolean).join('.');
8846

89-
/**
90-
* Create a throttled version of the called function then call it and retry it if the call fails with certain error code.
91-
*
92-
* @param {Function} func The target function.
93-
* @param {Any} that The this argument for the call.
94-
* @param {Array} args The list of arguments for the call.
95-
*
96-
* @return {Promise<Any>} The result of the function called.
97-
*/
98-
apply: (func, that, args) => {
99-
const throttler = getThrottler(limitKey, globalThrottler);
10047
return pRetry(async () => {
10148
try {
102-
return await throttler.wrap(func)(...args);
49+
return await getThrottler(limitKey, globalThrottler).wrap(request)(options);
10350
} catch (error) {
10451
if (SKIP_RETRY_CODES.includes(error.status)) {
10552
throw new pRetry.AbortError(error);
10653
}
10754
throw error;
10855
}
10956
}, RETRY_CONF);
110-
},
111-
});
112-
113-
module.exports = ({githubToken, githubUrl, githubApiPathPrefix, proxy} = {}) => {
114-
const baseUrl = githubUrl && urljoin(githubUrl, githubApiPathPrefix);
115-
const github = new Octokit({
116-
baseUrl,
117-
agent: proxy
118-
? baseUrl && url.parse(baseUrl).protocol.replace(':', '') === 'http'
119-
? new HttpProxyAgent(proxy)
120-
: new HttpsProxyAgent(proxy)
121-
: undefined,
12257
});
58+
12359
github.authenticate({type: 'token', token: githubToken});
124-
return new Proxy(github, handler(new Bottleneck({minTime: GLOBAL_RATE_LIMIT})));
60+
61+
return github;
12562
};

‎test/get-client.test.js

Lines changed: 25 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ import https from 'https';
44
import {promisify} from 'util';
55
import {readFile} from 'fs-extra';
66
import test from 'ava';
7-
import {isFunction, isPlainObject, inRange} from 'lodash';
7+
import {inRange} from 'lodash';
88
import {stub, spy} from 'sinon';
99
import proxyquire from 'proxyquire';
1010
import Proxy from 'proxy';
1111
import serverDestroy from 'server-destroy';
12+
import Octokit from '@octokit/rest';
1213
import rateLimit from './helpers/rate-limit';
1314

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

88-
test('Wrap Octokit in a proxy', t => {
89-
const github = getClient({githubToken: 'github_token'});
90-
91-
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github, ['repos']));
92-
t.true(isPlainObject(github.repos));
93-
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github.repos, ['createRelease']));
94-
t.true(isFunction(github.repos.createRelease));
95-
96-
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github, ['search']));
97-
t.true(isPlainObject(github.search));
98-
t.true(Reflect.apply(Object.prototype.hasOwnProperty, github.search, ['issues']));
99-
t.true(isFunction(github.search.issues));
100-
101-
t.falsy(github.unknown);
102-
});
103-
10489
test('Use the global throttler for all endpoints', async t => {
105-
const createRelease = stub().callsFake(() => Date.now());
106-
const createComment = stub().callsFake(() => Date.now());
107-
const issues = stub().callsFake(() => Date.now());
108-
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
10990
const rate = 150;
91+
92+
const octokit = new Octokit();
93+
octokit.hook.wrap('request', () => Date.now());
11094
const github = proxyquire('../lib/get-client', {
11195
'@octokit/rest': stub().returns(octokit),
11296
'./definitions/rate-limit': {RATE_LIMITS: {search: 1, core: 1}, GLOBAL_RATE_LIMIT: rate},
113-
})();
97+
})({githubToken: 'token'});
11498

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

134118
test('Use the same throttler for endpoints in the same rate limit group', async t => {
135-
const createRelease = stub().callsFake(() => Date.now());
136-
const createComment = stub().callsFake(() => Date.now());
137-
const issues = stub().callsFake(() => Date.now());
138-
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
139119
const searchRate = 300;
140120
const coreRate = 150;
121+
122+
const octokit = new Octokit();
123+
octokit.hook.wrap('request', () => Date.now());
141124
const github = proxyquire('../lib/get-client', {
142125
'@octokit/rest': stub().returns(octokit),
143126
'./definitions/rate-limit': {RATE_LIMITS: {search: searchRate, core: coreRate}, GLOBAL_RATE_LIMIT: 1},
144-
})();
127+
})({githubToken: 'token'});
145128

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

166149
test('Use different throttler for read and write endpoints', async t => {
167-
const createRelease = stub().callsFake(() => Date.now());
168-
const get = stub().callsFake(() => Date.now());
169-
const octokit = {repos: {createRelease, get}, authenticate: stub()};
170150
const writeRate = 300;
171151
const readRate = 150;
152+
153+
const octokit = new Octokit();
154+
octokit.hook.wrap('request', () => Date.now());
172155
const github = proxyquire('../lib/get-client', {
173156
'@octokit/rest': stub().returns(octokit),
174157
'./definitions/rate-limit': {RATE_LIMITS: {core: {write: writeRate, read: readRate}}, GLOBAL_RATE_LIMIT: 1},
175-
})();
158+
})({githubToken: 'token'});
176159

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

188171
test('Use the same throttler when retrying', async t => {
172+
const coreRate = 200;
173+
189174
// eslint-disable-next-line require-await
190-
const createRelease = stub().callsFake(async () => {
175+
const request = stub().callsFake(async () => {
191176
const err = new Error();
192177
err.time = Date.now();
193-
err.code = 404;
178+
err.status = 404;
194179
throw err;
195180
});
196-
197-
const octokit = {repos: {createRelease}, authenticate: stub()};
198-
const coreRate = 200;
181+
const octokit = new Octokit();
182+
octokit.hook.wrap('request', request);
199183
const github = proxyquire('../lib/get-client', {
200184
'@octokit/rest': stub().returns(octokit),
201185
'./definitions/rate-limit': {
202186
RETRY_CONF: {retries: 3, factor: 1, minTimeout: 1},
203187
RATE_LIMITS: {core: coreRate},
204188
GLOBAL_RATE_LIMIT: 1,
205189
},
206-
})();
190+
})({githubToken: 'token'});
207191

208192
await t.throws(github.repos.createRelease());
209-
t.is(createRelease.callCount, 4);
210193

211-
const {time: a} = await t.throws(createRelease.getCall(0).returnValue);
212-
const {time: b} = await t.throws(createRelease.getCall(1).returnValue);
213-
const {time: c} = await t.throws(createRelease.getCall(2).returnValue);
214-
const {time: d} = await t.throws(createRelease.getCall(3).returnValue);
194+
const {time: a} = await t.throws(request.getCall(0).returnValue);
195+
const {time: b} = await t.throws(request.getCall(1).returnValue);
196+
const {time: c} = await t.throws(request.getCall(2).returnValue);
197+
const {time: d} = await t.throws(request.getCall(3).returnValue);
215198

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

0 commit comments

Comments
 (0)
0