zzzprojects / System.Linq.Dynamic.Core

The .NET Standard / .NET Core version from the System Linq Dynamic functionality.
https://dynamic-linq.net/
Apache License 2.0
1.57k stars 228 forks source link

Incorrect Handling of Qualifiers in ConstantExpressionHelper #821

Closed RenanCarlosPereira closed 4 months ago

RenanCarlosPereira commented 4 months ago

Description: There is an issue in the ExpressionPromoter class where the qualifier in the numeric literal expressions is not correctly handled, leading to incorrect type promotions. This is due to the way qualifiers are stored and retrieved from the ConstantExpressionHelper.

Steps to Reproduce:

  1. Create an instance of ParsingConfig.
  2. Use NumberParser to parse a numeric literal with a qualifier.
  3. Attempt to promote the parsed expression to a different type using ExpressionPromoter.

Example 1:

[Fact]
public void promoter()
{
    // Assign
    var parsingConfig = new ParsingConfig();
    var numberParser = new NumberParser(parsingConfig);
    var promoter = new ExpressionPromoter(parsingConfig);

    var doubleExpression = numberParser.ParseRealLiteral("10.5d", 'd', true);

    // Act & Assert
    var promotedExpression = promoter.Promote(doubleExpression, typeof(decimal), true, true);

    Assert.NotNull(promotedExpression);
    Assert.Equal(typeof(decimal), promotedExpression.Type);
}

Example 2:

[Fact]
public void promoteDoubleToDecimal()
{
    // Assign
    var parsingConfig = new ParsingConfig();
    var numberParser = new NumberParser(parsingConfig);
    var promoter = new ExpressionPromoter(parsingConfig);

    var doubleExpression = numberParser.ParseRealLiteral("123.456d", 'd', true);

    // Act & Assert
    var promotedExpression = promoter.Promote(doubleExpression, typeof(decimal), true, true);

    Assert.NotNull(promotedExpression);
    Assert.Equal(typeof(decimal), promotedExpression.Type);
}

Expected Behavior: The Promote method should correctly handle the qualifier and convert the numeric literal to the specified target type.

Actual Behavior: The promotion fails, and the conversion does not occur as expected due to the incorrect handling of the qualifier in the ConstantExpressionHelper.

Additional Information: This issue affects applications that rely on dynamic LINQ queries where numeric-type promotions with qualifiers are necessary.

Proposed Solution: Ensure that the CreateLiteral and TryGetText methods in ConstantExpressionHelper correctly handle qualifiers when creating and retrieving constant expressions.

asulwer commented 4 months ago

d is for double m is for decimal C# Annotated Standard

asulwer commented 4 months ago

you can change the d to an m here var doubleExpression = numberParser.ParseRealLiteral("123.456d", 'd', true); or you can change decimal to double here var promotedExpression = promoter.Promote(doubleExpression, typeof(decimal), true, true);

either change solves the issue. use c# to cast

RenanCarlosPereira commented 4 months ago

But the problem is the cache, once you do it it will store in the Literals dictionary, and you wont be able to change it back

asulwer commented 4 months ago

But the problem is the cache, once you do it it will store in the Literals dictionary, and you wont be able to change it back

sounds like a new issue? i do not see this mentioned in the OP.

RenanCarlosPereira commented 4 months ago

yes, it is a bug, to reproduce it in the rules-engine we need to run this test:

The first expression will cause an exception, which we expect because of the type. but the second expression will also fail because the first one cached it in the Literals dictionary in the lib

[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 thow as its vald
    const string expression2 = "Board.NumberOfMembers = 0.2";
    var result2 = parser.Evaluate<bool>(expression2, new[] { parameter });
    result2.Should().BeFalse();
}
asulwer commented 4 months ago

changing this const string expression2 = "Board.NumberOfMembers = 0.2"; to this const string expression2 = "Board.NumberOfMembers = 0.2m";

solves this issue, as presented.

RenanCarlosPereira commented 4 months ago

changing this const string expression2 = "Board.NumberOfMembers = 0.2"; to this const string expression2 = "Board.NumberOfMembers = 0.2m";

solves this issue, as presented.

Yes, but we cant use that as a solution, The project rules-engine relays in a dynamic rule builder, we only know expressions in a runtime process, with that in mind, we cant control the order that the expressions will be executed.

I also cant force the customer to include the type (literlas d,m and f) in all the expressions they may have…

The fix is already in place in master, we just need to wait for a release

asulwer commented 4 months ago

i proposed this as a solution due to the missing literal notation in the example provided. c# does not interpret 2.0 as a decimal, 2.0m is interpreted as a decimal. this is a c# language failure.

i see that System.Linq.Dynamic.Core has a solution and i will update my fork asap.

Note: i have included a test for this scenario to verify the solution.