569122 - Improve api-utils.validateOptions()

Status: RESOLVED WONTFIX

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

Description

[Drew Willcoxon :adw]

api-utils.validateOptions() tries to do too much, so it's hard to explain, understand, and use.  You guys have all used it and given me good feedback, so if you have time I'd appreciate your thoughts on these changes.

First, the common case of matching types and optionally using some default value should be as simple and require the least amount of typing as possible.

Second, if you need more than that, the interface should be general, to let you do whatever you need, and easy to understand.

Third, you should be able to mix the two styles on the same options object, using one style for one property and the other style for another property.

Proposed API:

  // Constructor: Makes a new property validator.
  apiUtils.PropertyValidator(options);

  // Simple use: Throws if typeof options[propertyName] is not in
  // expectedTypes.  If defaultValue is given, undefined is allowed, and if
  // options[propertyName] is undefined, defaultValue is returned.
  propertyValidator.get(propertyName, expectedTypes, defaultValue);

  // General use: Throws if mapOkMsg.ok returns false, like how
  // validateOptions currently works.
  propertyValidator.get(propertyName, mapOkMsg);

Simple use examples:

  let val = new apiUtils.PropertyValidator(options);

  let optionalStr1 = val.get("optionalStr1", ["string"], "default val");
  // optionalStr1 == options.optionalStr1 or "default val"

  let optionalStr2 = val.get("optionalStr2", ["string", "undefined"]);
  // optionalStr2 == options.optionalStr2, possibly undefined

  let requiredStr1 = val.get("requiredStr1", ["string"]);
  // requiredStr1 == options.requiredStr1

General use example:

  let requiredStr2 = val.get("requiredStr2", {
    map: function (val) String(val),
    ok: function (val) typeof(val) === "string" && val,
    msg: "requiredStr2 must be a non-empty string"
  });

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Drew, this is really cool. The goals you list are indeed good improvements for the options validator, and the proposal covers them well.

I have a doubt about the undefined/default value behavior. Maybe you inverted the usage on the examples, but I don't understand the example 1. Specifically, I don't see what is the reason for providing a default value if "undefined" wasn't specified on the list of accepted types. Shouldn't the default value only be used in case that "undefined" was explicitly listed? that is:

let optinalStr1 = val.get("optionalStr1", ["string", "undefined"], "default val");
// optionalStr1 == options.optionalStr1 or "default val"

let optinalStr1 = val.get("optionalStr1", ["string", "undefined"]);
// optionalStr2 == options.optionalStr2, possibly undefined


Other than that, I really support this change. The only difference that I feel is that I kinda liked having a "curated" options object (one which I knew all options were already validated). It also made the validation be centralized in one place. However, I can still use this API that way if I chose to (centralized and build a curated object), so it's not really a problem.

Comment 2

[Drew Willcoxon :adw]

Thanks Felipe.

(In reply to comment #1)
> I have a doubt about the undefined/default value behavior. Maybe you inverted
> the usage on the examples, but I don't understand the example 1. Specifically,
> I don't see what is the reason for providing a default value if "undefined"
> wasn't specified on the list of accepted types.

That's an intentional shortcut:  If a default value is given, then you must mean that undefined is a valid type, so you don't have to type "undefined".  I understand that it complicates the API slightly, so I'm not opposed to always requiring you to specify undefined.

> Other than that, I really support this change. The only difference that I feel
> is that I kinda liked having a "curated" options object (one which I knew all
> options were already validated).

Oh, interesting.  The reasons for dropping the curated object approach is that it's more typing and probably more lines (because of all the object literals and their keys), and it creates headaches like, is |foo in validatedOptions| and if so under what circumstances?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> > Other than that, I really support this change. The only difference that I feel
> > is that I kinda liked having a "curated" options object (one which I knew all
> > options were already validated).
> 
> Oh, interesting.  The reasons for dropping the curated object approach is that
> it's more typing and probably more lines (because of all the object literals
> and their keys), and it creates headaches like, is |foo in validatedOptions|
> and if so under what circumstances?

I think I'm in the same boat. I don't mind a few extra lines or a bit of extra typing. It's both simple and explicit, so I think it works well.

I like the simple |is: [type, type]| syntax and I would support the addition of |default: "bar"| like we talked about - giving a really simple "I just care what type it is and if it's set" mode and the more advanced "do something fancy with map: and ok:".

That said, I did just realize that I need to reuse validation (options can be set after creating the object). So if I could have a reusable validator object, that would be awesome. Off-the-cuff, something like this:

> function Foo(options) {
>   let validator = PropertyValidator({ ... rules including "bar" ... });
>   options = validator.validateOptions(options);
>   this.__defineSetter__("bar", function(val) {
>     options = validator.validateValue("bar", val);
>   })
> }

Also, with your proposal, nothing would throw until the value gets accessed. This means you'd have to do all this validation explicitly upfront - ideally, an invalid option would throw from the c'tor, since it might not be accessed immediately.

Comment 4

[Drew Willcoxon :adw]

Thanks guys, good feedback.

(In reply to comment #3)
> I think I'm in the same boat. I don't mind a few extra lines or a bit of extra
> typing. It's both simple and explicit, so I think it works well.

So the complexity surrounding `foo in validatedOptions` -- it depends on whether `foo in options`, whether map() is given, whether map() throws -- doesn't bother you guys?  That's one of my main problems.

> Also, with your proposal, nothing would throw until the value gets accessed.
> This means you'd have to do all this validation explicitly upfront - ideally,
> an invalid option would throw from the c'tor, since it might not be accessed
> immediately.

Well, here's the ctor use case I was thinking of:

  function Ctor(options) {
    let val = new PropertyValidator(options);
    let foo = val.get("foo", [], ...);
    this.__defineGetter__("foo", function () foo);
  }

> That said, I did just realize that I need to reuse validation (options can be
> set after creating the object). So if I could have a reusable validator object,
> that would be awesome. Off-the-cuff, something like this:
> 
> > function Foo(options) {
> >   let validator = PropertyValidator({ ... rules including "bar" ... });
> >   options = validator.validateOptions(options);
> >   this.__defineSetter__("bar", function(val) {
> >     options = validator.validateValue("bar", val);
> >   })
> > }

This is great, Paul.  I like how you pass the rules, not the options, to the ctor.  Validation is guaranteed to be consistent every time the same validator is used.  As you point out, it's not uncommon to need to do validation more than once.

But with the split between simple and general, how should the API prevent the error of mixing `is` and `default` with map() and ok()?  This is ugly because it encourages that:

  PropertyValidator({
    foo: {
      is: ["string", "undefined"],
      default: "foo"
    },
    bar: {
      map: function () {},
      ok: function () {}
    }
  });

This is too verbose:

  ObjectValidator({
    foo: SimpleValueValidator({
      is: ["string", "undefined"],
      default: "foo"
    }),
    bar: GeneralValueValidator({
      map: function () {},
      ok: function () {}
    })
  });

Or maybe not?  Or maybe trading prettiness for less verbosity is OK?

The ValueValidators are kind of neat, since you could use them on their own to validate non-object values if need be:

  let value = "some value";
  let validator = SimpleValueValidator({
    is: ["string", "undefined"],
    default: "foo"
  });
  let validatedValue = validator.validate(value);

Comment 5

[Drew Willcoxon :adw]

Or imperatively:

  let validator = ObjectValidator();
  validator.simpleRule("foo", {
    is: ["string", "undefined"],
    default: "foo"
  });
  validator.generalRule("bar", {
    map: function () {},
    ok: function () {}
  });

Or:

  let validator = ObjectValidator();
  validator.rule("foo", ["string", "undefined"], "foo");
  validator.rule("bar", {
    map: function () {},
    ok: function () {}
  });

Comment 6

[:Felipe Gomes (needinfo for replies!)]

All great suggestions. Looking at all the examples, I think I like the idea of passing the rules to the validator, rather than the options object, although this adds back some of the verbosity you were trying to remove.

I think that most of the verbosity comes from the fact that we have two properties for each key: the allowed types, and the default value, leading to: 

keyName: {
  is: ['a', 'b'],
  default: 'c'
}

Now, what if the default value is removed from the rules, and used only during validation? The rules object can become simple keyName:array pairs, and the costructor would be:

let validator = Validator({
  foo: ["string", "undefined"],
  bar: ["number"]
});


for single values, we can validate with:
let val = validator.validate("foo", val, "default");

or for options:
let options = validator.validate(options);


Only downside is that this rules out validating the whole options object and have default values. But I'm a-ok on removing the options obj support entirely now that you reminded that all the "foo" in validateOptions headaches are due to that.

New rules ore the generic map/ok rules could be added later through validator.addRule()  (or even in the constructor if wanted)

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Talked with Drew, taking.

So I wrote a little wrapper to make a reusable validator while working on Request (see https://gist.github.com/51016aed7852f00ff478 for a trimmed summary of usage/implementation).

Drew's on board with passing the rules & creating a validator and it seems Felipe is too, so we'll do that. So that puts general usage so far as

  // c'tor
  validator = OptionsValidator({ /* rules */ });
  // validate all options
  options = validator.validateOptions(options);
  // validate a single option
  foo = validator.validateSingleOption("foo", newValue);

Now to figure out how we want to define the rules... here's what I'm thinking:

* To alleviate the foo in options problem, we'll simply say that only keys in the rules are allowed. Everything else will be removed (I think that matches current behavior)

* I would like to have a "default" value parameter for rules. Most of my rules are "map" (v = v || default) & "is". So we're going to have that

* If you're doing anything more complicated than setting a default, use "map".

* The parameters for each rule will be "ok" "map" "is" "default"

* "map" can only be used with "ok", "default" can only be used with "is".

* You don't have to define "default" or "map"; you must define "is" or "ok" (even if it's just a function that returns true)

* "ok" & "map" must be functions; "default" can be any type, "is" must be an array of types.

* "default" will only be used if, option[key] is undefined && undefined isn't in "is"

* "is" will be checked regardless of whether or not "default" was used & will follow "default"

* "map" will be run before "ok"

* Errors will be thrown from validator c'tor and addRule (if it gets implemented) if you don't follow the rules set above

Many of these guidelines are really just reiterating how validateOptions currently works. I tried to get them all, but I might have missed something. If down the line we need to revisit these, we will.


If we want the ability to add/remove rules, the would work like so:

  validator.addRule("bar", { /* rule */ });
  validator.removeRule("bar");


Now I'm not sure with that definition of rules if this got any less complicated, but I think it makes sense. It provides multiple pathways, yes, but they should be well contained. It's still verbose, but I'm of the belief that's ok.

Comment 8

[Drew Willcoxon :adw]

Paul and I chatted (chut?) offline.  As long as we're making progress I'm OK, so his proposal is fine with me.  As he says, we can continue to refine.

I like sanity-checking the dev's rules.  That will help.  It would still be good for the docs to distinguish between the two "simple" and "general" cases, even if the API doesn't really, except for the sanity checking.

validateOptions() and validateSingleOption() I like, but their names are a little long, especially since they contain "validate", which is already implied because you've got a validator object.  I can't think of a better verb, but at least validateOptions() should be validateObject() -- to signify any object can be validated, not just an "options" object -- and validateSingleOption() should be validateValue().  Or something.

Comment 9

[Drew Willcoxon :adw]

While I'm thinking about it: it should be possible to specify non-standard types in `is`:

function MyType() {}
validateOptions(opts, {
  foo: {
    is: [MyType]
  }
});

And the types of elements in arrays, maybe something like:

validateOptions(opts, {
  bar: {
    is: ["array"],
    elementsAre: ["number"]
  },
  baz: {
    is: ["array"],
    elementsAre: [MyType]
  }
});

Comment 10

[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".

Comment 11

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

Drew, Paul: are y'all still working on this and planning for it to happen?

Comment 12

[Drew Willcoxon :adw]

I'm not working on it or planning for it to happen, and I doubt Paul is, so I'll go ahead and wontfix it.  Paul, if I'm wrong, apologies, please reopen.