diff --git a/History.md b/History.md index 68ff8fa9f1..99af701950 100644 --- a/History.md +++ b/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 diff --git a/packages/minimongo/minimongo.js b/packages/minimongo/minimongo.js index b97663a8a7..28df4180ec 100644 --- a/packages/minimongo/minimongo.js +++ b/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 +}; diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 8e85819d29..6d1e5b9e3c 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/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 diff --git a/packages/minimongo/modify.js b/packages/minimongo/modify.js index e0df49a274..8ebd7a6a05 100644 --- a/packages/minimongo/modify.js +++ b/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. diff --git a/packages/minimongo/package.js b/packages/minimongo/package.js index c389236fd5..e576c8ecf0 100644 --- a/packages/minimongo/package.js +++ b/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', diff --git a/packages/minimongo/selector.js b/packages/minimongo/selector.js index affe510ebf..840d06c9ed 100644 --- a/packages/minimongo/selector.js +++ b/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; -}; diff --git a/packages/minimongo/upsert_document.js b/packages/minimongo/upsert_document.js new file mode 100644 index 0000000000..e5d9d864b7 --- /dev/null +++ b/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.'); + } +} diff --git a/packages/mongo/collection.js b/packages/mongo/collection.js index 4bb0993f1b..b22eb0248a 100644 --- a/packages/mongo/collection.js +++ b/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; } } diff --git a/packages/mongo/mongo_driver.js b/packages/mongo/mongo_driver.js index 9922b0394d..476fe7199a 100644 --- a/packages/mongo/mongo_driver.js +++ b/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) { diff --git a/packages/mongo/mongo_livedata_tests.js b/packages/mongo/mongo_livedata_tests.js index 1e1ea0174e..1df0a927e2 100644 --- a/packages/mongo/mongo_livedata_tests.js +++ b/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; }