SERVER-52307 Enable dots and dollars feature flag

Author: alyacb

src/mongo/db/pipeline/expression.cpp

@@ -7226,9 +7226,11 @@ void ExpressionDateTrunc::_doAddDependencies(DepsTracker* deps) const {
 }
 
 /* -------------------------- ExpressionGetField ------------------------------ */
-REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION(getField,
-                                         ExpressionGetField::parse,
-                                         feature_flags::gFeatureFlagDotsAndDollars);
+REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION_WITH_MIN_VERSION(
+    getField,
+    ExpressionGetField::parse,
+    feature_flags::gFeatureFlagDotsAndDollars,
+    ServerGlobalParams::FeatureCompatibility::Version::kVersion50);
 
 intrusive_ptr<Expression> ExpressionGetField::parse(ExpressionContext* const expCtx,
                                                     BSONElement expr,
@@ -7335,14 +7337,18 @@ Value ExpressionGetField::serialize(const bool explain) const {
 }
 
 /* -------------------------- ExpressionSetField ------------------------------ */
-REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION(setField,
-                                         ExpressionSetField::parse,
-                                         feature_flags::gFeatureFlagDotsAndDollars);
+REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION_WITH_MIN_VERSION(
+    setField,
+    ExpressionSetField::parse,
+    feature_flags::gFeatureFlagDotsAndDollars,
+    ServerGlobalParams::FeatureCompatibility::Version::kVersion50);
 
 // $unsetField is syntactic sugar for $setField where value is set to $$REMOVE.
-REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION(unsetField,
-                                         ExpressionSetField::parse,
-                                         feature_flags::gFeatureFlagDotsAndDollars);
+REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION_WITH_MIN_VERSION(
+    unsetField,
+    ExpressionSetField::parse,
+    feature_flags::gFeatureFlagDotsAndDollars,
+    ServerGlobalParams::FeatureCompatibility::Version::kVersion50);
 
 intrusive_ptr<Expression> ExpressionSetField::parse(ExpressionContext* const expCtx,
                                                     BSONElement expr,

src/mongo/db/pipeline/expression.h

@@ -80,20 +80,20 @@ class DocumentSource;
 
 /**
  * Registers a Parser so it can be called from parseExpression and friends (but only if
- * 'featureFlag' is enabled).
+ * 'featureFlag' is enabled) in a feature compatibility version >= X.
  *
  * As an example, if your expression looks like {"$foo": [1,2,3]} and should be flag-guarded by
- * feature_flags::gFoo, you would add this line:
- * REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION(foo, ExpressionFoo::parse, feature_flags::gFoo);
- *
- * An expression registered this way can be used in any featureCompatibilityVersion.
+ * feature_flags::gFoo and version >= X, you would add this line:
+ * REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION_WITH_MIN_VERSION(
+ *     foo, ExpressionFoo::parse, feature_flags::gFoo, X);
  */
-#define REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION(key, parser, featureFlag)    \
+#define REGISTER_FEATURE_FLAG_GUARDED_EXPRESSION_WITH_MIN_VERSION(            \
+    key, parser, featureFlag, minVersion)                                     \
     MONGO_INITIALIZER_GENERAL(                                                \
         addToExpressionParserMap_##key, ("default"), ("expressionParserMap")) \
     (InitializerContext*) {                                                   \
         if (featureFlag.isEnabledAndIgnoreFCV()) {                            \
-            Expression::registerExpression("$" #key, (parser), boost::none);  \
+            Expression::registerExpression("$" #key, (parser), (minVersion)); \
         }                                                                     \
     }
 

src/mongo/db/query/query_feature_flags.idl

@@ -52,4 +52,5 @@ feature_flags:
     featureFlagDotsAndDollars:
       description: "Feature flag for allowing use of field names containing dots and dollars"
       cpp_varname: gFeatureFlagDotsAndDollars
-      default: false
+      default: true
+      version: 5.0

src/mongo/db/update/rename_node_test.cpp

@@ -36,6 +36,7 @@
 #include "mongo/db/json.h"
 #include "mongo/db/pipeline/expression_context_for_test.h"
 #include "mongo/db/update/update_node_test_fixture.h"
+#include "mongo/idl/server_parameter_test_util.h"
 #include "mongo/unittest/death_test.h"
 #include "mongo/unittest/unittest.h"
 
@@ -473,6 +474,7 @@ TEST_F(RenameNodeTest, RenameFromNonExistentPathIsNoOp) {
 }
 
 TEST_F(RenameNodeTest, ApplyCannotRemoveRequiredPartOfDBRef) {
+    RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", false);
     auto update = fromjson("{$rename: {'a.$id': 'b'}}");
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     RenameNode node;
@@ -579,6 +581,7 @@ TEST_F(RenameNodeTest, ApplyCanRemoveImmutablePathIfNoop) {
 }
 
 TEST_F(RenameNodeTest, ApplyCannotCreateDollarPrefixedField) {
+    RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", false);
     auto update = fromjson("{$rename: {a: '$bad'}}");
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     RenameNode node;

src/mongo/db/update/set_node_test.cpp

@@ -1053,6 +1053,7 @@ TEST_F(SetNodeTest, ApplyCannotCreateDollarPrefixedFieldInsideSetElement) {
 }
 
 TEST_F(SetNodeTest, ApplyCannotCreateDollarPrefixedFieldAtStartOfPath) {
+    RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", false);
     auto update = fromjson("{$set: {'$bad.a': 1}}");
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     SetNode node;

src/mongo/db/update/unset_node_test.cpp

@@ -36,6 +36,7 @@
 #include "mongo/db/json.h"
 #include "mongo/db/pipeline/expression_context_for_test.h"
 #include "mongo/db/update/update_node_test_fixture.h"
+#include "mongo/idl/server_parameter_test_util.h"
 #include "mongo/unittest/death_test.h"
 #include "mongo/unittest/unittest.h"
 
@@ -369,18 +370,43 @@ TEST_F(UnsetNodeTest, ApplyFieldWithDot) {
 }
 
 TEST_F(UnsetNodeTest, ApplyCannotRemoveRequiredPartOfDBRef) {
-    auto update = fromjson("{$unset: {'a.$id': true}}");
-    boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
-    UnsetNode node;
-    ASSERT_OK(node.init(update["$unset"]["a.$id"], expCtx));
-
-    mutablebson::Document doc(fromjson("{a: {$ref: 'c', $id: 0}}"));
-    setPathTaken(makeRuntimeUpdatePathForTest("a.$id"));
-    ASSERT_THROWS_CODE_AND_WHAT(
-        node.apply(getApplyParams(doc.root()["a"]["$id"]), getUpdateNodeApplyParams()),
-        AssertionException,
-        ErrorCodes::InvalidDBRef,
-        "The DBRef $ref field must be followed by a $id field");
+    {
+        RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", false);
+        auto update = fromjson("{$unset: {'a.$id': true}}");
+        boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+        UnsetNode node;
+        ASSERT_OK(node.init(update["$unset"]["a.$id"], expCtx));
+
+        mutablebson::Document doc(fromjson("{a: {$ref: 'c', $id: 0}}"));
+        setPathTaken(makeRuntimeUpdatePathForTest("a.$id"));
+        ASSERT_THROWS_CODE_AND_WHAT(
+            node.apply(getApplyParams(doc.root()["a"]["$id"]), getUpdateNodeApplyParams()),
+            AssertionException,
+            ErrorCodes::InvalidDBRef,
+            "The DBRef $ref field must be followed by a $id field");
+    }
+    {
+        RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", true);
+        auto update = fromjson("{$unset: {'a.$id': true}}");
+        boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+        UnsetNode node;
+        ASSERT_OK(node.init(update["$unset"]["a.$id"], expCtx));
+
+        mutablebson::Document doc(fromjson("{a: {$ref: 'c', $id: 0}}"));
+        setPathTaken(makeRuntimeUpdatePathForTest("a.$id"));
+        auto result =
+            node.apply(getApplyParams(doc.root()["a"]["$id"]), getUpdateNodeApplyParams());
+        ASSERT_FALSE(result.noop);
+        ASSERT_FALSE(result.indexesAffected);
+        auto updated = BSON("a" << BSON("$ref"
+                                        << "c"));
+        ASSERT_EQUALS(updated, doc);
+        ASSERT_FALSE(doc.isInPlaceModeEnabled());
+
+        assertOplogEntry(fromjson("{$unset: {'a.$id': true}}"),
+                         fromjson("{$v: 2, diff: {sa: {d: {$id: false}}}}"));
+        ASSERT_EQUALS(getModifiedPaths(), "{a.$id}");
+    }
 }
 
 TEST_F(UnsetNodeTest, ApplyCanRemoveRequiredPartOfDBRefIfValidateForStorageIsFalse) {

src/mongo/dbtests/updatetests.cpp

@@ -44,6 +44,7 @@
 #include "mongo/db/lasterror.h"
 #include "mongo/db/ops/update.h"
 #include "mongo/dbtests/dbtests.h"
+#include "mongo/idl/server_parameter_test_util.h"
 
 namespace UpdateTests {
 
@@ -1740,8 +1741,17 @@ class PreserveIdWithIndex : public SetBase {  // Not using $set, but base class
 class CheckNoMods : public SetBase {
 public:
     void run() {
-        _client.update(ns(), BSONObj(), BSON("i" << 5 << "$set" << BSON("q" << 3)), true);
-        ASSERT(error());
+        {
+            RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", false);
+            _client.update(ns(), BSONObj(), BSON("i" << 5 << "$set" << BSON("q" << 3)), true);
+            ASSERT(error());
+        }
+        {
+            RAIIServerParameterControllerForTest controller("featureFlagDotsAndDollars", true);
+            _client.update(ns(), BSONObj(), BSON("_id" << 52307 << "$set" << BSON("q" << 3)), true);
+            ASSERT_BSONOBJ_EQ(fromjson("{'_id':52307,$set:{q:3}}"),
+                              _client.findOne(ns(), Query(BSON("_id" << 52307))));
+        }
     }
 };