Update README.md #1
Closed
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
Test pr
abhishekkumams
added a commit
that referenced
this pull request
Jul 25, 2022
…sole_app Hawaii-Cli tool to generate config for runtime Engine
2 tasks
severussundar
added a commit
that referenced
this pull request
Sep 15, 2023
## Why make this change? - Closes #1505 - Responses for Find requests with `$select` query string fields in addition to those requested in `$select` For the below GET request, ```http GET https://localhost:5001/api/Book?$select=title ``` the response contains the `id` field as well. If a table contains composite PK, all the Pks are returned in the response. Response: ```json { "value": [ { "title": "New title #1", "id": 1 }, { "title": "Also Awesome book", "id": 2 }, { "title": "Great wall of china explained", "id": 3 }, ... ] } ``` When both $select and $orderby are used together, the response contains $orderby fields in addition to the ones in $select ```http GET https://localhost:5001/api/Book?$select=id,publisher_id&$orderby=name ``` the response contains the `name` field as well. Response: ```json { "value": [ { "id": 2, "publisher_id": 1234, "name": "Also Awesome book" }, ... ] } ``` Similar behavior was observed with views, where the configured key-fields were returned in the response. If multiple key-fields are configured, all of them were returned.  ## What is this change? - To support `pagination` and `$first` with Find requests, it is necessary to provide the `nextLink` field in the response. For the calculation of `nextLink`, the fields such as primary keys, fields in `$orderby` clause are retrieved from the database in addition to the fields requested in the $select clause. The `nextLink` calculation is done at DAB layer after executing the database query because it is impossible to know beforehand whether a table/view would contain more than 100 records (or more entries than requested in the `$first` clause if that is applicable). - Seeing that these additional fields needs to be retrieved from the database, the database queries cannot be modified to select only the fields present in `$select` clause. So, the additional fields are removed at the DAB layer before returning the response. ## Why can't the database queries by modifed to select only the fields present in `$select` query string? When the table contains more than 100 records, the nextLink URL field is returned in the response. The nextLink URL contains $after which is a base 64 encoded value. $after is calculated using all the primary key and $orderby column values of the 100th record (or nth when $first=n is used). The row values from the 100th (or nth when $first=n is used) record is necessary because of two reasons a) The last record's values are necessary to correctly fetch the subsequent set of records. Let's say the first GET request fetches 100 records. To be able to fetch the next set of rows: 101-200 in the right ordering, the database query will include the condition ~ `WHERE ( ( [orderby_column] > [orderby_value_of_row100] ) OR ( [orderby_column] == [orderby_value_of_row100] && [pk_column] > [pk_value_of_row100])`. Here `orderby_value_of_row100` and `pk_value_of_row100` refers to the values of orderby column of the 100th row and pk column of the 100th row respectively. b) $orderby can be performed in ASC or DESC order. The 100th record will be different in both the ordering and we rely on the value of the 100th row to be able to select the next set of rows (101-200, etc.) Logic for $after field calculation: [SqlPaginationUtil.MakeCursorFromJsonElement()](https://github.com/Azure/data-api-builder/blob/7b4c8b5606012d6c9e8093a1ed9b63e5116faf75/src/Core/Resolvers/SqlPaginationUtil.cs#L123C2-L123C2) Granted, all of this is applicable only when nextLink field is necessary in the response, it is not possible to know beforehand whether a table contains more than 100 records ( or n records when first n records is selected using $first clause). We select 101 records (or n+1 records for $first=n) and then determine at DAB layer if nextLink field is necessary. If the database query returns 101 (or n+1 records), then we conclude that a nextLink field is necessary. Essentailly, not being able to know beforehand the number of records in the table forces to select the additional fields. ## How was this tested? - [x] Integration Tests - [x] Manual Tests ## Sample Request(s) - Primary Key fields are not returned in the response when absent in $select query string  - Orderby fields are not returned in the response 
seantleonard
added a commit
that referenced
this pull request
Feb 5, 2024
…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= ```
seantleonard
added a commit
that referenced
this pull request
Feb 6, 2024
…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= ```
seantleonard
added a commit
that referenced
this pull request
Feb 6, 2024
…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= ```
seantleonard
added a commit
that referenced
this pull request
Feb 12, 2024
…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
added a commit
that referenced
this pull request
Feb 13, 2024
…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 <[email protected]>
severussundar
added a commit
that referenced
this pull request
Apr 26, 2024
…ion (#1994) ## Why make this change? - Closes #1699 This PR tackles the **a)Database Query Generation** and **b)Selection Set Resolution** components of Multiple Mutation/Multiple Create feature. ## What is this change? ### 1. Enhancing the Hotchocolate input parsing logic At the moment, DAB supports create mutation operation on a single entity and a single item. The current parsing logic `BaseSqlQueryStructure.GQLMutArgumentToDictParams()` is written with a foundational idea that a) All the fields present in the create mutation input request belong to the top-level entity b) All the fields will be scalar fields. With the multiple create feature, these no longer hold true. Here, are some items that needs to be accounted for by the parsing logic. a) The fields present in the input request, could correspond to columns of the top-level entity or could be relationship fields b) Relationship fields are not scalar fields. They could be of Object type or List type depending on the type of relationship between top level entity and the related entity. c) Since, nesting levels without any limit is supported, a relationship field can further have a combination of scalar fields + relationship fields. This inner relationship field can again be a combination of scalar fields + relationship fields and so on... d) In addition to point create mutation, a many type mutation operation is also generated. Ex: `createbooks`, `createBookmarks_multiple`. These operations let the user create multiple items of the top-level entity. The root input field for this operation will be `items` as opposed to `item` (for a point create mutation operation). New parsing logic: `SqlMutationEngine.GQLMultipleCreateArgumentToDictParams()` Final data structure of the input params after parsing: Many type Multiple Create: `List<IDictionary<string, object?>>` Point Multiple Create: `IDictionary<string, object?>` ### 2. MultipleCreateStructure - New Wrapper class introduced `MultipleCreateStructure` is used to hold all the relevant information about an entity needed for processing it - determining if there are referencing/referenced entities, determining if there is a linking table insertion necessary along with it, populating the relationship fields in the current entity and linking entity. ### 3. Identifying and handling the implications of a point vs many multiple create operation Logic for identifying and handling different multiple type mutation operations - Point vs Many multiple create operation is added in `SqlMutationEngine.PerformMultipleCreateOperation()` ### 4. Understanding and processing the parsed mutation input request Logic added in `SqlMutationEngine.PerformDbInsertOperation()`. This function handles the core logic from understanding the different fields of the parsed input parameters all the way till the successful creation of records in the database. This function is invoked for both point and many type multiple create operations. The logic in this function at a high-level can be summarized as follows: - Identifying whether the input type is of list type vs object type (indicative of many vs one relationship cardinality) - From the parsed input parameters, identifying the referencing and referenced entities - Identifying and populating the fields belonging to the current and linking table entity. - Recurse for all the referenced entities - the top-level entity depends on the PKs of these entities. Hence, the referenced entities have to processed before the top-level entity. - Once the logic for all the referenced entities have been executed, all the foreign keys needed for the top-level entity should be available. Validate and populate all the foreign keys needed. - Build and execute the database query for creating a new record in the table backing the top-level entity. When building the database query, predicates corresponding to `create` policy (if defined) are added. - Store the PKs of the created record - this is needed for resolving the selection set. - Check if a record has to be created in the linking table. If so, build and execute the database query necessary. - Recurse for all the referencing entities - these entities depend on the PKs of the current entity. Hence, these entities need to processed after the top-level entity. ### 5. Introduction of synchronous methods in QueryExecutor: Given the nature of multiple create feature, the order of execution of code statements is highly important. Consider the example of Book - Publisher which are related through a N:1 relationship. Here, the insertion of Publisher item has to be completed before the logic for populating the foreign keys of Book item can begin to execute. To guarantee the correct order of execution as well maintain the transactional aspects of the feature (successfully rollback all the inserted items if there any failures), the following equivalent synchronous methods were needed. - `ExecuteQuery` - `ExecuteQueryAgainstDb` - `SetManagedIdentityAccessTokenIfAny` - `Read` - `ExtractResultSetFromDbDataReader` - `GetResultProperties` _Note:_ These synchronous methods are used only for the create part of the multiple create feature. The selection set resolution still continues to use the asynchronous equivalent of these methods and takes advantage of the benefits they offer. All the other features continue to use the asynchronous version of these methods. ### 6. Selection Set Resolution - Selection set resolution involves fetching the requested fields on **items created as result of current mutation operation**. The logic for selection set resolution of a point create mutation operation is the same as the one that is present today. However, to resolve the selection set of a many type multiple create operation such as `createbooks` or `createBookmarks_multiple`, new logic had to be introduced. The current limitation with `SqlInsertQueryStructure` is that it can accept PKs of 0 or 1 item. It cannot accept PKs for a list of items which is precisely what is needed for many type multiple create mutation operation. New constructor for `SqlInsertQueryStructure` to accept PKs for a list of items is introduced. - To account for a list of PKs in the eventual `SELECT` statement, the logic for adding predicates is updated. - Logic changes in `SqlQueryStructure.AddPrimaryKeyPredicates()` and `MsSqlQueryBuilder.BuildQueryForMultipleCreateOperation()` _Note:_ Exact database queries for each scenario and query plan analysis are added in the design doc. - For all the entities involved in the selection set, `read` policy will be honored. ### 7. Some more notes about the feature - When the feature flag for multiple create is disabled (or) when used with database types that do not support multiple create operations, a create mutation will follow the current logic of processing a create mutation. Only for databases that support multiple create and when the feature is enabled, this new logic kicks in. _Rationale_: Accounting for related entity fields in every step of processing the mutation request is uncessary. So, when it can be determined that the intention is to not use multiple create feature (either feature flag is disabled or database type does not support the feature), a lot of unnecessary logic can be totally skipped. - **All or None Behavior**: The scope of the transaction applies to the mutation operation as a whole. Irrespective of a point or many type multiple create operation, either all of the items will be created successfully or none of them will be created. ### 8. Introduced new entities and tables for testing Entity Name | Table backing the entity | --- | --- | Publisher_MM | publishers_mm | Book_MM | books_mm | Author_MM | authors_mm | Review_MM | reviews_mm | WebsiteUser_MM | website_users_mm | The intent of introducing these new tables and entities is to define relationships between only through config file and validate different create mutation scenarios. There are no foreign key constraints defined in any of the tables. The relationships defined are only through config file. ### 9. Database Queries executed: Consider the following multiple create mutation request ```graphql mutation multipleCreateExample { createbook( item: { title: "Harry Potter and the Goblet of Fire" publishers: { name: "Bloomsbury" } reviews: [ { content: "Review #1" website_users: { id: 5100, username: "Website User #1" } } { content: "Review #2", websiteuser_id: 1 } ] authors: [ { name: "J.K Rowling" birthdate: "1965-07-31" royalty_percentage: 100.0 } ] } ) { id title publisher_id publishers { id name } reviews { items { book_id id content } } authors { items { id name birthdate } } } } ``` The following database queries are executed in the same order: #### CREATE STATEMENTS ##### 1. Publisher ``` INSERT INTO [dbo].[publishers] ([name]) OUTPUT Inserted.[id] AS [id], Inserted.[name] AS [name] VALUES (@param0); ``` ##### 2. Book ``` INSERT INTO [dbo].[books] ([title], [publisher_id]) OUTPUT Inserted.[id] AS [id], Inserted.[title] AS [title], Inserted.[publisher_id] AS [publisher_id] VALUES (@param0, @param1); ``` ##### 3. Website User ``` INSERT INTO [dbo].[website_users] ([id], [username]) OUTPUT Inserted.[id] AS [id], Inserted.[username] AS [username] VALUES (@param0, @param1); ``` ##### 4. Reviews ``` INSERT INTO [dbo].[reviews] ([book_id], [content], [websiteuser_id]) OUTPUT Inserted.[book_id] AS [book_id], Inserted.[id] AS [id], Inserted.[content] AS [content], Inserted.[websiteuser_id] AS [websiteuser_id] VALUES (@param0, @param1, @Param2); ``` ``` INSERT INTO [dbo].[reviews] ([book_id], [content], [websiteuser_id]) OUTPUT Inserted.[book_id] AS [book_id], Inserted.[id] AS [id], Inserted.[content] AS [content], Inserted.[websiteuser_id] AS [websiteuser_id] VALUES (@param0, @param1, @Param2); ``` ##### 5. Authors ``` INSERT INTO [dbo].[authors] ([name], [birthdate]) OUTPUT Inserted.[id] AS [id], Inserted.[name] AS [name], Inserted.[birthdate] AS [birthdate] VALUES (@param0, @param1); ``` ##### 6. Linking table ``` INSERT INTO [dbo].[book_author_link] ([book_id], [royalty_percentage], [author_id]) OUTPUT Inserted.[book_id] AS [book_id], Inserted.[author_id] AS [author_id], Inserted.[royalty_percentage] AS [royalty_percentage] VALUES (@param0, @param1, @Param2); ``` #### SELECT STATEMENT ``` SELECT TOP 1 [table0].[id] AS [id], [table0].[title] AS [title], [table0].[publisher_id] AS [publisher_id], JSON_QUERY ([table1_subq].[data]) AS [publishers], JSON_QUERY (COALESCE([table4_subq].[data], '[]')) AS [reviews], JSON_QUERY (COALESCE([table8_subq].[data], '[]')) AS [authors] FROM [dbo].[books] AS [table0] OUTER APPLY (SELECT TOP 1 [table1].[id] AS [id], [table1].[name] AS [name] FROM [dbo].[publishers] AS [table1] WHERE [table0].[publisher_id] = [table1].[id] ORDER BY [table1].[id] ASC FOR JSON PATH, INCLUDE_NULL_VALUES,WITHOUT_ARRAY_WRAPPER) AS [table1_subq]([data]) OUTER APPLY (SELECT TOP 100 [table4].[book_id] AS [book_id], [table4].[id] AS [id], [table4].[content] AS [content] FROM [dbo].[reviews] AS [table4] WHERE [table4].[book_id] = [table0].[id] ORDER BY [table4].[book_id] ASC, [table4].[id] ASC FOR JSON PATH, INCLUDE_NULL_VALUES) AS [table4_subq]([data]) OUTER APPLY (SELECT TOP 100 [table8].[id] AS [id], [table8].[name] AS [name], [table8].[birthdate] AS [birthdate] FROM [dbo].[authors] AS [table8] INNER JOIN [dbo].[book_author_link] AS [table12] ON [table12].[author_id] = [table8].[id] WHERE [table12].[book_id] = [table0].[id] ORDER BY [table8].[id] ASC FOR JSON PATH, INCLUDE_NULL_VALUES) AS [table8_subq]([data]) WHERE [table0].[id] = @Param19 ORDER BY [table0].[id] ASC FOR JSON PATH, INCLUDE_NULL_VALUES,WITHOUT_ARRAY_WRAPPER ``` ## How was this tested? - [X] Existing unit tests and integration tests - validate the correct functioning of all other features in DAB - [X] Integration and Unit tests specific to multiple create validate that correctness of multiple create feature. - [X] Manual Tests ## Sample Request(s)   --------- Co-authored-by: Ayush Agarwal <[email protected]> Co-authored-by: Ayush Agarwal <[email protected]> Co-authored-by: Sean Leonard <[email protected]>
1 task
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.
Test pr