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

Expression parameter should be case sensitive #406

Closed
joshidp opened this issue Aug 7, 2020 · 5 comments
Closed

Expression parameter should be case sensitive #406

joshidp opened this issue Aug 7, 2020 · 5 comments
Assignees

Comments

@joshidp
Copy link

joshidp commented Aug 7, 2020

Hi,

Thanks for the great work!

I'm seeing one issue where I'm calling DynamicInvoke method with multiple parameters and one parameter is bool and another is a list of someclass now if the class has a prop which has same name as the bool parameter then DynamicInvoke is considering bool parameter instead of the prop inside the class list parameter. The only diff between them is of case. Please see the example below.

class DataClass
{
	public bool IsTrue;
}

bool isTrue = true;
var abc = new List<DataClass>() { new DataClass() { IsTrue = false } };
var x = new List<ParameterExpression>();
x.Add(Expression.Parameter(isTrue.GetType(), "isTrue"));
x.Add(Expression.Parameter(abc.GetType(), "abc"));

var config = new ParsingConfig();
string query = "abc.Where(IsTrue == true)";
var e = DynamicExpressionParser.ParseLambda(config, x.ToArray(), null, query);
Delegate del = e.Compile();
var result = del.DynamicInvoke(isTrue, abc);

As you can see in the above example there is one isTrue variable and one property in DataClass which is called IsTrue, please note the case difference of "i". The query DyanmicInvoke should be independent of the variable x named as isTrue in the parameter set because it's no where used in the query, but that's not the case if I set isTrue=true result has records and when I change isTure to false then there are no records in the result and there is no change in result if I modify the value of IsTrue property of abc list var. It means DynamicInvoke is not checking the casing of the fields.

Expected result:

  • parameter match should be case-sensitive.

  • if no parameter matches with case sensitivity then error should be thrown.

It's counter intuitive when we come from c# world and use LINQ like expressions.

Please share your thoughts why it's not working as expected.

Thanks

@JonathanMagnan JonathanMagnan self-assigned this Aug 7, 2020
@JonathanMagnan
Copy link
Member

Hello @joshidp ,

By default, I believe case insensitive should be the default behavior as it's now. Keep in mind a lot of people are building their dynamic string through some external source so the casing might not always be good.

However, we should have an option as well to force at least parameters to be case sensitive to support your scenario.

We will look at it.

Best Regards,

Jon


Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework ExtensionsEntity Framework ClassicBulk OperationsDapper Plus

Runtime Evaluation
Eval.Execute("x + y", new {x = 1, y = 2}); // return 3
C# Eval FunctionSQL Eval Function

@joshidp
Copy link
Author

joshidp commented Aug 8, 2020

Thanks for the reply.

JSON is case sensitive and LINQ is case sensitive so I feel default option for parameters in this lib also should be case sensitive, it protect us from getting incorrect results which can go unchecked as shown in my example.

Anyways as long as we have an option to set the casing requirement it should be ok.

Eagerly waiting for your PR/release to provide case sensitivity option.

Thanks

@JonathanMagnan
Copy link
Member

Hello @joshidp ,

The v1.2.2 has been released.

We added the option config.IsCaseSensitive = true. When enabled, the library will check for casing as well.

Due to the high popularity of this library and how important this behavior could impact existing code, making it true by default would have been way too big breaking changes.

Let me know if everything now works as expected with this option.

Best Regards,

Jon

@joshidp
Copy link
Author

joshidp commented Aug 19, 2020

It's working as expected, thanks for the fix 👍

@JonathanMagnan
Copy link
Member

Awesome @joshidp

don't hesitate to contact us for any questions, issues or feedback!

Best regards,

Jon

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

No branches or pull requests

2 participants