SERVER-29912 Require argument to $pop to be 1 or -1.

3 files changed, 33 insertions, 14 deletions

diff --git a/src/mongo/db/update/pop_node.cpp b/src/mongo/db/update/pop_node.cpp
index 38a36157382..5ec86bdc14d 100644
--- a/src/mongo/db/update/pop_node.cpp
+++ b/src/mongo/db/update/pop_node.cpp
@@ -30,12 +30,21 @@
 
 #include "mongo/db/update/pop_node.h"
 
+#include "mongo/db/matcher/expression_parser.h"
 #include "mongo/db/update/path_support.h"
 
 namespace mongo {
 
 Status PopNode::init(BSONElement modExpr, const CollatorInterface* collator) {
-    _popFromFront = modExpr.isNumber() && modExpr.number() < 0;
+    auto popVal = MatchExpressionParser::parseIntegerElementToLong(modExpr);
+    if (!popVal.isOK()) {
+        return popVal.getStatus();
+    }
+    if (popVal.getValue() != 1LL && popVal.getValue() != -1LL) {
+        return {ErrorCodes::FailedToParse,
+                str::stream() << "$pop expects 1 or -1, found: " << popVal.getValue()};
+    }
+    _popFromFront = (popVal.getValue() == -1LL);
     return Status::OK();
 }
 
diff --git a/src/mongo/db/update/pop_node_test.cpp b/src/mongo/db/update/pop_node_test.cpp
index 9866aeff435..9442544af47 100644
--- a/src/mongo/db/update/pop_node_test.cpp
+++ b/src/mongo/db/update/pop_node_test.cpp
@@ -56,36 +56,46 @@ TEST(PopNodeTest, InitSucceedsNegativeOne) {
     ASSERT_TRUE(popNode.popFromFront());
 }
 
-TEST(PopNodeTest, InitSucceedsZero) {
+TEST(PopNodeTest, InitFailsOnePointOne) {
+    auto update = fromjson("{$pop: {a: 1.1}}");
+    const CollatorInterface* collator = nullptr;
+    PopNode popNode;
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
+}
+
+TEST(PopNodeTest, InitFailsZero) {
     auto update = fromjson("{$pop: {a: 0}}");
     const CollatorInterface* collator = nullptr;
     PopNode popNode;
-    ASSERT_OK(popNode.init(update["$pop"]["a"], collator));
-    ASSERT_FALSE(popNode.popFromFront());
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
 }
 
-TEST(PopNodeTest, InitSucceedsString) {
+TEST(PopNodeTest, InitFailsString) {
     auto update = fromjson("{$pop: {a: 'foo'}}");
     const CollatorInterface* collator = nullptr;
     PopNode popNode;
-    ASSERT_OK(popNode.init(update["$pop"]["a"], collator));
-    ASSERT_FALSE(popNode.popFromFront());
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
 }
 
-TEST(PopNodeTest, InitSucceedsNestedObject) {
+TEST(PopNodeTest, InitFailsNestedObject) {
     auto update = fromjson("{$pop: {a: {b: 1}}}");
     const CollatorInterface* collator = nullptr;
     PopNode popNode;
-    ASSERT_OK(popNode.init(update["$pop"]["a"], collator));
-    ASSERT_FALSE(popNode.popFromFront());
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
 }
 
-TEST(PopNodeTest, InitSucceedsNestedArray) {
+TEST(PopNodeTest, InitFailsNestedArray) {
     auto update = fromjson("{$pop: {a: [{b: 1}]}}");
     const CollatorInterface* collator = nullptr;
     PopNode popNode;
-    ASSERT_OK(popNode.init(update["$pop"]["a"], collator));
-    ASSERT_FALSE(popNode.popFromFront());
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
+}
+
+TEST(PopNodeTest, InitFailsBool) {
+    auto update = fromjson("{$pop: {a: true}}");
+    const CollatorInterface* collator = nullptr;
+    PopNode popNode;
+    ASSERT_EQ(ErrorCodes::FailedToParse, popNode.init(update["$pop"]["a"], collator));
 }
 
 TEST(PopNodeTest, NoopWhenFirstPathComponentDoesNotExist) {
diff --git a/src/mongo/db/update/update_object_node_test.cpp b/src/mongo/db/update/update_object_node_test.cpp
index 524b5ec8295..61607e2292a 100644
--- a/src/mongo/db/update/update_object_node_test.cpp
+++ b/src/mongo/db/update/update_object_node_test.cpp
@@ -163,7 +163,7 @@ TEST(UpdateObjectNodeTest, ValidAddToSetPathParsesSuccessfully) {
 }
 
 TEST(UpdateObjectNodeTest, ValidPopPathParsesSuccessfully) {
-    auto update = fromjson("{$pop: {'a.b': 5}}");
+    auto update = fromjson("{$pop: {'a.b': 1}}");
     const CollatorInterface* collator = nullptr;
     std::map<StringData, std::unique_ptr<ArrayFilter>> arrayFilters;
     std::set<std::string> foundIdentifiers;