From dc44ba772551d932d04abe436a83d34010fc5637 Mon Sep 17 00:00:00 2001 From: Paul Melnikow Date: Tue, 17 Oct 2017 22:01:46 -0400 Subject: [PATCH] Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 (#1142) - Add tests to request-handler to prepare for some tweaks to caching for #820 - Clean up code in request-handler: renames, DRY, arrows, imports - Allow for clean shutdown of `setInterval` code. This requires the ability to cancel autosaving. - Upgrade to Mocha 4, and clean up so the process exits on its own (see mochajs/mocha#3044) - Better encapsulate analytics --- lib/analytics.js | 56 ++++++++++++++------ lib/github-auth.js | 49 ++++++++++++----- lib/in-process-server-test-helpers.js | 2 +- lib/request-handler.js | 51 ++++++++---------- lib/request-handler.spec.js | 75 +++++++++++++++++++++++++++ package.json | 2 +- server.js | 41 +++++++-------- 7 files changed, 192 insertions(+), 84 deletions(-) create mode 100644 lib/request-handler.spec.js diff --git a/lib/analytics.js b/lib/analytics.js index 06f9fb13cdcaf..ecff6ba7b7bd2 100644 --- a/lib/analytics.js +++ b/lib/analytics.js @@ -14,16 +14,32 @@ if (process.env.REDISTOGO_URL) { } let analytics = {}; +let autosaveIntervalId; -const analyticsAutoSaveFileName = process.env.SHIELDS_ANALYTICS_FILE || './analytics.json'; -const analyticsAutoSavePeriod = 10000; -setInterval(function analyticsAutoSave() { +const analyticsPath = process.env.SHIELDS_ANALYTICS_FILE || './analytics.json'; + +function performAutosave() { + const contents = JSON.stringify(analytics); if (useRedis) { - redis.set(analyticsAutoSaveFileName, JSON.stringify(analytics)); + redis.set(analyticsPath, contents); } else { - fs.writeFileSync(analyticsAutoSaveFileName, JSON.stringify(analytics)); + fs.writeFileSync(analyticsPath, contents); } -}, analyticsAutoSavePeriod); +} + +function scheduleAutosaving() { + const analyticsAutoSavePeriod = 10000; + autosaveIntervalId = setInterval(performAutosave, analyticsAutoSavePeriod); +} + +// For a clean shutdown. +function cancelAutosaving() { + if (autosaveIntervalId) { + clearInterval(autosaveIntervalId); + autosaveIntervalId = null; + } + performAutosave(); +} function defaultAnalytics() { const analytics = Object.create(null); @@ -43,11 +59,10 @@ function defaultAnalytics() { return analytics; } -// Auto-load analytics. -function analyticsAutoLoad() { +function load() { const defaultAnalyticsObject = defaultAnalytics(); if (useRedis) { - redis.get(analyticsAutoSaveFileName, function(err, value) { + redis.get(analyticsPath, function(err, value) { if (err == null && value != null) { // if/try/return trick: // if error, then the rest of the function is run. @@ -70,7 +85,7 @@ function analyticsAutoLoad() { } else { // Not using Redis. try { - analytics = JSON.parse(fs.readFileSync(analyticsAutoSaveFileName)); + analytics = JSON.parse(fs.readFileSync(analyticsPath)); // Extend analytics with a new value. for (const key in defaultAnalyticsObject) { if (!(key in analytics)) { @@ -106,12 +121,23 @@ function incrMonthlyAnalytics(monthlyAnalytics) { } catch(e) { console.error(e.stack); } } -function getAnalytics() { - return analytics; +function noteRequest(queryParams, match) { + incrMonthlyAnalytics(analytics.vendorMonthly); + if (queryParams.style === 'flat') { + incrMonthlyAnalytics(analytics.vendorFlatMonthly); + } else if (queryParams.style === 'flat-square') { + incrMonthlyAnalytics(analytics.vendorFlatSquareMonthly); + } +} + +function setRoutes (server) { + server.ajax.on('analytics/v1', (json, end) => { end(analytics); }); } module.exports = { - analyticsAutoLoad, - incrMonthlyAnalytics, - getAnalytics + load, + scheduleAutosaving, + cancelAutosaving, + noteRequest, + setRoutes }; diff --git a/lib/github-auth.js b/lib/github-auth.js index 8704cb3971142..e0a908eb68300 100644 --- a/lib/github-auth.js +++ b/lib/github-auth.js @@ -11,20 +11,37 @@ try { // is stored in this JSON data. serverSecrets = require('../private/secret.json'); } catch(e) {} + +// This is an initial value which makes the code work while the initial data +// is loaded. In the then() callback of scheduleAutosaving(), it's reassigned +// with a JsonSave object. let githubUserTokens = {data: []}; -const githubUserTokensFile = './private/github-user-tokens.json'; -autosave(githubUserTokensFile, {data: []}).then(function(f) { - githubUserTokens = f; - for (let i = 0; i < githubUserTokens.data.length; i++) { - addGithubToken(githubUserTokens.data[i]); - } - // Personal tokens allow access to GitHub private repositories. - // You can manage your personal GitHub token at - // . - if (serverSecrets && serverSecrets.gh_token) { - addGithubToken(serverSecrets.gh_token); + +function scheduleAutosaving() { + const githubUserTokensFile = './private/github-user-tokens.json'; + autosave(githubUserTokensFile, {data: []}).then(save => { + githubUserTokens = save; + for (let i = 0; i < githubUserTokens.data.length; i++) { + addGithubToken(githubUserTokens.data[i]); + } + // Personal tokens allow access to GitHub private repositories. + // You can manage your personal GitHub token at + // . + if (serverSecrets && serverSecrets.gh_token) { + addGithubToken(serverSecrets.gh_token); + } + }).catch(e => { + console.error('Could not create ' + githubUserTokensFile); + }); +} + +function cancelAutosaving() { + if (githubUserTokens.stop) { + githubUserTokens.stop(); + githubUserTokens.save(); + githubUserTokens = {data: []}; } -}).catch(function(e) { console.error('Could not create ' + githubUserTokensFile); }); +} function setRoutes(server) { server.route(/^\/github-auth$/, function(data, match, end, ask) { @@ -261,5 +278,9 @@ function constEq(a, b) { return (zero === 0); } -exports.setRoutes = setRoutes; -exports.request = githubRequest; +module.exports = { + scheduleAutosaving, + cancelAutosaving, + request: githubRequest, + setRoutes, +}; diff --git a/lib/in-process-server-test-helpers.js b/lib/in-process-server-test-helpers.js index 6199f464bad0d..1f5c166f6a6a0 100644 --- a/lib/in-process-server-test-helpers.js +++ b/lib/in-process-server-test-helpers.js @@ -55,7 +55,7 @@ function reset (server) { */ function stop (server) { if (server) { - server.camp.close(); + server.stop(); } } diff --git a/lib/request-handler.js b/lib/request-handler.js index b53eb400d028a..a735acf944921 100644 --- a/lib/request-handler.js +++ b/lib/request-handler.js @@ -4,18 +4,11 @@ const domain = require('domain'); const request = require('request'); const badge = require('./badge'); -const { - makeBadgeData: getBadgeData -} = require('./badge-data'); +const { makeBadgeData: getBadgeData } = require('./badge-data'); const log = require('./log'); const LruCache = require('./lru-cache'); -const { - incrMonthlyAnalytics, - getAnalytics -} = require('./analytics'); -const { - makeSend -} = require('./result-sender'); +const analytics = require('./analytics'); +const { makeSend } = require('./result-sender'); // We avoid calling the vendor's server for computation of the information in a // number of badges. @@ -35,33 +28,30 @@ const requestCache = new LruCache(1000); // Deep error handling for vendor hooks. const vendorDomain = domain.create(); -vendorDomain.on('error', function(err) { +vendorDomain.on('error', err => { log.error('Vendor hook error:', err.stack); }); function handleRequest (vendorRequestHandler) { - return function getRequest(data, match, end, ask) { - if (data.maxAge !== undefined && /^[0-9]+$/.test(data.maxAge)) { - ask.res.setHeader('Cache-Control', 'max-age=' + data.maxAge); + return (queryParams, match, end, ask) => { + if (queryParams.maxAge !== undefined && /^[0-9]+$/.test(queryParams.maxAge)) { + ask.res.setHeader('Cache-Control', 'max-age=' + queryParams.maxAge); } else { // Cache management - no cache, so it won't be cached by GitHub's CDN. ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate'); } + const reqTime = new Date(); - const date = (reqTime).toGMTString(); + const date = reqTime.toGMTString(); ask.res.setHeader('Expires', date); // Proxies, GitHub, see #221. ask.res.setHeader('Date', date); - incrMonthlyAnalytics(getAnalytics().vendorMonthly); - if (data.style === 'flat') { - incrMonthlyAnalytics(getAnalytics().vendorFlatMonthly); - } else if (data.style === 'flat-square') { - incrMonthlyAnalytics(getAnalytics().vendorFlatSquareMonthly); - } - const cacheIndex = match[0] + '?label=' + data.label + '&style=' + data.style - + '&logo=' + data.logo + '&logoWidth=' + data.logoWidth - + '&link=' + JSON.stringify(data.link) + '&colorA=' + data.colorA - + '&colorB=' + data.colorB; + analytics.noteRequest(queryParams, match); + + const cacheIndex = match[0] + '?label=' + queryParams.label + '&style=' + queryParams.style + + '&logo=' + queryParams.logo + '&logoWidth=' + queryParams.logoWidth + + '&link=' + JSON.stringify(queryParams.link) + '&colorA=' + queryParams.colorA + + '&colorB=' + queryParams.colorB; // Should we return the data right away? const cached = requestCache.get(cacheIndex); let cachedVersionSent = false; @@ -78,7 +68,7 @@ function handleRequest (vendorRequestHandler) { // In case our vendor servers are unresponsive. let serverUnresponsive = false; - const serverResponsive = setTimeout(function() { + const serverResponsive = setTimeout(() => { serverUnresponsive = true; if (cachedVersionSent) { return; } if (requestCache.has(cacheIndex)) { @@ -87,7 +77,7 @@ function handleRequest (vendorRequestHandler) { return; } ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate'); - const badgeData = getBadgeData('vendor', data); + const badgeData = getBadgeData('vendor', queryParams); badgeData.text[1] = 'unresponsive'; let extension; try { @@ -109,7 +99,8 @@ function handleRequest (vendorRequestHandler) { } options.headers = options.headers || {}; options.headers['User-Agent'] = options.headers['User-Agent'] || 'Shields.io'; - return request(options, function(err, res, body) { + + request(options, (err, res, body) => { if (res != null && res.headers != null) { const cacheControl = res.headers['cache-control']; if (cacheControl != null) { @@ -125,8 +116,8 @@ function handleRequest (vendorRequestHandler) { }); } - vendorDomain.run(function() { - vendorRequestHandler(data, match, function sendBadge(format, badgeData) { + vendorDomain.run(() => { + vendorRequestHandler(queryParams, match, function sendBadge(format, badgeData) { if (serverUnresponsive) { return; } clearTimeout(serverResponsive); // Check for a change in the data. diff --git a/lib/request-handler.spec.js b/lib/request-handler.spec.js new file mode 100644 index 0000000000000..5fd36437d88e9 --- /dev/null +++ b/lib/request-handler.spec.js @@ -0,0 +1,75 @@ +'use strict'; + +const assert = require('assert'); +const fetch = require('node-fetch'); +const config = require('./test-config'); +const Camp = require('camp'); +const analytics = require('./analytics'); +const { makeBadgeData: getBadgeData } = require('./badge-data'); +const { + handleRequest, + clearRequestCache +} = require('./request-handler'); + +const baseUri = `http://127.0.0.1:${config.port}`; + +function performTwoRequests (first, second) { + return fetch(`${baseUri}${first}`) + .then(res => { + assert.ok(res.ok); + return fetch(`${baseUri}${second}`) + .then(res => { + assert.ok(res.ok); + }) + }); +} + +describe('The request handler', function() { + before(analytics.load); + + let camp; + beforeEach(function (done) { + camp = Camp.start({ port: config.port, hostname: '::' }); + camp.on('listening', () => done()); + }); + afterEach(function (done) { + clearRequestCache(); + if (camp) { + camp.close(() => done()); + camp = null; + } + }); + + let handlerCallCount; + beforeEach(function () { + handlerCallCount = 0; + camp.route(/^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, + handleRequest((queryParams, match, sendBadge, request) => { + ++handlerCallCount; + const [, someValue, format] = match; + const badgeData = getBadgeData('testing', queryParams); + badgeData.text[1] = someValue; + sendBadge(format, badgeData); + })); + }); + + it('should cache identical requests', function () { + return performTwoRequests('/testing/123.svg', '/testing/123.svg').then(() => { + assert.equal(handlerCallCount, 1); + }); + }); + + it('should differentiate known query parameters', function () { + return performTwoRequests( + '/testing/123.svg?label=foo', + '/testing/123.svg?label=bar' + ).then(() => { assert.equal(handlerCallCount, 2); }); + }); + + it('should ignore unknown query parameters', function () { + return performTwoRequests( + '/testing/123.svg?foo=1', + '/testing/123.svg?foo=2' + ).then(() => { assert.equal(handlerCallCount, 1); }); + }); +}); diff --git a/package.json b/package.json index 187294257a37d..9b4c9d958d8fc 100644 --- a/package.json +++ b/package.json @@ -85,7 +85,7 @@ "is-svg": "^2.1.0", "lodash.difference": "^4.5.0", "minimist": "^1.2.0", - "mocha": "^3.2.0", + "mocha": "^4.0.1", "nock": "^9.0.13", "node-fetch": "^1.6.3", "nyc": "^11.2.1", diff --git a/server.js b/server.js index 9be0878051f2e..02036830616df 100644 --- a/server.js +++ b/server.js @@ -28,9 +28,6 @@ var querystring = require('querystring'); var prettyBytes = require('pretty-bytes'); var xml2js = require('xml2js'); var serverSecrets = require('./lib/server-secrets'); -if (serverSecrets && serverSecrets.gh_client_id) { - githubAuth.setRoutes(camp); -} log(tryUrl); const {latest: latestVersion} = require('./lib/version'); @@ -59,11 +56,7 @@ const { version: versionColor, age: ageColor } = require('./lib/color-formatters'); -const { - analyticsAutoLoad, - incrMonthlyAnalytics, - getAnalytics -} = require('./lib/analytics'); +const analytics = require('./lib/analytics'); const { makeColorB, isValidStyle, @@ -122,8 +115,14 @@ const { var semver = require('semver'); var serverStartTime = new Date((new Date()).toGMTString()); -analyticsAutoLoad(); -camp.ajax.on('analytics/v1', function(json, end) { end(getAnalytics()); }); +analytics.load(); +analytics.scheduleAutosaving(); +analytics.setRoutes(camp); + +githubAuth.scheduleAutosaving(); +if (serverSecrets && serverSecrets.gh_client_id) { + githubAuth.setRoutes(camp); +} var suggest = require('./lib/suggest.js'); camp.ajax.on('suggest/v1', suggest); @@ -133,9 +132,16 @@ function reset() { clearRegularUpdateCache(); } +function stop(callback) { + githubAuth.cancelAutosaving(); + analytics.cancelAutosaving(); + camp.close(callback); +} + module.exports = { camp, - reset + reset, + stop }; camp.notfound(/\.(svg|png|gif|jpg|json)/, function(query, match, end, request) { @@ -2434,9 +2440,6 @@ cache(function(data, match, sendBadge, request) { badgeData.text[1] = 'malformed'; sendBadge(format, badgeData); } - }).on('error', function(e) { - badgeData.text[1] = 'inaccessible'; - sendBadge(format, badgeData); }); })); @@ -2479,9 +2482,6 @@ cache(function(data, match, sendBadge, request) { badgeData.text[1] = 'malformed'; sendBadge(format, badgeData); } - }).on('error', function(e) { - badgeData.text[1] = 'inaccessible'; - sendBadge(format, badgeData); }); })); @@ -7129,12 +7129,7 @@ function(data, match, end, ask) { var color = escapeFormat(match[6]); var format = match[8]; - incrMonthlyAnalytics(getAnalytics().rawMonthly); - if (data.style === 'flat') { - incrMonthlyAnalytics(getAnalytics().rawFlatMonthly); - } else if (data.style === 'flat-square') { - incrMonthlyAnalytics(getAnalytics().rawFlatSquareMonthly); - } + analytics.noteRequest(data, match); // Cache management - the badge is constant. var cacheDuration = (3600*24*1)|0; // 1 day.