Thoughts on running validators when updating

[Tony Mobily]

Hi,

While this seems trivial at first... well it's not. At all.

When saving, it's easy to find out what validator to run on what field, because you have the object right there in front of you: you just go through every single property, and therefore have the full path there ready to go -- pronto.

It's not so easy with updates. The biggest challenge is to figure out the paths of the objects that are about to be updated.

Mongoose wisely adds a nice $set to the update queries. So, it's basically a matter of checking what you are about to set. But!

* The user might be using positioning operators. For example, { $set: { 'users.$.email':'n...@email.com } }. In this case, it *looks* like I can basically discard the positioning operator (including numbers).

* The user might be setting the whole object. And sub-objects after that. For example:

db.testing.update( { 'users.name': 'tony' }, { $set: { 'users.$.address': { street: 'other', suburb: 'again'}   } }  )

In this case, the paths are "users.addres.street", 'users.address.suburb". Except, they are _not_. Imagine for example that there are OTHER fields in "address" that are mandatory... a $set like this would zap them. So, we need to check if 'users.address' is then a schema: if it is, then we need to run the validation with the whole object setting it ( { street: 'other', suburb: 'again'} ). OK so:

* Step 1: normalise positions, zap /.[0-9]+./ and /.$./ (replace them with a dot)

* Step 2: Check where the path points:

  1) It's a schema: apply validation to it using the object belonging to $set. This will ensure that "missing" fields will still give out an error

  2) It's a field. Find the right validator, apply it. This is the easy way.

I looked at the code that does the validation. It looks hard, obviously, but that's because I don't understand the code well enough (remember, this is the guy who cannot quite work out what save() is called when you save a document...).

The only problem I have is that I am not 100000% sure this covers _all_ of the basis. I spent the morning trying lots of different possible $set queries with Mongodb, and all the attempts that would "break" what I wrote above would also break the Mongodb updates as they would have wrong syntax etc.

However, if I change update() so that it does do the validation as explained above, maybe we have a good starting point.

Merc.

[Aaron Heckmann]

Yeah I think your logic is sound. And yes, its not easy. Keep in mind it took four iterations just to get the current update logic working with casting.

If you make some progress, open a pull request and we can review/provide pointers.

--
--
http://mongoosejs.com - docs
http://plugins.mongoosejs.com - plugins search
http://github.com/learnboost/mongoose - source code
 
You received this message because you are subscribed to the Google
Groups "Mongoose Node.JS ORM" group.
To post to this group, send email to mongoo...@googlegroups.com
To unsubscribe from this group, send email to
mongoose-orm...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/mongoose-orm?hl=en
 
 

--
Aaron

@aaronheckmann

[Tony Mobily]

Hi,

OK thanks for the response!

Merc.

[Tony Mobily]

Hi,

One possible showstopper: setters expect "this" to be the document; while updating, we can't do that because we don't actually _have_ a document. (I discovered this from the code, since I had used "this" in a setter).

I assume the same applies to validators. This could possibly mean that validators that work fine in a load/save context using "this" will fail miserably when applied to an update...

* A different set of validators/setters would be madness

* Setting "this" to an empty document could possibly work but it's likely to have ugly side effects

Thoughts/help?

Merc.

On Tuesday, December 18, 2012 7:55:41 AM UTC+8, Tony Mobily wrote:

Hi,

OK thanks for the response!

Merc.

On Tue, Dec 18, 2012 at 7:00 AM, Aaron Heckmann
<aaron.h...@gmail.com> wrote:
> Yeah I think your logic is sound. And yes, its not easy. Keep in mind it
> took four iterations just to get the current update logic working with
> casting.
>
> If you make some progress, open a pull request and we can review/provide
> pointers.
>
>
> On Sun, Dec 16, 2012 at 6:35 PM, Tony Mobily <tonym...@gmail.com> wrote:
>>
>> Hi,
>>
>> While this seems trivial at first... well it's not. At all.
>> When saving, it's easy to find out what validator to run on what field,
>> because you have the object right there in front of you: you just go through
>> every single property, and therefore have the full path there ready to go --
>> pronto.
>>
>> It's not so easy with updates. The biggest challenge is to figure out the
>> paths of the objects that are about to be updated.
>>
>> Mongoose wisely adds a nice $set to the update queries. So, it's basically
>> a matter of checking what you are about to set. But!
>>
>> * The user might be using positioning operators. For example, { $set: {

>> 'users.$.email...@email.com } }. In this case, it *looks* like I can

[Tony Mobily]

Hi,

Well, I didn't think I would, but... well, I am getting closer.

I thin the lack of scope is not as tragic as I though. I have already written the code that works out the right validator for each set field, which is _fantastic_.

I should put hook() code around update() and findOneAndUpdate(). However! There is a lot of logic behind the working out of the parameter. If I write a hook, I will have to replicate all that. Aaron, are you all for the hook()?

What I am thinking right now, is this: I am writing the best code I can write to do the actual validation (I am nearly there). We will have to then decide where to put it...

I think it's doable. I really need it, so I am glad it's happening. The findOneAndSave() method was just silly.

Merc.

[Tony Mobily]

Hi,

My last message was clear as mud...
Ignore if you cannot follow it. I will work on it more tomorrow and possibly provide some semi working code to review.
I did 10 hours straight today, I think I am getting too hold for this sort of thing... :)

Merc.

[Aaron Heckmann]

I do not want any more hooks added. Users can hook more if they want but they already have performance and other subtle implications that I don't love. Looking forward to your code.

Merc.

--
--
http://mongoosejs.com - docs
http://plugins.mongoosejs.com - plugins search
http://github.com/learnboost/mongoose - source code

You received this message because you are subscribed to the Google
Groups "Mongoose Node.JS ORM" group.

To post to this group, send email to mongoo...@googlegroups.com
To unsubscribe from this group, send email to
mongoose-orm...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/mongoose-orm?hl=en

--
Aaron

@aaronheckmann

[Tony Mobily]

Hi,

Well that's good to hear, because we couldn't have anyway :D
The nature of the update query is different, it can have a callback
(which the validation would call) OR a query, which would need to fail
as soon as it's run. So, the only feasible way is to change the
method, as well as the query object (to make it so that it will fail
with the right error as soon as it's executed).

Working on it. Aiming at finishing this morning!

Merc.