TC-1020: Keep strategy directive around long enough for proper merge

lib/jsonmerger.js

@@ -48,7 +48,7 @@ module.exports = function jsonMerger(files, options) {
     // I am assuming here that _.each iterates in the same order as the object
     // was constructed. This appears to be valid, but is as of yet still
     // unconfirmed.
-    _.each(obj, (stage) => {
+    _.each(obj, stage => {
       // Don't append a merging strategy here, as this prevents a stage to
       // determine the merging strategy of a property in the default config
       _.merge(wrap(merged), wrap(stage.config));
@@ -68,17 +68,18 @@ module.exports = function jsonMerger(files, options) {
 
   // Returning `undefined` means merging is performed at the discretion of
   // `_.merge`.
-  function defaultStrategy(a, b) {
-    if (!_.isPlainObject(b)) {
+  function defaultStrategy(merged, single) {
+    if (!_.isPlainObject(single)) {
       // Cannot read a directive from this so just continue to merge natively
       return undefined;
     }
 
-    a = prepareInstances(a, b);
+    merged = prepareInstances(merged, single);
 
-    const strategy = getStrategy(b._strategy);
-    delete b._strategy;
-    return strategy(a, b);
+    const strategy = getStrategy(single._strategy);
+    const result = strategy(merged, single);
+
+    return result;
   }
 
   function getStrategy(strategy) {
@@ -103,39 +104,51 @@ module.exports = function jsonMerger(files, options) {
     }
   }
 
-  function prepareInstances(a, b) {
+  function prepareInstances(merged, single) {
     // Only act when we encounter an `instances` block
-    if (!(a && a._instances || b._instances)) {
-      return a;
+    if (!(merged && merged._instances || single._instances)) {
+      return merged;
     }
 
     // ensure we are working with comparable objects
-    if (!_.isPlainObject(a)) {
-      a = {};
+    if (!_.isPlainObject(merged)) {
+      merged = {};
     }
 
-    // Ensure a wildcard object is present in `a` for the initial merge
-    a[options.wildcard] = a[options.wildcard] || {};
+    // Ensure a wildcard object is present in `merged` for the initial merge
+    merged[options.wildcard] = merged[options.wildcard] || {};
 
-    // Merge each property of a with the wildcard value
-    a = _.mapValues(a, (value) => {
-      const wildcard = _.cloneDeep(b[options.wildcard] || {});
+    // Merge wildcard value of `single` in each property of `merged`. By doing this the wildcard of `single`
+    // is also merged into the wildcard of `merged`.
+    merged = _.mapValues(merged, value => {
+      const wildcard = _.cloneDeep(single[options.wildcard] || {});
       return unwrap(_.mergeWith(wrap(value), wrap(wildcard), options.strategy));
     });
 
-    // Initialize each new property with the previously merged wildcard value.
-    const newKeys = _.difference(_.keys(b), _.keys(a));
-    _.each(newKeys, (key) => {
-      a[key] = unwrap(_.mergeWith(wrap({}), wrap(a[options.wildcard]), options.strategy));
+    /**
+     * Initialize each property that is in `single`, but not in `merged` with the wildcard of `merged` (be aware:
+     * this wildcard has already been merged with the wildcard of `single` in the step above this one).
+     * *
+     * This initialization makes sure that each property of `single` uses the wildcard as "starting point" on which
+     * it's own properties will be merged.
+     */
+    const newKeys = _.difference(_.keys(single), _.keys(merged));
+    _.each(newKeys, key => {
+      merged[key] = unwrap(_.mergeWith(wrap({}), wrap(merged[options.wildcard]), options.strategy));
     });
 
-    // Ignore `b`'s wildcard in the upcoming merge since it has already been
-    // processed. This will still keep it available for future merges.
-    if (_.isPlainObject(b[options.wildcard])) {
-      b[options.wildcard]._strategy = 'ignore';
+    /**
+     * Ignore `single`'s wildcard in the upcoming merge since it has already been merged onto `merged`.
+     * *
+     * This ignore will still keep it available for future merges. For example, imagine having a config.js, projects.js
+     * and environments.js; after merging projects.js in config.js, the merged wildcard should still be
+     * the "starting point" upon which all properties of environments.js should be based.
+     */
+    if (_.isPlainObject(single[options.wildcard])) {
+      single[options.wildcard]._strategy = 'ignore';
     }
 
-    return a;
+    return merged;
   }
 
   function deleteStrategy() {
@@ -144,49 +157,51 @@ module.exports = function jsonMerger(files, options) {
     return null;
   }
 
-  function replaceStrategy(a, b) {
-    // Replace `a` by merging onto an empty object (this processes nested
-    // strategy directives)
-    return unwrap(_.mergeWith(wrap({}), wrap(b), options.strategy));
+  function replaceStrategy(merged, single) {
+    // Replace `merged` by merging onto an empty object (this processes nested
+    // strategy directives). Remove `single` strategy because it only applies for the current merge.
+    delete single._strategy;
+    return unwrap(_.mergeWith(wrap({}), wrap(single), options.strategy));
   }
 
-  function ignoreStrategy(a) {
-    // Ignore `b` by merging `a` with an empty object (this processes nested
-    // strategy directives)
-    return unwrap(_.mergeWith(wrap(a), wrap({}), options.strategy));
+  function ignoreStrategy(merged, single) {
+    // Ignore `single` by merging `merged` with an empty object (this processes nested
+    // strategy directives). Remove `single` strategy because it only applies for the current merge.
+    delete single._strategy;
+    return unwrap(_.mergeWith(wrap(merged), wrap({}), options.strategy));
   }
 
-  function concatenateArraysStrategy(a, b) {
-    return arrayStrategy(a, b, true);
+  function concatenateArraysStrategy(merged, single) {
+    return arrayStrategy(merged, single, true);
   }
 
-  function noConcatStrategy(a, b) {
-    return arrayStrategy(a, b, false);
+  function noConcatStrategy(merged, single) {
+    return arrayStrategy(merged, single, false);
   }
 
-  function arrayStrategy(a, b, concat) {
-    if (!_.isPlainObject(a)) {
+  function arrayStrategy(merged, single, concat) {
+    if (!_.isPlainObject(merged)) {
       // a is not an object and will therefore be replaced
-      return replaceStrategy(a, b);
+      return replaceStrategy(merged, single);
     }
-    _.each(b, (value, key) => {
-      if (_.isArray(a[key]) && _.isArray(value)) {
+    _.each(single, (value, key) => {
+      if (_.isArray(merged[key]) && _.isArray(value)) {
         // Concatenate or replace arrays
-        a[key] = concat ? a[key].concat(value) : value;
+        merged[key] = concat ? merged[key].concat(value) : value;
         return;
       }
       if (_.isPlainObject(value)) {
         // Recursively merge nested object
         const obj = {};
         obj[key] = value;
-        // assign the value because `a` may not have been an object.
-        a = _.mergeWith(a, obj, options.strategy);
+        // assign the value because `merged` may not have been an object.
+        merged = _.mergeWith(merged, obj, options.strategy);
         return;
       }
       // Assign primitive value
-      a[key] = value;
+      merged[key] = value;
     });
-    return a;
+    return merged;
   }
 
   /*
@@ -232,9 +247,16 @@ module.exports = function jsonMerger(files, options) {
       if (node === null && options.deleteNullValues) {
         return this.remove();
       }
-      if (_.isPlainObject(node) && node._instances) {
-        delete node._instances;
-        delete node[options.wildcard];
+      if (_.isPlainObject(node)) {
+        if (node._instances) {
+          delete node._instances;
+          delete node[options.wildcard];
+        }
+
+        if (node._strategy) {
+          delete node._strategy;
+        }
+
         this.update(node);
       }
     }));

package.json

@@ -27,16 +27,16 @@
   "homepage": "https://github.com/bsander/jsonmerger",
   "dependencies": {
     "djson": "^2.0.2",
-    "lodash": "^4.1.0",
+    "lodash": "^4.17.4",
     "traverse": "^0.6.6"
   },
   "devDependencies": {
-    "chai": "^3.4.1",
-    "eslint": "^3.17.1",
-    "eslint-config-mm": "^1.1.1",
-    "mocha": "^2.0.1",
-    "proxyquire": "^1.0.1",
-    "sinon": "^1.11.0",
-    "sinon-chai": "^2.6.0"
+    "chai": "^3.5.0",
+    "eslint": "^3.19.0",
+    "eslint-config-mm": "^1.2.0",
+    "mocha": "^3.2.0",
+    "proxyquire": "^1.7.11",
+    "sinon": "^2.1.0",
+    "sinon-chai": "^2.9.0"
   }
 }

test/jsonmerger.spec.js

@@ -151,6 +151,46 @@ describe('jsonMerger', () => {
       expect(result.get).to.be.a.function;
       expect(result.set).to.be.a.function;
     });
+    it('should properly merge arrays from nested wildcards', () => {
+      icin = {
+        level1: {
+          _instances: true,
+          '*': {
+            level2: {
+              _instances: true,
+              '*': {
+                array: [
+                  'one',
+                ]
+              },
+              specific2: {
+                _strategy: 'noconcat',
+                array: [
+                  'two',
+                ]
+              }
+            }
+          },
+          specific1: {
+            level2: {
+              specific2: {
+                array: [
+                  'three'
+                ]
+              }
+            }
+          }
+        }
+      };
+      jsonMerger = proxyquire('../lib/jsonmerger', {
+        os: os,
+        defaults: dcin,
+        instances: icin,
+        environments: ecin
+      });
+      result = jsonMerger(['instances']);
+      expect(result.level1.specific1.level2.specific2.array).to.deep.equal(['two', 'three']);
+    });
   });
   describe('with two (or more) regular config files', () => {
     const files = ['defaults', 'instances'];
@@ -246,6 +286,50 @@ describe('jsonMerger', () => {
         }
       });
     });
+    it('should properly merge arrays from nested wildcards when instances are configured in the default', () => {
+      dcin = {
+        level1: {
+          _instances: true,
+          '*': {
+            level2: {
+              _instances: true
+            }
+          }
+        }
+      };
+      icin = {
+        level1: {
+          '*': {
+            level2: {
+              '*': {
+                array: [
+                  'one',
+                ]
+              },
+              specific2: {
+                _strategy: 'noconcat',
+                array: [
+                  'two',
+                ]
+              }
+            }
+          },
+          specific1: {
+            level2: {
+              specific2: {}
+            }
+          }
+        }
+      };
+      jsonMerger = proxyquire('../lib/jsonmerger', {
+        os: os,
+        defaults: dcin,
+        instances: icin,
+        environments: ecin
+      });
+      result = jsonMerger(files);
+      expect(result.level1.specific1.level2.specific2.array).to.deep.equal(['two']);
+    });
     it('should drop the `*` instance from the config', () => {
       result = jsonMerger(files);
       expect(Object.keys(result.instances)).to.have.length(1);