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

bind expressions to correct constructor parameter #557

Merged
merged 5 commits into from
Dec 17, 2021

Conversation

jonasdaniels
Copy link
Contributor

@jonasdaniels jonasdaniels commented Nov 24, 2021

class test {
  int a;
  int b;
  int c; 
}
if ConstructorInfo ci.GetParameters() returns ParameterInfo[] {int a, int b, int c }

Picking them up by index works in most cases.
ex.
new(1 as a, 2 as b, 3 as c ) => {a = 1, b = 2, c = 3 }
but not if the identifiers are moved around(for example using the same type again)
new(1 as b, 2 as a, 3 as c ) => { a =1, b = 2, c = 3 }

Proposed solution.
Make sure to map the correct expression based the identifier to the same(based on type and name) constructor parameter

Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take a look at my comments

@StefH
Copy link
Collaborator

StefH commented Nov 26, 2021

@jonasdaniels

Can you double check if all tests run fine?

At my system, I get this test error:

 System.Linq.Dynamic.Core.Tests.QueryableTests.Select_Dynamic_IntoTypeWithNullableProperties2
   Source: QueryableTests.Select.cs line 253
   Duration: 26 ms

  Message: 
System.InvalidOperationException : Sequence contains no matching element

  Stack Trace: 
ThrowHelper.ThrowNoMatchException()
Enumerable.First[TSource](IEnumerable`1 source, Func`2 predicate)
ExpressionParser.CreateNewExpression(List`1 properties, List`1 expressions, Type newType) line 1480
ExpressionParser.ParseNew() line 1408
ExpressionParser.ParseIdentifier() line 991
ExpressionParser.ParsePrimaryStart() line 772
ExpressionParser.ParsePrimary() line 741
ExpressionParser.ParseUnary() line 736
ExpressionParser.ParseMultiplicative() line 682
ExpressionParser.ParseAdditive() line 651
<5 more frames...>
ExpressionParser.ParseOrOperator() line 265
ExpressionParser.ParseLambdaOperator() line 245
ExpressionParser.ParseNullCoalescingOperator() line 232
ExpressionParser.ParseConditionalOperator() line 216
ExpressionParser.Parse(Type resultType, Boolean createParameterCtor) line 154
DynamicExpressionParser.ParseLambda(Type delegateType, ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values) line 122
DynamicExpressionParser.ParseLambda(ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values) line 97
DynamicExpressionParser.ParseLambda(ParsingConfig parsingConfig, Boolean createParameterCtor, Type itType, Type resultType, String expression, Object[] values) line 355
DynamicQueryableExtensions.Select[TResult](IQueryable source, ParsingConfig config, String selector, Object[] args) line 1803
QueryableTests.Select_Dynamic_IntoTypeWithNullableProperties2() line 264

@jonasdaniels jonasdaniels requested a review from StefH December 2, 2021 17:51
Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you have updated the unit-tests, but actually I was expecting that you would fix the code, because the unit-test did work before, and ideally the unit-test should continue working, also with your new code.

Sop can you please take a look at the code and see if you can fix the code so that the unit-test can stay the same?

@jonasdaniels
Copy link
Contributor Author

@StefH
Reverted the test changes and added code to sequentially bind parameters.

However it feels unintuitiv to bind to the field names instead of the parameters names.

In the case:
QueryableTests.Select.cs line 265: "new (it as Time, DayOfWeek as DOWNull, DayOfWeek as DOW, Second as Sec, int?(Second) as SecNull)");"
it might make more sence to bind towards the fields since we dont have contructor with these names?

@StefH StefH merged commit 73d04db into zzzprojects:master Dec 17, 2021
@StefH StefH added the feature label Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants