Change the form of our NextLink used for pagination cursors#1851
Merged
aaronburtle merged 24 commits intomainfrom Nov 11, 2023
Merged
Change the form of our NextLink used for pagination cursors#1851aaronburtle merged 24 commits intomainfrom
aaronburtle merged 24 commits intomainfrom
Conversation
aaronburtle
requested review from
Aniruddh25,
JelteF,
Mathos1432,
aaronpowell,
abhishekkumams,
ayush3797,
gledis69,
mbhaskar,
milismsft,
ravishetye,
seantleonard,
severussundar and
tarazou9
as code owners
abhishekkumams
approved these changes
Oct 30, 2023
severussundar
requested changes
Nov 2, 2023
Contributor
severussundar
left a comment
severussundar
left a comment
There was a problem hiding this comment.
Need to fix failing test related to after token - RequestAfterTokenOnly
seantleonard
requested changes
Nov 3, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
I appreciate your efforts to update the nextLink. I like the new format. To be in a good state to merge, please add more comments for new classes, more descriptive variable names.
…//github.com/Azure/data-api-builder into dev/aaronburtle/ChangeNextLinkAndUpdateTests
seantleonard
requested changes
Nov 10, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
a few suggestions about naming consistency and what var names we should attribute with data received from request versus DAB internal datastructures mapping to database metadata.
severussundar
approved these changes
Nov 10, 2023
Contributor
severussundar
left a comment
severussundar
left a comment
There was a problem hiding this comment.
LGTM. Thank you for fixing this and incorporating the suggestions!
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
seantleonard
approved these changes
Nov 10, 2023
Contributor
seantleonard
left a comment
seantleonard
left a comment
There was a problem hiding this comment.
Thank you for addressing feedback! lgtm!
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
added a commit
that referenced
this pull request
Nov 13, 2023
## Why make this change?
Updates the form of our NextLink cursor used for pagination. This only
updates the part used for the response, which is then used to continue
the paginated requests. Matches the desired form.
## What is this change?
Rather than using part of the `SqlQueryStructures` to form the cursor,
we create an object that is only used specifically for the NextLink with
respect to the response/request. This means we don't need all of the
fields that are included in the pagination columns, and that the
information contained in this object is really unrelated to the query
structure.
We therefore introduce a new class in our pagination utilities which can
be used to facilitate the serialization and deserialization of the
NextLink cursor in the response/request. Using an anonymous object was
considered, but we re-use this object in enough places, and it holds
complex enough data, that it will be easier to understand and be more
reusable if we define a new class here inside of our pagination
utilities.
The Json structure that is contained within the response/request that we
(de)serialize with this new class is therefore modified.
Previous
```
[{"Value":0,"Direction":0,"TableSchema":"foo","TableName":"bar","ColumnName":"baz"}]")
```
Updated
```
[{"EntityName":"foobar","FieldName":"qux","Value":0,"Direction":0}]
```
Complete NextLink without being decoded
```
"nextLink": "https://localhost:5001/rest/Book?$first=1&$after=W3siVmFsdWUiOjEsIkRpcmVjdGlvbiI6MCwiVGFibGVTY2hlbWEiOiJkYm8iLCJUYWJsZU5hbWUiOiJib29rcyIsIkNvbHVtbk5hbWUiOiJpZCJ9XQ%3D%3D"
```
## How was this tested?
Modified our current test suite to match the new format.
## Sample Request(s)
and paginated request should work as long as it uses or generated a
cursor for pagination.
one such example for when REST has a path of /rest against the Book
entity.
https://localhost:5001/rest/Book?$first=1
---------
Co-authored-by: Aniruddh Munde <[email protected]>
Aniruddh25
added a commit
that referenced
this pull request
Nov 13, 2023
## Why make this change? This is simply cherry-picking the following PRs before creating the next stable 0.9 version: - #1876 - #1821 - #1854 - #1851 - #1872 - #1868 --------- Co-authored-by: Sean Leonard <[email protected]> Co-authored-by: Abhishek Kumar <[email protected]> Co-authored-by: neeraj-sharma2592 <[email protected]> Co-authored-by: Neeraj Sharma (from Dev Box) <[email protected]> Co-authored-by: aaronburtle <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Why make this change?
Updates the form of our NextLink cursor used for pagination. This only updates the part used for the response, which is then used to continue the paginated requests. Matches the desired form.
What is this change?
Rather than using part of the
SqlQueryStructuresto form the cursor, we create an object that is only used specifically for the NextLink with respect to the response/request. This means we don't need all of the fields that are included in the pagination columns, and that the information contained in this object is really unrelated to the query structure.We therefore introduce a new class in our pagination utilities which can be used to facilitate the serialization and deserialization of the NextLink cursor in the response/request. Using an anonymous object was considered, but we re-use this object in enough places, and it holds complex enough data, that it will be easier to understand and be more reusable if we define a new class here inside of our pagination utilities.
The Json structure that is contained within the response/request that we (de)serialize with this new class is therefore modified.
Previous
Updated
Complete NextLink without being decoded
How was this tested?
Modified our current test suite to match the new format.
Sample Request(s)
and paginated request should work as long as it uses or generated a cursor for pagination.
one such example for when REST has a path of /rest against the Book entity.
https://localhost:5001/rest/Book?$first=1