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

Add Dollar it in BuildUri response #1991

Merged
merged 11 commits into from
Feb 15, 2021
Merged

Add Dollar it in BuildUri response #1991

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
718c958
Add Dollar it in BuildUri response
KenitoInc Jan 27, 2021
9cee5ea
Add SingleValueFunctionCall logic
KenitoInc Feb 1, 2021
a1df6b2
Add AnyNode logic
KenitoInc Feb 1, 2021
985eed6
Add SingleValueOpenPropertyAccess logic
KenitoInc Feb 2, 2021
558ffba
Code cleanup
KenitoInc Feb 2, 2021
12fdbe8
Comment ParameterAlias
KenitoInc Feb 2, 2021
f8930bf
Add expand and filter unit tests
KenitoInc Feb 3, 2021
e0fc7f2
Add more tests and fix edge cases
KenitoInc Feb 3, 2021
cf66923
Refractor code
KenitoInc Feb 4, 2021
f60c37c
Add doc strings
KenitoInc Feb 5, 2021
5234ec1
Fix review comments
KenitoInc Feb 11, 2021
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
236 changes: 235 additions & 1 deletion src/Microsoft.OData.Core/Uri/NodeToStringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.OData
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using Microsoft.OData.Edm;
Expand Down Expand Up @@ -470,7 +471,240 @@ internal String TranslateNode(QueryNode node)
internal String TranslateFilterClause(FilterClause filterClause)
{
Debug.Assert(filterClause != null, "filterClause != null");
return this.TranslateNode(filterClause.Expression);

return BindNode(filterClause.Expression, filterClause.RangeVariable as ResourceRangeVariable);
}

/// <summary>
/// We return the <see cref="ResourceRangeVariableReferenceNode"/> within a <see cref="QueryNode"/>
/// </summary>
/// <param name="node">The node to extract the ResourceRangeVariableReferenceNode.</param>
/// <returns>The extracted ResourceRangeVariableReferenceNode.</returns>
private ResourceRangeVariableReferenceNode GetResourceRangeVariableReferenceNode(QueryNode node)
{
switch(node.Kind)
{
case QueryNodeKind.SingleValuePropertyAccess:
SingleValuePropertyAccessNode singleValuePropertyAccessNode = node as SingleValuePropertyAccessNode;
return GetResourceRangeVariableReferenceNode(singleValuePropertyAccessNode.Source);

case QueryNodeKind.Convert:
ConvertNode convertNode = node as ConvertNode;
return GetResourceRangeVariableReferenceNode(convertNode.Source);

case QueryNodeKind.Any:
AnyNode anyNode = node as AnyNode;
return GetResourceRangeVariableReferenceNode(anyNode.Source);

case QueryNodeKind.SingleValueFunctionCall:
SingleValueFunctionCallNode singleValueFunctionCallNode = node as SingleValueFunctionCallNode;
return GetResourceRangeVariableReferenceNode(singleValueFunctionCallNode.Parameters.First());

case QueryNodeKind.ResourceRangeVariableReference:
return node as ResourceRangeVariableReferenceNode;

case QueryNodeKind.SingleValueOpenPropertyAccess:
SingleValueOpenPropertyAccessNode singleValueOpenPropertyAccessNode = node as SingleValueOpenPropertyAccessNode;
return GetResourceRangeVariableReferenceNode(singleValueOpenPropertyAccessNode.Source);

case QueryNodeKind.SingleComplexNode:
SingleComplexNode singleComplexNode = node as SingleComplexNode;
return GetResourceRangeVariableReferenceNode(singleComplexNode.Source);

case QueryNodeKind.CollectionComplexNode:
CollectionComplexNode collectionComplexNode = node as CollectionComplexNode;
return GetResourceRangeVariableReferenceNode(collectionComplexNode.Source);
}

return null;
}

/// <summary>
/// Bind $it to the <see cref="QueryNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindNode(QueryNode node, ResourceRangeVariable filterClauseRangeVariable)
{
BinaryOperatorNode binaryNode = node as BinaryOperatorNode;
InNode inNode = node as InNode;
AnyNode anyNode = node as AnyNode;
SingleValueFunctionCallNode singleValueFunctionCallNode = node as SingleValueFunctionCallNode;
SingleValueOpenPropertyAccessNode singleValueOpenPropertyAccessNode = node as SingleValueOpenPropertyAccessNode;

if (binaryNode != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively,

BinaryOperatorNode binaryNode;
// ... other variable declarations

if ((binaryNode = node as BinaryOperatorNode) != null)
{
   // ...
}
// else if ...

I however don't know if the cost of doing all 5 explicit conversions using the as operator regardless is significant. In this case it might not matter since the most common scenario is handled by the else block and all the explicit conversions will have been done either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me hear the opinions of others on this.

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought -- we want to limit the number of casts.

Can we switch on QueryNode.Kind (or QueryNode.InternalKind)?

In reply to: 574336006 [](ancestors = 574336006)

{
return BindBinaryOperatorNode(binaryNode, filterClauseRangeVariable);
}
else if (inNode != null)
{
return BindInNode(inNode, filterClauseRangeVariable);
}
else if (anyNode != null)
{
return BindAnyNode(anyNode, filterClauseRangeVariable);
}
else if (singleValueFunctionCallNode != null)
{
return BindSingleValueFunctionCallNode(singleValueFunctionCallNode, filterClauseRangeVariable);
}
else if (singleValueOpenPropertyAccessNode != null)
{
return BindSingleValueOpenPropertyAccess(singleValueOpenPropertyAccessNode, filterClauseRangeVariable);
}
else
{
return this.TranslateNode(node);
}
}

/// <summary>
/// Bind $it to the <see cref="BinaryOperatorNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindBinaryOperatorNode(BinaryOperatorNode node, ResourceRangeVariable filterClauseRangeVariable)
{
string translatedNode = this.TranslateNode(node);
ResourceRangeVariableReferenceNode leftRangeVariableNode = GetResourceRangeVariableReferenceNode(node.Left);
ResourceRangeVariableReferenceNode rightRangeVariableNode = GetResourceRangeVariableReferenceNode(node.Right);

BinaryOperatorKind binaryNodeKind = node.OperatorKind;

string binaryOperator = this.BinaryOperatorNodeToString(binaryNodeKind);
string[] binarySeparator = { binaryOperator };
string[] substrings = translatedNode.Trim().Split(binarySeparator, StringSplitOptions.RemoveEmptyEntries);
string leftBinary = substrings[0].Trim();
string rightBinary = substrings[1].Trim();

if (leftRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, leftRangeVariableNode))
Copy link
Member

Choose a reason for hiding this comment

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

IsDifferentSource(filterClauseRangeVariable, leftRangeVariableNode) [](start = 49, length = 67)

So this logic is assuming that, if the range variable is a different source, the range variable must be $it, right? In which case, $this would work because it's the same type as the filterClauseRangeVariable so it will just fall through? How would this logic change when if we supported $root?

Minimally, we need to document these assumptions and add tests (if they don't already exist) for these different cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

{
leftBinary = "$it/" + leftBinary;
}

if (rightRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, rightRangeVariableNode))
{
rightBinary = "$it/" + rightBinary;
}

return leftBinary + ' ' + binaryOperator + ' ' + rightBinary;
}

/// <summary>
/// Bind $it to the <see cref="InNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindInNode(InNode node, ResourceRangeVariable filterClauseRangeVariable)
{
string translatedNode = this.TranslateNode(node);
ResourceRangeVariableReferenceNode leftRangeVariableNode = GetResourceRangeVariableReferenceNode(node.Left);

string inOperator = ExpressionConstants.KeywordIn;
string[] inSeparator = { inOperator };
string[] substrings = translatedNode.Trim().Split(inSeparator, StringSplitOptions.RemoveEmptyEntries);
string leftIn = substrings[0];
string rightIn = substrings[1];
Copy link
Member

Choose a reason for hiding this comment

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

This logic only needs to execute if the sources are different, so it would be better to move it within your if statement.


if (leftRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, leftRangeVariableNode))
{
leftIn = "$it/" + leftIn;
translatedNode = leftIn + inOperator + rightIn;
}
return translatedNode;
}

/// <summary>
/// Bind $it to the <see cref="AnyNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindAnyNode(AnyNode node, ResourceRangeVariable filterClauseRangeVariable)
{
string translatedNode = this.TranslateNode(node);
ResourceRangeVariableReferenceNode leftRangeVariableNode = GetResourceRangeVariableReferenceNode(node);

const string slashAny = "/any";
string[] anySeparator = { slashAny };
string[] substrings = translatedNode.Trim().Split(anySeparator, StringSplitOptions.RemoveEmptyEntries);
string leftAnyNodeSubstring = substrings[0];
string rightAnyNodeSubstring = substrings[1];

if (leftRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, leftRangeVariableNode))
{
leftAnyNodeSubstring = "$it/" + leftAnyNodeSubstring;
translatedNode = leftAnyNodeSubstring + slashAny + rightAnyNodeSubstring;
}
return translatedNode;
}

/// <summary>
/// Bind $it to the <see cref="SingleValueFunctionCallNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindSingleValueFunctionCallNode(SingleValueFunctionCallNode node, ResourceRangeVariable filterClauseRangeVariable)
{
string translatedNode = this.TranslateNode(node);
ResourceRangeVariableReferenceNode firstParameterRangeVariableNode = GetResourceRangeVariableReferenceNode(node);

char[] separators = { '(', ')' };
string[] subtrings = translatedNode.Trim().Split(separators, StringSplitOptions.RemoveEmptyEntries);
string withinBrackets = subtrings[1];
string[] parameters = withinBrackets.Trim().Split(',');
string leftParameter = parameters[0];
string rightParameter = parameters.Length == 2 ? parameters[1] : String.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

all of this logic should be moved inside the conditional and only executed in the case that we are inserting the $it.


if (firstParameterRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, firstParameterRangeVariableNode))
{
leftParameter = "$it/" + leftParameter;
translatedNode = parameters.Length == 1 ? subtrings[0] + '(' + leftParameter + ')' : subtrings[0] + '(' + leftParameter + ',' + rightParameter + ')';
}
return translatedNode;
}

/// <summary>
/// Bind $it to the <see cref="SingleValueOpenPropertyAccessNode"/> translated string.
/// </summary>
/// <param name="node">node to bind.</param>
/// <param name="filterClauseRangeVariable">The <see cref="FilterClause"/> range variable.</param>
/// <returns>The translated string with $it binding.</returns>
private string BindSingleValueOpenPropertyAccess(SingleValueOpenPropertyAccessNode node, ResourceRangeVariable filterClauseRangeVariable)
{
string translatedNode = this.TranslateNode(node);
ResourceRangeVariableReferenceNode rangeVariableNode = GetResourceRangeVariableReferenceNode(node);

if (rangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, rangeVariableNode))
{
translatedNode = "$it/" + translatedNode;
}
return translatedNode;
}

/// <summary>
/// Check whether Navigation source of the FilterClause rangeVariable is different from the Expression rangeVariable.
/// </summary>
/// <param name="filterClauseRangeVariable"><see cref="FilterClause"/> rangeVariable.</param>
/// <param name="rangeVariableReferenceNode">The rangeVariable of the expression.</param>
/// <returns>If Navigation Source are different, returns true. Otherwise false.</returns>
private static bool IsDifferentSource(ResourceRangeVariable filterClauseRangeVariable, ResourceRangeVariableReferenceNode rangeVariableReferenceNode)
{
Debug.Assert(filterClauseRangeVariable != null, "filterClause != null");
Debug.Assert(rangeVariableReferenceNode != null, "rangeVariableReferenceNode != null");

if (filterClauseRangeVariable.NavigationSource != rangeVariableReferenceNode.NavigationSource)
{
return true;
}
else
{
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

could simplify to:
return filterClauseRangeVariable.NavigationSource != rangeVariableReferenceNode.NavigationSource;

}

/// <summary>Translates a <see cref="OrderByClause"/> into a string.</summary>
Expand Down
12 changes: 10 additions & 2 deletions ...Tests/Microsoft.OData.Core.Tests/ScenarioTests/UriBuilder/FilterAndOrderByBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,9 +457,17 @@ public void BuildFilterWithInOperatorUsingSingleConstant()
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal(new Uri("http://gobbledygook/People?$filter=1%20in%20RelatedIDs"), actualUri);
}
#endregion

#region test $orderby
[Fact]
public void BuildFilterWithDollarItInFunctionCall()
{
Uri queryUri = new Uri("People(1)/RelatedSSNs?$filter=endswith($it,'xyz')", UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal(new Uri("http://gobbledygook/People(1)/RelatedSSNs?$filter=endswith%28%24it%2C%27xyz%27%29"), actualUri);
}
#endregion

#region test $orderby
[Fact]
public void BuildOrderByWithEntitySetShouldBeAbleToDetermineSets()
{
Expand Down
40 changes: 40 additions & 0 deletions ...onalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriBuilder/SelectExpandBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,46 @@ public void SelectWithNestedExpandShouldWork()
Assert.Equal("http://gobbledygook/People?$select=" + Uri.EscapeDataString("Name,MyOpenAddress/Test") + "&$expand=" + Uri.EscapeDataString("MyDog($filter=Color eq 'Brown';$orderby=Color;$search=Color)"), actualUri.OriginalString);
}

[Fact]
public void ExpandWithDollarItInFilterBinaryOperatorShouldWork()
{
Uri queryUri = new Uri("People?$expand=MyDog($select=Color;$expand=LionsISaw($filter=ID1 eq $it/ID))", UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($select=Color;$expand=LionsISaw($filter=ID1 eq $it/ID))"), actualUri.OriginalString);
}

[Fact]
public void ExpandWithDollarItInFilterComplexBinaryOperatorShouldWork()
{
Uri queryUri = new Uri("People?$expand=MyDog($filter=$it/MyAddress/City eq 'Seattle')", UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($filter=$it/MyAddress/City eq 'Seattle')"), actualUri.OriginalString);
}

[Fact]
public void ExpandWithDollarItInFilterInOperatorShouldWork()
{
Uri queryUri = new Uri("People?$expand=MyDog($filter=$it/ID in ['1', '2', '3'])", 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);
}

[Fact]
public void ExpandWithDollarItInFilterAnyOperatorShouldWork()
{
Uri queryUri = new Uri("People?$expand=MyDog($filter=$it/PreviousAddresses/any(d:d/City eq 'Seattle'))", UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($filter=$it/PreviousAddresses/any(d:d/City eq 'Seattle'))"), actualUri.OriginalString);
}

[Fact]
public void ExpandWithDollarItInFilterFunctionCallShouldWork()
{
Uri queryUri = new Uri("People?$expand=MyDog($filter=endswith($it/Name,'abcd'))", UriKind.Relative);
Uri actualUri = UriBuilder(queryUri, ODataUrlKeyDelimiter.Parentheses, settings);
Assert.Equal("http://gobbledygook/People?$expand=" + Uri.EscapeDataString("MyDog($filter=endswith($it/Name,'abcd'))"), actualUri.OriginalString);
}
Copy link
Member

Choose a reason for hiding this comment

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

do we have variants of each of these that don't use $it, and instead assume the range variable of the filter clause?


[Fact]
public void ExpandWithNestedQueryOptionsShouldWork()
{
Expand Down