From 8d1fe0ac61ac933f78e1f8f2e88c38ca064a2df0 Mon Sep 17 00:00:00 2001 From: Cache Hamm Date: Fri, 4 Jun 2021 09:33:38 -0600 Subject: [PATCH] Rework engine.removeRule to be based off of rule name --- CHANGELOG.md | 5 +- docs/engine.md | 9 +-- src/engine.js | 18 +++--- src/rule.js | 17 ------ test/engine.test.js | 107 +++++++++++++++++++++++------------ test/rule.test.js | 29 ---------- test/support/rule-factory.js | 1 + 7 files changed, 90 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e57e2f7..cc9bcf9 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/engine.md b/docs/engine.md index 593a2fd..46d0a1b 100644 --- a/docs/engine.md +++ b/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) diff --git a/src/engine.js b/src/engine.js index 970e2f0..c6407c3 100644 --- a/src/engine.js +++ b/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 } /** diff --git a/src/rule.js b/src/rule.js index 69067ec..5db0191 100644 --- a/src/rule.js +++ b/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 diff --git a/test/engine.test.js b/test/engine.test.js index 8136c03..6030466 100644 --- a/test/engine.test.js +++ b/test/engine.test.js @@ -86,18 +86,26 @@ 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/) @@ -105,59 +113,86 @@ describe('Engine', () => { }) 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) }) }) }) diff --git a/test/rule.test.js b/test/rule.test.js index ea367de..bc3fb6a 100644 --- a/test/rule.test.js +++ b/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/) diff --git a/test/support/rule-factory.js b/test/support/rule-factory.js index 2e9b08a..f946780 100644 --- a/test/support/rule-factory.js +++ b/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: [{