-
Notifications
You must be signed in to change notification settings - Fork 350
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 'isof' and 'cast' unquoted type params issue #3117
base: main
Are you sure you want to change the base?
Conversation
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
[InlineData("cast(MyAddress,Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
[InlineData("cast(null,Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
[InlineData("cast('',Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
public void CastFunctionWithUnquotedTypeParameter_WithIncorrectType_DoesNotThrowException(string filterQuery) |
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.
What is the expected behaviour of cast()
when the types are incorrect? If it doesn't throw an exception, what will happen? How is this scenario expected to be handled? How will a library like AspNetCore know that the cast is not possible? And would this be a breaking change?
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.
This is what is happening currently with cast
with quoted type parameter
- ODL will bind
literal type parameter token
toConstantNode
using BindLiteral(LiteralToken literalToken). The AspNetCore library will get thecast
node and tries to create LINQ Expression using Expression BindCastSingleValue(SingleValueFunctionCallNode node, QueryBinderContext context). It knows how to handleConstantNode
For cast
with unquoted type parameter
- ODL will bind
DottedIDentifierToken
asSingleResourceCastNode
using a method I have added with this PR. To support this in AspNetCore, I will update function Expression BindCastSingleValue(SingleValueFunctionCallNode node, QueryBinderContext context) to add a condition to cast QueryNode to SingleResourceCastNode like what was done with the PR: Adding a condition to castQueryNode
toSingleResourceCastNode
for Unquoted Type Parameter AspNetCoreOData#1313
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.
Thanks for this added context. Can you also shed light on the behaviour with respect to handling of types where a cast is not possible? Ideally, from the user's endpoint the behaviour should be the same regardless of whether they user quoted or non-quoted syntax, and regardless of our internal implementation details which you have highlighted.
So could you clarify how we handle both supported and invalid cast scenarios in both quoted and unquoted variants? In which cases are we throwing an exception? Is the exception thrown consistent? Is that the expected behaviour?
My expectation is that isof
should not throw an exception when a cast is invalid, but rather evaluate to a boolean that will perform the correct filter. I think cast
throwing an exception when a cast is invalid is reasonable. What is not clear to me is whether such an exception should be thrown in ODL or AspNetCore. For the case of cast
I would be cautious about changing the existing behaviour to avoid introducing a breaking change.
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.
With the current behavior, ODL does not throw exception or to be more specific, it doesn't check if target type is related to source type. It only bind the quoted type parameters
to constant node
and return the entire token as FunctionCall token. The AspNetCoreOData
, on the other end, will get the token and try to create LINQ expression. If expression is not possible, AspNetCoreOData returns null BindSingleResourceCastFunctionCall will return null
expression.
Let's look at this query:
/products?$filter=cast('Microsoft.AspNetCore.OData.E2E.Tests.Cast.Category')/Name eq 'Cat
AspNetCore will try to relate Entity Product
to Entity Category
in BindSingleResourceCastFunctionCall and returns null expression if one is not assignable from the other.
And then CreatePropertyAccessExpression will throw an expression when trying to get the Property Name from a null expression.
An exception like below will be thrown:
{
"error": {
"code": "",
"message": "Instance property 'Name' is not defined for type 'System.Object'"
}
}
[InlineData("cast(MyAddress,Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
[InlineData("cast(null,Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
[InlineData("cast('',Fully.Qualified.Namespace.Employee)/WorkID eq 345")] | ||
public void CastFunctionWithUnquotedTypeParameter_WithIncorrectType_DoesNotThrowException(string filterQuery) |
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.
Is this behaviour consistent with how cast is handled when the type param is quoted?
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.
- For
quoted type parameter
, ODL bind the token toConstantNode
and AspNetCore is responsible to create LINQ expression using Expression BindCastSingleValue(SingleValueFunctionCallNode node, QueryBinderContext context). It knows how to handleConstantNode
. - For
unquoted type parameter
, ODL bind the token toSingleResourceCastNode
. When bumping the ODL version in AspNetCore, I will have to update the method Expression BindCastSingleValue(SingleValueFunctionCallNode node, QueryBinderContext context) to allow cast toSingleResourceCastNode
as well.
[InlineData("isof(MyAddress,Fully.Qualified.Namespace.Pet1)")] | ||
[InlineData("isof(null,Fully.Qualified.Namespace.Person)")] | ||
[InlineData("isof('',Fully.Qualified.Namespace.Person)")] | ||
public void IsOfFunctionsWithUnquotedTypeParameter_WithIncorrectType_DoesNotThrowException(string filterQuery) |
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.
Is this behaviour consistent with how isof
is handled when the type param is quoted?
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.
Same applied to isof
as discussed in #3117 (comment), however, the method in AspNetCore
that is responsible to create LINQ is different. Here is the method Expression BindIsOf(SingleValueFunctionCallNode node, QueryBinderContext context). I will also update this method to allow cast to SingleResourceCastNode
as it currently only support cast to ConstantNode
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
…itive type. Add more tests
...ctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Outdated
Show resolved
Hide resolved
…itive type. Add more tests
bdf86ef
to
f2c4d60
Compare
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.
Issues
This pull request fixes #1744.
Description
When using unquoted type parameters in
isof
andcast
, the operations perform as expected provided that the source type can be cast to the target type. Issues arise when casting is not possible, resulting in the following exception:Lets say you have the following data model:
Executing the query
isof(NS.DerivedCategory)
will throw the exception:This occurs because ODL checks if
NS.Product
is related toNS.Category
using the CheckRelatedTo function and since the types are not related or one is not a derivative of the other, the exception is thrown.Main changes
According to the spec_cast and spec_isof specifications, there is no indication that an exception should be thrown if casting fails. This pull request introduces the
TryBindDottedIdentifierForIsOfOrCastFunctionCall
method, which handles the binding ofDottedIdentifierToken
specifically for theisof
andcast
function calls without performing theCheckRelatedTo
validation, thereby preventing exceptions when type casting is not possible.This change also aims to fix issue #3123 by binding the
EdmPrimitiveType DottedIdentifierToken
toConstantNode
instead ofSingleValueCastNode
. This is by ensuring that thenextToken
of the EdmPrimitiveType DottedIdentifierToken (e.g., Edm.Int64, Edm.String, Edm.Single, etc.) is null if the DottedIdentifierToken is a primitive type even ifcast
orisof
has 2 arguments e.g,cast(ID, Edm.Int32) lt 10
,isof(MyAddress, Fully.Qualified.Namespace.HomeAddress)
, etcChecklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.