Cleanup mergeOptions() (#3280)

* Fix mergeOptions() and add unit tests

* Removed comment

lib/network/modules/components/Edge.js

@@ -129,8 +129,8 @@ class Edge {
     // only deep extend the items in the field array. These do not have shorthand.
     util.selectiveDeepExtend(fields, parentOptions, newOptions, allowDeletion);
 
-    util.mergeOptions(parentOptions, newOptions, 'smooth', allowDeletion, globalOptions);
-    util.mergeOptions(parentOptions, newOptions, 'shadow', allowDeletion, globalOptions);
+    util.mergeOptions(parentOptions, newOptions, 'smooth', globalOptions);
+    util.mergeOptions(parentOptions, newOptions, 'shadow', globalOptions);
 
     if (newOptions.dashes !== undefined && newOptions.dashes !== null) {
       parentOptions.dashes = newOptions.dashes;
@@ -143,7 +143,7 @@ class Edge {
     if (newOptions.scaling !== undefined && newOptions.scaling !== null) {
       if (newOptions.scaling.min !== undefined) {parentOptions.scaling.min = newOptions.scaling.min;}
       if (newOptions.scaling.max !== undefined) {parentOptions.scaling.max = newOptions.scaling.max;}
-      util.mergeOptions(parentOptions.scaling, newOptions.scaling, 'label', allowDeletion, globalOptions.scaling);
+      util.mergeOptions(parentOptions.scaling, newOptions.scaling, 'label', globalOptions.scaling);
     }
     else if (allowDeletion === true && newOptions.scaling === null) {
       parentOptions.scaling = Object.create(globalOptions.scaling); // this sets the pointer of the option back to the global option.
@@ -158,9 +158,9 @@ class Edge {
         parentOptions.arrows.from.enabled   = arrows.indexOf("from")   != -1;
       }
       else if (typeof newOptions.arrows === 'object') {
-        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'to',     allowDeletion, globalOptions.arrows);
-        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'middle', allowDeletion, globalOptions.arrows);
-        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'from',   allowDeletion, globalOptions.arrows);
+        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'to',     globalOptions.arrows);
+        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'middle', globalOptions.arrows);
+        util.mergeOptions(parentOptions.arrows, newOptions.arrows, 'from',   globalOptions.arrows);
       }
       else {
         throw new Error("The arrow newOptions can only be an object or a string. Refer to the documentation. You used:" + JSON.stringify(newOptions.arrows));

lib/network/modules/components/Node.js

@@ -215,7 +215,7 @@ class Node {
     Node.checkMass(newOptions);
 
     // merge the shadow options into the parent.
-    util.mergeOptions(parentOptions, newOptions, 'shadow', allowDeletion, globalOptions);
+    util.mergeOptions(parentOptions, newOptions, 'shadow', globalOptions);
 
     // individual shape newOptions
     if (newOptions.color !== undefined && newOptions.color !== null) {
@@ -252,7 +252,7 @@ class Node {
 
     // handle the scaling options, specifically the label part
     if (newOptions.scaling !== undefined) {
-      util.mergeOptions(parentOptions.scaling, newOptions.scaling, 'label', allowDeletion, globalOptions.scaling);
+      util.mergeOptions(parentOptions.scaling, newOptions.scaling, 'label', globalOptions.scaling);
     }
   }
 

lib/util.js

@@ -1276,40 +1276,128 @@ exports.insertSort = function (a,compare) {
   return a;
 }
 
+
 /**
- * this is used to set the options of subobjects in the options object. A requirement of these subobjects
- * is that they have an 'enabled' element which is optional for the user but mandatory for the program.
+ * This is used to set the options of subobjects in the options object.
+ *
+ * A requirement of these subobjects is that they have an 'enabled' element
+ * which is optional for the user but mandatory for the program.
+ *
+ * The added value here of the merge is that option 'enabled' is set as required.
+ *
  *
- * @param [object] mergeTarget | this is either this.options or the options used for the groups.
- * @param [object] options     | options
- * @param [String] option      | this is the option key in the options argument
+ * @param {object} mergeTarget   | either this.options or the options used for the groups.
+ * @param {object} options       | options
+ * @param {String} option        | option key in the options argument
+ * @param {object} globalOptions | global options, passed in to determine value of option 'enabled'
  */
-exports.mergeOptions = function (mergeTarget, options, option, allowDeletion = false, globalOptions = {}) {
-  if (options[option] === null) {
-    mergeTarget[option] = Object.create(globalOptions[option]);
+exports.mergeOptions = function (mergeTarget, options, option, globalOptions = {}) {
+  // Local helpers
+  var isPresent = function(obj) {
+    return obj !== null && obj !== undefined;
   }
-  else {
-    if (options[option] !== undefined) {
-      if (typeof options[option] === 'boolean') {
-        mergeTarget[option].enabled = options[option];
-      }
-      else {
-        if (options[option].enabled === undefined) {
-          if (globalOptions[option] !== undefined  && globalOptions[option].enabled !== undefined) {
-            mergeTarget[option].enabled = globalOptions[option].enabled;
-          } else {
-            // assume this is the correct value
-            mergeTarget[option].enabled = true;
-          }
-        }
-        for (var prop in options[option]) {
-          if (options[option].hasOwnProperty(prop)) {
-            mergeTarget[option][prop] = options[option][prop];
-          }
-        }
+
+  var isObject = function(obj) {
+    return obj !== null && typeof obj === 'object';
+  }
+
+  // https://stackoverflow.com/a/34491287/1223531
+  var isEmpty = function(obj) {
+    for (var x in obj) { if (obj.hasOwnProperty(x)) return false; }
+    return true;
+  };
+
+  // Guards
+  if (!isObject(mergeTarget)) {
+    throw new Error('Parameter mergeTarget must be an object');
+  }
+
+  if (!isObject(options)) {
+    throw new Error('Parameter options must be an object');
+  }
+
+  if (!isPresent(option)) {
+    throw new Error('Parameter option must have a value');
+  }
+
+  if (!isObject(globalOptions)) {
+    throw new Error('Parameter globalOptions must be an object');
+  }
+
+
+  //
+  // Actual merge routine, separated from main logic
+  // Only a single level of options is merged. Deeper levels are ref'd. This may actually be an issue.
+  //
+  var doMerge = function(target, options, option) {
+    if (!isObject(target[option])) {
+      target[option] = {};
+    }
+
+    let src = options[option];
+    let dst = target[option];
+    for (var prop in src) {
+      if (src.hasOwnProperty(prop)) {
+        dst[prop] = src[prop];
       }
     }
+  };
+
+
+  // Local initialization
+  var srcOption     = options[option];
+  var globalPassed  = isObject(globalOptions) && !isEmpty(globalOptions);
+  var globalOption  = globalPassed? globalOptions[option]: undefined;
+  var globalEnabled = globalOption? globalOption.enabled: undefined;
+
+
+  /////////////////////////////////////////
+  // Main routine
+  /////////////////////////////////////////
+  if (srcOption === undefined) {
+    return;  // Nothing to do
+  }
+
+
+  if ((typeof srcOption) === 'boolean') {
+    if (!isObject(mergeTarget[option])) {
+      mergeTarget[option] = {};
+    }
+
+    mergeTarget[option].enabled = srcOption;
+    return;
+  } 
+
+  if (srcOption === null && !isObject(mergeTarget[option])) {
+    // If possible, explicit copy from globals
+    if (isPresent(globalOption)) {
+      mergeTarget[option] = Object.create(globalOption);
+    } else {
+      return;  // Nothing to do
+    }
+  }
+
+  if (!isObject(srcOption)) {
+    return;
   }
+
+  //
+  // Ensure that 'enabled' is properly set. It is required internally
+  // Note that the value from options will always overwrite the existing value
+  //
+  let enabled = true;  // default value
+
+  if (srcOption.enabled !== undefined) {
+    enabled = srcOption.enabled;
+  } else {
+    // Take from globals, if present
+    if (globalEnabled !== undefined) {
+      enabled = globalOption.enabled;
+    }
+  }
+
+  doMerge(mergeTarget, options, option);
+  mergeTarget[option].enabled = enabled;
 }
 
 

test/util.test.js

@@ -0,0 +1,134 @@
+var assert = require('assert');
+var util = require('../lib/util');
+
+
+//
+// The important thing with mergeOptions() is that 'enabled' is always set in target option.
+//
+describe('util.mergeOptions', function () {
+
+  it('handles good input without global options', function () {
+    var options = {
+      someValue: "silly value",
+      aBoolOption: false,
+      anObject: {
+        answer:42
+      },
+      anotherObject: {
+        enabled: false,
+      },
+      merge: null
+    };
+
+    // Case with empty target
+    var mergeTarget  = {};
+
+    util.mergeOptions(mergeTarget, options, 'someValue');
+    assert(mergeTarget.someValue === undefined, 'Non-object option should not be copied');
+    assert(mergeTarget.anObject === undefined);
+
+    util.mergeOptions(mergeTarget, options, 'aBoolOption');
+    assert(mergeTarget.aBoolOption !== undefined, 'option aBoolOption should now be an object');
+    assert(mergeTarget.aBoolOption.enabled === false, 'enabled value option aBoolOption should have been copied into object');
+
+    util.mergeOptions(mergeTarget, options, 'anObject');
+    assert(mergeTarget.anObject !== undefined, 'Option object is not copied');
+    assert(mergeTarget.anObject.answer === 42);
+    assert(mergeTarget.anObject.enabled === true);
+
+    util.mergeOptions(mergeTarget, options, 'anotherObject');
+    assert(mergeTarget.anotherObject.enabled === false, 'enabled value from options must have priority');
+
+    util.mergeOptions(mergeTarget, options, 'merge');
+    assert(mergeTarget.merge === undefined, 'Explicit null option should not be copied, there is no global option for it');
+
+    // Case with non-empty target
+    mergeTarget  = {
+      someValue: false,
+      aBoolOption: true,
+      anObject: {
+        answer: 49
+      },
+      anotherObject: {
+        enabled: true,
+      },
+      merge: 'hello'
+    };
+
+    util.mergeOptions(mergeTarget, options, 'someValue');
+    assert(mergeTarget.someValue === false, 'Non-object option should not be copied');
+    assert(mergeTarget.anObject.answer === 49, 'Sibling option should not be changed');
+
+    util.mergeOptions(mergeTarget, options, 'aBoolOption');
+    assert(mergeTarget.aBoolOption !== true, 'option enabled should have been overwritten');
+    assert(mergeTarget.aBoolOption.enabled === false, 'enabled value option aBoolOption should have been copied into object');
+
+    util.mergeOptions(mergeTarget, options, 'anObject');
+    assert(mergeTarget.anObject.answer === 42);
+    assert(mergeTarget.anObject.enabled === true);
+
+    util.mergeOptions(mergeTarget, options, 'anotherObject');
+    assert(mergeTarget.anotherObject !== undefined, 'Option object is not copied');
+    assert(mergeTarget.anotherObject.enabled === false, 'enabled value from options must have priority');
+
+    util.mergeOptions(mergeTarget, options, 'merge');
+    assert(mergeTarget.merge === 'hello', 'Explicit null-option should not be copied, already present in target');
+  });
+
+
+  it('gracefully handles bad input', function () {
+    var mergeTarget  = {};
+    var options = {
+      merge: null
+    };
+
+    var errMsg  = 'Non-object parameters should not be accepted';
+    assert.throws(() => util.mergeOptions(null, options, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(undefined, options, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(42, options, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, null, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, undefined, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, 42, 'anything'), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, options, null), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, options, undefined), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, options, 'anything', null), Error, errMsg);
+    assert.throws(() => util.mergeOptions(mergeTarget, options, 'anything', 'not an object'), Error, errMsg);
+
+
+    util.mergeOptions(mergeTarget, options, 'iDontExist');
+    assert(mergeTarget.iDontExist === undefined);
+  });
+
+
+  it('handles good input with global options', function () {
+    var mergeTarget  = {
+    };
+    var options = {
+      merge: null,
+      missingEnabled: {
+        answer: 42
+      },
+      alsoMissingEnabled: {  // has no enabled in globals
+        answer: 42
+      }
+    };
+
+    var globalOptions = {
+      merge: {
+        enabled: false
+      },
+      missingEnabled: {
+        enabled: false
+      }
+    };
+
+    util.mergeOptions(mergeTarget, options, 'merge', globalOptions);
+    assert(mergeTarget.merge.enabled === false, "null-option should create an empty target object");
+
+    util.mergeOptions(mergeTarget, options, 'missingEnabled', globalOptions);
+    assert(mergeTarget.missingEnabled.enabled === false);
+
+    util.mergeOptions(mergeTarget, options, 'alsoMissingEnabled', globalOptions);
+    assert(mergeTarget.alsoMissingEnabled.enabled === true);
+  });
+});