SERVER-13865 handle edge case for v2.4 when upserted _id not returned

(cherry picked from commit 090ea9a5ad1ed52e40b5a66df5ee1eab283845cb)

6 files changed, 145 insertions, 8 deletions

diff --git a/jstests/multiVersion/batch_write_commands_update_sharded.js b/jstests/multiVersion/batch_write_commands_update_sharded.js
index 310da28d562..0138a0fba68 100644
--- a/jstests/multiVersion/batch_write_commands_update_sharded.js
+++ b/jstests/multiVersion/batch_write_commands_update_sharded.js
@@ -1,5 +1,5 @@
 /**
- * This test checks update write command corner cases:
+ * This test checks update write command corner cases for mongos:
  *   -- nModified behavior
  *   -- other?
  */
@@ -82,6 +82,31 @@ var res = assert.commandWorked(coll24.getDB().runCommand(req));
 assert.eq(undefined, res.nModified, tojson(res))
 assert.eq(22, res.n, tojson(res))
 
+// Test non-OID upsert behavior
+
+// 2.6 mongod shard
+var upsertedId = ObjectId().toString();
+var req = {update:coll26.getName(),
+           updates:[
+                {q:{_id:upsertedId}, u:{$set:{a:1}}, upsert:true}
+           ]
+       };
+var res = assert.commandWorked(coll26.getDB().runCommand(req));
+assert.eq(0, res.nModified, "coll26: " + tojson(res));
+assert.eq(1, res.n, "coll26: " + tojson(res));
+assert.eq(upsertedId, res.upserted[0]._id,  "coll26: " + tojson(res));
+
+// 2.4 mongod shard
+var upsertedId = ObjectId().toString();
+var req = {update:coll24.getName(),
+           updates:[
+                {q:{_id:upsertedId}, u:{$set:{a:1}}, upsert:true}
+           ]
+       };
+var res = assert.commandWorked(coll24.getDB().runCommand(req));
+assert.eq(1, res.n, "coll24: " + tojson(res));
+assert.eq(upsertedId, res.upserted[0]._id,  "coll24: " + tojson(res));
+
 // mixed version mongod shards
 var req = {update:collMixed.getName(),
            updates:[
diff --git a/jstests/multiVersion/bulk_write_commands_update_singlenode.js b/jstests/multiVersion/bulk_write_commands_update_singlenode.js
new file mode 100644
index 00000000000..f45cb108fe7
--- /dev/null
+++ b/jstests/multiVersion/bulk_write_commands_update_singlenode.js
@@ -0,0 +1,25 @@
+/**
+ * This test checks update bulk api corner cases for single nodes:
+ *   -- upsert no _id
+ *   -- other?
+ */
+
+var mongod24 = MongoRunner.runMongod({ binVersion : "2.4" });
+
+var coll24 = mongod24.getCollection("foo.bar");
+
+// 2.4 mongod fluent api
+
+var upsertedId = ObjectId().toString();
+var bulk = coll24.initializeOrderedBulkOp();
+bulk.find({ _id : upsertedId }).upsert().update({ $set : { a : 1 } });
+var res = bulk.execute();
+printjson(res);
+assert.eq(1, res.nUpserted);
+assert.eq(upsertedId, res.getUpsertedIdAt(0)._id);
+
+jsTest.log("DONE!");
+
+MongoRunner.stopMongod(mongod24);
+
+
diff --git a/src/mongo/s/write_ops/batch_downconvert.cpp b/src/mongo/s/write_ops/batch_downconvert.cpp
index cdfa9264cac..d76591df5e8 100644
--- a/src/mongo/s/write_ops/batch_downconvert.cpp
+++ b/src/mongo/s/write_ops/batch_downconvert.cpp
@@ -149,11 +149,37 @@ namespace mongo {
         return Status::OK();
     }
 
-    void BatchSafeWriter::extractGLEStats( const BSONObj& gleResponse, GLEStats* stats ) {
+    void BatchSafeWriter::extractGLEStats(const BSONObj& gleResponse,
+                                          const BatchItemRef& batchItem,
+                                          GLEStats* stats) {
+
         stats->n = gleResponse["n"].numberInt();
+
         if ( !gleResponse["upserted"].eoo() ) {
             stats->upsertedId = gleResponse["upserted"].wrap( "upserted" );
         }
+        else if (batchItem.getOpType() == BatchedCommandRequest::BatchType_Update
+                 && !gleResponse["updatedExisting"].eoo()
+                 && !gleResponse["updatedExisting"].trueValue() && stats->n == 1) {
+
+            // v2.4 doesn't include the upserted field when the upserted id isn't an OID.
+            // Note that updatedExisting only appears in GLE after update.
+
+            // Rip out the upserted _id from the request item
+            // This is only safe b/c v2.4 doesn't allow modifiers on _id
+            BSONObj upsertedId;
+            if (!batchItem.getUpdate()->getUpdateExpr()["_id"].eoo()) {
+                upsertedId = batchItem.getUpdate()->getUpdateExpr()["_id"].wrap("upserted");
+            }
+            else if (!batchItem.getUpdate()->getQuery()["_id"].eoo()) {
+                upsertedId = batchItem.getUpdate()->getQuery()["_id"].wrap("upserted");
+            }
+
+            dassert(!upsertedId.isEmpty());
+
+            stats->upsertedId = upsertedId;
+        }
+
         if ( gleResponse["lastOp"].type() == Timestamp ) {
             stats->lastOp = gleResponse["lastOp"]._opTime();
         }
@@ -223,7 +249,7 @@ namespace mongo {
             //
 
             GLEStats stats;
-            extractGLEStats( gleResult, &stats );
+            extractGLEStats( gleResult, itemRef, &stats );
 
             // Special case for making legacy "n" field result for insert match the write
             // command result.
diff --git a/src/mongo/s/write_ops/batch_downconvert.h b/src/mongo/s/write_ops/batch_downconvert.h
index 521a48143eb..387ab2ce567 100644
--- a/src/mongo/s/write_ops/batch_downconvert.h
+++ b/src/mongo/s/write_ops/batch_downconvert.h
@@ -126,7 +126,9 @@ namespace mongo {
         /**
          * Given a GLE response, pulls out stats for the previous write operation.
          */
-        static void extractGLEStats( const BSONObj& gleResponse, GLEStats* stats );
+        static void extractGLEStats(const BSONObj& gleResponse,
+                                    const BatchItemRef& batchItem,
+                                    GLEStats* stats);
 
         /**
          * Given a GLE response, strips out all non-write-concern related information
diff --git a/src/mongo/s/write_ops/batch_downconvert_test.cpp b/src/mongo/s/write_ops/batch_downconvert_test.cpp
index b1d47ed4beb..024547ad470 100644
--- a/src/mongo/s/write_ops/batch_downconvert_test.cpp
+++ b/src/mongo/s/write_ops/batch_downconvert_test.cpp
@@ -57,7 +57,8 @@ namespace {
         ASSERT( !errors.wcError.get() );
 
         BatchSafeWriter::GLEStats stats;
-        BatchSafeWriter::extractGLEStats( gleResponse, &stats );
+        const BatchedCommandRequest dummyRequest(BatchedCommandRequest::BatchType_Insert);
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&dummyRequest, 0), &stats);
         ASSERT_EQUALS( stats.n, 0 );
         ASSERT( stats.upsertedId.isEmpty() );
     }
@@ -186,11 +187,51 @@ namespace {
         ASSERT( !errors.wcError.get() );
 
         BatchSafeWriter::GLEStats stats;
-        BatchSafeWriter::extractGLEStats( gleResponse, &stats );
+        const BatchedCommandRequest dummyRequest(BatchedCommandRequest::BatchType_Delete);
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&dummyRequest, 0), &stats);
         ASSERT_EQUALS( stats.n, 2 );
         ASSERT( stats.upsertedId.isEmpty() );
     }
 
+    TEST(GLEParsing, UpsertedStats) {
+        const BSONObj gleResponse = fromjson( "{ok: 1.0, n: 1, upserted: 'abcde',"
+                                              " updatedExisting: false}" );
+
+        BatchSafeWriter::GLEStats stats;
+        const BatchedCommandRequest dummyRequest(BatchedCommandRequest::BatchType_Update);
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&dummyRequest, 0), &stats);
+        ASSERT_EQUALS( stats.n, 1 );
+        ASSERT_EQUALS( stats.upsertedId.firstElement().str(), "abcde" );
+    }
+
+    TEST(GLEParsing, UpsertedV24Stats) {
+
+        // v24 doesn't give us back non-OID upserted ids
+
+        BatchedCommandRequest request(BatchedCommandRequest::BatchType_Update);
+
+        auto_ptr<BatchedUpdateDocument> updateDoc(new BatchedUpdateDocument);
+        updateDoc->setQuery(BSON("_id" << "abcde"));
+        updateDoc->setUpdateExpr(BSONObj());
+        request.getUpdateRequest()->addToUpdates(updateDoc.release());
+
+        updateDoc.reset(new BatchedUpdateDocument);
+        updateDoc->setQuery(BSON("_id" << "12345"));
+        updateDoc->setUpdateExpr(BSON("_id" << "abcde"));
+        request.getUpdateRequest()->addToUpdates(updateDoc.release());
+
+        const BSONObj gleResponse = fromjson("{ok: 1.0, n: 1, updatedExisting: false}");
+
+        BatchSafeWriter::GLEStats stats;
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&request, 0), &stats);
+        ASSERT_EQUALS(stats.n, 1);
+        ASSERT_EQUALS(stats.upsertedId.firstElement().str(), "abcde");
+
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&request, 1), &stats);
+        ASSERT_EQUALS(stats.n, 1);
+        ASSERT_EQUALS(stats.upsertedId.firstElement().str(), "abcde");
+    }
+
     TEST(GLEParsing, ReplTimeoutErrWithStats) {
         const BSONObj gleResponse =
             fromjson( "{ok: 1.0, err: 'timeout', errmsg: 'message', wtimeout: true,"
@@ -205,7 +246,8 @@ namespace {
         ASSERT_EQUALS( errors.wcError->getErrCode(), ErrorCodes::WriteConcernFailed );
 
         BatchSafeWriter::GLEStats stats;
-        BatchSafeWriter::extractGLEStats( gleResponse, &stats );
+        const BatchedCommandRequest dummyRequest(BatchedCommandRequest::BatchType_Insert);
+        BatchSafeWriter::extractGLEStats(gleResponse, BatchItemRef(&dummyRequest, 0), &stats);
         ASSERT_EQUALS( stats.n, 1 );
         ASSERT_EQUALS( stats.upsertedId.firstElement().str(), "abcde" );
     }
diff --git a/src/mongo/shell/bulk_api.js b/src/mongo/shell/bulk_api.js
index 5ee32608613..6d056064f81 100644
--- a/src/mongo/shell/bulk_api.js
+++ b/src/mongo/shell/bulk_api.js
@@ -1034,8 +1034,25 @@ var _bulk_api_module = (function() {
         }
 
         if(_legacyOp.batchType == UPDATE) {
-          if(result.upserted) {
+
+          // Unfortunately v2.4 GLE does not include the upserted field when
+          // the upserted _id is non-OID type.  We can detect this by the
+          // updatedExisting field + an n of 1
+          var upserted = result.upserted !== undefined ||
+                         (result.updatedExisting === false && result.n == 1);
+
+          if(upserted) {
             batchResult.n = batchResult.n + 1;
+
+            // If we don't have an upserted value, see if we can pull it from the update or the
+            // query
+            if (result.upserted === undefined) {
+                result.upserted = _legacyOp.operation.u._id;
+                if (result.upserted === undefined) {
+                    result.upserted = _legacyOp.operation.q._id;
+                }
+            }
+
             batchResult.upserted.push({
                 index: _legacyOp.index
               , _id: result.upserted