SERVER-4776 only use numeric comparison in updates when applying modi…

…fiers to an array

jstests/set7.js

@@ -39,6 +39,17 @@ t.update( {}, {$set:{"a.f":1}} );
 assert( db.getLastError() );
 assert.eq( [], t.findOne().a );
 
+// Test requiring proper ordering of multiple mods.
+t.drop();
+t.save( {a:[0,1,2,3,4,5,6,7,8,9,10]} );
+t.update( {}, {$set:{"a.11":11,"a.2":-2}} );
+assert.eq( [0,1,-2,3,4,5,6,7,8,9,10,11], t.findOne().a );
+
+// Test upsert case
+t.drop();
+t.update( {a:[0,1,2,3,4,5,6,7,8,9,10]}, {$set:{"a.11":11} }, true );
+assert.eq( [0,1,2,3,4,5,6,7,8,9,10,11], t.findOne().a );
+
 // SERVER-3750
 t.drop();
 t.save( {a:[]} );

jstests/update6.js

@@ -10,7 +10,7 @@ assert.eq( "c,d" , Object.keySet( t.findOne().b ).toString() , "B" );
 
 t.update( { a : 1 } , { $inc : { "b.0e" : 1 } } );
 assert.eq( 1 , t.findOne().b["0e"] , "C" );
-assert.eq( "c,d,0e" , Object.keySet( t.findOne().b ).toString() , "D" );
+assert.eq( "0e,c,d" , Object.keySet( t.findOne().b ).toString() , "D" );
 
 // -----
 

jstests/updatee.js

@@ -29,7 +29,7 @@ t.update({"profile-id" : "test"}, {$set: {"actual.02": "v4"}});
 
 q = t.findOne();
 assert.eq(q.actual["02"], "v4", "A4");
-assert(!q.actual["002"], "A5");
+assert.eq(q.actual["002"], "val4", "A5");
 
 t.update({"_id" : 1}, {$set : {"actual.2139043290148390248219423941.b" : 4}});
 q = t.findOne();
@@ -51,12 +51,12 @@ assert.eq(q.actual["000"], "val000", "A8 zeros");
 t.update({"_id" : 1}, {$set : {"actual.00" : "val00"}});
 q = t.findOne();
 assert.eq(q.actual["00"], "val00", "A8 00");
-assert(!q.actual["000"], "A9");
+assert.eq(q.actual["000"], "val000", "A9");
 
 t.update({"_id" : 1}, {$set : {"actual.000" : "val000"}});
 q = t.findOne();
 assert.eq(q.actual["000"], "val000", "A9");
-assert(!q.actual["00"], "A10");
+assert.eq(q.actual["00"], "val00", "A10");
 
 t.update({"_id" : 1}, {$set : {"actual.01" : "val01"}});
 q = t.findOne();

jstests/updatek.js

@@ -0,0 +1,13 @@
+// Test modifier operations on numerically equivalent string field names.  SERVER-4776
+
+t = db.jstests_updatek;
+
+t.drop();
+t.save( { _id:0, '1':{}, '01':{} } );
+t.update( {}, { $set:{ '1.b':1, '1.c':2 } } );
+assert.eq( { '01':{}, '1':{ b:1, c:2 }, _id:0 }, t.findOne() );
+
+t.drop();
+t.save( { _id:0, '1':{}, '01':{} } );
+t.update( {}, { $set:{ '1.b':1, '01.c':2 } } );
+assert.eq( { '01':{ c:2 }, '1':{ b:1 }, _id:0 }, t.findOne() );

src/mongo/bson/bsonmisc.h

@@ -48,7 +48,9 @@ namespace mongo {
         RIGHT_SUBFIELD = 2
     };
 
-    FieldCompareResult compareDottedFieldNames( const std::string& l , const std::string& r );
+    class LexNumCmp;
+    FieldCompareResult compareDottedFieldNames( const std::string& l , const std::string& r ,
+                                               const LexNumCmp& cmp );
 
     /** Use BSON macro to build a BSONObj from a stream
 

src/mongo/bson/bsonobjiterator.h

@@ -82,11 +82,10 @@ namespace mongo {
         const char* _theend;
     };
 
-    class BSONObjIteratorSorted {
+    /** Base class implementing ordered iteration through BSONElements. */
+    class BSONIteratorSorted {
     public:
-        BSONObjIteratorSorted( const BSONObj& o );
-
-        ~BSONObjIteratorSorted() {
+        ~BSONIteratorSorted() {
             assert( _fields );
             delete[] _fields;
             _fields = 0;
@@ -103,15 +102,32 @@ namespace mongo {
             return BSONElement();
         }
 
-    private:
-
+    protected:
         class ElementFieldCmp;
+        BSONIteratorSorted( const BSONObj &o, const ElementFieldCmp &cmp );
 
+    private:
         const char ** _fields;
         int _nfields;
         int _cur;
     };
 
+    /** Provides iteration of a BSONObj's BSONElements in lexical field order. */
+    class BSONObjIteratorSorted : public BSONIteratorSorted {
+    public:
+        BSONObjIteratorSorted( const BSONObj &object );
+    };
+
+    /**
+     * Provides iteration of a BSONArray's BSONElements in numeric field order.
+     * The elements of a bson array should always be numerically ordered by field name, but this
+     * implementation re-sorts them anyway.
+     */
+    class BSONArrayIteratorSorted : public BSONIteratorSorted {
+    public:
+        BSONArrayIteratorSorted( const BSONArray &array );
+    };
+
     /** transform a BSON array into a vector of BSONElements.
         we match array # positions with their vector position, and ignore
         any fields with non-numeric field names.

src/mongo/db/indexkey.cpp

@@ -22,6 +22,7 @@
 #include "btree.h"
 #include "ops/query.h"
 #include "background.h"
+#include "../util/stringutils.h"
 #include "../util/text.h"
 
 namespace mongo {
@@ -423,7 +424,8 @@ namespace mongo {
             BSONObjIterator y(b);
             while ( y.more() ) {
                 BSONElement f = y.next();
-                FieldCompareResult res = compareDottedFieldNames( e.fieldName() , f.fieldName() );
+                FieldCompareResult res = compareDottedFieldNames( e.fieldName() , f.fieldName() ,
+                                                                 LexNumCmp( false ) );
                 if ( res == SAME || res == LEFT_SUBFIELD || res == RIGHT_SUBFIELD )
                     return true;
             }

src/mongo/db/jsobj.cpp

@@ -334,7 +334,8 @@ namespace mongo {
         return fe.getGtLtOp();
     }
 
-    FieldCompareResult compareDottedFieldNames( const string& l , const string& r ) {
+    FieldCompareResult compareDottedFieldNames( const string& l , const string& r ,
+                                               const LexNumCmp& cmp ) {
         static int maxLoops = 1024 * 1024;
 
         size_t lstart = 0;
@@ -351,7 +352,7 @@ namespace mongo {
             const string& c = l.substr( lstart , lend - lstart );
             const string& d = r.substr( rstart , rend - rstart );
 
-            int x = LexNumCmp::cmp( c.c_str(), d.c_str() );
+            int x = cmp.cmp( c.c_str(), d.c_str() );
 
             if ( x < 0 )
                 return LEFT_BEFORE;
@@ -1199,21 +1200,26 @@ namespace mongo {
         return !(l.more() || r.more()); // false if lhs and rhs have diff nFields()
     }
 
-    /** Compare two bson elements, provided as const char *'s, by their field names. */
-    class BSONObjIteratorSorted::ElementFieldCmp {
+    /** Compare two bson elements, provided as const char *'s, by field name. */
+    class BSONIteratorSorted::ElementFieldCmp {
     public:
+        ElementFieldCmp( bool isArray );
         bool operator()( const char *s1, const char *s2 ) const;
     private:
         LexNumCmp _cmp;
     };
 
-    bool BSONObjIteratorSorted::ElementFieldCmp::operator()( const char *s1, const char *s2 )
+    BSONIteratorSorted::ElementFieldCmp::ElementFieldCmp( bool isArray ) :
+    _cmp( !isArray ) {
+    }
+
+    bool BSONIteratorSorted::ElementFieldCmp::operator()( const char *s1, const char *s2 )
     const {
-        // Skip type byte and compare field names.
+        // Skip the type byte and compare field names.
         return _cmp( s1 + 1, s2 + 1 );
     }        
 
-    BSONObjIteratorSorted::BSONObjIteratorSorted( const BSONObj& o ) {
+    BSONIteratorSorted::BSONIteratorSorted( const BSONObj &o, const ElementFieldCmp &cmp ) {
         _nfields = o.nFields();
         _fields = new const char*[_nfields];
         int x = 0;
@@ -1223,10 +1229,17 @@ namespace mongo {
             assert( _fields[x-1] );
         }
         assert( x == _nfields );
-        ElementFieldCmp cmp;
         std::sort( _fields , _fields + _nfields , cmp );
         _cur = 0;
     }
+
+    BSONObjIteratorSorted::BSONObjIteratorSorted( const BSONObj &object ) :
+    BSONIteratorSorted( object, ElementFieldCmp( false ) ) {
+    }
+
+    BSONArrayIteratorSorted::BSONArrayIteratorSorted( const BSONArray &array ) :
+    BSONIteratorSorted( array, ElementFieldCmp( true ) ) {
+    }
 
     bool BSONObjBuilder::appendAsNumber( const StringData& fieldName , const string& data ) {
         if ( data.size() == 0 || data == "-" || data == ".")

src/mongo/db/ops/update.cpp

@@ -605,7 +605,8 @@ namespace mongo {
     }
 
     template< class Builder >
-    void ModSetState::_appendNewFromMods( const string& root , ModState& m , Builder& b , set<string>& onedownseen ) {
+    void ModSetState::_appendNewFromMods( const string& root , ModState& m , Builder& b ,
+                                         set<string>& onedownseen ) {
         const char * temp = m.fieldName();
         temp += root.size();
         const char * dot = strchr( temp , '.' );
@@ -616,13 +617,13 @@ namespace mongo {
                 return;
             onedownseen.insert( nf );
             BSONObjBuilder bb ( b.subobjStart( nf ) );
-            createNewFromMods( nr , bb , BSONObj() ); // don't infer an array from name
+            // Always insert an object, even if the field name is numeric.
+            createNewObjFromMods( nr , bb , BSONObj() );
             bb.done();
         }
         else {
             appendNewFromMod( m , b );
         }
-
     }
 
     bool ModSetState::duplicateFieldName( const BSONElement &a, const BSONElement &b ) {
@@ -633,16 +634,40 @@ namespace mongo {
         str::equals( a.fieldName(), b.fieldName() );
     }
 
-    template< class Builder >
-    void ModSetState::createNewFromMods( const string& root , Builder& b , const BSONObj &obj ) {
-        DEBUGUPDATE( "\t\t createNewFromMods root: " << root );
-        BSONObjIteratorSorted es( obj );
-        BSONElement e = es.next();
-
-        ModStateHolder::iterator m = _mods.lower_bound( root );
+    ModSetState::ModStateRange ModSetState::modsForRoot( const string &root ) {
+        ModStateHolder::iterator mstart = _mods.lower_bound( root );
         StringBuilder buf;
         buf << root << (char)255;
         ModStateHolder::iterator mend = _mods.lower_bound( buf.str() );
+        return make_pair( mstart, mend );
+    }
+
+    void ModSetState::createNewObjFromMods( const string &root , BSONObjBuilder &b ,
+                                           const BSONObj &obj ) {
+        BSONObjIteratorSorted es( obj );
+        createNewFromMods( root, b, es, modsForRoot( root ), LexNumCmp( true ) );
+    }
+
+    void ModSetState::createNewArrayFromMods( const string &root , BSONArrayBuilder &b ,
+                                             const BSONArray &arr ) {
+        BSONArrayIteratorSorted es( arr );
+        ModStateRange objectOrderedRange = modsForRoot( root );
+        ModStateHolder arrayOrderedMods( LexNumCmp( false ) );
+        arrayOrderedMods.insert( objectOrderedRange.first, objectOrderedRange.second );
+        ModStateRange arrayOrderedRange( arrayOrderedMods.begin(), arrayOrderedMods.end() );
+        createNewFromMods( root, b, es, arrayOrderedRange, LexNumCmp( false ) );
+    }
+
+    template< class Builder >
+    void ModSetState::createNewFromMods( const string& root , Builder& b ,
+                                        BSONIteratorSorted& es ,
+                                        const ModStateRange& modRange ,
+                                        const LexNumCmp& lexNumCmp ) {
+
+        DEBUGUPDATE( "\t\t createNewFromMods root: " << root );
+        ModStateHolder::iterator m = modRange.first;
+        const ModStateHolder::const_iterator mend = modRange.second;
+        BSONElement e = es.next();
 
         set<string> onedownseen;
         BSONElement prevE;
@@ -658,7 +683,8 @@ namespace mongo {
             prevE = e;
 
             string field = root + e.fieldName();
-            FieldCompareResult cmp = compareDottedFieldNames( m->second.m->fieldName , field );
+            FieldCompareResult cmp = compareDottedFieldNames( m->second->m->fieldName , field ,
+                                                             lexNumCmp );
 
             DEBUGUPDATE( "\t\t\t field:" << field << "\t mod:" << m->second->m->fieldName << "\t cmp:" << cmp << "\t short: " << e.fieldName() );
 
@@ -671,13 +697,13 @@ namespace mongo {
                     if ( e.type() == Object ) {
                         BSONObjBuilder bb( b.subobjStart( e.fieldName() ) );
                         stringstream nr; nr << root << e.fieldName() << ".";
-                        createNewFromMods( nr.str() , bb , e.embeddedObject() );
+                        createNewObjFromMods( nr.str() , bb , e.Obj() );
                         bb.done();
                     }
                     else {
                         BSONArrayBuilder ba( b.subarrayStart( e.fieldName() ) );
                         stringstream nr; nr << root << e.fieldName() << ".";
-                        createNewFromMods( nr.str() , ba , e.embeddedObject() );
+                        createNewArrayFromMods( nr.str() , ba , BSONArray( e.embeddedObject() ) );
                         ba.done();
                     }
                     // inc both as we handled both
@@ -730,7 +756,7 @@ namespace mongo {
 
     BSONObj ModSetState::createNewFromMods() {
         BSONObjBuilder b( (int)(_obj.objsize() * 1.1) );
-        createNewFromMods( "" , b , _obj );
+        createNewObjFromMods( "" , b , _obj );
         return _newFromMods = b.obj();
     }
 

src/mongo/db/ops/update.h

@@ -419,7 +419,8 @@ namespace mongo {
 
             ModHolder::const_iterator start = _mods.lower_bound(fieldName.substr(0,idx));
             for ( ; start != _mods.end(); start++ ) {
-                FieldCompareResult r = compareDottedFieldNames( fieldName , start->first );
+                FieldCompareResult r = compareDottedFieldNames( fieldName , start->first ,
+                                                               LexNumCmp( true ) );
                 switch ( r ) {
                 case LEFT_SUBFIELD: return true;
                 case LEFT_BEFORE: return false;
@@ -535,7 +536,7 @@ namespace mongo {
         BSONObj _newFromMods; // keep this data alive, as oplog generation may depend on it
 
         ModSetState( const BSONObj& obj )
-            : _obj( obj ) , _inPlacePossible(true) {
+            : _obj( obj ) , _mods( LexNumCmp( true ) ) , _inPlacePossible(true) {
         }
 
         /**
@@ -547,8 +548,15 @@ namespace mongo {
             return _inPlacePossible;
         }
 
+        ModStateRange modsForRoot( const string &root );
+
+        void createNewObjFromMods( const string &root, BSONObjBuilder &b, const BSONObj &obj );
+        void createNewArrayFromMods( const string &root, BSONArrayBuilder &b,
+                                    const BSONArray &arr );
+
         template< class Builder >
-        void createNewFromMods( const string& root , Builder& b , const BSONObj &obj );
+        void createNewFromMods( const string& root , Builder& b , BSONIteratorSorted& es ,
+                               const ModStateRange& modRange , const LexNumCmp& lexNumCmp );
 
         template< class Builder >
         void _appendNewFromMods( const string& root , ModState& m , Builder& b , set<string>& onedownseen );

src/mongo/dbtests/basictests.cpp

@@ -334,8 +334,10 @@ namespace BasicTests {
 
     class LexNumCmp {
     public:
-        static void assertCmp( int expected, const char *s1, const char *s2 ) {
-            mongo::LexNumCmp cmp;
+        static void assertCmp( int expected, const char *s1, const char *s2,
+                              bool lexOnly = false ) {
+            mongo::LexNumCmp cmp( lexOnly );
+            ASSERT_EQUALS( expected, cmp.cmp( s1, s2, lexOnly ) );
             ASSERT_EQUALS( expected, cmp.cmp( s1, s2 ) );
             ASSERT_EQUALS( expected < 0, cmp( s1, s2 ) );
             ASSERT_EQUALS( expected < 0, cmp( string( s1 ), string( s2 ) ) );
@@ -435,6 +437,16 @@ namespace BasicTests {
             assertCmp( 0, "ac.t", "ac.t" );            
         }
     };
+
+    class LexNumCmpLexOnly : public LexNumCmp {
+    public:
+        void run() {
+            assertCmp( -1, "0", "00", true );
+            assertCmp( 1, "1", "01", true );
+            assertCmp( -1, "1", "11", true );
+            assertCmp( 1, "2", "11", true );
+        }
+    };
 
     class DatabaseValidNames {
     public:
@@ -674,6 +686,7 @@ namespace BasicTests {
 
             add< ArrayTests::basic1 >();
             add< LexNumCmp >();
+            add< LexNumCmpLexOnly >();
 
             add< DatabaseValidNames >();
             add< DatabaseOwnsNS >();