Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Cherrypick] Fix query string contents in nextLink for REST (remove d…
…upe $after query parameter) (#2032) Cherry picks #2006 to `main` # 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= ``` Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
- Loading branch information