bug SERVER-132 implemented pullAll/pushAll modifiers

5 files changed, 150 insertions, 24 deletions

diff --git a/db/query.cpp b/db/query.cpp
index 226b8a1cad4..b3c34a1ef41 100644
--- a/db/query.cpp
+++ b/db/query.cpp
@@ -278,10 +278,10 @@ namespace mongo {
         }
         static Mod::Op opFromStr( const char *fn ) {
 	  const char *valid[] = { "$inc", "$set", "$push", "$pushAll", "$pull", "$pullAll" };
-            for( int i = 0; i < 4; ++i )
+            for( int i = 0; i < 6; ++i )
                 if ( strcmp( fn, valid[ i ] ) == 0 )
                     return Mod::Op( i );
-            uassert( "Invalid modifier specified", false );
+            uassert( "Invalid modifier specified " + string( fn ), false );
             return Mod::INC;
         }
     public:
@@ -333,13 +333,13 @@ namespace mongo {
         }
         bool havePush() const {
             for ( vector<Mod>::const_iterator i = mods_.begin(); i != mods_.end(); i++ )
-                if ( i->op == Mod::PUSH )
+	      if ( i->op == Mod::PUSH || i->op == Mod::PUSH_ALL )
                     return true;
             return false;
         }
         void appendSizeSpecForPushes( BSONObjBuilder &b ) const {
             for ( vector<Mod>::const_iterator i = mods_.begin(); i != mods_.end(); i++ ) {
-                if ( i->op == Mod::PUSH ) {
+	      if ( i->op == Mod::PUSH || i->op == Mod::PUSH_ALL ) {
                     if ( i->pushStartSize == -1 )
                         b.appendNull( i->fieldName );
                     else
@@ -375,16 +375,28 @@ namespace mongo {
                         uassert( "Cannot apply $push/$pushAll modifier to non-array", e.type() == Array || e.eoo() );
                         inPlacePossible = false;
                         break;
-       		case Mod::PULL: {
-                        uassert( "Cannot apply $pull modifier to non-array", e.type() == Array || e.eoo() );
+       		case Mod::PULL:
+		case Mod::PULL_ALL: {
+                        uassert( "Cannot apply $pull/$pullAll modifier to non-array", e.type() == Array || e.eoo() );
 			BSONObjIterator i( e.embeddedObject() );
-			while( i.moreWithEOO() ) {
+			while( inPlacePossible && i.moreWithEOO() ) {
 			  BSONElement arrI = i.next();
 			  if ( arrI.eoo() )
 			    break;
-			  if ( arrI.woCompare( m.elt, false ) == 0 ) {
-			    inPlacePossible = false;
-			    break;
+			  if ( m.op == Mod::PULL ) {
+			    if ( arrI.woCompare( m.elt, false ) == 0 ) {
+			      inPlacePossible = false;
+			    }
+			  } else if ( m.op == Mod::PULL_ALL ) {
+			    BSONObjIterator j( m.elt.embeddedObject() );
+			    while( inPlacePossible && j.moreWithEOO() ) {
+			      BSONElement arrJ = j.next();
+			      if ( arrJ.eoo() )
+				break;
+			      if ( arrI.woCompare( arrJ, false ) == 0 ) {
+				inPlacePossible = false;
+			      }
+			    }
 			  }
 			}
                         break;
@@ -398,7 +410,7 @@ namespace mongo {
         for ( vector<Mod>::const_iterator i = mods_.begin(); i != mods_.end(); ++i ) {
             const Mod& m = *i;
             BSONElement e = obj.getFieldDotted(m.fieldName);
-	    if ( m.op == Mod::PULL )
+	    if ( m.op == Mod::PULL || m.op == Mod::PULL_ALL )
 	      continue;
             if ( m.op == Mod::INC ) {
                 m.setn( e.number() + m.getn() );
@@ -458,7 +470,6 @@ namespace mongo {
                          " or is referenced in another $set clause",
                         mayAddEmbedded( existing, m->fieldName ) );                
             if ( cmp == 0 ) {
-	      out() << "an op!" << endl;
                 BSONElement e = p->second;
                 if ( m->op == Mod::INC ) {
                     m->setn( m->getn() + e.number() );
@@ -477,13 +488,11 @@ namespace mongo {
                         ++startCount;
                     }
 		    if ( m->op == Mod::PUSH ) {
-		      out() << "Mod::PUSH" << endl;
 		      stringstream ss;
 		      ss << startCount;
 		      string nextIndex = ss.str();
 		      arr.appendAs( m->elt, nextIndex.c_str() );
 		    } else {
-		      out() << "Mod::PUSH_ALL" << endl;
 		      BSONObjIterator i( m->elt.embeddedObject() );
 		      int count = startCount;
 		      while( i.moreWithEOO() ) {
@@ -498,7 +507,7 @@ namespace mongo {
 		    }
                     arr.done();
                     m->pushStartSize = startCount;
-                } else if ( m->op == Mod::PULL ) {
+                } else if ( m->op == Mod::PULL || m->op == Mod::PULL_ALL ) {
 		  BSONObjBuilder arr( b2.subarrayStartAs( m->fieldName ) );
 		  BSONObjIterator i( e.embeddedObject() );
 		  int count = 0;
@@ -506,7 +515,19 @@ namespace mongo {
 		    BSONElement arrI = i.next();
 		    if ( arrI.eoo() )
 		      break;
-		    if ( arrI.woCompare( m->elt, false ) != 0 ) {
+		    bool allowed = true;
+		    if ( m->op == Mod::PULL ) {
+		      allowed = ( arrI.woCompare( m->elt, false ) != 0 );
+		    } else {
+		      BSONObjIterator j( m->elt.embeddedObject() );
+		      while( allowed && j.moreWithEOO() ) {
+			BSONElement arrJ = j.next();
+			if ( arrJ.eoo() )
+			  break;
+			allowed = ( arrI.woCompare( arrJ, false ) != 0 );
+		      }
+		    }
+		    if ( allowed ) {
 		      stringstream ss;
 		      ss << count++;
 		      string index = ss.str();
@@ -558,7 +579,7 @@ namespace mongo {
             if ( e.eoo() )
                 break;
             const char *fn = e.fieldName();
-            uassert( "Invalid modifier specified", e.type() == Object );
+            uassert( "Invalid modifier specified" + string( fn ), e.type() == Object );
             BSONObj j = e.embeddedObject();
             BSONObjIterator jt(j);
             Mod::Op op = opFromStr( fn );
@@ -578,6 +599,7 @@ namespace mongo {
                             strcmp( m.fieldName, i->fieldName ) != 0 );
                 }
                 uassert( "Modifier $inc allowed for numbers only", f.isNumber() || op != Mod::INC );
+                uassert( "Modifier $pushAll/pullAll allowed for arrays only", f.type() == Array || ( op != Mod::PUSH_ALL && op != Mod::PULL_ALL ) );
                 m.elt = f;
                 if ( f.type() == NumberDouble ) {
                     m.ndouble = (double *) f.value();
diff --git a/dbtests/repltests.cpp b/dbtests/repltests.cpp
index 186af9bfd14..4629a7d7ac3 100644
--- a/dbtests/repltests.cpp
+++ b/dbtests/repltests.cpp
@@ -672,6 +672,54 @@ namespace ReplTests {
             }                        
         };
 
+        class PushAll : public Base {
+        public:
+            void doIt() const {
+	      client()->update( ns(), BSON( "_id" << 0 ), fromjson( "{$pushAll:{a:[5.0,6.0]}}" ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{'_id':0,a:[4,5,6]}" ), one( fromjson( "{'_id':0}" ) ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0,a:[4]}" ) );
+            }            
+        };
+        
+        class PushAllUpsert : public Base {
+        public:
+            void doIt() const {
+	      client()->update( ns(), BSON( "_id" << 0 ), fromjson( "{$pushAll:{a:[5.0,6.0]}}" ), true );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{'_id':0,a:[4,5,6]}" ), one( fromjson( "{'_id':0}" ) ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0,a:[4]}" ) );
+            }            
+        };
+
+        class EmptyPushAll : public Base {
+        public:
+            void doIt() const {
+                client()->update( ns(), BSON( "_id" << 0 ), fromjson( "{$pushAll:{a:[5.0,6.0]}}" ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{'_id':0,a:[5,6]}" ), one( fromjson( "{'_id':0}" ) ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0}" ) );
+            }                        
+        };
+
         class Pull : public Base {
         public:
             void doIt() const {
@@ -704,6 +752,22 @@ namespace ReplTests {
             }            
         };
                 
+        class PullAll : public Base {
+        public:
+            void doIt() const {
+	      client()->update( ns(), BSON( "_id" << 0 ), fromjson( "{$pullAll:{a:[4,5]}}" ) );
+            }
+            using ReplTests::Base::check;
+            void check() const {
+                ASSERT_EQUALS( 1, count() );
+                check( fromjson( "{'_id':0,a:[6]}" ), one( fromjson( "{'_id':0}" ) ) );
+            }
+            void reset() const {
+                deleteAll( ns() );
+                insert( fromjson( "{'_id':0,a:[4,5,6]}" ) );
+            }            
+        };
+
     } // namespace Idempotence
     
     class DeleteOpIsIdBased : public Base {
@@ -869,8 +933,12 @@ namespace ReplTests {
             add< Idempotence::PushUpsert >();
             add< Idempotence::MultiPush >();
             add< Idempotence::EmptyPush >();
+            add< Idempotence::PushAll >();
+            add< Idempotence::PushAllUpsert >();
+            add< Idempotence::EmptyPushAll >();
             add< Idempotence::Pull >();
             add< Idempotence::PullNothing >();
+            add< Idempotence::PullAll >();
             add< DeleteOpIsIdBased >();
             add< DbIdsTest >();
             add< MemIdsTest >();
diff --git a/dbtests/updatetests.cpp b/dbtests/updatetests.cpp
index 10beba793c0..325a031a454 100644
--- a/dbtests/updatetests.cpp
+++ b/dbtests/updatetests.cpp
@@ -106,6 +106,20 @@ namespace UpdateTests {
         }
     };
 
+    class PushAllNonArray : public Fail {
+        void doIt() {
+	  insert( ns(), fromjson( "{a:[1]}" ) );
+            update( ns(), BSONObj(), fromjson( "{$pushAll:{a:'d'}}" ) );
+        }
+    };
+
+    class PullAllNonArray : public Fail {
+        void doIt() {
+	  insert( ns(), fromjson( "{a:[1]}" ) );
+            update( ns(), BSONObj(), fromjson( "{$pullAll:{a:'d'}}" ) );
+        }
+    };
+
     class IncTargetNonNumber : public Fail {
         void doIt() {
             insert( ns(), BSON( "a" << "a" ) );
@@ -474,6 +488,8 @@ namespace UpdateTests {
             add< ModNotFirst >();
             add< ModDuplicateFieldSpec >();
             add< IncNonNumber >();
+            add< PushAllNonArray >();
+            add< PullAllNonArray >();
             add< IncTargetNonNumber >();
             add< SetNum >();
             add< SetString >();
diff --git a/jstests/pullall.js b/jstests/pullall.js
new file mode 100644
index 00000000000..b720ce58204
--- /dev/null
+++ b/jstests/pullall.js
@@ -0,0 +1,18 @@
+t = db.jstests_pushall;
+t.drop();
+
+t.save( { a: [ 1, 2, 3 ] } );
+t.update( {}, { $pullAll: { a: [ 3 ] } } );
+assert.eq( [ 1, 2 ], t.findOne().a );
+t.update( {}, { $pullAll: { a: [ 3 ] } } );
+assert.eq( [ 1, 2 ], t.findOne().a );
+
+t.drop();
+t.save( { a: [ 1, 2, 3 ] } );
+t.update( {}, { $pullAll: { a: [ 2, 3 ] } } );
+assert.eq( [ 1 ], t.findOne().a );
+t.update( {}, { $pullAll: { a: [] } } );
+assert.eq( [ 1 ], t.findOne().a );
+t.update( {}, { $pullAll: { a: [ 1, 5 ] } } );
+assert.eq( [], t.findOne().a );
+
diff --git a/jstests/pushall.js b/jstests/pushall.js
index 83fd4a39b65..73e62b56721 100644
--- a/jstests/pushall.js
+++ b/jstests/pushall.js
@@ -4,11 +4,13 @@ t.drop();
 t.save( { a: [ 1, 2, 3 ] } );
 t.update( {}, { $pushAll: { a: [ 4 ] } } );
 assert.eq( [ 1, 2, 3, 4 ], t.findOne().a );
-// t.update( {}, { $pushAll: { a: [ 4 ] } } );
-// assert.eq( [ 1, 2, 3, 4, 4 ], t.findOne().a );
+t.update( {}, { $pushAll: { a: [ 4 ] } } );
+assert.eq( [ 1, 2, 3, 4, 4 ], t.findOne().a );
+
+t.drop();
+t.save( { a: [ 1, 2, 3 ] } );
+t.update( {}, { $pushAll: { a: [ 4, 5 ] } } );
+assert.eq( [ 1, 2, 3, 4, 5 ], t.findOne().a );
+t.update( {}, { $pushAll: { a: [] } } );
+assert.eq( [ 1, 2, 3, 4, 5 ], t.findOne().a );
 
-// t.drop();
-// t.update( {}, { $pushAll: { a: [ 4, 5 ] } } );
-// assert.eq( [ 1, 2, 3, 4, 5 ], t.findOne().a );
-// t.update( {}, { $pushAll: { a: [] } } );
-// assert.eq( [ 1, 2, 3, 4, 5 ], t.findOne().a );