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

seantleonard · seantleonard · commit 9f15d382fd4c · 2024-02-05T17:47:58.000-08:00
# 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= ```
diff --git a/src/Core/Resolvers/SqlPaginationUtil.cs b/src/Core/Resolvers/SqlPaginationUtil.cs
@@ -520,22 +520,40 @@ public static string Base64Decode(string base64EncodedData)
         /// <summary>
         /// Create the URL that will provide for the next page of results
         /// using the same query options.
+        /// Return value formatted as a JSON array: [{"nextLink":"[base]/api/[entity]?[queryParams_URIescaped]$after=[base64encodedPaginationToken]"}]
         /// </summary>
-        /// <param name="path">The request path.</param>
-        /// <param name="queryStringParameters">Collection of query string parameters.</param>
-        /// <param name="after">The values needed for next page.</param>
-        /// <returns>The string representing nextLink.</returns>
-        public static JsonElement CreateNextLink(string path, NameValueCollection? queryStringParameters, string after)
+        /// <param name="path">The request path excluding query parameters (e.g. https://localhost/api/myEntity)</param>
+        /// <param name="queryStringParameters">Collection of query string parameters that are URI escaped.</param>
+        /// <param name="newAfterPayload">The contents to add to the $after query parameter. Should be base64 encoded pagination token.</param>
+        /// <returns>JSON element - array with nextLink.</returns>
+        public static JsonElement CreateNextLink(string path, NameValueCollection? queryStringParameters, string newAfterPayload)
         {
+            if (queryStringParameters is null)
+            {
+                queryStringParameters = new();
+            }
+            else
+            {
+                // Purge old $after value so this function can replace it.
+                queryStringParameters.Remove("$after");
+            }
+
+            // To prevent regression of current behavior, retain the call to FormatQueryString
+            // which URI escapes other query parameters. Since $after has been removed,
+            // this will not affect the base64 encoded paging token.
             string queryString = FormatQueryString(queryStringParameters: queryStringParameters);
-            if (!string.IsNullOrWhiteSpace(after))
+
+            // When a new $after payload is provided, append it to the query string with the
+            // appropriate prefix: ? if $after is the only query parameter. & if $after is one of many query parameters.
+            if (!string.IsNullOrWhiteSpace(newAfterPayload))
             {
                 string afterPrefix = string.IsNullOrWhiteSpace(queryString) ? "?" : "&";
-                queryString += $"{afterPrefix}{RequestParser.AFTER_URL}={after}";
+                queryString += $"{afterPrefix}{RequestParser.AFTER_URL}={newAfterPayload}";
             }
 
             // ValueKind will be array so we can differentiate from other objects in the response
             // to be returned.
+            // [{"nextLink":"[base]/api/[entity]?[queryParams_URIescaped]$after=[base64encodedPaginationToken]"}]
             string jsonString = JsonSerializer.Serialize(new[]
             {
                 new
@@ -580,6 +598,7 @@ public static string FormatQueryString(NameValueCollection? queryStringParameter
 
             foreach (string key in queryStringParameters)
             {
+                // Whitespace or empty string query paramters are not supported.
                 if (string.IsNullOrWhiteSpace(key))
                 {
                     continue;
diff --git a/src/Service.Tests/Configuration/ConfigurationTests.cs b/src/Service.Tests/Configuration/ConfigurationTests.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Collections.Specialized;
 using System.IdentityModel.Tokens.Jwt;
 using System.IO;
 using System.IO.Abstractions;
@@ -16,6 +17,7 @@
 using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
+using System.Web;
 using Azure.DataApiBuilder.Config;
 using Azure.DataApiBuilder.Config.ObjectModel;
 using Azure.DataApiBuilder.Core.AuthenticationHelpers;
@@ -2717,6 +2719,95 @@ public async Task OpenApi_EntityLevelRestEndpoint()
             Assert.IsFalse(componentSchemasElement.TryGetProperty("Publisher", out _));
         }
 
+        /// <summary>
+        /// This test validates that DAB properly creates and returns a nextLink with a single $after
+        /// query parameter when sending paging requests.
+        /// The first request initiates a paging workload, meaning the response is expected to have a nextLink.
+        /// The validation occurs after the second request which uses the previously acquired nextLink
+        /// This test ensures that the second request's response body contains the expected nextLink which:
+        /// - is base64 encoded and NOT URI escaped e.g. the trailing "==" are not URI escaped to "%3D%3D"
+        /// - is not the same as the first response's nextLink -> DAB is properly injecting a new $after query param
+        /// and updating the new nextLink
+        /// - does not contain a comma (,) indicating that the URI namevaluecollection tracking the query parameters
+        /// did not come across two $after query parameters. This addresses a customer raised issue where two $after
+        /// query parameters were returned by DAB.
+        /// </summary>
+        [TestMethod]
+        [TestCategory(TestCategory.MSSQL)]
+        public async Task ValidateNextLinkUsage()
+        {
+            // Arrange - Setup test server with entity that has >1 record so that results can be paged.
+            // A short cut to using an entity with >100 records is to just include the $first=1 filter
+            // as done in this test, so that paging behavior can be invoked.
+
+            const string ENTITY_NAME = "Bookmark";
+
+            // At least one entity is required in the runtime config for the engine to start.
+            // Even though this entity is not under test, it must be supplied to the config
+            // file creation function.
+            Entity requiredEntity = new(
+                Source: new("bookmarks", EntitySourceType.Table, null, null),
+                Rest: new(Enabled: true),
+                GraphQL: new(Singular: "", Plural: "", Enabled: false),
+                Permissions: new[] { GetMinimalPermissionConfig(AuthorizationResolver.ROLE_ANONYMOUS) },
+                Relationships: null,
+                Mappings: null);
+
+            Dictionary<string, Entity> entityMap = new()
+            {
+                { ENTITY_NAME, requiredEntity }
+            };
+
+            CreateCustomConfigFile(globalRestEnabled: true, entityMap);
+
+            string[] args = new[]
+            {
+                $"--ConfigFileName={CUSTOM_CONFIG_FILENAME}"
+            };
+
+            using TestServer server = new(Program.CreateWebHostBuilder(args));
+            using HttpClient client = server.CreateClient();
+
+            // Setup and send GET request
+            HttpRequestMessage initialPaginationRequest = new(HttpMethod.Get, $"{RestRuntimeOptions.DEFAULT_PATH}/{ENTITY_NAME}?$first=1");
+            HttpResponseMessage initialPaginationResponse = await client.SendAsync(initialPaginationRequest);
+
+            // Process response body for first request and get the nextLink to use on subsequent request
+            // which represents what this test is validating.
+            string responseBody = await initialPaginationResponse.Content.ReadAsStringAsync();
+            Dictionary<string, JsonElement> responseProperties = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(responseBody);
+            string nextLinkUri = responseProperties["nextLink"].ToString();
+
+            // Act - Submit request with nextLink uri as target and capture response
+
+            HttpRequestMessage followNextLinkRequest = new(HttpMethod.Get, nextLinkUri);
+            HttpResponseMessage followNextLinkResponse = await client.SendAsync(followNextLinkRequest);
+
+            // Assert
+
+            Assert.AreEqual(HttpStatusCode.OK, followNextLinkResponse.StatusCode, message: "Expected request to succeed.");
+
+            // Process the response body and inspect the "nextLink" property for expected contents.
+            string followNextLinkResponseBody = await followNextLinkResponse.Content.ReadAsStringAsync();
+            Dictionary<string, JsonElement> followNextLinkResponseProperties = JsonSerializer.Deserialize<Dictionary<string, JsonElement>>(followNextLinkResponseBody);
+
+            string followUpResponseNextLink = followNextLinkResponseProperties["nextLink"].ToString();
+            Uri nextLink = new(uriString: followUpResponseNextLink);
+            NameValueCollection parsedQueryParameters = HttpUtility.ParseQueryString(query: nextLink.Query);
+            Assert.AreEqual(expected: false, actual: parsedQueryParameters["$after"].Contains(','), message: "nextLink erroneously contained two $after query parameters that were joined by HttpUtility.ParseQueryString(queryString).");
+            Assert.AreNotEqual(notExpected: nextLinkUri, actual: followUpResponseNextLink, message: "The follow up request erroneously returned the same nextLink value.");
+
+            // Do not use SqlPaginationUtils.Base64Encode()/Decode() here to eliminate test dependency on engine code to perform an assert.
+            try
+            {
+                Convert.FromBase64String(parsedQueryParameters["$after"]);
+            }
+            catch (FormatException)
+            {
+                Assert.Fail(message: "$after query parameter was not a valid base64 encoded value.");
+            }
+        }
+
         /// <summary>
         /// Helper function to write custom configuration file. with minimal REST/GraphQL global settings
         /// using the supplied entities.
