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

Added pagination to fetch_all method #345

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

stevenpince
Copy link
Contributor

@stevenpince stevenpince commented Nov 15, 2024

Problem
Zou has the ability to provide pagination, but Gazu ignores this functionality at the moment.

I used the get_preview_files_for_project method exposed at the /data/preview-files webhook to fetch all preview files, it becomes impossible to fetch these records. Zou yields a 502 Gateway Error. I am aware that this is an absurd query, but it's a good benchmark.

Pagination is available in Zou: https://github.com/cgwire/zou/blob/8be5dfd1744115ec08eb0c5668db62e1854ebbed/zou/app/services/files_service.py#L902

Solution
I added the "pagination" parameter to "fetch_all", which will cause the method to handle the query in slices of 100.
gazu.client.fetch_all('preview-files', params=project_id, paginated=True)`

This solution also works for Persons, Tasks and other resources that inherit from the ArgsMixin in Zou.

All tests ran green. I think it would be interesting to make this option the default at some point, but will leave it for TD's that need the functionality (and know what they're doing) for now.

Note: Also adding .idea to the .gitignore to support PyCharm (if you'd like to have the run config for the unit tests, let me know.)

@frankrousseau
Copy link
Contributor

Thank you for your contribution. We will look at it very soon.

@frankrousseau
Copy link
Contributor

@EvanBldy Include the merge of this PR in your todo-list.

@EvanBldy EvanBldy force-pushed the master branch 3 times, most recently from d51d1ad to b9c31e3 Compare November 26, 2024 23:38
@EvanBldy
Copy link
Member

Hi @stevenpince, thanks for your PR!
I have simplified some things.

@EvanBldy EvanBldy merged commit cba4bfd into cgwire:main Nov 27, 2024
12 checks passed
@steven-pi
Copy link

Hi @stevenpince, thanks for your PR! I have simplified some things.

The simplification you made broke handling of paginated requests that return less than 100 results.

if not isinstance(responce, dict) or not response.get('data', None) was there instead of the current 'if not paginated' on line 432. I added the comment "Return non-paginated responses" to clarify this.

Zou will return a list instead of a dict if a paginated response has less than 100 records. If a list is returned, the response can just be returned as well (as no pagination is active: results on page 1 are all results).

Currently, it will raise AttributeError: 'list' object has no attribute 'get' on line 435 because the response isn't returned when it's a list.

@EvanBldy
Copy link
Member

EvanBldy commented Dec 6, 2024

Hi @stevenpince, thanks for your PR! I have simplified some things.

The simplification you made broke handling of paginated requests that return less than 100 results.

if not isinstance(responce, dict) or not response.get('data', None) was there instead of the current 'if not paginated' on line 432. I added the comment "Return non-paginated responses" to clarify this.

Zou will return a list instead of a dict if a paginated response has less than 100 records. If a list is returned, the response can just be returned as well (as no pagination is active: results on page 1 are all results).

Currently, it will raise AttributeError: 'list' object has no attribute 'get' on line 435 because the response isn't returned when it's a list.

Hi @steven-pi, on which route it occurs?

I have tried with some of them and I cannot reproduce. This problem has to be fixed on Zou side I think.

@steven-pi
Copy link

Sorry for the late reply - it might be due to my production-deployed version of Zou, either way - the point is still valid: if Zou returns a list instead of the expected dict, (could be due to an older version of Zou talking to the current version of Gazu), Gazu should deal with it properly.

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.

4 participants