EAGLESIX-2651: Let: better sync w/ 2.6.5 code, fix tests to match new code

lib/pipeline/expressions/LetExpression.js

@@ -1,127 +1,126 @@
 "use strict";
 
 var LetExpression = module.exports = function LetExpression(vars, subExpression){
-	if (arguments.length !== 2) throw new Error("Two args expected");
+	if (arguments.length !== 2) throw new Error(klass.name + ": expected args: vars, subExpression");
 	this._variables = vars;
 	this._subExpression = subExpression;
 }, klass = LetExpression, Expression = require("./Expression"), base = Expression, proto = klass.prototype = Object.create(base.prototype, {constructor:{value:klass}});
 
-// DEPENDENCIES
-var Variables = require("./Variables"),
-	VariablesParseState = require("./VariablesParseState");
 
-// PROTOTYPE MEMBERS
+var Value = require("../Value"),
+	Variables = require("./Variables");
 
-klass.parse = function parse(expr, vpsIn){
-	//NOTE: DEVIATION FROM MONGO: I don't believe this works for us since the operator has been removed from expr by the time
-	// we get here.
-//	if(!("$let" in expr)) {
-//		throw new Error("Tried to create a $let with something other than let. Looks like your parse map went all funny.");
-//	}
-
-	if(typeof(expr) !== 'object' || (expr instanceof Array)) {
-		throw new Error("$let only supports an object as its argument: 16874");
-	}
 
-	var args = expr,
-		varsElem = args.vars,
-		inElem = args['in']; // args.in; ??
-
-	//NOTE: DEVIATION FROM MONGO: 1. These if statements are in a loop in the c++ version,
-	// 2. 'vars' and 'in' are each mandatory here. in the c++ code you only need one of the two.
-	// 3. Below, we croak if there are more than 2 arguments.  The original does not have this limitation, specifically.
-	// Upon further review, I think our code is more accurate.  The c++ code will accept if there are multiple 'in'
-	// or 'var' values. The previous ones will be overwritten by newer ones.
-	//
-	// Final note - I think this code is fine.
-	//
-	if(!varsElem) {
-		throw new Error("Missing 'vars' parameter to $let: 16876");
-	}
-	if(!inElem) {
-		throw new Error("Missing 'in' parameter to $let: 16877");
-	}
+function NameAndExpression(name, expr){
+	this.name = name;
+	this.expression = expr;
+}
 
-	// Should this be !== 2?  Why would we have fewer than 2 arguments?  Why do we even care what the length of the
-	// array is? It may be an optimization of sorts. But what we're really wanting here is, 'If any keys are not "in"
-	// or "vars" then we need to bugcheck.'
-	if(Object.keys(args).length > 2) {
-		var bogus = Object.keys(args).filter(function(x) {return !(x === 'in' || x === 'vars');});
-		throw new Error("Unrecognized parameter to $let: " + bogus.join(",") + "- 16875");
-	}
 
-	var vpsSub = vpsIn, //new VariablesParseState(vpsIn),
-		vars = {};
+klass.parse = function parse(expr, vpsIn){
 
-//	varsElem.forEach(function(varName){
-	Object.keys(varsElem).forEach(function(varName) {
-//		var varName = varsElem[key];
+	// if (!(exprFieldName === "$let")) throw new Error("Assertion failure"); //NOTE: DEVIATION FROM MONGO: we do not have exprFieldName here
+
+	if (Value.getType(expr) !== "Object")
+		throw new Error("$let only supports an object as it's argument; uassert code 16874");
+	var args = expr;
+
+	// varsElem must be parsed before inElem regardless of BSON order.
+	var varsElem,
+		inElem;
+	for (var argFieldName in args) {
+		var arg = args[argFieldName];
+		if (argFieldName === "vars") {
+			varsElem = arg;
+		} else if (argFieldName === "in") {
+			inElem = arg;
+		} else {
+			throw new Error("Unrecognized parameter to $let: " + argFieldName + "; uasserted code 16875");
+		}
+	}
+
+	if (!varsElem)
+		throw new Error("Missing 'vars' parameter to $let; uassert code 16876");
+	if (!inElem)
+		throw new Error("Missing 'in' parameter to $let; uassert code 16877");
+
+	// parse "vars"
+	var vpsSub = vpsIn, // vpsSub gets our vars, vpsIn doesn't.
+		vars = {}; // using like a VariableMap
+	if (Value.getType(varsElem) !== "Object") //NOTE: emulate varsElem.embeddedObjectUserCheck()
+		throw new Error("invalid parameter: expected an object (vars); uasserted code 10065");
+	for (var varName in varsElem) {
+		var varElem = varsElem[varName];
 		Variables.uassertValidNameForUserWrite(varName);
 		var id = vpsSub.defineVariable(varName);
 
-		vars[id] = {};
-		vars[id][varName] = Expression.parseOperand(varsElem, vpsIn);
-	});
+		vars[id] = new NameAndExpression(varName,
+			Expression.parseOperand(varElem, vpsIn)); // only has outer vars
+	}
+
+	// parse "in"
+	var subExpression = Expression.parseOperand(inElem, vpsSub); // has our vars
 
-	var subExpression = Expression.parseOperand(inElem, vpsSub);
 	return new LetExpression(vars, subExpression);
 };
 
+
 proto.optimize = function optimize() {
-	// This statement doesn't look necessary. We do this work later on if there aren't (or are!) variables.
-	if(this._variables.length == 0) {
+	if (Object.keys(this._variables).length === 0) {
+		// we aren't binding any variables so just return the subexpression
 		return this._subExpression.optimize();
 	}
 
-	for(var id in this._variables){
-		for(var name in this._variables[id]) {
-			//NOTE: DEVIATION FROM MONGO: This is actually ok. The c++ code does this with a single map. The js structure
-			// is nested objects.
-			var optimized = this._variables[id][name].optimize();
-			this._variables[id][name] = optimized;
-		}
+	for (var id in this._variables) {
+		this._variables[id].expression = this._variables[id].expression.optimize();
 	}
 
+	// TODO be smarter with constant "variables"
 	this._subExpression = this._subExpression.optimize();
 
 	return this;
 };
 
+
 proto.serialize = function serialize(explain) {
 	var vars = {};
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			vars[name] = this._variables[id][name];
-		}
+	for (var id in this._variables) {
+		vars[this._variables[id].name] = this._variables[id].expression.serialize(explain);
 	}
 
-	return {$let: {vars:vars, 'in':this._subExpression.serialize(explain)}};
+	return {
+		$let: {
+			vars: vars,
+			in : this._subExpression.serialize(explain)
+		}
+	};
 };
 
+
 proto.evaluateInternal = function evaluateInternal(vars) {
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			// It is guaranteed at parse-time that these expressions don't use the variable ids we
-			// are setting
-			var first = id,
-				second = this._variables[first][name];
-			//Note - The unary plus on first converts it to an int. Don't remove it.
-			vars.setValue(+first, second._expressions[name].evaluateInternal(vars));
-		}
+	for (var id in this._variables) {
+		var itFirst = +id, //NOTE: using the unary + to coerce it to a Number
+			itSecond = this._variables[itFirst];
+		// It is guaranteed at parse-time that these expressions don't use the variable ids we
+		// are setting
+		vars.setValue(itFirst,
+			itSecond.expression.evaluateInternal(vars));
 	}
 
 	return this._subExpression.evaluateInternal(vars);
 };
 
+
 proto.addDependencies = function addDependencies(deps, path){
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			var first = id,
-				second = this._variables[first][name];
-			second._expressions[name].addDependencies(deps);
-		}
+	for (var id in this._variables) {
+		var itFirst = +id, //NOTE: using the unary + to coerce it to a Number
+			itSecond = this._variables[itFirst];
+			itSecond.expression.addDependencies(deps);
 	}
+
+	// TODO be smarter when CURRENT is a bound variable
 	this._subExpression.addDependencies(deps);
 };
 
+
 Expression.registerExpression("$let", LetExpression.parse);

test/lib/pipeline/expressions/LetExpression_test.js

@@ -45,63 +45,62 @@ exports.LetExpression = {
 
 	"#parse()": {
 
-		beforeEach: function() {
-			this.dieForTheRightReason = function(expr, regex) {
-				var self = this;
-				assert.throws(function() {
-					Expression.parseOperand(expr, self.vps);
-				}, regex);
-			};
-		},
-
 		"should throw if $let isn't in expr": function() {
-			this.dieForTheRightReason({$xlet: ['$$a', 1]}, /15999/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$xlet: ['$$a', 1]}, self.vps);
+			}, /15999/);
 		},
 
 		"should throw if the $let expression isn't an object": function() {
-			this.dieForTheRightReason({$let: "this is not an object"}, /16874/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: "this is not an object"}, self.vps);
+			}, /16874/);
 		},
 
 		"should throw if the $let expression is an array": function() {
-			this.dieForTheRightReason({$let: [1, 2, 3]}, /16874/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: [1, 2, 3]}, self.vps);
+			}, /16874/);
 		},
 
 		"should throw if there is no vars parameter to $let": function() {
-			this.dieForTheRightReason({$let: {noVars: 1}}, /16876/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: undefined}}, self.vps);
+			}, /16876/);
 		},
 
 		"should throw if there is no input parameter to $let": function() {
-			this.dieForTheRightReason({$let: {vars: 1, noIn: 2}}, /16877/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: 1, in: undefined}}, self.vps);
+			}, /16877/);
 		},
 
 		"should throw if any of the arguments to $let are not 'in' or 'vars'": function() {
-			this.dieForTheRightReason({$let: {vars: 1, in: 2, zoot:3}}, /16875/);
-		},
-
-		"should throw if the var name is not writable (1)": function() {
-			this.dieForTheRightReason({$let: {vars: {a:"@"}, in: 2}}, /16873/);
-		},
-
-		"should throw if the var name is not writable (2)": function() {
-			this.dieForTheRightReason({$let: {vars: {a:"$$"}, in: 2}}, /16869/);
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: 1, in: 2, zoot:3}}, self.vps);
+			}, /16875/);
 		},
 
 		"should return a Let expression": function() {
 			var x = Expression.parseOperand({$let: {vars: {a:{$const:123}}, in: 2}}, this.vps);
 			assert(x instanceof LetExpression);
 			assert(x._subExpression instanceof ConstantExpression);
-			assert(x._subExpression.getValue() == 2);
-			assert(x._variables[0].a._expressions.a instanceof ConstantExpression);
-			assert.equal(x._variables[0].a._expressions.a.getValue(), 123, "Expected to see 123, but instead saw "+x._variables[0].a._expressions.a.getValue());
+			assert.strictEqual(x._subExpression.getValue(), 2);
+			assert(x._variables[0].expression instanceof ConstantExpression);
+			assert.strictEqual(x._variables[0].expression.getValue(), 123);
 		},
 
 		"should show we collect multiple vars": function() {
 			var x = Expression.parseOperand({$let: {vars: {a:{$const:1}, b:{$const:2}, c:{$const:3}}, in: 2}}, this.vps);
-			//TODO Kyle, this is the epitome of why I think the data structures are screwy. Put a break on the
-			//next line and look at x.
-			assert.deepEqual(x._variables[0].a._expressions.a.getValue(), 1);
-			assert.deepEqual(x._variables[1].b._expressions.b.getValue(), 2);
-			assert.deepEqual(x._variables[2].c._expressions.c.getValue(), 3);
+			assert.strictEqual(x._variables[0].expression.getValue(), 1);
+			assert.strictEqual(x._variables[1].expression.getValue(), 2);
+			assert.strictEqual(x._variables[2].expression.getValue(), 3);
 		},
 
 	},
@@ -109,30 +108,31 @@ exports.LetExpression = {
 	"#optimize()": {
 
 		beforeEach: function() {
-			this.testInOpt = function (expr, expected) {
-				assert(expr._subExpression instanceof ConstantExpression, "Expected the $multiply to be optimized to a constant. Saw '" + expr._subExpression.constructor.name + "'");
-				assert.equal(expr._subExpression.operands.length, 0, "Expected no operands, saw " + expr._subExpression.operands.length);
-				assert(expr._subExpression.getValue(), expected, "Expected the multiply to optimize to "+expected+" but saw "+expr._subExpression.getValue());
+			this.testInOpt = function(expr, expected) {
+				assert(expr._subExpression instanceof ConstantExpression, "should have $const subexpr");
+				assert.strictEqual(expr._subExpression.operands.length, 0);
+				assert.strictEqual(expr._subExpression.getValue(), expected);
 			};
-			this.testVarOpt = function (expr, expected) {
-				var here = expr._variables[0].a._expressions.a;
-				assert(here instanceof ConstantExpression, "Expected the $multiply to be optimized to a constant. Saw '" + here.constructor.name + "'");
-				assert(here.getValue(), expected, "Expected the multiply to optimize to "+expected+" but saw "+here.getValue());
+			this.testVarOpt = function(expr, expected) {
+				var varExpr = expr._variables[0].expression;
+				assert(varExpr instanceof ConstantExpression, "should have $const first var");
+				assert.strictEqual(varExpr.getValue(), expected);
 			};
 		},
 
-		"should optimize subexpressions if there are no variables": function() {
-			var x = Expression.parseOperand({$let: {vars: {}, in: {$multiply: [2,3]}}}, this.vps).optimize();
-			this.testInOpt(x, 6);
+		"should optimize to subexpression if no variables": function() {
+			var x = Expression.parseOperand({$let:{vars:{}, in:{$multiply:[2,3]}}}, this.vps).optimize();
+			assert(x instanceof ConstantExpression, "should become $const");
+			assert.strictEqual(x.getValue(), 6);
 		},
 
 		"should optimize variables": function() {
-			var x = Expression.parseOperand({$let: {vars: {a: {$multiply:[5,4]}}, in: {$const: 6}}}, this.vps).optimize();
+			var x = Expression.parseOperand({$let:{vars:{a:{$multiply:[5,4]}}, in:{$const:6}}}, this.vps).optimize();
 			this.testVarOpt(x, 20);
 		},
 
 		"should optimize subexpressions if there are variables": function() {
-			var x = Expression.parseOperand({$let: {vars: {a: {$multiply:[5,4]}}, in: {$multiply: [2,3]}}}, this.vps).optimize();
+			var x = Expression.parseOperand({$let:{vars:{a:{$multiply:[5,4]}}, in: {$multiply:[2,3]}}}, this.vps).optimize();
 			this.testInOpt(x, 6);
 			this.testVarOpt(x, 20);
 		},
@@ -143,13 +143,13 @@ exports.LetExpression = {
 
 		"should serialize variables and the subexpression": function() {
 			var s = Expression.parseOperand({$let: {vars: {a:{$const:1}, b:{$const:2}}, in: {$multiply: [2,3]}}}, this.vps).optimize().serialize("zoot");
-			var expected = '{"$let":{"vars":{"a":{"excludeId":false,"_atRoot":false,"_expressions":{"a":{"value":1,"operands":[]},"b":{"value":2,"operands":[]}},"_order":["a","b"]},"b":{"excludeId":false,"_atRoot":false,"_expressions":{"a":{"value":1,"operands":[]},"b":{"value":2,"operands":[]}},"_order":["a","b"]}},"in":{"$const":6}}}';
-			assert.deepEqual(JSON.stringify(s), expected);
+			var expected = {$let:{vars:{a:{$const:1},b:{$const:2}},in:{$const:6}}};
+			assert.deepEqual(s, expected);
 		},
 
 	},
 
-	"#evaluateInternal()": {
+	"#evaluate()": {
 
 		"should perform the evaluation for variables and the subexpression": function() {
 			var x = Expression.parseOperand({$let: {vars: {a: '$in1', b: '$in2'}, in: { $multiply: ["$$a", "$$b"] }}}, this.vps).optimize();