Skip to content

Commit

Permalink
Fix for Error occurs when you check whether a string or integer liter…
Browse files Browse the repository at this point in the history 
…al is in an enum collection (#3023)

* Refactor InBinder to convert left operand to enum type if necessary

* Add tests for filter with In Operator with enums member names and member integral values

* Add more functional tests for parseFilter with 'In' Operation with Enums

* Add a failed tests when Float are used as enum integral value with In Operator

* Add parse filter test with 'In' operator with enums where left operand is Invalid integral values

* Adding tests for errors thrown when using string literal that doesn't exists as enum member name

* Refactor to reuse methods and move logic that check if convert left node is required to private method

* Remove unnecessary ShouldConvertLeftOperand() method and put the logic inside BindInOperator()

* Make the new tests to only use generic methods introduced

* Resolve comments

* Reuse GetIEdmProperty() and GetIEdmType() methods in the other existing tests

* Merge related tests using [Theory]

* use string instead of object
  • Loading branch information
@WanjohiSammy
WanjohiSammy committed Aug 16, 2024
1 parent 7a93e7c commit 0c80fba
Show file tree
Hide file tree
Showing 3 changed files with 214 additions and 42 deletions.
7 changes: 7 additions & 0 deletions src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ internal QueryNode BindInOperator(InToken inToken, BindingState state)
inToken.Right, new EdmCollectionTypeReference(new EdmCollectionType(EdmCoreModel.Instance.GetUntyped())), state.Model);
}

// If the left operand is either an integral or a string type and the right operand is a collection of enums,
// Calls the MetadataBindingUtils.ConvertToTypeIfNeeded() method to convert the left operand to the same enum type as the right operand.
if ((right is CollectionPropertyAccessNode && right.ItemType.IsEnum()) && (left.TypeReference != null && (left.TypeReference.IsString() || left.TypeReference.IsIntegral())))
{
left = MetadataBindingUtils.ConvertToTypeIfNeeded(left, right.ItemType);
}

return new InNode(left, right);
}

Expand Down
191 changes: 149 additions & 42 deletions ...onalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//---------------------------------------------------------------------

using System;
using System.Linq;
using System.Collections.Generic;
using Microsoft.OData.Tests.UriParser;
using Microsoft.OData.UriParser;
Expand Down Expand Up @@ -52,7 +53,7 @@ public EnumFilterFunctionalTests()
this.userModel.AddElement(enumType);

// add enum property
this.entityType = new EdmEntityType("NS", "MyEntityType", isAbstract: false, isOpen: true, baseType: null);
this.entityType = new EdmEntityType("NS", "MyEntityType", isAbstract: false, isOpen: false, baseType: null);
var enumTypeReference = new EdmEnumTypeReference(enumType, true);
this.entityType.AddProperty(new EdmStructuralProperty(this.entityType, "Color", enumTypeReference));

Expand All @@ -70,6 +71,10 @@ public EnumFilterFunctionalTests()
var enumFlagsTypeReference = new EdmEnumTypeReference(enumFlagsType, true);
this.entityType.AddProperty(new EdmStructuralProperty(this.entityType, "ColorFlags", enumFlagsTypeReference));

// add colors collection
var colorTypeReference = new EdmEnumTypeReference(enumType, false);
this.entityType.AddStructuralProperty("Colors", new EdmCollectionTypeReference(new EdmCollectionType(colorTypeReference)));

this.userModel.AddElement(this.entityType);

var defaultContainer = new EdmEntityContainer("NS", "DefaultContainer");
Expand All @@ -94,11 +99,11 @@ public void ParseFilterWithEnum()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(GetColorProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

binaryNode
.Right
.ShouldBeEnumNode(this.GetColorType(this.userModel), (int)Color.Green);
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.Color"), (int)Color.Green);
}

[Fact]
Expand All @@ -116,11 +121,11 @@ public void ParseFilterWithEnumInt()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(GetColorProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

binaryNode
.Right
.ShouldBeEnumNode(this.GetColorType(this.userModel), (int)Color.Green);
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.Color"), (int)Color.Green);
}

[Fact]
Expand All @@ -138,11 +143,11 @@ public void ParseFilterWithHasOperatorEnumMemberName()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(this.GetColorFlagsType(this.userModel), (int)ColorFlags.Green);
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"), (int)ColorFlags.Green);
}

[Fact]
Expand All @@ -160,11 +165,11 @@ public void ParseFilterWithHasOperatorEnumUnderlyingValue()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(this.GetColorFlagsType(this.userModel), (int)ColorFlags.Green);
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"), (int)ColorFlags.Green);
}

[Fact]
Expand All @@ -182,11 +187,11 @@ public void ParseFilterWithHasOperatorEnumLiteralValueAsLeftOperand()

binaryNode
.Left
.ShouldBeEnumNode(this.GetColorFlagsType(this.userModel), (int)(ColorFlags.Green | ColorFlags.Red));
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red));

binaryNode
.Right
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));
}

[Fact]
Expand All @@ -204,11 +209,11 @@ public void ParseFilterWithHasOperatorNonFlagsEnum()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

binaryNode
.Right
.ShouldBeEnumNode(this.GetColorType(this.userModel), (int)Color.Green);
.ShouldBeEnumNode(this.GetIEdmType<IEdmEnumType>("NS.Color"), (int)Color.Green);
}

[Fact]
Expand All @@ -226,12 +231,12 @@ public void ParseFilterWithEnumNormalConbinedValues()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorFlagsType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"),
(int)(ColorFlags.Green | ColorFlags.Red));
}

Expand All @@ -250,18 +255,18 @@ public void ParseFilterWithEnumCombinedValuesOrderReversed()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorFlagsType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"),
(int)(ColorFlags.Green | ColorFlags.Red));
}

[Fact]
public void ParseFilterWithEnumValuesCompatibleWithString()
{
{
var filterQueryNode = ParseFilter(
"ColorFlags has 'Red'",
this.userModel,
Expand All @@ -274,12 +279,12 @@ public void ParseFilterWithEnumValuesCompatibleWithString()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorFlagsType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"),
"Red");
}

Expand All @@ -298,12 +303,12 @@ public void ParseFilterWithEnumDefinedConbinedValues()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorFlagsType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"),
(int)(ColorFlags.Green | ColorFlags.Red));
}

Expand All @@ -322,12 +327,12 @@ public void ParseFilterWithEnumCombinedUnderlyingValues()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorFlagsType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.ColorFlags"),
(int)(ColorFlags.Green | ColorFlags.Red));
}

Expand All @@ -346,12 +351,12 @@ public void ParseFilterWithEnumNegativeMember()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.Color"),
(int)(Color.White));

var constantNode = Assert.IsType<ConstantNode>(binaryNode.Right);
Expand All @@ -374,12 +379,12 @@ public void ParseFilterWithEnumUndefinedMember()

binaryNode
.Left
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorProp(this.userModel));
.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

binaryNode
.Right
.ShouldBeEnumNode(
this.GetColorType(this.userModel),
this.GetIEdmType<IEdmEnumType>("NS.Color"),
-132534290);

var constantNode = Assert.IsType<ConstantNode>(binaryNode.Right);
Expand All @@ -398,7 +403,7 @@ public void ParseFilterWithNullEnumValue()
{
var filterQueryNode = ParseFilter("Color eq null", this.userModel, this.entityType, this.entitySet);
var binaryNode = filterQueryNode.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal);
binaryNode.Left.ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorProp(this.userModel));
binaryNode.Left.ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("Color"));

var convertNode = Assert.IsType<ConvertNode>(binaryNode.Right);
convertNode.Source.ShouldBeConstantQueryNode((object)null);
Expand Down Expand Up @@ -515,28 +520,130 @@ public void ParseFilterEnumTypesWrongCast3()
parse.Throws<ODataException>(Strings.MetadataBinder_CastOrIsOfFunctionWithoutATypeArgument);
}

private IEdmStructuralProperty GetColorProp(IEdmModel model)
[Theory]
[InlineData("NS.Color'Green'")]
[InlineData("'Green'")]
[InlineData("'2'")]
[InlineData("2")]
public void ParseFilterWithInOperatorWithEnums(string filterOptionValue)
{
// Arrange
string filterQuery = $"{filterOptionValue} in Colors";
string enumValue = "Green";
string colorTypeName = "NS.Color";

IEdmEnumType colorType = this.GetIEdmType<IEdmEnumType>(colorTypeName);
IEdmEnumMember enumMember = colorType.Members.First(m => m.Name == enumValue);

string expectedLiteral = "'Green'";
if (filterOptionValue.StartsWith(colorTypeName)) // if the filterOptionValue is already fully qualified, then the expectedLiteral should be the same as filterOptionValue
{
expectedLiteral = "NS.Color'Green'";
enumValue = enumMember.Value.Value.ToString(); // "2" <cref="ODataEnumValue.Value"/> The backing type, can be "3" or "White" or "Black,Yellow,Cyan"
}

// Act
FilterClause filterQueryNode = ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);
SingleValueNode expression = filterQueryNode.Expression;

// Assert
Assert.Equal(2, enumMember.Value.Value);
Assert.Equal("Green", enumMember.Name);
InNode inNode = expression.ShouldBeInNode();
ConstantNode leftNode = Assert.IsType<ConstantNode>(inNode.Left);
leftNode.ShouldBeEnumNode(colorType, enumValue);

Assert.True(leftNode.TypeReference.IsEnum());
Assert.Equal(enumValue, leftNode.Value.ToString());
Assert.Equal(expectedLiteral, leftNode.LiteralText);

CollectionPropertyAccessNode rightNode = Assert.IsType<CollectionPropertyAccessNode>(inNode.Right);
rightNode.ShouldBeCollectionPropertyAccessQueryNode(this.GetIEdmProperty("Colors"));
Assert.Equal(colorType, rightNode.ItemType.Definition);

}

[Fact]
public void ParseFilterWithInOperatorWithEnumsMemberFloatIntegralValue_ThrowCollectionItemTypeMustBeSameAsSingleItemTypeException()
{
// Arrange
string filterQuery = $"3.0 in Colors";

// Act
Action test = () => ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);

// Assert
test.Throws<ArgumentException>(Strings.Nodes_InNode_CollectionItemTypeMustBeSameAsSingleItemType("NS.Color", "Edm.Single")); // Float are of Type Edm.Single
}

[Theory]
[InlineData("-20", "-20")]
[InlineData("'-20'", "-20")]
[InlineData("'3.0'", "3.0")]
public void ParseFilterWithInOperatorWithEnumsInvalidIntegralValues_ThrowsIsNotValidEnumConstantException(string integralValue, string errorMessageParam)
{
// Arrange
string filterQuery = $"{integralValue} in Colors";

// Act
Action action = () => ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);

// Assert
action.Throws<ODataException>(Strings.Binder_IsNotValidEnumConstant(errorMessageParam));
}

[Fact]
public void ParseFilterWithInOperatorWithEnumsMemberNameWithoutSingleQuotes_ThrowsPropertyNotDeclaredException()
{
// Arrange
string filterQuery = "Red in Colors";

// Act
Action action = () => ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);

// Assert
action.Throws<ODataException>(Strings.MetadataBinder_PropertyNotDeclared(this.entityType.FullName, "Red"));
}

[Theory]
[InlineData("'Yellow'")]
[InlineData("'Teal'")]
[InlineData("NS.Color'Yellow'")]
[InlineData("NS.Color'Teal'")]
public void ParseFilterWithInOperatorWithEnumsInvalidMemberNames_ThrowsIsNotValidEnumConstantException(string memberName)
{
// Arrange
string filterQuery = $"{memberName} in Colors";

string expectedExceptionParameter = memberName.StartsWith("'") ? memberName.Trim('\'') : memberName; // Trim('\'') method removes any single quotes from the start and end of the string

// Act
Action action = () => ParseFilter(filterQuery, this.userModel, this.entityType, this.entitySet);

// Assert
action.Throws<ODataException>(Strings.Binder_IsNotValidEnumConstant(expectedExceptionParameter));
}

private T GetIEdmType<T>(string typeName) where T : IEdmType
{
return (IEdmStructuralProperty)((IEdmStructuredType)model
.FindType("NS.MyEntityType"))
.FindProperty("Color");
return (T)this.userModel.FindType(typeName);
}

private IEdmEnumType GetColorType(IEdmModel model)
private T GetIEdmType<T>(IEdmModel model, string typeName) where T : IEdmType
{
return (IEdmEnumType)model.FindType("NS.Color");
return (T)model.FindType(typeName);
}

private IEdmStructuralProperty GetColorFlagsProp(IEdmModel model)
private IEdmStructuralProperty GetIEdmProperty(string propertyName)
{
return (IEdmStructuralProperty)((IEdmStructuredType)model
.FindType("NS.MyEntityType"))
.FindProperty("ColorFlags");
return (IEdmStructuralProperty)this.GetIEdmType<IEdmStructuredType>("NS.MyEntityType")
.FindProperty(propertyName);
}

private IEdmEnumType GetColorFlagsType(IEdmModel model)
private IEdmStructuralProperty GetIEdmProperty(string entityName, string propertyName)
{
return (IEdmEnumType)model.FindType("NS.ColorFlags");
return (IEdmStructuralProperty)this.GetIEdmType<IEdmStructuredType>(entityName)
.FindProperty(propertyName);
}

private FilterClause ParseFilter(string text, IEdmModel edmModel, IEdmEntityType edmEntityType, IEdmEntitySet edmEntitySet)
Expand Down
Loading

0 comments on commit 0c80fba

Please sign in to comment.