SERVER-6669 SERVER-4713 Validate that a positional match is found before applying an update mod with the positional operator.

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 >();