Improve mapped column handling for REST pagination and NextLink creation fixes by seantleonard · Pull Request #1319 · Azure/data-api-builder

seantleonard · 2023-03-09T20:55:19Z

Why make this change?

Closes [Bug]: Error "The given key was not present in the dictionary." returned when using mapping and pagination #1315 which highlights an issue with pagination logic not resolving column names from database results using "mapped" column names, when configured. Instead, the current column resolution always uses "backing" column names.

What is this change?

changes TryResolveJsonElementToScalarVariable(element.GetProperty(column)
- to: TryResolveJsonElementToScalarVariable(element.GetProperty(exposedColumnName)

Issue when loading request "FindMany" within pagination token

With a request like GET https://localhost:5001/api/todo
This result is returned as next link:

"nextLink": "https://localhost:5001/api/todo?System.Collections.Specialized.NameValueCollection"

Fixing the column resolution uncovered this bug with how initial pagination next links are generated. (not a problem when a pagination link is included with request). The NameValueCollection which contains all query string key/value pairs was not being resolved into a query string and the nextLink looked like:
"nextLink": "https://localhost:5001/api/todo/System.Collections.Specialized.NameValueCollection"
- The fix was to add explicit iteration of the key value pairs in the collection and generate a well-formed Query String using AspNetCore helper utilities.
- This uncovered a subtle difference in how the engine and test fixture escape query string payload for URLs. Tests were using a helper which incorrectly escaped invalid URI characters with lower case hex value. i.e. %3d. Then proper form would be uppercase -> %3D

How was this tested?

Integration Tests- updated existing integration tests.
Unit Tests

Sample Request(s)

Using the configuration/SQL db script provided in the issue, the following request will succeed now and provide a valid NextLink: https://localhost:5001/api/todo?$orderby=title desc, tid asc

…he returned db result. Should be mapped value not unmapped value. Then addressed uncovered encoding differences in tests vs engine and made consistent. i.e. `%3d` vs correctly upper case `%3D`

…found

aaronburtle

Nice find, thanks for fixing!

abhishekkumams

Interesting Find.

…found

src/Service/Resolvers/SqlPaginationUtil.cs

Aniruddh25

Thanks for fixing so quickly, waiting for addition of tests

…ected behavior of correct nextlink generation.

…found

…tity to all setup scripts for all db flavors and update reference configuration files.

…r used in the nextLink pagination cursor. Tests added for all db flavors, sql config references updated with new entities that are used in tests, and commands in config generation scripts are updated.

…ttps://github.com/Azure/data-api-builder into dev/seleonar/issue1315_restpagination_keynotfound

config-generators/dab-config.sql.reference.json

config-generators/postgresql-commands.txt

config-generators/dab-config.sql.reference.json

src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs

…arameter

…ms and is also not the first. Which checks for alternative prefix to after query param: &$ and not ?$

Aniruddh25

LGTM, after simplifying the config files!

…at was using the type table.

…rators as they are no present in the test project.

… dev/seleonar/issue1315_restpagination_keynotfound

… to not result in CICD errors.

…and mappedbookmarks table because generate_series is only compatible with sql2022 and not sql2019/localdb

…found

…ion fixes (#1319) ## Why make this change? - Closes #1315 which highlights an issue with pagination logic not resolving column names from database results using "mapped" column names, when configured. Instead, the current column resolution always uses "backing" column names. ## What is this change? - changes `TryResolveJsonElementToScalarVariable(element.GetProperty(column)` - to: `TryResolveJsonElementToScalarVariable(element.GetProperty(exposedColumnName)` ### Issue when loading request "FindMany" within pagination token With a request like ` GET https://localhost:5001/api/todo` This result is returned as next link: ```json "nextLink": "https://localhost:5001/api/todo?System.Collections.Specialized.NameValueCollection" ``` - Fixing the column resolution uncovered this bug with how **initial** pagination next links are generated. (not a problem when a pagination link is included with request). The `NameValueCollection` which contains all query string key/value pairs was not being resolved into a query string and the nextLink looked like: `"nextLink": "https://localhost:5001/api/todo/System.Collections.Specialized.NameValueCollection"` - The fix was to add explicit iteration of the key value pairs in the collection and generate a well-formed Query String using AspNetCore helper utilities. - This uncovered a subtle difference in how the engine and test fixture escape query string payload for URLs. Tests were using a helper which incorrectly escaped invalid URI characters with lower case hex value. i.e. `%3d`. Then proper form would be uppercase -> `%3D` - Discussion https://stackoverflow.com/a/48301702/18174950 - https://stackoverflow.com/a/47877559/18174950 - https://edi.wang/post/2018/11/25/netcore-webutility-urlencode-httputility-urlencode - https://stackoverflow.com/a/51829522/18174950 ## How was this tested? - [x] Integration Tests- updated existing integration tests. - [ ] Unit Tests ## Sample Request(s) - Using the configuration/SQL db script provided in the issue, the following request will succeed now and provide a valid NextLink: `https://localhost:5001/api/todo?$orderby=title desc, tid asc`

…ion fixes (#1319) - Closes #1315 which highlights an issue with pagination logic not resolving column names from database results using "mapped" column names, when configured. Instead, the current column resolution always uses "backing" column names. - changes `TryResolveJsonElementToScalarVariable(element.GetProperty(column)` - to: `TryResolveJsonElementToScalarVariable(element.GetProperty(exposedColumnName)` With a request like ` GET https://localhost:5001/api/todo` This result is returned as next link: ```json "nextLink": "https://localhost:5001/api/todo?System.Collections.Specialized.NameValueCollection" ``` - Fixing the column resolution uncovered this bug with how **initial** pagination next links are generated. (not a problem when a pagination link is included with request). The `NameValueCollection` which contains all query string key/value pairs was not being resolved into a query string and the nextLink looked like: `"nextLink": "https://localhost:5001/api/todo/System.Collections.Specialized.NameValueCollection"` - The fix was to add explicit iteration of the key value pairs in the collection and generate a well-formed Query String using AspNetCore helper utilities. - This uncovered a subtle difference in how the engine and test fixture escape query string payload for URLs. Tests were using a helper which incorrectly escaped invalid URI characters with lower case hex value. i.e. `%3d`. Then proper form would be uppercase -> `%3D` - Discussion https://stackoverflow.com/a/48301702/18174950 - https://stackoverflow.com/a/47877559/18174950 - https://edi.wang/post/2018/11/25/netcore-webutility-urlencode-httputility-urlencode - https://stackoverflow.com/a/51829522/18174950 - [x] Integration Tests- updated existing integration tests. - [ ] Unit Tests - Using the configuration/SQL db script provided in the issue, the following request will succeed now and provide a valid NextLink: `https://localhost:5001/api/todo?$orderby=title desc, tid asc`

## Why make this change? Cherry picking the following changes to create a patch for 0.5.x: - #1349 - #1319 - #1351 --------- Co-authored-by: Jun Su <70380073+junsu0ms@users.noreply.github.com> Co-authored-by: Sean Leonard <sean.leonard@microsoft.com> Co-authored-by: abhishekkumams <102276754+abhishekkumams@users.noreply.github.com>

…ion fixes (#1319) ## Why make this change? - Closes #1315 which highlights an issue with pagination logic not resolving column names from database results using "mapped" column names, when configured. Instead, the current column resolution always uses "backing" column names. ## What is this change? - changes `TryResolveJsonElementToScalarVariable(element.GetProperty(column)` - to: `TryResolveJsonElementToScalarVariable(element.GetProperty(exposedColumnName)` ### Issue when loading request "FindMany" within pagination token With a request like ` GET https://localhost:5001/api/todo` This result is returned as next link: ```json "nextLink": "https://localhost:5001/api/todo?System.Collections.Specialized.NameValueCollection" ``` - Fixing the column resolution uncovered this bug with how **initial** pagination next links are generated. (not a problem when a pagination link is included with request). The `NameValueCollection` which contains all query string key/value pairs was not being resolved into a query string and the nextLink looked like: `"nextLink": "https://localhost:5001/api/todo/System.Collections.Specialized.NameValueCollection"` - The fix was to add explicit iteration of the key value pairs in the collection and generate a well-formed Query String using AspNetCore helper utilities. - This uncovered a subtle difference in how the engine and test fixture escape query string payload for URLs. Tests were using a helper which incorrectly escaped invalid URI characters with lower case hex value. i.e. `%3d`. Then proper form would be uppercase -> `%3D` - Discussion https://stackoverflow.com/a/48301702/18174950 - https://stackoverflow.com/a/47877559/18174950 - https://edi.wang/post/2018/11/25/netcore-webutility-urlencode-httputility-urlencode - https://stackoverflow.com/a/51829522/18174950 ## How was this tested? - [x] Integration Tests- updated existing integration tests. - [ ] Unit Tests ## Sample Request(s) - Using the configuration/SQL db script provided in the issue, the following request will succeed now and provide a valid NextLink: `https://localhost:5001/api/todo?$orderby=title desc, tid asc`

seantleonard added 4 commits March 9, 2023 11:52

Fixes issue #1315 by correcting the column value used for lookup in t…

edbd9ec

…he returned db result. Should be mapped value not unmapped value. Then addressed uncovered encoding differences in tests vs engine and made consistent. i.e. `%3d` vs correctly upper case `%3D`

Remove extra whitespace.

9ab6b93

fix spacing

da8e896

remove local launchSettings changes.

ee1485c

seantleonard requested review from Aniruddh25, JelteF, Mathos1432, aaronburtle, aaronpowell, abhishekkumams, ayush3797, gledis69, junsu0ms, mbhaskar, milismsft, ravishetye, severussundar and tarazou9 as code owners March 9, 2023 20:55

seantleonard added 2 commits March 9, 2023 12:55

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

a9a8bdf

…found

remove launchsettings changes to match main

10a9cd0

aaronburtle approved these changes Mar 10, 2023

View reviewed changes

abhishekkumams approved these changes Mar 10, 2023

View reviewed changes

abhishekkumams and others added 2 commits March 10, 2023 10:27

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

789fb86

…found

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

90d850e

…found

severussundar reviewed Mar 10, 2023

View reviewed changes

src/Service/Resolvers/SqlPaginationUtil.cs Outdated Show resolved Hide resolved

severussundar approved these changes Mar 10, 2023

View reviewed changes

Aniruddh25 reviewed Mar 11, 2023

View reviewed changes

src/Service/Resolvers/SqlPaginationUtil.cs Show resolved Hide resolved

Aniruddh25 reviewed Mar 11, 2023

View reviewed changes

src/Service/Resolvers/SqlPaginationUtil.cs Show resolved Hide resolved

Aniruddh25 requested changes Mar 11, 2023

View reviewed changes

Add tests for ms,pg,my sql to validate >100 rows in resultset has exp…

2cf5041

…ected behavior of correct nextlink generation.

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

7651017

…found

This was referenced Mar 17, 2023

[Bug]: nextLink appears broken #1338

Closed

[Bug]: Unable to map primary key colum (?) - app crashes #1340

Closed

seantleonard added 3 commits March 17, 2023 13:57

Fix Postgres.sql setup commands for bookmarks table. add Bookmarks en…

fe94b54

…tity to all setup scripts for all db flavors and update reference configuration files.

Added test for ensuring mapped column usage in orderby query paramete…

d8f9064

…r used in the nextLink pagination cursor. Tests added for all db flavors, sql config references updated with new entities that are used in tests, and commands in config generation scripts are updated.

Merge branch 'dev/seleonar/issue1315_restpagination_keynotfound' of h…

0342b12

…ttps://github.com/Azure/data-api-builder into dev/seleonar/issue1315_restpagination_keynotfound

Aniruddh25 reviewed Mar 17, 2023

View reviewed changes

config-generators/dab-config.sql.reference.json Outdated Show resolved Hide resolved

Aniruddh25 reviewed Mar 17, 2023

View reviewed changes

config-generators/postgresql-commands.txt Show resolved Hide resolved

Aniruddh25 reviewed Mar 17, 2023

View reviewed changes

config-generators/dab-config.sql.reference.json Outdated Show resolved Hide resolved

Aniruddh25 reviewed Mar 18, 2023

View reviewed changes

src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs Show resolved Hide resolved

seantleonard added 2 commits March 17, 2023 17:11

Update reference config files in the test project.

0053db5

revised comment describing test FindMany_MappedColumn_NoOrderByQueryP…

b262634

…arameter

Aniruddh25 approved these changes Mar 18, 2023

View reviewed changes

Add coverage test for scenario when orderby is one of many query para…

f4fca94

…ms and is also not the first. Which checks for alternative prefix to after query param: &$ and not ?$

Aniruddh25 approved these changes Mar 18, 2023

View reviewed changes

seantleonard added 6 commits March 20, 2023 16:32

remove unneccessary test: FindTestWithQueryStringSelect_NoOrderBy, th…

e7e8994

…at was using the type table.

Remove dab-config.<db>.reference.json config files from config-gene…

5d374da

…rators as they are no present in the test project.

Merge branch 'main' of https://github.com/Azure/data-api-builder into…

1d83a55

… dev/seleonar/issue1315_restpagination_keynotfound

fix sql setup script to end generate_series function with a semicolon…

9bbc660

… to not result in CICD errors.

Use CTE common table expression to generate 10000 rows for bookmarks …

84bb6e6

…and mappedbookmarks table because generate_series is only compatible with sql2022 and not sql2019/localdb

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

5c609ff

…found

Aniruddh25 approved these changes Mar 23, 2023

View reviewed changes

Merge branch 'main' into dev/seleonar/issue1315_restpagination_keynot…

36255b5

…found

seantleonard merged commit 09648ee into main Mar 23, 2023

seantleonard deleted the dev/seleonar/issue1315_restpagination_keynotfound branch March 23, 2023 23:45

Aniruddh25 mentioned this pull request Mar 24, 2023

Cherry picking bug fixes and github urls usage #1354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve mapped column handling for REST pagination and NextLink creation fixes#1319

Improve mapped column handling for REST pagination and NextLink creation fixes#1319
seantleonard merged 24 commits intomainfrom
dev/seleonar/issue1315_restpagination_keynotfound

seantleonard commented Mar 9, 2023 •

edited

Loading

Uh oh!

aaronburtle left a comment

Uh oh!

abhishekkumams left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

seantleonard commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why make this change?

What is this change?

Issue when loading request "FindMany" within pagination token

How was this tested?

Sample Request(s)

Uh oh!

aaronburtle left a comment

Choose a reason for hiding this comment

Uh oh!

abhishekkumams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aniruddh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seantleonard commented Mar 9, 2023 •

edited

Loading