-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add pagination support to DataStore #365
Conversation
**Notes:** Introduce pagination support to the `DataStore.query` API. - `QueryPaginationInput` holds the pagination values - the `DataStore` plugin translates thar struct to SQLite limit/offset - add tests to make sure the new API behaves correctly under different scenarios
@@ -21,13 +21,15 @@ extension AWSMutationDatabaseAdapter: MutationEventSource { | |||
let fields = MutationEvent.keys | |||
let predicate = fields.inProcess == false || fields.inProcess == nil | |||
|
|||
// TODO remove this in favor of proper sorting/orderBy API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worthwhile for us to start linking backlog items into our TODOs, so we don't lose track of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call and I actually thought about it. We do have an item for this one, but it's a Pivotal story, which is not public. Do you think we should make these GitHub issues?
public static func page(_ page: UInt, | ||
limit: UInt = QueryPaginationInput.defaultLimit) -> QueryPaginationInput { | ||
return QueryPaginationInput(page: page, limit: limit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. in the case where we have page(0, limit: 0)
-- we could throw an exception.. or log some sort of error.. or.. just do nothing.
Seems like nothing could be okay -- what do you think?
Also, do we care to support an 'unlimited' parameter? (e.g. they want to specify a page, but specify an unlimited parameter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. in the case where we have page(0, limit: 0) -- we could throw an exception.. or log some sort of error.. or.. just do nothing.
Seems like nothing could be okay -- what do you think?
I'm willing to allow customers to do that if they want, like databases usually allow.
Also, do we care to support an 'unlimited' parameter? (e.g. they want to specify a page, but specify an unlimited parameter)
How does a page is defined without knowing its size? That could work in case of an offset
is used instead of a page
, but since we decided to calculate the offset based on the page
and limit
, both are needed (i.e. what range page 2 belongs to if we don't know the page size?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding page(0, limit:0) -- i was worried about unintentionally sending those values in.. but.. probably something we don't need to spend time debating.. i think it's a case where customers just need to be careful about what they're doing. :).
Regarding .unlimited:
Oops brain fart: I was thinking they might call page(0, limit:100), and then decide that based on those first 100 records, to then call page(1, limit: unlimited) -- but that doesn't make sense :)
Notes:
Introduce pagination support to the
DataStore.query
API.QueryPaginationInput
holds the pagination valuesDataStore
plugin translates that struct to SQLite limit/offsetBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.