-
Notifications
You must be signed in to change notification settings - Fork 352
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
Context url for expand #1286
Context url for expand #1286
Conversation
2549c22
to
27c8593
Compare
return string.IsNullOrEmpty(subExpand) && version <= ODataVersion.V4 ? null : | ||
expandNode + ODataConstants.ContextUriProjectionStart + subExpand + ODataConstants.ContextUriProjectionEnd; | ||
|
||
// TODO: cleanup the version parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: cleanup the version parameter. [](start = 12, length = 39)
// TODO: cleanup the version parameter. [](start = 12, length = 39)
what work is left? not passing the version parameter to the method? do you have an issue filed for that? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is just potential cleanups from caller side, wherever applicable, after reviewing changes in this PR and confirming that the dependency on version (V4/V401) should be removed. The cleanup should be straight forward when this PR is finalized. #Closed
{ | ||
// Qualified name: try to match bound and unbound operations | ||
found = this.edmModel.FindBoundOperations(token, this.structuredType).Count() != 0 | ||
|| this.edmModel.FindDeclaredOperations(token).Count() != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| this.edmModel.FindDeclaredOperations(token).Count() != 0 [](start = 20, length = 59)
Do you have a test case for this? I don't think there should ever be unbound operations nested within parens. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In SelectedPropertiesNodeTest, we have tests exercising unbound operations using qualified name such as TestModel.Action(Edm.Int32).
In reply to: 224137533 [](ancestors = 224137533)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that test is valid; the only time an operation should exist within a select list would be if it was bound to the structured type against which the select list is applied. Otherwise, the fact that there is an unbound operation with the same name is purely coincidental, and we should only resolve it as an operation if it can't be a navigation property (i.e., there is a dot in the name). So, in this case, I think the only valid interpretation would be a dynamic bound property that happened to have the same name as a defined, unbound property.
In reply to: 224629515 [](ancestors = 224629515,224137533)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsBoundOperationToken method has been replaced by the IsNavigationPropertyToken.
In reply to: 226138497 [](ancestors = 226138497,224629515,224137533)
found = this.edmModel.FindBoundOperations(token, this.structuredType).Count() != 0 | ||
|| this.edmModel.FindDeclaredOperations(token).Count() != 0; | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra line #Closed
int idx = token.LastIndexOf(nameSpaceSeparator); | ||
if (idx == -1) | ||
{ | ||
// Token is in unqualified form, try matching the unqualified name of the bound operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token is in unqualified form, try matching the unqualified name of the bound operation. [](start = 19, length = 87)
Should probably add comment that, according to spec, the name should always be qualified, but that older versions of the library returned unqualified. And, we should fix the library such that it no longer returns unqualified names in the contextUrl. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -106,12 +106,10 @@ internal static void Traverse<T>(this SelectExpandClause selectExpandClause, Fun | |||
{ | |||
string currentExpandClause = String.Join("/", expandSelectItem.PathToNavigationProperty.WalkWith(PathSegmentToStringTranslator.Instance).ToArray()); | |||
T subResult = default(T); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= default(T) [](start = 27, length = 13)
Since you are no longer conditionally calling Traverse, do you still need to initialize subResult? #Closed
@@ -106,12 +106,10 @@ internal static void Traverse<T>(this SelectExpandClause selectExpandClause, Fun | |||
{ | |||
string currentExpandClause = String.Join("/", expandSelectItem.PathToNavigationProperty.WalkWith(PathSegmentToStringTranslator.Instance).ToArray()); | |||
T subResult = default(T); | |||
if (expandSelectItem.SelectAndExpand.SelectedItems.Any()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expandSelectItem.SelectAndExpand.SelectedItems.Any()) [](start = 20, length = 53)
expandSelectItem.SelectAndExpand.SelectedItems.Any()) [](start = 20, length = 53)
why are you removing this check? won't traverse always return an empty subresult if there are no items in SelectedItems? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ "Accounts(101)?$expand=MyPaymentInstruments", new int[] {4, 15} }, | ||
{ "Accounts(101)?$select=AccountID&$expand=MyGiftCard($select=GiftCardID)", new int[] {2, 1} } | ||
{ "Accounts(101)?$select=AccountInfo/FirstName, AccountInfo/LastName", new int[] {1, 0, 0} }, | ||
{ "Accounts(101)?$select=AccountID&$expand=MyPaymentInstruments($select=PaymentInstrumentID;$expand=TheStoredPI)", new int[]{7,4,4} }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ "Accounts(101)?$select=AccountID&$expand=MyPaymentInstruments($select=PaymentInstrumentID;$expand=TheStoredPI) [](start = 0, length = 128)
{ "Accounts(101)?$select=AccountID&$expand=MyPaymentInstruments($select=PaymentInstrumentID;$expand=TheStoredPI) [](start = 0, length = 128)
Do we have any test cases that test multiple nested levels of expand? i.e., $select=AccountID&$expand=MyPaymentInstruments($select=PaymentInstrumentID;$expand=TheStoredPI($select=someproponstoredpi))? #Closed
int expectedLinksCount = mimeType.Contains(MimeTypes.ODataParameterFullMetadata) | ||
? testCase.Value[1] | ||
: testCase.Value[2]; | ||
Assert.AreEqual(expectedLinksCount, navigationLinks.Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss. I wouldn't expect expectedLinksCount to vary depending on the full versus minimal metadata. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the case of "Accounts(101)?$expand=MyPaymentInstruments($select=PaymentInstrumentID,FriendlyName)", full metadata response contains nav links for entities contained by PaymentInstruments, which are not supposed to be materialized in the minimal metadata response.
Edit:
I have made an update to SelectedPropertiesNode.Create(SelectExpandClause) to return entire tree only when AllSelected are true for current level and expanded entities.
Thanks for the catch! #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -211,13 +211,13 @@ public void EntryContextUriWithSelectString() | |||
|
|||
[Theory] | |||
// expand without select, $expand=A | |||
[InlineData(ODataVersion.V4, "TestModel.CapitolCity/Districts", "")] | |||
[InlineData(ODataVersion.V4, "TestModel.CapitolCity/Districts", "TestModel.CapitolCity/Districts()")] | |||
[InlineData(ODataVersion.V401, "TestModel.CapitolCity/Districts", "TestModel.CapitolCity/Districts()")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 4.0 and 4.01 are now the same, we can simplify these tests (and ensure 4.0 and 4.01 are the same) by removing the ODataVersion from the InlineData and having the tests run for both 4.0 and 4.01. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[InlineData(ODataVersion.V401, "Districts(City(Districts()))")] | ||
public void EntryContextUriWithExpandNestedExpandString(ODataVersion version, string expectedExpandWithoutNesting) | ||
[InlineData(ODataVersion.V4)] | ||
[InlineData(ODataVersion.V401)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just remove inlinedata and run for v4 and v401. #Closed
@@ -737,7 +737,7 @@ public void CollectionOfProjectedExpandedEntities() | |||
// Sample Request: http://host/service/Employees?$expand=Company | |||
// Context Url in Response: http://host/service/$metadata#Employees(Company()) | |||
[Theory] | |||
[InlineData(ODataVersion.V4, "\"{0}$metadata#Employees\"")] | |||
[InlineData(ODataVersion.V4, "\"{0}$metadata#Employees(AssociatedCompany())\"")] | |||
[InlineData(ODataVersion.V401, "\"{0}$metadata#Employees(AssociatedCompany())\"")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, can just run test for v4 and v4.01 with same string. #Closed
@@ -228,15 +228,15 @@ public void ExpandNavigationThatIsNotSelectedAutomaticallySelectsLink() | |||
{ | |||
const string selectClauseText = "Navigation2"; | |||
const string expandClauseText = "Navigation1"; | |||
const string expectedSelectClause = "Navigation2,Navigation1"; | |||
const string expectedSelectClause = "Navigation2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that they (happily) match, you can get rid of expectedSelectClaus, and just pass in selectClause in both places. #Closed
@@ -228,15 +228,15 @@ public void ExpandNavigationThatIsNotSelectedAutomaticallySelectsLink() | |||
{ | |||
const string selectClauseText = "Navigation2"; | |||
const string expandClauseText = "Navigation1"; | |||
const string expectedSelectClause = "Navigation2,Navigation1"; | |||
const string expectedSelectClause = "Navigation2"; | |||
this.ParseAndExtract(selectClauseText: selectClauseText, expandClauseText: expandClauseText, expectedSelectClauseFromOM: expectedSelectClause, expectedExpandClauseFromOM: "Navigation1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Navigation1" [](start = 183, length = 13)
"Navigation1" [](start = 183, length = 13)
seems like this should just be "expandClauseText" #Closed
this.ParseAndExtract(selectClauseText: selectClauseText, expandClauseText: expandClauseText, expectedSelectClauseFromOM: expectedSelectClause, expectedExpandClauseFromOM: "Navigation1"); | ||
} | ||
|
||
[Fact] | ||
public void ExpandNavigationThatIsNotSelectedInNonTopLevelAutomaticallySelectsLink() | ||
{ | ||
const string expandClauseText = "Navigation1($select=Navigation1;$expand=Navigation2)"; | ||
const string expectedExpandClauseText = "Navigation1($select=Navigation1,Navigation2;$expand=Navigation2)"; | ||
const string expectedExpandClauseText = "Navigation1($select=Navigation1;$expand=Navigation2)"; | ||
this.ParseAndExtract(selectClauseText: null, expandClauseText: expandClauseText, expectedSelectClauseFromOM: null, expectedExpandClauseFromOM: expectedExpandClauseText); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be able to get rid of this and just pass in expandClauseText (further enforcing that the expected is the same as the provided.) #Closed
@@ -1021,7 +1019,7 @@ public void ExpandSyntacticErrorMessageSpecifiesExpandAsWellAsSelect() | |||
public void MixOfSelectionTypesShouldWork() | |||
{ | |||
const string select = "Name,Birthdate,MyAddress,Fully.Qualified.Namespace.*,MyLions"; | |||
const string expectedSelect = "Name,Birthdate,MyAddress,Fully.Qualified.Namespace.*,MyLions,MyDog"; | |||
const string expectedSelect = "Name,Birthdate,MyAddress,Fully.Qualified.Namespace.*,MyLions"; | |||
const string expand = "MyDog"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since select is (by design) same as expectedSelect, you can get rid of expectedSelect and just use select. #Closed
{ | ||
const string expandClauseText = "MyDog($select=Color;$expand=MyPeople)"; | ||
const string selectClauseText = ""; | ||
const string expectedExpandClauseText = "MyDog($select=Color,MyPeople;$expand=MyPeople)"; | ||
const string expectedExpandClauseText = "MyDog($select=Color;$expand=MyPeople)"; | ||
const string expectedSelectClause = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove; just use expandClauseText #Closed
@@ -403,8 +403,9 @@ private static string CreateSelectExpandContextUriSegment(SelectExpandClause sel | |||
/// <returns>The generated expand string.</returns> | |||
private static string ProcessSubExpand(string expandNode, string subExpand, ODataVersion version) | |||
{ | |||
return string.IsNullOrEmpty(subExpand) && version <= ODataVersion.V4 ? null : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string.IsNullOrEmpty(subExpand) [](start = 19, length = 31)
Should we keep "string.IsNullOrEmpty(subExpand) " check and return "null"? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need to include the case of null and return "()" for expanded entity.
In reply to: 224171315 [](ancestors = 224171315)
@@ -22,5 +22,4 @@ SelectExpandQueryOption | |||
SelectedItems | |||
Path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug in the test or the code? I would have expected this to write out the selected function. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selected function "OrdersWithMoreThanTwoItems" is bound to entity collection but not the entity type.
In reply to: 224206728 [](ancestors = 224206728)
c91c50a
to
a09889e
Compare
@mikepizzo @xuzhg Thanks for the code review! Please check the latest commit, and let me know if there any other outstanding issues. #Resolved |
f9a7377
to
bc69d1d
Compare
- cleanups - Create SelectedPropertiesnode based on AllSelected values of current level and expanded nav properties.
bc69d1d
to
4933eca
Compare
ODL nightly Microsoft.OData.Core.7.6.0-Nightly201810151853 has been generated on the commit above and successfully validated by WebAPI project. #Resolved |
[InlineData(ODataVersion.V4, "\"{0}$metadata#Employees(AssociatedCompany)/$entity\"")] | ||
[InlineData(ODataVersion.V401, "\"{0}$metadata#Employees(AssociatedCompany())/$entity\"")] | ||
[InlineData(ODataVersion.V4, "\"{0}$metadata#Employees(AssociatedCompany,AssociatedCompany())/$entity\"")] | ||
[InlineData(ODataVersion.V401, "\"{0}$metadata#Employees(AssociatedCompany,AssociatedCompany())/$entity\"")] | ||
public void ExpandedMultiSegmentEntity(ODataVersion version, string contextString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again, we should just have a single string defined for the contextString and run the test for each ODataVersion using that same string definition (to enforce that the two versions don't diverge) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidated the two test cases. Thanks!
I have also checked the rest of the changes, looks like there are no other cases similar to these that can be combined.
In reply to: 226131792 [](ancestors = 226131792)
@@ -228,16 +228,14 @@ public void ExpandNavigationThatIsNotSelectedAutomaticallySelectsLink() | |||
{ | |||
const string selectClauseText = "Navigation2"; | |||
const string expandClauseText = "Navigation1"; | |||
const string expectedSelectClause = "Navigation2,Navigation1"; | |||
this.ParseAndExtract(selectClauseText: selectClauseText, expandClauseText: expandClauseText, expectedSelectClauseFromOM: expectedSelectClause, expectedExpandClauseFromOM: "Navigation1"); | |||
this.ParseAndExtract(selectClauseText: selectClauseText, expandClauseText: expandClauseText, expectedSelectClauseFromOM: selectClauseText, expectedExpandClauseFromOM: "Navigation1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Navigation1" [](start = 179, length = 13)
Should just be expandClauseText #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found = this.edmModel.FindDeclaredBoundOperations(this.structuredType) | ||
.Any(op => op.Name.Equals(token, StringComparison.Ordinal)); | ||
} | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the segment matches a declared navigation property, it is evaluated as a navigation property even if there is a bound operation with the same unqualified name. That's why the qualified name needs to be used to disambiguate a bound function from a defined navigation property. Our logic needs to conceptually be:
if it doesn't have parens, it's not a navigation property. Otherwise, if it has parens, then:
- if it matches a defined navigation property => treat it as a navigation property
- otherwise, if the name contains a dot => it's not a navigation property
- otherwise, if it matches an unqualified bound operation name => it's not a navigation property
- otherwise, it's a navigation property
Note that 3) is only required if we believe there are existing (invalid) responses containing unqualified bound operation names. If we don't believe that's the case, the logic becomes much simpler: if the name has parens and does not have a dot in the name, it's a nav prop. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Using the new method IsNavigationPropertyToken for the original !IsBoundOperation(). In the new method, implements the logic above, applying the rationale about qualified name for operation.
In reply to: 226136554 [](ancestors = 226136554)
{ | ||
// Qualified name: try to match bound and unbound operations | ||
found = this.edmModel.FindBoundOperations(token, this.structuredType).Count() != 0 | ||
|| this.edmModel.FindDeclaredOperations(token).Count() != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| this.edmModel.FindDeclaredOperations(token).Count() != 0 [](start = 20, length = 59)
See above; if it contains a dot we should already have omitted it. Otherwise, if the select list contains the unqualified name of an operation not bound to the current type, it should not be treated as an operation. #Resolved
{ | ||
SelectedPropertiesNode.Create("Action(Edm.Int32)").Should().HaveAction(this.cityType, this.action); | ||
SelectedPropertiesNode.Create("Action(Edm.Int32)", this.cityType, this.edmModel).Should().NotHaveAction(this.cityType, this.action); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the old version of this method created the selected properties node as the top-level node, which is valid for an unbound operation. With this change, you are selecting an unbound action from a type, which is not valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the original case was valid, because it could be creating selectedpropertiesnode for a top level action/action import. It was only your proposed change that nested bound it to this.cityType, that was invalid.
In reply to: 226493137 [](ancestors = 226493137,226137870)
@mikepizzo PR has been updated per your comments. Can you please take a look to whether there are anything else outstanding? |
|
||
// For better readability, set the value in if-else branches corresponding to decision tree above. | ||
bool found; | ||
if (this.structuredType.NavigationProperties().Any(_ => _.Name.Equals(token, StringComparison.Ordinal))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringComparison.Ordinal [](start = 89, length = 24)
Should respect case-sensitivity of current ODataUriResolver. #Closed
found = true; | ||
} | ||
else if (token.IndexOf(nameSpaceSeparator) != -1 || | ||
this.edmModel.FindBoundOperations(this.structuredType).Any(op => op.Name.Equals(token, StringComparison.Ordinal))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
op.Name.Equals(token, StringComparison.Ordinal) [](start = 86, length = 47)
This should also use case sensitivity of UriResolver. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, as Biao points out, this in the contextUrl which is case-normalized.
In reply to: 227065902 [](ancestors = 227065902)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Set the selectionType to EntireSubtree when the selected list contains only expand tokens. * For $expand, add navProp() to context-url. * ABNF v4; ensure explicit select; don't add link for expand to select items. * Resolve CR comments. - cleanups - Create SelectedPropertiesnode based on AllSelected values of current level and expanded nav properties. * Update for navProp token resolution logic per CR comments; cleanups
* Set the selectionType to EntireSubtree when the selected list contains only expand tokens. * For $expand, add navProp() to context-url. * ABNF v4; ensure explicit select; don't add link for expand to select items. * Resolve CR comments. - cleanups - Create SelectedPropertiesnode based on AllSelected values of current level and expanded nav properties. * Update for navProp token resolution logic per CR comments; cleanups
This reverts commit ec6fb33.
Revert "Context url for expand (OData#1286)" This reverts commit ec6fb33. Original PR not only modified how @odata.context formed for v4.0 and v4.01, but also removed a lot of version recognition code. That makes it’s pretty difficult to do conditionally. Please, keep in mind that default protocol version is v4.0 and client had to say that it wants v4.01. In AX I'm going to add telemetry to allow us to see what clients asked for Related work items: #1426955
Issues
This pull request fixes issue ##1265 ($expand in context url), including the two prerequisite issues #1259 and #1268. It also includes fix for issue #1104.
Please note that the PR #1264 associated with issue #1259 has been superseded by this PR, which contains the changes needed. PR #1264 will be cancelled in favor of this PR.
Description
Changes include the followings:
1). Add parenthesized token for navigation property for $expand clause in context url;
2). Context url select list parsing implementation update from ABNF V3 to ABNF V4.01 (#1268);
3). Ensure explicit selected navigation properties are present in context url (#1104);
4). Ensure navigation links are materialized for case of minimal metadata with expand-sub-select clause (#1259) .
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.