[SERVER-261] $set issue with dots Created: 26/Aug/09  Updated: 12/Jul/16  Resolved: 16/Jan/10

Status: Closed
Project: Core Server
Component/s: Write Ops
Affects Version/s: None
Fix Version/s: 1.3.1

Type: Bug Priority: Minor - P4
Reporter: Eliot Horowitz (Inactive) Assignee: Eliot Horowitz (Inactive)
Resolution: Done Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Participants:

 Description   

test in jstests/set1.js



 Comments   
Comment by auto [ 16/Jan/10 ]

Author:

{'name': 'Eliot Horowitz', 'email': 'eliot@10gen.com'}

Message: check for bad $set where set to an object with a . SERVER-261
http://github.com/mongodb/mongo/commit/53b5dae92e8c045f6aa94add14dc0564ee9ee6e9

Comment by auto [ 14/Jan/10 ]

Author:

{'name': 'Eliot Horowitz', 'email': 'eliot@10gen.com'}

Message: cleaning test for SERVER-261
http://github.com/mongodb/mongo/commit/2ca97f66235cdd348bc470c449c1468e61c56b86

Comment by Eliot Horowitz (Inactive) [ 09/Dec/09 ]

makingn a lot of changes to this code - so dont think that makes too much sense to do right now

Comment by Aaron Staple [ 09/Dec/09 ]

I'm going to work on adding dot checking when objcheck is enabled.

Comment by Aaron Staple [ 10/Sep/09 ]

Just to clarify: I'm deferring to others on the question of validation w/o --objcheck, but I'm happy to do the implementation myself.

Comment by Aaron Staple [ 10/Sep/09 ]

Let's see what Dwight thinks...

Comment by Eliot Horowitz (Inactive) [ 09/Sep/09 ]

yeah - though i still wonder if we should check this case:
t.update(

{ _id : 1 }

, { $set : { a :

{ 'b.c' : 'data'}

}});
even with objcheck off b/c it might be a common mistake.

i'll look into it.

Comment by Aaron Staple [ 09/Sep/09 ]

That's already implemented, so maybe --objcheck validation is all that remains for this bug?

Comment by Eliot Horowitz (Inactive) [ 09/Sep/09 ]

ugh, yeah, your'e right

so....
t.update( {_id:1}, {$set:{'a.dot':'data'}} );
start {} end : { a :

{ dot : "data" }

}
start { a :

{ b : 1 }

} end : { a :

{ b : 1 , dot : "data" }

}

t.update( {_id:1}, {$set:{a:

{dot:'data'}

}} );
start {} end : { a :

{ dot : "data" }

}
start { a :

{ b : 1 }

} end : { a :

{ dot : "data" }

}

Comment by Aaron Staple [ 09/Sep/09 ]

Ok, so is the suggestion that we should not allow $set to be used to explicitly set a field to an object value? For example, would the following be prohibited?

t.update( {_id:1}, {$set:{a:

{dot:'data',bot:'beta'}

}} );
t.update( {_id:1}, {$set:{a:{}}} );

Requiring the client to do
t.update( {_id:1}, {$set:{'a.dot':'data','a.bot':'beta'}} );
to get the desired behavior my first case above seems like a cumbersome interface, and generating an object from the 'a.dot', 'a.bot', etc representation wastes cpu.

Comment by Eliot Horowitz (Inactive) [ 08/Sep/09 ]

set1.js is invalid now - and my above example was sloppy

but actually, i think we really need to do is make:
t.update( {_id:1}, {$set:{a:

{dot:'data'}

}} );
illegal b/c its confusing

you should have to do:
t.update( {_id:1}, {$set:{'a.dot':'data'}} );

Comment by Aaron Staple [ 08/Sep/09 ]

Looking at the set1.js test, it seems that the desired output is actually:

{_id:1,emb:{a:

{dot:'data'}

}}

I argue that if you want the update to produce this your command should be
t.update( {_id:1}, {$set:{'emb.a':

{dot:'data'}

}} ); or t.update( {_id:1}, {$set:{'emb.a.dot':'data'}} );

and not

t.update( {_id:1}, {$set:{emb:

{'a.dot':'data'}

}} ), which would be invalid under --objcheck.

Comment by Aaron Staple [ 08/Sep/09 ]

Ok, I don't really understand what 'emb' is supposed to be in your examples. Running your first example I get the following:

t.update( {_id:1}, {$set:{emb:

{'a.dot':'data'}

}} );
> t.findOne();
{"_id" : 1 , "emb" : {"a.dot" : "data"}}

which is bad and I think should be validated with objcheck only.

You can get the behavior intended with either of the following:
t.update( {_id:1}, {$set:{'a.dot':'data'}} );
t.update( {_id:1}, {$set:{a:

{dot:'data'}

}} );

I don't understand why the 'emb' is placed in your examples or how come it is supposed to be absent from the desired output.

Comment by Eliot Horowitz (Inactive) [ 07/Sep/09 ]

no - that's not quite right

if you start with

{ _id : 1 }

after: update(

{ _id : 1 }

, { $set : { emb :

{ 'a.dot' : 'data'}

}});
you should have
{ _id : 1 , a :

{ dot : "data" }

}
after: update(

{ _id : 1 }

, { $set : { emb :

{ 'a.dot2' : 'data'}

}});
you should have
{ _id : 1 , a :

{ dot : "data" , dot2 : 'data' }

}

this: t.update( {}, {$set:{'b.c':

{'e.f':1}

}} )
should generate:
{ b : { c :

{ "e.f" : 1 }

} }
which is an illegal value, but the db should not check for that unless --objcheck is on

Comment by Aaron Staple [ 07/Sep/09 ]

Are you trying to suggest that the syntax t.update( {}, {$set:{'b.c':

{'e.f':1}

}} ) should be allowed, and that the implementation would add the subobject {b:{c:{e:{f:1}}}} ? That seems to me like really unintuitive syntax, and implementing it certainly wouldn't be a performance improvement over just auditing the 'e.f', unless you're trying to avoid driver level auditing of modifiers for some reason while keeping it for other updates and inserts. Also, why would we support this syntax for a modifier update but not on an insert?

If this is not what you're suggesting, I'm sorry but you're going to have to be more precise about what you want.

Comment by Eliot Horowitz (Inactive) [ 03/Sep/09 ]

right - so we just need to handle the creation of sub-object cast

Comment by Aaron Staple [ 03/Sep/09 ]

I think what was meant in the previous comment is that without "a." the $set works correctly, with "a." an illegal value is inserted. This is what I observe empirically.

All that's happening here is that the element named "emb" is inserted into the matching object. If that element is to be a subobject, there is no validation performed on the provided subobject. The same is true if we attempt to $push an object.

Since the value of the inserted element is to become part of a mongo document, it must obey the mongo document syntax. Clearly the element value

{'a.dot':'data'}

does not obey the mongo document syntax. To me it seems inconsistent to always validate document syntax when modifiers are employed but not when an object is inserted. Why not conditionally validate modifier elements at the driver level and / or with --objcheck instead?

This would require special casing modifiers in the driver level syntax check, but I believe that is already necessary. If you issue update t.update( {}, {a:{'b.c':1}} ) the syntax is invalid, but if you issue update t.update( {}, {$set:{'b.c':1}} ) the update is valid. However, as I've mentioned, t.update( {}, {$set:{'b.c':

{'e.f':1}

}} ) should not be valid.

Comment by Eliot Horowitz (Inactive) [ 03/Sep/09 ]

the problem is that:
t.update(

{ _id : 1 }

, { $set : { emb :

{ 'a.dot' : 'data'}

}});
does something very different if a is there or not.
if its there, it works correctly
it its not - it does something that creates an illegal value
so the sytnax is correct, so can't be blocked at the driver level

this is just a special case of $set that should be smarter

Comment by Dwight Merriman [ 03/Sep/09 ]

we should check for dots when --objcheck is enabled. i think then this can be closed. drivers checking is a good idea.

Comment by Wouter [ 01/Sep/09 ]

Maybe the driver should validate field names?!

Comment by Aaron Staple [ 31/Aug/09 ]

Our BSON "spec" (wiki page) states that dots aren't allowed in document field names, since we don't validate BSON in general not sure if we want to validate this case specifically.

Comment by Aaron Staple [ 31/Aug/09 ]

mongod itself doesn't check inserted objects for dots, but the js shell does in DBCollection.prototype._validateForStorage

client().insert( ns(), BSON( "a.b" << 5 ) );
log() << client().findOne( ns(), BSONObj() ) << endl;

> Sun Aug 30 21:21:43

{ _id: ObjId(4a9b4fd73d3ab47648ddb918), a.b: 5 }

Do we want validation for this $set case there as well?

Comment by Eliot Horowitz (Inactive) [ 26/Aug/09 ]

Yeah - it definitely shouldn't allow a field with a dot in the name.

I think failing is probably the right thing...

Comment by Michael Dirolf [ 26/Aug/09 ]

The current behavior shown in set1.js should be illegal - there should never be a way to get a document into the database that has '.' in a key name - leads to issues with queries.

My thought is that the update should fail in the first case? If not it shouldn't update the document the way it is now.

Generated at Thu Feb 08 02:53:33 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.