-
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
Changes from 3 commits
b3c0e51
026b8f6
215f7ff
4933eca
e1e7744
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 : | ||
expandNode + ODataConstants.ContextUriProjectionStart + subExpand + ODataConstants.ContextUriProjectionEnd; | ||
|
||
// TODO: cleanup the version parameter. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 |
||
return expandNode + ODataConstants.ContextUriProjectionStart + subExpand + ODataConstants.ContextUriProjectionEnd; | ||
} | ||
|
||
/// <summary>Create combined result string using selected items list and expand items list.</summary> | ||
|
@@ -417,15 +418,6 @@ private static string CombineSelectAndExpandResult(IList<string> selectList, ILi | |
|
||
if (selectList.Any()) | ||
{ | ||
// https://github.com/OData/odata.net/issues/1104 | ||
// If the user explicitly selects and expands a nav prop, we should include both forms in contextUrl | ||
// We can't, though, because SelectExpandClauseFinisher.AddExplicitNavPropLinksWhereNecessary adds all of | ||
// the expanded items to the select before it gets here, so we can't tell what is explicitly selected by the user. | ||
foreach (var item in expandList) | ||
{ | ||
string expandNode = item.Substring(0, item.IndexOf(ODataConstants.ContextUriProjectionStart)); | ||
selectList.Remove(expandNode); | ||
} | ||
|
||
currentExpandClause += String.Join(ODataConstants.ContextUriProjectionPropertySeparator, selectList.ToArray()); | ||
} | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Since you are no longer conditionally calling Traverse, do you still need to initialize subResult? #Closed |
||
if (expandSelectItem.SelectAndExpand.SelectedItems.Any()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
Traverse(expandSelectItem.SelectAndExpand, processSubResult, combineSelectAndExpand, version, out subResult); | ||
} | ||
Traverse(expandSelectItem.SelectAndExpand, processSubResult, combineSelectAndExpand, version, out subResult); | ||
|
||
var expandItem = processSubResult(currentExpandClause, subResult, version); | ||
|
||
if (expandItem != null) | ||
{ | ||
expandList.Add(expandItem); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -697,9 +697,16 @@ public void QueryEntityAndExpandContainedEntity() | |
{ | ||
Dictionary<string, int[]> testCases = new Dictionary<string, int[]>() | ||
{ | ||
{ "Accounts(101)?$expand=MyGiftCard", new int[] {2, 4} }, | ||
{ "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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
{ "Accounts(101)?$expand=MyPaymentInstruments($select=PaymentInstrumentID,FriendlyName)", new int[]{4, 15, 4} }, | ||
|
||
{ "Accounts(101)?$expand=MyGiftCard($select=GiftCardID)", new int[] {2, 4, 4} }, | ||
|
||
{ "Accounts(101)?$expand=MyGiftCard", new int[] {2, 4, 4} }, | ||
{ "Accounts(101)?$expand=MyPaymentInstruments", new int[] {4, 15, 15} }, | ||
{ "Accounts(101)?$select=AccountID&$expand=MyGiftCard($select=GiftCardID)", new int[] {2, 1, 1} }, | ||
{ "Accounts(101)?$select=AccountID,MyGiftCard&$expand=MyGiftCard($select=GiftCardID)", new int[] {2, 1, 1} }, | ||
}; | ||
|
||
ODataMessageReaderSettings readerSettings = new ODataMessageReaderSettings() { BaseUri = ServiceBaseUri }; | ||
|
@@ -782,7 +789,11 @@ public void QueryEntityAndExpandContainedEntity() | |
|
||
Assert.AreEqual(ODataReaderState.Completed, reader.State); | ||
Assert.AreEqual(testCase.Value[0], entries.Count); | ||
Assert.AreEqual(testCase.Value[1], navigationLinks.Count); | ||
|
||
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 commentThe 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 commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
} | ||
} | ||
|
@@ -1197,9 +1208,9 @@ public void InvokeActionFromODataClientWhichReturnsContainedEntity() | |
[TestMethod] | ||
public void CreateContainedEntityFromODataClientUsingAddRelatedObject() | ||
{ | ||
|
||
TestClientContext.Format.UseJson(Model); | ||
|
||
// create an an account entity and a contained PI entity | ||
Account newAccount = new Account() | ||
{ | ||
|
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 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)