diff --git a/CHANGELOG.md b/CHANGELOG.md index 924be69..73c5514 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ and this project attempts to adhere to [Semantic Versioning](https://semver.org/ - Added `@gh.mention` decorator for handling GitHub mentions in comments. Supports filtering by username pattern (exact match or regex) and scope (issues, PRs, or commits). +### Fixed + +- Fixed N+1 query pattern in `installation_repositories` event handlers by fetching all existing repository IDs in a single query instead of checking each repository individually. + ## [0.7.0] ### Added diff --git a/src/django_github_app/events/ainstallation.py b/src/django_github_app/events/ainstallation.py index 1adf0ba..bd6ac3d 100644 --- a/src/django_github_app/events/ainstallation.py +++ b/src/django_github_app/events/ainstallation.py @@ -43,17 +43,4 @@ async def async_installation_data(event: sansio.Event, gh: GitHubAPI, *args, **k async def async_installation_repositories( event: sansio.Event, gh: GitHubAPI, *args, **kwargs ): - removed = [repo["id"] for repo in event.data["repositories_removed"]] - added = [ - Repository( - installation=await Installation.objects.aget_from_event(event), - repository_id=repo["id"], - repository_node_id=repo["node_id"], - full_name=repo["full_name"], - ) - for repo in event.data["repositories_added"] - if not await Repository.objects.filter(repository_id=repo["id"]).aexists() - ] - - await Repository.objects.filter(repository_id__in=removed).adelete() - await Repository.objects.abulk_create(added) + await Repository.objects.async_repositories_from_event(event) diff --git a/src/django_github_app/events/installation.py b/src/django_github_app/events/installation.py index 872f6f9..c761088 100644 --- a/src/django_github_app/events/installation.py +++ b/src/django_github_app/events/installation.py @@ -39,17 +39,4 @@ def sync_installation_data(event: sansio.Event, gh: GitHubAPI, *args, **kwargs): @gh.event("installation_repositories") def sync_installation_repositories(event: sansio.Event, gh: GitHubAPI, *args, **kwargs): - removed = [repo["id"] for repo in event.data["repositories_removed"]] - added = [ - Repository( - installation=Installation.objects.get_from_event(event), - repository_id=repo["id"], - repository_node_id=repo["node_id"], - full_name=repo["full_name"], - ) - for repo in event.data["repositories_added"] - if not Repository.objects.filter(repository_id=repo["id"]).exists() - ] - - Repository.objects.filter(repository_id__in=removed).delete() - Repository.objects.bulk_create(added) + Repository.objects.sync_repositories_from_event(event) diff --git a/src/django_github_app/models.py b/src/django_github_app/models.py index ad244bb..44f8067 100644 --- a/src/django_github_app/models.py +++ b/src/django_github_app/models.py @@ -4,7 +4,9 @@ from enum import Enum from typing import Any +from asgiref.sync import sync_to_async from django.db import models +from django.db import transaction from django.utils import timezone from gidgethub import abc from gidgethub import sansio @@ -211,6 +213,45 @@ async def aget_from_event(self, event: sansio.Event): except Repository.DoesNotExist: return None + def sync_repositories_from_event(self, event: sansio.Event): + if event.event != "installation_repositories": + raise ValueError( + f"Expected 'installation_repositories' event, got '{event.event}'" + ) + + installation = Installation.objects.get_from_event(event) + + repositories_added = event.data["repositories_added"] + repositories_removed = event.data["repositories_removed"] + + existing_repo_ids = set( + self.filter( + repository_id__in=[repo["id"] for repo in repositories_added] + ).values_list("repository_id", flat=True) + ) + added = [ + Repository( + installation=installation, + repository_id=repo["id"], + repository_node_id=repo["node_id"], + full_name=repo["full_name"], + ) + for repo in repositories_added + if repo["id"] not in existing_repo_ids + ] + + removed = [repo["id"] for repo in repositories_removed] + + with transaction.atomic(): + self.bulk_create(added) + self.filter(repository_id__in=removed).delete() + + async def async_repositories_from_event(self, event: sansio.Event): + # Django's `transaction` is not async compatible yet, so we have the sync + # version called using `sync_to_async` -- an inversion from the rest of the library + # only defining async methods + await sync_to_async(self.sync_repositories_from_event)(event) + create_from_gh_data = async_to_sync_method(acreate_from_gh_data) get_from_event = async_to_sync_method(aget_from_event) diff --git a/tests/events/test_ainstallation.py b/tests/events/test_ainstallation.py index 4117e06..73414f5 100644 --- a/tests/events/test_ainstallation.py +++ b/tests/events/test_ainstallation.py @@ -134,7 +134,7 @@ async def test_async_installation_repositories(ainstallation, create_event): } ], } - event = create_event("installation", delivery_id="1234", **data) + event = create_event("installation_repositories", delivery_id="1234", **data) assert await Repository.objects.filter( repository_id=data["repositories_removed"][0]["id"] diff --git a/tests/events/test_installation.py b/tests/events/test_installation.py index 1d3f37a..b27d446 100644 --- a/tests/events/test_installation.py +++ b/tests/events/test_installation.py @@ -131,7 +131,7 @@ def test_sync_installation_repositories(installation, create_event): } ], } - event = create_event("installation", delivery_id="1234", **data) + event = create_event("installation_repositories", delivery_id="1234", **data) assert Repository.objects.filter( repository_id=data["repositories_removed"][0]["id"] diff --git a/tests/test_models.py b/tests/test_models.py index 16e7d76..3c24652 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -438,6 +438,118 @@ def test_get_from_event(self, repository, create_event): assert repo.full_name == data["repository"]["full_name"] assert repo.installation_id == repository.installation.id + def test_sync_repositories_from_event(self, installation, create_event): + existing_repo = baker.make( + "django_github_app.Repository", + installation=installation, + repository_id=seq.next(), + repository_node_id="existing_node", + full_name="owner/existing", + ) + repo_to_remove = baker.make( + "django_github_app.Repository", + installation=installation, + repository_id=seq.next(), + repository_node_id="remove_node", + full_name="owner/to_remove", + ) + + event = create_event( + "installation_repositories", + installation={"id": installation.installation_id}, + repositories_added=[ + { + "id": existing_repo.repository_id, + "node_id": "existing_node", + "full_name": "owner/existing", + }, + { + "id": seq.next(), + "node_id": "new_node", + "full_name": "owner/new", + }, + ], + repositories_removed=[ + {"id": repo_to_remove.repository_id}, + ], + ) + + Repository.objects.sync_repositories_from_event(event) + + assert Repository.objects.filter( + repository_id=existing_repo.repository_id + ).exists() + assert not Repository.objects.filter( + repository_id=repo_to_remove.repository_id + ).exists() + assert Repository.objects.filter(full_name="owner/new").exists() + assert Repository.objects.filter(installation=installation).count() == 2 + + @pytest.mark.asyncio + async def test_async_repositories_from_event(self, ainstallation, create_event): + existing_repo = await sync_to_async(baker.make)( + "django_github_app.Repository", + installation=ainstallation, + repository_id=seq.next(), + repository_node_id="existing_node", + full_name="owner/existing", + ) + repo_to_remove = await sync_to_async(baker.make)( + "django_github_app.Repository", + installation=ainstallation, + repository_id=seq.next(), + repository_node_id="remove_node", + full_name="owner/to_remove", + ) + + event = create_event( + "installation_repositories", + installation={"id": ainstallation.installation_id}, + repositories_added=[ + { + "id": existing_repo.repository_id, + "node_id": "existing_node", + "full_name": "owner/existing", + }, + { + "id": seq.next(), + "node_id": "new_node", + "full_name": "owner/new", + }, + ], + repositories_removed=[ + {"id": repo_to_remove.repository_id}, + ], + ) + + await Repository.objects.async_repositories_from_event(event) + + assert await Repository.objects.filter( + repository_id=existing_repo.repository_id + ).aexists() + assert not await Repository.objects.filter( + repository_id=repo_to_remove.repository_id + ).aexists() + assert await Repository.objects.filter(full_name="owner/new").aexists() + assert await Repository.objects.filter(installation=ainstallation).acount() == 2 + + def test_sync_repositories_from_event_wrong_event_type(self, create_event): + event = create_event("push") + + with pytest.raises( + ValueError, match="Expected 'installation_repositories' event" + ): + Repository.objects.sync_repositories_from_event(event) + + @pytest.mark.asyncio + async def test_async_repositories_from_event_wrong_event_type(self, create_event): + event = create_event("push") + + with pytest.raises( + ValueError, match="Expected 'installation_repositories' event" + ): + await Repository.objects.async_repositories_from_event(event) + class TestRepository: def test_get_gh_client(self, repository):