563555 - apiUtils.validateOptions() should provide shortcut for type names

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

Atul pointed out in bug 559859 comment 3 that apiUtils.validateOptions() should let you specify type names as a shortcut.  This patch does it a little differently than suggested.  You can define `is` to check that the value is of a certain type, while still mapping the value beforehand or doing more validation after in `ok`:

  var opts = { foo: 1337 };
  var requirements = {
    foo: {
      map: function (val) val.toString(),
      is: ["string"],
      ok: function (val) val.length > 0,
      msg: "foo must be a non-empty string."
    }
  };
  var validatedOpts = apiUtils.validateOptions(opts, requirements);
  // validatedOpts == { foo: "1337" }

Comment 1

[Atul Varma [:atul]]

Comment on attachment 443253 [details] [diff] [review]
patch

>+// Similar to typeof, except arrays and null are identified by "array" and
>+// "null", not "object".
>+function getTypeOf(val) {
>+  let typ = typeof(val);
>+  if (typ === "object") {
>+    if (!val)
>+      return "null";
>+    if (val.constructor && val.constructor.name === "Array")
>+      return "array";

If 'val' is untrusted, couldn't it be spoofed such that its constructor.name happens to be called "Array"?  Perhaps this code will be replaced by Array.isArray() soon enough anyways, but John Resig recommends this from his blog post [1]:

  Array.isArray = function( array ) {
    return Object.prototype.toString.call( array ) === "[object Array]";
  };

Dunno how secure this one is either, but at least I can't think of any immediate ways to hack it.

[1] http://ejohn.org/blog/ecmascript-5-strict-mode-json-and-more/

The only other question I have is whether validateOptions() supports any type coercion. For instance, what about cases where the argument passed in is the string '42', which in many JS situations would just be coerced to an integer when necessary (i.e., implicitly parseInt()'d)?  Do we want to support such coercion here or be more strict?  Whichever we choose, we should probably document it so developers know what to expect.

Other than those two issues, this looks great!

Comment 2

[Drew Willcoxon :adw]

Attachment: patch v2

(In reply to comment #1)
> Array.isArray() soon enough anyways, but John Resig recommends this from his
> blog post [1]:

Cool, thanks.  John practices what he preaches [1].  Crockford does this instead [2]:

  typeof value.length === 'number' &&
  !(value.propertyIsEnumerable('length')) &&
  typeof value.splice === 'function'

[1] http://code.jquery.com/jquery-1.4.2.js
[2] http://javascript.crockford.com/remedial.html

> The only other question I have is whether validateOptions() supports any type
> coercion. For instance, what about cases where the argument passed in is the
> string '42', which in many JS situations would just be coerced to an integer
> when necessary (i.e., implicitly parseInt()'d)?

Hmm, good question.  My gut feeling is that most of the time callers will want coercion that's obvious, like "42" <-> 42.  So I updated the patch to coerce values to boolean, number, and string types, but as I started to write the test it became clear that some cases are less obvious.  Number([]) == 0, Number([1]) == 1, and Number([1,2]) is NaN.  If a client passes an array by mistake into an API that takes a number, it should result in an error, not be silently accepted as valid.  validateOptions() could eliminate the less obvious cases, but then the docs need to explain what exactly is coerced to what.  And maybe in different use cases the meaning of "obvious" changes.  Overall I think it would make validateOptions() harder to keep in your head and use.

So, this patch explains that no coercion is done.

Comment 3

[Atul Varma [:atul]]

Comment on attachment 443417 [details] [diff] [review]
patch v2

Great, that sounds good. And if we figure out a straightforward/secure way to do type coercion in the future, it seems like we can "upgrade" this implementation while maintaining compatibility with anything that uses it.

The patch looks great! Only nit is that I might add a TODO where the 'Array.isArray()' use will eventually appear, to make it more likely that we will change it in the future, but that's about it.

Comment 4

[Drew Willcoxon :adw]

Thanks Atul.  Added the TODO.

http://hg.mozilla.org/labs/jetpack-sdk/rev/faf15219d107

Comment 5

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".