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

Issue/669 GraphQL Updates to PaginatedList #672

Closed

Conversation

jsmnhou
Copy link
Contributor

@jsmnhou jsmnhou commented Dec 2, 2024

Proposed Changes

  • Updates PaginatedList object (in paginated_list.py) to set default page size (per_page parameter) to 20 (previously 100) to adhere to 10/2024 GraphQL API changes
  • Add associated tests/fixtures (see test_paginated_list.py, fixtures/paginated_list.json)

Fixes #669

Effects

  • With these changes, all tests are passing and coverage report remains at 100%.

@jonespm
Copy link
Contributor

jonespm commented Dec 6, 2024

Hi @jsmnhou thanks for taking a look at this!

So the paginated list limit change (which was also currently delayed) was only targeted at the GraphQL API, not all API's. So unfortunately the code you contributed wouldn't fix this problem as we don't want the default changed for the other API's.

GraphQL is a special separate API and actually has it's own way of doing pagination using the Relay Connection Spec as mentioned on this documentation. It adds new first/after arguments and It doesn't use response headers like the other API's. I was thinking that the code here could be a method like fetch_all_pages and programmatically inject these and make the call. I can add some more examples to the original issue.

So it would need completely new custom code somewhere to handle this, maybe something like a paginated_graphql_list.py or maybe just in util.

The graphql call is in canvas.py

def graphql(self, query, variables=None, **kwargs):
sends the request "as-is".

I haven't looked at it at all, just filed this one as an issue. But unfortunately this fix wouldn't solve the problem.

@jsmnhou
Copy link
Contributor Author

jsmnhou commented Dec 7, 2024

@jonespm That makes sense, thank you so much for the reply! It definitely seems like I was on the wrong track. I'll go ahead and close this PR for now and open a new one if I get the chance to take a second look!

@jsmnhou jsmnhou closed this Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphQL needs to support pagination
2 participants