Skip to content

can not remove rule #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tjpgt opened this issue Apr 3, 2019 · 6 comments
Closed

can not remove rule #120

tjpgt opened this issue Apr 3, 2019 · 6 comments

Comments

@tjpgt
Copy link

tjpgt commented Apr 3, 2019

Hello,I'm looking for a rule engine written in js,and I find this project easy to use,and following is my problem using the engine.
I add a rule,then run the engine per 1 second.
After 3 seconds,I remove the rule.
However,I still got "hello-world!" per second.
I think there need to be a remove rule test.
I have a look at the source code,the problem can be solved by adding this.prioritizedRules = null to removeRule.

Here is my code

let Engine = require('json-rules-engine').Engine
let Rule = require('json-rules-engine').Rule

let engine = new Engine()
let rule = new Rule({
  conditions: {
    all: [{
      fact: 'displayMessage',
      operator: 'equal',
      value: true
    }]
  },
  event: {
    type: 'message',
    params: {
      data: 'hello-world!'
    }
  }
})
engine.addRule(rule)
engine.addFact('displayMessage', true);
engine.on('success', (event, almanac, ruleResult) => {
  console.log(event.params.data)
})
setInterval(() => {
  engine
    .run()
}, 1000);
setTimeout(() => {
  console.log('remove');
  engine.removeRule(rule);
}, 3000)
@CacheControl
Copy link
Owner

@hehejiuhuizhe This is correct - if the engine is run() after 1 second, it makes sense that the success event would be fired, since the rule still exists in the engine.

If the values were reversed and the rule was removed after 1 second and the engine run at 3 seconds, in that case seeing the event fired would be unexpected.

Does that make sense? Or am I misunderstanding and you're making a different point?

@tjpgt
Copy link
Author

tjpgt commented Apr 4, 2019

I mean the event would be fired when I use engine.run().But I have removed the rule so that there is no rule in the engine.
I think the expected behaviour is that after I remove the rule,the event won't be fired when the engine run.

@CacheControl
Copy link
Owner

@hehejiuhuizhe I think you may be missing the concept behind setTimeout(). Here's the timeline of events based on the code you posted:

t=0: Engine is created and rule added
t=1000: Engine is run and event fired
t=3000: Rule is removed

@tjpgt
Copy link
Author

tjpgt commented Apr 8, 2019

@CacheControl
I use setInterval to run the engine and setTimeout to remove the rule.So the timeline is :
t=0: Engine is created and rule added
t=1000: Engine is run and event fired
t=2000: Engine is run and event fired
t=3000: Rule is removed
t=3000: Engine is run and event fired
t=4000: Engine is run and event fired
t=5000: Engine is run and event fired
....
The problem is that after 3000ms,the event shouldn't be fired.

@ozanerturk
Copy link
Contributor

I checked the scenario @hehejiuhuizhe seems rigth. I came across with same result. when I debugged the code I realize run method calls prioritizeRules() and it returns directly priotizedRules object if the priotizedRules list is not empty. Despite removeRule() does not clear the priotizedRules list. So even if you call removeRule() , run method is first looking to priotizedRules list to do the job and because removeRule does not clear the list there is still available rule in priotizedRules list.

if (!this.prioritizedRules) {

removeRule (rule) {

Simply little cache mechanizm brokes it. The list has to be recreated after removeRule method called or force to create new one in prioritizeRules method.
@CacheControl if you think this is the reason I can implement the fix if we agree on an idea.

@CacheControl
Copy link
Owner

@ozanerturk thanks for checking into this, and the offer to implement a fix.

I agree with your assessment. I think a good fix would be to add a line for setting the prioritizedRules variable to null, similar to how addRule() does it:

this.prioritizedRules = null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants