microsoft / RulesEngine

A Json based Rules Engine with extensive Dynamic expression support
https://microsoft.github.io/RulesEngine/
MIT License
3.47k stars 528 forks source link

Caching Issue in Literals Dictionary Causes Valid Expression to Fail #614

Closed RenanCarlosPereira closed 2 days ago

RenanCarlosPereira commented 1 week ago

Description: There is a bug in the Rules Engine's RuleExpressionParser where a valid expression fails due to a caching issue in the Literals dictionary in System.Linq.Dynamic.Core.

Steps to Reproduce:

  1. Create a test case where a nullable decimal property is evaluated against a double literal.
  2. The first expression (Board.NumberOfMembers = 0.2d) throws an exception as expected due to a type mismatch.
  3. The second expression (Board.NumberOfMembers = 0.2) should be valid but throws an exception because the first expression is cached in the Literals dictionary.

Test Code:

[Fact]
public void Test()
{
    var board = new { NumberOfMembers = default(decimal?) };

    var parameter = RulesEngine.Models.RuleParameter.Create("Board", board);

    var parser = new RulesEngine.ExpressionBuilders.RuleExpressionParser();

    try
    {
        const string expression1 = "Board.NumberOfMembers = 0.2d";
        var result1 = parser.Evaluate<bool>(expression1, new[] { parameter });
        result1.Should().BeFalse();
    }
    catch (Exception)
    {
        // passing it over.
    }

    // This will throw an exception even if the expression is valid,
    // because the first one executed and cached in the Literals Dictionary.
    // This should not throw as it's valid.
    const string expression2 = "Board.NumberOfMembers = 0.2";
    var result2 = parser.Evaluate<bool>(expression2, new[] { parameter });
    result2.Should().BeFalse();
}

Expected Behavior: The second expression (Board.NumberOfMembers = 0.2) should be evaluated correctly and not throw an exception.

Actual Behavior: The second expression throws an exception due to the first expression being cached in the Literals dictionary.

Potential Cause: The caching mechanism in the Literals dictionary in System.Linq.Dynamic.Core does not handle different numeric literal types correctly, leading to invalid caching of expressions.

asulwer commented 1 week ago

this repo is no longer maintained. this issue will always stay open. please use a maintained fork, such as mine, to submit a new issue

i did see that your issue was resolved?

once a new version from zzzprojects is released then i will update my fork. i am adding this test to RulesEngine to support the future solution

asulwer commented 1 week ago

when i flip the two expressions the issue doesn't appear. maybe a temp solution?

RenanCarlosPereira commented 1 week ago

yeah, the issue is not showing up when you change the order because it is caching the first one. So once the dictionary has the value without the literal it will work. but that can't be a temporary solution because when I spin up my app the dictionary is empty, so if someone creates a rule passing that literal it will be cached and the only thing that I can do is restart the application.

I opened the bug here because the Nuget is pointing to this repo, how are we managing the nuget versions?

Do we have access to keep moving the Nuget from your fork?

asulwer commented 1 week ago

The Nuget solution will be an entirely new package. I am working on solving the many issues that exist before i release to Nuget. You can manually add the release from github until i release a Nuget solution?

i am working on a possible solution (as i type this) while we wait on zzzprojects.

asulwer commented 1 week ago

the second expression works IF you use literal notation. i presented this exact solution in a previous issue you opened 583?

const string expression2 = "Board.NumberOfMembers = 0.2m";

first one fails as expected and second one does not

This 'may' be a caching issue with Sytem.Linq.Dynamic.Core, as you say. based on this discussion the issue is your use of literal notation. the second fails because the literal notation is not correct so it falls back on what was previously used.

Note: 'falling back' is my educated guess as i have not looked at Ssytem.Linq.Dynamic.Core code to confirm this.

RenanCarlosPereira commented 1 week ago

I opened a PR there to fix this, that got merged to master today a few hours ago.

They just need to release a new version.

Not sure how you will do a workaround in this, cuz I checked the code and the expressions are created internally in the package

https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/824

RenanCarlosPereira commented 1 week ago

This other bug you mentioned is not related to this one

The 583 is evaluating the expression wrong because of fast compiler

asulwer commented 1 week ago

the other bug is a symptom of the same underlying issue, literal notation

RenanCarlosPereira commented 1 week ago

I don't think it is related man, the issue issue is related to the lib https://github.com/dadhi/FastExpressionCompiler

We can talk about it in that thread and leave this one. As this one will get resolved when we got a new version of dynamic linq

Check out this line

https://github.com/microsoft/RulesEngine/blob/dc5298989583954cd6aac598267747b4ce635f45/src/RulesEngine/ExpressionBuilders/RuleExpressionParser.cs#L66

Thats the problem with the fast compiler, thats why its not evaluating correctly. It might be a bug inside that other pack

What do you think?

asulwer commented 1 week ago

as to that other issue, i have that solved in my fork. i pushed a nuget package RulesEngineEx

RenanCarlosPereira commented 1 week ago

Nice, curious on how you fixed it, do you have an PR? 😄

asulwer commented 1 week ago

this project is dead at this location, the author stated so. he is no longer working for Microsoft and they have abandoned this project. further development is being done on my fork.

RenanCarlosPereira commented 2 days ago

this bug was resolved in the lib system.dynamic.linq.core in this PR: https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/824

there's a new version of this package that got released, the rules engine got updated with this package and the tests are passing now.

this was resolved in this fork: https://github.com/asulwer/RulesEngine