-
Notifications
You must be signed in to change notification settings - Fork 0
feat: handle pagination in policy collection mapping query #81
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
Conversation
### what use pagination with the cursor for query to get policy collection mappings ### why handle the case where the desired entry is not within first page of results ### testing acceptance tests and tested in sandbox ### docs n/a
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.
Comments are not blocking.
if !query.PolicyCollection.PolicyMappings.PageInfo.HasNextPage { | ||
return nil, NotFound{"Policy collection mapping not found"} | ||
} | ||
cursor = query.PolicyCollection.PolicyMappings.PageInfo.EndCursor |
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.
@albertodonato I haven't checked but am trusting you that this DTRT when there's < 100 results in total 😅
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.
yeah when there's < 100 results, HasNextPage
is false, so it would exit earlier.
} `graphql:"policyCollection(uuid: $uuid)"` | ||
} | ||
cursor := "" | ||
for { |
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.
@albertodonato The unbounded forever loop which only ends on the API tell us it's done or giving us an error makes me twitch a little. I don't know that there's a better way and it's definitely not blocking, just idle musing on my part.
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.
The way the connection protocol works is really meant to be unrolled page by page. It does have a total
attribute that we could use to know ahead how many entries are there, but it could in theory become inconsistent if entries are added/removed between calls, so we'd have to have additional logic around it.
} | ||
cursor := "" | ||
for { | ||
var query struct { |
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.
@albertodonato I was surprised to see this struct inline inside the for loop; I don't know if that's just normal/idiomatic for go, but it makes for quite deep indentation/nesting. Again, just unblocking commentary.
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.
yeah we can actually move the query and variables definition outside of the loop, since they don't really depend on the entry. Updated
83d409a
to
765a789
Compare
what
use pagination with the cursor for query to get policy collection mappings
why
handle the case where the desired entry is not within first page of results
testing
acceptance tests and tested in sandbox
docs
n/a