ignore inherited and non-enumerable object properties

[hegemonic]

The current version of tv4 includes several methods that iterate over object properties without filtering out inherited and non-enumerable properties. As a result, the data object fails validation in the following example, because data inherits the extraMethod property:

function DataMaker(o) {
    var self = this;
    Object.keys(o).forEach(function(key) {
        self[key] = o[key];
    });
}

DataMaker.prototype.extraMethod = function() {};

var schema = {
    properties: {
        foo: {type: 'boolean'}
    },
    additionalProperties: false
};
var data = new DataMaker({foo: true});

tv4.normSchema(schema);
var valid = tv4.validate(data, schema);

This pull request fixes the issue by iterating over the return value of Object.keys(obj) rather than calling for (var prop in obj). If you'd prefer to fix this issue in a different way, I'd be happy to close this pull request and submit a new one.

[hegemonic]

One more note: The index.html changes are just from running Grunt. I didn't change the source for that file.

[Bartvds]

@hegemonic: a contributor here: about the index.html: it is regenerated from the README.md.

I see the grunt-markdown task (and it's marked dependency) got updated to include these id attributes. When you installed the development modules you received that new version (they pushed it as semver 'patch' update).

[geraintluff]

This is an interesting point. JSON Schema was intended for JSON data, which doesn't have issues with inherited or non-enumerable properties, so this is in some ways a bit of a grey area.

Personally, my instinct is that if a property is accessible, then it's reasonable to validate it. I mean, if I had:

DataMaker.prototype.description = "(no description yet)";

but my schema said:

{
    "properties": {
        "description": {"type": "integer"}
    }
}

should that be valid? I mean, the property exists and is of an incorrect type, so I'd say "no, that's invalid". I think I feel similarly about "additionalProperties" - should moving a variable to a prototype make it not count?

@Bartvds: how do you feel about this?

[hegemonic]

@geraintluff, I would counter by pointing out that JSON.stringify skips inherited and non-enumerable properties.

Perhaps I should explain my use case as well: JSDoc 3 creates "doclet" objects that describe symbols in JavaScript files. Each doclet is an instance of a Doclet class, and the class prototype includes several different methods. If you run JSDoc with the -X command-line option, you can get a JSON dump of the doclets.

To enhance our test suite, I'm creating a JSON schema that I can use to validate the doclets. With the current version of tv4, doclets fail validation because they inherit from Doclet.prototype. This seems kind of silly, since JSON.stringify will ignore the inherited properties anyhow.

[geraintluff]

@hegemonic: that is a good point about JSON.stringify().

OK, so a behaviour change like this seems like it would deserve a minor version bump. Is it worth pro-actively putting in a setting to maintain the old behaviour, or shall we just wait until someone actually requests it?

[hegemonic]

@geraintluff: Totally up to you. If you'd like a setting to maintain the old behavior, just tell me what to call it and how you want to expose the setting.

[Bartvds]

Interesting issue. I think by default inherited properties should be validated just the same as own properties.

Accessing a property (eg: var x = o[key];) will resolve the whole inheritance chain and property configuration so it makes intuitive sense to validate like that (for the scenario from @geraintluff's example).

On the other hand, just like @hegemonic I like to use json-schema on JS objects, and the hasOwnProperty / non-enumerable check to filter methods would've been welcome. If it is opt-in then I'm in favour for it.

For the option I think we should move to an 'options object' for the validate() methods, instead of stacking another argument (as there are a few now).

Also now we are on the topic: using json-schema on JS objects could use a few more schema rules/flags. Like

• ownProperty, enumerable
• non-JSON data types (like function, date, regex, maybe nan, undefined etc)
• from ES5 frozen / sealed / extensible

So maybe we should look at the schema-rule part of this in a slightly wider scope and see if tv4 + json-schema can support these kind of JS specific 'extensions' (I guess it can if the implementation complexity is not too much).

[Bartvds]

I just noticed some new comments came in after I started to reply.

The JSON.stringify() argument is quite good indeed.

Even so consider this a +1 for a way to keep current behaviour accessible (even if it wouldn't be the default), it is especially handy to validate JS values (where inherited non-enumerable properties are important).

[hegemonic]

Got it. I'm happy to make this configurable. I can also update the validate() methods to accept either the current parameters or a new options object.

Just let me know what you'd like to call the option(s) for ignoring inherited and non-enumerable properties.

[geraintluff]

An options object sounds good at this point - as the third argument?

It might also be good to be able to modify the default options:

tv4.options({ownPropertiesOnly: true}); // merges against existing defaults
var freshTv4 = tv4.freshApi({language: 'de'}); // resets everything (schema cache, settings, etc.)
tv4.validate(data, schema, {checkRecursive: true}); // per-run options

@hegemonic: I apologise for the feature creep on this one. I don't want you to feel like your code (and your very useful question/insight) is not appreciated! If you'd rather, then this can be pulled into a separate branch and I/we can add the options stuff later.

(Also, I should definitely add a Contributors.txt...)

[geraintluff]

@Bartvds: for the extensions you mention, I would consider custom "type" values. My first thought is that unknown types should be either ignored with a warning (no other keywords checked), or result in an UNKNOWN_TYPE error. I'll put that in a separate issue (#79).

[Bartvds]

@geraintluff The options as third argument sounds good to me, I really like being able to set all of them both as default as well as per validation run.

I don't know in which order to add that with @hegemonic's changes, whatever is practical I guess.

[hegemonic]

@geraintluff: No worries about the feature creep...sometimes one idea leads to another.

Here's how I'd like to proceed:

• I'll update tv4.validate() to accept a third options argument. I'll make sure the existing signature still works, and I'll try to set things up so you can easily add tv4.options() and tv4.freshApi() later.
• I'll add a new option that allows you to validate inherited and non-enumerable properties (or I can make this two separate options if you prefer). Users will need to use the new tv4.validate() signature to use this option. By default, tv4.validate() will not validate inherited/non-enumerable properties.

I should be able to do this within the next several days. After that change is merged, you and @Bartvds can implement the other changes you discussed.

Does that sound good?

[Bartvds]

@hegemonic Note there are also tv4.validateResult() and tv4.validateMultiple(), both are friendlier for power-use then 'classic' tv4.validate(), because they return a result object instead of keeping error data on the API (handy if you run many validations from async code).

@geraintluff Maybe if tv4 needs to move to a new non-patch release because signatures change anyway it is a good moment to drop this statefull tv4.validate(). IIRC this notion floated around earlier. Just an idea though.

[geraintluff]

@hegemonic: that sounds great, thanks!

@Bartvds: I agree it's not great. Remove it completely, or just drop the stateful aspects and return a boolean?

[hegemonic]

@Bartvds: I'll be sure to address tv4.validateResult() and tv4.validateMultiple() as well.

[Bartvds]

@geraintluff I have no preference. Could be convenient to keep it as simple shorthand with just a boolean if it is documented as such.

[hegemonic]

Okay, I've taken care of the changes we discussed. I decided to use two separate options for inherited and non-enumerable properties, on the theory that anyone who needs those options will want really fine-grained control over validation.

I also added documentation for the two new options, along with banUnknownProperties, which was not previously documented. The new docs are a bit repetitive, but they get the job done.

Please let me know if you have any additional feedback before you merge this.

[Bartvds]

@geraintluff have you had a chance to look at this?

[geraintluff]

@Bartvds - I'm afraid this had completely slipped off my radar. The changes look reasonable, though - unfortunately, I think I've checked in some incompatible ones since, so the merge'll have to be manual...

[hegemonic]

@geraintluff: What can I do to help you merge this? Would you like me to rebase the changes off the current master?

[geraintluff]

@hegemonic - if you could that would be fantastic. :)

[hegemonic]

@geraintluff: Done! Hope this is now ready to merge.

[Bartvds]

It looks like some commits landed after you restarted the work: the merge is disabled (and the diff is odd).

So I think you need another rebase to solve the conflict.

[Bartvds]

We created a develop/next branch for the changes that need a version bump. Could you target on that branch?

Sorry for the confusion, but we're juggling a few simultaneous PR's so please bear with us (your feature is very welcome! :)

[hegemonic]

@Bartvds: Will do. I'll create a new pull request if necessary.

[hegemonic]

I'm going to close this pull request and submit a new one that targets the develop/next branch.