Skip to content
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 type casting exceptions in 'isof' and 'cast' calls. #3117

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion src/Microsoft.OData.Core/UriParser/Binders/FunctionCallBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,17 @@ internal QueryNode BindFunctionCall(FunctionCallToken functionCallToken)

// If there isn't, bind as Uri function
// Bind all arguments
List<QueryNode> argumentNodes = new List<QueryNode>(functionCallToken.Arguments.Select(ar => this.bindMethod(ar)));
List<QueryNode> argumentNodes = functionCallToken.Arguments.Select(argument =>
{
// If the function is IsOf or Cast and the argument is a dotted identifier, we need to bind it differently
if (UnboundFunctionNames.Contains(functionCallToken.Name) && argument.ValueToken is DottedIdentifierToken dottedIdentifier)
{
return this.TryBindDottedIdentifierForIsOfOrCastFunctionCall(dottedIdentifier);
}

return this.bindMethod(argument);
}).ToList();

return BindAsUriFunction(functionCallToken, argumentNodes);
}

Expand Down Expand Up @@ -426,6 +436,51 @@ private bool TryBindIdentifier(string identifier, IEnumerable<FunctionParameterT
return true;
}

/// <summary>
/// Binds a <see cref="DottedIdentifierToken"/> for the 'isof' and 'cast' function calls.
/// </summary>
/// <param name="dottedIdentifierToken">The dotted identifier token to bind.</param>
/// <returns>A <see cref="QueryNode"/> representing the bound single resource node.</returns>
/// <exception cref="ODataException">Thrown when the token cannot be bound as a single resource node.</exception>
private QueryNode TryBindDottedIdentifierForIsOfOrCastFunctionCall(DottedIdentifierToken dottedIdentifierToken)
{
QueryNode parent = null;
IEdmType parentType = null;

if (state.ImplicitRangeVariable != null)
{
if (dottedIdentifierToken.NextToken == null)
{
parent = NodeFactory.CreateRangeVariableReferenceNode(state.ImplicitRangeVariable);
parentType = state.ImplicitRangeVariable.TypeReference.Definition;
}
else
{
parent = this.bindMethod(dottedIdentifierToken.NextToken);
parentType = parent.GetEdmType();
}
}

IEdmSchemaType childType = UriEdmHelpers.FindTypeFromModel(state.Model, dottedIdentifierToken.Identifier, this.Resolver);

if (childType is not IEdmStructuredType childStructuredType)
{
return this.bindMethod(dottedIdentifierToken);
}

if (parentType != null)
{
this.state.ParsedSegments.Add(new TypeSegment(childType, parentType, null));
}

if (parent is CollectionResourceNode parentAsCollection)
{
return new CollectionResourceCastNode(parentAsCollection, childStructuredType);
}

return new SingleResourceCastNode(parent as SingleResourceNode, childStructuredType);
}

/// <summary>
/// Bind path segment's operation or operationImport's parameters.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,20 +409,24 @@ public void ParseFilterWithNullEnumValue()
convertNode.Source.ShouldBeConstantQueryNode((object)null);
}

[Fact]
public void ParseFilterCastMethod1()
[Theory]
[InlineData("cast(NS.Color'Green', 'Edm.String') eq 'blue'")]
[InlineData("cast(NS.Color'Green', Edm.String) eq 'blue'")]
public void ParseFilterCastMethod1(string filterQuery)
{
var filter = ParseFilter("cast(NS.Color'Green', 'Edm.String') eq 'blue'", this.userModel, this.entityType, this.entitySet);
var filter = ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);
var bon = filter.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal);
var convertNode = Assert.IsType<ConvertNode>(bon.Left);
var functionCallNode = Assert.IsType<SingleValueFunctionCallNode>(convertNode.Source);
Assert.Equal("cast", functionCallNode.Name); // ConvertNode is because cast() result's nullable=false.
}

[Fact]
public void ParseFilterCastMethod2()
[Theory]
[InlineData("cast('Green', 'NS.Color') eq NS.Color'Green'")]
[InlineData("cast('Green', NS.Color) eq NS.Color'Green'")]
public void ParseFilterCastMethod2(string filterQuery)
{
var filter = ParseFilter("cast('Green', 'NS.Color') eq NS.Color'Green'", this.userModel, this.entityType, this.entitySet);
var filter = ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);
var bon = filter.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal);
var functionCallNode = Assert.IsType<SingleValueFunctionCallNode>(bon.Left);
Assert.Equal("cast", functionCallNode.Name);
Expand Down Expand Up @@ -499,17 +503,21 @@ public void ParseFilterEnumMemberUndefined4()
parse.Throws<ODataException>(Strings.Binder_IsNotValidEnumConstant("NS.ColorFlags'Red,2'"));
}

[Fact]
public void ParseFilterEnumTypesWrongCast1()
[Theory]
[InlineData("cast(NS.ColorFlags'Green', 'Edm.Int64') eq 2")]
[InlineData("cast(NS.ColorFlags'Green', Edm.Int64) eq 2")]
public void ParseFilterEnumTypesWrongCast1(string filter)
{
Action parse = () => ParseFilter("cast(NS.ColorFlags'Green', 'Edm.Int64') eq 2", this.userModel, this.entityType, this.entitySet);
Action parse = () => ParseFilter(filter, this.userModel, this.entityType, this.entitySet);
parse.Throws<ODataException>(Strings.CastBinder_EnumOnlyCastToOrFromString);
}

[Fact]
public void ParseFilterEnumTypesWrongCast2()
[Theory]
[InlineData("cast(321, 'NS.ColorFlags') eq 2")]
[InlineData("cast(321, NS.ColorFlags) eq 2")]
public void ParseFilterEnumTypesWrongCast2(string filter)
{
Action parse = () => ParseFilter("cast(321, 'NS.ColorFlags') eq 2", this.userModel, this.entityType, this.entitySet);
Action parse = () => ParseFilter(filter, this.userModel, this.entityType, this.entitySet);
parse.Throws<ODataException>(Strings.CastBinder_EnumOnlyCastToOrFromString);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,31 @@ public void IsOfFunctionWorksWithOrWithoutSingleQuotesOnType()
singleValueFunctionCallNode.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Edm.String");
}

[Theory]
[InlineData("isof(Fully.Qualified.Namespace.Employee)")]
[InlineData("isof('Fully.Qualified.Namespace.Employee')")]
public void IsOfFunctionWithOneParameter_WithOrWithoutSingleQuotesOnTypeParameter_WorksAsExpected(string filterQuery)
{
// Arrange & Act
FilterClause filter = ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());

// Assert
SingleValueFunctionCallNode singleValueFunctionCallNode = filter.Expression.ShouldBeSingleValueFunctionCallQueryNode("isof");
ResourceRangeVariableReferenceNode rangeVariableReference = singleValueFunctionCallNode.Parameters.ElementAt(0).ShouldBeResourceRangeVariableReferenceNode("$it");
Assert.Equal("Fully.Qualified.Namespace.Person", rangeVariableReference.GetEdmTypeReference().FullName()); // $it is of type Person

if(singleValueFunctionCallNode.Parameters.ElementAt(1) is ConstantNode)
{
var constantNode = singleValueFunctionCallNode.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Fully.Qualified.Namespace.Employee");
Assert.Equal("Fully.Qualified.Namespace.Employee", constantNode.Value);
}
else
{
var singleResourceCastNode = singleValueFunctionCallNode.Parameters.ElementAt(1).ShouldBeSingleResourceCastNode(HardCodedTestModel.GetEmployeeTypeReference());
Assert.Equal("Fully.Qualified.Namespace.Employee", singleResourceCastNode.TypeReference.FullName());
}
}

[Fact]
public void IsOfFunctionWithOneParameter_WithSingleQuotesOnTypeParameter_ShouldBeConstantQueryNode()
{
Expand Down Expand Up @@ -805,6 +830,32 @@ public void IsOfFunctionWithOneParameter_WithoutSingleQuotesOnTypeParameter_Shou
Assert.Equal("Fully.Qualified.Namespace.Employee", singleResourceCastNode.TypeReference.FullName()); // Fully.Qualified.Namespace.Employee is the type parameter
}

[Theory]
[InlineData("isof(MyAddress, 'Fully.Qualified.Namespace.HomeAddress')")]
[InlineData("isof(MyAddress, Fully.Qualified.Namespace.HomeAddress)")]
public void IsOfFunctionWithTwoParameters_WithSingleQuotesOnTypeParameter_WorksAsExpected(string filterQuery)
{
// Arrange & Act
FilterClause filter = ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());

// Assert
SingleValueFunctionCallNode singleValueFunctionCallNode = filter.Expression.ShouldBeSingleValueFunctionCallQueryNode("isof");
SingleComplexNode singleComplexNode = singleValueFunctionCallNode.Parameters.ElementAt(0).ShouldBeSingleComplexNode(HardCodedTestModel.GetPersonAddressProp());
Assert.Equal("MyAddress", singleComplexNode.Property.Name); // MyAddress is the property name
Assert.Equal("Fully.Qualified.Namespace.Address", singleComplexNode.GetEdmTypeReference().FullName()); // MyAddress is of type Address

if (singleValueFunctionCallNode.Parameters.ElementAt(1) is ConstantNode)
{
var constantNode = singleValueFunctionCallNode.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Fully.Qualified.Namespace.HomeAddress");
Assert.Equal("Fully.Qualified.Namespace.HomeAddress", constantNode.Value); // 'Fully.Qualified.Namespace.Employee' is the type parameter
}
else
{
var singleResourceCastNode = singleValueFunctionCallNode.Parameters.ElementAt(1).ShouldBeSingleResourceCastNode(HardCodedTestModel.GetHomeAddressReference());
Assert.Equal("Fully.Qualified.Namespace.HomeAddress", singleResourceCastNode.TypeReference.FullName()); // Fully.Qualified.Namespace.HomeAddress is the type parameter
}
}

[Fact]
public void IsOfFunctionWithTwoParameters_WithSingleQuotesOnTypeParameter_ShouldBeConstantQueryNode()
{
Expand Down Expand Up @@ -844,27 +895,31 @@ public void IsOfFunctionWithTwoParameters_WithoutSingleQuotesOnTypeParameter_Sho
}

[Theory]
[InlineData("isof(Fully.Qualified.Namespace.Pet1)", "Fully.Qualified.Namespace.Pet1")]
[InlineData("cast(Fully.Qualified.Namespace.HomeAddress)/City eq 'City1'", "Fully.Qualified.Namespace.HomeAddress")]
public void IsOfAndCastFunctionsWithSingleParameterWithoutSingleQuotes_WithIncorrectType_ThrowException(string filterQuery, string fullyQualifiedTypeName)
[InlineData("isof(Fully.Qualified.Namespace.Pet1)")]
[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)
Copy link
Contributor

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?

Copy link
Contributor Author

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

{
// Arrange & Act
Action test = () => ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
var exception = Record.Exception(() => ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet()));

// Assert
test.Throws<ODataException>(Strings.MetadataBinder_HierarchyNotFollowed(fullyQualifiedTypeName, "Fully.Qualified.Namespace.Person"));
Assert.Null(exception);
}

[Theory]
[InlineData("isof(MyAddress,Fully.Qualified.Namespace.Pet1)", "Fully.Qualified.Namespace.Pet1")]
[InlineData("cast(MyAddress,Fully.Qualified.Namespace.Employee)/WorkID eq 345", "Fully.Qualified.Namespace.Employee")]
public void IsOfAndCastFunctionsWithTwoParameterWhereTypeParameterIsWithoutSingleQuotes_WithIncorrectType_ThrowException(string filterQuery, string fullyQualifiedTypeName)
[InlineData("cast(Fully.Qualified.Namespace.HomeAddress)/City eq 'City1'")]
[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)
Copy link
Contributor

@habbes habbes Nov 13, 2024

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?

Copy link
Contributor Author

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

For cast with unquoted type parameter

Copy link
Contributor

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.

Copy link
Contributor Author

@WanjohiSammy WanjohiSammy Nov 14, 2024

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.

image

An exception like below will be thrown:

{
    "error": {
        "code": "",
        "message": "Instance property 'Name' is not defined for type 'System.Object'"
    }
}

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
// Arrange & Act
Action test = () => ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
var exception = Record.Exception(() => ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet()));

// Assert
test.Throws<ODataException>(Strings.MetadataBinder_HierarchyNotFollowed(fullyQualifiedTypeName, "Fully.Qualified.Namespace.Address"));
Assert.Null(exception);
}

[Fact]
Expand All @@ -890,14 +945,31 @@ public void CastFunctionWorksForEnum()
bon.Right.ShouldBeEnumNode(HardCodedTestModel.TestModel.FindType("Fully.Qualified.Namespace.ColorPattern") as IEdmEnumType, 2L);
}

[Fact]
public void CastFunctionWorksForCastFromNullToEnum()
[Theory]
[InlineData("cast(null, Fully.Qualified.Namespace.ColorPattern) eq Fully.Qualified.Namespace.ColorPattern'blue'")]
[InlineData("cast(null, 'Fully.Qualified.Namespace.ColorPattern') eq Fully.Qualified.Namespace.ColorPattern'blue'")]
public void CastFunctionWorksForCastFromNullToEnum(string filterQuery)
{
FilterClause filter = ParseFilter("cast(null, Fully.Qualified.Namespace.ColorPattern) eq Fully.Qualified.Namespace.ColorPattern'blue'", HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
FilterClause filter = ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
var bon = filter.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal);
var singleFunctionCallNode = bon.Left.ShouldBeSingleValueFunctionCallQueryNode("cast");
Assert.Null(Assert.IsType<ConstantNode>(singleFunctionCallNode.Parameters.ElementAt(0)).Value);
singleFunctionCallNode.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Fully.Qualified.Namespace.ColorPattern");

string fullyQualifiedTypeName;
QueryNode secondParameterNode = singleFunctionCallNode.Parameters.ElementAt(1);
if(secondParameterNode is SingleResourceCastNode singleResourceCastNode)
{
secondParameterNode.ShouldBeSingleCastNode(HardCodedTestModel.GetColorPatternTypeReference());
fullyQualifiedTypeName = singleResourceCastNode.TypeReference.FullName();
}
else
{
ConstantNode constantNode = secondParameterNode.ShouldBeConstantQueryNode("Fully.Qualified.Namespace.ColorPattern");
fullyQualifiedTypeName = constantNode.Value as string;
}

Assert.Equal("Fully.Qualified.Namespace.ColorPattern", fullyQualifiedTypeName);
Assert.Equal("Fully.Qualified.Namespace.ColorPattern'blue'", Assert.IsType<ConstantNode>(bon.Right).LiteralText);
bon.Right.ShouldBeEnumNode(HardCodedTestModel.TestModel.FindType("Fully.Qualified.Namespace.ColorPattern") as IEdmEnumType, 2L);
}

Expand All @@ -912,16 +984,28 @@ public void LiteralTextShouldNeverBeNullForConstantNodeOfDottedIdentifier()
Assert.Equal("Fully.Qualified.Namespace.ColorPattern'blue'", Assert.IsType<ConstantNode>(bon.Right).LiteralText);
}

[Fact]
public void CastFunctionProducesAnEntityType()
[Theory]
[InlineData("cast(MyDog, 'Fully.Qualified.Namespace.Dog')/Color eq 'blue'")]
[InlineData("cast(MyDog, Fully.Qualified.Namespace.Dog)/Color eq 'blue'")]
public void CastFunctionProducesAnEntityType(string filterQuery)
{
FilterClause filter = ParseFilter("cast(MyDog, 'Fully.Qualified.Namespace.Dog')/Color eq 'blue'", HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
FilterClause filter = ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType(), HardCodedTestModel.GetPeopleSet());
SingleResourceFunctionCallNode function = filter.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal)
.Left.ShouldBeSingleValuePropertyAccessQueryNode(HardCodedTestModel.GetDogColorProp())
.Source.ShouldBeSingleResourceFunctionCallNode("cast");
Assert.Equal(2, function.Parameters.Count());
function.Parameters.ElementAt(0).ShouldBeSingleNavigationNode(HardCodedTestModel.GetPersonMyDogNavProp());
function.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Fully.Qualified.Namespace.Dog");
if(function.Parameters.ElementAt(1) is SingleResourceCastNode)
{
var singleResourceCastNode = function.Parameters.ElementAt(1).ShouldBeSingleCastNode(HardCodedTestModel.GetDogTypeReference());
Assert.Equal("Fully.Qualified.Namespace.Dog", singleResourceCastNode.TypeReference.FullName());
}
else
{
var constantNode = function.Parameters.ElementAt(1).ShouldBeConstantQueryNode("Fully.Qualified.Namespace.Dog");
Assert.Equal("Fully.Qualified.Namespace.Dog", constantNode.Value as string);
}

Assert.IsType<BinaryOperatorNode>(filter.Expression).Right.ShouldBeConstantQueryNode("blue");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1785,6 +1785,16 @@ public static IEdmEntityType GetFramedPaintingType()
return TestModel.FindType("Fully.Qualified.Namespace.FramedPainting") as IEdmEntityType;
}

public static IEdmComplexType GetColorPatternType()
{
return TestModel.FindType("Fully.Qualified.Namespace.ColorPattern") as IEdmComplexType;
}

public static IEdmComplexTypeReference GetColorPatternTypeReference()
{
return new EdmComplexTypeReference(GetColorPatternType(), false);
}

/// <summary>
/// Gets a type reference to a painting. We use 'false' for nullable because that is the value the product should set
/// it to when we have to create a reference (like for the item type of the collection you are filtering or something).
Expand Down