From 54a36e94741fdcce883755d00686ac5b05fea91d Mon Sep 17 00:00:00 2001 From: Paul Melnikow <github@paulmelnikow.com> Date: Sat, 1 Dec 2018 13:57:34 -0500 Subject: [PATCH] Refactor cache-header handling and config, create NonMemoryCachingBaseService, rewrite [flip] (#2360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's a lot going on in this PR, though it's all interdependent, so the only way I can see to break it up into smaller pieces would be serially. 1. I completely refactored the functions for managing cache headers. These have been added to `services/cache-headers.js`, and in some ways set the stage for the rest of this PR. - There are ample higher-level test of the functionality via `request-handler`. Refactoring these tests was deferred. Cache headers were previously dealt with in three places: - `request-handler.js`, for the dynamic badges. This function now calls `setCacheHeaders`. - `base-static.js`, for the static badges. This method now calls the wordy `serverHasBeenUpSinceResourceCached` and `setCacheHeadersForStaticResource`. - The bitFlip badge in `server.js`. 👈 This is what set all this in motion. This badge has been refactored to a new-style service based on a new `NoncachingBaseService` which does not use the Shields in-memory cache that the dynamic badges user. - I'm open to clearer names for `NoncachingBaseService`, which is kind of terrible. Absent alternatives, I wrote a short essay of clarification in the docstring. 😝 2. In the process of writing `NoncachingBaseService`, I discovered it takes several lines of code to instantiate and invoke a service. These would be duplicated in three or four places in production code, and in lots and lots of tests. I kept the line that goes from regex to namedParams (for reasons) and moved the rest into a static method called `invoke()`, which instantiates and invokes the service. This _replaced_ the instance method `invokeHandler`. - I gently reworked the unit tests to use `invoke` instead of `invokeHandler`– generally for the better. - I made a small change to `BaseStatic`. Now it invokes `handle()` async as the dynamic badges do. This way it could use `BaseService.invoke()`. 3. There was logic in `request-handler` for processing environment variables, validating them, and setting defaults. This could have been lifted whole-hog to `services/cache-headers.js`, though I didn't do that. Instead I moved it to `server-config.js`. Ideally `server-config` is the only module that should access `process.env`. This puts the defaults and config validation in one place, decouples the config schema from the entire rest of the application, and significantly simplifies our ability to test different configs, particularly on small units of code. (We were doing this well enough before in `request-handler.spec`, though it required mutating the environment, which was kludgy.) Some of the `request-handler` tests could be rewritten at a higher level, with lower-level data-driven tests directly against `cache-headers`. --- lib/coalesce.js | 5 + lib/coalesce.spec.js | 23 ++++ lib/request-handler.js | 47 ++------ lib/request-handler.spec.js | 104 ++++++++--------- lib/server-config.js | 7 +- package-lock.json | 168 ++++++++++++++++++--------- package.json | 2 + server.js | 19 +-- server.spec.js | 5 +- services/base-json.spec.js | 62 +++++----- services/base-non-memory-caching.js | 55 +++++++++ services/base-static.js | 42 +++---- services/base-svg-scraping.spec.js | 74 ++++++------ services/base-xml.spec.js | 76 ++++++------ services/base.js | 46 ++++---- services/base.spec.js | 102 ++++++++-------- services/cache-headers.js | 99 ++++++++++++++++ services/cache-headers.spec.js | 173 ++++++++++++++++++++++++++++ services/flip/flip.service.js | 33 ++++++ services/flip/flip.tester.js | 12 ++ services/legacy-service.js | 8 +- services/time/time.service.js | 4 +- 22 files changed, 785 insertions(+), 381 deletions(-) create mode 100644 lib/coalesce.js create mode 100644 lib/coalesce.spec.js create mode 100644 services/base-non-memory-caching.js create mode 100644 services/cache-headers.js create mode 100644 services/cache-headers.spec.js create mode 100644 services/flip/flip.service.js create mode 100644 services/flip/flip.tester.js diff --git a/lib/coalesce.js b/lib/coalesce.js new file mode 100644 index 0000000000..4875b409c7 --- /dev/null +++ b/lib/coalesce.js @@ -0,0 +1,5 @@ +'use strict' + +module.exports = function coalesce(...candidates) { + return candidates.find(c => c !== undefined && c !== null) +} diff --git a/lib/coalesce.spec.js b/lib/coalesce.spec.js new file mode 100644 index 0000000000..ed596d4276 --- /dev/null +++ b/lib/coalesce.spec.js @@ -0,0 +1,23 @@ +'use strict' + +const { test, given } = require('sazerac') +const coalesce = require('./coalesce') + +// Sticking with our one-line spread implementation, and defaulting to +// `undefined` instead of `null`, though h/t to +// https://github.com/royriojas/coalescy for these tests! + +describe('coalesce', function() { + test(coalesce, function() { + given().expect(undefined) + given(null, []).expect([]) + given(null, [], {}).expect([]) + given(null, undefined, 0, {}).expect(0) + + const a = null, + c = 0, + d = 1 + let b + given(a, b, c, d).expect(0) + }) +}) diff --git a/lib/request-handler.js b/lib/request-handler.js index 7fc717b278..e9938da800 100644 --- a/lib/request-handler.js +++ b/lib/request-handler.js @@ -11,6 +11,7 @@ const analytics = require('./analytics') const { makeSend } = require('./result-sender') const queryString = require('query-string') const { Inaccessible } = require('../services/errors') +const { setCacheHeaders } = require('../services/cache-headers') // We avoid calling the vendor's server for computation of the information in a // number of badges. @@ -57,23 +58,6 @@ function flattenQueryParams(queryParams) { return Array.from(union).sort() } -function getBadgeMaxAge(handlerOptions, queryParams) { - let maxAge = isInt(process.env.BADGE_MAX_AGE_SECONDS) - ? parseInt(process.env.BADGE_MAX_AGE_SECONDS) - : 120 - if (handlerOptions.cacheLength) { - // If we've set a more specific cache length for this badge (or category), - // use that instead of env.BADGE_MAX_AGE_SECONDS. - maxAge = handlerOptions.cacheLength - } - if (isInt(queryParams.maxAge) && parseInt(queryParams.maxAge) > maxAge) { - // Only allow queryParams.maxAge to override the default if it is greater - // than the default. - maxAge = parseInt(queryParams.maxAge) - } - return maxAge -} - // handlerOptions can contain: // - handler: The service's request handler function // - queryParams: An array of the field names of any custom query parameters @@ -89,31 +73,23 @@ function getBadgeMaxAge(handlerOptions, queryParams) { // (undesirable and hard to debug). // // Pass just the handler function as shorthand. -function handleRequest(handlerOptions) { +function handleRequest(cacheHeaderConfig, handlerOptions) { if (typeof handlerOptions === 'function') { handlerOptions = { handler: handlerOptions } } const allowedKeys = flattenQueryParams(handlerOptions.queryParams) + const { cacheLength: serviceCacheLengthSeconds } = handlerOptions return (queryParams, match, end, ask) => { const reqTime = new Date() - const maxAge = getBadgeMaxAge(handlerOptions, queryParams) - // send both Cache-Control max-age and Expires - // in case the client implements HTTP/1.0 but not HTTP/1.1 - if (maxAge === 0) { - ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate') - ask.res.setHeader('Expires', reqTime.toGMTString()) - } else { - ask.res.setHeader('Cache-Control', `max-age=${maxAge}`) - ask.res.setHeader( - 'Expires', - new Date(+reqTime + maxAge * 1000).toGMTString() - ) - } - - ask.res.setHeader('Date', reqTime.toGMTString()) + setCacheHeaders({ + cacheHeaderConfig, + serviceCacheLengthSeconds, + queryParams, + res: ask.res, + }) analytics.noteRequest(queryParams, match) @@ -269,14 +245,9 @@ function clearRequestCache() { requestCache.clear() } -function isInt(number) { - return number !== undefined && /^[0-9]+$/.test(number) -} - module.exports = { handleRequest, clearRequestCache, // Expose for testing. _requestCache: requestCache, - getBadgeMaxAge, } diff --git a/lib/request-handler.spec.js b/lib/request-handler.spec.js index 26b46e3e52..cd269f77d7 100644 --- a/lib/request-handler.spec.js +++ b/lib/request-handler.spec.js @@ -1,7 +1,6 @@ 'use strict' const { expect } = require('chai') -const { test, given } = require('sazerac') const fetch = require('node-fetch') const config = require('./test-config') const Camp = require('camp') @@ -11,7 +10,6 @@ const { handleRequest, clearRequestCache, _requestCache, - getBadgeMaxAge, } = require('./request-handler') const baseUri = `http://127.0.0.1:${config.port}` @@ -32,8 +30,6 @@ describe('The request handler', function() { before(analytics.load) let camp - const initialBadgeMaxAge = process.env.BADGE_MAX_AGE_SECONDS - beforeEach(function(done) { camp = Camp.start({ port: config.port, hostname: '::' }) camp.on('listening', () => done()) @@ -44,14 +40,15 @@ describe('The request handler', function() { camp.close(() => done()) camp = null } - process.env.BADGE_MAX_AGE_SECONDS = initialBadgeMaxAge }) + const standardCacheHeaders = { defaultCacheLengthSeconds: 120 } + describe('the options object calling style', function() { beforeEach(function() { camp.route( /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, - handleRequest({ handler: fakeHandler }) + handleRequest(standardCacheHeaders, { handler: fakeHandler }) ) }) @@ -66,7 +63,7 @@ describe('The request handler', function() { beforeEach(function() { camp.route( /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, - handleRequest(fakeHandler) + handleRequest(standardCacheHeaders, fakeHandler) ) }) @@ -82,38 +79,50 @@ describe('The request handler', function() { let handlerCallCount beforeEach(function() { handlerCallCount = 0 + }) + + function register({ cacheHeaderConfig }) { camp.route( /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, - handleRequest((queryParams, match, sendBadge, request) => { - ++handlerCallCount - fakeHandler(queryParams, match, sendBadge, request) - }) + handleRequest( + cacheHeaderConfig, + (queryParams, match, sendBadge, request) => { + ++handlerCallCount + fakeHandler(queryParams, match, sendBadge, request) + } + ) ) - }) + } - it('should cache identical requests', async function() { - await performTwoRequests('/testing/123.svg', '/testing/123.svg') - expect(handlerCallCount).to.equal(1) - }) + context('With standard cache settings', function() { + beforeEach(function() { + register({ cacheHeaderConfig: standardCacheHeaders }) + }) - it('should differentiate known query parameters', async function() { - await performTwoRequests( - '/testing/123.svg?label=foo', - '/testing/123.svg?label=bar' - ) - expect(handlerCallCount).to.equal(2) - }) + it('should cache identical requests', async function() { + await performTwoRequests('/testing/123.svg', '/testing/123.svg') + expect(handlerCallCount).to.equal(1) + }) - it('should ignore unknown query parameters', async function() { - await performTwoRequests( - '/testing/123.svg?foo=1', - '/testing/123.svg?foo=2' - ) - expect(handlerCallCount).to.equal(1) + it('should differentiate known query parameters', async function() { + await performTwoRequests( + '/testing/123.svg?label=foo', + '/testing/123.svg?label=bar' + ) + expect(handlerCallCount).to.equal(2) + }) + + it('should ignore unknown query parameters', async function() { + await performTwoRequests( + '/testing/123.svg?foo=1', + '/testing/123.svg?foo=2' + ) + expect(handlerCallCount).to.equal(1) + }) }) - it('should set the expires header to current time + BADGE_MAX_AGE_SECONDS', async function() { - process.env.BADGE_MAX_AGE_SECONDS = 900 + it('should set the expires header to current time + defaultCacheLengthSeconds', async function() { + register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 900 } }) const res = await fetch(`${baseUri}/testing/123.json`) const expectedExpiry = new Date( +new Date(res.headers.get('date')) + 900000 @@ -123,7 +132,7 @@ describe('The request handler', function() { }) it('should set the expires header to current time + maxAge', async function() { - process.env.BADGE_MAX_AGE_SECONDS = 0 + register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 0 } }) const res = await fetch(`${baseUri}/testing/123.json?maxAge=3600`) const expectedExpiry = new Date( +new Date(res.headers.get('date')) + 3600000 @@ -132,8 +141,8 @@ describe('The request handler', function() { expect(res.headers.get('cache-control')).to.equal('max-age=3600') }) - it('should ignore maxAge if maxAge < BADGE_MAX_AGE_SECONDS', async function() { - process.env.BADGE_MAX_AGE_SECONDS = 600 + it('should ignore maxAge if maxAge < defaultCacheLengthSeconds', async function() { + register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 600 } }) const res = await fetch(`${baseUri}/testing/123.json?maxAge=300`) const expectedExpiry = new Date( +new Date(res.headers.get('date')) + 600000 @@ -143,7 +152,7 @@ describe('The request handler', function() { }) it('should set Cache-Control: no-cache, no-store, must-revalidate if maxAge=0', async function() { - process.env.BADGE_MAX_AGE_SECONDS = 0 + register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 0 } }) const res = await fetch(`${baseUri}/testing/123.json`) expect(res.headers.get('expires')).to.equal(res.headers.get('date')) expect(res.headers.get('cache-control')).to.equal( @@ -151,17 +160,10 @@ describe('The request handler', function() { ) }) - it('should set the expires header to current time + 120 if BADGE_MAX_AGE_SECONDS not set', async function() { - delete process.env.BADGE_MAX_AGE_SECONDS - const res = await fetch(`${baseUri}/testing/123.json`) - const expectedExpiry = new Date( - +new Date(res.headers.get('date')) + 120000 - ).toGMTString() - expect(res.headers.get('expires')).to.equal(expectedExpiry) - expect(res.headers.get('cache-control')).to.equal('max-age=120') - }) - describe('the cache key', function() { + beforeEach(function() { + register({ cacheHeaderConfig: standardCacheHeaders }) + }) const expectedCacheKey = '/testing/123.json?colorB=123&label=foo' it('should match expected and use canonical order - 1', async function() { const res = await fetch( @@ -186,7 +188,7 @@ describe('The request handler', function() { handlerCallCount = 0 camp.route( /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, - handleRequest({ + handleRequest(standardCacheHeaders, { queryParams: ['foo'], handler: (queryParams, match, sendBadge, request) => { ++handlerCallCount @@ -205,14 +207,4 @@ describe('The request handler', function() { }) }) }) - - describe('getBadgeMaxAge function', function() { - process.env.BADGE_MAX_AGE_SECONDS = 120 - test(getBadgeMaxAge, () => { - given({}, {}).expect(120) - given({ cacheLength: 900 }, {}).expect(900) - given({ cacheLength: 900 }, { maxAge: 1000 }).expect(1000) - given({ cacheLength: 900 }, { maxAge: 400 }).expect(900) - }) - }) }) diff --git a/lib/server-config.js b/lib/server-config.js index 01832d822c..3861158f79 100644 --- a/lib/server-config.js +++ b/lib/server-config.js @@ -63,7 +63,8 @@ const config = { baseUri: process.env.GITHUB_URL || 'https://api.github.com', debug: { enabled: envFlag(process.env.GITHUB_DEBUG_ENABLED, false), - intervalSeconds: process.env.GITHUB_DEBUG_INTERVAL_SECONDS || 300, + intervalSeconds: + parseInt(process.env.GITHUB_DEBUG_INTERVAL_SECONDS) || 300, }, }, trace: envFlag(process.env.TRACE_SERVICES), @@ -71,6 +72,10 @@ const config = { profiling: { makeBadge: envFlag(process.env.PROFILE_MAKE_BADGE), }, + cacheHeaders: { + defaultCacheLengthSeconds: + parseInt(process.env.BADGE_MAX_AGE_SECONDS) || 120, + }, rateLimit: envFlag(process.env.RATE_LIMIT, true), handleInternalErrors: envFlag(process.env.HANDLE_INTERNAL_ERRORS, true), } diff --git a/package-lock.json b/package-lock.json index a25e2a2148..d69417c7a8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1423,8 +1423,7 @@ "acorn": { "version": "2.7.0", "resolved": "https://registry.npmjs.org/acorn/-/acorn-2.7.0.tgz", - "integrity": "sha1-q259nYhqrKiwhbwzEreaGYQz8Oc=", - "optional": true + "integrity": "sha1-q259nYhqrKiwhbwzEreaGYQz8Oc=" }, "acorn-dynamic-import": { "version": "2.0.2", @@ -1717,7 +1716,7 @@ }, "util": { "version": "0.10.3", - "resolved": "https://registry.npmjs.org/util/-/util-0.10.3.tgz", + "resolved": "http://registry.npmjs.org/util/-/util-0.10.3.tgz", "integrity": "sha1-evsa/lCAUkZInj23/g7TeTNqwPk=", "dev": true, "requires": { @@ -2422,6 +2421,15 @@ } } }, + "chai-datetime": { + "version": "1.5.0", + "resolved": "https://registry.npmjs.org/chai-datetime/-/chai-datetime-1.5.0.tgz", + "integrity": "sha1-N0LxiwJMdbdqK37uKRZiMkRnWWw=", + "dev": true, + "requires": { + "chai": ">1.9.0" + } + }, "chai-string": { "version": "1.5.0", "resolved": "https://registry.npmjs.org/chai-string/-/chai-string-1.5.0.tgz", @@ -3458,8 +3466,7 @@ "cssom": { "version": "0.3.2", "resolved": "https://registry.npmjs.org/cssom/-/cssom-0.3.2.tgz", - "integrity": "sha1-uANhcMefB6kP8vFuIihAJ6JDhIs=", - "optional": true + "integrity": "sha1-uANhcMefB6kP8vFuIihAJ6JDhIs=" }, "cssstyle": { "version": "0.2.37", @@ -3894,7 +3901,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -5258,7 +5265,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -5405,7 +5412,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -5481,8 +5488,7 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "aproba": { "version": "1.2.0", @@ -5503,14 +5509,12 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -5525,20 +5529,17 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "core-util-is": { "version": "1.0.2", @@ -5655,8 +5656,7 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "ini": { "version": "1.3.5", @@ -5668,7 +5668,6 @@ "version": "1.0.0", "bundled": true, "dev": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -5683,7 +5682,6 @@ "version": "3.0.4", "bundled": true, "dev": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -5691,14 +5689,12 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "minipass": { "version": "2.2.4", "bundled": true, "dev": true, - "optional": true, "requires": { "safe-buffer": "^5.1.1", "yallist": "^3.0.0" @@ -5717,7 +5713,6 @@ "version": "0.5.1", "bundled": true, "dev": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -5798,8 +5793,7 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "object-assign": { "version": "4.1.1", @@ -5811,7 +5805,6 @@ "version": "1.4.0", "bundled": true, "dev": true, - "optional": true, "requires": { "wrappy": "1" } @@ -5897,8 +5890,7 @@ "safe-buffer": { "version": "5.1.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "safer-buffer": { "version": "2.1.2", @@ -5934,7 +5926,6 @@ "version": "1.0.2", "bundled": true, "dev": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -5954,7 +5945,6 @@ "version": "3.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -5998,14 +5988,12 @@ "wrappy": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "yallist": { "version": "3.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true } } }, @@ -6497,7 +6485,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "requires": { "core-util-is": "~1.0.0", @@ -7984,7 +7972,7 @@ "dependencies": { "chalk": { "version": "1.1.3", - "resolved": "https://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz", + "resolved": "http://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz", "integrity": "sha1-qBFcVeSnAv5NFQq9OHKCKn4J/Jg=", "dev": true, "requires": { @@ -8443,6 +8431,12 @@ "resolved": "https://registry.npmjs.org/mdn-data/-/mdn-data-1.1.4.tgz", "integrity": "sha512-FSYbp3lyKjyj3E7fMl6rYvUdX0FBXaluGqlFoYESWQlyUTq8R+wp0rkFxoYFqZlHCvsUXGjyJmLQSnXToYhOSA==" }, + "media-typer": { + "version": "0.3.0", + "resolved": "http://registry.npmjs.org/media-typer/-/media-typer-0.3.0.tgz", + "integrity": "sha1-hxDXrwqmJvj/+hzgAWhUUmMlV0g=", + "dev": true + }, "mem": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/mem/-/mem-1.1.0.tgz", @@ -8482,7 +8476,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -8607,6 +8601,18 @@ } } }, + "merge-descriptors": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/merge-descriptors/-/merge-descriptors-1.0.1.tgz", + "integrity": "sha1-sAqqVW3YtEVoFQ7J0blT8/kMu2E=", + "dev": true + }, + "methods": { + "version": "1.1.2", + "resolved": "https://registry.npmjs.org/methods/-/methods-1.1.2.tgz", + "integrity": "sha1-VSmk1nZUE07cxSZmVoNbD4Ua/O4=", + "dev": true + }, "micromatch": { "version": "3.1.10", "resolved": "https://registry.npmjs.org/micromatch/-/micromatch-3.1.10.tgz", @@ -8983,6 +8989,12 @@ "integrity": "sha512-MFh0d/Wa7vkKO3Y3LlacqAEeHK0mckVqzDieUKTT+KGxi+zIpeVsFxymkIiRpbpDziHc290Xr9A1O4Om7otoRA==", "dev": true }, + "net": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/net/-/net-1.0.2.tgz", + "integrity": "sha1-0XV+yaf7I3HYPPR1XOPifhCCk4g=", + "dev": true + }, "next": { "version": "6.1.2", "resolved": "https://registry.npmjs.org/next/-/next-6.1.2.tgz", @@ -10139,7 +10151,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -10163,6 +10175,24 @@ } } }, + "node-mocks-http": { + "version": "1.7.3", + "resolved": "https://registry.npmjs.org/node-mocks-http/-/node-mocks-http-1.7.3.tgz", + "integrity": "sha512-wayzLNhEroH3lJj113pFKQ1cd1GKG1mXoZR1HcKp/o9a9lTGGgVY/hYeLajiIFr/z4tXFKOdfJickqqihBtn9g==", + "dev": true, + "requires": { + "accepts": "^1.3.5", + "depd": "^1.1.0", + "fresh": "^0.5.2", + "merge-descriptors": "^1.0.1", + "methods": "^1.1.2", + "mime": "^1.3.4", + "net": "^1.0.2", + "parseurl": "^1.3.1", + "range-parser": "^1.2.0", + "type-is": "^1.6.16" + } + }, "node-modules-regexp": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/node-modules-regexp/-/node-modules-regexp-1.0.0.tgz", @@ -10333,7 +10363,6 @@ "resolved": false, "integrity": "sha1-DNkKVhCT810KmSVsIrcGlDP60Rc=", "dev": true, - "optional": true, "requires": { "kind-of": "^3.0.2", "longest": "^1.0.1", @@ -10703,8 +10732,7 @@ "version": "1.1.6", "resolved": false, "integrity": "sha512-NcdALwpXkTm5Zvvbk7owOUSvVvBKDgKP5/ewfXEznmQFfs4ZRmanOeKBTjRVjka3QFoN6XJ+9F3USqfHqTaU5w==", - "dev": true, - "optional": true + "dev": true }, "is-builtin-module": { "version": "1.0.0", @@ -10800,7 +10828,6 @@ "resolved": false, "integrity": "sha1-MeohpzS6ubuw8yRm2JOupR5KPGQ=", "dev": true, - "optional": true, "requires": { "is-buffer": "^1.1.5" } @@ -10853,8 +10880,7 @@ "version": "1.0.1", "resolved": false, "integrity": "sha1-MKCy2jj3N3DoKUoNIuZiXtd9AJc=", - "dev": true, - "optional": true + "dev": true }, "lru-cache": { "version": "4.1.3", @@ -11157,8 +11183,7 @@ "version": "1.6.1", "resolved": false, "integrity": "sha1-jcrkcOHIirwtYA//Sndihtp15jc=", - "dev": true, - "optional": true + "dev": true }, "require-directory": { "version": "2.1.1", @@ -11921,7 +11946,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -12064,6 +12089,12 @@ "better-assert": "~1.0.0" } }, + "parseurl": { + "version": "1.3.2", + "resolved": "https://registry.npmjs.org/parseurl/-/parseurl-1.3.2.tgz", + "integrity": "sha1-/CidTtiZMRlGDBViUyYs3I3mW/M=", + "dev": true + }, "pascalcase": { "version": "0.1.1", "resolved": "https://registry.npmjs.org/pascalcase/-/pascalcase-0.1.1.tgz", @@ -12821,7 +12852,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -14024,7 +14055,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "requires": { "core-util-is": "~1.0.0", @@ -14236,7 +14267,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -14297,7 +14328,7 @@ }, "readable-stream": { "version": "2.3.6", - "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", + "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz", "integrity": "sha512-tQtKA9WIAhBF3+VLAseyMqZeBjW0AHJoxOtYqSUZNJxauErmLbVm2FW1y+J/YA9dUrAC39ITejlZWhVIwawkKw==", "dev": true, "requires": { @@ -14879,6 +14910,33 @@ "resolved": "https://registry.npmjs.org/type-detect/-/type-detect-4.0.3.tgz", "integrity": "sha1-Dj8mcLRAmbC0bChNE2p+9Jx0wuo=" }, + "type-is": { + "version": "1.6.16", + "resolved": "https://registry.npmjs.org/type-is/-/type-is-1.6.16.tgz", + "integrity": "sha512-HRkVv/5qY2G6I8iab9cI7v1bOIdhm94dVjQCPFElW9W+3GeDOSHmy2EBYe4VTApuzolPcmgFTN3ftVJRKR2J9Q==", + "dev": true, + "requires": { + "media-typer": "0.3.0", + "mime-types": "~2.1.18" + }, + "dependencies": { + "mime-db": { + "version": "1.37.0", + "resolved": "https://registry.npmjs.org/mime-db/-/mime-db-1.37.0.tgz", + "integrity": "sha512-R3C4db6bgQhlIhPU48fUtdVmKnflq+hRdad7IyKhtFj06VPNVdk2RhiYL3UjQIlso8L+YxAtFkobT0VK+S/ybg==", + "dev": true + }, + "mime-types": { + "version": "2.1.21", + "resolved": "https://registry.npmjs.org/mime-types/-/mime-types-2.1.21.tgz", + "integrity": "sha512-3iL6DbwpyLzjR3xHSFNFeb9Nz/M8WDkX33t1GFQnFOllWk8pOrh/LSrB5OXlnlW5P9LH73X6loW/eogc+F5lJg==", + "dev": true, + "requires": { + "mime-db": "~1.37.0" + } + } + } + }, "typedarray": { "version": "0.0.6", "resolved": "https://registry.npmjs.org/typedarray/-/typedarray-0.0.6.tgz", diff --git a/package.json b/package.json index 40031e7a63..3bd2f7d08f 100644 --- a/package.json +++ b/package.json @@ -114,6 +114,7 @@ "babel-eslint": "^10.0.0", "caller": "^1.0.1", "chai": "^4.1.2", + "chai-datetime": "^1.5.0", "chai-string": "^1.4.0", "chainsmoker": "^0.1.0", "child-process-promise": "^2.2.1", @@ -154,6 +155,7 @@ "next": "^6.1.1", "nock": "^10.0.0", "node-fetch": "^2.3.0", + "node-mocks-http": "^1.7.3", "nyc": "^13.0.1", "opn-cli": "^4.0.0", "prettier": "1.15.3", diff --git a/server.js b/server.js index 132b7495d4..b554d25215 100644 --- a/server.js +++ b/server.js @@ -105,6 +105,7 @@ loadServiceClasses().forEach(serviceClass => { camp, handleRequest: cache, githubApiProvider }, { handleInternalErrors: config.handleInternalErrors, + cacheHeaders: config.cacheHeaders, profiling: config.profiling, } ) @@ -113,7 +114,7 @@ loadServiceClasses().forEach(serviceClass => // User defined sources - JSON response camp.route( /^\/badge\/dynamic\/(json|xml|yaml)\.(svg|png|gif|jpg|json)$/, - cache({ + cache(config.cacheHeaders, { queryParams: ['uri', 'url', 'query', 'prefix', 'suffix'], handler: function(query, match, sendBadge, request) { const type = match[1] @@ -226,22 +227,6 @@ camp.route( }) ) -// Production cache debugging. -let bitFlip = false -camp.route(/^\/flip\.svg$/, (data, match, end, ask) => { - const cacheSecs = 60 - ask.res.setHeader('Cache-Control', `max-age=${cacheSecs}`) - const reqTime = new Date() - const date = new Date(+reqTime + cacheSecs * 1000).toGMTString() - ask.res.setHeader('Expires', date) - const badgeData = getBadgeData('flip', data) - bitFlip = !bitFlip - badgeData.text[1] = bitFlip ? 'on' : 'off' - badgeData.colorscheme = bitFlip ? 'brightgreen' : 'red' - const svg = makeBadge(badgeData) - makeSend('svg', ask.res, end)(svg) -}) - // Any badge, old version. This route must be registered last. camp.route(/^\/([^/]+)\/(.+).png$/, (queryParams, match, end, ask) => { const [, label, message] = match diff --git a/server.spec.js b/server.spec.js index 781e8e5011..9320288b3a 100644 --- a/server.spec.js +++ b/server.spec.js @@ -2,6 +2,7 @@ const { expect } = require('chai') const config = require('./lib/test-config') +const serverConfig = require('./lib/server-config') const fetch = require('node-fetch') const fs = require('fs') const isPng = require('is-png') @@ -33,7 +34,7 @@ describe('The server', function() { // at _combinedTickCallback (internal/process/next_tick.js:141:11) // at process._tickDomainCallback (internal/process/next_tick.js:218:9) loadServiceClasses().forEach(serviceClass => - serviceClass.register({ camp: dummyCamp, handleRequest }, {}) + serviceClass.register({ camp: dummyCamp, handleRequest }, serverConfig) ) dummyCamp.close() dummyCamp = undefined @@ -50,7 +51,7 @@ describe('The server', function() { const baseUri = `http://127.0.0.1:${config.port}` let server - before('Start running the server', function() { + before('Start the server', function() { this.timeout(5000) server = serverHelpers.start() }) diff --git a/services/base-json.spec.js b/services/base-json.spec.js index ae47d85361..bc8ba92a10 100644 --- a/services/base-json.spec.js +++ b/services/base-json.spec.js @@ -32,7 +32,7 @@ class DummyJsonService extends BaseJsonService { describe('BaseJsonService', function() { describe('Making requests', function() { - let sendAndCacheRequest, serviceInstance + let sendAndCacheRequest beforeEach(function() { sendAndCacheRequest = sinon.stub().returns( Promise.resolve({ @@ -40,25 +40,22 @@ describe('BaseJsonService', function() { res: { statusCode: 200 }, }) ) - serviceInstance = new DummyJsonService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) }) it('invokes _sendAndCacheRequest', async function() { - await serviceInstance.invokeHandler({}, {}) + await DummyJsonService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.json', - { - headers: { Accept: 'application/json' }, - } + { headers: { Accept: 'application/json' } } ) }) it('forwards options to _sendAndCacheRequest', async function() { - Object.assign(serviceInstance, { + class WithOptions extends DummyJsonService { async handle() { const { value } = await this._requestJson({ schema: dummySchema, @@ -66,10 +63,13 @@ describe('BaseJsonService', function() { options: { method: 'POST', qs: { queryParam: 123 } }, }) return { message: value } - }, - }) + } + } - await serviceInstance.invokeHandler({}, {}) + await WithOptions.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.json', @@ -88,12 +88,12 @@ describe('BaseJsonService', function() { buffer: '{"requiredString": "some-string"}', res: { statusCode: 200 }, }) - const serviceInstance = new DummyJsonService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyJsonService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ message: 'some-string', }) }) @@ -103,12 +103,12 @@ describe('BaseJsonService', function() { buffer: '{"unexpectedKey": "some-string"}', res: { statusCode: 200 }, }) - const serviceInstance = new DummyJsonService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyJsonService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ color: 'lightgray', message: 'invalid response data', }) @@ -119,12 +119,12 @@ describe('BaseJsonService', function() { buffer: 'not json', res: { statusCode: 200 }, }) - const serviceInstance = new DummyJsonService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyJsonService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ color: 'lightgray', message: 'unparseable json response', }) diff --git a/services/base-non-memory-caching.js b/services/base-non-memory-caching.js new file mode 100644 index 0000000000..225983a2fb --- /dev/null +++ b/services/base-non-memory-caching.js @@ -0,0 +1,55 @@ +'use strict' + +const makeBadge = require('../gh-badges/lib/make-badge') +const { makeSend } = require('../lib/result-sender') +const BaseService = require('./base') +const { setCacheHeaders } = require('./cache-headers') + +// Badges are subject to two independent types of caching: in-memory and +// downstream. +// +// Services deriving from `NonMemoryCachingBaseService` are not cached in +// memory on the server. This means that each request that hits the server +// triggers another call to the handler. When using badges for server +// diagnostics, that's useful! +// +// In constrast, The `handle()` function of most other `BaseService` +// subclasses is wrapped in onboard, in-memory caching. See `lib /request- +// handler.js` and `BaseService.prototype.register()`. +// +// All services, including those extending NonMemoryCachingBaseServices, may +// be cached _downstream_. This is governed by cache headers, which are +// configured by the service, the user's request, and the server's default +// cache length. +module.exports = class NonMemoryCachingBaseService extends BaseService { + static register({ camp }, serviceConfig) { + const { cacheHeaders: cacheHeaderConfig } = serviceConfig + const { _cacheLength: serviceCacheLengthSeconds } = this + + camp.route(this._regex, async (queryParams, match, end, ask) => { + const namedParams = this._namedParamsForMatch(match) + const serviceData = await this.invoke( + {}, + serviceConfig, + namedParams, + queryParams + ) + + const badgeData = this._makeBadgeData(queryParams, serviceData) + // The final capture group is the extension. + const format = match.slice(-1)[0] + badgeData.format = format + + const svg = makeBadge(badgeData) + + setCacheHeaders({ + cacheHeaderConfig, + serviceCacheLengthSeconds, + queryParams, + res: ask.res, + }) + + makeSend(format, ask.res, end)(svg) + }) + } +} diff --git a/services/base-static.js b/services/base-static.js index cbaac7ea03..6d146ce21e 100644 --- a/services/base-static.js +++ b/services/base-static.js @@ -4,53 +4,49 @@ const makeBadge = require('../gh-badges/lib/make-badge') const { makeSend } = require('../lib/result-sender') const analytics = require('../lib/analytics') const BaseService = require('./base') - -const serverStartTime = new Date(new Date().toGMTString()) +const { + serverHasBeenUpSinceResourceCached, + setCacheHeadersForStaticResource, +} = require('./cache-headers') module.exports = class BaseStaticService extends BaseService { - // Note: Since this is a static service, it is not `async`. - handle(namedParams, queryParams) { - throw new Error(`Handler not implemented for ${this.constructor.name}`) - } - static register({ camp }, serviceConfig) { - camp.route(this._regex, (queryParams, match, end, ask) => { + const { + profiling: { makeBadge: shouldProfileMakeBadge }, + } = serviceConfig + + camp.route(this._regex, async (queryParams, match, end, ask) => { analytics.noteRequest(queryParams, match) - if (+new Date(ask.req.headers['if-modified-since']) >= +serverStartTime) { + if (serverHasBeenUpSinceResourceCached(ask.req)) { // Send Not Modified. ask.res.statusCode = 304 ask.res.end() return } - const serviceInstance = new this({}, serviceConfig) const namedParams = this._namedParamsForMatch(match) - let serviceData - try { - // Note: no `await`. - serviceData = serviceInstance.handle(namedParams, queryParams) - } catch (error) { - serviceData = serviceInstance._handleError(error) - } + const serviceData = await this.invoke( + {}, + serviceConfig, + namedParams, + queryParams + ) const badgeData = this._makeBadgeData(queryParams, serviceData) - // The final capture group is the extension. const format = match.slice(-1)[0] badgeData.format = format - if (serviceConfig.profiling.makeBadge) { + if (shouldProfileMakeBadge) { console.time('makeBadge total') } const svg = makeBadge(badgeData) - if (serviceConfig.profiling.makeBadge) { + if (shouldProfileMakeBadge) { console.timeEnd('makeBadge total') } - const cacheDuration = 3600 * 24 * 1 // 1 day. - ask.res.setHeader('Cache-Control', `max-age=${cacheDuration}`) - ask.res.setHeader('Last-Modified', serverStartTime.toGMTString()) + setCacheHeadersForStaticResource(ask.res) makeSend(format, ask.res, end)(svg) }) diff --git a/services/base-svg-scraping.spec.js b/services/base-svg-scraping.spec.js index 550f8b1c0e..cfa37b2bce 100644 --- a/services/base-svg-scraping.spec.js +++ b/services/base-svg-scraping.spec.js @@ -53,7 +53,7 @@ describe('BaseSvgScrapingService', function() { }) describe('Making requests', function() { - let sendAndCacheRequest, serviceInstance + let sendAndCacheRequest beforeEach(function() { sendAndCacheRequest = sinon.stub().returns( Promise.resolve({ @@ -61,25 +61,22 @@ describe('BaseSvgScrapingService', function() { res: { statusCode: 200 }, }) ) - serviceInstance = new DummySvgScrapingService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) }) it('invokes _sendAndCacheRequest with the expected header', async function() { - await serviceInstance.invokeHandler({}, {}) + await DummySvgScrapingService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.svg', - { - headers: { Accept: 'image/svg+xml' }, - } + { headers: { Accept: 'image/svg+xml' } } ) }) it('forwards options to _sendAndCacheRequest', async function() { - Object.assign(serviceInstance, { + class WithCustomOptions extends DummySvgScrapingService { async handle() { const { value } = await this._requestSvg({ schema, @@ -90,10 +87,13 @@ describe('BaseSvgScrapingService', function() { }, }) return { message: value } - }, - }) + } + } - await serviceInstance.invokeHandler({}, {}) + await WithCustomOptions.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.svg', @@ -112,36 +112,36 @@ describe('BaseSvgScrapingService', function() { buffer: exampleSvg, res: { statusCode: 200 }, }) - const serviceInstance = new DummySvgScrapingService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummySvgScrapingService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ message: exampleMessage, }) }) it('allows overriding the valueMatcher', async function() { - const sendAndCacheRequest = async () => ({ - buffer: '<desc>a different message</desc>', - res: { statusCode: 200 }, - }) - const serviceInstance = new DummySvgScrapingService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - Object.assign(serviceInstance, { + class WithValueMatcher extends BaseSvgScrapingService { async handle() { return this._requestSvg({ schema, valueMatcher: />([^<>]+)<\/desc>/, url: 'http://example.com/foo.svg', }) - }, + } + } + const sendAndCacheRequest = async () => ({ + buffer: '<desc>a different message</desc>', + res: { statusCode: 200 }, }) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await WithValueMatcher.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ message: 'a different message', }) }) @@ -151,12 +151,12 @@ describe('BaseSvgScrapingService', function() { buffer: 'not svg yo', res: { statusCode: 200 }, }) - const serviceInstance = new DummySvgScrapingService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummySvgScrapingService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ color: 'lightgray', message: 'unparseable svg response', }) diff --git a/services/base-xml.spec.js b/services/base-xml.spec.js index 801cb5885c..5c3ad696ab 100644 --- a/services/base-xml.spec.js +++ b/services/base-xml.spec.js @@ -32,7 +32,7 @@ class DummyXmlService extends BaseXmlService { describe('BaseXmlService', function() { describe('Making requests', function() { - let sendAndCacheRequest, serviceInstance + let sendAndCacheRequest beforeEach(function() { sendAndCacheRequest = sinon.stub().returns( Promise.resolve({ @@ -40,25 +40,22 @@ describe('BaseXmlService', function() { res: { statusCode: 200 }, }) ) - serviceInstance = new DummyXmlService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) }) it('invokes _sendAndCacheRequest', async function() { - await serviceInstance.invokeHandler({}, {}) + await DummyXmlService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.xml', - { - headers: { Accept: 'application/xml, text/xml' }, - } + { headers: { Accept: 'application/xml, text/xml' } } ) }) it('forwards options to _sendAndCacheRequest', async function() { - Object.assign(serviceInstance, { + class WithCustomOptions extends BaseXmlService { async handle() { const { value } = await this._requestXml({ schema: dummySchema, @@ -66,10 +63,13 @@ describe('BaseXmlService', function() { options: { method: 'POST', qs: { queryParam: 123 } }, }) return { message: value } - }, - }) + } + } - await serviceInstance.invokeHandler({}, {}) + await WithCustomOptions.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) expect(sendAndCacheRequest).to.have.been.calledOnceWith( 'http://example.com/foo.xml', @@ -88,12 +88,12 @@ describe('BaseXmlService', function() { buffer: '<requiredString>some-string</requiredString>', res: { statusCode: 200 }, }) - const serviceInstance = new DummyXmlService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyXmlService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ message: 'some-string', }) }) @@ -115,14 +115,12 @@ describe('BaseXmlService', function() { '<requiredString>some-string with trailing whitespace </requiredString>', res: { statusCode: 200 }, }) - const serviceInstance = new DummyXmlServiceWithParserOption( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - - const serviceData = await serviceInstance.invokeHandler({}, {}) - - expect(serviceData).to.deep.equal({ + expect( + await DummyXmlServiceWithParserOption.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ message: 'some-string with trailing whitespace ', }) }) @@ -132,12 +130,12 @@ describe('BaseXmlService', function() { buffer: '<unexpectedAttribute>some-string</unexpectedAttribute>', res: { statusCode: 200 }, }) - const serviceInstance = new DummyXmlService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyXmlService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ color: 'lightgray', message: 'invalid response data', }) @@ -148,12 +146,12 @@ describe('BaseXmlService', function() { buffer: 'not xml', res: { statusCode: 200 }, }) - const serviceInstance = new DummyXmlService( - { sendAndCacheRequest }, - { handleInternalErrors: false } - ) - const serviceData = await serviceInstance.invokeHandler({}, {}) - expect(serviceData).to.deep.equal({ + expect( + await DummyXmlService.invoke( + { sendAndCacheRequest }, + { handleInternalErrors: false } + ) + ).to.deep.equal({ color: 'lightgray', message: 'unparseable xml response', }) diff --git a/services/base.js b/services/base.js index f1279c06b9..13b13ab117 100644 --- a/services/base.js +++ b/services/base.js @@ -11,6 +11,7 @@ const { InvalidParameter, Deprecated, } = require('./errors') +const coalesce = require('../lib/coalesce') const validate = require('../lib/validate') const { checkErrorResponse } = require('../lib/error-helper') const { @@ -23,10 +24,6 @@ const { staticBadgeUrl } = require('../lib/make-badge-url') const trace = require('./trace') const validateExample = require('./validate-example') -function coalesce(...candidates) { - return candidates.find(c => c !== undefined) -} - class BaseService { constructor({ sendAndCacheRequest }, { handleInternalErrors }) { this._requestFetcher = sendAndCacheRequest @@ -249,6 +246,7 @@ class BaseService { build: 30, license: 3600, version: 300, + debug: 60, } return cacheLengths[this.category] } @@ -321,20 +319,28 @@ class BaseService { } } - async invokeHandler(namedParams, queryParams) { - trace.logTrace( - 'inbound', - emojic.womanCook, - 'Service class', - this.constructor.name - ) + static async invoke( + context = {}, + config = {}, + namedParams = {}, + queryParams = {} + ) { + trace.logTrace('inbound', emojic.womanCook, 'Service class', this.name) trace.logTrace('inbound', emojic.ticket, 'Named params', namedParams) trace.logTrace('inbound', emojic.crayon, 'Query params', queryParams) + + const serviceInstance = new this(context, config) + + let serviceData try { - return await this.handle(namedParams, queryParams) + serviceData = await serviceInstance.handle(namedParams, queryParams) } catch (error) { - return this._handleError(error) + serviceData = serviceInstance._handleError(error) } + + trace.logTrace('outbound', emojic.shield, 'Service data', serviceData) + + return serviceData } static _makeBadgeData(overrides, serviceData) { @@ -385,27 +391,25 @@ class BaseService { } static register({ camp, handleRequest, githubApiProvider }, serviceConfig) { + const { cacheHeaders: cacheHeaderConfig } = serviceConfig camp.route( this._regex, - handleRequest({ + handleRequest(cacheHeaderConfig, { queryParams: this.route.queryParams, handler: async (queryParams, match, sendBadge, request) => { - const serviceInstance = new this( + const namedParams = this._namedParamsForMatch(match) + const serviceData = await this.invoke( { sendAndCacheRequest: request.asPromise, sendAndCacheRequestWithCallbacks: request, githubApiProvider, }, - serviceConfig - ) - const namedParams = this._namedParamsForMatch(match) - const serviceData = await serviceInstance.invokeHandler( + serviceConfig, namedParams, queryParams ) - trace.logTrace('outbound', emojic.shield, 'Service data', serviceData) - const badgeData = this._makeBadgeData(queryParams, serviceData) + const badgeData = this._makeBadgeData(queryParams, serviceData) // The final capture group is the extension. const format = match.slice(-1)[0] sendBadge(format, badgeData) diff --git a/services/base.spec.js b/services/base.spec.js index 3dbaf79835..34fdbca275 100644 --- a/services/base.spec.js +++ b/services/base.spec.js @@ -198,12 +198,14 @@ describe('BaseService', function() { }) it('Invokes the handler as expected', async function() { - const serviceInstance = new DummyService({}, defaultConfig) - const serviceData = await serviceInstance.invokeHandler( - { namedParamA: 'bar.bar.bar' }, - { queryParamA: '!' } - ) - expect(serviceData).to.deep.equal({ + expect( + await DummyService.invoke( + {}, + defaultConfig, + { namedParamA: 'bar.bar.bar' }, + { queryParamA: '!' } + ) + ).to.deep.equal({ message: 'Hello namedParamA: bar.bar.bar with queryParamA: !', }) }) @@ -220,11 +222,10 @@ describe('BaseService', function() { sandbox.stub(trace, 'logTrace') }) it('Invokes the logger as expected', async function() { - const serviceInstance = new DummyService({}, defaultConfig) - await serviceInstance.invokeHandler( - { - namedParamA: 'bar.bar.bar', - }, + await DummyService.invoke( + {}, + defaultConfig, + { namedParamA: 'bar.bar.bar' }, { queryParamA: '!' } ) expect(trace.logTrace).to.be.calledWithMatch( @@ -237,9 +238,7 @@ describe('BaseService', function() { 'inbound', sinon.match.string, 'Named params', - { - namedParamA: 'bar.bar.bar', - } + { namedParamA: 'bar.bar.bar' } ) expect(trace.logTrace).to.be.calledWith( 'inbound', @@ -252,17 +251,17 @@ describe('BaseService', function() { describe('Error handling', function() { it('Handles internal errors', async function() { - const serviceInstance = new DummyService( - {}, - { handleInternalErrors: true } - ) - serviceInstance.handle = () => { - throw Error("I've made a huge mistake") + class ThrowingService extends DummyService { + async handle() { + throw Error("I've made a huge mistake") + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke( + {}, + { handleInternalErrors: true }, + { namedParamA: 'bar.bar.bar' } + ) ).to.deep.equal({ color: 'lightgray', label: 'shields', @@ -271,19 +270,14 @@ describe('BaseService', function() { }) describe('Handles known subtypes of ShieldsInternalError', function() { - let serviceInstance - beforeEach(function() { - serviceInstance = new DummyService({}, {}) - }) - it('handles NotFound errors', async function() { - serviceInstance.handle = () => { - throw new NotFound() + class ThrowingService extends DummyService { + async handle() { + throw new NotFound() + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke({}, {}, { namedParamA: 'bar.bar.bar' }) ).to.deep.equal({ color: 'red', message: 'not found', @@ -291,13 +285,13 @@ describe('BaseService', function() { }) it('handles Inaccessible errors', async function() { - serviceInstance.handle = () => { - throw new Inaccessible() + class ThrowingService extends DummyService { + async handle() { + throw new Inaccessible() + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke({}, {}, { namedParamA: 'bar.bar.bar' }) ).to.deep.equal({ color: 'lightgray', message: 'inaccessible', @@ -305,13 +299,13 @@ describe('BaseService', function() { }) it('handles InvalidResponse errors', async function() { - serviceInstance.handle = () => { - throw new InvalidResponse() + class ThrowingService extends DummyService { + async handle() { + throw new InvalidResponse() + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke({}, {}, { namedParamA: 'bar.bar.bar' }) ).to.deep.equal({ color: 'lightgray', message: 'invalid', @@ -319,13 +313,13 @@ describe('BaseService', function() { }) it('handles Deprecated', async function() { - serviceInstance.handle = () => { - throw new Deprecated() + class ThrowingService extends DummyService { + async handle() { + throw new Deprecated() + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke({}, {}, { namedParamA: 'bar.bar.bar' }) ).to.deep.equal({ color: 'lightgray', message: 'no longer available', @@ -333,13 +327,13 @@ describe('BaseService', function() { }) it('handles InvalidParameter errors', async function() { - serviceInstance.handle = () => { - throw new InvalidParameter() + class ThrowingService extends DummyService { + async handle() { + throw new InvalidParameter() + } } expect( - await serviceInstance.invokeHandler({ - namedParamA: 'bar.bar.bar', - }) + await ThrowingService.invoke({}, {}, { namedParamA: 'bar.bar.bar' }) ).to.deep.equal({ color: 'red', message: 'invalid parameter', @@ -431,7 +425,7 @@ describe('BaseService', function() { it('handles the request', async function() { expect(mockHandleRequest).to.have.been.calledOnce - const { handler: requestHandler } = mockHandleRequest.getCall(0).args[0] + const { handler: requestHandler } = mockHandleRequest.getCall(0).args[1] const mockSendBadge = sinon.spy() const mockRequest = { diff --git a/services/cache-headers.js b/services/cache-headers.js new file mode 100644 index 0000000000..baca92da87 --- /dev/null +++ b/services/cache-headers.js @@ -0,0 +1,99 @@ +'use strict' + +const assert = require('assert') +const Joi = require('joi') +const coalesce = require('../lib/coalesce') + +const serverStartTimeGMTString = new Date().toGMTString() +const serverStartTimestamp = Date.now() + +const queryParamSchema = Joi.object({ + // Not using nonNegativeInteger because it's not required. + maxAge: Joi.number() + .integer() + .min(0), +}).required() + +function coalesceCacheLength( + cacheHeaderConfig, + serviceCacheLengthSeconds, + queryParams +) { + const { defaultCacheLengthSeconds } = cacheHeaderConfig + // The config returns a number so this would only fail if we break the + // wiring. Better to fail obviously than silently. + assert(defaultCacheLengthSeconds !== undefined) + + const ourCacheLength = coalesce( + serviceCacheLengthSeconds, + defaultCacheLengthSeconds + ) + + const { value: { maxAge: overrideCacheLength } = {}, error } = Joi.validate( + queryParams, + queryParamSchema, + { allowUnknown: true } + ) + + if (!error && overrideCacheLength !== undefined) { + // The user can request _more_ caching, but not less. + return Math.max(overrideCacheLength, ourCacheLength) + } else { + return ourCacheLength + } +} + +function setHeadersForCacheLength(res, cacheLengthSeconds) { + const now = new Date() + const nowGMTString = now.toGMTString() + + // Send both Cache-Control max-age and Expires in case the client implements + // HTTP/1.0 but not HTTP/1.1. + let cacheControl, expires + if (cacheLengthSeconds === 0) { + // Prevent as much downstream caching as possible. + cacheControl = 'no-cache, no-store, must-revalidate' + expires = nowGMTString + } else { + cacheControl = `max-age=${cacheLengthSeconds}` + expires = new Date(now.getTime() + cacheLengthSeconds * 1000).toGMTString() + } + + res.setHeader('Date', nowGMTString) + res.setHeader('Cache-Control', cacheControl) + res.setHeader('Expires', expires) +} + +function setCacheHeaders({ + cacheHeaderConfig, + serviceCacheLengthSeconds, + queryParams, + res, +}) { + const cacheLengthSeconds = coalesceCacheLength( + cacheHeaderConfig, + serviceCacheLengthSeconds, + queryParams + ) + setHeadersForCacheLength(res, cacheLengthSeconds) +} + +const staticCacheControlHeader = `max-age=${24 * 3600}` // 1 day. +function setCacheHeadersForStaticResource(res) { + res.setHeader('Cache-Control', staticCacheControlHeader) + res.setHeader('Last-Modified', serverStartTimeGMTString) +} + +function serverHasBeenUpSinceResourceCached(req) { + return ( + serverStartTimestamp <= new Date(req.headers['if-modified-since']).getTime() + ) +} + +module.exports = { + coalesceCacheLength, + setCacheHeaders, + setHeadersForCacheLength, + setCacheHeadersForStaticResource, + serverHasBeenUpSinceResourceCached, +} diff --git a/services/cache-headers.spec.js b/services/cache-headers.spec.js new file mode 100644 index 0000000000..2a3562ffdc --- /dev/null +++ b/services/cache-headers.spec.js @@ -0,0 +1,173 @@ +'use strict' + +const { test, given } = require('sazerac') +const chai = require('chai') +const { expect } = require('chai') +const sinon = require('sinon') +const httpMocks = require('node-mocks-http') +const { + coalesceCacheLength, + setHeadersForCacheLength, + setCacheHeaders, + setCacheHeadersForStaticResource, + serverHasBeenUpSinceResourceCached, +} = require('./cache-headers') + +chai.use(require('chai-datetime')) + +describe('Cache header functions', function() { + let res + beforeEach(function() { + res = httpMocks.createResponse() + }) + + describe('coalesceCacheLength', function() { + const exampleCacheConfig = { defaultCacheLengthSeconds: 777 } + test(coalesceCacheLength, () => { + given(exampleCacheConfig, undefined, {}).expect(777) + given(exampleCacheConfig, 900, {}).expect(900) + given(exampleCacheConfig, 900, { maxAge: 1000 }).expect(1000) + given(exampleCacheConfig, 900, { maxAge: 400 }).expect(900) + given(exampleCacheConfig, 900, { maxAge: '-1000' }).expect(900) + given(exampleCacheConfig, 900, { maxAge: '' }).expect(900) + given(exampleCacheConfig, 900, { maxAge: 'not a number' }).expect(900) + }) + }) + + describe('setHeadersForCacheLength', function() { + let sandbox + beforeEach(function() { + sandbox = sinon.createSandbox() + sandbox.useFakeTimers() + }) + afterEach(function() { + sandbox.restore() + sandbox = undefined + }) + + it('should set the correct Date header', function() { + // Confidence check. + expect(res._headers.date).to.equal(undefined) + + // Act. + setHeadersForCacheLength(res, 123) + + // Assert. + const now = new Date().toGMTString() + expect(res._headers.date).to.equal(now) + }) + + context('cacheLengthSeconds is zero', function() { + beforeEach(function() { + setHeadersForCacheLength(res, 0) + }) + + it('should set the expected Cache-Control header', function() { + expect(res._headers['cache-control']).to.equal( + 'no-cache, no-store, must-revalidate' + ) + }) + + it('should set the expected Expires header', function() { + expect(res._headers.expires).to.equal(new Date().toGMTString()) + }) + }) + + context('cacheLengthSeconds is nonzero', function() { + beforeEach(function() { + setHeadersForCacheLength(res, 123) + }) + + it('should set the expected Cache-Control header', function() { + expect(res._headers['cache-control']).to.equal('max-age=123') + }) + + it('should set the expected Expires header', function() { + const expires = new Date(Date.now() + 123 * 1000).toGMTString() + expect(res._headers.expires).to.equal(expires) + }) + }) + }) + + describe('setCacheHeaders', function() { + it('sets the expected fields', function() { + const expectedFields = ['date', 'cache-control', 'expires'] + expectedFields.forEach(field => + expect(res._headers[field]).to.equal(undefined) + ) + + setCacheHeaders({ + cacheHeaderConfig: { defaultCacheLengthSeconds: 1234 }, + serviceCacheLengthSeconds: 567, + queryParams: { maxAge: 999999 }, + res, + }) + + expectedFields.forEach(field => + expect(res._headers[field]) + .to.be.a('string') + .and.have.lengthOf.at.least(1) + ) + }) + }) + + describe('setCacheHeadersForStaticResource', function() { + beforeEach(function() { + setCacheHeadersForStaticResource(res) + }) + + it('should set the expected Cache-Control header', function() { + expect(res._headers['cache-control']).to.equal(`max-age=${24 * 3600}`) + }) + + it('should set the expected Last-Modified header', function() { + const lastModified = res._headers['last-modified'] + expect(new Date(lastModified)).to.be.withinTime( + // Within the last 60 seconds. + new Date(Date.now() - 60 * 1000), + new Date() + ) + }) + }) + + describe('serverHasBeenUpSinceResourceCached', function() { + // The stringified req's are hard to understand. I thought Sazerac + // provided a way to override the describe message, though I can't find it. + context('when there is no If-Modified-Since header', function() { + it('returns false', function() { + const req = httpMocks.createRequest() + expect(serverHasBeenUpSinceResourceCached(req)).to.equal(false) + }) + }) + context('when the If-Modified-Since header is invalid', function() { + it('returns false', function() { + const req = httpMocks.createRequest({ + headers: { 'If-Modified-Since': 'this-is-not-a-date' }, + }) + expect(serverHasBeenUpSinceResourceCached(req)).to.equal(false) + }) + }) + context( + 'when the If-Modified-Since header is before the process started', + function() { + it('returns false', function() { + const req = httpMocks.createRequest({ + headers: { 'If-Modified-Since': '2018-02-01T05:00:00.000Z' }, + }) + expect(serverHasBeenUpSinceResourceCached(req)).to.equal(false) + }) + } + ) + context( + 'when the If-Modified-Since header is after the process started', + function() { + it('returns true', function() { + const req = httpMocks.createRequest({ + headers: { 'If-Modified-Since': new Date().toGMTString() }, + }) + expect(serverHasBeenUpSinceResourceCached(req)).to.equal(true) + }) + } + ) + }) +}) diff --git a/services/flip/flip.service.js b/services/flip/flip.service.js new file mode 100644 index 0000000000..07bcb963cf --- /dev/null +++ b/services/flip/flip.service.js @@ -0,0 +1,33 @@ +'use strict' + +const NonMemoryCachingBaseService = require('../base-non-memory-caching') + +let bitFlip = false + +// Production cache debugging. +module.exports = class Flip extends NonMemoryCachingBaseService { + static get category() { + return 'debug' + } + + static get route() { + return { base: 'flip', pattern: '' } + } + + static get defaultBadgeData() { + return { label: 'flip' } + } + + static render({ bit }) { + if (bit) { + return { message: 'on', color: 'brightgreen' } + } else { + return { message: 'off', color: 'red' } + } + } + + handle() { + bitFlip = !bitFlip + return this.constructor.render({ bit: bitFlip }) + } +} diff --git a/services/flip/flip.tester.js b/services/flip/flip.tester.js new file mode 100644 index 0000000000..49250b1c2e --- /dev/null +++ b/services/flip/flip.tester.js @@ -0,0 +1,12 @@ +'use strict' + +const t = require('../create-service-tester')() +module.exports = t + +t.create('First request') + .get('.json') + .expectJSON({ name: 'flip', value: 'on' }) + +t.create('Second request') + .get('.json') + .expectJSON({ name: 'flip', value: 'off' }) diff --git a/services/legacy-service.js b/services/legacy-service.js index 9dbcddda8e..ebf6e3514e 100644 --- a/services/legacy-service.js +++ b/services/legacy-service.js @@ -8,13 +8,11 @@ class LegacyService extends BaseService { throw Error(`registerLegacyRouteHandler() not implemented for ${this.name}`) } - static register( - { camp, handleRequest: cache, githubApiProvider }, - serviceConfig - ) { + static register({ camp, handleRequest, githubApiProvider }, serviceConfig) { + const { cache: cacheHeaderConfig } = serviceConfig this.registerLegacyRouteHandler({ camp, - cache, + cache: (...args) => handleRequest(cacheHeaderConfig, ...args), githubApiProvider, }) } diff --git a/services/time/time.service.js b/services/time/time.service.js index e3c3e858e5..7638b98d37 100644 --- a/services/time/time.service.js +++ b/services/time/time.service.js @@ -1,8 +1,8 @@ 'use strict' -const BaseService = require('../base') +const NonMemoryCachingBaseService = require('../base-non-memory-caching') -module.exports = class Time extends BaseService { +module.exports = class Time extends NonMemoryCachingBaseService { async handle() { return { message: new Date() } } -- GitLab