SERVER-41416 Remove _convertToFieldPaths from MongoProcessCommon

11 files changed, 58 insertions, 63 deletions

diff --git a/src/mongo/db/pipeline/document_source_merge.cpp b/src/mongo/db/pipeline/document_source_merge.cpp
index ad68c53598c..e953e813bc0 100644
--- a/src/mongo/db/pipeline/document_source_merge.cpp
+++ b/src/mongo/db/pipeline/document_source_merge.cpp
@@ -284,6 +284,25 @@ DocumentSourceMergeSpec parseMergeSpecAndResolveTargetNamespace(const BSONElemen
 
     return mergeSpec;
 }
+
+/**
+ * Converts an array of field names into a set of FieldPath. Throws if 'fields' contains
+ * duplicate elements.
+ */
+boost::optional<std::set<FieldPath>> convertToFieldPaths(
+    const boost::optional<std::vector<std::string>>& fields) {
+
+    if (!fields)
+        return boost::none;
+
+    std::set<FieldPath> fieldPaths;
+
+    for (const auto& field : *fields) {
+        const auto res = fieldPaths.insert(FieldPath(field));
+        uassert(31465, str::stream() << "Found a duplicate field '" << field << "'", res.second);
+    }
+    return fieldPaths;
+}
 }  // namespace
 
 std::unique_ptr<DocumentSourceMerge::LiteParsed> DocumentSourceMerge::LiteParsed::parse(
@@ -425,9 +444,10 @@ boost::intrusive_ptr<DocumentSource> DocumentSourceMerge::createFromBson(
         mergeSpec.getWhenMatched() ? mergeSpec.getWhenMatched()->mode : kDefaultWhenMatched;
     auto whenNotMatched = mergeSpec.getWhenNotMatched().value_or(kDefaultWhenNotMatched);
     auto pipeline = mergeSpec.getWhenMatched() ? mergeSpec.getWhenMatched()->pipeline : boost::none;
+    auto fieldPaths = convertToFieldPaths(mergeSpec.getOn());
     auto [mergeOnFields, targetCollectionVersion] =
         expCtx->mongoProcessInterface->ensureFieldsUniqueOrResolveDocumentKey(
-            expCtx, mergeSpec.getOn(), mergeSpec.getTargetCollectionVersion(), targetNss);
+            expCtx, std::move(fieldPaths), mergeSpec.getTargetCollectionVersion(), targetNss);
 
     return DocumentSourceMerge::create(std::move(targetNss),
                                        expCtx,
diff --git a/src/mongo/db/pipeline/document_source_merge_test.cpp b/src/mongo/db/pipeline/document_source_merge_test.cpp
index f78f3195ec1..d58db582df5 100644
--- a/src/mongo/db/pipeline/document_source_merge_test.cpp
+++ b/src/mongo/db/pipeline/document_source_merge_test.cpp
@@ -888,5 +888,22 @@ TEST_F(DocumentSourceMergeTest, OnlyObjectCanBeUsedAsLetVariables) {
     }
 }
 
+TEST_F(DocumentSourceMergeTest, FailsToParseIfOnFieldHaveDuplicates) {
+    auto spec = BSON("$merge" << BSON("into"
+                                      << "target_collection"
+                                      << "on"
+                                      << BSON_ARRAY("x"
+                                                    << "y"
+                                                    << "x")));
+    ASSERT_THROWS_CODE(createMergeStage(spec), AssertionException, 31465);
+
+    spec = BSON("$merge" << BSON("into"
+                                 << "target_collection"
+                                 << "on"
+                                 << BSON_ARRAY("_id"
+                                               << "_id")));
+    ASSERT_THROWS_CODE(createMergeStage(spec), AssertionException, 31465);
+}
+
 }  // namespace
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.cpp b/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.cpp
index ba215f30972..3dd6df322fa 100644
--- a/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.cpp
+++ b/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.cpp
@@ -541,7 +541,7 @@ std::unique_ptr<ResourceYielder> CommonMongodProcessInterface::getResourceYielde
 std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
 CommonMongodProcessInterface::ensureFieldsUniqueOrResolveDocumentKey(
     const boost::intrusive_ptr<ExpressionContext>& expCtx,
-    boost::optional<std::vector<std::string>> fields,
+    boost::optional<std::set<FieldPath>> fieldPaths,
     boost::optional<ChunkVersion> targetCollectionVersion,
     const NamespaceString& outputNs) const {
     if (targetCollectionVersion) {
@@ -551,20 +551,19 @@ CommonMongodProcessInterface::ensureFieldsUniqueOrResolveDocumentKey(
         checkRoutingInfoEpochOrThrow(expCtx, outputNs, *targetCollectionVersion);
     }
 
-    if (!fields) {
+    if (!fieldPaths) {
         uassert(51124, "Expected fields to be provided from mongos", !expCtx->fromMongos);
         return {std::set<FieldPath>{"_id"}, targetCollectionVersion};
     }
 
     // Make sure the 'fields' array has a supporting index. Skip this check if the command is sent
     // from mongos since the 'fields' check would've happened already.
-    auto fieldPaths = _convertToFieldPaths(*fields);
     if (!expCtx->fromMongos) {
         uassert(51183,
                 "Cannot find index to verify that join fields will be unique",
-                fieldsHaveSupportingUniqueIndex(expCtx, outputNs, fieldPaths));
+                fieldsHaveSupportingUniqueIndex(expCtx, outputNs, *fieldPaths));
     }
-    return {fieldPaths, targetCollectionVersion};
+    return {*fieldPaths, targetCollectionVersion};
 }
 
 write_ops::Insert CommonMongodProcessInterface::buildInsertOp(const NamespaceString& nss,
diff --git a/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.h b/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.h
index bbf414a5224..f20b4e0b77d 100644
--- a/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.h
+++ b/src/mongo/db/pipeline/process_interface/common_mongod_process_interface.h
@@ -107,7 +107,7 @@ public:
 
     std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
     ensureFieldsUniqueOrResolveDocumentKey(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                                           boost::optional<std::vector<std::string>> fields,
+                                           boost::optional<std::set<FieldPath>> fieldPaths,
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const final;
 
diff --git a/src/mongo/db/pipeline/process_interface/common_process_interface.cpp b/src/mongo/db/pipeline/process_interface/common_process_interface.cpp
index ae19e6a7635..a0e1057e033 100644
--- a/src/mongo/db/pipeline/process_interface/common_process_interface.cpp
+++ b/src/mongo/db/pipeline/process_interface/common_process_interface.cpp
@@ -183,19 +183,6 @@ std::vector<FieldPath> CommonProcessInterface::_shardKeyToDocumentKeyFields(
     return result;
 }
 
-std::set<FieldPath> CommonProcessInterface::_convertToFieldPaths(
-    const std::vector<std::string>& fields) const {
-    std::set<FieldPath> fieldPaths;
-
-    for (const auto& field : fields) {
-        const auto res = fieldPaths.insert(FieldPath(field));
-        uassert(ErrorCodes::BadValue,
-                str::stream() << "Found a duplicate field '" << field << "'",
-                res.second);
-    }
-    return fieldPaths;
-}
-
 std::string CommonProcessInterface::getHostAndPort(OperationContext* opCtx) const {
     return getHostNameCachedAndPort();
 }
diff --git a/src/mongo/db/pipeline/process_interface/common_process_interface.h b/src/mongo/db/pipeline/process_interface/common_process_interface.h
index 7f68f8d201f..2184e4b2ee6 100644
--- a/src/mongo/db/pipeline/process_interface/common_process_interface.h
+++ b/src/mongo/db/pipeline/process_interface/common_process_interface.h
@@ -103,12 +103,6 @@ protected:
     virtual void _reportCurrentOpsForTransactionCoordinators(OperationContext* opCtx,
                                                              bool includeIdle,
                                                              std::vector<BSONObj>* ops) const = 0;
-
-    /**
-     * Converts an array of field names into a set of FieldPath. Throws if 'fields' contains
-     * duplicate elements.
-     */
-    std::set<FieldPath> _convertToFieldPaths(const std::vector<std::string>& fields) const;
 };
 
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/process_interface/mongo_process_interface.h b/src/mongo/db/pipeline/process_interface/mongo_process_interface.h
index e697ed0d3bc..3f0cfee2f37 100644
--- a/src/mongo/db/pipeline/process_interface/mongo_process_interface.h
+++ b/src/mongo/db/pipeline/process_interface/mongo_process_interface.h
@@ -408,17 +408,16 @@ public:
     virtual std::unique_ptr<ResourceYielder> getResourceYielder() const = 0;
 
     /**
-     * If the user supplied the 'fields' array, ensures that it can be used to uniquely identify a
-     * document. Otherwise, picks a default unique key, which can be either the "_id" field, or
-     * or a shard key, depending on the 'outputNs' collection type and the server type (mongod or
-     * mongos). Also returns an optional ChunkVersion, populated with the version stored in the
-     * sharding catalog when we asked for the shard key (on mongos only). On mongod, this is the
-     * value of the 'targetCollectionVersion' parameter, which is the target shard version of the
-     * collection, as sent by mongos.
+     * If the user did not provide the 'fieldPaths' set, a default unique key will be picked,
+     * which can be either the "_id" field, or a shard key, depending on the 'outputNs' collection
+     * type and the server type (mongod or mongos). Also returns an optional ChunkVersion,
+     * populated with the version stored in the sharding catalog when we asked for the shard key
+     * (on mongos only). On mongod, this is the value of the 'targetCollectionVersion' parameter,
+     * which is the target shard version of the collection, as sent by mongos.
      */
     virtual std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
     ensureFieldsUniqueOrResolveDocumentKey(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                                           boost::optional<std::vector<std::string>> fields,
+                                           boost::optional<std::set<FieldPath>> fieldPaths,
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const = 0;
 };
diff --git a/src/mongo/db/pipeline/process_interface/mongos_process_interface.cpp b/src/mongo/db/pipeline/process_interface/mongos_process_interface.cpp
index 6c28dfd47f6..0a266c36890 100644
--- a/src/mongo/db/pipeline/process_interface/mongos_process_interface.cpp
+++ b/src/mongo/db/pipeline/process_interface/mongos_process_interface.cpp
@@ -322,23 +322,21 @@ bool MongosProcessInterface::fieldsHaveSupportingUniqueIndex(
 std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
 MongosProcessInterface::ensureFieldsUniqueOrResolveDocumentKey(
     const boost::intrusive_ptr<ExpressionContext>& expCtx,
-    boost::optional<std::vector<std::string>> fields,
+    boost::optional<std::set<FieldPath>> fieldPaths,
     boost::optional<ChunkVersion> targetCollectionVersion,
     const NamespaceString& outputNs) const {
     invariant(expCtx->inMongos);
     uassert(
         51179, "Received unexpected 'targetCollectionVersion' on mongos", !targetCollectionVersion);
 
-    if (fields) {
-        // Convert 'fields' array to a set of FieldPaths.
-        auto fieldPaths = _convertToFieldPaths(*fields);
+    if (fieldPaths) {
         uassert(51190,
                 "Cannot find index to verify that join fields will be unique",
-                fieldsHaveSupportingUniqueIndex(expCtx, outputNs, fieldPaths));
+                fieldsHaveSupportingUniqueIndex(expCtx, outputNs, *fieldPaths));
 
         // If the user supplies the 'fields' array, we don't need to attach a ChunkVersion for the
         // shards since we are not at risk of 'guessing' the wrong shard key.
-        return {fieldPaths, boost::none};
+        return {*fieldPaths, boost::none};
     }
 
     // In case there are multiple shards which will perform this stage in parallel, we need to
diff --git a/src/mongo/db/pipeline/process_interface/mongos_process_interface.h b/src/mongo/db/pipeline/process_interface/mongos_process_interface.h
index 2e505f47ddd..252ee20d0a6 100644
--- a/src/mongo/db/pipeline/process_interface/mongos_process_interface.h
+++ b/src/mongo/db/pipeline/process_interface/mongos_process_interface.h
@@ -219,7 +219,7 @@ public:
 
     std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
     ensureFieldsUniqueOrResolveDocumentKey(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                                           boost::optional<std::vector<std::string>> fields,
+                                           boost::optional<std::set<FieldPath>> fieldPaths,
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const override;
 
diff --git a/src/mongo/db/pipeline/process_interface/standalone_process_interface_test.cpp b/src/mongo/db/pipeline/process_interface/standalone_process_interface_test.cpp
index 5d7cc9d535a..12fe460d674 100644
--- a/src/mongo/db/pipeline/process_interface/standalone_process_interface_test.cpp
+++ b/src/mongo/db/pipeline/process_interface/standalone_process_interface_test.cpp
@@ -63,21 +63,6 @@ public:
     }
 };
 
-TEST_F(ProcessInterfaceStandaloneTest, FailsToEnsureFieldsUniqueIfFieldsHaveDuplicates) {
-    auto expCtx = getExpCtx();
-    auto targetCollectionVersion = boost::none;
-    auto processInterface = makeProcessInterface();
-
-    ASSERT_THROWS_CODE(processInterface->ensureFieldsUniqueOrResolveDocumentKey(
-                           expCtx, {{"_id", "_id"}}, targetCollectionVersion, expCtx->ns),
-                       AssertionException,
-                       ErrorCodes::BadValue);
-    ASSERT_THROWS_CODE(processInterface->ensureFieldsUniqueOrResolveDocumentKey(
-                           expCtx, {{"x", "y", "x"}}, targetCollectionVersion, expCtx->ns),
-                       AssertionException,
-                       ErrorCodes::BadValue);
-}
-
 TEST_F(ProcessInterfaceStandaloneTest,
        FailsToEnsureFieldsUniqueIfTargetCollectionVersionIsSpecifiedOnMongos) {
     auto expCtx = getExpCtx();
diff --git a/src/mongo/db/pipeline/process_interface/stub_mongo_process_interface.h b/src/mongo/db/pipeline/process_interface/stub_mongo_process_interface.h
index d197918719b..2d2d798ab92 100644
--- a/src/mongo/db/pipeline/process_interface/stub_mongo_process_interface.h
+++ b/src/mongo/db/pipeline/process_interface/stub_mongo_process_interface.h
@@ -243,18 +243,14 @@ public:
 
     std::pair<std::set<FieldPath>, boost::optional<ChunkVersion>>
     ensureFieldsUniqueOrResolveDocumentKey(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                                           boost::optional<std::vector<std::string>> fields,
+                                           boost::optional<std::set<FieldPath>> fieldPaths,
                                            boost::optional<ChunkVersion> targetCollectionVersion,
                                            const NamespaceString& outputNs) const override {
-        if (!fields) {
+        if (!fieldPaths) {
             return {std::set<FieldPath>{"_id"}, targetCollectionVersion};
         }
 
-        std::set<FieldPath> fieldPaths;
-        for (const auto& field : *fields) {
-            fieldPaths.insert(FieldPath(field));
-        }
-        return {fieldPaths, targetCollectionVersion};
+        return {*fieldPaths, targetCollectionVersion};
     }
 };
 }  // namespace mongo