fix N+1 query in "installation_repositories" event handlers (#97)

Author: joshuadavidthomas

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

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)

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)

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)
 

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"]

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"]

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):