EAGLESIX-2651: Cond: fix minor bugs w/ 2.6.5 port

lib/pipeline/expressions/CondExpression.js

@@ -6,114 +6,53 @@
  * @namespace mungedb-aggregate.pipeline.expressions
  * @module mungedb-aggregate
  * @constructor
- **/
-var CondExpression = module.exports = function CondExpression(vars) {
-		if (arguments.length !== 0) throw new Error("zero args expected");
+ */
+var CondExpression = module.exports = function CondExpression() {
+	if (arguments.length !== 0) throw new Error(klass.name + ": expected args: NONE");
     base.call(this);
-}, klass = CondExpression,
-	base = require("./FixedArityExpressionT")(klass, 3),
-	proto = klass.prototype = Object.create(base.prototype, {
-		constructor: {
-			value: klass
-		}
-	});
+}, klass = CondExpression, base = require("./FixedArityExpressionT")(CondExpression, 3), proto = klass.prototype = Object.create(base.prototype, {constructor:{value:klass}});
 
-// DEPENDENCIES
 var Value = require("../Value"),
-    Expression = require("./Expression"),
-	FixedArityExpressionT = require("./FixedArityExpressionT");
-
-// PROTOTYPE MEMBERS
-klass.opName = "$cond";
-proto.getOpName = function getOpName() {
-    return klass.opName;
-};
+    Expression = require("./Expression");
 
-/**
- * A utility class. Not in the c++ code.  If an operand has a .value, or if it has a ._fieldPath, it is not nullish.
- * I don't have 100% confidence these are the only choices.
- * @param op				An operand that should be tested for being null-ish.
- * @returns {boolean}
- */
-klass.isNullish = function isNullish(op) {
-	return (op.value === null || op.value === undefined) && !op._fieldPath;
+proto.evaluateInternal = function evaluateInternal(vars) {
+	var cond = this.operands[0].evaluateInternal(vars);
+	var idx = Value.coerceToBool(cond) ? 1 : 2;
+	return this.operands[idx].evaluateInternal(vars);
 };
 
-/**
- *
- * @param expr	- I expect this to be the RHS of $cond:{...} or $cond:[,,,]
- * @param vps
- * @returns {*}
- */
 klass.parse = function parse(expr, vps) {
-	// There may only be one argument - an array of 3 items, or a hash containing 3 keys.
-    //this.checkArgLimit(3);
-
-    // if not an object, return;
-	// todo I don't understand why we'd do this.  shouldn't expr be {}, [], or wrong?
-    if (typeof(expr) !== 'object')
-		return FixedArityExpressionT.parse(expr, vps);
-
-//NOTE: Deviation from Mongo: The c++ code has a verify( $cond ) structure. The implementation below will never work as
-// $cond has been removed before we get here.
-//	if(!(klass.opName in expr)) {
-//		throw new Error("Invalid expression. Expected to see '"+klass.opName+"'");
-//	}
+    if (Value.getType(expr) !== "Object") {
+		return base.parse(expr, vps);
+	}
+	// verify(str::equals(expr.fieldName(), "$cond")); //NOTE: DEVIATION FROM MONGO: we do not have fieldName any more and not sure this is even possible anyway
 
     var ret = new CondExpression();
+	ret.operands.length = 3;
 
 	var args = expr;
-
-	if (args instanceof Array) {
-		// it's the array form. Convert it to the object form.
-		if (args.length !== 3) throw new Error("$cond requires exactly three arguments");
-		for(var i=0; i<3; i++) ret.operands[i] = Expression.parseOperand(args[i], vps);
-	} else {
-		// This is the object form. Find the keys regardless of their order.
-		Object.keys(args).forEach(function (arg) {
-			if (arg === 'if') {
-				ret.operands[0] = Expression.parseOperand(args.if, vps);
-			}
-			else if (arg === 'then') {
-				ret.operands[1] = Expression.parseOperand(args.then, vps);
-			}
-			else if (arg === 'else') {
-				ret.operands[2] = Expression.parseOperand(args.else, vps);
-			}
-			else {
-				throw new Error("Unrecognized parameter to $cond: '" + arg + "'; code 17083");
-			}
-		});
+	for (var argfieldName in args) {
+		if (!args.hasOwnProperty(argfieldName)) continue;
+		if (argfieldName === "if") {
+			ret.operands[0] = Expression.parseOperand(args.if, vps);
+		} else if (argfieldName === "then") {
+			ret.operands[1] = Expression.parseOperand(args.then, vps);
+		} else if (argfieldName === "else") {
+			ret.operands[2] = Expression.parseOperand(args.else, vps);
+		} else {
+			throw new Error("Unrecognized parameter to $cond: '" + argfieldName + "'; uasserted code 17083");
+		}
 	}
 
-	// The Operands array should have been loaded.  Make sure they are all reasonable.
-	// TODO I think isNullish is brittle.
-    if (klass.isNullish(ret.operands[0])) throw new Error("Missing 'if' parameter to $cond; code 17080");
-    if (klass.isNullish(ret.operands[1])) throw new Error("Missing 'then' parameter to $cond; code 17081");
-    if (klass.isNullish(ret.operands[2])) throw new Error("Missing 'else' parameter to $cond; code 17082");
+    if (!ret.operands[0]) throw new Error("Missing 'if' parameter to $cond; uassert code 17080");
+    if (!ret.operands[1]) throw new Error("Missing 'then' parameter to $cond; uassert code 17081");
+    if (!ret.operands[2]) throw new Error("Missing 'else' parameter to $cond; uassert code 17082");
 
     return ret;
 };
 
-/**
- * Use the $cond operator with the following syntax:
- * { $cond: { if: <boolean-expression>, then: <true-case>, else: <false-case-> } }
- * -or-
- * { $cond: [ <boolean-expression>, <true-case>, <false-case> ] }
- * @method evaluate
- **/
-proto.evaluateInternal = function evaluateInternal(vars) {
-		var pCond1 = this.operands[0].evaluateInternal(vars);
+Expression.registerExpression("$cond", CondExpression.parse);
 
-		this.idx = 0;
-		if (Value.coerceToBool(pCond1)) {
-			this.idx = 1;
-		} else {
-			this.idx = 2;
-		}
-
-		return this.operands[this.idx].evaluateInternal(vars);
+proto.getOpName = function getOpName() {
+	return "$cond";
 };
-
-/** Register Expression */
-Expression.registerExpression(klass.opName, klass.parse);

test/lib/pipeline/expressions/CondExpression_test.js

@@ -5,127 +5,113 @@ var assert = require("assert"),
 	VariablesIdGenerator = require("../../../../lib/pipeline/expressions/VariablesIdGenerator"),
 	Expression = require("../../../../lib/pipeline/expressions/Expression");
 
+// 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.CondExpression = {
 
-	"CondExpression": {
+	"constructor()": {
 
-		"constructor()": {
+		"should not throw an Error when constructing without args": function testConstructor(){
+			assert.doesNotThrow(function(){
+				new CondExpression();
+			});
+		},
+
+		"should throw Error when constructing with 1 arg": function testConstructor1(){
+			assert.throws(function(){
+				new CondExpression(1);
+			});
+		},
 
-			"should not throw an Error when constructing without args": function testConstructor(){
-				assert.doesNotThrow(function(){
-					new CondExpression();
+	},
+
+	"#getOpName()": {
+
+		"should return the correct op name; $cond": function testOpName(){
+			assert.equal(new CondExpression().getOpName(), "$cond");
+		},
+
+	},
+
+	"#evaluate()": {
+		"array style": {
+
+			"should fail if there aren't enough arguments": function() {
+				assert.throws(function(){
+					Expression.parseOperand({$cond:[1,2]}, {});
 				});
 			},
 
-			"should throw Error when constructing with 1 arg": function testConstructor1(){
+			"should fail if there are too many arguments": function() {
 				assert.throws(function(){
-					new CondExpression(1);
+					Expression.parseOperand({$cond:[1, 2, 3, 4]}, {});
 				});
-			}
-		},
+			},
 
-		"#getOpName()": {
+			"should evaluate boolean expression as true, then return 1; [ true === true, 1, 0 ]": function () {
+				assert.strictEqual(Expression.parseOperand({$cond: [ true, 1, 0 ]}, {}).evaluate({}), 1);
+			},
 
-			"should return the correct op name; $cond": function testOpName(){
-				assert.equal(new CondExpression().getOpName(), "$cond");
-			}
+			"should evaluate boolean expression as false, then return 0; [ false === true, 1, 0 ]": function () {
+				assert.strictEqual(Expression.parseOperand({$cond: [ false, 1, 0 ]}, {}).evaluate({}), 0);
+			},
 
 		},
 
-		"#evaluateInternal()": {
-			"array style": {
+		"object style": {
 
-				"should fail if there aren't enough arguments": function() {
-					assert.throws(function(){
-						Expression.parseOperand({$cond:[1,2]}, {});
-					});
-				},
-				"should fail if there are too many arguments": function() {
-					assert.throws(function(){
-						Expression.parseOperand({$cond:[1, 2, 3, 4]}, {});
-					});
-				},
-				"should evaluate boolean expression as true, then return 1; [ true === true, 1, 0 ]": function () {
-					assert.strictEqual(Expression.parseOperand({$cond: [ true, 1, 0 ]}, {}).evaluateInternal({}), 1);
-				},
-
-				"should evaluate boolean expression as false, then return 0; [ false === true, 1, 0 ]": function () {
-					assert.strictEqual(Expression.parseOperand({$cond: [ false, 1, 0 ]}, {}).evaluateInternal({}), 0);
-				},
-				"should fail when the 'if' position is empty": function(){
-					assert.throws(function(){
-						Expression.parseOperand({$cond:[undefined, 2, 3]}, {});
-					});
-				},
-				"should fail when the 'then' position is empty": function(){
-					assert.throws(function(){
-						Expression.parseOperand({$cond:[1, undefined, 3]}, {});
-					});
-				},
-				"should fail when the 'else' position is empty": function(){
+			beforeEach: function(){
+				this.shouldFail = function(expr) {
 					assert.throws(function(){
-						Expression.parseOperand({$cond:[1, 2, undefined]}, {});
+						Expression.parseOperand(expr, {});
 					});
-				}
+				};
+				this.vps = new VariablesParseState(new VariablesIdGenerator());
 			},
 
-			"object style": {
-				beforeEach: function(){
-					this.shouldFail = function(expr) {
-						assert.throws(function(){
-							Expression.parseOperand(expr, {});
-						});
-					};
-					this.vps = new VariablesParseState(new VariablesIdGenerator());
-				},
-				"should fail because the $cond is missing": function(){
-					this.shouldFail({$zoot:[true, 1, 0 ]}, {});
-				},
-				"should fail because of missing if": function(){
-					this.shouldFail({$cond:{xif:1, then:2, else:3}});
-				},
-				"should fail because of missing then": function(){
-					this.shouldFail({$cond:{if:1, xthen:2, else:3}});
-				},
-				"should fail because of missing else": function(){
-					this.shouldFail({$cond:{if:1, then:2, xelse:3}});
-				},
-				"should fail because of empty if": function(){
-					this.shouldFail({$cond:{if:undefined, then:2, else:3}});
-				},
-				"should fail because of empty then": function(){
-					this.shouldFail({$cond:{if:1, then:undefined, else:3}});
-				},
-				"should fail because of empty else": function(){
-					this.shouldFail({$cond:{if:1, then:2, else:undefined}});
-				},
-				"should fail because of mystery args": function(){
-					this.shouldFail({$cond:{if:1, then:2, else:3, zoot:4}});
-				},
-				"should evaluate true": function(){
-					assert.strictEqual(
-						Expression.parseOperand({$cond:{ if: true, then: 1, else: 0}}, {}).evaluate({}),
-						1);
-				},
-				"should evaluate true even with mixed up args": function(){
-					assert.strictEqual(
-						Expression.parseOperand({$cond:{ else: 0, then: 1, if: "$a" }}, this.vps).evaluate({a: 1}),
-						1);
-				},
-				"should evaluate false": function(){
-					assert.strictEqual(
-						Expression.parseOperand({$cond:{ if: "$a", then: 0, else: 1}}, this.vps).evaluate({a: 0}),
-						1);
-				},
-				"should evaluate false even with mixed up args": function() {
-					assert.strictEqual(
-						Expression.parseOperand({$cond: { else: 1, then: 0, if: "$a"}}, this.vps).evaluate({a: 0}),
-						1);
-				}
-			}
-		}
-	}
-};
+			"should fail because of missing if": function(){
+				this.shouldFail({$cond:{ then:2, else:3}});
+			},
+
+			"should fail because of missing then": function(){
+				this.shouldFail({$cond:{if:1,  else:3}});
+			},
+
+			"should fail because of missing else": function(){
+				this.shouldFail({$cond:{if:1, then:2 }});
+			},
+
+			"should fail because of mystery args": function(){
+				this.shouldFail({$cond:{if:1, then:2, else:3, zoot:4}});
+			},
+
+			"should evaluate true": function(){
+				assert.strictEqual(
+					Expression.parseOperand({$cond:{ if: true, then: 1, else: 0}}, {}).evaluate({}),
+					1);
+			},
 
-if (!module.parent)(new(require("mocha"))()).ui("exports").reporter("spec").addFile(__filename).run(process.exit);
+			"should evaluate true even with mixed up args": function(){
+				assert.strictEqual(
+					Expression.parseOperand({$cond:{ else: 0, then: 1, if: "$a" }}, this.vps).evaluate({a: 1}),
+					1);
+			},
+
+			"should evaluate false": function(){
+				assert.strictEqual(
+					Expression.parseOperand({$cond:{ if: "$a", then: 0, else: 1}}, this.vps).evaluate({a: 0}),
+					1);
+			},
+
+			"should evaluate false even with mixed up args": function() {
+				assert.strictEqual(
+					Expression.parseOperand({$cond: { else: 1, then: 0, if: "$a"}}, this.vps).evaluate({a: 0}),
+					1);
+			},
+
+		},
+
+	},
+
+};