-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fixes #1126: enable built-in expression in $orderby #1150
Conversation
/// <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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think it's more suitable to put this in a <remarks>
tag instead of the <summary>
. #resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest creating an issue to track and then referencing the issue url here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this note should go in the <remarks>
instead of <summary>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
case ClrCanonicalFunctions.IndexofFunctionName: | ||
result = AllowedFunctions.IndexOf; | ||
break; | ||
case "isof": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this in ClrCanonicalFunctions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why. But, I prefer to use "isof" directly in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved.
/// </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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add any tests on validation errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, otherwise LGTM
…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)
....