-
Notifications
You must be signed in to change notification settings - Fork 55
Fix multi-paged queries #297
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
Previously instances of PagedSubquery would not check their returned PageInfo to see if they had any more pages of results to load and so each subquery would only get run at most once. It also didn't always call the `addResult` delegate to add a page of results to the parent collection.
} | ||
else | ||
else if (subqueryRunners.Any()) |
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.
Given the change to add hasMore
it's now possible to reach this branch with no subqueryRunners
and then Peek
throws an exception
@@ -201,7 +201,7 @@ public async Task Can_AutoPage_Issues_Comments_With_Subquery() | |||
{ | |||
var query = new Query() | |||
.Repository(owner: "octokit", name: "octokit.net") | |||
.Issues().AllPages(50) | |||
.Issues().AllPages(100) |
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.
There are over 1000 issues in this repo now, so increase the page size to speed up the tests and reduce load on GitHub
|
||
// This is the first run, so run the master page. | ||
subqueryResultSinks = new Dictionary<ISubquery, List<Action<object>>>(); |
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.
We need to reinitialize this each time - the indexes of the List<>
in each entry needs to match up with the parentIds
and pageInfos
extracted from the page of results. It's fine though because when we loop through them we enqueue a new subquery runner which captures the sinks
@jMarkP this is a bit embarrassing, but I've been unable to run these integration tests locally even on the main branch. I keep seeing the following error: I've also tried the Our integration tests aren't running in CI at the moment, so I'd like to validate the test fixes personally before shipping this PR. Do you mind explaining how you're running the integration tests here so I can try to replicate it? Thanks! |
Hi @kfcampbell! I was running the integration tests locally on my Mac (both within Rider and via `dotnet --info`/Users/mark/dev/octokit.graphql.net/Octokit.GraphQL.IntegrationTests % dotnet --info .NET SDK: Version: 7.0.202 Commit: 6c74320bc3 That I think however that running the tests via dotnet 6 or 7 is probably the easiest thing to try here. It can still build and run older netstandard2.0 targets, and net 3.1 is out of LTS so may be diminishing returns trying to get it to run. I've just run the integration tests in the supplied devcontainer in this repo (which runs .net 7.0.305) and while there are still some failing integration tests (which are failing on main too), this PR looks to have reduced the number of failing tests from 5 to 3 and fixed the
|
Two of the integration tests are failing becauise As both are pretty simple fixes I've included them in this PR and now I get a clean run of the integration tests when running in the supplied devcontainer:
|
@kfcampbell - do you think there's any chance to get this (and #299) merged this week? I'm hoping to make use of these fixes in some upcoming work. |
@jMarkP thank you so much for the clear explanations! I missed those required environment variables somehow 🤦. When exporting valid credentials, I still see one failing test deterministically, but that's a big improvement on the |
Resolves #227
Behavior
Before the change?
AllPages
to query multi-page result sets would only ever query at most 2 pages of results: the first, top level query, and one invocation of each subquery.PagedSubquery
, thepageInfo
structure was never examined to see if there were any more pages to fetch, and so the caller would only ever call it for one page of results.PagedSubquery
code would only appendResults
to the parent collection when it had (theoretically) finished paginating. But each invocation ofRunPage
overwrites theResults
variable and so everything but the last page would have been lost.Pseudocode for the previous
RunPage
logic for bothPagedQuery
andPagedSubquery
(they share an implementation of this logic):After the change?
hasMore
field to record if the givenRunner
has more results it needs to processRunPage
logic given above is extended to check that flag before continuing on to process the stack ofRunners
RunPage
onPagedSubquery
now appends any new results to the parent collection, and clears it to make sure the next invocation is clean.Variables
passed intoPagedSubquery.Runner
weren't copied to a newDictionary
Other information
IssueTests
integration tests for meAdditional info
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Type: Breaking change
label)If
Yes
, what's the impact:Pull request type
Please add the corresponding label for change this PR introduces:
Type: Bug
Type: Feature
Type: Documentation
Type: Maintenance