Add Dollar it in BuildUri response #1991
Conversation
|
|
||
| if (rightBinaryNode != null && IsDifferentSource(filterClause, rightBinaryNode)) | ||
| { | ||
| rightBinary = "$it/" + rightBinary; |
There was a problem hiding this comment.
I am worry about the implementation here, does it only work for one level? I mean $it/Product? Do we work: $it/Address/City
There was a problem hiding this comment.
It should support $it/Address/City
I will add tests to validate that.
| rightBinary = "$it/" + rightBinary; | ||
| } | ||
|
|
||
| translatedNode = leftBinary + binaryOperator + rightBinary; |
There was a problem hiding this comment.
translatedNode = leftBinary + binaryOperator + rightBinary; [](start = 20, length = 59)
If it's not a difference source, we should return the existing string "translateNode".
| return this.TranslateNode(filterClause.Expression); | ||
| string translatedNode = this.TranslateNode(filterClause.Expression); | ||
|
|
||
| switch (filterClause.Expression.Kind) |
There was a problem hiding this comment.
can we refactor the implementation here?
- separate the whole into methods
- Use the traverse, for example: https://github.com/OData/WebApi/blob/master/src/Microsoft.AspNet.OData.Shared/Query/Expressions/FilterBinder.cs#L184
KenitoInc
requested review from
ElizabethOkerio,
gathogojr,
habbes,
marabooy and
mikepizzo
KenitoInc
added
the
Ready for review
| SingleValueFunctionCallNode singleValueFunctionCallNode = node as SingleValueFunctionCallNode; | ||
| SingleValueOpenPropertyAccessNode singleValueOpenPropertyAccessNode = node as SingleValueOpenPropertyAccessNode; | ||
|
|
||
| if (binaryNode != null) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let me hear the opinions of others on this.
There was a problem hiding this comment.
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)
| string leftBinary = substrings[0].Trim(); | ||
| string rightBinary = substrings[1].Trim(); | ||
|
|
||
| if (leftRangeVariableNode != null && IsDifferentSource(filterClauseRangeVariable, leftRangeVariableNode)) |
There was a problem hiding this comment.
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.
| string[] inSeparator = { inOperator }; | ||
| string[] substrings = translatedNode.Trim().Split(inSeparator, StringSplitOptions.RemoveEmptyEntries); | ||
| string leftIn = substrings[0]; | ||
| string rightIn = substrings[1]; |
There was a problem hiding this comment.
This logic only needs to execute if the sources are different, so it would be better to move it within your if statement.
| char[] parameterSeparators = { ',' }; | ||
| string[] parameters = withinBrackets.Trim().Split(parameterSeparators, StringSplitOptions.RemoveEmptyEntries); | ||
| string leftParameter = parameters[0]; | ||
| string rightParameter = parameters.Length == 2 ? parameters[1] : String.Empty; |
There was a problem hiding this comment.
all of this logic should be moved inside the conditional and only executed in the case that we are inserting the $it.
| else | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
could simplify to:
return filterClauseRangeVariable.NavigationSource != rangeVariableReferenceNode.NavigationSource;
| 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); | ||
| } |
There was a problem hiding this comment.
do we have variants of each of these that don't use $it, and instead assume the range variable of the filter clause?
|
A few comments, otherwise looks good. |
Issues
This pull request fixes issue #1974 .
Description
Briefly describe the changes of this pull request.
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.