Fix improper type coercions, especially on array types.

STRML · STRML · commit 34b0665841d7 · 2015-04-02T21:46:33.000-04:00
This moves the `arrayItemDelimiters` configuration into http-context.js,
as it is an HTTP implementation detail like querystring parsing and is
not generalizable to remote methods exposed in other ways.

This PR also makes argument coercion more robust for any, object,
and array types.
diff --git a/lib/http-context.js b/lib/http-context.js
@@ -34,9 +34,9 @@ function HttpContext(req, res, method, options) {
   this.req = req;
   this.res = res;
   this.method = method;
+  this.options = options || {};
   this.args = this.buildArgs(method);
   this.methodString = method.stringName;
-  this.options = options || {};
   this.supportedTypes = this.options.supportedTypes || DEFAULT_SUPPORTED_TYPES;
 
   if (this.supportedTypes === DEFAULT_SUPPORTED_TYPES && !this.options.xml) {
@@ -69,6 +69,13 @@ HttpContext.prototype.buildArgs = function(method) {
     var name = o.name || o.arg;
     var val;
 
+    // Support array types, such as ['string']
+    var isArrayType = Array.isArray(o.type);
+    var otype = isArrayType ? o.type[0] : o.type;
+    otype = (typeof otype === 'string') && otype.toLowerCase();
+    var isAny = !otype || otype === 'any';
+
+    // This is an http method keyword, which requires special parsing.
     if (httpFormat) {
       switch (typeof httpFormat) {
         case 'function':
@@ -112,21 +119,51 @@ HttpContext.prototype.buildArgs = function(method) {
       }
     } else {
       val = ctx.getArgByName(name, o);
+      // Safe to coerce the contents of this
+      if (typeof val === 'object' && (!isArrayType || isAny)) {
+        val = coerceAll(val);
+      }
     }
 
-    // cast booleans and numbers
-    var dynamic;
-    var otype = (typeof o.type === 'string') && o.type.toLowerCase();
-    var isAny = !otype || otype === 'any';
+    // If we expect an array type and we received a string, parse it with JSON.
+    // If that fails, parse it with the arrayItemDelimiters option.
+    if (val && typeof val === 'string' && isArrayType) {
+      var parsed = false;
+      if (val[0] === '[') {
+        try {
+          val = JSON.parse(val);
+          parsed = true;
+        } catch (e) {}
+      }
+      if (!parsed && ctx.options.arrayItemDelimiters) {
+        // Construct delimiter regex if input was an array. Overwrite option
+        // so this only needs to happen once.
+        var delims = this.options.arrayItemDelimiters;
+        if (Array.isArray(delims)) {
+          delims = new RegExp(delims.map(escapeRegex).join('|'), 'g');
+          this.options.arrayItemDelimiters = delims;
+        }
+
+        val = val.split(delims);
+      }
+    }
 
-    // try to coerce dynamic args when input is a string
+    // Coerce dynamic args when input is a string.
     if (isAny && typeof val === 'string') {
-      val = coerce(val);
+      val = coerceAll(val);
     }
 
+    // If the input is not an array, but we were expecting one, create
+    // an array. Create an empty array if input is empty.
+    if (!Array.isArray(val) && isArrayType) {
+      if (val !== undefined && val !== '') val = [val];
+      else val = [];
+    }
+
+    // For boolean and number types, convert certain strings to that type.
+    // The user can also define new dynamic types.
     if (Dynamic.canConvert(otype)) {
-      dynamic = new Dynamic(val, ctx);
-      val = dynamic.to(otype);
+      val = dynamic(val, otype, ctx);
     }
 
     // set the argument value
@@ -169,11 +206,6 @@ HttpContext.prototype.getArgByName = function(name, options) {
   // req.query
   // req.header
 
-  // coerce simple types in objects
-  if (typeof arg === 'object') {
-    arg = coerceAll(arg);
-  }
-
   return arg;
 };
 
@@ -189,6 +221,21 @@ var isint = /^[0-9]+$/;
 
 var isfloat = /^([0-9]+)?\.[0-9]+$/;
 
+// see http://stackoverflow.com/a/6969486/69868
+function escapeRegex(d) {
+  return d.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, '\\$&');
+}
+
+// Use dynamic to coerce a value or array of values.
+function dynamic(val, toType, ctx) {
+  if (Array.isArray(val)) {
+    return val.map(function(v) {
+      return dynamic(v, toType, ctx);
+    });
+  }
+  return (new Dynamic(val, ctx)).to(toType);
+}
+
 function coerce(str) {
   if (typeof str !== 'string') return str;
   if ('null' === str) return null;
diff --git a/lib/shared-method.js b/lib/shared-method.js
@@ -162,68 +162,14 @@ SharedMethod.prototype.invoke = function(scope, args, remotingOptions, cb) {
     for (var i = 0; i < accepts.length; i++) {
       var desc = accepts[i];
       var name = desc.name || desc.arg;
-      var targetType = desc.type;
       var uarg = SharedMethod.convertArg(desc, args[name]);
-      var actualType = SharedMethod.getType(uarg);
-
-      // is the arg optional?
-      // arg was not provided
-      if (actualType === 'undefined') {
-        if (desc.required) {
-          return cb(badArgumentError(name + ' is a required arg'));
-        } else {
-          // Add the argument even if it's undefined to stick with the accepts
-          formattedArgs.push(undefined);
-          continue;
-        }
-      }
-
-      if (actualType === 'number' && Number.isNaN(uarg)) {
-        return cb(badArgumentError(name + ' must be a number'));
-      }
 
-      // convert strings
-      if (actualType === 'string' && desc.type !== 'any' && actualType !== targetType) {
-        // First attempt to parse the argument using the usual method. For arrays,
-        // this is JSON.parse() so it can throw.
-        try {
-          uarg = convertValueToTargetType(uarg, targetType);
-        } catch (e) {
-          // If the parsing threw, it's probably an array that failed to parse.
-          // Try the `arrayItemDelimiters` option to parse it. This gives us support
-          // for querystring-style `a,b,c`, for example.
-          var targetIsArray = Array.isArray(targetType) && targetType.length === 1;
-          if (targetIsArray && remotingOptions.arrayItemDelimiters) {
-            var delims = remotingOptions.arrayItemDelimiters;
-            if (Array.isArray(delims)) {
-              delims = new RegExp(delims.map(escapeRegex).join('|'), 'g');
-              remotingOptions.arrayItemDelimiters = delims;
-            }
-
-            // If we received the empty string (e.g. &arg=), make it an empty
-            // array. Otherwise, the split would turn it into ['']
-            if (uarg.length) {
-              uarg = uarg.split(delims);
-            } else {
-              uarg = [];
-            }
-
-            // use for() instead of .map() so that we don't create fns in a loop
-            for (var ix in uarg) {
-              debug('convert %j to %s', uarg[ix], targetType[0]);
-              uarg[ix] = convertValueToTargetType(uarg[ix], targetType[0]);
-            }
-          } else {
-            // Target isn't an array, or arrayItemDelimiters weren't specified. At this
-            // point we're out of options so let's throw.
-            var message = util.format('invalid value for argument \'%s\' of type ' +
-              '\'%s\': %s', name, desc.type, uarg);
-            debug('- %s - ' + message, sharedMethod.name);
-            return cb(badArgumentError(message));
-          }
-        }
+      try {
+        uarg = coerceAccepts(uarg, desc, name);
+      } catch (e) {
+        debug('- %s - ' + e.message, sharedMethod.name);
+        return cb(e);
       }
-
       // Add the argument even if it's undefined to stick with the accepts
       formattedArgs.push(uarg);
     }
@@ -279,6 +225,64 @@ function escapeRegex(d) {
   return d.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, '\\$&');
 }
 
+/**
+ * Coerce an 'accepts' value into its final type.
+ * If using HTTP, some coercion is already done in http-context.
+ *
+ * This should only do very simple coercion.
+ *
+ * @param  {*} uarg            Argument value.
+ * @param  {Object} desc       Argument description.
+ * @return {*}                 Coerced argument.
+ */
+function coerceAccepts(uarg, desc) {
+  // If array, invoke for each member of the array.
+  if (Array.isArray(uarg)) {
+    return uarg.map(function(arg) {
+      return coerceAccepts(arg, desc);
+    });
+  }
+
+  var name = desc.name || desc.arg;
+  var targetType = desc.type;
+  var actualType = SharedMethod.getType(uarg);
+
+  // Target type may be an array. If so, expect the arg to be a member of that array,
+  // since we map this function over arrays.
+  var targetIsArray = Array.isArray(targetType) && targetType.length === 1;
+  if (targetIsArray) {
+    targetType = targetType[0];
+  }
+
+  // is the arg optional?
+  // arg was not provided
+  if (actualType === 'undefined') {
+    if (desc.required) {
+      throw new badArgumentError(name + ' is a required arg');
+    } else {
+      return undefined;
+    }
+  }
+
+  if (actualType === 'number' && Number.isNaN(uarg)) {
+    throw new badArgumentError(name + ' must be a number');
+  }
+
+  // convert strings
+  if (actualType === 'string' && targetType !== 'any' && actualType !== targetType) {
+    // JSON.parse can throw, so catch this error.
+    try {
+      uarg = convertValueToTargetType(uarg, targetType);
+    } catch (e) {
+      var message = util.format('invalid value for argument \'%s\' of type ' +
+        '\'%s\': %s. Received type was %s. Error: %s',
+        name, targetType, uarg, typeof uarg, e.message);
+      throw new badArgumentError(message);
+    }
+  }
+  return uarg;
+}
+
 function convertValueToTargetType(value, targetType) {
   switch (targetType) {
     case 'string':
diff --git a/test/http-context.test.js b/test/http-context.test.js
@@ -35,6 +35,16 @@ describe('HttpContext', function() {
         input: 'null',
         expectedValue: 'null'
       }));
+      it('should coerce array types properly with non-array input', givenMethodExpectArg({
+        type: ['string'],
+        input: 123,
+        expectedValue: ['123']
+      }));
+      it('should not coerce a single string into a number', givenMethodExpectArg({
+        type: ['string'],
+        input: '123',
+        expectedValue: ['123']
+      }));
     });
 
     describe('arguments without a defined type (or any)', function() {
@@ -78,7 +88,7 @@ function givenMethodExpectArg(options) {
     app.get('/', function(req, res) {
       var ctx = new HttpContext(req, res, method);
       try {
-        expect(ctx.args.testArg).to.equal(options.expectedValue);
+        expect(ctx.args.testArg).to.eql(options.expectedValue);
       } catch (e) {
         return done(e);
       }
diff --git a/test/rest.test.js b/test/rest.test.js
@@ -11,7 +11,6 @@ var Promise = global.Promise || require('bluebird');
 var ACCEPT_XML_OR_ANY = 'application/xml,*/*;q=0.8';
 var TEST_ERROR = new Error('expected test error');
 
-
 describe('strong-remoting-rest', function() {
   var app;
   var appSupportingJsonOnly;
@@ -901,6 +900,97 @@ describe('strong-remoting-rest', function() {
         });
     });
 
+    it('should coerce strings with type set to "any"', function(done) {
+      remotes.foo = {
+        bar: function(a, b, c, fn) {
+          fn(null, c === true ? a + b : 0);
+        }
+      };
+
+      var fn = remotes.foo.bar;
+
+      fn.shared = true;
+      fn.accepts = [
+        {arg: 'a', type: 'any'},
+        {arg: 'b', type: 'any'},
+        {arg: 'c', type: 'any'}
+      ];
+      fn.returns = {root: true};
+
+      json('get', '/foo/bar?a=42&b=0.42&c=true')
+        .expect(200, function(err, res) {
+          assert.equal(res.body, 42.42);
+          done();
+        });
+    });
+
+    it('should coerce contents of array with simple array types', function(done) {
+      remotes.foo = {
+        bar: function(a, fn) {
+          fn(null, a.reduce(function(memo, val) { return memo + val; }, 0));
+        }
+      };
+
+      var fn = remotes.foo.bar;
+
+      fn.shared = true;
+      fn.accepts = [
+        {arg: 'a', type: ['number']}
+      ];
+      fn.returns = {root: true};
+
+      json('get', '/foo/bar?a=["1","2","3","4","5"]')
+        .expect(200, function(err, res) {
+          assert.equal(res.body, 15);
+          done();
+        });
+    });
+
+    it('should pass an array argument even when non-array passed', function(done) {
+      remotes.foo = {
+        bar: function(a, fn) {
+          fn(null, Array.isArray(a));
+        }
+      };
+
+      var fn = remotes.foo.bar;
+
+      fn.shared = true;
+      fn.accepts = [
+        {arg: 'a', type: ['number']}
+      ];
+      fn.returns = {root: true};
+
+      json('get',
+        '/foo/bar?a=1234')
+        .expect(200, function(err, res) {
+          assert.equal(res.body, true);
+          done();
+        });
+    });
+
+    it('should coerce contents of array with simple array types', function(done) {
+      remotes.foo = {
+        bar: function(a, fn) {
+          fn(null, a.reduce(function(memo, val) { return memo + val; }, 0));
+        }
+      };
+
+      var fn = remotes.foo.bar;
+
+      fn.shared = true;
+      fn.accepts = [
+        {arg: 'a', type: ['number']}
+      ];
+      fn.returns = {root: true};
+
+      json('get', '/foo/bar?a=["1","2","3","4","5"]')
+        .expect(200, function(err, res) {
+          assert.equal(res.body, 15);
+          done();
+        });
+    });
+
     it('should allow empty body for json request', function(done) {
       remotes.foo = {
         bar: function(a, b, fn) {
@@ -2183,7 +2273,7 @@ describe('strong-remoting-rest', function() {
 
       objects.afterError(method.name, function(ctx, next) {
         if (Array.isArray(hookContext)) {
-          hookContext.push(context);
+          hookContext.push(ctx);
         } else if (typeof hookContext === 'object') {
           hookContext = [hookContext, ctx];
         } else {
