From 46e4e83ec02fb425e8e39efd8b7e1bcb6213dacd Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 25 Mar 2025 15:43:00 -0500 Subject: [PATCH 1/3] Add models for GitHub App --- readthedocs/oauth/admin.py | 10 ++ readthedocs/oauth/constants.py | 2 + .../oauth/migrations/0018_githubapp.py | 117 +++++++++++++ readthedocs/oauth/models.py | 154 ++++++++++++++++++ 4 files changed, 283 insertions(+) create mode 100644 readthedocs/oauth/migrations/0018_githubapp.py diff --git a/readthedocs/oauth/admin.py b/readthedocs/oauth/admin.py index 853e0492048..cad63237a67 100644 --- a/readthedocs/oauth/admin.py +++ b/readthedocs/oauth/admin.py @@ -2,12 +2,22 @@ from django.contrib import admin +from .models import GitHubAppInstallation from .models import RemoteOrganization from .models import RemoteOrganizationRelation from .models import RemoteRepository from .models import RemoteRepositoryRelation +@admin.register(GitHubAppInstallation) +class GitHubAppInstallationAdmin(admin.ModelAdmin): + list_display = ( + "installation_id", + "target_type", + "target_id", + ) + + @admin.register(RemoteRepository) class RemoteRepositoryAdmin(admin.ModelAdmin): """Admin configuration for the RemoteRepository model.""" diff --git a/readthedocs/oauth/constants.py b/readthedocs/oauth/constants.py index 454c1e6e957..aa307b60a9c 100644 --- a/readthedocs/oauth/constants.py +++ b/readthedocs/oauth/constants.py @@ -1,9 +1,11 @@ GITHUB = "github" +GITHUB_APP = "githubapp" GITLAB = "gitlab" BITBUCKET = "bitbucket" VCS_PROVIDER_CHOICES = ( (GITHUB, "GitHub"), + (GITHUB_APP, "GitHub"), (GITLAB, "GitLab"), (BITBUCKET, "Bitbucket"), ) diff --git a/readthedocs/oauth/migrations/0018_githubapp.py b/readthedocs/oauth/migrations/0018_githubapp.py new file mode 100644 index 00000000000..e8b28ec0924 --- /dev/null +++ b/readthedocs/oauth/migrations/0018_githubapp.py @@ -0,0 +1,117 @@ +# Generated by Django 4.2.18 on 2025-02-03 21:58 +import django.db.models.deletion +import django_extensions.db.fields +from django.db import migrations +from django.db import models +from django_safemigrate import Safe + + +class Migration(migrations.Migration): + safe = Safe.before_deploy + + dependencies = [ + ("oauth", "0017_remove_unused_indexes"), + ] + + operations = [ + migrations.CreateModel( + name="GitHubAppInstallation", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "created", + django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, verbose_name="created" + ), + ), + ( + "modified", + django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, verbose_name="modified" + ), + ), + ( + "installation_id", + models.PositiveBigIntegerField( + db_index=True, + help_text="The application installation ID", + unique=True, + ), + ), + ( + "target_id", + models.PositiveBigIntegerField( + help_text="A GitHub account ID, it can be from a user or an organization" + ), + ), + ( + "target_type", + models.CharField( + choices=[("User", "User"), ("Organization", "Organization")], + help_text="Account type that the target_id belongs to (user or organization)", + max_length=255, + ), + ), + ( + "extra_data", + models.JSONField( + default=dict, + help_text="Extra data returned by the webhook when the installation is created", + ), + ), + ], + options={ + "get_latest_by": "modified", + "verbose_name": "GitHub app installation", + "abstract": False, + }, + ), + migrations.AddField( + model_name="remoterepository", + name="github_app_installation", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="repositories", + to="oauth.githubappinstallation", + verbose_name="GitHub App Installation", + ), + ), + migrations.AlterField( + model_name="remoteorganization", + name="vcs_provider", + field=models.CharField( + choices=[ + ("github", "GitHub"), + ("githubapp", "GitHub"), + ("gitlab", "GitLab"), + ("bitbucket", "Bitbucket"), + ], + max_length=32, + verbose_name="VCS provider", + ), + ), + migrations.AlterField( + model_name="remoterepository", + name="vcs_provider", + field=models.CharField( + choices=[ + ("github", "GitHub"), + ("githubapp", "GitHub"), + ("gitlab", "GitLab"), + ("bitbucket", "Bitbucket"), + ], + max_length=32, + verbose_name="VCS provider", + ), + ), + ] diff --git a/readthedocs/oauth/models.py b/readthedocs/oauth/models.py index fe622d30906..59b452b17e5 100644 --- a/readthedocs/oauth/models.py +++ b/readthedocs/oauth/models.py @@ -1,5 +1,7 @@ """OAuth service models.""" +from functools import cached_property + import structlog from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User @@ -12,6 +14,7 @@ from readthedocs.projects.constants import REPO_CHOICES from readthedocs.projects.models import Project +from .constants import GITHUB_APP from .constants import VCS_PROVIDER_CHOICES from .querysets import RemoteOrganizationQuerySet from .querysets import RemoteRepositoryQuerySet @@ -20,6 +23,146 @@ log = structlog.get_logger(__name__) +class GitHubAppInstallationManager(models.Manager): + def get_or_create_installation( + self, *, installation_id, target_id, target_type, extra_data=None + ): + """ + Get or create a GitHub app installation. + + Only the installation_id is unique, the target_id and target_type could change, + but this should never happen. + """ + installation, created = self.get_or_create( + installation_id=installation_id, + defaults={ + "target_id": target_id, + "target_type": target_type, + "extra_data": extra_data or {}, + }, + ) + # NOTE: An installation can't change its target_id or target_type. + # This should never happen, unless this assumption is wrong. + if installation.target_id != target_id or installation.target_type != target_type: + log.exception( + "Installation target_id or target_type changed", + installation_id=installation.installation_id, + target_id=installation.target_id, + target_type=installation.target_type, + new_target_id=target_id, + new_target_type=target_type, + ) + installation.target_id = target_id + installation.target_type = target_type + installation.save() + return installation, created + + +class GitHubAccountType(models.TextChoices): + USER = "User", _("User") + ORGANIZATION = "Organization", _("Organization") + + +class GitHubAppInstallation(TimeStampedModel): + installation_id = models.PositiveBigIntegerField( + help_text=_("The application installation ID"), + unique=True, + db_index=True, + ) + target_id = models.PositiveBigIntegerField( + help_text=_("A GitHub account ID, it can be from a user or an organization"), + ) + target_type = models.CharField( + help_text=_("Account type that the target_id belongs to (user or organization)"), + choices=GitHubAccountType.choices, + max_length=255, + ) + extra_data = models.JSONField( + help_text=_("Extra data returned by the webhook when the installation is created"), + default=dict, + ) + + objects = GitHubAppInstallationManager() + + class Meta(TimeStampedModel.Meta): + verbose_name = _("GitHub app installation") + + @cached_property + def service(self): + """Return the service for this installation.""" + from readthedocs.oauth.services import GitHubAppService + + return GitHubAppService(self) + + def delete(self, *args, **kwargs): + """Override delete method to remove orphaned organizations.""" + self.delete_repositories() + return super().delete(*args, **kwargs) + + def delete_repositories(self, repository_ids: list[int] | None = None): + """ + Delete repositories linked to this installation. + + When an installation is deleted, we delete all its remote repositories + and relations, users will need to manually link the projects to each repository again. + + We also remove organizations that don't have any repositories after removing the repositories. + + :param repository_ids: List of repository ids (remote ID) to delete. + If None, all repositories will be considered for deletion. + """ + if repository_ids is not None and not repository_ids: + log.info("No repositories to delete") + return + + remote_organizations = RemoteOrganization.objects.filter( + repositories__github_app_installation=self, + vcs_provider=GITHUB_APP, + ) + remote_repositories = self.repositories.filter(vcs_provider=GITHUB_APP) + if repository_ids: + remote_organizations = remote_organizations.filter( + repositories__remote_id__in=repository_ids + ) + remote_repositories = remote_repositories.filter(remote_id__in=repository_ids) + + # Fetch all IDs before deleting the repositories, so we can filter the organizations later. + remote_organizations_ids = list(remote_organizations.values_list("id", flat=True)) + + count, deleted = remote_repositories.delete() + log.info( + "Deleted repositories projects", + count=count, + deleted=deleted, + installation_id=self.installation_id, + ) + + count, deleted = RemoteOrganization.objects.filter( + id__in=remote_organizations_ids, + repositories=None, + ).delete() + log.info( + "Deleted orphaned organizations", + count=count, + deleted=deleted, + installation_id=self.installation_id, + ) + + def delete_organization(self, organization_id: int): + """Delete an organization and all its repositories and relations from the database.""" + count, deleted = RemoteOrganization.objects.filter( + remote_id=str(organization_id), + vcs_provider=GITHUB_APP, + ).delete() + log.info( + "Deleted organization", + count=count, + deleted=deleted, + organization_id=organization_id, + installation_id=self.installation_id, + ) + + class RemoteOrganization(TimeStampedModel): """ Organization from remote service. @@ -173,6 +316,17 @@ class RemoteRepository(TimeStampedModel): remote_id = models.CharField(max_length=128) vcs_provider = models.CharField(_("VCS provider"), choices=VCS_PROVIDER_CHOICES, max_length=32) + github_app_installation = models.ForeignKey( + GitHubAppInstallation, + verbose_name=_("GitHub App Installation"), + related_name="repositories", + null=True, + blank=True, + # When an installation is deleted, we delete all its remote repositories + # and relations, users will need to manually link the projects to each repository again. + on_delete=models.CASCADE, + ) + objects = RemoteRepositoryQuerySet.as_manager() class Meta: From b40c38066b25cef55fa5be8e31a441e802c106b1 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 25 Mar 2025 15:47:22 -0500 Subject: [PATCH 2/3] We don't need these yet --- readthedocs/oauth/models.py | 78 ------------------------------------- 1 file changed, 78 deletions(-) diff --git a/readthedocs/oauth/models.py b/readthedocs/oauth/models.py index 59b452b17e5..2f177ce6314 100644 --- a/readthedocs/oauth/models.py +++ b/readthedocs/oauth/models.py @@ -1,7 +1,5 @@ """OAuth service models.""" -from functools import cached_property - import structlog from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User @@ -14,7 +12,6 @@ from readthedocs.projects.constants import REPO_CHOICES from readthedocs.projects.models import Project -from .constants import GITHUB_APP from .constants import VCS_PROVIDER_CHOICES from .querysets import RemoteOrganizationQuerySet from .querysets import RemoteRepositoryQuerySet @@ -87,81 +84,6 @@ class GitHubAppInstallation(TimeStampedModel): class Meta(TimeStampedModel.Meta): verbose_name = _("GitHub app installation") - @cached_property - def service(self): - """Return the service for this installation.""" - from readthedocs.oauth.services import GitHubAppService - - return GitHubAppService(self) - - def delete(self, *args, **kwargs): - """Override delete method to remove orphaned organizations.""" - self.delete_repositories() - return super().delete(*args, **kwargs) - - def delete_repositories(self, repository_ids: list[int] | None = None): - """ - Delete repositories linked to this installation. - - When an installation is deleted, we delete all its remote repositories - and relations, users will need to manually link the projects to each repository again. - - We also remove organizations that don't have any repositories after removing the repositories. - - :param repository_ids: List of repository ids (remote ID) to delete. - If None, all repositories will be considered for deletion. - """ - if repository_ids is not None and not repository_ids: - log.info("No repositories to delete") - return - - remote_organizations = RemoteOrganization.objects.filter( - repositories__github_app_installation=self, - vcs_provider=GITHUB_APP, - ) - remote_repositories = self.repositories.filter(vcs_provider=GITHUB_APP) - if repository_ids: - remote_organizations = remote_organizations.filter( - repositories__remote_id__in=repository_ids - ) - remote_repositories = remote_repositories.filter(remote_id__in=repository_ids) - - # Fetch all IDs before deleting the repositories, so we can filter the organizations later. - remote_organizations_ids = list(remote_organizations.values_list("id", flat=True)) - - count, deleted = remote_repositories.delete() - log.info( - "Deleted repositories projects", - count=count, - deleted=deleted, - installation_id=self.installation_id, - ) - - count, deleted = RemoteOrganization.objects.filter( - id__in=remote_organizations_ids, - repositories=None, - ).delete() - log.info( - "Deleted orphaned organizations", - count=count, - deleted=deleted, - installation_id=self.installation_id, - ) - - def delete_organization(self, organization_id: int): - """Delete an organization and all its repositories and relations from the database.""" - count, deleted = RemoteOrganization.objects.filter( - remote_id=str(organization_id), - vcs_provider=GITHUB_APP, - ).delete() - log.info( - "Deleted organization", - count=count, - deleted=deleted, - organization_id=organization_id, - installation_id=self.installation_id, - ) - class RemoteOrganization(TimeStampedModel): """ From c68f6c43ee89d3504392c5eee6217a013a783f55 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 26 Mar 2025 09:49:52 -0500 Subject: [PATCH 3/3] Updates from review --- readthedocs/oauth/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/oauth/models.py b/readthedocs/oauth/models.py index 2f177ce6314..7d0a678c786 100644 --- a/readthedocs/oauth/models.py +++ b/readthedocs/oauth/models.py @@ -42,7 +42,7 @@ def get_or_create_installation( # This should never happen, unless this assumption is wrong. if installation.target_id != target_id or installation.target_type != target_type: log.exception( - "Installation target_id or target_type changed", + "Installation target_id or target_type changed. This shouldn't happen -- look into it", installation_id=installation.installation_id, target_id=installation.target_id, target_type=installation.target_type,