Skip to content
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

Incorrect Handling of Qualifiers in ConstantExpressionHelper #821

Closed
RenanCarlosPereira opened this issue Jun 20, 2024 · 8 comments
Closed
Assignees
Labels

Comments

@RenanCarlosPereira
Copy link
Contributor

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
Copy link

asulwer commented Jun 23, 2024

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

@asulwer
Copy link

asulwer commented Jun 23, 2024

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
Copy link
Contributor Author

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
Copy link

asulwer commented Jun 23, 2024

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
Copy link
Contributor Author

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();
}

@StefH StefH changed the title Bug: Incorrect Handling of Qualifiers in ConstantExpressionHelper Incorrect Handling of Qualifiers in ConstantExpressionHelper Jun 25, 2024
@StefH StefH self-assigned this Jun 25, 2024
@StefH StefH added the bug label Jun 25, 2024
@StefH StefH closed this as completed Jun 25, 2024
@asulwer
Copy link

asulwer commented Jun 25, 2024

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

solves this issue, as presented.

@RenanCarlosPereira
Copy link
Contributor Author

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
Copy link

asulwer commented Jun 25, 2024

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.

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

No branches or pull requests

3 participants