From 0c80fbaed7ea37f79a007ebb346179a6132d1092 Mon Sep 17 00:00:00 2001 From: Samuel Wanjohi Date: Fri, 16 Aug 2024 15:46:44 +0300 Subject: [PATCH] Fix for Error occurs when you check whether a string or integer literal 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 --- .../UriParser/Binders/InBinder.cs | 7 + .../UriParser/EnumFilterFunctionalTests.cs | 191 ++++++++++++++---- .../FilterAndOrderByFunctionalTests.cs | 58 ++++++ 3 files changed, 214 insertions(+), 42 deletions(-) diff --git a/src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs b/src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs index 953576c020..d80dc638d9 100644 --- a/src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs +++ b/src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs @@ -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); } diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs index d8a39ba1f5..64b9bb37b9 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/EnumFilterFunctionalTests.cs @@ -5,6 +5,7 @@ //--------------------------------------------------------------------- using System; +using System.Linq; using System.Collections.Generic; using Microsoft.OData.Tests.UriParser; using Microsoft.OData.UriParser; @@ -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)); @@ -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"); @@ -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("NS.Color"), (int)Color.Green); } [Fact] @@ -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("NS.Color"), (int)Color.Green); } [Fact] @@ -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("NS.ColorFlags"), (int)ColorFlags.Green); } [Fact] @@ -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("NS.ColorFlags"), (int)ColorFlags.Green); } [Fact] @@ -182,11 +187,11 @@ public void ParseFilterWithHasOperatorEnumLiteralValueAsLeftOperand() binaryNode .Left - .ShouldBeEnumNode(this.GetColorFlagsType(this.userModel), (int)(ColorFlags.Green | ColorFlags.Red)); + .ShouldBeEnumNode(this.GetIEdmType("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red)); binaryNode .Right - .ShouldBeSingleValuePropertyAccessQueryNode(this.GetColorFlagsProp(this.userModel)); + .ShouldBeSingleValuePropertyAccessQueryNode(this.GetIEdmProperty("ColorFlags")); } [Fact] @@ -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("NS.Color"), (int)Color.Green); } [Fact] @@ -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("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red)); } @@ -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("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red)); } [Fact] public void ParseFilterWithEnumValuesCompatibleWithString() - { + { var filterQueryNode = ParseFilter( "ColorFlags has 'Red'", this.userModel, @@ -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("NS.ColorFlags"), "Red"); } @@ -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("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red)); } @@ -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("NS.ColorFlags"), (int)(ColorFlags.Green | ColorFlags.Red)); } @@ -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("NS.Color"), (int)(Color.White)); var constantNode = Assert.IsType(binaryNode.Right); @@ -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("NS.Color"), -132534290); var constantNode = Assert.IsType(binaryNode.Right); @@ -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(binaryNode.Right); convertNode.Source.ShouldBeConstantQueryNode((object)null); @@ -515,28 +520,130 @@ public void ParseFilterEnumTypesWrongCast3() parse.Throws(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(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" 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(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(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(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(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(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(Strings.Binder_IsNotValidEnumConstant(expectedExceptionParameter)); + } + + private T GetIEdmType(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(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("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(entityName) + .FindProperty(propertyName); } private FilterClause ParseFilter(string text, IEdmModel edmModel, IEdmEntityType edmEntityType, IEdmEntitySet edmEntitySet) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs index 729d6eeeb8..4ee9d76858 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs @@ -2041,6 +2041,64 @@ public void FilterWithInOperationWithEnums() Assert.Equal("FavoriteColors", Assert.IsType(inNode.Right).Property.Name); } + [Theory] + [InlineData("'SolidYellow'")] + [InlineData("'12'")] + [InlineData("12")] + public void FilterWithInOperationWithEnumsMemberIntegralValue(string filterOptionValue) + { + // Arrange + string filterQuery = $"{filterOptionValue} in FavoriteColors"; + + string expectedLiteralText = "'SolidYellow'"; + string expectedfullyQualifiedName = "Fully.Qualified.Namespace.ColorPattern'SolidYellow'"; + string expectedCollectionPropertyName = "FavoriteColors"; + + // Act + FilterClause filter = ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType()); + + // Assert + InNode inNode = Assert.IsType(filter.Expression); + ConstantNode left = Assert.IsType(inNode.Left); + ODataEnumValue enumValue = Assert.IsType(left.Value); + + Assert.Equal(expectedLiteralText, left.LiteralText); + Assert.Equal(expectedfullyQualifiedName, (enumValue.TypeName + left.LiteralText)); + Assert.Equal(expectedCollectionPropertyName, Assert.IsType(inNode.Right).Property.Name); + } + + [Theory] + [InlineData("53")] + [InlineData("'53'")] + public void FilterWithInOperationWithEnumsInvalidMemberIntegralValue_ThrowsIsNotValidEnumConstantException(string integralValue) + { + // Arrange + string filterQuery = $"{integralValue} in FavoriteColors"; + + // Act + Action action = () => ParseFilter(filterQuery, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType()); + + // Assert + action.Throws(ODataErrorStrings.Binder_IsNotValidEnumConstant("53")); + } + + [Theory] + [InlineData("'Teal'")] + [InlineData("NS.Color'Teal'")] + public void FilterWithInOperationWithEnumsInvalidMemberNames_ThrowsIsNotValidEnumConstantException(string memberName) + { + // Arrange + string filterQuery = $"{memberName} in FavoriteColors"; + + 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, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType()); + + // Assert + action.Throws(ODataErrorStrings.Binder_IsNotValidEnumConstant(expectedExceptionParameter)); + } + [Fact] public void FilterWithInOperationWithAny() {