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

Jump to bottom

Fix Filter and Orderby in Expand applied on bound functions #2701

Open
wants to merge 1 commit into
base: release-7.x
Choose a base branch
from
Open

Fix Filter and Orderby in Expand applied on bound functions #2701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions src/Microsoft.OData.Core/UriParser/ODataPathInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//---------------------------------------------------------------------

using System.Collections.Generic;
using System.Linq;
using Microsoft.OData.Edm;

namespace Microsoft.OData.UriParser
Expand Down Expand Up @@ -36,6 +37,7 @@ public ODataPathInfo(ODataPath odataPath)

this.targetNavigationSource = lastSegment.TargetEdmNavigationSource;
this.targetEdmType = lastSegment.EdmType;

if (this.targetEdmType != null)
{
IEdmCollectionType collectionType = this.targetEdmType as IEdmCollectionType;
Expand All @@ -44,6 +46,19 @@ public ODataPathInfo(ODataPath odataPath)
this.targetEdmType = collectionType.ElementType.Definition;
}
}

// If the last segment is an OperationSegment for a bound operation and the targetNavigationSource is null,
// We use the targetNavigationSource for the previous segment.
// e.g People/My.Function.GetPeopleWithDogs()
if (this.targetNavigationSource == null && lastSegment is OperationSegment operationSegment)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where you'd check if previous is null to avoid doing a lot of work for nothing...

{
IEdmOperation operation = operationSegment.Operations.FirstOrDefault();

if (operation.IsBound & previous != null)
Comment on lines +55 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

FirstOrDefault() would return null if no element if found and therefore the Ln57 would through an error. If we are certain about getting an element, then FirstOrDefault() would be unnecessary, using First() would suffice. Better still we can use Single() to retrieve the element if we are always expecting a single element.

{
this.targetNavigationSource = previous.TargetEdmNavigationSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could previous.TargetEdmNavigationSource also be null, or that isn't an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the return type of the of the operation does not match the navigation source of the previous segment?
For example, what if we have something like Customers/NS.GetTopOrders()?$expand=Products($filter=Category eq 'Electronics') where GetTopOrders returns a collection of Order entities from an Orders entity set, which is different from Customers entity set?

Copy link
Member

@xuzhg xuzhg Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should delegate the targetNS to the NS of preview segment.
we'd figure out why ODL needs the targetNS? If the last operation doesn't have the targetNS, can we use the return type of the operation to do something? I do think we use the TargetNS to calculate the context URL, if we don't have the NS, we should use the type to generate the Context URL. @mikepizzo

Copy link
Contributor

Choose a reason for hiding this comment

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

@KenitoInc Without even getting into whether this is the right fix, previous.TargetNavigationSource is null if previous is a KeySegment - e.g. for the case where the function is bound to an entity. On the other hand, (previous as KeySegment).NavigationSource is not null.

}
}
}

this.segments = odataPath;
Expand Down
54 changes: 54 additions & 0 deletions ...alTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/SelectExpandFunctionalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1583,6 +1583,41 @@ public void DollarThisinFilterInsideSelectInsideExpandShouldReferenceSelectedIte
Assert.Equal("Edm.String", typeReference.Definition.FullTypeName()); // Nicknames is a collection of strings.
}

[Fact]
public void ParseFilter_InExpand_AppliedOnBoundFunctionResults()
{
// Arrange & Act
Uri uri = new Uri("http://gobbledygook/People/Fully.Qualified.Namespace.GetPeopleWhoHaveDogs?$expand=MyFriendsDogs($filter=ID eq 1)");
IEdmStructuredType expectedLeft = (IEdmStructuredType)HardCodedTestModel.GetDogType();

SelectExpandClause selectExpandClause = ParseSelectExpand(uri);

// Assert
ExpandedNavigationSelectItem expandedSelectItem = (ExpandedNavigationSelectItem)Assert.Single(selectExpandClause.SelectedItems); // $expand=MyFriendsDogs(...)
Assert.NotNull(expandedSelectItem.FilterOption); // $filter=ID eq 1
BinaryOperatorNode binaryOperatorNode = expandedSelectItem.FilterOption.Expression.ShouldBeBinaryOperatorNode(BinaryOperatorKind.Equal);
IEdmStructuredType left = (binaryOperatorNode.Left as SingleValuePropertyAccessNode).Property.DeclaringType;
string right = (binaryOperatorNode.Right as ConstantNode).LiteralText;

Assert.Equal(expectedLeft, left);
Assert.Equal("1", right);
}

[Fact]
public void ParseOrderBy_InExpand_AppliedOnBoundFunctionResults()
{
// Arrange & Act
Uri uri = new Uri("http://gobbledygook/People/Fully.Qualified.Namespace.GetPeopleWhoHaveDogs?$expand=MyFriendsDogs($orderby=ID)");

SelectExpandClause selectExpandClause = ParseSelectExpand(uri);
// Assert
ExpandedNavigationSelectItem expandedSelectItem = (ExpandedNavigationSelectItem)Assert.Single(selectExpandClause.SelectedItems); // $expand=MyFriendsDogs(...)
Assert.NotNull(expandedSelectItem.OrderByOption); // $orderby=ID

SingleValuePropertyAccessNode singleValuePropertyAccessNode = expandedSelectItem.OrderByOption.Expression.ShouldBeSingleValuePropertyAccessQueryNode(HardCodedTestModel.GetDogIdProp());
Assert.Equal(OrderByDirection.Ascending, expandedSelectItem.OrderByOption.Direction);
}

[Fact]
public void SelectAndExpandShouldFailOnSelectWrongComplexProperties()
{
Expand Down Expand Up @@ -2402,6 +2437,25 @@ private static SelectItem ParseSingleExpand(string expand, IEdmEntityType entity

return expandedSelectionItem;
}

private static SelectExpandClause ParseSelectExpand(Uri uri)
{
List<CustomQueryOptionToken> queries = Microsoft.OData.UriParser.QueryOptionUtils.ParseQueryOptions(uri);
ODataUriParser parser = new ODataUriParser(HardCodedTestModel.TestModel, new Uri("http://gobbledygook/"), uri);

ODataPath path = parser.ParsePath();
var dic = queries.ToDictionary(customQueryOptionToken => customQueryOptionToken.Name, customQueryOptionToken => queries.GetQueryOptionValue(customQueryOptionToken.Name));

ODataQueryOptionParser queryOptionParser = new ODataQueryOptionParser(HardCodedTestModel.TestModel, path, dic)
{
Configuration = { ParameterAliasValueAccessor = parser.ParameterAliasValueAccessor }
};

FilterClause filterClause = queryOptionParser.ParseFilter();
SelectExpandClause selectExpandClause = queryOptionParser.ParseSelectAndExpand();

return selectExpandClause;
}
#endregion

#region Stringify Object Model Helpers
Expand Down