From 07390981f473a48e9eb4410aff24d551c498aab9 Mon Sep 17 00:00:00 2001 From: spalger Date: Thu, 6 Jul 2017 16:41:41 -0700 Subject: [PATCH 1/3] [kbnServer/extensions] add server.addMemoizedFactoryToRequest() helper --- src/server/kbn_server.js | 3 + .../add_memoized_factory_to_request.js | 101 ++++++++++++++++++ src/server/server_extensions/index.js | 1 + .../server_extensions_mixin.js | 44 ++++++++ 4 files changed, 149 insertions(+) create mode 100644 src/server/server_extensions/__tests__/add_memoized_factory_to_request.js create mode 100644 src/server/server_extensions/index.js create mode 100644 src/server/server_extensions/server_extensions_mixin.js diff --git a/src/server/kbn_server.js b/src/server/kbn_server.js index 570b2fdec4a1f..5e5a5121cc11e 100644 --- a/src/server/kbn_server.js +++ b/src/server/kbn_server.js @@ -21,6 +21,7 @@ import pluginsInitializeMixin from './plugins/initialize'; import { indexPatternsMixin } from './index_patterns'; import { savedObjectsMixin } from './saved_objects'; import { statsMixin } from './stats'; +import { serverExtensionsMixin } from './server_extensions'; const rootDir = fromRoot('.'); @@ -37,6 +38,8 @@ export default class KbnServer { configSetupMixin, // sets this.server httpMixin, + // adds methods for extending this.server + serverExtensionsMixin, loggingMixin, warningsMixin, statusMixin, diff --git a/src/server/server_extensions/__tests__/add_memoized_factory_to_request.js b/src/server/server_extensions/__tests__/add_memoized_factory_to_request.js new file mode 100644 index 0000000000000..5d3750bdfe7b2 --- /dev/null +++ b/src/server/server_extensions/__tests__/add_memoized_factory_to_request.js @@ -0,0 +1,101 @@ +import sinon from 'sinon'; +import expect from 'expect.js'; + +import { serverExtensionsMixin } from '../server_extensions_mixin'; + +describe('server.addMemoizedFactoryToRequest()', () => { + const setup = () => { + class Request {} + + class Server { + constructor() { + sinon.spy(this, 'decorate'); + } + decorate(type, name, value) { + switch (type) { + case 'request': return Request.prototype[name] = value; + case 'server': return Server.prototype[name] = value; + default: throw new Error(`Unexpected decorate type ${type}`); + } + } + } + + const server = new Server(); + serverExtensionsMixin({}, server); + return { server, Request }; + }; + + it('throws when propertyName is not a string', () => { + const { server } = setup(); + expect(() => server.addMemoizedFactoryToRequest()).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest(null)).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest(1)).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest(true)).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest(/abc/)).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest(['foo'])).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest([1])).to.throwError('propertyName must be a string'); + expect(() => server.addMemoizedFactoryToRequest({})).to.throwError('propertyName must be a string'); + }); + + it('throws when factory is not a function', () => { + const { server } = setup(); + expect(() => server.addMemoizedFactoryToRequest('name')).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', null)).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', 1)).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', true)).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', /abc/)).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', ['foo'])).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', [1])).to.throwError('factory must be a function'); + expect(() => server.addMemoizedFactoryToRequest('name', {})).to.throwError('factory must be a function'); + }); + + it('throws when factory takes more than one arg', () => { + const { server } = setup(); + /* eslint-disable no-unused-vars */ + expect(() => server.addMemoizedFactoryToRequest('name', () => {})).to.not.throwError('more than one argument'); + expect(() => server.addMemoizedFactoryToRequest('name', (a) => {})).to.not.throwError('more than one argument'); + + expect(() => server.addMemoizedFactoryToRequest('name', (a, b) => {})).to.throwError('more than one argument'); + expect(() => server.addMemoizedFactoryToRequest('name', (a, b, c) => {})).to.throwError('more than one argument'); + expect(() => server.addMemoizedFactoryToRequest('name', (a, b, c, d) => {})).to.throwError('more than one argument'); + expect(() => server.addMemoizedFactoryToRequest('name', (a, b, c, d, e) => {})).to.throwError('more than one argument'); + /* eslint-enable no-unused-vars */ + }); + + it('decorates request objects with a function at `propertyName`', () => { + const { server, Request } = setup(); + + expect(new Request()).to.not.have.property('decorated'); + server.addMemoizedFactoryToRequest('decorated', () => {}); + expect(new Request()).to.have.property('decorated').a('function'); + }); + + it('caches invocations of the factory to the request instance', () => { + const { server, Request } = setup(); + const factory = sinon.stub().returnsArg(0); + server.addMemoizedFactoryToRequest('foo', factory); + + const request1 = new Request(); + const request2 = new Request(); + + // call `foo()` on both requests a bunch of times, each time + // the return value should be exactly the same + expect(request1.foo()).to.be(request1); + expect(request1.foo()).to.be(request1); + expect(request1.foo()).to.be(request1); + expect(request1.foo()).to.be(request1); + expect(request1.foo()).to.be(request1); + expect(request1.foo()).to.be(request1); + + expect(request2.foo()).to.be(request2); + expect(request2.foo()).to.be(request2); + expect(request2.foo()).to.be(request2); + expect(request2.foo()).to.be(request2); + + // only two different requests, so factory should have only been + // called twice with the two request objects + sinon.assert.calledTwice(factory); + sinon.assert.calledWithExactly(factory, request1); + sinon.assert.calledWithExactly(factory, request2); + }); +}); diff --git a/src/server/server_extensions/index.js b/src/server/server_extensions/index.js new file mode 100644 index 0000000000000..af5dedc384df8 --- /dev/null +++ b/src/server/server_extensions/index.js @@ -0,0 +1 @@ +export { serverExtensionsMixin } from './server_extensions_mixin'; diff --git a/src/server/server_extensions/server_extensions_mixin.js b/src/server/server_extensions/server_extensions_mixin.js new file mode 100644 index 0000000000000..49d306b00313e --- /dev/null +++ b/src/server/server_extensions/server_extensions_mixin.js @@ -0,0 +1,44 @@ + +export function serverExtensionsMixin(kbnServer, server) { + /** + * Decorate all request objects with a new method, `methodName`, + * that will call the `factory` on first invocation and return + * the result of the first call to subsequent invocations. + * + * @method server.addMemoizedFactoryToRequest + * @param {string} methodName location on the request this + * factory should be added + * @param {Function} factory the factory to add to the request, + * which will be called once per request + * with a single argument, the request. + * @return {undefined} + */ + server.decorate('server', 'addMemoizedFactoryToRequest', (methodName, factory) => { + if (typeof methodName !== 'string') { + throw new TypeError('methodName must be a string'); + } + + if (typeof factory !== 'function') { + throw new TypeError('factory must be a function'); + } + + if (factory.length > 1) { + throw new TypeError(` + factory must not take more than one argument, the request object. + Memoization is done based on the request instance and is cached and reused + regardless of other arguments. If you want to have a per-request cache that + also does some sort of secondary memoization then return an object or function + from the memoized decorator and do secordary memoization there. + `); + } + + const requestCache = new WeakMap(); + server.decorate('request', methodName, function () { + if (!requestCache.has(this)) { + requestCache.set(this, factory(this)); + } + + return requestCache.get(this); + }); + }); +} From 6c71806d661ff1fd4e643efe64ff5961a4ec3204 Mon Sep 17 00:00:00 2001 From: spalger Date: Thu, 6 Jul 2017 16:44:01 -0700 Subject: [PATCH 2/3] [server/uiSettings] use server.addMemoizedFactoryToRequest() helper --- .../ui_settings/__tests__/ui_settings_mixin_integration.js | 5 +++++ src/ui/ui_settings/ui_settings_mixin.js | 4 ++-- src/ui/ui_settings/ui_settings_service_for_request.js | 7 ------- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js b/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js index a81d7d6d8d3a3..cdc7a2b48ebcd 100644 --- a/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js +++ b/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js @@ -38,6 +38,11 @@ describe('uiSettingsMixin()', () => { const server = { log: sinon.stub(), config: () => config, + addMemoizedFactoryToRequest(name, factory) { + this.decorate('request', name, function () { + return factory(this); + }); + }, decorate: sinon.spy((type, name, value) => { decorations[type][name] = value; }), diff --git a/src/ui/ui_settings/ui_settings_mixin.js b/src/ui/ui_settings/ui_settings_mixin.js index 62b59db991542..9c8468e759560 100644 --- a/src/ui/ui_settings/ui_settings_mixin.js +++ b/src/ui/ui_settings/ui_settings_mixin.js @@ -44,8 +44,8 @@ export function uiSettingsMixin(kbnServer, server, config) { }); }); - server.decorate('request', 'getUiSettingsService', function () { - return getUiSettingsServiceForRequest(server, this, { + server.addMemoizedFactoryToRequest('getUiSettingsService', request => { + return getUiSettingsServiceForRequest(server, request, { getDefaults, readInterceptor, }); diff --git a/src/ui/ui_settings/ui_settings_service_for_request.js b/src/ui/ui_settings/ui_settings_service_for_request.js index d43657b6d8f42..434c94f07a9a7 100644 --- a/src/ui/ui_settings/ui_settings_service_for_request.js +++ b/src/ui/ui_settings/ui_settings_service_for_request.js @@ -1,7 +1,5 @@ import { uiSettingsServiceFactory } from './ui_settings_service_factory'; -const BY_REQUEST_CACHE = new WeakMap(); - /** * Get/create an instance of UiSettingsService bound to a specific request. * Each call is cached (keyed on the request object itself) and subsequent @@ -19,10 +17,6 @@ const BY_REQUEST_CACHE = new WeakMap(); * @return {UiSettingsService} */ export function getUiSettingsServiceForRequest(server, request, options = {}) { - if (BY_REQUEST_CACHE.has(request)) { - return BY_REQUEST_CACHE.get(request); - } - const { readInterceptor, getDefaults @@ -37,6 +31,5 @@ export function getUiSettingsServiceForRequest(server, request, options = {}) { } }); - BY_REQUEST_CACHE.set(request, uiSettingsService); return uiSettingsService; } From 109a0c8852635227f81926eacaba3ff900c22dae Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 10 Jul 2017 12:23:11 -0700 Subject: [PATCH 3/3] [kbnServer/extensions] name `this` for clarity --- src/server/server_extensions/server_extensions_mixin.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/server/server_extensions/server_extensions_mixin.js b/src/server/server_extensions/server_extensions_mixin.js index 49d306b00313e..8a962bc3611aa 100644 --- a/src/server/server_extensions/server_extensions_mixin.js +++ b/src/server/server_extensions/server_extensions_mixin.js @@ -34,11 +34,13 @@ export function serverExtensionsMixin(kbnServer, server) { const requestCache = new WeakMap(); server.decorate('request', methodName, function () { - if (!requestCache.has(this)) { - requestCache.set(this, factory(this)); + const request = this; + + if (!requestCache.has(request)) { + requestCache.set(request, factory(request)); } - return requestCache.get(this); + return requestCache.get(request); }); }); }