SERVER-7186 Addressed idempotency violations in update operators.

5 files changed, 632 insertions, 103 deletions

diff --git a/src/mongo/db/ops/update.cpp b/src/mongo/db/ops/update.cpp
index 8f9abf5ba7c..30e57e38e43 100644
--- a/src/mongo/db/ops/update.cpp
+++ b/src/mongo/db/ops/update.cpp
@@ -102,21 +102,8 @@ namespace mongo {
                 DEV verify( mods->size() );
 
                 BSONObj pattern = patternOrig;
-                if ( mss->haveArrayDepMod() ) {
-                    BSONObjBuilder patternBuilder;
-                    patternBuilder.appendElements( pattern );
-                    mss->appendSizeSpecForArrayDepMods( patternBuilder );
-                    pattern = patternBuilder.obj();
-                }
-
-                if( mss->needOpLogRewrite() ) {
-                    DEBUGUPDATE( "\t rewrite update: " << mss->getOpLogRewrite() );
-                    logOp("u", ns, mss->getOpLogRewrite() ,
-                          &pattern, 0, fromMigrate );
-                }
-                else {
-                    logOp("u", ns, updateobj, &pattern, 0, fromMigrate );
-                }
+                DEBUGUPDATE( "\t rewrite update: " << mss->getOpLogRewrite() );
+                logOp("u", ns, mss->getOpLogRewrite() , &pattern, 0, fromMigrate );
             }
             return UpdateResult( 1 , 1 , 1 , BSONObj() );
         } // end $operator update
@@ -336,13 +323,13 @@ namespace mongo {
                     const BSONObj& onDisk = loc.obj();
 
                     ModSet* useMods = mods.get();
-                    bool forceRewrite = false;
+                    //bool forceRewrite = false;
 
                     auto_ptr<ModSet> mymodset;
                     if ( details.hasElemMatchKey() && mods->hasDynamicArray() ) {
                         useMods = mods->fixDynamicArray( details.elemMatchKey() );
                         mymodset.reset( useMods );
-                        forceRewrite = true;
+                        //forceRewrite = true;
                     }
 
                     auto_ptr<ModSetState> mss = useMods->prepare( onDisk );
@@ -395,21 +382,8 @@ namespace mongo {
                     if ( logop ) {
                         DEV verify( mods->size() );
 
-                        if ( mss->haveArrayDepMod() ) {
-                            BSONObjBuilder patternBuilder;
-                            patternBuilder.appendElements( pattern );
-                            mss->appendSizeSpecForArrayDepMods( patternBuilder );
-                            pattern = patternBuilder.obj();
-                        }
-
-                        if ( forceRewrite || mss->needOpLogRewrite() ) {
-                            DEBUGUPDATE( "\t rewrite update: " << mss->getOpLogRewrite() );
-                            logOp("u", ns, mss->getOpLogRewrite() ,
-                                  &pattern, 0, fromMigrate );
-                        }
-                        else {
-                            logOp("u", ns, updateobj, &pattern, 0, fromMigrate );
-                        }
+                        DEBUGUPDATE( "\t rewrite update: " << mss->getOpLogRewrite() );
+                        logOp("u", ns, mss->getOpLogRewrite() , &pattern, 0, fromMigrate );
                     }
                     numModded++;
                     if ( ! multi )
diff --git a/src/mongo/db/ops/update_internal.cpp b/src/mongo/db/ops/update_internal.cpp
index 69409304598..dccc22e0175 100644
--- a/src/mongo/db/ops/update_internal.cpp
+++ b/src/mongo/db/ops/update_internal.cpp
@@ -98,6 +98,9 @@ namespace mongo {
 
         case INC: {
             appendIncremented( builder , in , ms );
+            // We don't need to "fix" this operation into a $set, for oplog purposes,
+            // here. ModState::appendForOpLog will do that for us. It relies on the new value
+            // being in inc{int,long,double} inside the ModState that wraps around this Mod.
             break;
         }
 
@@ -114,27 +117,23 @@ namespace mongo {
 
         case PUSH: {
             uassert( 10131 ,  "$push can only be applied to an array" , in.type() == Array );
-            BSONObjBuilder bb( builder.subarrayStart( shortFieldName ) );
+            BSONArrayBuilder bb( builder.subarrayStart( shortFieldName ) );
             BSONObjIterator i( in.embeddedObject() );
-            int n=0;
             while ( i.more() ) {
                 bb.append( i.next() );
-                n++;
             }
 
-            ms.pushStartSize = n;
-
-            bb.appendAs( elt ,  bb.numStr( n ) );
-            bb.done();
+            bb.append( elt );
+            ms.fixedOpName = "$set";
+            ms.forceEmptyArray = true;
+            ms.fixedArray = BSONArray(bb.done().getOwned());
             break;
         }
 
         case ADDTOSET: {
             uassert( 12592 ,  "$addToSet can only be applied to an array" , in.type() == Array );
-            BSONObjBuilder bb( builder.subarrayStart( shortFieldName ) );
-
+            BSONArrayBuilder bb( builder.subarrayStart( shortFieldName ) );
             BSONObjIterator i( in.embeddedObject() );
-            int n=0;
 
             if ( isEach() ) {
 
@@ -144,7 +143,6 @@ namespace mongo {
                 while ( i.more() ) {
                     BSONElement cur = i.next();
                     bb.append( cur );
-                    n++;
                     toadd.erase( cur );
                 }
 
@@ -153,7 +151,7 @@ namespace mongo {
                     while ( i.more() ) {
                         BSONElement e = i.next();
                         if ( toadd.count(e) ) {
-                            bb.appendAs( e , BSONObjBuilder::numStr( n++ ) );
+                            bb.append( e );
                             toadd.erase( e );
                         }
                     }
@@ -167,17 +165,18 @@ namespace mongo {
                 while ( i.more() ) {
                     BSONElement cur = i.next();
                     bb.append( cur );
-                    n++;
                     if ( elt.woCompare( cur , false ) == 0 )
                         found = true;
                 }
 
                 if ( ! found )
-                    bb.appendAs( elt ,  bb.numStr( n ) );
+                    bb.append( elt );
 
             }
 
-            bb.done();
+            ms.fixedOpName = "$set";
+            ms.forceEmptyArray = true;
+            ms.fixedArray = BSONArray(bb.done().getOwned());
             break;
         }
 
@@ -187,30 +186,28 @@ namespace mongo {
             uassert( 10132 ,  "$pushAll can only be applied to an array" , in.type() == Array );
             uassert( 10133 ,  "$pushAll has to be passed an array" , elt.type() );
 
-            BSONObjBuilder bb( builder.subarrayStart( shortFieldName ) );
+            BSONArrayBuilder bb( builder.subarrayStart( shortFieldName ) );
 
             BSONObjIterator i( in.embeddedObject() );
-            int n=0;
             while ( i.more() ) {
                 bb.append( i.next() );
-                n++;
             }
 
-            ms.pushStartSize = n;
-
             i = BSONObjIterator( elt.embeddedObject() );
             while ( i.more() ) {
-                bb.appendAs( i.next() , bb.numStr( n++ ) );
+                bb.append( i.next() );
             }
 
-            bb.done();
+            ms.fixedOpName = "$set";
+            ms.forceEmptyArray = true;
+            ms.fixedArray = BSONArray(bb.done().getOwned());
             break;
         }
 
         case PULL:
         case PULL_ALL: {
             uassert( 10134 ,  "$pull/$pullAll can only be applied to an array" , in.type() == Array );
-            BSONObjBuilder bb( builder.subarrayStart( shortFieldName ) );
+            BSONArrayBuilder bb( builder.subarrayStart( shortFieldName ) );
 
             //temporarily record the things to pull. only use this set while 'elt' in scope.
             BSONElementSet toPull;
@@ -221,8 +218,6 @@ namespace mongo {
                 }
             }
 
-            int n = 0;
-
             BSONObjIterator i( in.embeddedObject() );
             while ( i.more() ) {
                 BSONElement e = i.next();
@@ -236,36 +231,36 @@ namespace mongo {
                 }
 
                 if ( allowed )
-                    bb.appendAs( e , bb.numStr( n++ ) );
+                    bb.append( e );
             }
 
-            bb.done();
+            // If this is the last element of the array, then we want to write the empty array to the
+            // oplog.
+            ms.fixedOpName = "$set";
+            ms.forceEmptyArray = true;
+            ms.fixedArray = BSONArray(bb.done().getOwned());
             break;
         }
 
         case POP: {
             uassert( 10135 ,  "$pop can only be applied to an array" , in.type() == Array );
-            BSONObjBuilder bb( builder.subarrayStart( shortFieldName ) );
+            BSONArrayBuilder bb( builder.subarrayStart( shortFieldName ) );
 
-            int n = 0;
 
             BSONObjIterator i( in.embeddedObject() );
             if ( elt.isNumber() && elt.number() < 0 ) {
                 // pop from front
                 if ( i.more() ) {
                     i.next();
-                    n++;
                 }
 
                 while( i.more() ) {
-                    bb.appendAs( i.next() , bb.numStr( n - 1 ) );
-                    n++;
+                    bb.append( i.next() );
                 }
             }
             else {
                 // pop from back
                 while( i.more() ) {
-                    n++;
                     BSONElement arrI = i.next();
                     if ( i.more() ) {
                         bb.append( arrI );
@@ -273,9 +268,9 @@ namespace mongo {
                 }
             }
 
-            ms.pushStartSize = n;
-            verify( ms.pushStartSize == in.embeddedObject().nFields() );
-            bb.done();
+            ms.fixedOpName = "$set";
+            ms.forceEmptyArray = true;
+            ms.fixedArray = BSONArray(bb.done().getOwned());
             break;
         }
 
@@ -311,8 +306,25 @@ namespace mongo {
             }
 
             switch( in.type() ) {
-            case NumberInt: builder.append( shortFieldName , x ); break;
-            case NumberLong: builder.append( shortFieldName , y ); break;
+
+            case NumberInt:
+                builder.append( shortFieldName , x );
+                // By recording the result of the bit manipulation into the ModSet, we'll be
+                // set up so that this $bit operation be "fixed" as a $set of the final result
+                // in the oplog. This will happen in appendForOpLog and what triggers it is
+                // setting the incType in the ModSet that is around this Mod.
+                ms.incType = NumberInt;
+                ms.incint = x;
+                break;
+
+            case NumberLong:
+                // Please see comment on fixing this $bit into a $set for logging purposes in
+                // the NumberInt case.
+                builder.append( shortFieldName , y );
+                ms.incType = NumberLong;
+                ms.inclong = y;
+                break;
+
             default: verify( 0 );
             }
 
@@ -320,10 +332,14 @@ namespace mongo {
         }
 
         case RENAME_FROM: {
+            // We don't need to "fix" this operation into a $set here. ModState::appendForOpLog
+            // will do that for us. It relies on the field name being stored on this Mod.
             break;
         }
 
         case RENAME_TO: {
+            // We don't need to "fix" this operation into a $set here, for the same reason we
+            // didn't either with RENAME_FROM.
             ms.handleRename( builder, shortFieldName );
             break;
         }
@@ -504,6 +520,7 @@ namespace mongo {
     }
 
     void ModState::appendForOpLog( BSONObjBuilder& b ) const {
+        // dontApply logic is deprecated for all but $rename.
         if ( dontApply ) {
             return;
         }
@@ -537,10 +554,20 @@ namespace mongo {
         DEBUGUPDATE( "\t\t\t\t\t appendForOpLog name:" << name << " fixed: " << fixed
                      << " fn: " << m->fieldName );
 
+        if (strcmp(name, "$unset") == 0) {
+            BSONObjBuilder bb(b.subobjStart(name));
+            bb.append(m->fieldName, 1);
+            bb.done();
+            return;
+        }
+
         BSONObjBuilder bb( b.subobjStart( name ) );
         if ( fixed ) {
             bb.appendAs( *fixed , m->fieldName );
         }
+        else if ( ! fixedArray.isEmpty() || forceEmptyArray ) {
+            bb.append( m->fieldName, fixedArray );
+        }
         else {
             bb.appendAs( m->elt , m->fieldName );
         }
@@ -579,19 +606,33 @@ namespace mongo {
 
             switch ( m.m->op ) {
             case Mod::UNSET:
+                m.fixedOpName = "$unset";
+                break;
+
             case Mod::ADDTOSET:
+                m.fixedOpName = "$set";
+                m.fixed = &(m.old);
+                break;
+
             case Mod::RENAME_FROM:
             case Mod::RENAME_TO:
                 // this should have been handled by prepare
                 break;
+
             case Mod::PULL:
             case Mod::PULL_ALL:
                 // this should have been handled by prepare
+                m.fixedOpName = "$set";
+                m.fixed = &(m.old);
                 break;
+
             case Mod::POP:
-                verify( m.old.eoo() || ( m.old.isABSONObj() && m.old.Obj().isEmpty() ) );
+                verify( m.old.isABSONObj() && m.old.Obj().isEmpty() );
+                m.fixedOpName = "$set";
+                m.fixed = &(m.old);
                 break;
                 // [dm] the BSONElementManipulator statements below are for replication (correct?)
+
             case Mod::INC:
                 if ( isOnDisk )
                     m.m->IncrementMe( m.old );
@@ -600,12 +641,14 @@ namespace mongo {
                 m.fixedOpName = "$set";
                 m.fixed = &(m.old);
                 break;
+
             case Mod::SET:
                 if ( isOnDisk )
                     BSONElementManipulator( m.old ).ReplaceTypeAndValue( m.m->elt );
                 else
                     BSONElementManipulator( m.old ).replaceTypeAndValue( m.m->elt );
                 break;
+
             default:
                 uassert( 13478 ,  "can't apply mod in place - shouldn't have gotten here" , 0 );
             }
@@ -618,11 +661,25 @@ namespace mongo {
                                           set<string>& onedownseen ) {
         Mod& m = *((Mod*)(modState.m)); // HACK
         switch (m.op) {
-        // unset/pull/pullAll on nothing does nothing, so don't append anything
-        case Mod::UNSET:
+        // unset/pull/pullAll on nothing does nothing, so don't append anything. Still,
+        // explicitly log that the target array was reset.
+        case Mod::POP:
         case Mod::PULL:
         case Mod::PULL_ALL:
+        case Mod::UNSET:
+            modState.fixedOpName = "$unset";
             return;
+
+        // $rename may involve dotted path creation, so we want to make sure we're not
+        // creating a path here for a rename that's a no-op. In other words if we're
+        // issuing a {$rename: {a.b : c.d} } that's a no-op, we don't want to create
+        // the a and c paths here. See test NestedNoName in the 'repl' suite.
+        case Mod::RENAME_FROM:
+        case Mod::RENAME_TO:
+            if (modState.dontApply) {
+                return;
+            }
+
         default:
             ;// fall through
         }
diff --git a/src/mongo/db/ops/update_internal.h b/src/mongo/db/ops/update_internal.h
index c69cd9fcf65..e3aa8c8dcf9 100644
--- a/src/mongo/db/ops/update_internal.h
+++ b/src/mongo/db/ops/update_internal.h
@@ -399,7 +399,9 @@ namespace mongo {
 
         const char* fixedOpName;
         BSONElement* fixed;
-        int pushStartSize;
+        BSONArray fixedArray;
+        bool forceEmptyArray;
+        int DEPRECATED_pushStartSize;
 
         BSONType incType;
         int incint;
@@ -411,7 +413,8 @@ namespace mongo {
         ModState() {
             fixedOpName = 0;
             fixed = 0;
-            pushStartSize = -1;
+            forceEmptyArray = false;
+            DEPRECATED_pushStartSize = -1;
             incType = EOO;
             dontApply = false;
         }
@@ -424,7 +427,7 @@ namespace mongo {
             return m->fieldName;
         }
 
-        bool needOpLogRewrite() const {
+        bool DEPRECATED_needOpLogRewrite() const {
             if ( dontApply )
                 return false;
 
@@ -438,8 +441,7 @@ namespace mongo {
             case Mod::BIT:
             case Mod::BITAND:
             case Mod::BITOR:
-                // TODO: should we convert this to $set?
-                return false;
+                return true;
             default:
                 return false;
             }
@@ -518,49 +520,64 @@ namespace mongo {
             switch ( m.op ) {
 
             case Mod::PUSH: {
+                ms.fixedOpName = "$set";
                 if ( m.isEach() ) {
-                    b.appendArray( m.shortFieldName, m.getEach() );
+                    BSONObj arr = m.getEach();
+                    b.appendArray( m.shortFieldName, arr );
+                    ms.forceEmptyArray = true;
+                    ms.fixedArray = BSONArray(arr.getOwned());
                 } else {
                     BSONObjBuilder arr( b.subarrayStart( m.shortFieldName ) );
                     arr.appendAs( m.elt, "0" );
-                    arr.done();
+                    ms.forceEmptyArray = true;
+                    ms.fixedArray = BSONArray(arr.done().getOwned());
                 }
                 break;
             }
+
             case Mod::ADDTOSET: {
+                ms.fixedOpName = "$set";
                 if ( m.isEach() ) {
                     // Remove any duplicates in given array
-                    BSONObjBuilder arr( b.subarrayStart( m.shortFieldName ) );
+                    BSONArrayBuilder arr( b.subarrayStart( m.shortFieldName ) );
                     BSONElementSet toadd;
                     m.parseEach( toadd );
                     BSONObjIterator i( m.getEach() );
-                    int n = 0;
+                    // int n = 0;
                     while ( i.more() ) {
                         BSONElement e = i.next();
                         if ( toadd.count(e) ) {
-                            arr.appendAs( e , BSONObjBuilder::numStr( n++ ) );
+                            arr.append( e );
                             toadd.erase( e );
                         }
                     }
-                    arr.done();
+                    ms.forceEmptyArray = true;
+                    ms.fixedArray = BSONArray(arr.done().getOwned());
                 }
                 else {
-                    BSONObjBuilder arr( b.subarrayStart( m.shortFieldName ) );
-                    arr.appendAs( m.elt, "0" );
-                    arr.done();
+                    BSONArrayBuilder arr( b.subarrayStart( m.shortFieldName ) );
+                    arr.append( m.elt );
+                    ms.forceEmptyArray = true;
+                    ms.fixedArray = BSONArray(arr.done().getOwned());
                 }
                 break;
             }
 
             case Mod::PUSH_ALL: {
                 b.appendAs( m.elt, m.shortFieldName );
+                ms.fixedOpName = "$set";
+                ms.forceEmptyArray = true;
+                ms.fixedArray = BSONArray(m.elt.Obj());
                 break;
             }
 
-            case Mod::UNSET:
+            case Mod::POP:
             case Mod::PULL:
             case Mod::PULL_ALL:
-                // no-op b/c unset/pull of nothing does nothing
+            case Mod::UNSET:
+                // No-op b/c unset/pull of nothing does nothing. Still, explicilty log that
+                // the target array was reset.
+                ms.fixedOpName = "$unset";
                 break;
 
             case Mod::INC:
@@ -570,10 +587,12 @@ namespace mongo {
                 b.appendAs( m.elt, m.shortFieldName );
                 break;
             }
+
             // shouldn't see RENAME_FROM here
             case Mod::RENAME_TO:
                 ms.handleRename( b, m.shortFieldName );
                 break;
+
             default:
                 stringstream ss;
                 ss << "unknown mod in appendNewFromMod: " << m.op;
@@ -601,9 +620,9 @@ namespace mongo {
 
         // re-writing for oplog
 
-        bool needOpLogRewrite() const {
+        bool DEPRECATED_needOpLogRewrite() const {
             for ( ModStateHolder::const_iterator i = _mods.begin(); i != _mods.end(); i++ )
-                if ( i->second->needOpLogRewrite() )
+                if ( i->second->DEPRECATED_needOpLogRewrite() )
                     return true;
             return false;
         }
@@ -615,21 +634,21 @@ namespace mongo {
             return b.obj();
         }
 
-        bool haveArrayDepMod() const {
+        bool DEPRECATED_haveArrayDepMod() const {
             for ( ModStateHolder::const_iterator i = _mods.begin(); i != _mods.end(); i++ )
                 if ( i->second->m->arrayDep() )
                     return true;
             return false;
         }
 
-        void appendSizeSpecForArrayDepMods( BSONObjBuilder& b ) const {
+        void DEPRECATED_appendSizeSpecForArrayDepMods( BSONObjBuilder& b ) const {
             for ( ModStateHolder::const_iterator i = _mods.begin(); i != _mods.end(); i++ ) {
                 const ModState& m = *i->second;
                 if ( m.m->arrayDep() ) {
-                    if ( m.pushStartSize == -1 )
+                    if ( m.DEPRECATED_pushStartSize == -1 )
                         b.appendNull( m.fieldName() );
                     else
-                        b << m.fieldName() << BSON( "$size" << m.pushStartSize );
+                        b << m.fieldName() << BSON( "$size" << m.DEPRECATED_pushStartSize );
                 }
             }
         }
diff --git a/src/mongo/dbtests/repltests.cpp b/src/mongo/dbtests/repltests.cpp
index 23ae7680092..7896f4ef8ad 100644
--- a/src/mongo/dbtests/repltests.cpp
+++ b/src/mongo/dbtests/repltests.cpp
@@ -1009,6 +1009,41 @@ namespace ReplTests {
             }
         };
 
+        class NestedNoRename : public Base {
+        public:
+            void doIt() const {
+                client()->update( ns(), BSON( "_id" << 0 ),
+                                  fromjson( "{$rename:{'a.b':'c.d'},$set:{z:1}}"
+                                      ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( BSON( "_id" << 0 << "z" << 1 ) , one( fromjson("{'_id':0}" ) ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0}" ) );
+            }
+        };
+
+        class AddToSetEmptyMissing : public Base {
+        public:
+            void doIt() const {
+                client()->update( ns(), BSON( "_id" << 0 ), fromjson(
+                                      "{$addToSet:{a:{$each:[]}}}" ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{_id:0,a:[]}" ), one( fromjson("{'_id':0}" ) )
+                    );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0}" ) );
+            }
+        };
 
     } // namespace Idempotence
 
@@ -1230,6 +1265,8 @@ namespace ReplTests {
             add< Idempotence::RenameReplace >();
             add< Idempotence::RenameOverwrite >();
             add< Idempotence::NoRename >();
+            add< Idempotence::NestedNoRename >();
+            add< Idempotence::AddToSetEmptyMissing >();
             add< DeleteOpIsIdBased >();
             add< DatabaseIgnorerBasic >();
             add< DatabaseIgnorerUpdate >();
diff --git a/src/mongo/dbtests/updatetests.cpp b/src/mongo/dbtests/updatetests.cpp
index e916299924a..28efdfe5787 100644
--- a/src/mongo/dbtests/updatetests.cpp
+++ b/src/mongo/dbtests/updatetests.cpp
@@ -640,8 +640,23 @@ namespace UpdateTests {
                 test( BSON( "$push" << BSON( "a" << 5 ) ) , fromjson( "{a:[1]}" ) , fromjson( "{a:[1,5]}" ) );
             }
         };
-        
-        class IncRewrite {
+
+
+        class IncRewriteInPlace {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 2 );
+                BSONObj mod = BSON( "$inc" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << 3 ) ), modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // Check if not applying in place changes anything.
+        class InRewriteForceNotInPlace {
         public:
             void run() {
                 BSONObj obj = BSON( "a" << 2 );
@@ -649,11 +664,10 @@ namespace UpdateTests {
                 ModSet modSet( mod );
                 auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
                 modSetState->createNewFromMods();
-                ASSERT( modSetState->needOpLogRewrite() );
                 ASSERT_EQUALS( BSON( "$set" << BSON( "a" << 3 ) ), modSetState->getOpLogRewrite() );
             }
         };
-   
+
         class IncRewriteNestedArray {
         public:
             void run() {
@@ -661,13 +675,410 @@ namespace UpdateTests {
                 BSONObj mod = BSON( "$inc" << BSON( "a.0" << 1 ) );
                 ModSet modSet( mod );
                 auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
-                modSetState->createNewFromMods();
-                ASSERT( modSetState->needOpLogRewrite() );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
                 ASSERT_EQUALS( BSON( "$set" << BSON( "a.0" << 3 ) ),
-                              modSetState->getOpLogRewrite() );
+                               modSetState->getOpLogRewrite() );
+                // XXX we want inc to do a full $set to start with, not a positional one.
+                // XXX fix me.
+            }
+        };
+
+        class IncRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 2 );
+                BSONObj mod = BSON( "$inc" << BSON( "a" << 1 ) << "$set" << BSON( "b" << 2) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << 3 ) << "$set" << BSON("b" << 2)),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class IncRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "c" << 1 );
+                BSONObj mod = BSON( "$inc" << BSON( "a" << 1 ) << "$set" << BSON( "b" << 2) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << 1 ) << "$set" << BSON("b" << 2)),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // Push is never applied in place
+        class PushRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2 ) );
+                BSONObj mod = BSON( "$push" << BSON( "a" << 3 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2 << 3 ) ) ),
+                                     modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PushRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "b" << 1 );
+                BSONObj mod = BSON( "$push" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 ) ) ),
+                                     modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PushAllRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2 ) );
+                BSONObj modAll = BSON( "$pushAll" << BSON( "a" << BSON_ARRAY( 3 << 4 << 5 ) ) );
+                ModSet modSetAll( modAll );
+                auto_ptr<ModSetState> modSetStateAll = modSetAll.prepare( obj );
+                ASSERT_FALSE( modSetStateAll->canApplyInPlace() );
+                modSetStateAll->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2 << 3 << 4 << 5) ) ),
+                                     modSetStateAll->getOpLogRewrite() );
+            }
+        };
+
+        class PushAllRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "b" << 1 );
+                BSONObj modAll = BSON( "$pushAll" << BSON( "a" << BSON_ARRAY( 1 << 2 << 3) ) );
+                ModSet modSetAll( modAll );
+                auto_ptr<ModSetState> modSetStateAll = modSetAll.prepare( obj );
+                ASSERT_FALSE( modSetStateAll->canApplyInPlace() );
+                modSetStateAll->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2 << 3 ) ) ),
+                               modSetStateAll->getOpLogRewrite() );
+            }
+        };
+
+        // Pull is only in place if it's a no-op.
+        class PullRewriteInPlace {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2 ) );
+                BSONObj modMatcher = BSON( "$pull" << BSON( "a" << BSON( "$gt" << 3 ) ) );
+                ModSet modSetMatcher( modMatcher );
+                auto_ptr<ModSetState> modSetStateMatcher = modSetMatcher.prepare( obj );
+                ASSERT_TRUE( modSetStateMatcher->canApplyInPlace() );
+                modSetStateMatcher->applyModsInPlace(false);
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2) ) ),
+                               modSetStateMatcher->getOpLogRewrite() );
+            }
+        };
+
+        class PullRewriteForceNotInPlace {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2 ) );
+                BSONObj modMatcher = BSON( "$pull" << BSON( "a" << BSON( "$gt" << 3 ) ) );
+                ModSet modSetMatcher( modMatcher );
+                auto_ptr<ModSetState> modSetStateMatcher = modSetMatcher.prepare( obj );
+                modSetStateMatcher->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2) ) ),
+                               modSetStateMatcher->getOpLogRewrite() );
+            }
+        };
+
+        class PullRewriteNonExistingUnsets {
+        public:
+            void run() {
+                BSONObj obj;
+                BSONObj modMatcher = BSON( "$pull" << BSON( "a" << BSON( "$gt" << 3 ) ) );
+                ModSet modSetMatcher( modMatcher );
+                auto_ptr<ModSetState> modSetStateMatcher = modSetMatcher.prepare( obj );
+                ASSERT_FALSE( modSetStateMatcher->canApplyInPlace() );
+                modSetStateMatcher->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "a" << 1 ) ),
+                               modSetStateMatcher->getOpLogRewrite() );
+            }
+        };
+
+        class PullRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2 ) );
+                BSONObj mod = BSON( "$pull" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 2 ) ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PullRewriteLastExistingField {
+        public:
+            void run() {
+                // check last pull corner case
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 2 ) );
+                BSONObj mod = BSON( "$pull" << BSON( "a" << 2 ) );
+                ModSet modSetLast( mod );
+                auto_ptr<ModSetState> modSetStateLast = modSetLast.prepare( obj );
+                ASSERT_FALSE( modSetStateLast->canApplyInPlace() );
+                modSetStateLast->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSONArray() ) ),
+                               modSetStateLast->getOpLogRewrite() );
+            }
+        };
+
+        class PullRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "b" << 1 );
+                BSONObj mod = BSON( "$pull" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "a" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // Pop is only applied in place if the target array remains the same size (i.e. if
+        // it is empty already.
+        class PopRewriteEmptyArray {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSONArray() );
+                BSONObj mod = BSON( "$pop" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSONArray() ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PopRewriteLastElement {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$pop" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSONArray() ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PopRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 << 2) );
+                BSONObj mod = BSON( "$pop" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 ) ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class PopRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$pop" << BSON( "b" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "b" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // AddToSet is in place if it is a no-op.
+        class AddToSetRewriteInPlace {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$addToSet" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 ) ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class AddToSetRewriteForceNotInPlace {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$addToSet" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 ) ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class AddToSetRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$addToSet" << BSON( "a" << 2 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << BSON_ARRAY( 1 << 2 ) ) ),
+                               modSetState->getOpLogRewrite() );
             }
         };
 
+        class AddToSetRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << BSON_ARRAY( 1 ) );
+                BSONObj mod = BSON( "$addToSet" << BSON( "b" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "b" << BSON_ARRAY( 1 ) ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // Rename doesn't log if both fields are not present.
+        class RenameRewriteBothNonExistent {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 1 );
+                BSONObj mod = BSON( "$rename" << BSON( "b" << "c" ) );
+                ModSet modSet( mod );
+                                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
+                ASSERT_EQUALS( BSONObj(), modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class RenameRewriteExistingToField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "b" << 100 );
+                BSONObj mod = BSON( "$rename" << BSON( "a" << "b" ) );
+                ModSet modSet( mod );
+                                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_TRUE( modSetState->canApplyInPlace() );
+                modSetState->applyModsInPlace(false);
+                ASSERT_EQUALS( BSONObj(), modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class RenameRewriteExistingFromField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 100 );
+                BSONObj mod = BSON( "$rename" << BSON( "a" << "b" ) );
+                ModSet modSet( mod );
+                                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "a" << 1 ) << "$set" << BSON ( "b" << 100 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class RenameRewriteBothExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 100 << "b" << 200);
+                BSONObj mod = BSON( "$rename" << BSON( "a" << "b" ) );
+                ModSet modSet( mod );
+                                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "a" << 1 ) << "$set" << BSON ( "b" << 100 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        // $bit is never applied in place currently
+        class BitRewriteExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 0 );
+                BSONObj mod = BSON( "$bit" << BSON( "a" << BSON( "or" << 1 ) ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "a" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class BitRewriteNonExistingField {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 0 );
+                BSONObj mod = BSON( "$bit" << BSON( "b" << BSON( "or" << 1 ) ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "b" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class SetIsNotRewritten {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 0 );
+                BSONObj mod = BSON( "$set" << BSON( "b" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$set" << BSON( "b" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
+
+        class UnsetIsNotRewritten {
+        public:
+            void run() {
+                BSONObj obj = BSON( "a" << 0 );
+                BSONObj mod = BSON( "$unset" << BSON( "a" << 1 ) );
+                ModSet modSet( mod );
+                auto_ptr<ModSetState> modSetState = modSet.prepare( obj );
+                ASSERT_FALSE( modSetState->canApplyInPlace() );
+                modSetState->createNewFromMods();
+                ASSERT_EQUALS( BSON( "$unset" << BSON( "a" << 1 ) ),
+                               modSetState->getOpLogRewrite() );
+            }
+        };
     };
 
     namespace basic {
@@ -933,8 +1344,39 @@ namespace UpdateTests {
             add< ModSetTests::inc2 >();
             add< ModSetTests::set1 >();
             add< ModSetTests::push1 >();
-            add< ModSetTests::IncRewrite >();
+
+            add< ModSetTests::IncRewriteInPlace >();
+            add< ModSetTests::InRewriteForceNotInPlace >();
             add< ModSetTests::IncRewriteNestedArray >();
+            add< ModSetTests::IncRewriteExistingField >();
+            add< ModSetTests::IncRewriteNonExistingField >();
+            add< ModSetTests::PushRewriteExistingField >();
+            add< ModSetTests::PushRewriteNonExistingField >();
+            add< ModSetTests::PushAllRewriteExistingField >();
+            add< ModSetTests::PushAllRewriteNonExistingField >();
+            add< ModSetTests::PullRewriteInPlace >();
+            add< ModSetTests::PullRewriteForceNotInPlace >();
+            add< ModSetTests::PullRewriteNonExistingUnsets >();
+            add< ModSetTests::PullRewriteExistingField >();
+            add< ModSetTests::PullRewriteLastExistingField >();
+            add< ModSetTests::PullRewriteNonExistingField >();
+            add< ModSetTests::PopRewriteEmptyArray >();
+            add< ModSetTests::PopRewriteLastElement >();
+            add< ModSetTests::PopRewriteExistingField >();
+            add< ModSetTests::PopRewriteNonExistingField >();
+            add< ModSetTests::AddToSetRewriteInPlace >();
+            add< ModSetTests::AddToSetRewriteForceNotInPlace >();
+            add< ModSetTests::AddToSetRewriteExistingField >();
+            add< ModSetTests::AddToSetRewriteNonExistingField >();
+            add< ModSetTests::RenameRewriteBothNonExistent >();
+            add< ModSetTests::RenameRewriteExistingToField >();
+            add< ModSetTests::RenameRewriteExistingFromField >();
+            add< ModSetTests::RenameRewriteBothExistingField >();
+            add< ModSetTests::BitRewriteExistingField >();
+            // XXX $bit over non-existing field is missing. Probably out of scope to fix it here.
+            // add< ModSetTests::BitRewriteNonExistingField >();
+            add< ModSetTests::SetIsNotRewritten >();
+            add< ModSetTests::UnsetIsNotRewritten >();
 
             add< basic::inc1 >();
             add< basic::inc2 >();