Backend support for github provider

backend/code_review_backend/app/settings.py

@@ -212,7 +212,7 @@
 PHABRICATOR_HOST = "https://phabricator.services.mozilla.com"
 
 # Limit the automatic creation of reositories to allowed hosts
-ALLOWED_REPOSITORY_HOSTS = ["hg.mozilla.org"]
+ALLOWED_REPOSITORY_HOSTS = ["hg.mozilla.org", "github.com"]
 
 DYNO = env("DYNO")
 # Heroku settings override to run the web app through dyno

backend/code_review_backend/issues/admin.py

@@ -14,19 +14,30 @@ class RepositoryAdmin(admin.ModelAdmin):
 class DiffInline(admin.TabularInline):
     # Read only inline
     model = Diff
-    readonly_fields = ("id", "repository", "mercurial_hash", "phid", "review_task_id")
+    readonly_fields = (
+        "id",
+        "repository",
+        "mercurial_hash",
+        "provider_id",
+        "review_task_id",
+    )
 
 
 class RevisionAdmin(admin.ModelAdmin):
     list_display = (
         "id",
-        "phabricator_id",
+        "provider",
+        "provider_id",
         "title",
         "bugzilla_id",
         "base_repository",
         "head_repository",
     )
-    list_filter = ("base_repository", "head_repository")
+    list_filter = (
+        "base_repository",
+        "head_repository",
+        "provider",
+    )
     inlines = (DiffInline,)
 
 

backend/code_review_backend/issues/api.py

@@ -6,6 +6,7 @@
 from datetime import date, datetime, timedelta
 
 from django.conf import settings
+from django.core.exceptions import BadRequest
 from django.db.models import BooleanField, Count, ExpressionWrapper, Prefetch, Q
 from django.db.models.functions import TruncDate
 from django.shortcuts import get_object_or_404
@@ -77,10 +78,15 @@ def create(self, request, *args, **kwargs):
         # When a revision already exists with that phabricator ID we return its data without creating a new one
         # This value is used by the bot to identify a revision and publish new Phabricator diffs.
         # The phabricator ID can be null (on mozilla-central) so we must always try to create a revision for that case
-        phabricator_id = request.data["phabricator_id"]
-        if phabricator_id is not None:
+        try:
+            provider = request.data["provider"]
+            provider_id = request.data["provider_id"]
+        except KeyError:
+            raise BadRequest("Invalid provider identification")
+        if provider_id is not None:
             if revision := Revision.objects.filter(
-                phabricator_id=phabricator_id
+                provider=provider,
+                provider_id=provider_id,
             ).first():
                 serializer = RevisionSerializer(
                     instance=revision, context={"request": request}
@@ -166,7 +172,7 @@ def get_queryset(self):
         if query is not None:
             search_query = (
                 Q(id__icontains=query)
-                | Q(revision__phabricator_id__icontains=query)
+                | Q(revision__provider_id__icontains=query)
                 | Q(revision__bugzilla_id__icontains=query)
                 | Q(revision__title__icontains=query)
             )
@@ -200,14 +206,14 @@ class IssueViewSet(
 
     def get_queryset(self):
         # Required to generate the OpenAPI documentation
-        if not self.kwargs.get("diff_id"):
+        if not self.kwargs.get("diff_provider_id"):
             return Issue.objects.none()
-        diff = get_object_or_404(Diff, id=self.kwargs["diff_id"])
+        diff = get_object_or_404(Diff, provider_id=self.kwargs["diff_provider_id"])
         return (
             Issue.objects.filter(issue_links__diff=diff)
             .annotate(publishable=Q(issue_links__in_patch=True) & Q(level=LEVEL_ERROR))
             .values(
-                "id",
+                "provider_id",
                 "hash",
                 "analyzer",
                 "analyzer_check",
@@ -497,7 +503,9 @@ def get_queryset(self):
     basename="revision-diffs",
 )
 router.register(r"diff", DiffViewSet, basename="diffs")
-router.register(r"diff/(?P<diff_id>\d+)/issues", IssueViewSet, basename="issues")
+router.register(
+    r"diff/(?P<diff_provider_id>[0-9a-zA-Z-]+)/issues", IssueViewSet, basename="issues"
+)
 urls = router.urls + [
     path(
         "revision/<int:revision_id>/issues/",

backend/code_review_backend/issues/compare.py

@@ -12,7 +12,8 @@ def detect_new_for_revision(diff: Diff, path: str, hash: str) -> bool:
     assert diff is not None, "Missing diff"
     return not IssueLink.objects.filter(
         revision_id=diff.revision_id,
-        diff_id__lt=diff.id,
+        # Ideally we would rely on the diff integer ID, but that information was lost adding the Github support
+        diff__created__lt=diff.created,
         issue__path=path,
         issue__hash=hash,
     ).exists()

backend/code_review_backend/issues/management/commands/load_in_patch.py

@@ -79,11 +79,11 @@ def process_diff(diff: Diff):
             detect_in_patch(issue_link, lines) for issue_link in diff.issue_links.all()
         ]
         logging.info(
-            f"Found {len([i for i in issue_links if i.in_patch])} issue link in patch for {diff.id}"
+            f"Found {len([i for i in issue_links if i.in_patch])} issue link in patch for {diff.provider_id}"
         )
         IssueLink.objects.bulk_update(issue_links, ["in_patch"])
     except Exception as e:
-        logging.info(f"Failure on diff {diff.id}: {e}")
+        logging.info(f"Failure on diff {diff.provider_id}: {e}")
 
 
 class Command(BaseCommand):

backend/code_review_backend/issues/management/commands/load_issues.py

@@ -213,9 +213,9 @@ def build_revision_and_diff(self, data, task_id):
             return None, None
 
         revision, _ = head_repository.head_revisions.get_or_create(
-            phabricator_id=data["id"],
+            provider="phabricator",
+            provider_id=data["id"],
             defaults={
-                "phabricator_phid": data["phid"],
                 "title": data["title"],
                 "bugzilla_id": int(data["bugzilla_id"])
                 if data["bugzilla_id"]
@@ -224,10 +224,9 @@ def build_revision_and_diff(self, data, task_id):
             },
         )
         diff, _ = revision.diffs.get_or_create(
-            id=data["diff_id"],
+            provider_id=data["diff_phid"],
             defaults={
                 "repository": head_repository,
-                "phid": data["diff_phid"],
                 "review_task_id": task_id,
                 "mercurial_hash": data["mercurial_revision"],
             },

backend/code_review_backend/issues/migrations/0016_revision_diff_generic_provider.py

@@ -0,0 +1,55 @@
+# Generated by Django 5.1.6 on 2026-01-21 16:01
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("issues", "0015_remove_repository_phid_alter_repository_id"),
+    ]
+
+    operations = [
+        migrations.AlterModelOptions(
+            name="revision",
+            options={"ordering": ("provider", "provider_id", "id")},
+        ),
+        migrations.RemoveConstraint(
+            model_name="revision",
+            name="revision_unique_phab_id",
+        ),
+        migrations.RenameField(
+            model_name="revision",
+            old_name="phabricator_id",
+            new_name="provider_id",
+        ),
+        migrations.AddField(
+            model_name="revision",
+            name="provider",
+            field=models.CharField(
+                choices=[("phabricator", "Phabricator"), ("github", "Github")],
+                default="phabricator",
+                max_length=20,
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="revision",
+            constraint=models.UniqueConstraint(
+                condition=models.Q(("provider_id__isnull", False)),
+                fields=("provider_id",),
+                name="revision_unique_phab_id",
+            ),
+        ),
+        migrations.RemoveConstraint(
+            model_name="revision",
+            name="revision_unique_phab_phabid",
+        ),
+        migrations.RemoveField(
+            model_name="revision",
+            name="phabricator_phid",
+        ),
+        migrations.RenameField(
+            model_name="diff",
+            old_name="phid",
+            new_name="provider_id",
+        ),
+    ]

backend/code_review_backend/issues/migrations/0017_diff_auto_pk.py

@@ -0,0 +1,67 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from django.db import migrations, models
+from django.db.models import F
+
+
+def _update_provider_id(apps, schema_editor):
+    """
+    Update all diffs, setting the current ID as the provider ID.
+    This step is required no to loose the Phabricator integer ID,
+    which is the reference used by most developers.
+    The previous value (PHID-DIFF-xxxxxxxxxxxxxxxxxxxx) is dropped,
+    then the PK is replaced by a random UUID, allowing the support
+    of other providers without a known unique integer identifier.
+    """
+    Diff = apps.get_model("issues", "Diff")
+    Diff.objects.update(provider_id=F("id"))
+
+
+def _reset_postgres_sequense(apps, schema_editor):
+    """
+    PostgreSQL needs to update the auto increment sequence value, otherwise
+    conflicts would happen when we reach the initial Diff ID.
+
+    This command was initially generated with ./manage.py sqlsequencereset issues
+    """
+    if schema_editor.connection.vendor != "postgresql":
+        # The SQLite backend automatically handles the auto increment update
+        return
+    schema_editor.execute(
+        """SELECT setval(
+            pg_get_serial_sequence('"issues_diff"','id'),
+            coalesce(max("id"), 1),
+            max("id") IS NOT null
+        ) FROM "issues_diff";
+        """,
+    )
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("issues", "0016_revision_diff_generic_provider"),
+    ]
+
+    operations = [
+        # Save the Phabricator integer ID in Diff.provider_id
+        migrations.RunPython(
+            _update_provider_id,
+            reverse_code=migrations.RunPython.noop,
+        ),
+        migrations.AlterField(
+            model_name="diff",
+            name="id",
+            field=models.AutoField(primary_key=True, serialize=False),
+        ),
+        migrations.AlterModelOptions(
+            name="diff",
+            options={"ordering": ("created",)},
+        ),
+        # Restore the sequence with PostgreSQL backend
+        migrations.RunPython(
+            _reset_postgres_sequense,
+            reverse_code=migrations.RunPython.noop,
+        ),
+    ]

backend/code_review_backend/issues/models.py

@@ -13,6 +13,13 @@
 LEVEL_ERROR = "error"
 ISSUE_LEVELS = ((LEVEL_WARNING, "Warning"), (LEVEL_ERROR, "Error"))
 
+PROVIDER_PHABRICATOR = "phabricator"
+PROVIDER_GITHUB = "github"
+PROVIDERS = (
+    (PROVIDER_PHABRICATOR, "Phabricator"),
+    (PROVIDER_GITHUB, "Github"),
+)
+
 
 class Repository(models.Model):
     id = models.AutoField(primary_key=True)
@@ -33,11 +40,12 @@ def __str__(self):
 
 class Revision(models.Model):
     id = models.BigAutoField(primary_key=True)
+
     # Phabricator references will be left empty when ingesting a decision task (e.g. from MC or autoland)
-    phabricator_id = models.PositiveIntegerField(unique=True, null=True, blank=True)
-    phabricator_phid = models.CharField(
-        max_length=40, unique=True, null=True, blank=True
+    provider = models.CharField(
+        max_length=20, choices=PROVIDERS, default=PROVIDER_PHABRICATOR
     )
+    provider_id = models.PositiveIntegerField(unique=True, null=True, blank=True)
 
     created = models.DateTimeField(auto_now_add=True)
     updated = models.DateTimeField(auto_now=True)
@@ -72,43 +80,45 @@ class Revision(models.Model):
     bugzilla_id = models.PositiveIntegerField(null=True)
 
     class Meta:
-        ordering = ("phabricator_id", "id")
+        ordering = ("provider", "provider_id", "id")
 
         indexes = (models.Index(fields=["head_repository", "head_changeset"]),)
         constraints = [
             models.UniqueConstraint(
-                fields=["phabricator_id"],
+                fields=["provider_id"],
                 name="revision_unique_phab_id",
-                condition=Q(phabricator_id__isnull=False),
-            ),
-            models.UniqueConstraint(
-                fields=["phabricator_phid"],
-                name="revision_unique_phab_phabid",
-                condition=Q(phabricator_phid__isnull=False),
+                condition=Q(provider_id__isnull=False),
             ),
         ]
 
     def __str__(self):
-        if self.phabricator_id is not None:
-            return f"D{self.phabricator_id} - {self.title}"
+        if self.provider == PROVIDER_PHABRICATOR and self.provider_id is not None:
+            return f"Phabricator D{self.provider_id} - {self.title}"
         return f"#{self.id} - {self.title}"
 
     @property
-    def phabricator_url(self):
-        if self.phabricator_id is None:
+    def url(self):
+        if self.provider_id is None:
             return
-        parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST)
-        return f"{parser.scheme}://{parser.netloc}/D{self.phabricator_id}"
+
+        if self.provider == PROVIDER_PHABRICATOR:
+            parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST)
+            return f"{parser.scheme}://{parser.netloc}/D{self.provider_id}"
+        elif self.provider == PROVIDER_GITHUB:
+            return f"{self.base_repository.url}/issues/{self.provider_id}"
+        else:
+            raise NotImplementedError
 
 
 class Diff(models.Model):
-    """Reference of a specific code patch (diff) in Phabricator.
+    """Reference of a specific code patch (diff) in Phabricator or Github.
     A revision can be linked to multiple successive diffs, or none in case of a repository push.
     """
 
-    # Phabricator's attributes
-    id = models.PositiveIntegerField(primary_key=True)
-    phid = models.CharField(max_length=40, unique=True)
+    id = models.AutoField(primary_key=True)
+
+    # Provider ID is unique among all providers (integer for Phabricator, hash for github)
+    provider_id = models.CharField(max_length=40, unique=True)
     created = models.DateTimeField(auto_now_add=True)
     updated = models.DateTimeField(auto_now=True)
 
@@ -126,10 +136,10 @@ class Diff(models.Model):
     )
 
     def __str__(self):
-        return f"Diff {self.id}"
+        return f"Diff {self.provider_id}"
 
     class Meta:
-        ordering = ("id",)
+        ordering = ("created",)
 
 
 class IssueLink(models.Model):