[config] transform plugin deprecations before checking for unused set…

…tings (#21294) * transform plugin deprecations before checking for unused settings * async deprecations provider * refactor * assign * async tests
elastic · Nov 1, 2018 · d451d6b · d451d6b
1 parent 2e590a8
commit d451d6b
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 42 deletions.
diff --git a/src/deprecation/get_transform.js b/src/deprecation/get_transform.js
@@ -0,0 +1,30 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { noop } from 'lodash';
+
+import { createTransform } from './create_transform';
+import { rename, unused } from './deprecations';
+
+export async function getTransform(spec) {
+  const deprecationsProvider = spec.getDeprecationsProvider() || noop;
+  if (!deprecationsProvider) return;
+  const transforms = await deprecationsProvider({ rename, unused }) || [];
+  return createTransform(transforms);
+}
diff --git a/src/deprecation/index.js b/src/deprecation/index.js
@@ -20,4 +20,5 @@
 import { rename, unused } from './deprecations';
 
 export { createTransform } from './create_transform';
+export { getTransform } from './get_transform';
 export const Deprecations = { rename, unused };
diff --git a/src/plugin_discovery/plugin_config/settings.js b/src/plugin_discovery/plugin_config/settings.js
@@ -17,15 +17,10 @@
  * under the License.
  */
 
-import { get, noop } from 'lodash';
+import { get } from 'lodash';
 
 import * as serverConfig from '../../server/config';
-import { createTransform, Deprecations } from '../../deprecation';
-
-async function getDeprecationTransformer(spec) {
-  const provider = spec.getDeprecationsProvider() || noop;
-  return createTransform(await provider(Deprecations) || []);
-}
+import { getTransform } from '../../deprecation';
 
 /**
  *  Get the settings for a pluginSpec from the raw root settings while
@@ -38,7 +33,7 @@ async function getDeprecationTransformer(spec) {
  */
 export async function getSettings(spec, rootSettings, logDeprecation) {
   const prefix = spec.getConfigPrefix();
-  const transformer = await getDeprecationTransformer(spec);
   const rawSettings = get(serverConfig.transformDeprecations(rootSettings), prefix);
-  return transformer(rawSettings, logDeprecation);
+  const transform = await getTransform(spec);
+  return transform(rawSettings, logDeprecation);
 }
diff --git a/src/server/config/complete.js b/src/server/config/complete.js
@@ -20,13 +20,20 @@
 import { difference } from 'lodash';
 import { transformDeprecations } from './transform_deprecations';
 import { unset, formatListAsProse, getFlattenedObject } from '../../utils';
+import { getTransform } from '../../deprecation';
+
 
 const getFlattenedKeys = object => (
   Object.keys(getFlattenedObject(object))
 );
 
-const getUnusedConfigKeys = (disabledPluginSpecs, rawSettings, configValues) => {
-  const settings = transformDeprecations(rawSettings);
+async function getUnusedConfigKeys(plugins, disabledPluginSpecs, rawSettings, configValues) {
+  // transform deprecated settings
+  const transforms = [
+    transformDeprecations,
+    ...await Promise.all(plugins.map(({ spec }) => getTransform(spec)))
+  ];
+  const settings = transforms.reduce((a, c) => c(a), rawSettings);
 
   // remove config values from disabled plugins
   for (const spec of disabledPluginSpecs) {
@@ -44,27 +51,32 @@ const getUnusedConfigKeys = (disabledPluginSpecs, rawSettings, configValues) =>
   }
 
   return difference(inputKeys, appliedKeys);
-};
+}
 
-export default function (kbnServer, server, config) {
+export default async function (kbnServer, server, config) {
 
   server.decorate('server', 'config', function () {
     return kbnServer.config;
   });
 
-  const unusedKeys = getUnusedConfigKeys(kbnServer.disabledPluginSpecs, kbnServer.settings, config.get())
-    .map(key => `"${key}"`);
+  const unusedKeys = await getUnusedConfigKeys(
+    kbnServer.plugins,
+    kbnServer.disabledPluginSpecs,
+    kbnServer.settings,
+    config.get()
+  );
 
   if (!unusedKeys.length) {
     return;
   }
 
-  const desc = unusedKeys.length === 1
+  const formattedUnusedKeys = unusedKeys.map(key => `"${key}"`);
+  const desc = formattedUnusedKeys.length === 1
     ? 'setting was'
     : 'settings were';
 
   const error = new Error(
-    `${formatListAsProse(unusedKeys)} ${desc} not applied. ` +
+    `${formatListAsProse(formattedUnusedKeys)} ${desc} not applied. ` +
     'Check for spelling errors and ensure that expected ' +
     'plugins are installed.'
   );

diff --git a/src/server/config/complete.test.js b/src/server/config/complete.test.js
@@ -34,6 +34,7 @@ describe('server/config completeMixin()', function () {
       settings = {},
       configValues = {},
       disabledPluginSpecs = [],
+      plugins = [],
     } = options;
 
     const server = {
@@ -48,32 +49,31 @@ describe('server/config completeMixin()', function () {
       settings,
       server,
       config,
-      disabledPluginSpecs
+      disabledPluginSpecs,
+      plugins
     };
 
-    const callCompleteMixin = () => {
-      completeMixin(kbnServer, server, config);
-    };
+    const callCompleteMixin = () => completeMixin(kbnServer, server, config);
 
     return { config, callCompleteMixin, server };
   };
 
   describe('server decoration', () => {
-    it('adds a config() function to the server', () => {
+    it('adds a config() function to the server', async () => {
       const { config, callCompleteMixin, server } = setup({
         settings: {},
         configValues: {}
       });
 
-      callCompleteMixin();
+      await callCompleteMixin();
       sinon.assert.calledOnce(server.decorate);
       sinon.assert.calledWith(server.decorate, 'server', 'config', sinon.match.func);
       expect(server.decorate.firstCall.args[2]()).toBe(config);
     });
   });
 
   describe('all settings used', () => {
-    it('should not throw', function () {
+    it('should not throw', async function () {
       const { callCompleteMixin } = setup({
         settings: {
           used: true
@@ -83,11 +83,11 @@ describe('server/config completeMixin()', function () {
         },
       });
 
-      callCompleteMixin();
+      await expect(callCompleteMixin()).resolves.toBe(undefined);
     });
 
     describe('more config values than settings', () => {
-      it('should not throw', function () {
+      it('should not throw', async function () {
         const { callCompleteMixin } = setup({
           settings: {
             used: true
@@ -98,13 +98,13 @@ describe('server/config completeMixin()', function () {
           }
         });
 
-        callCompleteMixin();
+        await expect(callCompleteMixin()).resolves.toBe(undefined);
       });
     });
   });
 
   describe('env setting specified', () => {
-    it('should not throw', () => {
+    it('should not throw', async () => {
       const { callCompleteMixin } = setup({
         settings: {
           env: 'development'
@@ -116,12 +116,12 @@ describe('server/config completeMixin()', function () {
         }
       });
 
-      callCompleteMixin();
+      await expect(callCompleteMixin()).resolves.toBe(undefined);
     });
   });
 
   describe('settings include non-default array settings', () => {
-    it('should not throw', () => {
+    it('should not throw', async () => {
       const { callCompleteMixin } = setup({
         settings: {
           foo: [
@@ -134,12 +134,13 @@ describe('server/config completeMixin()', function () {
         }
       });
 
-      callCompleteMixin();
+      await expect(callCompleteMixin()).resolves.toBe(undefined);
     });
   });
 
   describe('some settings unused', () => {
-    it('should throw an error', function () {
+    it('should throw an error', async function () {
+      expect.assertions(1);
       const { callCompleteMixin } = setup({
         settings: {
           unused: true
@@ -148,12 +149,15 @@ describe('server/config completeMixin()', function () {
           used: true
         }
       });
-
-      expect(callCompleteMixin).toThrowError('"unused" setting was not applied');
+      try {
+        await callCompleteMixin();
+      } catch(error) {
+        expect(error.message).toMatch('"unused" setting was not applied');
+      }
     });
 
     describe('error thrown', () => {
-      it('has correct code, processExitCode, and message', () => {
+      it('has correct code, processExitCode, and message', async () => {
         expect.assertions(3);
 
         const { callCompleteMixin } = setup({
@@ -171,7 +175,7 @@ describe('server/config completeMixin()', function () {
         });
 
         try {
-          callCompleteMixin();
+          await callCompleteMixin();
         } catch (error) {
           expect(error).toHaveProperty('code', 'InvalidConfig');
           expect(error).toHaveProperty('processExitCode', 64);
@@ -182,7 +186,7 @@ describe('server/config completeMixin()', function () {
   });
 
   describe('deprecation support', () => {
-    it('should transform settings when determining what is unused', function () {
+    it('should transform settings when determining what is unused', async function () {
       sandbox.spy(transformDeprecationsNS, 'transformDeprecations');
 
       const settings = {
@@ -196,12 +200,12 @@ describe('server/config completeMixin()', function () {
         }
       });
 
-      callCompleteMixin();
+      await callCompleteMixin();
       sinon.assert.calledOnce(transformDeprecations);
       sinon.assert.calledWithExactly(transformDeprecations, settings);
     });
 
-    it('should use transformed settings when considering what is used', function () {
+    it('should use transformed settings when considering what is used', async function () {
       sandbox.stub(transformDeprecationsNS, 'transformDeprecations').callsFake((settings) => {
         settings.bar = settings.foo;
         delete settings.foo;
@@ -217,13 +221,35 @@ describe('server/config completeMixin()', function () {
         }
       });
 
-      callCompleteMixin();
+      await callCompleteMixin();
       sinon.assert.calledOnce(transformDeprecations);
     });
+
+    it('should transform deprecated plugin settings', async () => {
+      const { callCompleteMixin } = setup({
+        settings: {
+          foo1: 'bar'
+        },
+        configValues: {
+          foo2: 'bar'
+        },
+        plugins: [
+          {
+            spec: {
+              getDeprecationsProvider() {
+                return async ({ rename }) => [rename('foo1', 'foo2')];
+              }
+            }
+          }
+        ],
+      });
+
+      await expect(callCompleteMixin()).resolves.toBe(undefined);
+    });
   });
 
   describe('disabled plugins', () => {
-    it('ignores config for plugins that are disabled', () => {
+    it('ignores config for plugins that are disabled', async () => {
       const { callCompleteMixin } = setup({
         settings: {
           foo: {
@@ -241,7 +267,7 @@ describe('server/config completeMixin()', function () {
         configValues: {}
       });
 
-      expect(callCompleteMixin).not.toThrowError();
+      await expect(callCompleteMixin()).resolves.toBe(undefined);
     });
   });
 });