SERVER-43806 Update Internal JS Reduce to parse static argument separately

11 files changed, 346 insertions, 157 deletions

diff --git a/src/mongo/db/commands/mr_common.cpp b/src/mongo/db/commands/mr_common.cpp
index 5b9e7e4d172..7ac5056e5ac 100644
--- a/src/mongo/db/commands/mr_common.cpp
+++ b/src/mongo/db/commands/mr_common.cpp
@@ -40,9 +40,9 @@
 #include "mongo/db/catalog/document_validation.h"
 #include "mongo/db/commands.h"
 #include "mongo/db/jsobj.h"
+#include "mongo/db/pipeline/accumulator_js_reduce.h"
 #include "mongo/db/pipeline/document_source.h"
 #include "mongo/db/pipeline/document_source_group.h"
-#include "mongo/db/pipeline/document_source_limit.h"
 #include "mongo/db/pipeline/document_source_match.h"
 #include "mongo/db/pipeline/document_source_merge.h"
 #include "mongo/db/pipeline/document_source_out.h"
@@ -123,15 +123,12 @@ auto translateMap(boost::intrusive_ptr<ExpressionContext> expCtx, std::string co
 }
 
 auto translateReduce(boost::intrusive_ptr<ExpressionContext> expCtx, std::string code) {
-    auto accumulatorArguments = ExpressionObject::create(
-        expCtx,
-        make_vector<std::pair<std::string, boost::intrusive_ptr<Expression>>>(
-            std::pair{"data"s,
-                      ExpressionFieldPath::parse(expCtx, "$emits", expCtx->variablesParseState)},
-            std::pair{"eval"s, ExpressionConstant::create(expCtx, Value{code})}));
-    auto jsReduce = AccumulationStatement{"value", std::move(accumulatorArguments), [expCtx]() {
-                                              return AccumulatorInternalJsReduce::create(expCtx);
-                                          }};
+    auto accumulatorArgument =
+        ExpressionFieldPath::parse(expCtx, "$emits", expCtx->variablesParseState);
+    auto reduceFactory = [expCtx, funcSource = code]() {
+        return AccumulatorInternalJsReduce::create(expCtx, funcSource);
+    };
+    AccumulationStatement jsReduce("value", std::move(accumulatorArgument), reduceFactory);
     auto groupExpr = ExpressionFieldPath::parse(expCtx, "$emits.k", expCtx->variablesParseState);
     return DocumentSourceGroup::create(expCtx,
                                        std::move(groupExpr),
diff --git a/src/mongo/db/pipeline/SConscript b/src/mongo/db/pipeline/SConscript
index 5d3c3e16b4c..29cec941a02 100644
--- a/src/mongo/db/pipeline/SConscript
+++ b/src/mongo/db/pipeline/SConscript
@@ -77,6 +77,7 @@ env.Library(
     source=[
         'expression.cpp',
         'expression_trigonometric.cpp',
+        'make_js_function.cpp'
         ],
     LIBDEPS=[
         '$BUILD_DIR/mongo/db/exec/document_value/document_value',
@@ -124,6 +125,7 @@ env.Library(
         'field_path',
     ]
 )
+
 env.Library(
     target='granularity_rounder',
     source=[
diff --git a/src/mongo/db/pipeline/accumulation_statement.h b/src/mongo/db/pipeline/accumulation_statement.h
index 14b8ea953ab..91ac3a1aff3 100644
--- a/src/mongo/db/pipeline/accumulation_statement.h
+++ b/src/mongo/db/pipeline/accumulation_statement.h
@@ -52,7 +52,7 @@ namespace mongo {
     }
 
 /**
- * A class representing a user-specified accummulation, including the field name to put the
+ * A class representing a user-specified accumulation, including the field name to put the
  * accumulated result in, which accumulator to use, and the expression used to obtain the input to
  * the Accumulator.
  */
@@ -104,7 +104,9 @@ public:
     boost::intrusive_ptr<Accumulator> makeAccumulator() const;
 
 private:
-    Accumulator::Factory _factory;
+    // A no argument function object that can be called to create an Accumulator.
+    const Accumulator::Factory _factory;
 };
 
+
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/accumulator.h b/src/mongo/db/pipeline/accumulator.h
index 4219211a565..2bd9ed9f482 100644
--- a/src/mongo/db/pipeline/accumulator.h
+++ b/src/mongo/db/pipeline/accumulator.h
@@ -92,7 +92,6 @@ public:
     /// Reset this accumulator to a fresh state ready to receive input.
     virtual void reset() = 0;
 
-
     virtual bool isAssociative() const {
         return false;
     }
@@ -101,6 +100,20 @@ public:
         return false;
     }
 
+    /**
+     * Serializes this accumulator to a valid MQL accumulation statement that would be legal
+     * inside a $group.
+     *
+     * The parameter expression represents the input to any accumulator created by the
+     * serialized accumulation statement.
+     *
+     * When executing on a sharded cluster, the result of this function will be sent to each
+     * individual shard.
+     */
+    virtual Document serialize(boost::intrusive_ptr<Expression> expression, bool explain) const {
+        return DOC(getOpName() << expression->serialize(explain));
+    }
+
     virtual AccumulatorDocumentsNeeded documentsNeeded() const {
         return AccumulatorDocumentsNeeded::kAllDocuments;
     }
@@ -133,7 +146,6 @@ genericParseSingleExpressionAccumulator(boost::intrusive_ptr<ExpressionContext>
     return {exprValue, [expCtx]() { return AccName::create(expCtx); }};
 }
 
-
 class AccumulatorAddToSet final : public Accumulator {
 public:
     explicit AccumulatorAddToSet(const boost::intrusive_ptr<ExpressionContext>& expCtx);
@@ -179,7 +191,6 @@ private:
     Value _first;
 };
 
-
 class AccumulatorLast final : public Accumulator {
 public:
     explicit AccumulatorLast(const boost::intrusive_ptr<ExpressionContext>& expCtx);
@@ -200,7 +211,6 @@ private:
     Value _last;
 };
 
-
 class AccumulatorSum final : public Accumulator {
 public:
     explicit AccumulatorSum(const boost::intrusive_ptr<ExpressionContext>& expCtx);
@@ -227,7 +237,6 @@ private:
     Decimal128 decimalTotal;
 };
 
-
 class AccumulatorMinMax : public Accumulator {
 public:
     enum Sense : int {
@@ -271,7 +280,6 @@ public:
         const boost::intrusive_ptr<ExpressionContext>& expCtx);
 };
 
-
 class AccumulatorPush final : public Accumulator {
 public:
     explicit AccumulatorPush(const boost::intrusive_ptr<ExpressionContext>& expCtx);
@@ -288,7 +296,6 @@ private:
     std::vector<Value> vpValue;
 };
 
-
 class AccumulatorAvg final : public Accumulator {
 public:
     explicit AccumulatorAvg(const boost::intrusive_ptr<ExpressionContext>& expCtx);
@@ -362,29 +369,4 @@ private:
     MutableDocument _output;
 };
 
-class AccumulatorInternalJsReduce final : public Accumulator {
-public:
-    static constexpr auto kAccumulatorName = "$_internalJsReduce"_sd;
-
-    static boost::intrusive_ptr<Accumulator> create(
-        const boost::intrusive_ptr<ExpressionContext>& expCtx);
-
-    explicit AccumulatorInternalJsReduce(const boost::intrusive_ptr<ExpressionContext>& expCtx)
-        : Accumulator(expCtx) {
-        _memUsageBytes = sizeof(*this);
-    }
-
-    const char* getOpName() const final {
-        return kAccumulatorName.rawData();
-    }
-    void processInternal(const Value& input, bool merging) final;
-    Value getValue(bool toBeMerged) final;
-    void reset() final;
-
-private:
-    std::vector<Value> _values;
-
-    std::string _funcSource;
-    Value _key;
-};
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/accumulator_js_reduce.cpp b/src/mongo/db/pipeline/accumulator_js_reduce.cpp
index 16bba9c0a90..d6891834c50 100644
--- a/src/mongo/db/pipeline/accumulator_js_reduce.cpp
+++ b/src/mongo/db/pipeline/accumulator_js_reduce.cpp
@@ -31,52 +31,79 @@
 
 #include "mongo/bson/bsonobjbuilder.h"
 #include "mongo/db/commands/test_commands_enabled.h"
-#include "mongo/db/pipeline/accumulation_statement.h"
-#include "mongo/db/pipeline/accumulator.h"
-#include "mongo/db/pipeline/javascript_execution.h"
+#include "mongo/db/pipeline/accumulator_js_reduce.h"
+#include "mongo/db/pipeline/make_js_function.h"
 
 namespace mongo {
 
-REGISTER_ACCUMULATOR(_internalJsReduce,
-                     genericParseSingleExpressionAccumulator<AccumulatorInternalJsReduce>);
+REGISTER_ACCUMULATOR(_internalJsReduce, AccumulatorInternalJsReduce::parseInternalJsReduce);
+
+std::pair<boost::intrusive_ptr<Expression>, Accumulator::Factory>
+AccumulatorInternalJsReduce::parseInternalJsReduce(boost::intrusive_ptr<ExpressionContext> expCtx,
+                                                   BSONElement elem,
+                                                   VariablesParseState vps) {
+    uassert(31326,
+            str::stream() << kAccumulatorName << " requires a document argument, but found "
+                          << elem.type(),
+            elem.type() == BSONType::Object);
+    BSONObj obj = elem.embeddedObject();
+
+    std::string funcSource;
+    boost::intrusive_ptr<Expression> dataExpr;
+
+    for (auto&& element : obj) {
+        if (element.fieldNameStringData() == "eval") {
+            funcSource = parseReduceFunction(element);
+        } else if (element.fieldNameStringData() == "data") {
+            dataExpr = Expression::parseOperand(expCtx, element, vps);
+        } else {
+            uasserted(31243,
+                      str::stream() << "Invalid argument specified to " << kAccumulatorName << ": "
+                                    << element.toString());
+        }
+    }
+    uassert(31245,
+            str::stream() << kAccumulatorName
+                          << " requires 'eval' argument, recieved input: " << obj.toString(false),
+            !funcSource.empty());
+    uassert(31349,
+            str::stream() << kAccumulatorName
+                          << " requires 'data' argument, recieved input: " << obj.toString(false),
+            dataExpr);
+
+    auto factory = [expCtx, funcSource = funcSource]() {
+        return AccumulatorInternalJsReduce::create(expCtx, funcSource);
+    };
+
+    return {std::move(dataExpr), std::move(factory)};
+}
+
+std::string AccumulatorInternalJsReduce::parseReduceFunction(BSONElement func) {
+    uassert(
+        31244,
+        str::stream() << kAccumulatorName
+                      << " requires the 'eval' argument to be of type string, or code but found "
+                      << func.type(),
+        func.type() == BSONType::String || func.type() == BSONType::Code);
+    return func._asCode();
+}
 
 void AccumulatorInternalJsReduce::processInternal(const Value& input, bool merging) {
     if (input.missing()) {
         return;
     }
-
     uassert(31242,
             str::stream() << kAccumulatorName << " requires a document argument, but found "
                           << input.getType(),
             input.getType() == BSONType::Object);
-
-    Document accInput = input.getDocument();
-    uassert(31243,
-            str::stream() << kAccumulatorName << " requires both an 'eval' and 'data' argument",
-            accInput.size() == 2ull && !accInput["eval"].missing() && !accInput["data"].missing());
+    Document data = input.getDocument();
 
     uassert(
-        31244,
+        31251,
         str::stream() << kAccumulatorName
-                      << " requires the 'eval' argument to be of type string, or code but found "
-                      << accInput["eval"].getType(),
-        accInput["eval"].getType() == BSONType::String ||
-            accInput["eval"].getType() == BSONType::Code);
-
-    uassert(31245,
-            str::stream() << kAccumulatorName
-                          << " requires the 'data' argument to be of type Object, but found "
-                          << accInput["data"].getType(),
-            accInput["data"].getType() == BSONType::Object);
-
-    Document data = accInput["data"].getDocument();
-    _funcSource = accInput["eval"].getType() == BSONType::String ? accInput["eval"].getString()
-                                                                 : accInput["eval"].getCode();
-
-    uassert(31251,
-            str::stream() << kAccumulatorName
-                          << " requires the 'data' argument to have a 'k' and 'v' field",
-            data.size() == 2ull && !data["k"].missing() && !data["v"].missing());
+                      << " requires the 'data' argument to have a 'k' and 'v' field. Instead found"
+                      << data.toString(),
+        data.size() == 2ull && !data["k"].missing() && !data["v"].missing());
 
     _key = data["k"];
 
@@ -85,10 +112,6 @@ void AccumulatorInternalJsReduce::processInternal(const Value& input, bool mergi
 }
 
 Value AccumulatorInternalJsReduce::getValue(bool toBeMerged) {
-    uassert(31241,
-            "Cannot run server-side javascript without the javascript engine enabled",
-            getGlobalScriptEngine());
-
     auto val = [&]() {
         if (_values.size() < 1) {
             return Value{};
@@ -100,30 +123,22 @@ Value AccumulatorInternalJsReduce::getValue(bool toBeMerged) {
         }
 
         auto expCtx = getExpressionContext();
-        auto jsExec = expCtx->getJsExecWithScope();
-
-        ScriptingFunction func = jsExec->getScope()->createFunction(_funcSource.c_str());
+        auto reduceFunc = makeJsFunc(expCtx, _funcSource.toString());
 
-        uassert(31247, "The reduce function failed to parse in the javascript engine", func);
 
         // Function signature: reduce(key, values).
         BSONObj params = BSON_ARRAY(_key << bsonValues.arr());
         // For reduce, the key and values are both passed as 'params' so there's no need to set
         // 'this'.
         BSONObj thisObj;
-        return jsExec->callFunction(func, params, thisObj);
+        return expCtx->getJsExecWithScope()->callFunction(reduceFunc, params, thisObj);
     }();
 
     // If we're merging after this, wrap the value in the same format it was inserted in.
     if (toBeMerged) {
-        MutableDocument doc;
         MutableDocument output;
-        doc.addField("k", _key);
-        doc.addField("v", val);
-
-        output.addField("eval", Value(_funcSource));
-        output.addField("data", Value(doc.freeze()));
-
+        output.addField("k", _key);
+        output.addField("v", val);
         return Value(output.freeze());
     } else {
         return val;
@@ -131,13 +146,13 @@ Value AccumulatorInternalJsReduce::getValue(bool toBeMerged) {
 }
 
 boost::intrusive_ptr<Accumulator> AccumulatorInternalJsReduce::create(
-    const boost::intrusive_ptr<ExpressionContext>& expCtx) {
+    const boost::intrusive_ptr<ExpressionContext>& expCtx, StringData funcSource) {
 
     uassert(ErrorCodes::BadValue,
             str::stream() << kAccumulatorName << " not allowed without enabling test commands.",
             getTestCommandsEnabled());
 
-    return new AccumulatorInternalJsReduce(expCtx);
+    return make_intrusive<AccumulatorInternalJsReduce>(expCtx, funcSource);
 }
 
 void AccumulatorInternalJsReduce::reset() {
@@ -145,4 +160,11 @@ void AccumulatorInternalJsReduce::reset() {
     _memUsageBytes = sizeof(*this);
     _key = Value{};
 }
+
+// Returns this accumulator serialized as a Value along with the reduce function.
+Document AccumulatorInternalJsReduce::serialize(boost::intrusive_ptr<Expression> expression,
+                                                bool explain) const {
+    return DOC(
+        getOpName() << DOC("data" << expression->serialize(explain) << "eval" << _funcSource));
+}
 }  // namespace mongo
diff --git a/src/mongo/db/pipeline/accumulator_js_reduce.h b/src/mongo/db/pipeline/accumulator_js_reduce.h
new file mode 100644
index 00000000000..bc2725a5567
--- /dev/null
+++ b/src/mongo/db/pipeline/accumulator_js_reduce.h
@@ -0,0 +1,78 @@
+/**
+ *    Copyright (C) 2019-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#pragma once
+
+#include <boost/intrusive_ptr.hpp>
+#include <vector>
+
+#include "mongo/db/pipeline/accumulation_statement.h"
+#include "mongo/db/pipeline/accumulator.h"
+#include "mongo/db/pipeline/expression_context.h"
+
+namespace mongo {
+
+class AccumulatorInternalJsReduce final : public Accumulator {
+public:
+    static constexpr auto kAccumulatorName = "$_internalJsReduce"_sd;
+
+    static boost::intrusive_ptr<Accumulator> create(
+        const boost::intrusive_ptr<ExpressionContext>& expCtx, StringData funcSource);
+
+    static std::pair<boost::intrusive_ptr<Expression>, Accumulator::Factory> parseInternalJsReduce(
+        boost::intrusive_ptr<ExpressionContext> expCtx, BSONElement elem, VariablesParseState vps);
+
+    AccumulatorInternalJsReduce(const boost::intrusive_ptr<ExpressionContext>& expCtx,
+                                StringData funcSource)
+        : Accumulator(expCtx), _funcSource(funcSource) {
+        _memUsageBytes = sizeof(*this);
+    }
+
+    const char* getOpName() const final {
+        return kAccumulatorName.rawData();
+    }
+
+    void processInternal(const Value& input, bool merging) final;
+
+    Value getValue(bool toBeMerged) final;
+
+    void reset() final;
+
+    virtual Document serialize(boost::intrusive_ptr<Expression> expression,
+                               bool explain) const override;
+
+private:
+    static std::string parseReduceFunction(BSONElement func);
+
+    StringData _funcSource;
+    std::vector<Value> _values;
+    Value _key;
+};
+
+}  // namespace mongo
diff --git a/src/mongo/db/pipeline/accumulator_js_test.cpp b/src/mongo/db/pipeline/accumulator_js_test.cpp
index ff6d24c49ec..41f12ac8898 100644
--- a/src/mongo/db/pipeline/accumulator_js_test.cpp
+++ b/src/mongo/db/pipeline/accumulator_js_test.cpp
@@ -33,8 +33,7 @@
 
 #include "mongo/db/exec/document_value/document.h"
 #include "mongo/db/exec/document_value/document_value_test_util.h"
-#include "mongo/db/pipeline/accumulation_statement.h"
-#include "mongo/db/pipeline/accumulator.h"
+#include "mongo/db/pipeline/accumulator_js_reduce.h"
 #include "mongo/db/pipeline/expression_context_for_test.h"
 #include "mongo/db/pipeline/process_interface_standalone.h"
 #include "mongo/db/service_context_d_test_fixture.h"
@@ -77,23 +76,32 @@ namespace InternalJsReduce {
 
 template <typename AccName>
 static void assertProcessFailsWithCode(const boost::intrusive_ptr<ExpressionContext>& expCtx,
+                                       const std::string& eval,
                                        Value processArgument,
                                        int code) {
-    auto accum = AccName::create(expCtx);
+    auto accum = AccName::create(expCtx, eval);
     ASSERT_THROWS_CODE(accum->process(processArgument, false), AssertionException, code);
 }
 
+static void assertParsingFailsWithCode(const boost::intrusive_ptr<ExpressionContext>& expCtx,
+                                       BSONElement elem,
+                                       int code) {
+    ASSERT_THROWS_CODE(AccumulatorInternalJsReduce::parseInternalJsReduce(
+                           expCtx, elem, expCtx->variablesParseState),
+                       AssertionException,
+                       code);
+}
+
 template <typename AccName>
 static void assertExpectedResults(const boost::intrusive_ptr<ExpressionContext>& expCtx,
-                                  std::string eval,
+                                  const std::string& eval,
                                   std::vector<Value> data,
                                   Value expectedResult) {
     // Asserts that result equals expected result when not sharded.
     {
-        auto accum = AccName::create(expCtx);
+        auto accum = AccName::create(expCtx, eval);
         for (auto&& val : data) {
-            auto input = Value(DOC("eval" << eval << "data" << val));
-            accum->process(input, false);
+            accum->process(val, false);
         }
         Value result = accum->getValue(false);
         ASSERT_VALUE_EQ(expectedResult, result);
@@ -102,11 +110,10 @@ static void assertExpectedResults(const boost::intrusive_ptr<ExpressionContext>&
 
     // Asserts that result equals expected result when all input is on one shard.
     {
-        auto accum = AccName::create(expCtx);
-        auto shard = AccName::create(expCtx);
+        auto accum = AccName::create(expCtx, eval);
+        auto shard = AccName::create(expCtx, eval);
         for (auto&& val : data) {
-            auto input = Value(DOC("eval" << eval << "data" << val));
-            shard->process(input, false);
+            shard->process(val, false);
         }
         accum->process(shard->getValue(true), true);
         Value result = accum->getValue(false);
@@ -116,11 +123,10 @@ static void assertExpectedResults(const boost::intrusive_ptr<ExpressionContext>&
 
     // Asserts that result equals expected result when each input is on a separate shard.
     {
-        auto accum = AccName::create(expCtx);
+        auto accum = AccName::create(expCtx, eval);
         for (auto&& val : data) {
-            auto input = Value(DOC("eval" << eval << "data" << val));
-            auto shard = AccName::create(expCtx);
-            shard->process(input, false);
+            auto shard = AccName::create(expCtx, eval);
+            shard->process(val, false);
             accum->process(shard->getValue(true), true);
         }
         Value result = accum->getValue(false);
@@ -154,13 +160,12 @@ TEST_F(MapReduceFixture, InternalJsReduceProducesExpectedResults) {
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceIdempotentOnlyWhenJSFunctionIsIdempotent) {
-    auto accum = AccumulatorInternalJsReduce::create(getExpCtx());
+    std::string eval("function(key, values) { return Array.sum(values) + 1; };");
+    auto accum = AccumulatorInternalJsReduce::create(getExpCtx(), eval);
 
     // A non-idempotent Javascript function will produce non-idempotent results. In this case a
     // single document reduce causes a change in value.
-    auto input =
-        Value(DOC("eval" << std::string("function(key, values) { return Array.sum(values) + 1; };")
-                         << "data" << Value(DOC("k" << std::string("foo") << "v" << Value(5)))));
+    auto input = Value(DOC("k" << std::string("foo") << "v" << Value(5)));
     auto expectedResult = Value(6.0);
 
     accum->process(input, false);
@@ -171,12 +176,11 @@ TEST_F(MapReduceFixture, InternalJsReduceIdempotentOnlyWhenJSFunctionIsIdempoten
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsWhenEvalContainsInvalidJavascript) {
+    std::string eval("INVALID_JAVASCRIPT");
     // Multiple source documents.
     {
-        auto accum = AccumulatorInternalJsReduce::create(getExpCtx());
-
-        auto input = Value(DOC("eval" << std::string("INVALID_JAVASCRIPT") << "data"
-                                      << Value(DOC("k" << Value(1) << "v" << Value(2)))));
+        auto accum = AccumulatorInternalJsReduce::create(getExpCtx(), "INVALID_JAVASCRIPT");
+        auto input = Value(DOC("k" << Value(1) << "v" << Value(2)));
         accum->process(input, false);
         accum->process(input, false);
 
@@ -185,10 +189,9 @@ TEST_F(MapReduceFixture, InternalJsReduceFailsWhenEvalContainsInvalidJavascript)
 
     // Single source document.
     {
-        auto accum = AccumulatorInternalJsReduce::create(getExpCtx());
+        auto accum = AccumulatorInternalJsReduce::create(getExpCtx(), "INVALID_JAVASCRIPT");
 
-        auto input = Value(DOC("eval" << std::string("INVALID_JAVASCRIPT") << "data"
-                                      << Value(DOC("k" << Value(1) << "v" << Value(2)))));
+        auto input = Value(DOC("k" << Value(1) << "v" << Value(2)));
         accum->process(input, false);
 
         ASSERT_THROWS_CODE(accum->getValue(false), DBException, ErrorCodes::JSInterpreterFailure);
@@ -196,62 +199,79 @@ TEST_F(MapReduceFixture, InternalJsReduceFailsWhenEvalContainsInvalidJavascript)
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsIfArgumentNotDocument) {
-
     auto argument = Value(2);
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31242);
+    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(
+        getExpCtx(), "function(key, values) { return Array.sum(values); };", argument, 31242);
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsIfEvalAndDataArgumentsNotProvided) {
     // Data argument missing.
-    auto argument =
-        Value(DOC("eval" << std::string("function(key, values) { return Array.sum(values); };")));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31243);
+    BSONObjBuilder noData;
+    noData.append("$_internalJsReduce",
+                  BSON("eval"
+                       << "function(key, values) { return Array.sum(values); };"));
+    assertParsingFailsWithCode(getExpCtx(), noData.obj().getField("$_internalJsReduce"), 31349);
 
     // Eval argument missing.
-    argument = Value(DOC("data" << Value(DOC("k" << Value(1) << "v" << Value(2)))));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31243);
+    BSONObjBuilder noEval;
+    noEval.append("$_internalJsReduce", BSON("data" << BSON("k" << Value(1) << "v" << Value(2))));
+    assertParsingFailsWithCode(getExpCtx(), noEval.obj().getField("$_internalJsReduce"), 31245);
+}
+
+TEST_F(MapReduceFixture, InternalJsReduceFailsIfExtraArgumentsAreSpecified) {
+    // Data argument missing.
+    BSONObjBuilder obj;
+    obj.append("eval", "function(key, values) { return Array.sum(values); };");
+    obj.append("data", BSON("k" << std::string("foo") << "v" << Value(2)));
+    obj.append("extraField", 1);
+    BSONObjBuilder wrap;
+    wrap.append("$_internalJsReduce", obj.obj());
+    assertParsingFailsWithCode(getExpCtx(), wrap.obj().getField("$_internalJsReduce"), 31243);
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsIfEvalArgumentNotOfTypeStringOrCode) {
-    auto argument = Value(
-        DOC("eval" << 1 << "data" << Value(DOC("k" << std::string("foo") << "v" << Value(2)))));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31244);
+    BSONObjBuilder codeTypeInt;
+    codeTypeInt.appendIntOrLL("eval", 1);
+    codeTypeInt.append("data", BSON("k" << std::string("foo") << "v" << Value(2)));
+    BSONObjBuilder wrapInt;
+    wrapInt.append("$_internalJsReduce", codeTypeInt.obj());
+    assertParsingFailsWithCode(getExpCtx(), wrapInt.obj().getField("$_internalJsReduce"), 31244);
 
     // MapReduce does not accept JavaScript function of BSON type CodeWScope.
     BSONObjBuilder objBuilder;
     objBuilder.appendCodeWScope(
         "eval", "function(key, values) { return Array.sum(values); };", BSONObj());
     objBuilder.append("data", BSON("k" << std::string("foo") << "v" << Value(2)));
-
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(
-        getExpCtx(), Value(objBuilder.obj()), 31244);
+    BSONObjBuilder wrapCode;
+    wrapCode.append("$_internalJsReduce", objBuilder.obj());
+    assertParsingFailsWithCode(getExpCtx(), wrapCode.obj().getField("$_internalJsReduce"), 31244);
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsIfDataArgumentNotDocument) {
-    auto argument =
-        Value(DOC("eval" << std::string("function(key, values) { return Array.sum(values); };")
-                         << "data" << Value(2)));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31245);
+    std::string eval("function(key, values) { return Array.sum(values); };");
+    auto argument = Value(Value(2));
+    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(
+        getExpCtx(), std::move(eval), argument, 31242);
 }
 
 TEST_F(MapReduceFixture, InternalJsReduceFailsIfDataArgumentDoesNotContainExpectedFields) {
+    std::string eval("function(key, values) { return Array.sum(values); };");
     // No "k" field.
-    auto argument = Value(
-        DOC("eval" << std::string("function(key, values) { return Array.sum(values); };") << "data"
-                   << Value(DOC("foo" << std::string("keyVal") << "v" << Value(2)))));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31251);
+    auto argument = Value(DOC(
+        "eval" << eval << "data" << Value(DOC("foo" << std::string("keyVal") << "v" << Value(2)))));
+    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), eval, argument, 31251);
 
     // No "v" field.
     argument = Value(
         DOC("eval" << std::string("function(key, values) { return Array.sum(values); };") << "data"
                    << Value(DOC("k" << std::string("keyVal") << "bar" << Value(2)))));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31251);
+    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), eval, argument, 31251);
 
     // Both "k" and "v" fields missing.
     argument =
         Value(DOC("eval" << std::string("function(key, values) { return Array.sum(values); };")
                          << "data" << Value(Document())));
-    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), argument, 31251);
+    assertProcessFailsWithCode<AccumulatorInternalJsReduce>(getExpCtx(), eval, argument, 31251);
 }
 
 }  // namespace InternalJsReduce
diff --git a/src/mongo/db/pipeline/document_source_group.cpp b/src/mongo/db/pipeline/document_source_group.cpp
index 4f04626c341..0f961f8c363 100644
--- a/src/mongo/db/pipeline/document_source_group.cpp
+++ b/src/mongo/db/pipeline/document_source_group.cpp
@@ -35,7 +35,6 @@
 #include "mongo/db/exec/document_value/document.h"
 #include "mongo/db/exec/document_value/value.h"
 #include "mongo/db/exec/document_value/value_comparator.h"
-#include "mongo/db/jsobj.h"
 #include "mongo/db/pipeline/accumulation_statement.h"
 #include "mongo/db/pipeline/accumulator.h"
 #include "mongo/db/pipeline/document_source_group.h"
@@ -244,8 +243,7 @@ Value DocumentSourceGroup::serialize(boost::optional<ExplainOptions::Verbosity>
     for (auto&& accumulatedField : _accumulatedFields) {
         intrusive_ptr<Accumulator> accum = accumulatedField.makeAccumulator();
         insides[accumulatedField.fieldName] =
-            Value(DOC(accum->getOpName()
-                      << accumulatedField.expression->serialize(static_cast<bool>(explain))));
+            Value(accum->serialize(accumulatedField.expression, static_cast<bool>(explain)));
     }
 
     if (_doingMerge) {
@@ -412,7 +410,6 @@ intrusive_ptr<DocumentSource> DocumentSourceGroup::createFromBson(
     while (groupIterator.more()) {
         BSONElement groupField(groupIterator.next());
         StringData pFieldName = groupField.fieldNameStringData();
-
         if (pFieldName == "_id") {
             uassert(
                 15948, "a group's _id may only be specified once", pGroup->_idExpressions.empty());
@@ -695,10 +692,10 @@ boost::optional<DocumentSource::DistributedPlanLogic> DocumentSourceGroup::distr
         // However, for some accumulators, the expression to be accumulated will be different. The
         // original accumulator may be collecting an expression based on a field expression or
         // constant.  Here, we accumulate the output of the same name from the prior group.
-        auto copiedAccumuledField = accumulatedField;
-        copiedAccumuledField.expression =
-            ExpressionFieldPath::parse(pExpCtx, "$$ROOT." + accumulatedField.fieldName, vps);
-        mergingGroup->addAccumulator(copiedAccumuledField);
+        auto copiedAccumulatedField = accumulatedField;
+        copiedAccumulatedField.expression =
+            ExpressionFieldPath::parse(pExpCtx, "$$ROOT." + copiedAccumulatedField.fieldName, vps);
+        mergingGroup->addAccumulator(copiedAccumulatedField);
     }
 
     // {shardsStage, mergingStage, sortPattern}
diff --git a/src/mongo/db/pipeline/expression_javascript.cpp b/src/mongo/db/pipeline/expression_javascript.cpp
index b27ff8652a3..a80604731b4 100644
--- a/src/mongo/db/pipeline/expression_javascript.cpp
+++ b/src/mongo/db/pipeline/expression_javascript.cpp
@@ -32,6 +32,7 @@
 #include "mongo/db/auth/authorization_session.h"
 #include "mongo/db/commands/test_commands_enabled.h"
 #include "mongo/db/pipeline/expression_javascript.h"
+#include "mongo/db/pipeline/make_js_function.h"
 #include "mongo/db/query/query_knobs_gen.h"
 
 namespace mongo {
@@ -107,18 +108,14 @@ Value ExpressionInternalJsEmit::serialize(bool explain) const {
 }
 
 Value ExpressionInternalJsEmit::evaluate(const Document& root, Variables* variables) const {
-    uassert(31246,
-            "Cannot run server-side javascript without the javascript engine enabled",
-            getGlobalScriptEngine());
-
     Value thisVal = _thisRef->evaluate(root, variables);
     uassert(31225, "'this' must be an object.", thisVal.getType() == BSONType::Object);
 
     // If the scope does not exist and is created by the following call, then make sure to
     // re-bind emit() and the given function to the new scope.
     auto expCtx = getExpressionContext();
-    auto jsExec = expCtx->getJsExecWithScope();
 
+    auto jsExec = expCtx->getJsExecWithScope();
     // Inject the native "emit" function to be called from the user-defined map function. This
     // particular Expression/ExpressionContext may be reattached to a new OperationContext (and thus
     // a new JS Scope) when used across getMore operations, so this method will handle that case for
@@ -128,8 +125,7 @@ Value ExpressionInternalJsEmit::evaluate(const Document& root, Variables* variab
     // Although inefficient to "create" a new function every time we evaluate, this will usually end
     // up being a simple cache lookup. This is needed because the JS Scope may have been recreated
     // on a new thread if the expression is evaluated across getMores.
-    auto func = jsExec->getScope()->createFunction(_funcSource.c_str());
-    uassert(31226, "The map function failed to parse in the javascript engine", func);
+    auto func = makeJsFunc(expCtx, _funcSource.c_str());
 
     BSONObj thisBSON = thisVal.getDocument().toBson();
     BSONObj params;
diff --git a/src/mongo/db/pipeline/make_js_function.cpp b/src/mongo/db/pipeline/make_js_function.cpp
new file mode 100644
index 00000000000..d23e4628694
--- /dev/null
+++ b/src/mongo/db/pipeline/make_js_function.cpp
@@ -0,0 +1,50 @@
+/**
+ *    Copyright (C) 2019-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#include "mongo/platform/basic.h"
+
+#include "mongo/db/pipeline/make_js_function.h"
+
+namespace mongo {
+
+// Given a function represented as a string, constructs a JS execution context and attempts to parse
+// it as a JS function.
+ScriptingFunction makeJsFunc(boost::intrusive_ptr<ExpressionContext> expCtx,
+                             const std::string& func) {
+    uassert(31241,
+            "Cannot run server-side javascript without the javascript engine enabled",
+            getGlobalScriptEngine());
+    auto jsExec = expCtx->getJsExecWithScope();
+    ScriptingFunction parsedFunc = jsExec->getScope()->createFunction(func.c_str());
+    uassert(
+        31247, "The user-defined function failed to parse in the javascript engine", parsedFunc);
+    return parsedFunc;
+}
+
+}  // namespace mongo
diff --git a/src/mongo/db/pipeline/make_js_function.h b/src/mongo/db/pipeline/make_js_function.h
new file mode 100644
index 00000000000..7ae894256dc
--- /dev/null
+++ b/src/mongo/db/pipeline/make_js_function.h
@@ -0,0 +1,43 @@
+/**
+ *    Copyright (C) 2019-present MongoDB, Inc.
+ *
+ *    This program is free software: you can redistribute it and/or modify
+ *    it under the terms of the Server Side Public License, version 1,
+ *    as published by MongoDB, Inc.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    Server Side Public License for more details.
+ *
+ *    You should have received a copy of the Server Side Public License
+ *    along with this program. If not, see
+ *    <http://www.mongodb.com/licensing/server-side-public-license>.
+ *
+ *    As a special exception, the copyright holders give permission to link the
+ *    code of portions of this program with the OpenSSL library under certain
+ *    conditions as described in each individual source file and distribute
+ *    linked combinations including the program with the OpenSSL library. You
+ *    must comply with the Server Side Public License in all respects for
+ *    all of the code used other than as permitted herein. If you modify file(s)
+ *    with this exception, you may extend this exception to your version of the
+ *    file(s), but you are not obligated to do so. If you do not wish to do so,
+ *    delete this exception statement from your version. If you delete this
+ *    exception statement from all source files in the program, then also delete
+ *    it in the license file.
+ */
+
+#pragma once
+
+#include "mongo/db/pipeline/expression_context.h"
+#include "mongo/db/pipeline/javascript_execution.h"
+
+namespace mongo {
+
+/**
+ * Parses and returns an executable Javascript function.
+ */
+ScriptingFunction makeJsFunc(boost::intrusive_ptr<ExpressionContext> expCtx,
+                             const std::string& func);
+
+}  // namespace mongo