search

CacheControl / json-rules-engine

A rules engine expressed in JSON
ISC License
2.54k stars 455 forks source link

Condition classes #356

Closed chris-pardy closed 8 months ago

chris-pardy commented 9 months ago

A major refactoring of the code around rules / conditions that makes no change to the actual execution of the code but does have a large impact on the potential to extend the rules engine.

Previously the Condition class was used for all the different levels of nested conditions. However only the comparison conditions could be executed with the all, any, not, and condition types being handled by the Rule class. In an effort to make this more extensible I've split the code into 5 condition subclasses for each type of condition and moved the handling code into the condition, out of the Rule. Additionally rather than the Condition constructor knowing how to build a condition I've added a new ConditionConstructor class that knows how to build conditions and passes itself to the Condition subclasses. With the ConditionConstructor present I've added support for passing it to the Rule and Engine class and then moved the check for top-level conditions to a TopLevelConditionConstructor class. By default there is no change to behavior of the engine, however you can easily set the conditionConstructor property on the Engine to do things like drop the need for top-level conditions, or add new types of conditions. New types of conditions in particular allow for vastly new features to be added that are effectively impossible to support now.

CacheControl commented 9 months ago

Will look at this today

chris-pardy commented 9 months ago

@chris-pardy thanks, the state of the Condition class has been bothering me for a while, so if nothing else this is a nice refactor. Look forward to seeing your longer term ideas come to fruition.

I've got a lot going on in my personal life atm, so I didn't go over this with a fine comb; maybe a good idea to have someone else from your org give it a review.

Sounds good. I'll get a second set of eyes on it. If you think the approach at a high level is in the right direction I'll fix the build issues.

chris-pardy commented 8 months ago

@CacheControl After doing a bit of thinking I'm actually going to close this PR. The problem with creating extensible condition classes like this is that they fundamentally break some of the typing that is exposed by the library.

Going into a 7.0 version I think this is the right approach but it's also changes pretty widely what can be done with something like the rule result. For instance if someone added a new "ternary operator condition" to the set of conditions then the structure of the rule result would need to account for this.

CacheControl commented 8 months ago

Makes sense, thanks for doing due diligence before proceeding!