OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
853 stars 476 forks source link

GroupByWrapper should implement IComparable #1969

Open badcommandorfilename opened 4 years ago

badcommandorfilename commented 4 years ago

The query: /odata/Donations?$apply=groupby((email), aggregate(amount with sum as Total))

Will throw an ArgumentException "(Failed to compare two elements in the array.)" -> "At least one object must implement IComparable."

The output result type of the $apply is an IEnumerable<GroupByWrapper> , and apparently the serializer really needs to sort this before returning the result (even though I don't care and intentionally didn't specify a sort order). The default Compare function is used...

//System.Collections.Comparer.cs
public int Compare(Object a, Object b)

FWIW, specifying an orderby doesn't solve the issue (the sort operation is being applied to GroupByWrappers, not the output EdmType).

/odata/Donations?$apply=groupby((email), aggregate(amount%20with%20sum%20as%20Total))&$orderby=Total

Assemblies affected

PackageReference Include="Microsoft.AspNetCore.OData" Version="7.1.0"

Reproduce steps

  1. Example Model Class:
public class
{
    public string Email { get; set; }
    public decimal Amount {get; set; }
}
  1. Any result that generates an IEnumerable<GroupByWrapper> (also any other kind of DynamicTypeWrapper) e.g. ?$apply=groupby((email), aggregate(amount with sum as Total))

Expected result

[
    {
        "email": "new@test.org",
        "Total": 11.00000
    },
    {
        "email": "test@test.biz",
        "Total": 17.00000
    },
    {
        "email": "test@token.biz",
        "Total": 82.00000
    },
    {
        "email": "test@test.org",
        "Total": 538.00000
    }
]

Actual result

System.Collections.Comparer.cs throws: ArgumentException "(Failed to compare two elements in the array.)" -> "At least one object must implement IComparable."

Additional detail

Stacktrace for fun:

at System.Collections.Generic.GenericArraySortHelper1.Sort(T[] keys, Int32 index, Int32 length, IComparer1 comparer) in E:\A\_work\482\s\src\mscorlib\src\System\Collections\Generic\ArraySortHelper.cs:line 399 at System.Array.Sort[T](T[] array, Int32 index, Int32 length, IComparer1 comparer) in E:\A_work\482\s\src\mscorlib\src\System\Array.cs:line 1815 at System.Linq.EnumerableSorter1.Sort(TElement[] elements, Int32 count) in E:\A\_work\115\s\corefx\src\System.Linq\src\System\Linq\OrderedEnumerable.cs:line 512 at System.Linq.OrderedEnumerable1.d3.MoveNext() in E:\A_work\115\s\corefx\src\System.Linq\src\System\Linq\OrderedEnumerable.cs:line 24 at System.Linq.Lookup2.Create[TSource](IEnumerable1 source, Func2 keySelector, Func2 elementSelector, IEqualityComparer1 comparer) in E:\A\_work\115\s\corefx\src\System.Linq\src\System\Linq\Lookup.cs:line 80 at System.Linq.GroupedEnumerable3.ToArray() in E:\A_work\115\s\corefx\src\System.Linq\src\System\Linq\Grouping.cs:line 227 at System.Linq.Buffer1..ctor(IEnumerable1 source) in E:\A_work\115\s\corefx\src\System.Linq\src\System\Linq\Buffer.cs:line 33 at System.Linq.OrderedEnumerable`1.d3.MoveNext() in E:\A_work\115\s\corefx\src\System.Linq\src\System\Linq\OrderedEnumerable.cs:line 30 at System.Linq.Enumerable.SelectIPartitionIterator2.MoveNext() in E:\A\_work\115\s\corefx\src\System.Linq\src\System\Linq\Select.cs:line 639 at Microsoft.EntityFrameworkCore.Query.Internal.LinqOperatorProvider.ExceptionInterceptor1.EnumeratorExceptionInterceptor.MoveNext() in //src/EFCore/Query/Internal/LinqOperatorProvider.cs:line 143 at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeList(JsonWriter writer, IEnumerable values, JsonArrayContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) in //Src/Newtonsoft.Json/Serialization/JsonSerializerInternalWriter.cs:line 677 at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type objectType) in //Src/Newtonsoft.Json/Serialization/JsonSerializerInternalWriter.cs:line 80 at Newtonsoft.Json.JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type objectType) in //Src/Newtonsoft.Json/JsonSerializer.cs:line 1151 at Microsoft.AspNetCore.Mvc.Formatters.JsonOutputFormatter.WriteObject(TextWriter writer, Object value) in /_/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonOutputFormatter.cs:line 91 at Microsoft.AspNetCore.Mvc.Formatters.JsonOutputFormatter.d11.MoveNext() in /_/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonOutputFormatter.cs:line 153 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in E:\A_work\482\s\src\mscorlib\src\System\Runtime\ExceptionServices\ExceptionDispatchInfo.cs:line 132 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) in E:\A_work\482\s\src\mscorlib\src\System\Runtime\CompilerServices\TaskAwaiter.cs:line 155 at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.d20.MoveNext() in /_/src/Mvc/Mvc.Core/src/Internal/ResourceInvoker.cs:line 138`

badcommandorfilename commented 4 years ago

Just FYI, I can submit a pull request to fix this if a maintainer is happy with the proposal to have "unsorted" be the default for aggregate responses.

My suggested patch would be something like:

namespace Microsoft.AspNet.OData.Query.Expressions
{
    /// <summary>
    /// Represents a container class that contains properties that are grouped by using $apply.
    /// </summary>
-    public abstract class DynamicTypeWrapper
+    public abstract class DynamicTypeWrapper : IComparable
    {
        /// <summary>
        /// Gets values stored in the wrapper
        /// </summary>
        public abstract Dictionary<string, object> Values { get; }

+        /// <summary>
+        /// IComparable implementation for sorting the output order of the response
+        /// Default implementation is unsorted
+        /// </summary>
+        /// <param name="obj">Comparison object for sorting</param>
+        /// <returns></returns>
+        public virtual int CompareTo(object obj)
+        {
+            return 0;
+        }

This will fix the thrown exception and return a usable response.

However, it doesn't move any closer to permitting user-defined sorting like: /odata/Donations?$apply=groupby((email), aggregate(amount with sum as Total))&orderby=Total - if this is already on the roadmap or something I don't want to duplicate work.

xuzhg commented 4 years ago

@badcommandorfilename We'd love to see your contribution. @kosinsky Would you please take a look?

kosinsky commented 4 years ago

It's weird that we have that exception. We are generating OrderBy statement that looks like OrderBy(x=>x.email).ThenBy(x=>x.Total) when no $orderby specified to provide stable order

And if order by specified we are sorting by provided properties. Unit test that verifies that (one of many): https://github.com/OData/WebApi/blob/5061dcad757599b93ff990079832123a73e0e091/test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationTests.cs#L168

As a result we shouldn't have sorts GroupByWrapper object itself, because underlining sources like SQL need list of columns to sort by

I can see that exceptions are happening in EFCore. Could you share provider (In-memory, SQL Server etc.) and version?

badcommandorfilename commented 4 years ago

Hi @kosinsky - thanks for the prompt response: we're using <TargetFramework>netstandard2.0</TargetFramework> with SQL Server 10.50.4042

    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="2.2.6" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Proxies" Version="2.2.6" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.Relational" Version="2.2.6" />
    <PackageReference Include="System.Linq.Dynamic.Core" Version="1.0.13" />
kosinsky commented 4 years ago

I think I have an idea what happens. We support to serialization options:

  1. OData complaint In that case output looks like
    {
    "@odata.context":"....",
    "value": [
                {
                    "email": "new@test.org",
                    "Total": 11.00000
                },
                {
                    "email": "test@test.biz",
                    "Total": 17.00000
                },
                {
                    "email": "test@token.biz",
                    "Total": 82.00000
                },
                {
                    "email": "test@test.org",
                    "Total": 538.00000
                }
            ]
    }

    Controller should derive from ODataController or have ODataFormatting attribute

  2. Generic ASP.NET - You using that one (Newtonsoft.Json in the exception)

All tests for aggregations are using only 1. It's possible that generic serialization adds extra sorting and implementing IComparable is required.

Could you try to add tests that demo an issue and add IComparable implementation? BTW, It's possible that it will reveal new issue. Please, @mention me in early version of the PR, if you need help.

You could use $select/$expand tests as an example: OData formatted: https://github.com/OData/WebApi/blob/d02bc61ea7b31ada1e54abbeebbecb3c5df0e3ac/test/E2ETest/Microsoft.Test.E2E.AspNet.OData/QueryComposition/SelectExpandEFTests.cs Generic JSON: https://github.com/OData/WebApi/blob/d02bc61ea7b31ada1e54abbeebbecb3c5df0e3ac/test/E2ETest/Microsoft.Test.E2E.AspNet.OData/QueryComposition/JsonSelectExpandTests.cs