diff options
author | aaron <aaron@10gen.com> | 2012-12-22 13:34:06 -0800 |
---|---|---|
committer | aaron <aaron@10gen.com> | 2012-12-27 12:20:18 -0800 |
commit | f498a5c235e4da255d9cfea336b66b9c30403706 (patch) | |
tree | 169f3cccc648f1e29ca740a1781cb1b75831221c | |
parent | 5b24436ec22e8925d921a5f070e5c46132915f6c (diff) | |
download | mongo-f498a5c235e4da255d9cfea336b66b9c30403706.tar.gz |
SERVER-6669 SERVER-4713 Validate that a positional match is found before applying an update mod with the positional operator.
-rw-r--r-- | jstests/multiVersion/multi_version_sharding_passthrough.js | 1 | ||||
-rw-r--r-- | jstests/updatel.js | 48 | ||||
-rw-r--r-- | src/mongo/SConscript | 1 | ||||
-rw-r--r-- | src/mongo/db/ops/update.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/ops/update_internal.cpp | 19 | ||||
-rw-r--r-- | src/mongo/dbtests/updatetests.cpp | 76 |
6 files changed, 145 insertions, 4 deletions
diff --git a/jstests/multiVersion/multi_version_sharding_passthrough.js b/jstests/multiVersion/multi_version_sharding_passthrough.js index cf3c4f4b245..e35447f56b0 100644 --- a/jstests/multiVersion/multi_version_sharding_passthrough.js +++ b/jstests/multiVersion/multi_version_sharding_passthrough.js @@ -319,6 +319,7 @@ var v22Only = [ /^all3$/, /^updatei$/, /^updatej$/, /^updatek$/, + /^updatel$/, /^use_power_of_2$/, /^useindexonobjgtlt$/, /^where4$/, diff --git a/jstests/updatel.js b/jstests/updatel.js new file mode 100644 index 00000000000..b4a80061a2b --- /dev/null +++ b/jstests/updatel.js @@ -0,0 +1,48 @@ +// The positional operator allows an update modifier field path to contain a sentinel ('$') path +// part that is replaced with the numeric position of an array element matched by the update's query +// spec. <http://docs.mongodb.org/manual/reference/operators/#_S_> + +// If no array element position from a query is available to substitute for the positional operator +// setinel ('$'), the update fails with an error. SERVER-6669 SERVER-4713 + +t = db.jstests_updatel; +t.drop(); + + + +// The collection is empty, forcing an upsert. In this case the query has no array position match +// to substiture for the positional operator. SERVER-4713 +t.update( {}, { $set:{ 'a.$.b':1 } }, true ); +assert( db.getLastError(), "An error is reported." ); +assert.eq( 0, t.count(), "No upsert occurred." ); + + + +// Save a document to the collection so it is no longer empty. +t.save( { _id:0 } ); + +// Now, with an existing document, trigger an update rather than an upsert. The query has no array +// position match to substiture for the positional operator. SERVER-6669 +t.update( {}, { $set:{ 'a.$.b':1 } } ); +assert( db.getLastError(), "An error is reported." ); +assert.eq( [ { _id:0 } ], t.find().toArray(), "No update occurred." ); + + + +// Now, try with an update by _id (without a query array match). +t.update( { _id:0 }, { $set:{ 'a.$.b':1 } } ); +assert( db.getLastError(), "An error is reported." ); +assert.eq( [ { _id:0 } ], t.find().toArray(), "No update occurred." ); + + + +// Seed the collection with a document suitable for the following check. +t.remove(); +t.save( { _id:0, a:[ { b:{ c:1 } } ] } ); + +// Now, attempt to apply an update with two nested positional operators. There is a positional +// query match for the first positional operator but not the second. Note that dollar sign +// substitution for multiple positional opertors is not implemented (SERVER-831). +t.update( { 'a.b.c':1 }, { $set:{ 'a.$.b.$.c':2 } } ); +assert( db.getLastError(), "An error is reported" ); +assert.eq( [ { _id:0, a:[ { b:{ c:1 } } ] } ], t.find().toArray(), "No update occurred." ); diff --git a/src/mongo/SConscript b/src/mongo/SConscript index 66a3cfe87d3..768bb87e336 100644 --- a/src/mongo/SConscript +++ b/src/mongo/SConscript @@ -540,6 +540,7 @@ env.StaticLibrary("serveronly", serverOnlyFiles, LIBDEPS=["coreshard", "db/auth/authmongod", "db/fts/ftsmongod", + "db/ops/update", "dbcmdline", "defaultversion", "geoparser", diff --git a/src/mongo/db/ops/update.cpp b/src/mongo/db/ops/update.cpp index c3037a1da98..c3894131c23 100644 --- a/src/mongo/db/ops/update.cpp +++ b/src/mongo/db/ops/update.cpp @@ -268,10 +268,6 @@ namespace mongo { debug.nscanned++; if ( mods.get() && mods->hasDynamicArray() ) { - // The Cursor must have a Matcher to record an elemMatchKey. But currently - // a modifier on a dynamic array field may be applied even if there is no - // elemMatchKey, so a matcher cannot be required. - //verify( c->matcher() ); details.requestElemMatchKey(); } diff --git a/src/mongo/db/ops/update_internal.cpp b/src/mongo/db/ops/update_internal.cpp index 397e810e8df..c02bba93ccf 100644 --- a/src/mongo/db/ops/update_internal.cpp +++ b/src/mongo/db/ops/update_internal.cpp @@ -21,6 +21,7 @@ #include <algorithm> // for max #include "mongo/db/oplog.h" +#include "mongo/db/ops/field_ref.h" #include "mongo/db/jsobjmanipulator.h" #include "mongo/db/pdfile.h" #include "mongo/util/mongoutils/str.h" @@ -539,6 +540,24 @@ namespace mongo { ModState& ms = *mss->_mods[i->first]; const Mod& m = i->second; + + // Check for any positional operators that have not been replaced with a numeric field + // name (from a query match element). + // Only perform this positional operator validation in 'strictApply' mode. When + // replicating from a legacy primary that does not implement this validation, the + // secondary bypasses validation and remains consistent with the primary. + if ( m.strictApply ) { + FieldRef fieldRef; + fieldRef.parse( m.fieldName ); + StringData positionalOpField( "$" ); + for( size_t i = 0; i < fieldRef.numParts(); ++i ) { + uassert( 16650, + "Cannot apply the positional operator without a corresponding query " + "field containing an array.", + fieldRef.getPart( i ).compare( positionalOpField ) != 0 ); + } + } + BSONElement e = obj.getFieldDotted(m.fieldName); ms.m = &m; diff --git a/src/mongo/dbtests/updatetests.cpp b/src/mongo/dbtests/updatetests.cpp index ff7c44264e5..0cf177ace91 100644 --- a/src/mongo/dbtests/updatetests.cpp +++ b/src/mongo/dbtests/updatetests.cpp @@ -2157,6 +2157,77 @@ namespace UpdateTests { modSetState->getOpLogRewrite() ); } }; + + class PositionalWithoutElemMatchKey { + public: + void run() { + BSONObj querySpec = BSONObj(); + BSONObj modSpec = BSON( "$set" << BSON( "a.$" << 1 ) ); + ModSet modSet( modSpec ); + + // A positional operator must be replaced with an array index before calling + // prepare(). + ASSERT_THROWS( modSet.prepare( querySpec ), UserException ); + } + }; + + class PositionalWithoutNestedElemMatchKey { + public: + void run() { + BSONObj querySpec = BSONObj(); + BSONObj modSpec = BSON( "$set" << BSON( "a.b.c.$.e.f" << 1 ) ); + ModSet modSet( modSpec ); + + // A positional operator must be replaced with an array index before calling + // prepare(). + ASSERT_THROWS( modSet.prepare( querySpec ), UserException ); + } + }; + + class DbrefPassesPositionalValidation { + public: + void run() { + BSONObj querySpec = BSONObj(); + BSONObj modSpec = BSON( "$set" << BSON( "a.$ref" << "foo" << "a.$id" << 0 ) ); + ModSet modSet( modSpec ); + + // A positional operator must be replaced with an array index before calling + // prepare(), but $ prefixed fields encoding dbrefs are allowed. + modSet.prepare( querySpec ); // Does not throw. + } + }; + + class NoPositionalValidationOnReplication { + public: + void run() { + BSONObj querySpec = BSONObj(); + BSONObj modSpec = BSON( "$set" << BSON( "a.$" << 1 ) ); + ModSet modSet( modSpec, set<string>(), NULL, true ); + + // No positional operator validation is performed if a ModSet is 'forReplication'. + modSet.prepare( querySpec ); // Does not throw. + } + }; + + class NoPositionalValidationOnPartialFixedArrayReplication { + public: + void run() { + BSONObj querySpec = BSONObj( BSON( "a.b" << 1 ) ); + BSONObj modSpec = BSON( "$set" << BSON( "a.$.b.$" << 1 ) ); + ModSet modSet( modSpec, set<string>(), NULL, true ); + + // Attempt to fix the positional operator fields. + scoped_ptr<ModSet> fixedMods( modSet.fixDynamicArray( "0" ) ); + + // The first positional field is replaced, but the second is not (until SERVER-831 + // is implemented). + ASSERT( fixedMods->haveModForField( "a.0.b.$" ) ); + + // No positional operator validation is performed if a ModSet is 'forReplication', + // even after an attempt to fix the positional operator fields. + fixedMods->prepare( querySpec ); // Does not throw. + } + }; }; namespace basic { @@ -2510,6 +2581,11 @@ namespace UpdateTests { // add< ModSetTests::BitRewriteNonExistingField >(); add< ModSetTests::SetIsNotRewritten >(); add< ModSetTests::UnsetIsNotRewritten >(); + add< ModSetTests::PositionalWithoutElemMatchKey >(); + add< ModSetTests::PositionalWithoutNestedElemMatchKey >(); + add< ModSetTests::DbrefPassesPositionalValidation >(); + add< ModSetTests::NoPositionalValidationOnReplication >(); + add< ModSetTests::NoPositionalValidationOnPartialFixedArrayReplication >(); add< basic::inc1 >(); add< basic::inc2 >(); |