From 007be133131a7bf59eb3747e70777f81e5388204 Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Sat, 23 Dec 2017 08:03:16 +0100
Subject: [PATCH] feat: add api caching to gitlab (#1324)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This PR ports across GitHub’s caching approach to the GitLab platform API.
---
lib/platform/gitlab/gl-got-wrapper.js | 29 +++++--
lib/platform/gitlab/index.js | 15 +++-
.../gitlab/__snapshots__/index.spec.js.snap | 83 ++++++++++++-------
test/platform/gitlab/gl-got-wrapper.spec.js | 9 ++
test/platform/gitlab/index.spec.js | 29 +++----
test/workers/repository/init/apis.spec.js | 3 +-
6 files changed, 112 insertions(+), 56 deletions(-)
diff --git a/lib/platform/gitlab/gl-got-wrapper.js b/lib/platform/gitlab/gl-got-wrapper.js
index af2f259dce..e517af745b 100644
--- a/lib/platform/gitlab/gl-got-wrapper.js
+++ b/lib/platform/gitlab/gl-got-wrapper.js
@@ -1,17 +1,32 @@
const glGot = require('gl-got');
const parseLinkHeader = require('parse-link-header');
+let cache = {};
+
async function get(path, opts, retries = 5) {
+ const method = opts && opts.method ? opts.method : 'get';
+ if (method === 'get' && cache[path]) {
+ logger.debug({ path }, 'Returning cached result');
+ return cache[path];
+ }
+ logger.debug({ path }, method.toUpperCase());
const res = await glGot(path, opts);
if (opts && opts.paginate) {
// Check if result is paginated
- const linkHeader = parseLinkHeader(res.headers.link);
- if (linkHeader && linkHeader.next) {
- res.body = res.body.concat(
- (await get(linkHeader.next.url, opts, retries)).body
- );
+ try {
+ const linkHeader = parseLinkHeader(res.headers.link);
+ if (linkHeader && linkHeader.next) {
+ res.body = res.body.concat(
+ (await get(linkHeader.next.url, opts, retries)).body
+ );
+ }
+ } catch (err) {
+ logger.warn({ err }, 'Pagination error');
}
}
+ if (method === 'get' && path.startsWith('projects/')) {
+ cache[path] = res;
+ }
return res;
}
@@ -22,4 +37,8 @@ for (const x of helpers) {
get(url, Object.assign({}, opts, { method: x.toUpperCase() }));
}
+get.reset = function reset() {
+ cache = {};
+};
+
module.exports = get;
diff --git a/lib/platform/gitlab/index.js b/lib/platform/gitlab/index.js
index 732db8abea..27bc32595b 100644
--- a/lib/platform/gitlab/index.js
+++ b/lib/platform/gitlab/index.js
@@ -2,7 +2,7 @@ const get = require('./gl-got-wrapper');
const { createFile, updateFile } = require('./helpers');
-const config = {};
+let config = {};
module.exports = {
getRepos,
@@ -84,9 +84,9 @@ async function initRepo(repoName, token, endpoint) {
if (endpoint) {
process.env.GITLAB_ENDPOINT = endpoint;
}
+ config = {};
+ get.reset();
config.repoName = urlEscape(repoName);
- config.fileList = null;
- config.prList = null;
try {
const res = await get(`projects/${config.repoName}`);
config.defaultBranch = res.body.default_branch;
@@ -94,6 +94,9 @@ async function initRepo(repoName, token, endpoint) {
logger.debug(`${repoName} default branch = ${config.baseBranch}`);
// Discover our user email
config.email = (await get(`user`)).body.email;
+ delete config.prList;
+ delete config.fileList;
+ await Promise.all([getPrList(), getFileList()]);
} catch (err) {
logger.error({ err }, `GitLab init error`);
throw err;
@@ -455,6 +458,12 @@ async function mergePr(iid) {
// Generic File operations
async function getFile(filePath, branchName) {
+ logger.debug(`getFile(filePath=${filePath}, branchName=${branchName})`);
+ if (!branchName || branchName === config.baseBranch) {
+ if (config.fileList && !config.fileList.includes(filePath)) {
+ return null;
+ }
+ }
try {
const url = `projects/${config.repoName}/repository/files/${urlEscape(
filePath
diff --git a/test/platform/gitlab/__snapshots__/index.spec.js.snap b/test/platform/gitlab/__snapshots__/index.spec.js.snap
index 6b2300c701..db56da34ea 100644
--- a/test/platform/gitlab/__snapshots__/index.spec.js.snap
+++ b/test/platform/gitlab/__snapshots__/index.spec.js.snap
@@ -3,7 +3,7 @@
exports[`platform/gitlab addAssignees(issueNo, assignees) should add the given assignees to the issue 1`] = `
Array [
Array [
- "projects/some%2Frepo/merge_requests/42?assignee_id=123",
+ "projects/undefined/merge_requests/42?assignee_id=123",
],
]
`;
@@ -11,7 +11,7 @@ Array [
exports[`platform/gitlab addAssignees(issueNo, assignees) should warn if more than one assignee 1`] = `
Array [
Array [
- "projects/some%2Frepo/merge_requests/42?assignee_id=123",
+ "projects/undefined/merge_requests/42?assignee_id=123",
],
]
`;
@@ -75,13 +75,7 @@ exports[`platform/gitlab getBranchLastCommitTime should return a Date 1`] = `201
exports[`platform/gitlab getBranchPr(branchName) should return null if no PR exists 1`] = `
Array [
Array [
- "projects/some%2Frepo",
- ],
- Array [
- "user",
- ],
- Array [
- "projects/some%2Frepo/merge_requests?state=opened&per_page=100",
+ "projects/undefined/merge_requests?state=opened&per_page=100",
Object {
"paginate": true,
},
@@ -92,22 +86,16 @@ Array [
exports[`platform/gitlab getBranchPr(branchName) should return the PR object 1`] = `
Array [
Array [
- "projects/some%2Frepo",
- ],
- Array [
- "user",
- ],
- Array [
- "projects/some%2Frepo/merge_requests?state=opened&per_page=100",
+ "projects/undefined/merge_requests?state=opened&per_page=100",
Object {
"paginate": true,
},
],
Array [
- "projects/some%2Frepo/merge_requests/undefined",
+ "projects/undefined/merge_requests/undefined",
],
Array [
- "projects/some%2Frepo/repository/branches/some-branch",
+ "projects/undefined/repository/branches/some-branch",
],
]
`;
@@ -212,11 +200,21 @@ Array [
Array [
"user",
],
+ Array [
+ "projects/some%2Frepo%2Fproject/merge_requests?per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
+ Array [
+ "projects/some%2Frepo%2Fproject/repository/tree?ref=undefined&recursive=true&per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
]
`;
-exports[`platform/gitlab initRepo should escape all forward slashes in project names 2`] = `Object {}`;
-
exports[`platform/gitlab initRepo should initialise the config for the repo - 0 1`] = `
Array [
Array [
@@ -225,6 +223,18 @@ Array [
Array [
"user",
],
+ Array [
+ "projects/some%2Frepo/merge_requests?per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
+ Array [
+ "projects/some%2Frepo/repository/tree?ref=undefined&recursive=true&per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
]
`;
@@ -238,6 +248,18 @@ Array [
Array [
"user",
],
+ Array [
+ "projects/some%2Frepo/merge_requests?per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
+ Array [
+ "projects/some%2Frepo/repository/tree?ref=undefined&recursive=true&per_page=100",
+ Object {
+ "paginate": true,
+ },
+ ],
]
`;
@@ -251,18 +273,21 @@ Array [
Array [
"user",
],
-]
-`;
-
-exports[`platform/gitlab initRepo should initialise the config for the repo - 2 2`] = `Object {}`;
-
-exports[`platform/gitlab setBaseBranch(branchName) sets the base branch 1`] = `
-Array [
Array [
- "projects/some%2Frepo",
+ "projects/some%2Frepo/merge_requests?per_page=100",
+ Object {
+ "paginate": true,
+ },
],
Array [
- "user",
+ "projects/some%2Frepo/repository/tree?ref=undefined&recursive=true&per_page=100",
+ Object {
+ "paginate": true,
+ },
],
]
`;
+
+exports[`platform/gitlab initRepo should initialise the config for the repo - 2 2`] = `Object {}`;
+
+exports[`platform/gitlab setBaseBranch(branchName) sets the base branch 1`] = `Array []`;
diff --git a/test/platform/gitlab/gl-got-wrapper.spec.js b/test/platform/gitlab/gl-got-wrapper.spec.js
index e1d3382c85..962a9b7e7b 100644
--- a/test/platform/gitlab/gl-got-wrapper.spec.js
+++ b/test/platform/gitlab/gl-got-wrapper.spec.js
@@ -52,4 +52,13 @@ describe('platform/gl-got-wrapper', () => {
const res = await get.post('some-url');
expect(res.body).toEqual(body);
});
+ it('returns cached', async () => {
+ get.reset();
+ glGot.mockReturnValueOnce({
+ body: {},
+ });
+ const res1 = await get('projects/foo');
+ const res2 = await get('projects/foo');
+ expect(res1).toEqual(res2);
+ });
});
diff --git a/test/platform/gitlab/index.spec.js b/test/platform/gitlab/index.spec.js
index ea823b0967..5ed2efc730 100644
--- a/test/platform/gitlab/index.spec.js
+++ b/test/platform/gitlab/index.spec.js
@@ -93,6 +93,7 @@ describe('platform/gitlab', () => {
if (envToken !== undefined) {
process.env.GITLAB_TOKEN = envToken;
}
+ get.mockReturnValue({ body: [] });
const config = await initRepo('some/repo', ...args);
expect(get.mock.calls).toMatchSnapshot();
expect(config).toMatchSnapshot();
@@ -101,9 +102,9 @@ describe('platform/gitlab', () => {
});
});
it(`should escape all forward slashes in project names`, async () => {
- const config = await initRepo('some/repo/project', 'some-token');
+ get.mockReturnValue({ body: [] });
+ await initRepo('some/repo/project', 'some-token');
expect(get.mock.calls).toMatchSnapshot();
- expect(config).toMatchSnapshot();
});
it('should throw an error if no token is provided', async () => {
let err;
@@ -136,7 +137,6 @@ describe('platform/gitlab', () => {
});
describe('setBaseBranch(branchName)', () => {
it('sets the base branch', async () => {
- await initRepo('some/repo', 'token');
// getBranchCommit
get.mockImplementationOnce(() => ({
body: {
@@ -151,7 +151,6 @@ describe('platform/gitlab', () => {
});
describe('getFileList', () => {
it('returns empty array if error', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce(() => {
throw new Error('some error');
});
@@ -159,7 +158,6 @@ describe('platform/gitlab', () => {
expect(files).toEqual([]);
});
it('warns if truncated result', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce(() => ({
body: [],
}));
@@ -167,7 +165,6 @@ describe('platform/gitlab', () => {
expect(files.length).toBe(0);
});
it('caches the result', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce(() => ({
body: [],
}));
@@ -177,7 +174,6 @@ describe('platform/gitlab', () => {
expect(files.length).toBe(0);
});
it('should return the files matching the fileName', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce(() => ({
body: [
{ type: 'blob', path: 'symlinks/package.json', mode: '120000' },
@@ -241,7 +237,6 @@ describe('platform/gitlab', () => {
});
describe('getBranchPr(branchName)', () => {
it('should return null if no PR exists', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce(() => ({
body: [],
}));
@@ -250,7 +245,6 @@ describe('platform/gitlab', () => {
expect(pr).toBe(null);
});
it('should return the PR object', async () => {
- await initRepo('some/repo', 'token');
get.mockReturnValueOnce({
body: [{ number: 91, source_branch: 'somebranch' }],
});
@@ -283,12 +277,10 @@ describe('platform/gitlab', () => {
});
});
it('returns success if requiredStatusChecks null', async () => {
- await initRepo('some/repo', 'token');
const res = await gitlab.getBranchStatus('somebranch', null);
expect(res).toEqual('success');
});
it('return failed if unsupported requiredStatusChecks', async () => {
- await initRepo('some/repo', 'token');
const res = await gitlab.getBranchStatus('somebranch', ['foo']);
expect(res).toEqual('failed');
});
@@ -368,7 +360,6 @@ describe('platform/gitlab', () => {
});
describe('setBranchStatus', () => {
it('sets branch status', async () => {
- await initRepo('some/repo', 'token');
// getBranchCommit
get.mockReturnValueOnce({
body: {
@@ -401,7 +392,6 @@ describe('platform/gitlab', () => {
});
describe('getBranchLastCommitTime', () => {
it('should return a Date', async () => {
- await initRepo('some/repo', 'token');
get.mockReturnValueOnce({
body: [
{
@@ -436,7 +426,6 @@ describe('platform/gitlab', () => {
expect(res).toMatchSnapshot();
});
it('handles error', async () => {
- await initRepo('some/repo', 'token');
get.mockReturnValueOnce({
body: [],
});
@@ -446,7 +435,6 @@ describe('platform/gitlab', () => {
});
describe('addAssignees(issueNo, assignees)', () => {
it('should add the given assignees to the issue', async () => {
- await initRepo('some/repo', 'token');
get.mockReturnValueOnce({
body: [{ id: 123 }],
});
@@ -454,7 +442,6 @@ describe('platform/gitlab', () => {
expect(get.put.mock.calls).toMatchSnapshot();
});
it('should warn if more than one assignee', async () => {
- await initRepo('some/repo', 'token');
get.mockReturnValueOnce({
body: [{ id: 123 }],
});
@@ -462,7 +449,6 @@ describe('platform/gitlab', () => {
expect(get.put.mock.calls).toMatchSnapshot();
});
it('should swallow error', async () => {
- await initRepo('some/repo', 'token');
get.mockImplementationOnce({});
await gitlab.addAssignees(42, ['someuser', 'someotheruser']);
expect(get.put.mock.calls).toHaveLength(0);
@@ -470,7 +456,6 @@ describe('platform/gitlab', () => {
});
describe('addReviewers(issueNo, reviewers)', () => {
it('should add the given reviewers to the PR', async () => {
- await initRepo('some/repo', 'token');
await gitlab.addReviewers(42, ['someuser', 'someotheruser']);
});
});
@@ -611,6 +596,10 @@ describe('platform/gitlab', () => {
});
});
describe('getFile(filePath, branchName)', () => {
+ beforeEach(async () => {
+ get.mockReturnValueOnce({ body: [{ type: 'blob', path: 'some-path' }] });
+ await gitlab.getFileList();
+ });
it('gets the file', async () => {
get.mockReturnValueOnce({
body: {
@@ -620,6 +609,10 @@ describe('platform/gitlab', () => {
const res = await gitlab.getFile('some-path');
expect(res).toMatchSnapshot();
});
+ it('short cuts 404', async () => {
+ const res = await gitlab.getFile('some-missing-path');
+ expect(res).toBe(null);
+ });
it('returns null for 404', async () => {
get.mockImplementationOnce(() => Promise.reject({ statusCode: 404 }));
const res = await gitlab.getFile('some-path', 'some-branch');
diff --git a/test/workers/repository/init/apis.spec.js b/test/workers/repository/init/apis.spec.js
index a67342d6fb..98bfbef2ee 100644
--- a/test/workers/repository/init/apis.spec.js
+++ b/test/workers/repository/init/apis.spec.js
@@ -20,7 +20,8 @@ describe('workers/repository/init/apis', () => {
config.repository = 'some/name';
glGot.mockReturnValueOnce({ body: {} });
glGot.mockReturnValueOnce({ body: {} });
- glGot.mockReturnValueOnce({ body: {} });
+ glGot.mockReturnValueOnce({ body: [] });
+ glGot.mockReturnValueOnce({ body: [] });
await initApis(config, 'some-token');
});
it('runs vsts', async () => {
--
GitLab