Skip to content

Commit

Permalink
Fix ArgumentNullException for: $filter=property in [''] (#3122)
Browse files Browse the repository at this point in the history 
* Add Square Bracket when constructing collectionNode. Also add some tests
  • Loading branch information
@WanjohiSammy
WanjohiSammy authored Dec 10, 2024
1 parent af53242 commit 5e715a2
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 16 deletions.
26 changes: 15 additions & 11 deletions src/Microsoft.OData.Core/UriParser/Binders/InBinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,21 +106,25 @@ private CollectionNode GetCollectionOperandFromToken(QueryToken queryToken, IEdm
LiteralToken literalToken = queryToken as LiteralToken;
if (literalToken != null)
{
string originalLiteralText = literalToken.OriginalText;

// Parentheses-based collections are not standard JSON but bracket-based ones are.
// Temporarily switch our collection to bracket-based so that the JSON reader will
// correctly parse the collection. Then pass the original literal text to the token.
string bracketLiteralText = originalLiteralText;
if (bracketLiteralText[0] == '(')
string bracketLiteralText = literalToken.OriginalText;

if (bracketLiteralText[0] == '(' || bracketLiteralText[0] == '[')
{
Debug.Assert(bracketLiteralText[bracketLiteralText.Length - 1] == ')',
"Collection with opening '(' should have corresponding ')'");
Debug.Assert((bracketLiteralText[0] == '(' && bracketLiteralText[^1] == ')') || (bracketLiteralText[0] == '[' && bracketLiteralText[^1] == ']'),
$"Collection with opening '{bracketLiteralText[0]}' should have corresponding '{(bracketLiteralText[0] == '(' ? ')' : ']')}'");

StringBuilder replacedText = new StringBuilder(bracketLiteralText);
replacedText[0] = '[';
replacedText[replacedText.Length - 1] = ']';
bracketLiteralText = replacedText.ToString();
if (bracketLiteralText[0] == '(' && bracketLiteralText[^1] == ')')
{
bracketLiteralText = string.Create(bracketLiteralText.Length, bracketLiteralText, (span, state) =>
{
state.AsSpan().CopyTo(span);
span[0] = '[';
span[^1] = ']';
});
}

Debug.Assert(expectedType.IsCollection());
string expectedTypeFullName = expectedType.Definition.AsElementType().FullTypeName();
Expand Down Expand Up @@ -155,7 +159,7 @@ private CollectionNode GetCollectionOperandFromToken(QueryToken queryToken, IEdm
}

object collection = ODataUriConversionUtils.ConvertFromCollectionValue(bracketLiteralText, model, expectedType);
LiteralToken collectionLiteralToken = new LiteralToken(collection, originalLiteralText, expectedType);
LiteralToken collectionLiteralToken = new LiteralToken(collection, literalToken.OriginalText, expectedType);
operand = this.bindMethod(collectionLiteralToken) as CollectionConstantNode;
}
else
Expand Down
31 changes: 31 additions & 0 deletions ...Tests/Tests/Client/Microsoft.OData.Client.E2E.Tests/ClientTests/Tests/ClientQueryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,37 @@ public async Task DollarFilter_UsingContains_ExecutesSuccessfully(string query,
Assert.Equal(expectedCount, result.Length);
}

[Theory]
[InlineData("People?$filter=Name in ('')")]
[InlineData("People?$filter=Name in ['']")]
[InlineData("People?$filter=Name in ( '' )")]
[InlineData("People?$filter=Name in [ '' ]")]
[InlineData("People?$filter=Name in (\"\")")]
[InlineData("People?$filter=Name in [\"\"]")]
[InlineData("People?$filter=Name in ( \"\" )")]
[InlineData("People?$filter=Name in [ \"\" ]")]
[InlineData("People?$filter=Name in ( ' ' )")]
[InlineData("People?$filter=Name in [ ' ' ]")]
[InlineData("People?$filter=Name in ( \" \" )")]
[InlineData("People?$filter=Name in [ \" \"]")]
[InlineData("People?$filter=Name in ( '', ' ' )")]
[InlineData("People?$filter=Name in [ '', ' ' ]")]
[InlineData("People?$filter=Name in ( \"\", \" \" )")]
[InlineData("People?$filter=Name in [ \"\", \" \" ]")]
[InlineData("People?$filter=Name in ( '', \" \" )")]
[InlineData("People?$filter=Name in [ '', \" \" ]")]
[InlineData("People?$filter=Name in ( \"\", ' ' )")]
[InlineData("People?$filter=Name in [ \"\", ' ' ]")]
[InlineData("People?$filter=Name in [ 'null', 'null' ]")]
public async Task DollarFilter_WithCollectionWithEmptyString_ExecutesSuccessfully(string query)
{
// Act
var response = await _context.ExecuteAsync<Common.Clients.EndToEnd.Person>(new Uri(_baseUri.OriginalString + query));

// Assert
Assert.Empty(response.ToArray());
}

[Fact]
public async Task Using_LinqContains_ExecutesSuccessfully()
{
Expand Down
10 changes: 10 additions & 0 deletions ...Tests/Microsoft.OData.Core.Tests/ScenarioTests/UriBuilder/FilterAndOrderByBuilderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,16 @@ public void BuildFilterWithInOperatorUsingBracketedCollectionConstant()
Assert.Equal(new Uri("http://gobbledygook/People?$filter=ID%20in%20[1%2C2%2C3]"), actualUri, new UriComparer<Uri>());
}

[Theory]
[InlineData("People?$filter=Name in ('')", "http://gobbledygook/People?$filter=Name%20in%20(%27%27)")]
[InlineData("People?$filter=Name in ['']", "http://gobbledygook/People?$filter=Name%20in%20[%27%27]")]
public void BuildFilterWithInOperatorUsingCollectionWithEmptyString(string filterQuery, string expectedQuery)
{
Uri queryUri = new Uri(filterQuery, UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal(new Uri(expectedQuery), actualUri, new UriComparer<Uri>());
}

[Fact]
public void BuildFilterWithInOperatorUsingSingleConstant()
{
Expand Down
12 changes: 8 additions & 4 deletions ...onalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriBuilder/SelectExpandBuilderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -668,12 +668,16 @@ public void ExpandWithDollarItInFilterComplexBinaryOperatorShouldWork()
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($filter=$it/MyAddress/City eq 'Seattle')"), actualUri.OriginalString);
}

[Fact]
public void ExpandWithDollarItInFilterInOperatorShouldWork()
[Theory]
[InlineData("People?$expand=MyDog($filter=$it/ID in ['1', '2', '3'])", "MyDog($filter=$it/ID in ['1', '2', '3'])")]
[InlineData("People?$expand=MyDog($filter=$it/ID in ('1', '2', '3'))", "MyDog($filter=$it/ID in ('1', '2', '3'))")]
[InlineData("People?$expand=MyDog($filter=$it/Name in (''))", "MyDog($filter=$it/Name in (''))")]
[InlineData("People?$expand=MyDog($filter=$it/Name in [''])", "MyDog($filter=$it/Name in [''])")]
public void ExpandWithDollarItInFilterInOperatorShouldWork(string filterQuery, string expectedSubQuery)
{
Uri queryUri = new Uri("People?$expand=MyDog($filter=$it/ID in ['1', '2', '3'])", UriKind.Relative);
Uri queryUri = new Uri(filterQuery, UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($filter=$it/ID in ['1', '2', '3'])"), actualUri.OriginalString);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString(expectedSubQuery), actualUri.OriginalString);
}

[Fact]
Expand Down
107 changes: 106 additions & 1 deletion ...sts/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2212,45 +2212,86 @@ public void FilterWithInOperationWithParensStringCollection_EscapedSingleQuote()

[Theory]
[InlineData("('a''bc')", "('a''bc')", 1)]
[InlineData("['a''bc']", "['a''bc']", 1)]
[InlineData("('''def')", "('''def')", 1)]
[InlineData("['''def']", "['''def']", 1)]
[InlineData("('xyz''')", "('xyz''')", 1)]
[InlineData("['xyz''']", "['xyz''']", 1)]
[InlineData("('''pqr''')", "('''pqr''')", 1)]
[InlineData("['''pqr''']", "['''pqr''']", 1)]
[InlineData("('a''bc','''def')", "('a''bc','''def')", 2)]
[InlineData("['a''bc','''def']", "['a''bc','''def']", 2)]
[InlineData("('a''bc','xyz''')", "('a''bc','xyz''')", 2)]
[InlineData("['a''bc','xyz''']", "['a''bc','xyz''']", 2)]
[InlineData("('a''bc','''pqr''')", "('a''bc','''pqr''')", 2)]
[InlineData("['a''bc','''pqr''']", "['a''bc','''pqr''']", 2)]
[InlineData("('''def','a''bc')", "('''def','a''bc')", 2)]
[InlineData("['''def','a''bc']", "['''def','a''bc']", 2)]
[InlineData("('''def','xyz''')", "('''def','xyz''')", 2)]
[InlineData("['''def','xyz''']", "['''def','xyz''']", 2)]
[InlineData("('''def','''pqr''')", "('''def','''pqr''')", 2)]
[InlineData("['''def','''pqr''']", "['''def','''pqr''']", 2)]
[InlineData("('xyz''','a''bc')", "('xyz''','a''bc')", 2)]
[InlineData("['xyz''','a''bc']", "['xyz''','a''bc']", 2)]
[InlineData("('xyz''','''def')", "('xyz''','''def')", 2)]
[InlineData("['xyz''','''def']", "['xyz''','''def']", 2)]
[InlineData("('xyz''','''pqr''')", "('xyz''','''pqr''')", 2)]
[InlineData("['xyz''','''pqr''']", "['xyz''','''pqr''']", 2)]
[InlineData("('''pqr''','a''bc')", "('''pqr''','a''bc')", 2)]
[InlineData("['''pqr''','a''bc']", "['''pqr''','a''bc']", 2)]
[InlineData("('''pqr''','''def')", "('''pqr''','''def')", 2)]
[InlineData("['''pqr''','''def']", "['''pqr''','''def']", 2)]
[InlineData("('''pqr''','xyz''')", "('''pqr''','xyz''')", 2)]
[InlineData("['''pqr''','xyz''']", "['''pqr''','xyz''']", 2)]
[InlineData("('a''bc','''def','xyz''')", "('a''bc','''def','xyz''')", 3)]
[InlineData("['a''bc','''def','xyz''']", "['a''bc','''def','xyz''']", 3)]
[InlineData("('a''bc','''def','''pqr''')", "('a''bc','''def','''pqr''')", 3)]
[InlineData("['a''bc','''def','''pqr''']", "['a''bc','''def','''pqr''']", 3)]
[InlineData("('a''bc','xyz''','''def')", "('a''bc','xyz''','''def')", 3)]
[InlineData("['a''bc','xyz''','''def']", "['a''bc','xyz''','''def']", 3)]
[InlineData("('a''bc','xyz''','''pqr''')", "('a''bc','xyz''','''pqr''')", 3)]
[InlineData("['a''bc','xyz''','''pqr''']", "['a''bc','xyz''','''pqr''']", 3)]
[InlineData("('a''bc','''pqr''','''def')", "('a''bc','''pqr''','''def')", 3)]
[InlineData("['a''bc','''pqr''','''def']", "['a''bc','''pqr''','''def']", 3)]
[InlineData("('a''bc','''pqr''','xyz''')", "('a''bc','''pqr''','xyz''')", 3)]
[InlineData("['a''bc','''pqr''','xyz''']", "['a''bc','''pqr''','xyz''']", 3)]
[InlineData("('''def','a''bc','xyz''')", "('''def','a''bc','xyz''')", 3)]
[InlineData("['''def','a''bc','xyz''']", "['''def','a''bc','xyz''']", 3)]
[InlineData("('''def','a''bc','''pqr''')", "('''def','a''bc','''pqr''')", 3)]
[InlineData("['''def','a''bc','''pqr''']", "['''def','a''bc','''pqr''']", 3)]
[InlineData("('''def','xyz''','a''bc')", "('''def','xyz''','a''bc')", 3)]
[InlineData("['''def','xyz''','a''bc']", "['''def','xyz''','a''bc']", 3)]
[InlineData("('''def','xyz''','''pqr''')", "('''def','xyz''','''pqr''')", 3)]
[InlineData("['''def','xyz''','''pqr''']", "['''def','xyz''','''pqr''']", 3)]
[InlineData("('''def','''pqr''','a''bc')", "('''def','''pqr''','a''bc')", 3)]
[InlineData("['''def','''pqr''','a''bc']", "['''def','''pqr''','a''bc']", 3)]
[InlineData("('''def','''pqr''','xyz''')", "('''def','''pqr''','xyz''')", 3)]
[InlineData("['''def','''pqr''','xyz''']", "['''def','''pqr''','xyz''']", 3)]
[InlineData("('xyz''','a''bc','''def')", "('xyz''','a''bc','''def')", 3)]
[InlineData("['xyz''','a''bc','''def']", "['xyz''','a''bc','''def']", 3)]
[InlineData("('xyz''','a''bc','''pqr''')", "('xyz''','a''bc','''pqr''')", 3)]
[InlineData("['xyz''','a''bc','''pqr''']", "['xyz''','a''bc','''pqr''']", 3)]
[InlineData("('xyz''','''def','''pqr''')", "('xyz''','''def','''pqr''')", 3)]
[InlineData("['xyz''','''def','''pqr''']", "['xyz''','''def','''pqr''']", 3)]
[InlineData("('xyz''','''def','a''bc')", "('xyz''','''def','a''bc')", 3)]
[InlineData("['xyz''','''def','a''bc']", "['xyz''','''def','a''bc']", 3)]
[InlineData("('xyz''','''pqr''','a''bc')", "('xyz''','''pqr''','a''bc')", 3)]
[InlineData("['xyz''','''pqr''','a''bc']", "['xyz''','''pqr''','a''bc']", 3)]
[InlineData("('xyz''','''pqr''','''def')", "('xyz''','''pqr''','''def')", 3)]
[InlineData("['xyz''','''pqr''','''def']", "['xyz''','''pqr''','''def']", 3)]
[InlineData("('''pqr''','a''bc','''def')", "('''pqr''','a''bc','''def')", 3)]
[InlineData("['''pqr''','a''bc','''def']", "['''pqr''','a''bc','''def']", 3)]
[InlineData("('''pqr''','a''bc','xyz''')", "('''pqr''','a''bc','xyz''')", 3)]
[InlineData("['''pqr''','a''bc','xyz''']", "['''pqr''','a''bc','xyz''']", 3)]
[InlineData("('''pqr''','''def','a''bc')", "('''pqr''','''def','a''bc')", 3)]
[InlineData("['''pqr''','''def','a''bc']", "['''pqr''','''def','a''bc']", 3)]
[InlineData("('''pqr''','''def','xyz''')", "('''pqr''','''def','xyz''')", 3)]
[InlineData("['''pqr''','''def','xyz''']", "['''pqr''','''def','xyz''']", 3)]
[InlineData("('''pqr''','xyz''','a''bc')", "('''pqr''','xyz''','a''bc')", 3)]
[InlineData("['''pqr''','xyz''','a''bc']", "['''pqr''','xyz''','a''bc']", 3)]
[InlineData("('''pqr''','xyz''','''def')", "('''pqr''','xyz''','''def')", 3)]
[InlineData("['''pqr''','xyz''','''def']", "['''pqr''','xyz''','''def']", 3)]

public void FilterWithInExpressionContainingEscapedSingleQuotes(string inExpr, string parsedExpr, int count)
{
FilterClause filter = ParseFilter($"SSN in {inExpr}", HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());
Expand Down Expand Up @@ -2516,6 +2557,24 @@ public void FilterWithInOperationWithEmptyString(string filterClause)
Assert.Equal("\"\"", constantNode.LiteralText);
}

[Theory]
[InlineData("SSN in ['']")] // Edm.String
[InlineData("SSN in [ '' ]")] // Edm.String
[InlineData("SSN in [\"\"]")] // Edm.String
[InlineData("SSN in [ \"\" ]")] // Edm.String
public void FilterWithInOperationWithEmptyStringInSquareBrackets(string filterClause)
{
FilterClause filter = ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());

var inNode = Assert.IsType<InNode>(filter.Expression);

CollectionConstantNode collectionNode = Assert.IsType<CollectionConstantNode>(inNode.Right);
Assert.Equal(1, collectionNode.Collection.Count);

ConstantNode constantNode = collectionNode.Collection.First();
Assert.Equal("\"\"", constantNode.LiteralText);
}

[Theory]
[InlineData("SSN in ( ' ' )", " ")] // 1 space
[InlineData("SSN in ( ' ' )", " ")] // 3 spaces
Expand All @@ -2530,7 +2589,27 @@ public void FilterWithInOperationWithWhitespace(string filterClause, string expe
CollectionConstantNode collectionNode = Assert.IsType<CollectionConstantNode>(inNode.Right);

// A single whitespace or multiple whitespaces are valid literals
Assert.Equal(1, collectionNode.Collection.Count);
Assert.Single(collectionNode.Collection);

ConstantNode constantNode = collectionNode.Collection.First();
Assert.Equal(expectedLiteralText, constantNode.LiteralText);
}

[Theory]
[InlineData("SSN in [ ' ' ]", " ")] // 1 space
[InlineData("SSN in [ ' ' ]", " ")] // 3 spaces
[InlineData("SSN in [ \" \" ]", " ")] // 2 spaces
[InlineData("SSN in [ \" \" ]", " ")] // 4 spaces
public void FilterWithInOperationWithWhitespaceInSquareBrackets(string filterClause, string expectedLiteralText)
{
FilterClause filter = ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());

var inNode = Assert.IsType<InNode>(filter.Expression);

CollectionConstantNode collectionNode = Assert.IsType<CollectionConstantNode>(inNode.Right);

// A single whitespace or multiple whitespaces are valid literals
Assert.Single(collectionNode.Collection);

ConstantNode constantNode = collectionNode.Collection.First();
Assert.Equal(expectedLiteralText, constantNode.LiteralText);
Expand All @@ -2553,10 +2632,30 @@ public void FilterWithInOperationWithEmptyStringAndWhitespace(string filterClaus
Assert.Equal(2, collectionNode.Collection.Count);
}

[Theory]
[InlineData("SSN in [ '', ' ' ]")] // Edm.String
[InlineData("SSN in [ \"\", \" \" ]")] // Edm.String
[InlineData("SSN in [ '', \" \" ]")] // Edm.String
[InlineData("SSN in [ \"\", ' ' ]")] // Edm.String
public void FilterWithInOperationWithEmptyStringAndWhitespaceInSquareBrackets(string filterClause)
{
FilterClause filter = ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());

var inNode = Assert.IsType<InNode>(filter.Expression);

CollectionConstantNode collectionNode = Assert.IsType<CollectionConstantNode>(inNode.Right);

// A single whitespace or multiple whitespaces are valid literals
Assert.Equal(2, collectionNode.Collection.Count);
}

[Theory]
[InlineData("MyGuid in ( '' )", "")] // Edm.Guid
[InlineData("MyGuid in ( ' ' )", " ")] // Edm.Guid
[InlineData("MyGuid in ( \" \" )", " ")] // Edm.Guid
[InlineData("MyGuid in [ '' ]", "")] // Edm.Guid
[InlineData("MyGuid in [ ' ' ]", " ")] // Edm.Guid
[InlineData("MyGuid in [ \" \" ]", " ")] // Edm.Guid
public void FilterWithInOperationGuidWithEmptyQuotesThrows(string filterClause, string quotedString)
{
Action parse = () => ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());
Expand All @@ -2567,6 +2666,9 @@ public void FilterWithInOperationGuidWithEmptyQuotesThrows(string filterClause,
[InlineData("Birthdate in ( '' )", "")] // Edm.DateTimeOffset
[InlineData("Birthdate in ( \" \" )", " ")] // Edm.DateTimeOffset
[InlineData("Birthdate in (' ')", " ")] // Edm.DateTimeOffset
[InlineData("Birthdate in [ '' ]", "")] // Edm.DateTimeOffset
[InlineData("Birthdate in [ \" \" ]", " ")] // Edm.DateTimeOffset
[InlineData("Birthdate in [' ']", " ")] // Edm.DateTimeOffset
public void FilterWithInOperationDateTimeOffsetWithEmptyQuotesThrows(string filterClause, string quotedString)
{
Action parse = () => ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());
Expand All @@ -2577,6 +2679,9 @@ public void FilterWithInOperationDateTimeOffsetWithEmptyQuotesThrows(string filt
[InlineData("MyDate in ( '' )", "")] // Edm.Date
[InlineData("MyDate in ( \" \" )", " ")] // Edm.Date
[InlineData("MyDate in (' ')", " ")] // Edm.Date
[InlineData("MyDate in [ '' ]", "")] // Edm.Date
[InlineData("MyDate in [ \" \" ]", " ")] // Edm.Date
[InlineData("MyDate in [' ']", " ")] // Edm.Date
public void FilterWithInOperationDateWithEmptyQuotesThrows(string filterClause, string quotedString)
{
Action parse = () => ParseFilter(filterClause, HardCodedTestModel.TestModel, HardCodedTestModel.GetPersonType());
Expand Down

0 comments on commit 5e715a2

Please sign in to comment.