Skip to content
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

Inner paging doesn't work #227

Closed
HiKoLaaa opened this issue May 10, 2020 · 7 comments · Fixed by #297
Closed

Inner paging doesn't work #227

HiKoLaaa opened this issue May 10, 2020 · 7 comments · Fixed by #297
Assignees
Labels
Type: Bug Something isn't working as documented

Comments

@HiKoLaaa
Copy link

Hello,
I had used example from docs

var query = new Query()
    .Repository("octokit", "octokit.net")
    .Issues()
    .AllPages() // Auto-pages Issues
    .Select(issue => new
    {
        issue.Id,
        Comments = issue
            .Comments(null, null, null, null)
            .AllPages() // Auto-pages Issue Comments
            .Select(comment => comment.Body)
            .ToList(),
    });

In my repository 200+ issues. I suggested that if I ran this query, I was getting all issues with their comments. But result was only 200. When I changed paging from auto to manual, so I had got all my issues.

Manual:

var query = new Query()
    .Repository("octokit", "octokit.net")
    .Issues()
    .AllPages() // Auto-pages Issues
    .Select(issue => new
    {
        issue.Id,
        Comments = issue
            .Comments(100, null, null, null)
            .Nodes
            .Select(comment => comment.Body)
            .ToList(),
    });
@terrajobst
Copy link

terrajobst commented Jul 1, 2020

Yeah, paging is a big issue for me too. It's a bit unclear to me how one would do manual paging inner sources. It seems one would have rerun the entire query and manually stich together the objects that were created, which seems quite tedious. Also, my brain hurts trying to think how this would work when multiple inner sources are used.

terrajobst added a commit to terrajobst/apireview.net that referenced this issue Jul 1, 2020
Right now, is a bug preventing us from using this in production though:

    octokit/octokit.graphql.net#234

There is another one that might bite us too:

    octokit/octokit.graphql.net#227
@jcansdale
Copy link
Collaborator

@HiKoLaaa,

In my repository 200+ issues. I suggested that if I ran this query, I was getting all issues with their comments. But result was only 200. When I changed paging from auto to manual, so I had got all my issues.

I'm running into this limit as well. Did you try debugging the issue further? It would be great if this could be fixed.

@jcansdale jcansdale added the bug label Jul 3, 2020
@jcansdale jcansdale self-assigned this Jul 3, 2020
@jcansdale
Copy link
Collaborator

We do have AllPages unit and integrations tests. We even have some with inner paging, but we only check that they complete. See:

var results = (await Connection.Run(query)).ToArray();

@HiKoLaaa
Copy link
Author

HiKoLaaa commented Jul 3, 2020

@jcansdale, hi, no, I didn't try investigate it.

@jcansdale
Copy link
Collaborator

jcansdale commented Jul 3, 2020

I have a failing integration test for this here #237

This existing assert so early caught the issue:
https://github.com/octokit/octokit.graphql.net/pull/237/files#diff-33750d5aaabf5716b9b31d7ec4dc1200R196

It only passed because it was checking more than 100 issues were returned with a page size of 100 entries. Because it's returning the first 2 pages, it returns 200 issues. I've changed the page size to 50 so that it returns only 100 issues and the assert fails.

I'm struggling to debug the AllPages code, so any help would be very welcome!

@jcansdale
Copy link
Collaborator

jcansdale commented Jul 5, 2020

The issue appears to be that subqueries only seem to have a single subquery.

For example. here is the main query:

image

This has two subqueries, one for issues and another for issue comments. Because the issues hasNextPage == true, it starts another query.

Unfortunately, this subquery only contains the issue comments subquery. This means that even though issues comes back with hasNextPage == true, another issue query is never started. There is an issue comments subquery, so if there are more issue comments, another query will be started for these.

image

This explains why there are only ever 2 pages of outer elements returned. The main query works fine and has multiple subqueries, but any further queries only have a single subquery. This means that any outer elements with hasNextPage == true are ignored.

@jcansdale
Copy link
Collaborator

@terrajobst,

Yeah, paging is a big issue for me too. It's a bit unclear to me how one would do manual paging inner sources. It seems one would have rerun the entire query and manually stitch together the objects that were created, which seems quite tedious.

The bug appears to be with missing outer sources. This is much easier to fix manually. For example:
jcansdale/gpr#82

terrajobst added a commit to terrajobst/apireview.net that referenced this issue Jul 6, 2020
Right now, is a bug preventing us from using this in production though:

    octokit/octokit.graphql.net#234

There is another one that might bite us too:

    octokit/octokit.graphql.net#227
terrajobst added a commit to terrajobst/apireview.net that referenced this issue Jul 6, 2020
Right now, is a bug preventing us from using this in production though:

    octokit/octokit.graphql.net#234

There is another one that might bite us too:

    octokit/octokit.graphql.net#227
terrajobst added a commit to terrajobst/apireview.net that referenced this issue Jul 7, 2020
Right now, is a bug preventing us from using this in production though:

    octokit/octokit.graphql.net#234

There is another one that might bite us too:

    octokit/octokit.graphql.net#227
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed bug labels Oct 26, 2022
@jMarkP jMarkP mentioned this issue Jul 9, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants