Fixes #1126: enable built-in expression in $orderby #1150
Conversation
xuzhg
requested review from
KenitoInc,
habbes,
mikepizzo,
ElizabethOkerio and
gathogojr
| /// <summary> | ||
| /// Represents the order by expression in the $orderby clause. | ||
| /// Use this to represent other $orderby except 'Property,OpenProperty,$count, $it' orderBy expression. | ||
| /// Again, in the next major release, we don't need this class. |
There was a problem hiding this comment.
I'd suggest creating an issue to track this and tag it as a major release opportunity and then referencing the issue url here.
There was a problem hiding this comment.
Also, I think it's more suitable to put this in a <remarks> tag instead of the <summary>. #resolved
| @@ -14,6 +14,7 @@ namespace Microsoft.AspNetCore.OData.Query | |||
| { | |||
| /// <summary> | |||
| /// Represents a single order by expression in the $orderby clause. | |||
| /// saxu: Why do we need this class and its derived type? only fetch the PropertyPath? In the next major release, we can consider to remove all of these. | |||
There was a problem hiding this comment.
I'd suggest creating an issue to track and then referencing the issue url here.
There was a problem hiding this comment.
I think this note should go in the <remarks> instead of <summary>
| case ClrCanonicalFunctions.IndexofFunctionName: | ||
| result = AllowedFunctions.IndexOf; | ||
| break; | ||
| case "isof": |
There was a problem hiding this comment.
Why isn't this in ClrCanonicalFunctions?
There was a problem hiding this comment.
I don't know why. But, I prefer to use "isof" directly in the code.
| /// </summary> | ||
| /// <param name="propertyAccessNode">The single value property access node.</param> | ||
| /// <param name="validatorContext">The validation context.</param> | ||
| protected virtual void ValidateSingleValuePropertyAccessNode(SingleValuePropertyAccessNode propertyAccessNode, OrderByValidatorContext validatorContext) |
There was a problem hiding this comment.
Did you add any tests on validation errors?
There was a problem hiding this comment.
We already have the orderby validator tests in the UT: OrderByQueryValidatorTest.cs
src/Microsoft.AspNetCore.OData/Query/Validator/OrderByQueryValidator.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Validator/OrderByQueryValidator.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.AspNetCore.OData.E2E.Tests/ODataOrderByTest/OrderByAdvancedTest.cs
Outdated
Show resolved
Hide resolved
|
Curious there'd be any value in adding E2E tests to verify the behaviour against EF query provider. |
…idator.cs Co-authored-by: John Gathogo <john.gathogo@microsoft.com>
Co-authored-by: John Gathogo <john.gathogo@microsoft.com>
Fixes #1126
Support odata query expression in $orderby. For example:
$orderby=tolower(name)
$orderby=year(Birthday)
....