-
Notifications
You must be signed in to change notification settings - Fork 153
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
fix $orderby with $apply and groupby aggregations #919
fix $orderby with $apply and groupby aggregations #919
Conversation
bec82c1
to
a3a3e6d
Compare
src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinderContext.cs
Outdated
Show resolved
Hide resolved
Great ! 👍 |
@ElizabethOkerio Thanks. Can this fix be later merged to 8.0.12? Or we should wait for the next release? |
This still fails if the group by has more that one property. It works on version 8.0.4 For example: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. |
@REP2AV just tried this https://localhost:7098/odata/Customers?$apply=groupby((Name,LastName),aggregate(Amount%20with%20sum%20as%20TotalAmount))&$orderby=Name and it works. Are you using v8.2.0? can you share a repro? This will fail if you try to do $orderby on a property that is not an output of the group-by transformation. This is not supported. check this comment: #420 (comment) |
@ElizabethOkerio Yes, using 8.2.0. I've creted this repo with a minimal working example. /api/v2/WeatherForecasts?$apply=groupby((TemperatureC),aggregate($count as Count))&$orderby=TemperatureC -> works |
This PR fixes these issues: #889, #420
Executing a query like this: https://localhost:7098/odata/Customers?$apply=groupby((Name),aggregate(Amount with sum as TotalAmount))&$orderby=Name in AspNetCore OData greater than 8.0.4 and EfCore throws an exception.