Make Attribute detection more strict

The old Attribute.isAttribute would allow objects lacking the toBer
method to be attached to Change objects.  This would result in errors
during serialization.

With the stricter Attribute detection, it's necessary to convert
Attribute-like objects containing type (string) and vals (array)
properties into real Attribute objects.  This precise detection is
necessary to avoid falling back to the object-keys-into-attributes

That other logic which will turn a well structured Attribute-like object
such as this:

  { type: 'valid', vals: ['something'] }

... and turn it into something broken like this:

  [
    { type: 'type', vals: ['valid'] },
    { type: 'vals', vals: ['something'] }
  ]

lib/attribute.js

@@ -98,8 +98,9 @@ Attribute.prototype.addValue = function (val) {
 
 /* BEGIN JSSTYLED */
 Attribute.compare = function compare(a, b) {
-  if (!(a instanceof Attribute) || !(b instanceof Attribute))
+  if (!(Attribute.isAttribute(a)) || !(Attribute.isAttribute(b))) {
     throw new TypeError('can only compare Attributes');
+  }
 
   if (a.type < b.type) return -1;
   if (a.type > b.type) return 1;
@@ -156,36 +157,41 @@ Attribute.prototype.toBer = function (ber) {
   return ber;
 };
 
+
 Attribute.toBer = function (attr, ber) {
   return Attribute.prototype.toBer.call(attr, ber);
 };
 
 
-/* BEGIN JSSTYLED */
 Attribute.isAttribute = function (attr) {
-  if (!attr) return false;
-  if (typeof (attr) !== 'object') return false;
-  if (attr instanceof Attribute) return true;
-  if (!attr.type || typeof (attr.type) !== 'string') return false;
-  if (!attr.vals || !Array.isArray(attr.vals)) return false;
-  for (var i = 0; i < attr.vals.length; i++) {
-    if (typeof (attr.vals[i]) !== 'string' && !Buffer.isBuffer(attr.vals[i]))
-      return false;
+  if (!attr || typeof (attr) !== 'object') {
+    return false;
   }
-
-  return true;
+  if (attr instanceof Attribute) {
+    return true;
+  }
+  if ((typeof (attr.toBer) === 'function') &&
+      (typeof (attr.type) === 'string') &&
+      (Array.isArray(attr.vals)) &&
+      (attr.vals.filter(function (item) {
+         return (typeof (item) === 'string' ||
+                  Buffer.isBuffer(item));
+       }).length === attr.vals.length)) {
+    return true;
+  }
+  return false;
 };
-/* END JSSTLYED */
 
 
 Attribute.prototype.toString = function () {
   return JSON.stringify(this.json);
 };
 
+
 function formatGuid(format, data) {
   for (var i = 0; i < data.length; i++) {
     var re = new RegExp('\\{' + i + '\\}', 'g');
-    // Leading 0 is needed if value of data[i] is less than 16 (of 10 as hex). 
+    // Leading 0 is needed if value of data[i] is less than 16 (of 10 as hex).
     var dataStr = data[i].toString(16);
     format = format.replace(re, data[i] >= 16 ? dataStr : '0' + dataStr);
   }

lib/change.js

@@ -52,12 +52,22 @@ function Change(options) {
   this.__defineGetter__('modification', function () {
     return self._modification;
   });
-
   this.__defineSetter__('modification', function (attr) {
     if (Attribute.isAttribute(attr)) {
       self._modification = attr;
       return;
     }
+    // Does it have an attribute-like structure
+    if (Object.keys(attr).length == 2 &&
+        typeof (attr.type) === 'string' &&
+        Array.isArray(attr.vals)) {
+      self._modification = new Attribute({
+        type: attr.type,
+        vals: attr.vals
+      });
+      return;
+    }
+
     var keys = Object.keys(attr);
     if (keys.length > 1)
       throw new Error('Only one attribute per Change allowed');

lib/client/client.js

@@ -606,14 +606,25 @@ Client.prototype.modify = function modify(name, change, controls, callback) {
     if (typeof (change.modification) !== 'object')
       throw new Error('change.modification (object) required');
 
-    Object.keys(change.modification).forEach(function (k) {
-      var mod = {};
-      mod[k] = change.modification[k];
+    if (Object.keys(change.modification).length == 2 &&
+        typeof (change.modification.type) === 'string' &&
+        Array.isArray(change.modification.vals)) {
+      // Use modification directly if it's already normalized:
       changes.push(new Change({
         operation: change.operation || change.type,
-        modification: mod
+        modification: change.modification
       }));
-    });
+    } else {
+      // Normalize the modification object
+      Object.keys(change.modification).forEach(function (k) {
+        var mod = {};
+        mod[k] = change.modification[k];
+        changes.push(new Change({
+          operation: change.operation || change.type,
+          modification: mod
+        }));
+      });
+    }
   }
 
   if (Change.isChange(change)) {

test/attribute.test.js

@@ -80,3 +80,61 @@ test('parse', function (t) {
   t.equal(attr.vals[1], 'bar');
   t.end();
 });
+
+
+test('isAttribute', function (t) {
+  var isA = Attribute.isAttribute;
+  t.notOk(isA(null));
+  t.notOk(isA('asdf'));
+  t.ok(isA(new Attribute({
+    type: 'foobar',
+    vals: ['asdf']
+  })));
+
+  t.ok(isA({
+    type: 'foo',
+    vals: ['item', new Buffer(5)],
+    toBer: function () { /* placeholder */ }
+  }));
+
+  // bad type in vals
+  t.notOk(isA({
+    type: 'foo',
+    vals: ['item', null],
+    toBer: function () { /* placeholder */ }
+  }));
+
+  t.end();
+});
+
+
+test('compare', function (t) {
+  var comp = Attribute.compare;
+  var a = new Attribute({
+    type: 'foo',
+    vals: ['bar']
+  });
+  var b = new Attribute({
+    type: 'foo',
+    vals: ['bar']
+  });
+  t.equal(comp(a, b), 0);
+
+  // Different types
+  a.type = 'boo';
+  t.equal(comp(a, b), -1);
+  t.equal(comp(b, a), 1);
+  a.type = 'foo';
+
+  // Different value counts
+  a.vals = ['bar', 'baz'];
+  t.equal(comp(a, b), 1);
+  t.equal(comp(b, a), -1);
+
+  // Different value contents (same count)
+  a.vals = ['baz'];
+  t.equal(comp(a, b), 1);
+  t.equal(comp(b, a), -1);
+
+  t.end();
+});