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

Eliminate N+1 API requests in issues #135

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Eliminate N+1 API requests in issues #135

merged 1 commit into from
Jun 6, 2024

Conversation

shrik450
Copy link
Collaborator

@shrik450 shrik450 commented Jun 5, 2024

Fixes #70.

This issue was caused by Issue fetching the repository every time it is initialized via parse_response. This leads to an N+1 query, which can be especially cumbersome when fetching all issues of a repository with many issues. This commit fixes that by propagating the repository from the caller of Issue.parse_response, and eager loading it only in cases where one issue at a time is loaded.

Fixes #70.

This issue was caused by `Issue` fetching the repository every time it
is initialized via `parse_response`. This leads to an N+1 query,
which can be especially cumbersome when fetching all issues of a
repository with many issues. This commit fixes that by propagating the
repository from the caller of `Issue.parse_response`, and eager loading
it only in cases where one issue at a time is loaded.
@shrik450 shrik450 requested a review from a team as a code owner June 5, 2024 18:26
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the tests exercise this code-path?

@shrik450
Copy link
Collaborator Author

shrik450 commented Jun 6, 2024

Yep, there's many tests which call Repository.get_issues, Issue.request, Repository.create_issue etc.

@shrik450 shrik450 merged commit c6ca88d into main Jun 6, 2024
4 checks passed
@shrik450 shrik450 deleted the su/issues-n branch June 6, 2024 14:50
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.

N+1 query when fetching repo issues
3 participants