[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 . |
| Comment by auto [ 14/Jan/10 ] |
|
Author: {'name': 'Eliot Horowitz', 'email': 'eliot@10gen.com'}Message: cleaning test for |
| 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: , { $set : { a : { 'b.c' : 'data'} }}); 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.... } } end : { a : { b : 1 , dot : "data" }} t.update( {_id:1}, {$set:{a: {dot:'data'}}} ); } } 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'}}} ); Requiring the client to do |
| 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: }} ); you should have to do: |
| 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 }} ); 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'}}} ); which is bad and I think should be validated with objcheck only. You can get the behavior intended with either of the following: }} ); 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'} }}); } , { $set : { emb : { 'a.dot2' : 'data'} }}); } this: t.update( {}, {$set:{'b.c': {'e.f':1}}} ) } } |
| 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: , { $set : { emb : { 'a.dot' : 'data'} }}); 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 ) ); > 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. |