Merge pull request #378 from chris-pardy/bug-fixes

Bug fixes

examples/13-using-operator-decorators.js

@@ -79,12 +79,11 @@ async function start () {
     }
   }
 
-  engine.addRule(caseInsensitiveValidTags);
+  engine.addRule(caseInsensitiveValidTags)
 
   // third run with a tag that is valid if case insensitive
   facts = { tags: ['dev', 'PROD'] }
-  await engine.run(facts);
-
+  await engine.run(facts)
 }
 start()
 

src/almanac.js

@@ -105,7 +105,7 @@ export default class Almanac {
     } else {
       fact = new Fact(id, valueOrMethod, options)
     }
-    debug(`almanac::addFact id:${factId}`)
+    debug('almanac::addFact', { id: factId })
     this.factMap.set(factId, fact)
     if (fact.isConstant()) {
       this._setFactValue(fact, {}, fact.value)
@@ -120,7 +120,7 @@ export default class Almanac {
    * @param {Mixed} value - constant value of the fact
    */
   addRuntimeFact (factId, value) {
-    debug(`almanac::addRuntimeFact id:${factId}`)
+    debug('almanac::addRuntimeFact', { id: factId })
     const fact = new Fact(factId, value)
     return this._addConstantFact(fact)
   }
@@ -150,22 +150,22 @@ export default class Almanac {
       const cacheVal = cacheKey && this.factResultsCache.get(cacheKey)
       if (cacheVal) {
         factValuePromise = Promise.resolve(cacheVal)
-        debug(`almanac::factValue cache hit for fact:${factId}`)
+        debug('almanac::factValue cache hit for fact', { id: factId })
       } else {
-        debug(`almanac::factValue cache miss for fact:${factId}; calculating`)
+        debug('almanac::factValue cache miss, calculating', { id: factId })
         factValuePromise = this._setFactValue(fact, params, fact.calculate(params, this))
       }
     }
     if (path) {
-      debug(`condition::evaluate extracting object property ${path}`)
+      debug('condition::evaluate extracting object', { property: path })
       return factValuePromise
         .then(factValue => {
           if (factValue != null && typeof factValue === 'object') {
             const pathValue = this.pathResolver(factValue, path)
-            debug(`condition::evaluate extracting object property ${path}, received: ${JSON.stringify(pathValue)}`)
+            debug('condition::evaluate extracting object', { property: path, received: pathValue })
             return pathValue
           } else {
-            debug(`condition::evaluate could not compute object path(${path}) of non-object: ${factValue} <${typeof factValue}>; continuing with ${factValue}`)
+            debug('condition::evaluate could not compute object path of non-object', { path, factValue, type: typeof factValue })
             return factValue
           }
         })

src/condition.js

@@ -101,9 +101,12 @@ export default class Condition {
     ]).then(([rightHandSideValue, leftHandSideValue]) => {
       const result = op.evaluate(leftHandSideValue, rightHandSideValue)
       debug(
-        `condition::evaluate <${JSON.stringify(leftHandSideValue)} ${
-          this.operator
-        } ${JSON.stringify(rightHandSideValue)}?> (${result})`
+        'condition::evaluate', {
+          leftHandSideValue,
+          operator: this.operator,
+          rightHandSideValue,
+          result
+        }
       )
       return {
         result,

src/debug.js

@@ -1,10 +1,14 @@
-export default function debug (message) {
+
+function createDebug () {
   try {
     if ((typeof process !== 'undefined' && process.env && process.env.DEBUG && process.env.DEBUG.match(/json-rules-engine/)) ||
         (typeof window !== 'undefined' && window.localStorage && window.localStorage.debug && window.localStorage.debug.match(/json-rules-engine/))) {
-      console.log(message)
+      return console.debug.bind(console)
     }
   } catch (ex) {
     // Do nothing
   }
+  return () => {}
 }
+
+export default createDebug()

src/engine.js

@@ -172,7 +172,7 @@ class Engine extends EventEmitter {
     } else {
       fact = new Fact(id, valueOrMethod, options)
     }
-    debug(`engine::addFact id:${factId}`)
+    debug('engine::addFact', { id: factId })
     this.facts.set(factId, fact)
     return this
   }
@@ -241,11 +241,11 @@ class Engine extends EventEmitter {
   evaluateRules (ruleArray, almanac) {
     return Promise.all(ruleArray.map((rule) => {
       if (this.status !== RUNNING) {
-        debug(`engine::run status:${this.status}; skipping remaining rules`)
+        debug('engine::run, skipping remaining rules', { status: this.status })
         return Promise.resolve()
       }
       return rule.evaluate(almanac).then((ruleResult) => {
-        debug(`engine::run ruleResult:${ruleResult.result}`)
+        debug('engine::run', { ruleResult: ruleResult.result })
         almanac.addResult(ruleResult)
         if (ruleResult.result) {
           almanac.addEvent(ruleResult.event, 'success')
@@ -286,7 +286,7 @@ class Engine extends EventEmitter {
       }
 
       almanac.addFact(fact)
-      debug(`engine::run initialized runtime fact:${fact.id} with ${fact.value}<${typeof fact.value}>`)
+      debug('engine::run initialized runtime fact', { id: fact.id, value: fact.value, type: typeof fact.value })
     }
     const orderedSets = this.prioritizeRules()
     let cursor = Promise.resolve()

src/operator-map.js

@@ -22,7 +22,7 @@ export default class OperatorMap {
     } else {
       operator = new Operator(operatorOrName, cb)
     }
-    debug(`operatorMap::addOperator name:${operator.name}`)
+    debug('operatorMap::addOperator', { name: operator.name })
     this.operators.set(operator.name, operator)
   }
 
@@ -64,7 +64,7 @@ export default class OperatorMap {
     } else {
       decorator = new OperatorDecorator(decoratorOrName, cb)
     }
-    debug(`operatorMap::addOperatorDecorator name:${decorator.name}`)
+    debug('operatorMap::addOperatorDecorator', { name: decorator.name })
     this.decorators.set(decorator.name, decorator)
   }
 
@@ -110,15 +110,15 @@ export default class OperatorMap {
         const decoratorName = opName.slice(0, firstDecoratorIndex)
         const decorator = this.decorators.get(decoratorName)
         if (!decorator) {
-          debug(`operatorMap::get invalid decorator named ${decoratorName}`)
+          debug('operatorMap::get invalid decorator', { name: decoratorName })
           return null
         }
         // we're going to apply this later, use unshift since we'll apply in reverse order
         decorators.unshift(decorator)
         // continue looking for a known operator with the rest of the name
         opName = opName.slice(firstDecoratorIndex + 1)
       } else {
-        debug(`operatorMap::get no operator named ${opName}`)
+        debug('operatorMap::get no operator', { name: opName })
         return null
       }
     }

src/rule.js

@@ -249,7 +249,7 @@ class Rule extends EventEmitter {
       return Promise.all(
         conditions.map((condition) => evaluateCondition(condition))
       ).then((conditionResults) => {
-        debug('rule::evaluateConditions results', conditionResults)
+        debug('rule::evaluateConditions', { results: conditionResults })
         return method.call(conditionResults, (result) => result === true)
       })
     }
@@ -274,36 +274,16 @@ class Rule extends EventEmitter {
         // this also covers the 'not' case which should only ever have a single condition
         return evaluateCondition(conditions[0])
       }
-      let method = Array.prototype.some
-      if (operator === 'all') {
-        method = Array.prototype.every
-      }
       const orderedSets = this.prioritizeConditions(conditions)
-      let cursor = Promise.resolve()
+      let cursor = Promise.resolve(operator === 'all')
       // use for() loop over Array.forEach to support IE8 without polyfill
       for (let i = 0; i < orderedSets.length; i++) {
         const set = orderedSets[i]
-        let stop = false
         cursor = cursor.then((setResult) => {
-          // after the first set succeeds, don't fire off the remaining promises
-          if ((operator === 'any' && setResult === true) || stop) {
-            debug(
-              'prioritizeAndRun::detected truthy result; skipping remaining conditions'
-            )
-            stop = true
-            return true
-          }
-
-          // after the first set fails, don't fire off the remaining promises
-          if ((operator === 'all' && setResult === false) || stop) {
-            debug(
-              'prioritizeAndRun::detected falsey result; skipping remaining conditions'
-            )
-            stop = true
-            return false
-          }
-          // all conditions passed; proceed with running next set in parallel
-          return evaluateConditions(set, method)
+          // rely on the short-circuiting behavior of || and && to avoid evaluating subsequent conditions
+          return operator === 'any'
+            ? (setResult || evaluateConditions(set, Array.prototype.some))
+            : (setResult && evaluateConditions(set, Array.prototype.every))
         })
       }
       return cursor

test/engine-operator-map.test.js

@@ -77,10 +77,10 @@ describe('Engine Operator Map', () => {
   })
 
   it('the swap decorator', () => {
-    const factValue = 1;
-    const jsonValue = [1, 2, 3];
+    const factValue = 1
+    const jsonValue = [1, 2, 3]
 
-    const op = engine.operators.get('swap:contains');
+    const op = engine.operators.get('swap:contains')
     expect(op.evaluate(factValue, jsonValue)).to.be.true()
   })
 })