-
Notifications
You must be signed in to change notification settings - Fork 285
[Cherrypick] Fix query string contents in nextLink for REST (remove dupe $after query parameter) #2032
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
Merged
Aniruddh25
merged 2 commits into
main
from
dev/sean/cherrypick-2006-nextLink-After-patch
Feb 13, 2024
Merged
[Cherrypick] Fix query string contents in nextLink for REST (remove dupe $after query parameter) #2032
Aniruddh25
merged 2 commits into
main
from
dev/sean/cherrypick-2006-nextLink-After-patch
Feb 13, 2024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…query parameter) (#2006) # Fix query string contents in nextLink for REST ## Why? The change committed in #1895 was written to fix how DAB writes the `nextLink` value in a response body to a GET request using pagination. The `nextLink` contents is a URL which includes a query parameter `$after` which contains base64 encoded pagination metadata. #1895 inadvertently triggered the `nextLink` creation process to append an additional `$after` query parameter instead of overwriting the `$after` query parameter present in the request. Sample request: ```https GET https://localhost:5001/api/Bookmarks?$first=1 ``` Response: ```json { "value": [ { "id": 1, "bkname": "Test Item #1" } ], "nextLink": "https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0=" } ``` The `$after` query parameter's value is a properly formatted base64 encoded string as fixed in #1895. The value is an opaque string and from the above sample response translates to: ```json [{"EntityName":"Bookmarks","FieldName":"id","FieldValue":1,"Direction":0}] ``` However, once the `nextLink` is used to fetch another page, the next page's results include an invalid response body because the `nextLink` is improperly formed: ```https GET https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0= ``` ```json { "value": [ { "id": 2, "bkname": "Test Item #2" } ], "nextLink": "https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0%3D&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MiwiRGlyZWN0aW9uIjowfV0=" } ``` The invalid and and unexpected value is: `$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0%3D` Not only is it URL escaped, but it is a duplicate value that shouldn't be present. ## What is this change? This change essentially removes the old `$after` query parameter (key and value) from the queryStringParameters NamedValueCollection passed in to `CreateNextLink(...)`: ```csharp public static JsonElement CreateNextLink(string path, NameValueCollection? queryStringParameters, string after){} ``` Due the NamedValueCollection being a special object created by `System.Web.HttpUtility.HttpQSCollection` > The ParseQueryString method uses UTF8 format to parse the query string In the returned NameValueCollection, URL encoded characters are decoded and multiple occurrences of the same query string parameter are listed as a single entry with a comma separating each value. The values are URI escaped and this change purges that uri escaped value: ```csharp // Purge old $after value so this function can replace it. queryStringParameters.Remove("$after"); ``` ```csharp string queryString = FormatQueryString(queryStringParameters: queryStringParameters); if (!string.IsNullOrWhiteSpace(after)) { string afterPrefix = string.IsNullOrWhiteSpace(queryString) ? "?" : "&"; queryString += $"{afterPrefix}{RequestParser.AFTER_URL}={after}"; } ``` The above code: 1. Creates an initial `queryString` where the values are URL encoded. 2. Checks if a new after link is available to inject into query params 3. APPENDS a NEW and unique `$after` query param. (old code simply appended an `$after` query parameter even if one already existed.) 4. JSON Serialize and then JSON Deserialize probably to get rid of some sort of unexpected formatting. ## How was this change tested? Added integration test to ensure nextLink creation doesn't regress and to exercise acquiring the `nextLink` from subsequent pages. The test setup makes an initial request formed to invoke DAB to return a `nextLink` in the response. The assertions occur on the **2nd request** which uses the `nextLink` returned from the first request: 1. Response is 200 -> 400 would indicate issue with query parameter payload 1. Ensures `nextLink` value different from the value returned on the first request used during test setup -> ensures DAB is not recycling a single $after query parameter/nextLink value. 1. Ensures `nextLink` query parameter `$after` does not have a comma, which would indicate that parsing of the query string detected two instance of the `$after` query parameter, which per dotnet behavior, concatenates the values and separates them with a comma. 1. Ensure `$after` value is base64encoded and not URI encoded. ## Sample request As illustrated earlier: ```https GET https://localhost:5001/api/Bookmarks?$first=1 ``` Use the `nextLink` returned to send a follow up pagination request: ```https GET https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0= ```
Aniruddh25
approved these changes
Feb 12, 2024
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
ayush3797
approved these changes
Feb 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cherry picks #2006 to
mainFix query string contents in nextLink for REST
Why?
The change committed in #1895 was written to fix how DAB writes the
nextLinkvalue in a response body to a GET request using pagination.The
nextLinkcontents is a URL which includes a query parameter$afterwhich contains base64 encoded pagination metadata.#1895 inadvertently triggered the
nextLinkcreation process to append an additional$afterquery parameter instead of overwriting the$afterquery parameter present in the request.Sample request:
Response:
{ "value": [ { "id": 1, "bkname": "Test Item #00001" } ], "nextLink": "https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0=" }The
$afterquery parameter's value is a properly formatted base64 encoded string as fixed in #1895. The value is an opaque string andfrom the above sample response translates to:
[{"EntityName":"Bookmarks","FieldName":"id","FieldValue":1,"Direction":0}]However, once the
nextLinkis used to fetch another page, the next page's results include an invalid response body becausethe
nextLinkis improperly formed:{ "value": [ { "id": 2, "bkname": "Test Item #00002" } ], "nextLink": "https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0%3D&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MiwiRGlyZWN0aW9uIjowfV0=" }The invalid and and unexpected value is:
$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0%3DNot only is it URL escaped, but it is a duplicate value that shouldn't be present.
What is this change?
This change essentially removes the old
$afterquery parameter (key and value) from the queryStringParametersNamedValueCollection passed in to
CreateNextLink(...):Due the NamedValueCollection being a special object created by
System.Web.HttpUtility.HttpQSCollectionThe values are URI escaped and this change purges that uri escaped value:
The above code:
queryStringwhere the values are URL encoded.$afterquery param. (old code simply appended an$afterquery parameter even if one already existed.)How was this change tested?
Added integration test to ensure nextLink creation doesn't regress and to exercise acquiring
the
nextLinkfrom subsequent pages. The test setup makes an initial request formed to invoke DAB to returna
nextLinkin the response. The assertions occur on the 2nd request which uses thenextLinkreturned fromthe first request:
nextLinkvalue different from the value returned on the first request used during test setup -> ensuresDAB is not recycling a single $after query parameter/nextLink value.
nextLinkquery parameter$afterdoes not have a comma, which would indicate that parsing of the querystring detected two instance of the
$afterquery parameter, which per dotnet behavior, concatenates the values and separatesthem with a comma.
$aftervalue is base64encoded and not URI encoded.Sample request
As illustrated earlier:
Use the
nextLinkreturned to send a follow up pagination request: