Meteor-Community-Packages / meteor-collection2

A Meteor package that extends Mongo.Collection to provide support for specifying a schema and then validating against that schema when inserting and updating.
https://packosphere.com/aldeed/collection2
MIT License
1.02k stars 108 forks source link

Schema validation applied to update operator #77

Closed lingz closed 10 years ago

lingz commented 10 years ago

I'm not sure if this is the supposed behavior, but when doing

Collection.update(id, {$addToSet: {field: "item"}})

the entire schema validation runs against the

{$addToSet: {field: "item"}}

Object, which will fails, as it only contains a fraction of the entire object. I'm wondering if this type of behavior is supported at all, or if we must pass $set on the entire object every time? A workaround is to use Meteor methods, but that would necessitate a call to the server for every minor operation. Is this the current design or am I implementing something wrong?

aldeed commented 10 years ago

Yes, SimpleSchema is designed to correctly validate modifiers. But it's possible you've found a bug in the logic. What is your schema and what failures do you get?

lingz commented 10 years ago

Take this toy example

Books Schema:

name:
  type: String
slug:
  type: String
  optional: true
  autovalue: function() {
    return this.field("name").value.replace(" ", "-");
  }
authors:
 type: [String]
 defaultValue: []

bookId = Books.insert({name: "My Great Book"})
Books.update(bookId, {
  $addToSet: {
    authors: "Me"
  }
}

This will throw an error when it tries to clean the update operator, and it is trying to generate the slug with autovalue, as this.field("name").value will return undefined. Is this a bug or the correct behavior? It doesn't seem intuitive to be using the same schema to validate update operators as the object itself.

aldeed commented 10 years ago

Yes, that's correct. Your autoValue should only generate a slug when name is set or updated:

autoValue: function() {
  var name = this.field("name");
  if (name.isSet && name.operator !== "$unset") {
    return name.value.replace(" ", "-");
  }
}

(In your case, you don't actually need to check whether the operator is $unset because unsetting the name would never be allowed by your schema. But I included it to show how you'd have to consider the operator if name were optional.)

The schema should reflect what you want in the database. SimpleSchema is smart enough to examine an update modifier and figure out what would be in the database if the update were allowed. When writing autoValue or custom functions, you have to think a bit differently, not about what's in the database but about what's in the document or modifier being validated. Technically autoValues should be written to examine and handle both inserts and updates, including any possible operators.

Side point: Doesn't your replace only replace the first space? I think you need replace(/ /g, "-").

lingz commented 10 years ago

Ah, I see. So we need to guard the autovalues against firing during update operations. I didn't think every defaultValue / autoValue was set on every update, so this was my fault (though to be fair, I think it would be helpful to have the nature of autoValue / defaultValue outlined more in the documentation). Though on second though, it makes sense as the most obvious way to implement this simpleSchema functionality.

What about operators like $addToSet, where doing autovalues wouldn't make sense? Also, does the redundant database operations hurt performance (I'm not sure, but would this interact with oplog tailing)?

(on the sidenote, the thing about replace was only illustrative, we use a regex in actual code)

aldeed commented 10 years ago

I agree it needs to be documented better. Not sure when I'll get around to it.

There isn't any specific handling for certain operators right now. If you define an autoValue function, it is called every time ss.clean is called. If it returns a value, that property-value is added to the insert doc or update modifier. (If you don't return a pseudo-modifier for an update, it is automatically interpreted as the $set: value pseudo-modifier.) It's as simple as that.

There are no redundant database operations. The schema-validation layer is before the db op layer. Neither simple-schema nor collection2 do any interaction with the database.