Merge branch 'fix/upserts' into devel (#8815).

History.md

@@ -42,6 +42,11 @@
   [PR #8754](https://github.com/meteor/meteor/pull/8754)
   [Issue #1173](https://github.com/meteor/meteor/issues/1173)
 
+* The `minimongo` and `mongo` packages are now compliant with the upsert behavior
+  of MongoDB 2.6 and higher. **As a result support for MongoDB 2.4 has been dropped.**
+  This mainly changes the effect of the selector on newly inserted documents.
+  [PR #8815](https://github.com/meteor/meteor/pull/8815)
+
 * `reactive-dict` now supports setting initial data when defining a named
   `ReactiveDict`. No longer run migration logic when used on the server,
   this is to prevent duplicate name error on reloads. Initial data is now

packages/minimongo/minimongo.js

@@ -1,4 +1,5 @@
 import { assertHasValidFieldNames } from './validation.js';
+import populateDocumentWithQueryFields from './upsert_document.js';
 
 // XXX type checking on selectors (graceful error if malformed)
 
@@ -783,22 +784,7 @@ LocalCollection.prototype.update = function (selector, mod, options, callback) {
   var insertedId;
   if (updateCount === 0 && options.upsert) {
 
-    let selectorModifier = LocalCollection._selectorIsId(selector) 
-      ? { _id: selector } 
-      : selector;
-
-    selectorModifier = LocalCollection._removeDollarOperators(selectorModifier);
-
-    const newDoc = {};
-    if (selectorModifier._id) {
-      newDoc._id = selectorModifier._id;
-      delete selectorModifier._id;
-    }
-
-    // This double _modify call is made to help work around an issue where collection 
-    // upserts won't work properly, with nested properties (see issue #8631).
-    LocalCollection._modify(newDoc, {$set: selectorModifier});
-    LocalCollection._modify(newDoc, mod, {isInsert: true});
+    const newDoc = LocalCollection._createUpsertDocument(selector, mod);
 
     if (! newDoc._id && options.insertedId)
       newDoc._id = options.insertedId;
@@ -1132,3 +1118,50 @@ LocalCollection.prototype.resumeObservers = function () {
   }
   self._observeQueue.drain();
 };
+
+LocalCollection._isModificationMod = function (mod) {
+  var isReplace = false;
+  var isModify = false;
+  for (var k in mod) {
+    if (k.substr(0, 1) === '$') {
+      isModify = true;
+    } else {
+      isReplace = true;
+    }
+  }
+  if (isModify && isReplace) {
+    throw new Error(
+      "Update parameter cannot have both modifier and non-modifier fields.");
+  }
+  return isModify;
+};
+
+// Calculates the document to insert in case we're doing an upsert and the selector
+// does not match any elements
+LocalCollection._createUpsertDocument = function (selector, modifier) {
+  let selectorDocument = populateDocumentWithQueryFields(selector);
+  const isModify = LocalCollection._isModificationMod(modifier);
+  
+  const newDoc = {};
+  if (selectorDocument._id) {
+    newDoc._id = selectorDocument._id;
+    delete selectorDocument._id;
+  }
+
+  // This double _modify call is made to help with nested properties (see issue #8631).
+  // We do this even if it's a replacement for validation purposes (e.g. ambiguous id's)
+  LocalCollection._modify(newDoc, { $set: selectorDocument });
+  LocalCollection._modify(newDoc, modifier, { isInsert: true });
+
+  if (isModify) {
+    return newDoc;
+  }
+
+  // Replacement can take _id from query document
+  const replacement = Object.assign({}, modifier);
+  if (newDoc._id) {
+    replacement._id = newDoc._id;
+  }
+
+  return replacement
+};

packages/minimongo/minimongo_tests.js

@@ -2340,7 +2340,10 @@ Tinytest.add("minimongo - modify", function (test) {
     // The query is relevant for 'a.$.b'.
     coll.update(query, mod);
     var actual = coll.findOne();
-    delete actual._id;  // added by insert
+
+    if (!expected._id) {
+      delete actual._id;  // added by insert
+    }
 
     if (typeof expected === "function") {
       expected(actual, EJSON.stringify({input: doc, mod: mod}));
@@ -2379,6 +2382,21 @@ Tinytest.add("minimongo - modify", function (test) {
     test.equal(actual, expected);
   };
 
+  var upsertUpdate = function (initialDoc, query, mod, expected) {
+    var coll = new LocalCollection;
+
+    coll.insert(initialDoc);
+    var result = coll.upsert(query, mod);
+
+    var actual = coll.findOne();
+
+    if (!expected._id) {
+      delete actual._id;
+    }
+
+    test.equal(actual, expected);
+  }
+
   var upsertException = function (query, mod) {
     var coll = new LocalCollection;
     test.throws(function(){
@@ -2608,8 +2626,12 @@ Tinytest.add("minimongo - modify", function (test) {
   modify({a: [1], b: 2}, {$set: {'a.2': 9}}, {a: [1, null, 9], b: 2});
   modify({a: {b: 1}}, {$set: {'a.c': 9}}, {a: {b: 1, c: 9}});
   modify({}, {$set: {'x._id': 4}}, {x: {_id: 4}});
+  
+  // Changing _id is disallowed
   exception({}, {$set: {_id: 4}});
-  exception({_id: 4}, {$set: {_id: 4}});  // even not-changing _id is bad
+  exception({_id: 1}, {$set: {_id: 4}})
+  modify({_id: 4}, {$set: {_id: 4}}, {_id: 4});  // not-changing _id is not bad
+  
   //restricted field names
   exception({a:{}}, {$set:{a:{$a:1}}});
   exception({ a: {} }, { $set: { a: { c:
@@ -2911,6 +2933,90 @@ Tinytest.add("minimongo - modify", function (test) {
     { foo: {}, bar: 'baz' }
   );
 
+
+  // Tests for https://github.com/meteor/meteor/issues/8806
+
+  upsert({"a": {"b": undefined, "c": null}}, {"$set": {"c": "foo"}}, {"a": {"b": undefined, "c": null}, "c": "foo"})
+  upsert({"a": {"$eq": "bar" }}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+  // $all with 1 statement is similar to $eq
+  upsert({"a": {"$all": ["bar"] }}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+  upsert({"a": {"$eq": "bar" }, "b": "baz"}, {"$set": {"c": "foo"}}, {"a": "bar", "b": "baz", "c": "foo"})
+
+  upsert({"a": {"$exists": true}}, {"$set": {"c": "foo"}}, {"c": "foo"})
+  upsert({"a": {"$exists": true, "$eq": "foo"}}, {"$set": {"c": "foo"}}, {"a": "foo", "c": "foo"})
+  upsert({"a": {"$gt": 3, "$eq": 2}}, {"$set": {"c": "foo"}}, {"a": 2, "c": "foo"})
+
+  // $and
+  upsert({"$and": [{"a": {"$eq": "bar"}}]}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+  upsert({"$and": [{"a": {"$all": ["bar"]}}]}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+  upsert({"$and": [{"a": {"$all": ["bar"]}}]}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+
+  // $or with one statement is handled similar to $and
+  upsert({"$or": [{"a": "bar"}]}, {"$set": {"c": "foo"}}, {"a": "bar", "c": "foo"})
+
+  // $or with multiple statements is ignored
+  upsert({"$or": [{"a": "bar"}, {"b": "baz"}]}, {"$set": {"c": "foo"}}, {"c": "foo"})
+
+  // Negative logical operators are ignored
+  upsert({"$nor": [{"a": "bar"}]}, {"$set": {"c": "foo"}}, {"c": "foo"})
+
+  // Filter out empty objects after filtering out operators
+  upsert({"a": {"$exists": true}}, {"$set": {"c": "foo"}}, {"c": "foo"})
+
+  // But leave actual empty objects
+  upsert({"a": {}}, {"$set": {"c": "foo"}}, {"a": {}, "c": "foo"})
+
+   // Also filter out shorthand regexp notation
+  upsert({"a": /a/}, {"$set": {"c": "foo"}}, {"c": "foo"})
+
+  // Test nested fields
+  upsert({"$and": [{"a.a": "foo"}, {"$or": [{"a.b": "baz"}]}]}, {"$set": {"c": "foo"}}, {"a": {"a": "foo", "b": "baz"}, "c": "foo"})
+
+  // Test for https://github.com/meteor/meteor/issues/5294
+  upsert({"a": {"$ne": 444}}, {"$push": {"a": 123}}, {"a": [123]})
+
+  // Mod takes precedence over query
+  upsert({"a": "foo"}, {"a": "bar"}, {"a": "bar"})
+  upsert({"a": "foo"}, {"$set":{"a": "bar"}}, {"a": "bar"})
+
+  // Replacement can take _id from query
+  upsert({"_id": "foo", "foo": "bar"}, {"bar": "foo"}, {"_id": "foo", "bar": "foo"})
+
+  // Replacement update keeps _id
+  upsertUpdate({"_id": "foo", "bar": "baz"}, {"_id":"foo"}, {"bar": "crow"}, {"_id": "foo", "bar": "crow"});
+  
+  // Nested fields don't work with literal objects
+  upsertException({"a": {}, "a.b": "foo"}, {});
+
+  // You can't have an ambiguious ID
+  upsertException({"_id":"foo"}, {"_id":"bar"});
+  upsertException({"_id":"foo"}, {"$set":{"_id":"bar"}});
+
+  // You can't set the same field twice
+  upsertException({"$and": [{"a": "foo"}, {"a": "foo"}]}, {}); //not even with same value
+  upsertException({"a": {"$all": ["foo", "bar"]}}, {});
+  upsertException({"$and": [{"a": {"$eq": "foo"}}, {"$or": [{"a": {"$all": ["bar"]}}]}]}, {});
+
+  // You can't have nested dotted fields
+  upsertException({"a": {"foo.bar": "baz"}}, {});
+
+  // You can't have dollar-prefixed fields above the first level (logical operators not counted)
+  upsertException({"a": {"a": {"$eq": "foo"}}}, {});
+  upsertException({"a": {"a": {"$exists": true}}}, {});
+
+  // You can't mix operators with other fields
+  upsertException({"a": {"$eq": "bar", "b": "foo"}}, {})
+  upsertException({"a": {"b": "foo", "$eq": "bar"}}, {})
+
+  var mongoIdForUpsert = new MongoID.ObjectID('44915733af80844fa1cef07a');
+  upsert({_id: mongoIdForUpsert},{$setOnInsert: {a: 123}},{a: 123})
+
+  // Test for https://github.com/meteor/meteor/issues/7758
+  upsert({n_id: mongoIdForUpsert, c_n: "bar"},
+    {$set: { t_t_o: "foo"}},
+    {n_id: mongoIdForUpsert, t_t_o: "foo", c_n: "bar"});
+
+
   exception({}, {$set: {_id: 'bad'}});
 
   // $bit

packages/minimongo/modify.js

@@ -25,9 +25,10 @@ LocalCollection._modify = function (doc, mod, options) {
   var newDoc;
 
   if (!isModifier) {
-    if (mod._id && !EJSON.equals(doc._id, mod._id))
-      throw MinimongoError("Cannot change the _id of a document");
-
+    if (mod._id && doc._id && !EJSON.equals(doc._id, mod._id)) {
+      throw MinimongoError(`The _id field cannot be changed from {_id: "${doc._id}"} to {_id: "${mod._id}"}`);
+    }
+    
     // replace the whole document
     assertHasValidFieldNames(mod);
     newDoc = mod;
@@ -47,10 +48,6 @@ LocalCollection._modify = function (doc, mod, options) {
           throw MinimongoError("An empty update path is not valid.");
         }
 
-        if (keypath === '_id' && op !== '$setOnInsert') {
-          throw MinimongoError("Mod on _id not allowed");
-        }
-
         var keyparts = keypath.split('.');
 
         if (! _.all(keyparts, _.identity)) {
@@ -70,6 +67,12 @@ LocalCollection._modify = function (doc, mod, options) {
         modFunc(target, field, arg, keypath, newDoc);
       });
     });
+
+    if (doc._id && !EJSON.equals(doc._id, newDoc._id)) {
+      throw MinimongoError('After applying the update to the document {_id: ' +
+      `"${doc._id}" , ...}, the (immutable) field '_id' was found to have` +
+      ` been altered to _id: "${newDoc._id}"`);
+    }
   }
 
   // move new document into place.

packages/minimongo/package.js

@@ -28,6 +28,7 @@ Package.onUse(function (api) {
     'minimongo.js',
     'wrap_transform.js',
     'helpers.js',
+    'upsert_document.js',
     'selector.js',
     'sort.js',
     'projection.js',

packages/minimongo/selector.js

@@ -1250,33 +1250,3 @@ LocalCollection._f = {
     throw Error("Unknown type to sort");
   }
 };
-
-const objectOnlyHasDollarKeys = (object) => {
-  const keys = Object.keys(object);
-  return keys.length > 0 && keys.every(key => key.charAt(0) === '$');
-};
-
-// When performing an upsert, the incoming selector object can be re-used as
-// the upsert modifier object, as long as Mongo query and projection
-// operators (prefixed with a $ character) are removed from the newly
-// created modifier object. This function attempts to strip all $ based Mongo
-// operators when creating the upsert modifier object.
-// NOTE: There is a known issue here in that some Mongo $ based opeartors
-// should not actually be stripped.
-// See https://github.com/meteor/meteor/issues/8806.
-LocalCollection._removeDollarOperators = (selector) => {
-  let cleansed = {};
-  Object.keys(selector).forEach((key) => {
-    const value = selector[key];
-    if (key.charAt(0) !== '$' && !objectOnlyHasDollarKeys(value)) {
-      if (value !== null
-          && value.constructor
-          && Object.getPrototypeOf(value) === Object.prototype) {
-        cleansed[key] = LocalCollection._removeDollarOperators(value);
-      } else {
-        cleansed[key] = value;
-      }
-    }
-  });
-  return cleansed;
-};

packages/minimongo/upsert_document.js

@@ -0,0 +1,124 @@
+// Creating a document from an upsert is quite tricky.
+// E.g. this selector: {"$or": [{"b.foo": {"$all": ["bar"]}}]}, should result in: {"b.foo": "bar"}
+// But this selector: {"$or": [{"b": {"foo": {"$all": ["bar"]}}}]} should throw an error
+
+// Some rules (found mainly with trial & error, so there might be more):
+// - handle all childs of $and (or implicit $and)
+// - handle $or nodes with exactly 1 child
+// - ignore $or nodes with more than 1 child
+// - ignore $nor and $not nodes
+// - throw when a value can not be set unambiguously
+// - every value for $all should be dealt with as separate $eq-s
+// - threat all children of $all as $eq setters (=> set if $all.length === 1, otherwise throw error)
+// - you can not mix '$'-prefixed keys and non-'$'-prefixed keys
+// - you can only have dotted keys on a root-level
+// - you can not have '$'-prefixed keys more than one-level deep in an object
+
+// Fills a document with certain fields from an upsert selector
+export default function populateDocumentWithQueryFields (query, document = {}) {
+  if (Object.getPrototypeOf(query) === Object.prototype) {
+    // handle implicit $and
+    Object.keys(query).forEach(function (key) {
+      const value = query[key];
+      if (key === '$and') {
+        // handle explicit $and
+        value.forEach(sq => populateDocumentWithQueryFields(sq, document));
+      } else if (key === '$or') {
+        // handle $or nodes with exactly 1 child
+        if (value.length === 1) {
+          populateDocumentWithQueryFields(value[0], document);
+        }
+      } else if (key[0] !== '$') {
+        // Ignore other '$'-prefixed logical selectors
+        populateDocumentWithKeyValue(document, key, value);
+      }
+    })
+  } else {
+    // Handle meteor-specific shortcut for selecting _id
+    if (LocalCollection._selectorIsId(query)) {
+      insertIntoDocument(document, '_id', query);
+    }
+  }
+
+  return document;
+}
+
+// Handles one key/value pair to put in the selector document
+function populateDocumentWithKeyValue (document, key, value) {
+  if (value && Object.getPrototypeOf(value) === Object.prototype) {
+    populateDocumentWithObject(document, key, value);
+  } else if (!(value instanceof RegExp)) {
+    insertIntoDocument(document, key, value);
+  }
+}
+
+// Handles a key, value pair to put in the selector document 
+// if the value is an object
+function populateDocumentWithObject (document, key, value) {
+  const keys = Object.keys(value);
+  const unprefixedKeys = keys.filter(k => k[0] !== '$');
+
+  if (unprefixedKeys.length > 0 || !keys.length) {
+    // Literal (possibly empty) object ( or empty object ) 
+    // Don't allow mixing '$'-prefixed with non-'$'-prefixed fields
+    if (keys.length !== unprefixedKeys.length) {
+      throw new Error(`unknown operator: ${unprefixedKeys[0]}`);
+    }
+    validateObject(value, key);
+    insertIntoDocument(document, key, value);
+  } else {
+    Object.keys(value).forEach(function (k) {
+      const v = value[k];
+      if (k === '$eq') {
+        populateDocumentWithKeyValue(document, key, v);
+      } else if (k === '$all') {
+        // every value for $all should be dealt with as separate $eq-s
+        v.forEach(vx => populateDocumentWithKeyValue(document, key, vx));
+      }
+    });
+  }
+}
+
+// Actually inserts a key value into the selector document
+// However, this checks there is no ambiguity in setting 
+// the value for the given key, throws otherwise
+function insertIntoDocument (document, key, value) {
+  Object.keys(document).forEach(existingKey => {
+    if (
+      (existingKey.length > key.length && existingKey.indexOf(key) === 0)
+      || (key.length > existingKey.length && key.indexOf(existingKey) === 0)
+    ) {
+      throw new Error('cannot infer query fields to set, both paths ' +
+        `'${existingKey}' and '${key}' are matched`);
+    } else if (existingKey === key) {
+      throw new Error(`cannot infer query fields to set, path '${key}' ` +
+        'is matched twice');
+    }
+  });
+
+  document[key] = value;
+}
+
+// Recursively validates an object that is nested more than one level deep
+function validateObject (obj, path) {
+  if (obj && Object.getPrototypeOf(obj) === Object.prototype) {
+    Object.keys(obj).forEach(function (key) {
+      validateKeyInPath(key, path);
+      validateObject(obj[key], path + '.' + key);
+    });
+  }
+}
+
+// Validates the key in a path. 
+// Objects that are nested more then 1 level cannot have dotted fields 
+// or fields starting with '$'
+function validateKeyInPath (key, path) {
+  if (key.includes('.')) {
+    throw new Error(`The dotted field '${key}' in '${path}.${key}' ` +
+      'is not valid for storage.');
+  }
+  if (key[0] === '$') {
+    throw new Error(`The dollar ($) prefixed field  '${path}.${key}' ` +
+      'is not valid for storage.');
+  }
+}

packages/mongo/collection.js

@@ -562,6 +562,7 @@ Mongo.Collection.prototype.update = function update(selector, modifier, ...optio
       insertedId = options.insertedId;
     } else if (!selector || !selector._id) {
       insertedId = this._makeNewID();
+      options.generatedId = true;
       options.insertedId = insertedId;
     }
   }

packages/mongo/mongo_driver.js

@@ -511,10 +511,9 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
     var mongoSelector = replaceTypes(selector, replaceMeteorAtomWithMongo);
     var mongoMod = replaceTypes(mod, replaceMeteorAtomWithMongo);
 
-    var isModify = isModificationMod(mongoMod);
-    var knownId = selector._id || mod._id;
+    var isModify = LocalCollection._isModificationMod(mongoMod);
 
-    if (options._forbidReplace && ! isModify) {
+    if (options._forbidReplace && !isModify) {
       var err = new Error("Invalid modifier. Replacements are forbidden.");
       if (callback) {
         return callback(err);
@@ -523,22 +522,45 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
       }
     }
 
-    if (options.upsert && (! knownId) && options.insertedId) {
-      // XXX If we know we're using Mongo 2.6 (and this isn't a replacement)
-      //     we should be able to just use $setOnInsert instead of this
-      //     simulated upsert thing. (We can't use $setOnInsert with
-      //     replacements because there's nowhere to write it, and $setOnInsert
-      //     can't set _id on Mongo 2.4.)
-      //
-      //     Also, in the future we could do a real upsert for the mongo id
-      //     generation case, if the the node mongo driver gives us back the id
-      //     of the upserted doc (which our current version does not).
-      //
-      //     For more context, see
-      //     https://github.com/meteor/meteor/issues/2278#issuecomment-64252706
+    // We've already run replaceTypes/replaceMeteorAtomWithMongo on
+    // selector and mod.  We assume it doesn't matter, as far as
+    // the behavior of modifiers is concerned, whether `_modify`
+    // is run on EJSON or on mongo-converted EJSON.
+
+    // Run this code up front so that it fails fast if someone uses
+    // a Mongo update operator we don't support.
+    let knownId;
+    if (options.upsert) {
+      try {
+        let newDoc = LocalCollection._createUpsertDocument(selector, mod);
+        knownId = newDoc._id;
+      } catch (err) {
+        if (callback) {
+          return callback(err);
+        } else {
+          throw err;
+        }
+      }
+    }
+
+    if (options.upsert &&
+        ! isModify &&
+        ! knownId &&
+        options.insertedId &&
+        ! (options.insertedId instanceof Mongo.ObjectID &&
+           options.generatedId)) {
+      // In case of an upsert with a replacement, where there is no _id defined
+      // in either the query or the replacement doc, mongo will generate an id itself. 
+      // Therefore we need this special strategy if we want to control the id ourselves.
+
+      // We don't need to do this when:
+      // - This is not a replacement, so we can add an _id to $setOnInsert
+      // - The id is defined by query or mod we can just add it to the replacement doc
+      // - The user did not specify any id preference and the id is a Mongo ObjectId, 
+      //     then we can just let Mongo generate the id
+
       simulateUpsertWithInsertedId(
-        collection, mongoSelector, mongoMod,
-        isModify, options,
+        collection, mongoSelector, mongoMod, options,
         // This callback does not need to be bindEnvironment'ed because
         // simulateUpsertWithInsertedId() wraps it and then passes it through
         // bindEnvironmentForWrite.
@@ -554,6 +576,15 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
         }
       );
     } else {
+      
+      if (options.upsert && !knownId && options.insertedId && isModify) {
+        if (!mongoMod.hasOwnProperty('$setOnInsert')) {
+          mongoMod.$setOnInsert = {};
+        }
+        knownId = options.insertedId;
+        Object.assign(mongoMod.$setOnInsert, replaceTypes({_id: options.insertedId}, replaceMeteorAtomWithMongo));
+      }
+      
       collection.update(
         mongoSelector, mongoMod, mongoOpts,
         bindEnvironmentForWrite(function (err, result) {
@@ -563,10 +594,14 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
               // If this was an upsert() call, and we ended up
               // inserting a new doc and we know its id, then
               // return that id as well.
-
-              if (options.upsert && meteorResult.insertedId && knownId) {
-                meteorResult.insertedId = knownId;
+              if (options.upsert && meteorResult.insertedId) {
+                if (knownId) {
+                  meteorResult.insertedId = knownId;
+                } else if (meteorResult.insertedId instanceof MongoDB.ObjectID) {
+                  meteorResult.insertedId = new Mongo.ObjectID(meteorResult.insertedId.toHexString());
+                }
               }
+
               callback(err, meteorResult);
             } else {
               callback(err, meteorResult.numberAffected);
@@ -582,23 +617,6 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
   }
 };
 
-var isModificationMod = function (mod) {
-  var isReplace = false;
-  var isModify = false;
-  for (var k in mod) {
-    if (k.substr(0, 1) === '$') {
-      isModify = true;
-    } else {
-      isReplace = true;
-    }
-  }
-  if (isModify && isReplace) {
-    throw new Error(
-      "Update parameter cannot have both modifier and non-modifier fields.");
-  }
-  return isModify;
-};
-
 var transformResult = function (driverResult) {
   var meteorResult = { numberAffected: 0 };
   if (driverResult) {
@@ -626,26 +644,18 @@ var NUM_OPTIMISTIC_TRIES = 3;
 
 // exposed for testing
 MongoConnection._isCannotChangeIdError = function (err) {
-  // First check for what this error looked like in Mongo 2.4.  Either of these
-  // checks should work, but just to be safe...
-  if (err.code === 13596) {
-    return true;
-  }
 
   // Mongo 3.2.* returns error as next Object:
-  // {name: String, code: Number, err: String}
-  // Older Mongo returns:
   // {name: String, code: Number, errmsg: String}
+  // Older Mongo returns:
+  // {name: String, code: Number, err: String}
   var error = err.errmsg || err.err;
 
-  if (error.indexOf('cannot change _id of a document') === 0) {
-    return true;
-  }
-
-  // Now look for what it looks like in Mongo 2.6.  We don't use the error code
-  // here, because the error code we observed it producing (16837) appears to be
+  // We don't use the error code here
+  // because the error code we observed it producing (16837) appears to be
   // a far more generic error code based on examining the source.
-  if (error.indexOf('The _id field cannot be changed') === 0) {
+  if (error.indexOf('The _id field cannot be changed') === 0
+    || error.indexOf("the (immutable) field '_id' was found to have been altered to _id") !== -1) {
     return true;
   }
 
@@ -653,66 +663,20 @@ MongoConnection._isCannotChangeIdError = function (err) {
 };
 
 var simulateUpsertWithInsertedId = function (collection, selector, mod,
-                                             isModify, options, callback) {
-  // STRATEGY:  First try doing a plain update.  If it affected 0 documents,
-  // then without affecting the database, we know we should probably do an
-  // insert.  We then do a *conditional* insert that will fail in the case
-  // of a race condition.  This conditional insert is actually an
-  // upsert-replace with an _id, which will never successfully update an
-  // existing document.  If this upsert fails with an error saying it
-  // couldn't change an existing _id, then we know an intervening write has
-  // caused the query to match something.  We go back to step one and repeat.
+                                             options, callback) {
+  // STRATEGY: First try doing an upsert with a generated ID.
+  // If this throws an error about changing the ID on an existing document
+  // then without affecting the database, we know we should probably try
+  // an update without the generated ID. If it affected 0 documents, 
+  // then without affecting the database, we the document that first
+  // gave the error is probably removed and we need to try an insert again
+  // We go back to step one and repeat.
   // Like all "optimistic write" schemes, we rely on the fact that it's
   // unlikely our writes will continue to be interfered with under normal
   // circumstances (though sufficiently heavy contention with writers
   // disagreeing on the existence of an object will cause writes to fail
   // in theory).
 
-  var newDoc;
-  // Run this code up front so that it fails fast if someone uses
-  // a Mongo update operator we don't support.
-  if (isModify) {
-    // We've already run replaceTypes/replaceMeteorAtomWithMongo on
-    // selector and mod.  We assume it doesn't matter, as far as
-    // the behavior of modifiers is concerned, whether `_modify`
-    // is run on EJSON or on mongo-converted EJSON.
-    var selectorDoc = LocalCollection._removeDollarOperators(selector);
-
-    newDoc = selectorDoc;
-
-    // Convert dotted keys into objects. (Resolves issue #4522).
-    _.each(newDoc, function (value, key) {
-      var trail = key.split(".");
-
-      if (trail.length > 1) {
-        //Key is dotted. Convert it into an object.
-        delete newDoc[key];
-
-        var obj = newDoc,
-            leaf = trail.pop();
-
-        // XXX It is not quite certain what should be done if there are clashing
-        // keys on the trail of the dotted key. For now we will just override it
-        // It wouldn't be a very sane query in the first place, but should look
-        // up what mongo does in this case.
-
-        while ((key = trail.shift())) {
-          if (typeof obj[key] !== "object") {
-            obj[key] = {};
-          }
-
-          obj = obj[key];
-        }
-
-        obj[leaf] = value;
-      }
-    });
-
-    LocalCollection._modify(newDoc, mod, {isInsert: true});
-  } else {
-    newDoc = mod;
-  }
-
   var insertedId = options.insertedId; // must exist
   var mongoOptsForUpdate = {
     safe: true,
@@ -723,6 +687,10 @@ var simulateUpsertWithInsertedId = function (collection, selector, mod,
     upsert: true
   };
 
+  var replacementWithId = Object.assign(
+    replaceTypes({_id: insertedId}, replaceMeteorAtomWithMongo),
+    mod);
+
   var tries = NUM_OPTIMISTIC_TRIES;
 
   var doUpdate = function () {
@@ -746,9 +714,6 @@ var simulateUpsertWithInsertedId = function (collection, selector, mod,
   };
 
   var doConditionalInsert = function () {
-    var replacementWithId = _.extend(
-      replaceTypes({_id: insertedId}, replaceMeteorAtomWithMongo),
-      newDoc);
     collection.update(selector, replacementWithId, mongoOptsForInsert,
                       bindEnvironmentForWrite(function (err, result) {
                         if (err) {

packages/mongo/mongo_livedata_tests.js

@@ -1646,10 +1646,10 @@ if (Meteor.isServer) {
     var run = test.runId();
     var coll = new Mongo.Collection("livedata_upsert_errorparse_collection_"+run, collectionOptions);
 
-    coll.insert({_id: 'foobar'});
+    coll.insert({_id:'foobar', foo: 'bar'});
     var err;
     try {
-      coll.update({_id: 'foobar'}, {_id: 'cowbar'});
+      coll.update({foo: 'bar'}, {_id: 'cowbar'});
     } catch (e) {
       err = e;
     }