Only rely on provider_id

* [x] Update tests

Author: vrigal

backend/code_review_backend/issues/api.py

@@ -206,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",
@@ -503,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

@@ -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,
-                "provider_id": data["diff_phid"],
                 "review_task_id": task_id,
                 "mercurial_hash": data["mercurial_revision"],
             },

backend/code_review_backend/issues/models.py

@@ -136,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):

backend/code_review_backend/issues/serializers.py

@@ -135,13 +135,14 @@ class DiffSerializer(serializers.ModelSerializer):
         queryset=Repository.objects.all(), slug_field="url"
     )
     issues_url = serializers.HyperlinkedIdentityField(
-        view_name="issues-list", lookup_url_kwarg="diff_id"
+        view_name="issues-list",
+        lookup_url_kwarg="diff_provider_id",
+        lookup_field="provider_id",
     )
 
     class Meta:
         model = Diff
         fields = (
-            "id",
             "provider_id",
             "review_task_id",
             "repository",
@@ -163,7 +164,7 @@ class DiffLightSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = Diff
-        fields = ("id", "repository", "revision")
+        fields = ("provider_id", "repository", "revision")
 
 
 class DiffFullSerializer(serializers.ModelSerializer):
@@ -175,7 +176,9 @@ class DiffFullSerializer(serializers.ModelSerializer):
     revision = RevisionSerializer(read_only=True)
     repository = RepositorySerializer(read_only=True)
     issues_url = serializers.HyperlinkedIdentityField(
-        view_name="issues-list", lookup_url_kwarg="diff_id"
+        view_name="issues-list",
+        lookup_url_kwarg="diff_provider_id",
+        lookup_field="provider_id",
     )
     nb_issues = serializers.IntegerField(read_only=True)
     nb_issues_publishable = serializers.IntegerField(read_only=True)
@@ -185,9 +188,8 @@ class DiffFullSerializer(serializers.ModelSerializer):
     class Meta:
         model = Diff
         fields = (
-            "id",
-            "revision",
             "provider_id",
+            "revision",
             "review_task_id",
             "repository",
             "mercurial_hash",
@@ -264,7 +266,8 @@ class SingleIssueBulkSerializer(IssueSerializer):
 
 
 class IssueBulkSerializer(serializers.Serializer):
-    diff_id = serializers.PrimaryKeyRelatedField(
+    diff_provider_id = serializers.SlugRelatedField(
+        slug_field="provider_id",
         # Initialized depending on the revision used for the creation
         queryset=Diff.objects.none(),
         style={"base_template": "input.html"},
@@ -277,11 +280,11 @@ def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         if not self.context.get("revision"):
             return
-        self.fields["diff_id"].queryset = self.context["revision"].diffs.all()
+        self.fields["diff_provider_id"].queryset = self.context["revision"].diffs.all()
 
     @transaction.atomic
     def create(self, validated_data):
-        diff = validated_data.get("diff_id", None)
+        diff = validated_data.get("diff_provider_id", None)
         link_attrs = defaultdict(list)
         # Separate attributes that are specific to the IssueLink M2M
         for issue in validated_data["issues"]:
@@ -340,7 +343,7 @@ def create(self, validated_data):
                 output.append(output_link)
 
         return {
-            "diff_id": diff,
+            "diff_provider_id": diff,
             "issues": output,
         }
 

backend/code_review_backend/issues/tests/test_api.py

@@ -38,8 +38,7 @@ def setUp(self):
             base_repository=self.repo,
         )
         self.diff = self.revision.diffs.create(
-            id=1234,
-            provider_id="PHID-DIFF-xxx",
+            provider_id="PHID-DIFF-1234",
             review_task_id="deadbeef123",
             mercurial_hash="coffee12345",
             repository=self.repo_try,
@@ -153,7 +152,7 @@ def test_create_diff(self):
         self.diff.delete()
         data = {
             "id": 1234,
-            "provider_id": "PHID-DIFF-xxx",
+            "provider_id": "PHID-DIFF-1234",
             "review_task_id": "deadbeef123",
             "mercurial_hash": "coffee12345",
             "repository": "http://repo.test/try",
@@ -181,12 +180,13 @@ def test_create_diff(self):
 
         # Response should have url to create issues
         self.assertEqual(
-            response.json()["issues_url"], "http://testserver/v1/diff/1234/issues/"
+            response.json()["issues_url"],
+            "http://testserver/v1/diff/PHID-DIFF-1234/issues/",
         )
 
         # Check a diff has been created
         self.assertEqual(Diff.objects.count(), 1)
-        diff = Diff.objects.get(pk=1234)
+        diff = Diff.objects.get(provider_id="PHID-DIFF-1234")
         self.assertEqual(diff.mercurial_hash, "coffee12345")
         self.assertEqual(diff.revision, self.revision)
 
@@ -204,13 +204,17 @@ def test_create_issue_disabled(self):
         }
 
         # No auth will give a permission denied
-        response = self.client.post("/v1/diff/1234/issues/", data, format="json")
+        response = self.client.post(
+            "/v1/diff/PHID-DIFF-1234/issues/", data, format="json"
+        )
         self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
         # Once authenticated, creation will work
         self.assertEqual(Issue.objects.count(), 0)
         self.client.force_authenticate(user=self.user)
-        response = self.client.post("/v1/diff/1234/issues/", data, format="json")
+        response = self.client.post(
+            "/v1/diff/PHID-DIFF-1234/issues/", data, format="json"
+        )
         self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)
 
         # Do not check the content of issue created as it's a random UUID
@@ -281,7 +285,7 @@ def test_create_issue_bulk(self):
         self.assertDictEqual(
             issue_data,
             {
-                "diff_id": None,
+                "diff_provider_id": None,
                 "issues": [
                     {
                         "analyzer": "remote-flake8",
@@ -334,7 +338,7 @@ def test_create_issue_bulk_with_diff(self):
         Check we can create issues on a revision with a reference to a diff
         """
         data = {
-            "diff_id": 1234,
+            "diff_provider_id": "PHID-DIFF-1234",
             "issues": [
                 {
                     "hash": "somemd5hash",
@@ -360,7 +364,7 @@ def test_create_issue_bulk_with_diff(self):
         self.assertDictEqual(
             response.json(),
             {
-                "diff_id": 1234,
+                "diff_provider_id": "PHID-DIFF-1234",
                 "issues": [
                     {
                         "analyzer": "remote-flake8",
@@ -551,7 +555,7 @@ def test_create_issue_bulk_duplicate(self):
         self.assertDictEqual(
             issue_data,
             {
-                "diff_id": None,
+                "diff_provider_id": None,
                 # The same issue link is returned twice in the resulting payload
                 "issues": 2
                 * [

backend/code_review_backend/issues/tests/test_diff.py

@@ -67,7 +67,7 @@ def test_list_diffs(self):
                 "previous": None,
                 "results": [
                     {
-                        "id": 3,
+                        "provider_id": "PHID-DIFF-3",
                         "revision": {
                             "id": 1,
                             "base_repository": "http://repo.test/myrepo",
@@ -87,18 +87,17 @@ def test_list_diffs(self):
                             "slug": "myrepo-try",
                             "url": "http://repo.test/try",
                         },
-                        "provider_id": "PHID-DIFF-3",
                         "review_task_id": "task-2",
                         "mercurial_hash": "30b501affc4d3b9c670fc297ab903b406afd5f04",
-                        "issues_url": "http://testserver/v1/diff/3/issues/",
+                        "issues_url": "http://testserver/v1/diff/PHID-DIFF-3/issues/",
                         "nb_issues": 0,
                         "nb_issues_publishable": 0,
                         "nb_warnings": 0,
                         "nb_errors": 0,
                         "created": self.now,
                     },
                     {
-                        "id": 2,
+                        "provider_id": "PHID-DIFF-2",
                         "revision": {
                             "id": 2,
                             "base_repository": "http://repo.test/myrepo",
@@ -118,18 +117,17 @@ def test_list_diffs(self):
                             "slug": "myrepo-try",
                             "url": "http://repo.test/try",
                         },
-                        "provider_id": "PHID-DIFF-2",
                         "review_task_id": "task-1",
                         "mercurial_hash": "32d2a594cfef74fcb524028d1521d0d4bd98bd35",
-                        "issues_url": "http://testserver/v1/diff/2/issues/",
+                        "issues_url": "http://testserver/v1/diff/PHID-DIFF-2/issues/",
                         "nb_issues": 0,
                         "nb_issues_publishable": 0,
                         "nb_warnings": 0,
                         "nb_errors": 0,
                         "created": self.now,
                     },
                     {
-                        "id": 1,
+                        "provider_id": "PHID-DIFF-1",
                         "revision": {
                             "id": 1,
                             "base_repository": "http://repo.test/myrepo",
@@ -149,10 +147,9 @@ def test_list_diffs(self):
                             "slug": "myrepo-try",
                             "url": "http://repo.test/try",
                         },
-                        "provider_id": "PHID-DIFF-1",
                         "review_task_id": "task-0",
                         "mercurial_hash": "a2ac78b7d12d6e55b9b15c1c2048a16c58c6c803",
-                        "issues_url": "http://testserver/v1/diff/1/issues/",
+                        "issues_url": "http://testserver/v1/diff/PHID-DIFF-1/issues/",
                         "nb_issues": 0,
                         "nb_issues_publishable": 0,
                         "nb_warnings": 0,
@@ -172,7 +169,10 @@ def test_filter_repo(self):
         response = self.client.get("/v1/diff/?repository=myrepo")
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(response.json()["count"], 3)
-        self.assertEqual([d["id"] for d in response.json()["results"]], [3, 2, 1])
+        self.assertEqual(
+            [d["provider_id"] for d in response.json()["results"]],
+            ["PHID-DIFF-3", "PHID-DIFF-2", "PHID-DIFF-1"],
+        )
 
         # Missing repo
         response = self.client.get("/v1/diff/?repository=missing")
@@ -188,13 +188,18 @@ def test_search(self):
         response = self.client.get("/v1/diff/?search=10001")
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(response.json()["count"], 1)
-        self.assertEqual([d["id"] for d in response.json()["results"]], [2])
+        self.assertEqual(
+            [d["provider_id"] for d in response.json()["results"]], ["PHID-DIFF-2"]
+        )
 
         # In title
         response = self.client.get("/v1/diff/?search=revision 1")
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(response.json()["count"], 2)
-        self.assertEqual([d["id"] for d in response.json()["results"]], [3, 1])
+        self.assertEqual(
+            [d["provider_id"] for d in response.json()["results"]],
+            ["PHID-DIFF-3", "PHID-DIFF-1"],
+        )
 
     def test_filter_issues(self):
         """
@@ -205,7 +210,10 @@ def test_filter_issues(self):
         response = self.client.get("/v1/diff/?issues=no")
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertEqual(response.json()["count"], 3)
-        self.assertEqual([d["id"] for d in response.json()["results"]], [3, 2, 1])
+        self.assertEqual(
+            [d["provider_id"] for d in response.json()["results"]],
+            ["PHID-DIFF-3", "PHID-DIFF-2", "PHID-DIFF-1"],
+        )
 
         # Any issues
         response = self.client.get("/v1/diff/?issues=any")

backend/code_review_backend/issues/tests/test_stats.py

@@ -245,7 +245,6 @@ def check_issue(issue):
             diffs = issue["diffs"]
             self.assertEqual(len(diffs), 1)
             diff = diffs[0]
-            self.assertTrue(diff["id"] > 0)
             self.assertEqual(diff["repository"], "http://repo.test/myrepo-try")
 
             # Revision