search

AutoMapper / AutoMapper.Extensions.OData

Creates LINQ expressions from ODataQueryOptions and executes the query.
MIT License
141 stars 39 forks source link

multiple orderby throws exception #3

Closed xaviervv closed 4 years ago

xaviervv commented 4 years ago

Opened on SO as well: stack overflow

When trying to execute something like /odata/roles?$orderby=IsActive,Name I get the below exception.

"No generic method 'ThenBy' on type 'System.Linq.Queryable' is compatible with the supplied type arguments and arguments. No type arguments should be provided if the method is non-generic. "

"System.InvalidOperationException: No generic method 'ThenBy' on type 'System.Linq.Queryable' is compatible with the supplied type arguments and arguments. No type arguments should be provided if the method is non-generic. at System.Linq.Expressions.Expression.FindMethod(Type type, String methodName, Type[] typeArgs, Expression[] args, BindingFlags flags) at System.Linq.Expressions.Expression.Call(Type type, String methodName, Type[] typeArguments, Expression[] arguments) at AutoMapper.AspNet.OData.LinqExtensions.GetOrderByMethod[T](ODataQueryOptions1 options, Expression expression) at AutoMapper.AspNet.OData.LinqExtensions.GetQueryableExpression[T](ODataQueryOptions1 options) at AutoMapper.AspNet.OData.QueryableExtensions.GetAsync[TModel,TData](IQueryable1 query, IMapper mapper, ODataQueryOptions1 options, HandleNullPropagationOption handleNullPropagation) at Mpi.PlanningApp.Web.Features.Roles.RolesController.Get(ODataQueryOptions1 options) in C:\Users\A330429\Source\Repos\VVC\Planning App\src\Mpi.PlanningApp.Web\Features\Roles\RolesController.cs:line 52 at Microsoft.AspNetCore.Mvc.Internal.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments) at System.Threading.Tasks.ValueTask1.get_Result() at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeActionMethodAsync() at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeNextActionFilterAsync() at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Rethrow(ActionExecutedContext context) at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync() at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext) at Hellang.Middleware.ProblemDetails.ProblemDetailsMiddleware.Invoke(HttpContext context)"

Posted more code examples on stack overflow. If it's needed, I can copy it to here.

BlaiseD commented 4 years ago

Need a sample we can run. If you can't post it here then add as a new repo.

Does the error occur with the latest expression mapping package? (installed to your web project)

xaviervv commented 4 years ago

To be sure I updated the package to the lastest version, by manually installing it, and also updated the odata microsoft package (one version up) but still the same issue.

I was planning on updating to .net core 3.1 as a test as well, but once I did that, I could not even find the .net core package of this library, gues it's not supported yet?

I will trim my project and post it soon

xaviervv commented 4 years ago

Ok, I made a repro sample, in a completely new project. I left out all of the layers, feature folder layout, mediatr, error handling and other plugins.

Change the connectionstring in appsettings.development and the migration should run automatically and create a couple of records to be able to execute the query.

Once started call: https://localhost:5001/odata/roles?$orderby=Description,Name

I Kept 3 classes for many to many to follow the identityDbContext setup

$expand call isn't currently working, while it is on my own project, but that is for another day or time.

xaviervv commented 4 years ago

I linked your library so I could debug it locally. LinqExtensions.GetOrderByMethod: mce = Expression.Call(typeof(Queryable), mce.Method.Name, new Type[] { typeof(T), mce.Arguments[1].GetReturnType() }, expression, mce.Arguments[1]); with $orderby=Name => mce.Method.Name = OrderBy with $orderby=Name,Description => mce.Method.Name = ThenBy

There is a missing OrderBy for the first property (i think) and then the remaining error for the 'No generic method' part

xaviervv commented 4 years ago

@BlaiseD I found a solution, but it needs to be tested properly, I only tested it for 2 columns ordering, but it needs to be tested in combination with all other operands. I'll see if I can do this tonight.

` ///

/// Get OrderBy Method /// /// /// /// /// public static MethodCallExpression GetOrderByMethod(this ODataQueryOptions options, Expression expression) { if (options.OrderBy == null && options.Top == null) return null;

        if (options.OrderBy == null)
        {
            return Expression.Call
                (
                    typeof(Queryable),
                    "Take",
                    new[] { typeof(T) }, expression, Expression.Constant(options.Top.Value)
                );
        }

        IQueryable queryable = Enumerable.Empty<T>().AsQueryable();
        queryable = options.OrderBy.ApplyTo(queryable, new ODataQuerySettings());
        MethodCallExpression mce = (MethodCallExpression)queryable.Expression;

        ParameterExpression arg = Expression.Parameter(typeof(T), "x");

        var first = true;
        foreach (var node in options.OrderBy.OrderByNodes)
        {
            PropertyInfo pi = typeof(T).GetProperty(((OrderByPropertyNode)node).Property.Name, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance);
            Expression propertyExpr = Expression.Property(arg, pi.Name);
            LambdaExpression lambdaExp = Expression.Lambda(propertyExpr, arg);

            string methodName;

            if (first)
            {
                first = false;

                methodName = node.Direction == OrderByDirection.Descending ? "OrderByDescending" : "OrderBy";
                mce = Expression.Call(typeof(Queryable), methodName, new Type[] { typeof(T), pi.PropertyType }, expression, Expression.Quote(lambdaExp));
            }
            else
            {
                methodName = node.Direction == OrderByDirection.Descending ? "ThenByDescending" : "ThenBy";
                mce = Expression.Call(typeof(Queryable), methodName, new Type[] { typeof(T), pi.PropertyType }, mce, Expression.Quote(lambdaExp));
            }

            // mce = Expression.Call(typeof(Queryable), methodName, new Type[] { typeof(T), pi.PropertyType }, mce, Expression.Quote(lambdaExp));

        }

        //mce = Expression.Call(typeof(Queryable), mce.Method.Name, new Type[] { typeof(T), mce.Arguments[1].GetReturnType() }, expression, mce.Arguments[1]);

        if (options.Skip != null)
            mce = Expression.Call(typeof(Queryable), "Skip", new[] { typeof(T) }, mce, Expression.Constant(options.Skip.Value));
        if (options.Top != null)
            mce = Expression.Call(typeof(Queryable), "Take", new[] { typeof(T) }, mce, Expression.Constant(options.Top.Value));

        return mce;
    }`
BlaiseD commented 4 years ago

This is nice. If you plan to create a PR consider shorter methods like below.

You won't need the appyTo (you're creating the whole lambda expression from scratch). Also no need for ParameterExpression arg. That already lives in GetQueryableExpression<T>. I don't know if OData supports sorting on child properties of references but hence the GetMemberInfoFromFullName below.

        public static MethodCallExpression GetOrderByMethod<T>(this ODataQueryOptions<T> options, Expression expression)
        {
            if (options.OrderBy == null && options.Top == null)
                return null;

            if (options.OrderBy == null)
            {
                return Expression.Call
                (
                    typeof(Queryable),
                    "Take",
                    new[] { typeof(T) }, expression, Expression.Constant(options.Top.Value)
                );
            }

            //IQueryable queryable = Enumerable.Empty<T>().AsQueryable();
            //queryable = options.OrderBy.ApplyTo(queryable, new ODataQuerySettings());
            //MethodCallExpression mce = (MethodCallExpression)queryable.Expression;

            //mce = Expression.Call(typeof(Queryable), mce.Method.Name, new Type[] { typeof(T), mce.Arguments[1].GetReturnType() }, expression, mce.Arguments[1]);

            //if (options.Skip != null)
            //    mce = Expression.Call(typeof(Queryable), "Skip", new[] { typeof(T) }, mce, Expression.Constant(options.Skip.Value));
            //if (options.Top != null)
            //    mce = Expression.Call(typeof(Queryable), "Take", new[] { typeof(T) }, mce, Expression.Constant(options.Top.Value));

            //return mce;

            return options.OrderBy.OrderByNodes.Aggregate(null, (MethodCallExpression mce, OrderByNode orderByNode) =>
            {
                OrderByPropertyNode propertyNode = (OrderByPropertyNode)orderByNode;
                return mce == null
                    ? expression.GetOrderByCall(propertyNode.Property.Name, orderByNode.Direction)
                    : mce.GetThenByCall(propertyNode.Property.Name, orderByNode.Direction);
            })
            .GetSkipCall(options.Skip)
            .GetTakeCall(options.Top);
        }

        public static MethodCallExpression GetSkipCall(this MethodCallExpression expression, SkipQueryOption skip)
        {
            if (skip == null) return expression;

            return Expression.Call
            (
                typeof(Queryable),
                "Skip",
                new[] { expression.GetUnderlyingElementType() },
                expression,
                Expression.Constant(skip.Value)
            );
        }

        public static MethodCallExpression GetTakeCall(this MethodCallExpression expression, TopQueryOption top)
        {
            if (top == null) return expression;

            return Expression.Call
            (
                typeof(Queryable),
                "Take",
                new[] { expression.GetUnderlyingElementType() },
                expression,
                Expression.Constant(top.Value)
            );
        }

        public static MethodCallExpression GetOrderByCall(this Expression expression, string memberFullName, OrderByDirection sortDirection, string selectorParameterName = "a")
        {
            Type sourceType = expression.GetUnderlyingElementType();
            MemberInfo memberInfo = sourceType.GetMemberInfoFromFullName(memberFullName);
            return Expression.Call
            (
                typeof(Queryable),
                sortDirection == OrderByDirection.Ascending ? "OrderBy" : "OrderByDescending",
                new Type[] { sourceType, memberInfo.GetMemberType() },
                expression,
                memberFullName.GetTypedSelector(sourceType, selectorParameterName)
            );
        }

        public static MethodCallExpression GetThenByCall(this MethodCallExpression expression, string memberFullName, OrderByDirection sortDirection, string selectorParameterName = "a")
        {
            Type sourceType = expression.GetUnderlyingElementType();
            MemberInfo memberInfo = sourceType.GetMemberInfoFromFullName(memberFullName);
            return Expression.Call
            (
                typeof(Queryable),
                sortDirection == OrderByDirection.Ascending ? "ThenBy" : "ThenByDescending",
                new Type[] { sourceType, memberInfo.GetMemberType() },
                expression,
                memberFullName.GetTypedSelector(sourceType, selectorParameterName)
            );
        }

        public static Type GetUnderlyingElementType(this Expression expression)
            => expression.Type.GetUnderlyingElementType();

        public static MemberInfo GetMemberInfoFromFullName(this Type type, string propertyFullName)
        {
            if (propertyFullName.IndexOf('.') < 0)
            {
                return type.GetMemberInfo(propertyFullName);
            }

            string propertyName = propertyFullName.Substring(0, propertyFullName.IndexOf('.'));
            string childFullName = propertyFullName.Substring(propertyFullName.IndexOf('.') + 1);

            return GetMemberInfoFromFullName(type.GetMemberInfo(propertyName).GetMemberType(), childFullName);
        }
xaviervv commented 4 years ago

Did everything quick and dirty as a test, will refactor the code as you suggested and test it with multiple filters, orderby, select statements, ...

May not be before the weekend, but I'll try.

BlaiseD commented 4 years ago

Sounds good - no hurry.

xaviervv commented 4 years ago

I replaced the method I had, with the multiple methods from you.

Quickly tested, here are the results.

There are too many other issues, or maybe it's just me, but I can't fix them all. When I looked into this plugin, I originally thought only the order by would be an issue, but it doesn't seem that way; So I will have to find another way than to use this library I think. If it's a me issue, please say so, and I will look further.

I haven't tested sub querying on expand as I have to update my model first (not a navigation property)

Client Side Any combination of $filter, $orderby (descending or not, multiple columns or single), $select, and $expand is working pure client side, as a result.

Working example: /odata/roles?$filter=IsActive&$orderby=Name,Description%20desc&$select=Name,Description

DB Select On the DB side: the properties from DTO are all returned, so it doesn't take into account that I only want one or two properties on the DB side.

Paging url: odata/roles?$orderby=Name,Description&$skip=1&$top=1 SQL being generated: SELECT [a].[Id], [a].[Description], [a].[Name] FROM [identity].[Roles] AS [a] ORDER BY [a].[Name], [a].[Description] OFFSET @__p_0 ROWS FETCH NEXT @__p_0 ROWS ONLY when tested locally in SMMS as: SELECT [a].[Id], [a].[Description], [a].[Name] FROM [identity].[Roles] AS [a] ORDER BY [a].[Name], [a].[Description] OFFSET 1 ROWS FETCH NEXT 1 ROWS ONLY I get to see results OData response doesn't show any results

Order Correctly translated to db

Filtering Correctly translated to db

I may will investigate further in the weekend, but I'm on a deadline, and I already spend too much time on trying to fix this. Easiest fix right now is to simply give up on automapper for my odata calls and return my entities.

BlaiseD commented 4 years ago

Had some ideas on the select in the past but no solution yet. Will look at paging - thought it was working before.

xaviervv commented 4 years ago

At one point I simply took the iqueryable before all of the expression filtering and manipulations in getasync as is. It simply added a select around the filtered ordered by query with paging mixed all over the place. And it used $it.property every where. Was a last ditch efford.

Can look at it again this weekend, outside of the clients' time.

Get Outlook for Androidhttps://aka.ms/ghei36

From: Blaise Taylor notifications@github.com Sent: Thursday, January 30, 2020 10:46:21 PM To: AutoMapper/AutoMapper.Extensions.OData AutoMapper.Extensions.OData@noreply.github.com Cc: Xavier tuffen_91@hotmail.com; Author author@noreply.github.com Subject: Re: [AutoMapper/AutoMapper.Extensions.OData] multiple orderby throws exception (#3)

Had some ideas on the select in the pasthttps://github.com/BlaiseD/LogicBuilder.OData/issues/1 but no solution yet. Will look at paging - thought it was working before.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/AutoMapper/AutoMapper.Extensions.OData/issues/3?email_source=notifications&email_token=AIMW3QW22GWN6GOCZTXYFUTRANDC3A5CNFSM4KMU3IWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKMVVGI#issuecomment-580475545, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIMW3QQCV6FP36VF3MGDSF3RANDC3ANCNFSM4KMU3IWA.

BlaiseD commented 4 years ago

The problem with paging is that we handle paging by generating the Queryable.Skip().Take() expression but OData also queries the result i.e. you have two records in the database. Our query skips the first one and OData's EnableQuery skips the second one. Remove [EnableQuery()] from your controller method for paging to work. I'll create 3 separate issues for:

We should only address the ThenBy() bug in this issue.

xaviervv commented 4 years ago

Sounds good. I will create a pr with the current fix tonight with the smaller methods you provided. I tested that, and works as it should now.

Get Outlook for Androidhttps://aka.ms/ghei36

From: Blaise Taylor notifications@github.com Sent: Friday, January 31, 2020 11:42:05 AM To: AutoMapper/AutoMapper.Extensions.OData AutoMapper.Extensions.OData@noreply.github.com Cc: Xavier tuffen_91@hotmail.com; Author author@noreply.github.com Subject: Re: [AutoMapper/AutoMapper.Extensions.OData] multiple orderby throws exception (#3)

The problem with paging is that we handle paging by generating the Queryable.Skip().Take() expression but OData also queries the result i.e. you have two records in the database. Our query skips the first one and OData's EnableQuery skips the second one. Remove [EnableQuery()] from your controller method for paging to work. I'll create 3 separate issues for:

We'll only address the ThenBy() bug in this issue.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/AutoMapper/AutoMapper.Extensions.OData/issues/3?email_source=notifications&email_token=AIMW3QV5EHPTFKXHGGWD5ZLRAP573A5CNFSM4KMU3IWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKOIERA#issuecomment-580682308, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIMW3QTQBJTU5WSKQUVK4XLRAP573ANCNFSM4KMU3IWA.

xaviervv commented 4 years ago

Small update: you were right. I removed the EnableQuery attribute, and everything works for paging, except for the count. I already found the code to apply, somewhere, but will open a 2nd issue with the line to fix

I'm not sure why I even kept the attribute. Now that I think about it, it doesn't make much sense. The whole query is already being applied on ef core, it shouldn't be applied again.

I will live with the select for now, it's only a really minor inconvenience for now, and the data sets won't be that huge in the beginning; Especially with all the filtering being applied.

xaviervv commented 4 years ago

Ok, I created 3 PRs to handle expand, count, orderby

I merged everything together locally, and seems to be working. But you may want to make the code prettier in order to match the current style of your project.

Take your time with that. I will use my local branch for the time being, until the 3 fixes are available.

I have no time left to check out select, so I won't tackle that.

I mentioned something regarding updating docs, in regard to not using DataOptions with EnableQueryAttribute, but I will leave that, and the descission, up to you.

Everything we need is working now; Thanks for your feedback.

I will still keep an eye on here.

Br

Xavier