Fix query string contents in nextLink for REST (remove dupe $after query parameter)

[seantleonard]

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:

GET https://localhost:5001/api/Bookmarks?$first=1

Response:

{
    "value": [
        {
            "id": 1,
            "bkname": "Test Item #00001"
        }
    ],
    "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:

[{"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:

GET https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0=

{
    "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%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(...):

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:

// Purge old $after value so this function can replace it.
queryStringParameters.Remove("$after");

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
2. 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.
3. 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.
4. Ensure $after value is base64encoded and not URI encoded.

Sample request

As illustrated earlier:

GET https://localhost:5001/api/Bookmarks?$first=1

Use the nextLink returned to send a follow up pagination request:

GET https://localhost:5001/api/Bookmarks?$first=1&$after=W3siRW50aXR5TmFtZSI6IkJvb2ttYXJrcyIsIkZpZWxkTmFtZSI6ImlkIiwiRmllbGRWYWx1ZSI6MSwiRGlyZWN0aW9uIjowfV0=

[seantleonard]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Aniruddh25]

LGTM, Thank you so much for the quick fix, and the comprehensive tests

[abhishekkumams]

LGTM

[severussundar]

Thank you for the fix :)

[seantleonard]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).