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.

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 SelectExpandNode for navigation properties on derived/complex types #1359

Merged
merged 2 commits into from Dec 18, 2018

Conversation

Projects
None yet
3 participants
@mikepizzo
Copy link
Member

commented Dec 12, 2018

Issues

PR #1286 fixed a number of issues related to how non-selected expanded navigation properties were represented in the ContextUrl. However, in the new code that attempts to determine if the entire subtree is selected, it looks only at direct children to see if they are navigation properties and not deeper.

For example, prior to this fix the following URL would return a SelectExpandNode with SelectionType=PartialSubtree.

         Employees?$expand=ns.Manager/DirectReports

After the PR, this is correctly returned as EntireSubtree

Description

The primary purpose of this PR is to set whether the SelectNode is entiresubtree based on a deep traverse of child paths, rather than only looking at direct children.

Other changes, in addition to new tests, include some test clean-up as well as code optimizations around deferring creation of hashsets until required.

Checklist (Uncheck if it is not completed)

  • [x ] Test cases added
  • [x ] Build and test with one-click build and test script passed

@mikepizzo mikepizzo force-pushed the mikepizzo:ContextUrlNavPropFixes branch from 63b788e to 0988a28 Dec 12, 2018

@mikepizzo mikepizzo force-pushed the mikepizzo:ContextUrlNavPropFixes branch from 0988a28 to f9bfb38 Dec 12, 2018

@mikepizzo mikepizzo added this to the 7.6 milestone Dec 12, 2018

@KanishManuja-MS KanishManuja-MS self-requested a review Dec 18, 2018

DetermineSelectionType(this);
}

private static void DetermineSelectionType(SelectedPropertiesNode node)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

DetermineSelectionType [](start = 28, length = 22)

I'd suppose to move the private static method end of this class. #Closed

@KanishManuja-MS KanishManuja-MS requested a review from xuzhg Dec 18, 2018

DetermineSelectionType(this);
}

private static void DetermineSelectionType(SelectedPropertiesNode node)

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

SelectedPropertiesNode node [](start = 51, length = 27)

It seems we also can use instance method, not the static. But, either way is ok for me. Only to call this static method using "this" a little bit confused me. #Resolved

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 18, 2018

Author Member

Yes, I like that better.


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

@@ -63,8 +63,16 @@ public SelectedPropertiesNodeTests()
this.defaultContainer.AddEntitySet("Cities", cityType);
this.defaultContainer.AddEntitySet("Districts", districtType);

EdmComplexType airportType = new EdmComplexType("TestModel", "Airport");

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

airportType [](start = 27, length = 11)

should we add "airportType" into edm model? maybe i miss that. #Resolved

This comment has been minimized.

Copy link
@mikepizzo

mikepizzo Dec 18, 2018

Author Member

This and regionalAirportType seemed to be implicitly added to the model, but I made them explicit.


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

}

[Fact]
public void ExpandTokenForNavigationPropertyDerivedOnComplexShouldHaveEntireSubtree()

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

DerivedOn [](start = 52, length = 9)

Maybe "....OnDerivedComplex...." #Resolved

}

[Fact]
public void ExpandTokenForNavigationPropertyDerivedOnComplexWithSelectShouldHavePartialSubtree()

This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

DerivedOnComplex [](start = 52, length = 16)

Maybe "...OnDerivedComplex..." #Resolved

}


This comment has been minimized.

Copy link
@xuzhg

xuzhg Dec 18, 2018

Member

remove the un-used new line #Resolved

@xuzhg

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

The change looks good to me, but left some small comments. Thanks #Resolved

@mikepizzo

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

Thanks for the comments Sam; I've addressed them and a new PR is building.


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

@xuzhg xuzhg merged commit 5733ae5 into OData:master Dec 18, 2018

2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.