diff options
author | Tess Avitabile <tess.avitabile@mongodb.com> | 2016-11-17 14:09:29 -0500 |
---|---|---|
committer | Tess Avitabile <tess.avitabile@mongodb.com> | 2016-11-17 17:06:33 -0500 |
commit | 5c5fe01994749cdd9ef35d0bf13e0fd67abf7e31 (patch) | |
tree | 9cb09486d67f498eff1e426dacb06de0348c5fd2 | |
parent | b9c8db05bbdb8001eca3139dc76dbc9bcfe7470e (diff) | |
download | mongo-5c5fe01994749cdd9ef35d0bf13e0fd67abf7e31.tar.gz |
Revert "SERVER-14662 Reject ambiguous positional projections and updates"
This reverts commit 2eea3f09aea9c92df9aa0d4e47840869bf04d7b8.
-rw-r--r-- | jstests/core/and2.js | 14 | ||||
-rw-r--r-- | jstests/core/elemMatchProjection.js | 34 | ||||
-rw-r--r-- | jstests/core/updatel.js | 49 | ||||
-rw-r--r-- | src/mongo/db/exec/projection_exec.cpp | 11 | ||||
-rw-r--r-- | src/mongo/db/exec/projection_exec_test.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/exec/update.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/matcher/expression.h | 14 | ||||
-rw-r--r-- | src/mongo/db/matcher/match_details.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/matcher/match_details.h | 5 |
9 files changed, 63 insertions, 79 deletions
diff --git a/jstests/core/and2.js b/jstests/core/and2.js index f3b014c13ad..a794268130b 100644 --- a/jstests/core/and2.js +++ b/jstests/core/and2.js @@ -11,3 +11,17 @@ t.drop(); t.save({a: [1, 2]}); t.update({$and: [{a: 1}]}, {$set: {'a.$': 5}}); assert.eq([5, 2], t.findOne().a); + +// Make sure dollar sign operator with $and is consistent with no $and case +t.drop(); +t.save({a: [1, 2], b: [3, 4]}); +t.update({a: 1, b: 4}, {$set: {'a.$': 5}}); +// Probably not what we want here, just trying to make sure $and is consistent +assert.eq({a: [1, 5], b: [3, 4]}, t.find({}, {_id: 0}).toArray()[0]); + +// Make sure dollar sign operator with $and is consistent with no $and case +t.drop(); +t.save({a: [1, 2], b: [3, 4]}); +t.update({a: 1, $and: [{b: 4}]}, {$set: {'a.$': 5}}); +// Probably not what we want here, just trying to make sure $and is consistent +assert.eq({a: [1, 5], b: [3, 4]}, t.find({}, {_id: 0}).toArray()[0]); diff --git a/jstests/core/elemMatchProjection.js b/jstests/core/elemMatchProjection.js index e7aa5194607..4e80e8a296e 100644 --- a/jstests/core/elemMatchProjection.js +++ b/jstests/core/elemMatchProjection.js @@ -214,14 +214,6 @@ assert.eq( t.find({group: 10}, {_id: 0, x: {$elemMatch: {a: 1}}, y: {$elemMatch: {c: 3}}}).toArray()[0], "multiple $elemMatch on unique fields 1"); -assert.eq({"x": [{"y": [{"a": 1, "b": 2}, {"a": 3, "b": 4}]}]}, - t.find({group: 8}, {_id: 0, x: {$elemMatch: {y: {$elemMatch: {a: 3}}}}}).toArray()[0], - "nested $elemMatch"); - -assert.throws(function() { - t.find({group: 3, 'x.a': 1}, {'x.$': 1, y: {$elemMatch: {aa: 1}}}).toArray(); -}, [], "throw on positional operator with $elemMatch"); - if (false) { assert.eq(2, // SERVER-1243: handle multiple $elemMatch results t.find({group: 4}, {x: {$elemMatchAll: {a: {$lte: 2}}}}).toArray()[0].x.length, @@ -254,3 +246,29 @@ a = t.find({group: 3}, {x: {$elemMatch: {a: 1}}}).batchSize(1); while (a.hasNext()) { assert.eq(1, a.next().x[0].a, "positional getMore test"); } + +// verify the positional update operator matches the same element as the the positional find. this +// is to ensure consistent behavior with updates until SERVER-1013 is resolved, at which point the +// following tests should be updated. + +t.update({group: 10, 'x.a': 3, 'y.c': 1}, {$set: {'x.$': 100}}, false, true); +// updated the wrong element, so the following assertions should be true +assert.eq(100, + t.find({group: 10, 'y.c': 1, x: 100}, {'x.$': 1}).toArray()[0].x[0], + "wrong single element match after update"); + +assert.eq(100, + t.find({group: 10, x: 100, 'y.c': 1}, {'x.$': 1}).toArray()[0].x[0], + "wrong single element match after update"); + +t.remove({group: 10}); +t.insert({group: 10, x: [{a: 1, b: 2}, {a: 3, b: 4}], y: [{c: 1, d: 2}, {c: 3, d: 4}]}); + +t.update({group: 10, 'y.c': 1, 'x.a': 3}, {$set: {'x.$': 100}}, false, true); +// updated the correct element +assert.eq(100, + t.find({group: 10, 'y.c': 1, x: 100}, {'x.$': 1}).toArray()[0].x[0], + "right single element match after update"); +assert.eq(100, + t.find({group: 10, x: 100, 'y.c': 1}, {'x.$': 1}).toArray()[0].x[0], + "right single element match after update"); diff --git a/jstests/core/updatel.js b/jstests/core/updatel.js index 69aa997a224..78bd9b252dc 100644 --- a/jstests/core/updatel.js +++ b/jstests/core/updatel.js @@ -11,60 +11,45 @@ t.drop(); // The collection is empty, forcing an upsert. In this case the query has no array position match // to substiture for the positional operator. SERVER-4713 -assert.writeError(t.update({}, {$set: {'a.$.b': 1}}, true)); +res = t.update({}, {$set: {'a.$.b': 1}}, true); +assert(res.hasWriteError(), "An error is reported."); assert.eq(0, t.count(), "No upsert occurred."); // Save a document to the collection so it is no longer empty. -assert.writeOK(t.save({_id: 0})); +t.save({_id: 0}); // Now, with an existing document, trigger an update rather than an upsert. The query has no array // position match to substiture for the positional operator. SERVER-6669 -assert.writeError(t.update({}, {$set: {'a.$.b': 1}})); +res = t.update({}, {$set: {'a.$.b': 1}}); +assert(res.hasWriteError(), "An error is reported."); assert.eq([{_id: 0}], t.find().toArray(), "No update occurred."); // Now, try with an update by _id (without a query array match). -assert.writeError(t.update({_id: 0}, {$set: {'a.$.b': 1}})); +res = t.update({_id: 0}, {$set: {'a.$.b': 1}}); +assert(res.hasWriteError(), "An error is reported."); assert.eq([{_id: 0}], t.find().toArray(), "No update occurred."); // Seed the collection with a document suitable for the following check. -assert.writeOK(t.remove({})); -assert.writeOK(t.save({_id: 0, a: [{b: {c: 1}}]})); +t.remove({}); +t.save({_id: 0, a: [{b: {c: 1}}]}); // Now, attempt to apply an update with two nested positional operators. There is a positional // query match for the first positional operator but not the second. Note that dollar sign // substitution for multiple positional opertors is not implemented (SERVER-831). -assert.writeError(t.update({'a.b.c': 1}, {$set: {'a.$.b.$.c': 2}})); +res = t.update({'a.b.c': 1}, {$set: {'a.$.b.$.c': 2}}); +assert(res.hasWriteError(), "An error is reported"); assert.eq([{_id: 0, a: [{b: {c: 1}}]}], t.find().toArray(), "No update occurred."); // SERVER-1155 test an update with the positional operator // that has a regex in the query field t.drop(); -assert.writeOK(t.insert({_id: 1, arr: [{a: "z", b: 1}]})); -assert.writeOK(t.update({"arr.a": /^z$/}, {$set: {"arr.$.b": 2}}, false, true)); +t.insert({_id: 1, arr: [{a: "z", b: 1}]}); +res = t.update({"arr.a": /^z$/}, {$set: {"arr.$.b": 2}}, false, true); +assert.writeOK(res); assert.eq(t.findOne().arr[0], {a: "z", b: 2}); t.drop(); -assert.writeOK(t.insert({_id: 1, arr: [{a: "z", b: 1}, {a: "abc", b: 2}, {a: "lmn", b: 3}]})); -assert.writeOK(t.update({"arr.a": /l/}, {$inc: {"arr.$.b": 2}}, false, true)); +t.insert({_id: 1, arr: [{a: "z", b: 1}, {a: "abc", b: 2}, {a: "lmn", b: 3}]}); +res = t.update({"arr.a": /l/}, {$inc: {"arr.$.b": 2}}, false, true); +assert.writeOK(res); assert.eq(t.findOne().arr[2], {a: "lmn", b: 5}); - -// Test updates with ambiguous positional operator. -t.drop(); -assert.writeOK(t.insert({_id: 0, a: [1, 2]})); -assert.writeError(t.update({$and: [{a: 1}, {a: 2}]}, {$set: {'a.$': 5}})); -assert.eq([{_id: 0, a: [1, 2]}], t.find().toArray(), "No update occurred."); - -t.drop(); -assert.writeOK(t.insert({_id: 0, a: [1], b: [2]})); -assert.writeError(t.update({a: 1, b: 2}, {$set: {'a.$': 5}})); -assert.eq([{_id: 0, a: [1], b: [2]}], t.find().toArray(), "No update occurred."); - -t.drop(); -assert.writeOK(t.insert({_id: 0, a: [1], b: [2]})); -assert.writeError(t.update({a: {$elemMatch: {$lt: 2}}, b: 2}, {$set: {'a.$': 5}})); -assert.eq([{_id: 0, a: [1], b: [2]}], t.find().toArray(), "No update occurred."); - -t.drop(); -assert.writeOK(t.insert({_id: 0, a: [{b: 1}, {c: 2}]})); -assert.writeError(t.update({'a.b': 1, 'a.c': 2}, {$set: {'a.$': 5}})); -assert.eq([{_id: 0, a: [{b: 1}, {c: 2}]}], t.find().toArray(), "No update occurred.");
\ No newline at end of file diff --git a/src/mongo/db/exec/projection_exec.cpp b/src/mongo/db/exec/projection_exec.cpp index 0bc0809e01a..21fef7d571a 100644 --- a/src/mongo/db/exec/projection_exec.cpp +++ b/src/mongo/db/exec/projection_exec.cpp @@ -267,13 +267,6 @@ Status ProjectionExec::transform(WorkingSetMember* member) const { matchDetails.requestElemMatchKey(); verify(NULL != _queryExpression); verify(_queryExpression->matchesBSON(member->obj.value(), &matchDetails)); - - // Performing a positional projection requires valid MatchDetails. For example, - // ambiguity caused by multiple implicit array traversal predicates can lead to invalid - // match details. - if (!matchDetails.isValid()) { - return Status(ErrorCodes::InternalError, "ambiguous positional projection"); - } } Status projStatus = transform(member->obj.value(), &bob, &matchDetails); @@ -406,10 +399,6 @@ Status ProjectionExec::transform(const BSONObj& in, arrayDetails.requestElemMatchKey(); if (matcher->second->matchesBSON(in, &arrayDetails)) { - // Since we create a special matcher for each $elemMatch projection, we should always - // have valid MatchDetails. - invariant(arrayDetails.isValid()); - FieldMap::const_iterator fieldIt = _fields.find(elt.fieldName()); if (_fields.end() == fieldIt) { return Status(ErrorCodes::BadValue, diff --git a/src/mongo/db/exec/projection_exec_test.cpp b/src/mongo/db/exec/projection_exec_test.cpp index b9e240a5a24..fc458b1c3f5 100644 --- a/src/mongo/db/exec/projection_exec_test.cpp +++ b/src/mongo/db/exec/projection_exec_test.cpp @@ -189,12 +189,6 @@ TEST(ProjectionExecTest, TransformPositionalDollar) { // Invalid position $ projections. testTransform("{'a.$': 1}", "{a: {$size: 1}}", "{a: [5]}", false, ""); - - // Ambigous position $ projections. - testTransform("{'a.$': 1}", "{$and: [{a: 1}, {a: 2}]}", "{a: [1, 2]}", false, ""); - testTransform("{'a.$': 1}", "{a: 1, b: 2}", "{a: [1], b: [2]}", false, ""); - testTransform("{'a.$': 1}", "{a: {$elemMatch: {$lt: 2}}, b: 2}", "{a: [1], b: [2]}", false, ""); - testTransform("{'a.$': 1}", "{'a.b': 1, 'a.c': 2}", "{a: [{b: 1}, {c: 2}]}", false, ""); } // diff --git a/src/mongo/db/exec/update.cpp b/src/mongo/db/exec/update.cpp index 7df6d43c3f6..b5dbe6eff99 100644 --- a/src/mongo/db/exec/update.cpp +++ b/src/mongo/db/exec/update.cpp @@ -508,10 +508,6 @@ BSONObj UpdateStage::transformAndUpdate(const Snapshotted<BSONObj>& oldObj, Reco dassert(cq); verify(cq->root()->matchesBSON(oldObj.value(), &matchDetails)); - // If we have matched more than one array position, we cannot perform a positional update - // operation. - uassert(34411, "ambiguous positional update operation", matchDetails.isValid()); - string matchedField; if (matchDetails.hasElemMatchKey()) matchedField = matchDetails.elemMatchKey(); diff --git a/src/mongo/db/matcher/expression.h b/src/mongo/db/matcher/expression.h index a0ae4114991..59e18608cc9 100644 --- a/src/mongo/db/matcher/expression.h +++ b/src/mongo/db/matcher/expression.h @@ -189,16 +189,12 @@ public: // XXX document virtual bool equivalent(const MatchExpression* other) const = 0; - /** - * Determine if a document satisfies the tree-predicate. - * - * The caller may optionally provide a non-null MatchDetails as an out-parameter. For matching - *documents, the MatchDetails provide further info on how the document was - *matched---specifically, which array element matched an array predicate. - * - * The caller must check that the MatchDetails is valid via the isValid() method before using. - */ + // + // Determine if a document satisfies the tree-predicate. + // + virtual bool matches(const MatchableDocument* doc, MatchDetails* details = 0) const = 0; + virtual bool matchesBSON(const BSONObj& doc, MatchDetails* details = 0) const; /** diff --git a/src/mongo/db/matcher/match_details.cpp b/src/mongo/db/matcher/match_details.cpp index 3d221a91562..6675bf08e15 100644 --- a/src/mongo/db/matcher/match_details.cpp +++ b/src/mongo/db/matcher/match_details.cpp @@ -38,7 +38,7 @@ namespace mongo { using std::string; -MatchDetails::MatchDetails() : _elemMatchKeyRequested(false), _isValid(true) { +MatchDetails::MatchDetails() : _elemMatchKeyRequested() { resetOutput(); } @@ -58,9 +58,6 @@ std::string MatchDetails::elemMatchKey() const { void MatchDetails::setElemMatchKey(const std::string& elemMatchKey) { if (_elemMatchKeyRequested) { - if (_elemMatchKey) { - _isValid = false; - } _elemMatchKey.reset(new std::string(elemMatchKey)); } } diff --git a/src/mongo/db/matcher/match_details.h b/src/mongo/db/matcher/match_details.h index aaaa64e4ccd..15a0a606678 100644 --- a/src/mongo/db/matcher/match_details.h +++ b/src/mongo/db/matcher/match_details.h @@ -73,14 +73,9 @@ public: void setElemMatchKey(const std::string& elemMatchKey); - bool isValid() const { - return _isValid; - } - private: bool _loadedRecord; bool _elemMatchKeyRequested; - bool _isValid; std::unique_ptr<std::string> _elemMatchKey; }; } |