Collapsing Update Objects

[aldeed]

There are a number of areas where the variety of possible modifier formats makes things confusing for validation, or for extra checks that are done in collection2. A good solution might be to collapse update objects in a manner similar to what is currently done with insert objects. In other words, rearrange keys in the modifier object so that the first level of keys matches the format of the schema keys. Then have all the modifier objects below that. Certain modifier objects might make this difficult, so we'll have to see if it's possible.

An example:

{
  $set: {
    'name.first': Foo,
    'name.last': Bar,
    age: 45
  },
  $inc: { updates: 1 },
  $push: { scores: 89 }
}

Is run through the collapse function and becomes:

{
  'name.first': { $set: Foo },
  'name.last': { $set: Bar },
  age: { $set: 45 },
  updates: { $inc: 1 },
  scores: { $push: 89 }
}

This makes validation much easier because we now only have to loop through the first level and look from schema rules, and then check for any operators under that and perform appropriate validations. This will make it much easier to implement validation checks for the remaining operators that are not currently supported. Also, the rearranged object can be passed to custom validation functions, autoValue functions, etc. to make it much easier for those to validate, set values, etc.

[mquandalle]

I totally agree, this is the syntax that we need for update validation. Actually I've started to think about it because a autoValue function can return a mongo modifier like { $inc: 1} but only for themselves, it can't do a $set on an other field, that why the "collapsing" syntax makes sense.

So we basically need two new functions in the simple-schema package, one for transform a mongo modifier in a indexed by field keys document, and the opposite. This piece of code might help.

Do you have time to implement it?

[aldeed]

Right, looking at your { $inc: 1 } example is what gave me this idea. simple-schema.js already has private functions collapseObj and expandObj that do this, but currently only for insert docs. So those two functions need to be enhanced to rearrange update modifiers, too, and then we need to add a public API for them. I will have time in a couple days. I'll be leaving soon and will be without access to dev machine for a day or so.

[mquandalle]

One more thing, in the collapsed document, if there is no mongo modifier we should set the default to $set this allow autoValue to just return value instead of {$set: value}. But even more, I think that the function that return the collapsed document should return it without the $set modifiers, like this:

{
  'name.first': Foo,
  'name.last': Bar,
  age: 45,
  updates: { $inc: 1 },
  scores: { $push: 89 }
}

That way, the autoValue function could handle both insert and update case with a single doc.name.first (no more need to do

if (type === 'insert')
  return doc.name.first
else
  return doc.name.first.$set

)

[aldeed]

I'm reworking expand/collapse, and I realized that I don't think pulling $set keys up a level will work. The reason is because anything we collapse we need to be able to expand back to what it was. If we strip out the $set level when collapsing, the expand function won't know that those values need to go into $set.

[mquandalle]

https://github.com/mquandalle/meteor-collection2/blob/autoValue/collection2.js#L191-L194?

[aldeed]

It was fine to do that in c2 because you knew you were doing an update, but simple-schema doesn't know whether it's an update object or an insert object. It just tries to support everything blindly.

[mquandalle]

Ok so keep $set for now.

Collapsing object may be a relatively big change, I think that you should publish this work in a separate public git branch, not in master. Then I'll try to use this branch in aldeed/meteor-collection2#9 to see if the API works for this use case.

[aldeed]

Yep, I'm already working on it in a separate branch. It's a mess right now, but I'll push it to github when it's testable.

[aldeed]

It's really difficult to handle all the possibilities of normal docs and modifier operators when doing validate() or validateOne(). I'm thinking that an easy solution might be to make the caller indicate whether they're passing an object in which $ operators are OK. Something like this:

mySchema.validate(doc, {modifier: true});

By default it would not allow modifiers, so that would be a breaking change. With all the object manipulation happening behind the scenes, I'm a little worried there might be an integrity issue where SS would think it's an update object and validate it accordingly, but then somehow ends up being used as an insert object.

It's possible this isn't necessary. When I finish the changes, you can look at that branch and see what you think.

[mquandalle]

I agree with this API change (already thought of it).

In many cases SimpleSchema is hidden behind an Autoform (UI validation on the client) or behind a collection2 (insert/update/delete). For both of them there is an automatic way to know if we do an insert or an update, and mySchema.validate is called behind the scene, not directly by the developer.

For other cases when one need to manually call .validate, I think it's not a problem to add a argument that say if I want to validate a document or a document modifier. That makes sense (this is not the same kind of validation), and that makes things more clear in all internals API.

[aldeed]

Take a look at the devel-oo branch when you have a chance. I think most of the changes are done, but there are still a few small things left to do. I haven't updated the condition function used by check and Match.test yet. This is one place where we call .validate() but we don't know whether it's a normal doc or modifier object. One option would be to make an educated guess in the condition function. Another option would be to make an educated guess in the validate and validateOne functions, if options.modifier is not set. This would have the added benefit of making everything backwards compatible.

Another nifty thing I added is validation contexts. This pulls the validation and reactivity code out from the SimpleSchema object and into a separate object that has the validate methods and all of the reactive methods on it. The reason for doing this is so that you can have multiple reactivity contexts that share the same schema but refer to different objects.

Example usage:

var ss = new SimpleSchema({
    requiredString: {
        type: String
    }
});
var ssContext1 = ss.newContext();
ssContext1.validate({});
ss.valid(); // false (for backwards compatibility)
ssContext1.valid(); // false

var ssContext2 = ss.newContext();
ssContext2.validate({
  requiredString: "test"
});
ss.valid(); // true (reflects most recent context validated)
ssContext1.valid(); // still false
ssContext2.valid(); // true

In the case of an autoform, having multiple contexts means that you could have several autoforms on the same screen, all using the same collection2, but each one would be independently valid or invalid. So for example you could have an insert form beside an update form, and when you try to submit an invalid insert form, the fields in the insert form would turn red but the fields in the update form would not.

The validation context should have been separate from the schema from the beginning. The readme will say to use them, but for now I will leave the reactive methods on the SimpleSchema instance, too, for backwards compatibility.

[mquandalle]

I agree with the need of a validation context. About the API, maybe something like this is better:

var ssContext1 = new ss.validation(); // new ss.validationContext?
ssContext1.validate({}); // the same
ssContext1.isValid(); // match backbone API, more clear

Need to think more about this. I'm going to take a look at your code ;)

[mquandalle]

Do we really need backwards compatibility? That makes things more difficult to understand (for instance the Deps management is duplicated in two objects SimpleSchema and SimpleSchemaValidationContext, the same with regEx and .match that are no more used), and maybe difficult to test. What do you think of removing all the unnecessary stuff and keep this work in a separate branch (devel), and then release it as a 0.2 version that breaks the compatibility. There are not so many users of those packages (yet), most of them doesn't call .validate() themselves, and if they don't want to upgrade they can still continue to use previous versions.

I think we should try to get the clearer code as possible.

[aldeed]

I don't have a strong opinion. Generally I'm fine with compatibility breaks because Meteor is rapidly evolving and people expect them. But we could also leave the code there and add console.warns, then release that in 0.1 stream, then remove the code, then release that it 0.2 stream. For readability, all of the deprecated stuff could simply be moved to a separate js file.