EAGLESIX-2651: Map: better sync w/ 2.6.5 code, test more thoroughly, fix more variables related bugs

lib/pipeline/expressions/Expression.js

@@ -178,9 +178,8 @@ klass.expressionParserMap = {};
  * REGISTER_EXPRESSION("$foo", ExpressionFoo::parse);
  */
 klass.registerExpression = function registerExpression(key, parserFunc) {
-	if (key in klass.expressionParserMap) {
+	if (key in klass.expressionParserMap)
 		throw new Error("Duplicate expression (" + key + ") detected; massert code 17064");
-	}
 	klass.expressionParserMap[key] = parserFunc;
 	return 1;
 };
@@ -267,7 +266,7 @@ proto.optimize = function optimize() {
  *             where {a:1} inclusion objects aren't allowed, they get
  *             NULL.
  */
-proto.addDependencies = function addDependencies(deps, path) {
+proto.addDependencies = function addDependencies(deps, path) { //jshint ignore:line
 	throw new Error("WAS NOT IMPLEMENTED BY INHERITOR!");
 };
 
@@ -285,7 +284,7 @@ proto.isSimple = function isSimple() {
  * If explain is false, returns a Value parsable by parseOperand().
  * @method serialize
  */
-proto.serialize = function serialize(explain) {
+proto.serialize = function serialize(explain) { //jshint ignore:line
 	throw new Error("WAS NOT IMPLEMENTED BY INHERITOR!");
 };
 
@@ -299,7 +298,7 @@ proto.serialize = function serialize(explain) {
  * @param vars
  */
 proto.evaluate = function evaluate(vars) {
-	if (!(vars instanceof Variables)) vars = new Variables(0, vars); /// Evaluate expression with specified inputs and return result. (only used by tests)
+	if (vars instanceof Object && vars.constructor === Object) vars = new Variables(0, vars); /// Evaluate expression with specified inputs and return result. (only used by tests)
 	return this.evaluateInternal(vars);
 };
 
@@ -322,7 +321,7 @@ klass.removeFieldPrefix = function removeFieldPrefix(prefixedField) {
  * @method evaluate
  * @returns the computed value
  */
-proto.evaluateInternal = function evaluateInternal(vars) {
+proto.evaluateInternal = function evaluateInternal(vars) { //jshint ignore:line
 	throw new Error("WAS NOT IMPLEMENTED BY INHERITOR!");
 };
 

lib/pipeline/expressions/FieldPathExpression.js

@@ -2,7 +2,6 @@
 
 var Expression = require("./Expression"),
     Variables = require("./Variables"),
-    Value = require("../Value"),
     FieldPath = require("../FieldPath");
 
 /**

lib/pipeline/expressions/MapExpression.js

@@ -1,111 +1,110 @@
 "use strict";
 
 var MapExpression = module.exports = function MapExpression(varName, varId, input, each){
-	if (arguments.length !== 4) throw new Error("Four args expected");
+	if (arguments.length !== 4) throw new Error(klass.name + ": args expected: varName, varId, input, each");
 	this._varName = varName;
 	this._varId = varId;
 	this._input = input;
 	this._each = each;
 }, klass = MapExpression, Expression = require("./Expression"), base = Expression, proto = klass.prototype = Object.create(base.prototype, {constructor:{value:klass}});
 
-// DEPENDENCIES
-var Variables = require("./Variables"),
-	VariablesParseState = require("./VariablesParseState");
+var Value = require("../Value"),
+	Variables = require("./Variables");
 
-// PROTOTYPE MEMBERS
+klass.parse = function parse(expr, vpsIn) {
 
-klass.parse = function parse(expr, vpsIn){
-	if(!("$map" in expr)) {
-		throw new Error("Tried to create a $let with something other than let. Looks like your parse map went all funny.");
-	}
+	// if (!(exprFieldName)) throw new Error("Assertion failure"); //NOTE: DEVIATION FROM MONGO: we do not have exprFieldName here
 
-	if(typeof(expr.$map) !== 'object' || (expr.$map instanceof Array)) {
-		throw new Error("$map only supports an object as its argument:16878");
+	if (Value.getType(expr) !== "Object") {
+		throw new Error("$map only supports an object as it's argument; uassert code 16878");
 	}
 
-	var args = expr.$map,
-		inputElem = args.input,
-		inElem = args['in'],
-		asElem = args.as;
-
-	if(!inputElem) {
-		throw new Error("Missing 'input' parameter to $map: 16880");
-	}
-	if(!asElem) {
-		throw new Error("Missing 'as' parameter to $map: 16881");
-	}
-	if(!inElem) {
-		throw new Error("Missing 'in' parameter to $map: 16882");
+	// "in" must be parsed after "as" regardless of BSON order
+	var inputElem,
+		asElem,
+		inElem,
+		args = expr;
+	for (var argFieldName in args) {
+		var arg = args[argFieldName];
+		if (argFieldName === "input") {
+			inputElem = arg;
+		} else if (argFieldName === "as") {
+			asElem = arg;
+		} else if (argFieldName === "in") {
+			inElem = arg;
+		} else {
+			throw new Error("Unrecognized parameter to $map: " + argFieldName + "; uassert code 16879");
+		}
 	}
 
-
-	if(Object.keys(args).length > 3) {
-		var bogus = Object.keys(args).filter(function(x) {return !(x === 'in' || x === 'as' || x === 'input');});
-		throw new Error("Unrecognized parameter to $map: " + bogus.join(",") + "- 16879");
-	}
+	if (!inputElem) throw new Error("Missing 'input' parameter to $map; uassert code 16880");
+	if (!asElem) throw new Error("Missing 'as' parameter to $map; uassert code 16881");
+	if (!inElem) throw new Error("Missing 'in' parameter to $map; uassert code 16882");
 
 	// parse "input"
-	var input = Expression.parseOperand(inputElem, vpsIn);
+	var input = Expression.parseOperand(inputElem, vpsIn); // only has outer vars
 
 	// parse "as"
-	var vpsSub = new VariablesParseState(vpsIn),
+	var vpsSub = vpsIn, // vpsSub gets our vars, vpsIn doesn't.
 		varName = asElem;
-
 	Variables.uassertValidNameForUserWrite(varName);
 	var varId = vpsSub.defineVariable(varName);
 
-	// parse ""in
-	var invert = Expression.parseOperand(inElem, vpsSub);
+	// parse "in"
+	var inExpr = Expression.parseOperand(inElem, vpsSub); // has access to map variable
 
-	return new MapExpression(varName, varId, input, invert);
+	return new MapExpression(varName, varId, input, inExpr);
 };
 
-
 proto.optimize = function optimize() {
+	// TODO handle when _input is constant
 	this._input = this._input.optimize();
 	this._each = this._each.optimize();
 	return this;
 };
 
 proto.serialize = function serialize(explain) {
-	return {$map: {input:this._input.serialize(explain),
-				   as: this._varName,
-				   'in': this._each.serialize(explain)}};
+	return {
+		$map: {
+			input: this._input.serialize(explain),
+			as: this._varName,
+			in : this._each.serialize(explain)
+		}
+	};
 };
 
 proto.evaluateInternal = function evaluateInternal(vars) {
-
 	// guaranteed at parse time that this isn't using our _varId
 	var inputVal = this._input.evaluateInternal(vars);
-	if( inputVal === null || inputVal === undefined) {
+	if (inputVal === null || inputVal === undefined)
 		return null;
-	}
 
-	if(!(inputVal instanceof Array)) {
-		throw new Error("Uassert 16883: Input to $map must be an Array, not a " + typeof inputVal);
+	if (!(inputVal instanceof Array)){
+		throw new Error("input to $map must be an Array not " +
+			Value.getType(inputVal) + "; uassert code 16883");
 	}
 
-	if(inputVal.length === 0) {
+	if (inputVal.length === 0)
 		return inputVal;
-	}
 
-	// Diverge from Mongo source here, as Javascript has a builtin map operator.
-	return inputVal.map(function(x) {
-	   vars.setValue(this._varId, x);
-	   var toInsert = this._each.evaluateInternal(vars);
-	   if(toInsert === undefined) {
-		   toInsert = null;
-	   }
+	var output = new Array(inputVal.length);
+	for (var i = 0, l = inputVal.length; i < l; i++) {
+		vars.setValue(this._varId, inputVal[i]);
+
+		var toInsert = this._each.evaluateInternal(vars);
+		if (toInsert === undefined)
+			toInsert = null; // can't insert missing values into array
 
-	   return toInsert;
-   });
+		output[i] = toInsert;
+	}
+
+	return output;
 };
 
-proto.addDependencies = function addDependencies(deps, path){
-	this._input.addDependencies(deps, path);
-	this._each.addDependencies(deps, path);
+proto.addDependencies = function addDependencies(deps, path) { //jshint ignore:line
+	this._input.addDependencies(deps);
+	this._each.addDependencies(deps);
 	return deps;
 };
 
-
 Expression.registerExpression("$map", klass.parse);

lib/pipeline/expressions/Variables.js

@@ -1,5 +1,6 @@
 "use strict";
 
+// TODO: Look into merging with ExpressionContext and possibly ObjectCtx.
 /**
  * The state used as input and working space for Expressions.
  * @class Variables

lib/pipeline/expressions/VariablesParseState.js

@@ -31,7 +31,7 @@ var VariablesParseState = module.exports = function VariablesParseState(idGenera
  *
  * NOTE: Name validation is responsibility of caller.
  */
-proto.defineVariable = function generateId(name) {
+proto.defineVariable = function defineVariable(name) {
 	// caller should have validated before hand by using Variables::uassertValidNameForUserWrite
 	if (name === "ROOT")
 		throw new Error("Can't redefine ROOT; massert code 17275");
@@ -46,7 +46,7 @@ proto.defineVariable = function generateId(name) {
  * @method getVariable
  * @param name {String} The name of the variable
  */
-proto.getVariable = function getIdCount(name) {
+proto.getVariable = function getVariable(name) {
 	var it = this._variables[name];
 	if (typeof it === "number")
 		return it;

test/lib/pipeline/expressions/MapExpression_test.js

@@ -1,39 +1,115 @@
 "use strict";
 var assert = require("assert"),
 	MapExpression = require("../../../../lib/pipeline/expressions/MapExpression"),
+	Expression = require("../../../../lib/pipeline/expressions/Expression"),
+	AddExpression = require("../../../../lib/pipeline/expressions/AddExpression"), // jshint ignore:line
+	IfNullExpression = require("../../../../lib/pipeline/expressions/IfNullExpression"), // jshint ignore:line
+	Variables = require("../../../../lib/pipeline/expressions/Variables"),
+	VariablesIdGenerator = require("../../../../lib/pipeline/expressions/VariablesIdGenerator"),
+	VariablesParseState = require("../../../../lib/pipeline/expressions/VariablesParseState"),
 	DepsTracker = require("../../../../lib/pipeline/DepsTracker"),
-	Expression = require("../../../../lib/pipeline/expressions/Expression");
+	utils = require("./utils"),
+	constify = utils.constify,
+	expressionToJson = utils.expressionToJson;
 
 // Mocha one-liner to make these tests self-hosted
 if (!module.parent)return(require.cache[__filename] = null, (new (require("mocha"))({ui: "exports", reporter: "spec", grep: process.env.TEST_GREP})).addFile(__filename).run(process.exit));
 
-module.exports = {
+exports.MapExpression = {
 
-	"MapExpression": {
+	"constructor()": {
 
-		"constructor()": {
+		"should accept 4 arguments": function () {
+			new MapExpression(1, 2, 3, 4);
+		},
+
+		"should accept only 4 arguments": function () {
+			assert.throws(function () { new MapExpression(); });
+			assert.throws(function () { new MapExpression(1); });
+			assert.throws(function () { new MapExpression(1, 2); });
+			assert.throws(function () { new MapExpression(1, 2, 3); });
+			assert.throws(function () { new MapExpression(1, 2, 3, 4, 5); });
+		},
+
+	},
 
-			"should accept 4 arguments": function () {
-				new MapExpression(1, 2, 3, 4);
-			},
+	"#optimize()": {
 
-			"should not accept less than 4 arguments": function () {
-				assert.throws(function () { new MapExpression(); });
-				assert.throws(function () { new MapExpression(1); });
-				assert.throws(function () { new MapExpression(1, 2); });
-				assert.throws(function () { new MapExpression(1, 2, 3); });
-			},
+		"should optimize both $map.input and $map.in": function() {
+			var spec = {$map:{
+					input: {$ifNull:[null, {$const:[1,2,3]}]},
+					as: "i",
+					in: {$add:["$$i","$$i",1,2]},
+				}},
+				idGenerator = new VariablesIdGenerator(),
+				vps = new VariablesParseState(idGenerator),
+				expr = Expression.parseOperand(spec, vps),
+				optimized = expr.optimize();
+			assert.strictEqual(optimized, expr, "should be same reference");
+			assert.deepEqual(expressionToJson(optimized._input), {$const:[1,2,3]});
+			assert.deepEqual(expressionToJson(optimized._each), constify({$add:["$$i","$$i",1,2]}));
 		},
 
-		"optimize": {
-			"trivial case": function() {
-				var m = new MapExpression("test", "varname", new Expression(), new Expression());
-				assert.ok(m.optimize());
-			},
+	},
+
+	"#serialize()": {
+
+		"should serialize to consistent order": function() {
+			var spec = {$map:{
+					as: "i",
+					in: {$add:["$$i","$$i"]},
+					input: {$const:[1,2,3]},
+				}},
+				expected = {$map:{
+					input: {$const:[1,2,3]},
+					as: "i",
+					in: {$add:["$$i","$$i"]},
+				}},
+				idGenerator = new VariablesIdGenerator(),
+				vps = new VariablesParseState(idGenerator),
+				expr = Expression.parseOperand(spec, vps);
+			assert.deepEqual(expressionToJson(expr), expected);
 		},
 
-		"addDependencies": {
-			"trivial case - calls addDependencies on _input and _each": function () {}
-		}
-	}
+	},
+
+	"#evaluate()": {
+
+		"should be able to map over a simple array": function() {
+			var spec = {$map:{
+					input: {$const:[1,2,3]},
+					as: "i",
+					in: {$add:["$$i","$$i"]},
+				}},
+				idGenerator = new VariablesIdGenerator(),
+				vps = new VariablesParseState(idGenerator),
+				expr = Expression.parseOperand(spec, vps),
+				vars = new Variables(1, {}); // must set numVars (usually handled by doc src)
+			assert.deepEqual(expr.evaluate(vars), [2, 4, 6]);
+		},
+
+	},
+
+	"#addDependencies()": {
+
+		"should add dependencies to both $map.input and $map.in": function () {
+			var spec = {$map:{
+					input: "$inputArray",
+					as: "i",
+					in: {$add:["$$i","$someConst"]},
+				}},
+				idGenerator = new VariablesIdGenerator(),
+				vps = new VariablesParseState(idGenerator),
+				expr = Expression.parseOperand(spec, vps),
+				deps = new DepsTracker();
+			expr.addDependencies(deps);
+			assert.strictEqual(Object.keys(deps.fields).length, 2);
+			assert.strictEqual("inputArray" in deps.fields, true);
+			assert.strictEqual("someConst" in deps.fields, true);
+			assert.strictEqual(deps.needWholeDocument, false);
+			assert.strictEqual(deps.needTextScore, false);
+		},
+
+	},
+
 };