SERVER-12657 better validity checks for ns and index descriptors in write ops

6 files changed, 104 insertions, 75 deletions

diff --git a/src/mongo/db/commands/write_commands/batch_executor.cpp b/src/mongo/db/commands/write_commands/batch_executor.cpp
index 8a0d115d251..121d541a29b 100644
--- a/src/mongo/db/commands/write_commands/batch_executor.cpp
+++ b/src/mongo/db/commands/write_commands/batch_executor.cpp
@@ -98,6 +98,14 @@ namespace mongo {
         return error;
     }
 
+    static void toBatchError( const Status& status, BatchedCommandResponse* response ) {
+        response->clear();
+        response->setErrCode( status.code() );
+        response->setErrMessage( status.reason() );
+        response->setOk( false );
+        dassert( response->isValid(NULL) );
+    }
+
     static void noteInCriticalSection( WriteErrorDetail* staleError ) {
         BSONObjBuilder builder;
         if ( staleError->isErrInfoSet() )
@@ -109,34 +117,60 @@ namespace mongo {
     void WriteBatchExecutor::executeBatch( const BatchedCommandRequest& request,
                                            BatchedCommandResponse* response ) {
 
-        // TODO: Lift write concern parsing out of this entirely.
+        // Validate namespace
+        const NamespaceString nss = NamespaceString( request.getNS() );
+        if ( !nss.isValid() ) {
+            toBatchError( Status( ErrorCodes::InvalidNamespace,
+                                  nss.ns() + " is not a valid namespace" ),
+                          response );
+            return;
+        }
+
+        // Make sure we can write to the namespace
+        Status allowedStatus = userAllowedWriteNS( nss );
+        if ( !allowedStatus.isOK() ) {
+            toBatchError( allowedStatus, response );
+            return;
+        }
+
+        // Validate insert index requests
+        // TODO: Push insert index requests through createIndex once all upgrade paths support it
+        string errMsg;
+        if ( request.isInsertIndexRequest() && !request.isValidIndexRequest( &errMsg ) ) {
+            toBatchError( Status( ErrorCodes::InvalidOptions, errMsg ), response );
+            return;
+        }
+
+        // Validate write concern
+        // TODO: Lift write concern parsing out of this entirely
         WriteConcernOptions writeConcern;
-        Status status = Status::OK();
 
         BSONObj wcDoc;
         if ( request.isWriteConcernSet() ) {
             wcDoc = request.getWriteConcern();
         }
 
+        Status wcStatus = Status::OK();
         if ( wcDoc.isEmpty() ) {
-            status = writeConcern.parse( _defaultWriteConcern );
+            wcStatus = writeConcern.parse( _defaultWriteConcern );
         }
         else {
-            status = writeConcern.parse( wcDoc );
+            wcStatus = writeConcern.parse( wcDoc );
         }
 
-        if ( status.isOK() ) {
-            status = validateWriteConcern( writeConcern );
+        if ( wcStatus.isOK() ) {
+            wcStatus = validateWriteConcern( writeConcern );
         }
 
-        if ( !status.isOK() ) {
-            response->setErrCode( status.code() );
-            response->setErrMessage( status.reason() );
-            response->setOk( false );
-            dassert( response->isValid(NULL) );
+        if ( !wcStatus.isOK() ) {
+            toBatchError( wcStatus, response );
             return;
         }
 
+        //
+        // End validation
+        //
+
         bool silentWC = writeConcern.wMode.empty() && writeConcern.wNumNodes == 0
                         && writeConcern.syncMode == WriteConcernOptions::NONE;
 
@@ -170,7 +204,7 @@ namespace mongo {
             _client->curop()->setMessage( "waiting for write concern" );
 
             WriteConcernResult res;
-            status = waitForWriteConcern( writeConcern, _client->getLastOp(), &res );
+            Status status = waitForWriteConcern( writeConcern, _client->getLastOp(), &res );
 
             if ( !status.isOK() ) {
                 wcError.reset( toWriteConcernError( status, res ) );
@@ -654,32 +688,13 @@ namespace mongo {
         }
     }
 
-    // Does preprocessing of inserts, special casing for indexes
-    // TODO: Simplify this when indexes aren't here anymore
-    static StatusWith<BSONObj> normalizeInsert( const BatchItemRef& insertItem ) {
-
-        if ( insertItem.getRequest()->isInsertIndexRequest() ) {
-
-            StatusWith<BSONObj> normalInsert = fixDocumentForInsert( insertItem.getDocument() );
-            if ( normalInsert.isOK() && insertItem.getDocument()["ns"].type() != String ) {
-                return StatusWith<BSONObj>( ErrorCodes::BadValue, "tried to create an index "
-                                            "without specifying namespace" );
-            }
-            else {
-                return normalInsert;
-            }
-        }
-        else {
-            return fixDocumentForInsert( insertItem.getDocument() );
-        }
-    }
-
     // Goes over the request and preprocesses normalized versions of all the inserts in the request
     static void normalizeInserts( const BatchedCommandRequest& request,
                                   vector<StatusWith<BSONObj> >* normalInserts ) {
 
         for ( size_t i = 0; i < request.sizeWriteOps(); ++i ) {
-            StatusWith<BSONObj> normalInsert = normalizeInsert( BatchItemRef( &request, i ) );
+            BSONObj insertDoc = request.getInsertRequest()->getDocumentsAt( i );
+            StatusWith<BSONObj> normalInsert = fixDocumentForInsert( insertDoc );
             normalInserts->push_back( normalInsert );
             if ( request.getOrdered() && !normalInsert.isOK() )
                 break;
diff --git a/src/mongo/db/commands/write_commands/write_commands.cpp b/src/mongo/db/commands/write_commands/write_commands.cpp
index 49cd233ae31..aed4d028b2c 100644
--- a/src/mongo/db/commands/write_commands/write_commands.cpp
+++ b/src/mongo/db/commands/write_commands/write_commands.cpp
@@ -36,7 +36,6 @@
 #include "mongo/db/curop.h"
 #include "mongo/db/json.h"
 #include "mongo/db/lasterror.h"
-#include "mongo/db/ops/insert.h"
 #include "mongo/db/server_parameters.h"
 #include "mongo/db/stats/counters.h"
 
@@ -131,10 +130,6 @@ namespace mongo {
         NamespaceString nss(dbName, request.getNS());
         request.setNS(nss.ns());
 
-        Status status = userAllowedWriteNS( nss );
-        if ( !status.isOK() )
-            return appendCommandStatus( result, status );
-
         BSONObj defaultWriteConcern;
         // This is really bad - it's only safe because we leak the defaults by overriding them with
         // new defaults and because we never reset to an empty default.
diff --git a/src/mongo/s/cluster_write.cpp b/src/mongo/s/cluster_write.cpp
index 348c1c3104b..5d726737e67 100644
--- a/src/mongo/s/cluster_write.cpp
+++ b/src/mongo/s/cluster_write.cpp
@@ -306,45 +306,33 @@ namespace mongo {
         return false;
     }
 
+    static void toBatchError( const Status& status, BatchedCommandResponse* response ) {
+        response->clear();
+        response->setErrCode( status.code() );
+        response->setErrMessage( status.reason() );
+        response->setOk( false );
+        dassert( response->isValid(NULL) );
+    }
+
     void ClusterWriter::write( const BatchedCommandRequest& request,
                                BatchedCommandResponse* response ) {
 
-        // App-level validation of a create index insert
-        if ( request.isInsertIndexRequest() ) {
-            if ( request.sizeWriteOps() != 1 ) {
-
-                // Invalid request to create index
-                response->setOk( false );
-                response->setErrCode( ErrorCodes::InvalidOptions );
-                response->setErrMessage( "invalid batch request for index creation" );
-
-                dassert( response->isValid( NULL ) );
-                return;
-            }
-
-            NamespaceString ns( request.getTargetingNS() );
-            if ( !ns.isValid() ) {
-                response->setOk( false );
-                response->setN( 0 );
-                response->setErrCode( ErrorCodes::InvalidNamespace );
-                string errMsg( str::stream() << ns.ns() << " is not a valid namespace to index" );
-                response->setErrMessage( errMsg );
-                return;
-            }
+        const NamespaceString nss = NamespaceString( request.getNS() );
+        if ( !nss.isValid() ) {
+            toBatchError( Status( ErrorCodes::InvalidNamespace,
+                                  nss.ns() + " is not a valid namespace" ),
+                          response );
+            return;
         }
 
-        NamespaceString ns( request.getNS() );
-        if ( !ns.isValid() ) {
-            response->setOk( false );
-            response->setN( 0 );
-            response->setErrCode( ErrorCodes::InvalidNamespace );
-            string errMsg( str::stream() << ns.ns() << " is not a valid namespace" );
-            response->setErrMessage( errMsg );
+        string errMsg;
+        if ( request.isInsertIndexRequest() && !request.isValidIndexRequest( &errMsg ) ) {
+            toBatchError( Status( ErrorCodes::InvalidOptions, errMsg ), response );
             return;
         }
 
         // Config writes and shard writes are done differently
-        string dbName = ns.db().toString();
+        string dbName = nss.db().toString();
         if ( dbName == "config" || dbName == "admin" ) {
 
             bool verboseWC = request.isVerboseWC();
@@ -353,12 +341,10 @@ namespace mongo {
             if ( request.sizeWriteOps() != 1 ||
                     ( request.isWriteConcernSet() &&
                             !validConfigWC( request.getWriteConcern() ))) {
-                // Invalid config server write
-                response->setOk( false );
-                response->setErrCode( ErrorCodes::InvalidOptions );
-                response->setErrMessage( "invalid batch request for config write" );
 
-                dassert( response->isValid( NULL ) );
+                toBatchError( Status( ErrorCodes::InvalidOptions,
+                                      "invalid batch request for config write" ),
+                              response );
                 return;
             }
 
diff --git a/src/mongo/s/write_ops/batched_command_request.cpp b/src/mongo/s/write_ops/batched_command_request.cpp
index a7e210ced9b..ce90f54be87 100644
--- a/src/mongo/s/write_ops/batched_command_request.cpp
+++ b/src/mongo/s/write_ops/batched_command_request.cpp
@@ -93,6 +93,34 @@ namespace mongo {
         return extractUniqueIndex( getInsertRequest()->getDocumentsAt( 0 ) );
     }
 
+    bool BatchedCommandRequest::isValidIndexRequest( string* errMsg ) const {
+
+        string dummy;
+        if ( !errMsg )
+            errMsg = &dummy;
+        dassert( isInsertIndexRequest() );
+
+        if ( sizeWriteOps() != 1 ) {
+            *errMsg = "invalid batch request for index creation";
+            return false;
+        }
+
+        NamespaceString targetNSS( getTargetingNS() );
+        if ( !targetNSS.isValid() ) {
+            *errMsg = targetNSS.ns() + " is not a valid namespace to index";
+            return false;
+        }
+
+        NamespaceString reqNSS( getNS() );
+        if ( reqNSS.db().compare( targetNSS.db() ) != 0 ) {
+            *errMsg = targetNSS.ns() + " namespace is not in the request database "
+                      + reqNSS.db().toString();
+            return false;
+        }
+
+        return true;
+    }
+
     static void extractIndexNSS( const BSONObj& indexDesc, NamespaceString* indexNSS ) {
         *indexNSS = NamespaceString( indexDesc["ns"].str() );
     }
diff --git a/src/mongo/s/write_ops/batched_command_request.h b/src/mongo/s/write_ops/batched_command_request.h
index 1678ea57c13..5702d6077b5 100644
--- a/src/mongo/s/write_ops/batched_command_request.h
+++ b/src/mongo/s/write_ops/batched_command_request.h
@@ -107,6 +107,7 @@ namespace mongo {
         // Index creation is also an insert, but a weird one.
         bool isInsertIndexRequest() const;
         bool isUniqueIndexRequest() const;
+        bool isValidIndexRequest( std::string* errMsg ) const;
         std::string getTargetingNS() const;
         BSONObj getIndexKeyPattern() const;
 
diff --git a/src/mongo/shell/collection.js b/src/mongo/shell/collection.js
index 339c055d0c4..1023d8ce1ec 100644
--- a/src/mongo/shell/collection.js
+++ b/src/mongo/shell/collection.js
@@ -491,8 +491,12 @@ DBCollection.prototype.createIndex = function( keys , options ){
     var o = this._indexSpec( keys, options );
 
     if ( this._mongo.useWriteCommands() ) {
-        delete o.ns; // ns is passed to the first element in the command.
-        return this._db.runCommand({ createIndexes: this.getName(), indexes: [o] });
+
+        // TODO: Use createIndexes command once fully supported by upgrade process
+        var bulk = this.getDB().system.indexes.initializeOrderedBulkOp();
+        bulk.insert(o);
+
+        return bulk.execute().toSingleResult();
     }
     else {
         this._db.getCollection( "system.indexes" ).insert( o , 0, true );