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.

Sign up for GitHub

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 for Error occurs when you check whether a string or integer literal is in an enum collection #3023

Merged
merged 14 commits into from
Aug 16, 2024
Merged

Fix for Error occurs when you check whether a string or integer literal is in an enum collection #3023

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a101461
Refactor InBinder to convert left operand to enum type if necessary
WanjohiSammy Jul 22, 2024
cdc0cb1
Add tests for filter with In Operator with enums member names and mem…
WanjohiSammy Jul 22, 2024
2eb3a56
Add more functional tests for parseFilter with 'In' Operation with Enums
WanjohiSammy Jul 23, 2024
1ad2413
Add a failed tests when Float are used as enum integral value with In…
WanjohiSammy Jul 25, 2024
8f35954
Add parse filter test with 'In' operator with enums where left operan…
WanjohiSammy Jul 25, 2024
2f7bda7
Adding tests for errors thrown when using string literal that doesn't…
WanjohiSammy Jul 29, 2024
a2d1935
Refactor to reuse methods and move logic that check if convert left n…
WanjohiSammy Jul 30, 2024
299516f
Remove unnecessary ShouldConvertLeftOperand() method and put the logi…
WanjohiSammy Aug 14, 2024
5f65354
Make the new tests to only use generic methods introduced
WanjohiSammy Aug 14, 2024
b524e5b
Resolve comments
WanjohiSammy Aug 14, 2024
f0fbbde
Merge branch 'fix/2371-allow-filter-stringliterals-in-enum-collection…
WanjohiSammy Aug 14, 2024
8f0698b
Reuse GetIEdmProperty() and GetIEdmType() methods in the other existi…
WanjohiSammy Aug 14, 2024
07655ee
Merge related tests using [Theory]
WanjohiSammy Aug 15, 2024
3f4f00a
use string instead of object
WanjohiSammy Aug 15, 2024
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
7 changes: 7 additions & 0 deletions src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs
View file
Open in desktop
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
Open in desktop
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"));
WanjohiSammy marked this conversation as resolved.
Show resolved Hide resolved

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)
WanjohiSammy marked this conversation as resolved.
Show resolved Hide resolved
{
// 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")
WanjohiSammy marked this conversation as resolved.
Show resolved Hide resolved
.FindProperty(propertyName);
}

private IEdmEnumType GetColorFlagsType(IEdmModel model)
private IEdmStructuralProperty GetIEdmProperty(string entityName, string propertyName)
WanjohiSammy marked this conversation as resolved.
Show resolved Hide resolved
{
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
Loading