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

Context lost in object initializer #294

Closed
david-garcia-garcia opened this issue Aug 24, 2019 · 6 comments
Closed

Context lost in object initializer #294

david-garcia-garcia opened this issue Aug 24, 2019 · 6 comments
Labels

Comments

@david-garcia-garcia
Copy link
Contributor

david-garcia-garcia commented Aug 24, 2019

After parsing firs argument in object initializer, context is lost:

 var users = new[]
            {
                new { name = "Juan", age = 25 },
                new { name = "Juan", age = 25 },
                new { name = "David", age = 12 },
                new { name = "Juan", age = 25 },
                new { name = "Juan", age = 4 },
                new { name = "Pedro", age = 2 },
                new { name = "Juan", age = 25 }
            }.ToList();

IQueryable query;
string results;

// This works
query = users.AsQueryable();
query = query.GroupBy(CustomParsingConfig.ParsingConfig, "new(name as name)", "it");
query = query.Select("new (it.Key as Key, new(it.Sum(x => x.age) as ageSum) as nativeAggregates, it as Grouping)");
results = JsonConvert.SerializeObject(query);

// This does not
query = users.AsQueryable();
query = query.GroupBy(CustomParsingConfig.ParsingConfig, "new(name as name)", "it");
query = query.Select("new (it.Key as Key, new(it.Sum(x => x.age) as ageSum, it.Sum(x => x.age) as ageSum2) as nativeAggregates, it as Grouping)");
results = JsonConvert.SerializeObject(query);
@StefH StefH added the bug label Aug 24, 2019
@david-garcia-garcia
Copy link
Contributor Author

david-garcia-garcia commented Aug 24, 2019

There seems to be several factors leading to this fail.

One of them is ExpressionPromoter no being able to promote Lambdas, I had to add this to an overriden ExpressionPromoter:

            if (expr is LambdaExpression le)
            {
                if (((LambdaExpression)le).ReturnType == type)
                {
                    return expr;
                }

                if (le.ReturnType == type.GetUnderlyingType() && type.IsNullable())
                {
                    // Boxing
                    var boxed = Expression.Convert(le.Body, type);

                    // Use Expression.Lambda to get back to strong typing
                    // TODO: We should make this workd with Func<,,>, Func<,,,> etc..., but as the library has no handling of multidmensional argument lambdas so there's no point in doing so now...
                    var delegateType = typeof(Func<,>).MakeGenericType(le.Parameters[0].Type, type);
                    return Expression.Lambda(delegateType, boxed, le.Parameters);
                }
            }

I can't make this a PR now because...

  • GetUnderlyingType and IsNullable are not from this library, but could be added.
  • ReturnType is not available in .Net framework 3.5 or below, I'm not sure how to handle that as the library aims at being compatible to >= 3.5

@StefH
Copy link
Collaborator

StefH commented Aug 24, 2019

  1. GetUnderlyingType
  2. IsNullable

Are both defined in
https://github.com/StefH/System.Linq.Dynamic.Core/blob/master/src/System.Linq.Dynamic.Core/Parser/TypeHelper.cs

ReturnType : Isn't there another property you could use?

@StefH
Copy link
Collaborator

StefH commented Aug 24, 2019

I think the replacement for ReturnType is lambdaExpression.Body.Type:

// ReSharper disable once CheckNamespace
namespace System.Linq.Expressions
{
    internal static class LambdaExpressionExtensions
    {
        public static Type GetReturnType(this LambdaExpression lambdaExpression)
        {
#if !NET35
            return lambdaExpression.ReturnType;
#else
            return lambdaExpression.Body.Type;
#endif
        }
    }
}

I'm currently adding this here:
https://github.com/StefH/System.Linq.Dynamic.Core/blob/AverageAsync/src/System.Linq.Dynamic.Core/Compatibility/LambdaExpressionExtensions.cs

@david-garcia-garcia
Copy link
Contributor Author

Thanks! I am currently working in a workaround for the issue in our project, will deal with the root cause when time permits, I've been reading through the code and have an idea on how to restructure the code so we can get rid of the custom aggregate parsing logic in favor of a generic handling of method calls (including extension methods).

Having that there will help for sure.

@david-garcia-garcia
Copy link
Contributor Author

Sorry I got confused and mixed things (all these issues are very related to the fact that lambda/method call parsing is very ad-hoc and harcdoded to very specific cases).

The initial issue reported here was that an expression could not contain more than one lambda with the same lambda argument.

The reason seems to be that the whole library is single-lambda oriented, and that is a big handicap. After a lambda has been parsed, the context was not being restored for the rest of the parsing.

This PR shows the failure:

#296

This PR has the fix:

#297

StefH pushed a commit that referenced this issue Aug 29, 2019
* only test

* fix

* xx

* xx
@StefH
Copy link
Collaborator

StefH commented Aug 29, 2019

Thanks for reporting and fixing.

Closing this one...

@StefH StefH closed this as completed Aug 29, 2019
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

2 participants