Rework engine.removeRule to be based off of rule name

Author: Cache Hamm

CHANGELOG.md

@@ -1,5 +1,6 @@
-#### 7.0.0 / 2021-06-03
-  * Rules now receive a default `id` property, when none is passed to the constructor.
+#### 6.1.0 / 2021-06-03
+  * engine.removeRule() now supports removing rules by name
+  * Added engine.updateRule(rule)
 
 #### 6.0.1 / 2021-03-09
   * Updates Typescript types to include `failureEvents` in EngineResult.

docs/engine.md

@@ -74,7 +74,6 @@ engine.removeFact('speed-of-light')
 ### engine.addRule(Rule instance|Object options)
 
 Adds a rule to the engine.  The engine will execute the rule upon the next ```run()```
-if the rule doesn't have value set at rule.id, a random string will be set instead.
 
 ```js
 let Rule = require('json-rules-engine').Rule
@@ -93,9 +92,11 @@ let rule = new Rule()
 engine.addRule(rule)
 ```
 
- ### engine.removeRule(Rule instance)
+ ### engine.removeRule(Rule instance | Any ruleName) -> Boolean
 
- Removes a rule from the engine.
+ Removes a rule from the engine, either by passing a rule object or a rule name. When removing by rule name, all rules matching the provided name will be removed.
+
+ Method returns true when rule was successfully remove, or false when not found.
 
 ```javascript
 // adds a rule
@@ -105,7 +106,7 @@ engine.addRule(rule)
 //remove it
 engine.removeRule(rule)
 //or
-engine.removeRule(rule.id)
+engine.removeRule(rule.name)
 ```
 
  ### engine.updateRule(Rule instance|Object options)

src/engine.js

@@ -60,7 +60,7 @@ class Engine extends EventEmitter {
    * @param {object|Rule} rule - rule definition. Must be a instance of Rule
    */
   updateRule (rule) {
-    const ruleIndex = this.rules.findIndex(ruleInEngine => (ruleInEngine.id === rule.id) && rule.id)
+    const ruleIndex = this.rules.findIndex(ruleInEngine => ruleInEngine.name === rule.name)
     if (ruleIndex > -1) {
       this.rules.splice(ruleIndex, 1)
       this.addRule(rule)
@@ -75,19 +75,21 @@ class Engine extends EventEmitter {
    * @param {object|Rule|string} rule - rule definition. Must be a instance of Rule
    */
   removeRule (rule) {
+    let ruleRemoved = false
     if (!(rule instanceof Rule)) {
-      const filteredRules = this.rules.filter(ruleInEngine => ruleInEngine.id !== rule)
-      const ruleRemoved = filteredRules.length !== this.rules.length
+      const filteredRules = this.rules.filter(ruleInEngine => ruleInEngine.name !== rule)
+      ruleRemoved = filteredRules.length !== this.rules.length
       this.rules = filteredRules
-      this.prioritizedRules = null
-      return ruleRemoved
     } else {
-      if (!rule) return false
       const index = this.rules.indexOf(rule)
-      if (index === -1) return false
+      if (index > -1) {
+        ruleRemoved = Boolean(this.rules.splice(index, 1).length)
+      }
+    }
+    if (ruleRemoved) {
       this.prioritizedRules = null
-      return Boolean(this.rules.splice(index, 1).length)
     }
+    return ruleRemoved
   }
 
   /**

src/rule.js

@@ -15,7 +15,6 @@ class Rule extends EventEmitter {
    * @param {string} options.event.params - parameters to pass to the event listener
    * @param {Object} options.conditions - conditions to evaluate when processing this rule
    * @param {any} options.name - identifier for a particular rule, particularly valuable in RuleResult output
-   * @param {any} options.id - identifier for a particular rule, particularly valuable in RuleResult output
    * @return {Rule} instance
    */
   constructor (options) {
@@ -36,13 +35,6 @@ class Rule extends EventEmitter {
       this.setName(options.name)
     }
 
-    if (!options || options.id === undefined) {
-      // auto generate an id if none was provided
-      this.setId('_' + Math.random().toString(36).substr(2, 9))
-    } else {
-      this.setId(options.id)
-    }
-
     const priority = (options && options.priority) || 1
     this.setPriority(priority)
 
@@ -61,15 +53,6 @@ class Rule extends EventEmitter {
     return this
   }
 
-  /**
-   * Sets the unique id of the rule
-   * @param {any} id - any truthy input
-   */
-  setId (id) {
-    this.id = id
-    return this
-  }
-
   /**
    * Sets the name of the rule
    * @param {any} name - any truthy input and zero is allowed

test/engine.test.js

@@ -86,78 +86,113 @@ describe('Engine', () => {
 
   describe('updateRule()', () => {
     it('updates rule', () => {
-      const rule = new Rule(factories.rule())
-      engine.addRule(rule)
+      let rule1 = new Rule(factories.rule({ name: 'rule1' }))
+      let rule2 = new Rule(factories.rule({ name: 'rule2' }))
+      engine.addRule(rule1)
+      engine.addRule(rule2)
       expect(engine.rules[0].conditions.all.length).to.equal(2)
-      rule.conditions = { all: [] }
-      engine.updateRule(rule)
-      expect(engine.rules[0].conditions.all.length).to.equal(0)
+      expect(engine.rules[1].conditions.all.length).to.equal(2)
+
+      rule1.conditions = { all: [] }
+      engine.updateRule(rule1)
+
+      rule1 = engine.rules.find(rule => rule.name === 'rule1')
+      rule2 = engine.rules.find(rule => rule.name === 'rule2')
+      expect(rule1.conditions.all.length).to.equal(0)
+      expect(rule2.conditions.all.length).to.equal(2)
     })
 
     it('should throw error if rule not found', () => {
-      const rule1 = new Rule(factories.rule())
+      const rule1 = new Rule(factories.rule({ name: 'rule1' }))
       engine.addRule(rule1)
-      const rule2 = new Rule(factories.rule())
+      const rule2 = new Rule(factories.rule({ name: 'rule2' }))
       expect(() => {
         engine.updateRule(rule2)
       }).to.throw(/Engine: updateRule\(\) rule not found/)
     })
   })
 
   describe('removeRule()', () => {
-    context('rule instance', () => {
-      it('removes the rule', () => {
-        const rule = new Rule(factories.rule())
-        engine.addRule(rule)
-        expect(engine.rules.length).to.equal(1)
+    function setup () {
+      const rule1 = new Rule(factories.rule({ name: 'rule1' }))
+      const rule2 = new Rule(factories.rule({ name: 'rule2' }))
+      engine.addRule(rule1)
+      engine.addRule(rule2)
+      engine.prioritizeRules()
+
+      return [rule1, rule2]
+    }
+    context('remove by rule.name', () => {
+      it('removes a single rule', () => {
+        const [rule1] = setup()
+        expect(engine.rules.length).to.equal(2)
 
-        const isRemoved = engine.removeRule(rule)
+        const isRemoved = engine.removeRule(rule1.name)
 
         expect(isRemoved).to.be.true()
-        expect(engine.rules.length).to.equal(0)
+        expect(engine.rules.length).to.equal(1)
         expect(engine.prioritizedRules).to.equal(null)
       })
 
-      it('can only remove added rules', () => {
-        expect(engine.rules.length).to.equal(0)
-        const rule = new Rule(factories.rule())
+      it('removes multiple rules with the same name', () => {
+        const [rule1] = setup()
+        const rule3 = new Rule(factories.rule({ name: rule1.name }))
+        engine.addRule(rule3)
+        expect(engine.rules.length).to.equal(3)
 
-        const isRemoved = engine.removeRule(rule)
+        const isRemoved = engine.removeRule(rule1.name)
 
-        expect(isRemoved).to.equal(false)
+        expect(isRemoved).to.be.true()
+        expect(engine.rules.length).to.equal(1)
+        expect(engine.prioritizedRules).to.equal(null)
       })
 
-      it('clears the "prioritizedRules" cache', () => {
-        const rule = new Rule(factories.rule())
-        engine.addRule(rule)
-        engine.prioritizeRules()
-        engine.removeRule(rule)
-        expect(engine.prioritizedRules).to.equal(null)
+      it('returns false when rule cannot be found', () => {
+        setup()
+        expect(engine.rules.length).to.equal(2)
+
+        const isRemoved = engine.removeRule('not-found-name')
+
+        expect(isRemoved).to.be.false()
+        expect(engine.rules.length).to.equal(2)
+        expect(engine.prioritizedRules).to.not.equal(null)
       })
     })
+    context('remove by rule object', () => {
+      it('removes a single rule', () => {
+        const [rule1] = setup()
+        expect(engine.rules.length).to.equal(2)
 
-    context('rule id', () => {
-      it('removes rule based on rule id', () => {
-        const rule = new Rule(factories.rule())
-        engine.addRule(rule)
+        const isRemoved = engine.removeRule(rule1)
+
+        expect(isRemoved).to.be.true()
         expect(engine.rules.length).to.equal(1)
+        expect(engine.prioritizedRules).to.equal(null)
+      })
 
-        const isRemoved = engine.removeRule(rule.id)
+      it('removes a single rule, even if two have the same name', () => {
+        const [rule1] = setup()
+        const rule3 = new Rule(factories.rule({ name: rule1.name }))
+        engine.addRule(rule3)
+        expect(engine.rules.length).to.equal(3)
+
+        const isRemoved = engine.removeRule(rule1)
 
         expect(isRemoved).to.be.true()
-        expect(engine.rules.length).to.equal(0)
+        expect(engine.rules.length).to.equal(2)
         expect(engine.prioritizedRules).to.equal(null)
       })
 
       it('returns false when rule cannot be found', () => {
-        const rule = new Rule(factories.rule())
-        engine.addRule(rule)
-        expect(engine.rules.length).to.equal(1)
+        setup()
+        expect(engine.rules.length).to.equal(2)
 
-        const isRemoved = engine.removeRule('not-found-id')
+        const rule3 = new Rule(factories.rule({ name: 'rule3' }))
+        const isRemoved = engine.removeRule(rule3)
 
         expect(isRemoved).to.be.false()
-        expect(engine.rules.length).to.equal(1)
+        expect(engine.rules.length).to.equal(2)
+        expect(engine.prioritizedRules).to.not.equal(null)
       })
     })
   })

test/rule.test.js

@@ -54,27 +54,6 @@ describe('Rule', () => {
       expect(rule.ruleEvent).to.eql(opts.event)
       expect(rule.name).to.eql(opts.name)
     })
-
-    context('id', () => {
-      it('autogenerates an id if none was provided', () => {
-        const rule = new Rule()
-        expect(rule.id).to.have.lengthOf(10)
-      })
-
-      it('sets the id if provided', () => {
-        let rule = new Rule({ id: 'test-id' })
-        expect(rule.id).to.equal('test-id')
-
-        rule = new Rule({ id: 0 })
-        expect(rule.id).to.equal(0)
-
-        rule = new Rule({ id: '' })
-        expect(rule.id).to.equal('')
-
-        rule = new Rule({ id: null })
-        expect(rule.id).to.equal(null)
-      })
-    })
   })
 
   describe('event emissions', () => {
@@ -117,14 +96,6 @@ describe('Rule', () => {
     })
   })
 
-  describe('setId()', () => {
-    it('changes the rule id', () => {
-      const newId = 'test-id-123'
-      rule.setId(newId)
-      expect(rule.id).to.equal(newId)
-    })
-  })
-
   describe('setEvent()', () => {
     it('throws if no argument provided', () => {
       expect(() => rule.setEvent()).to.throw(/Rule: setEvent\(\) requires event object/)

test/support/rule-factory.js

@@ -3,6 +3,7 @@
 module.exports = (options) => {
   options = options || {}
   return {
+    name: options.name,
     priority: options.priority || 1,
     conditions: options.conditions || {
       all: [{