Use API v2 in bot's workflow

backend/code_review_backend/app/urls.py

@@ -11,6 +11,7 @@
 from rest_framework import permissions
 
 from code_review_backend.issues import api
+from code_review_backend.issues.v2 import api as api_v2
 
 # Build Swagger schema view
 schema_view = get_schema_view(
@@ -28,6 +29,7 @@
 urlpatterns = [
     path("", lambda request: redirect("docs/", permanent=False)),
     path("v1/", include(api.urls)),
+    path("v2/", include(api_v2.urls)),
     path("admin/", admin.site.urls),
     path(
         r"docs<format>\.json|\.yaml)",

backend/code_review_backend/issues/v2/api.py

@@ -0,0 +1,127 @@
+# 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.core.exceptions import BadRequest
+from django.db.models import Exists, OuterRef
+from django.shortcuts import get_object_or_404
+from django.urls import path
+from django.utils.functional import cached_property
+from rest_framework.exceptions import ValidationError
+from rest_framework.generics import ListAPIView
+
+from code_review_backend.issues.models import Diff, IssueLink
+from code_review_backend.issues.v2.serializers import DiffIssuesSerializer
+
+
+class IssueList(ListAPIView):
+    """
+    Returns issues that are linked to a diff, and depending on the selected mode:
+    * known: Issues that have also being detected from the base repository (e.g. mozilla-central).
+        `previous_diff_id` is always returned as null when listing known issues.
+    * unresolved: Issues that were linked to the previous diff of the same parent revision, and are
+        still present on the current diff. Filters out issues that are known by the backend.
+    * closed: Issues that were listed on the previous diff of the same parent revision, but does
+        not exist on the current diff.
+
+    This endpoint does not uses pagination.
+    """
+
+    allowed_modes = ["unresolved", "known", "closed"]
+    pagination_class = None
+
+    def get_serializer(self, queryset, many=True):
+        return DiffIssuesSerializer(
+            {
+                "previous_diff_id": self.previous_diff and self.previous_diff.id,
+                "issues": queryset,
+            },
+            context=self.get_serializer_context(),
+        )
+
+    @cached_property
+    def diff(self):
+        return get_object_or_404(
+            Diff.objects.select_related("revision"), id=self.kwargs["diff_id"]
+        )
+
+    @cached_property
+    def previous_diff(self):
+        if self.mode == "known":
+            # No need to search for a previous diff for known issues
+            return None
+        return (
+            self.diff.revision.diffs.filter(created__lt=self.diff.created)
+            .order_by("created")
+            .last()
+        )
+
+    @cached_property
+    def mode(self):
+        if (mode := self.kwargs["mode"]) not in self.allowed_modes:
+            raise ValidationError(
+                detail=f"mode argument must be one of {self.allowed_modes}"
+            )
+        return mode
+
+    def get_queryset(self):
+        return getattr(self, f"{self.mode}_issues").filter(**{self.mode: True})
+
+    def distinct_issues(self, qs):
+        """
+        Convert a list of issue links to unique couples of (issue_id, issue_hash)
+        """
+        attributes = ("issue_id", "issue__hash")
+        return qs.order_by(*attributes).values(*attributes).distinct(*attributes)
+
+    @property
+    def known_issues(self):
+        return self.distinct_issues(
+            self.diff.issue_links.annotate(
+                known=Exists(
+                    IssueLink.objects.filter(
+                        revision__head_repository_id=self.diff.revision.base_repository_id,
+                        issue=OuterRef("issue"),
+                    )
+                )
+            )
+        )
+
+    @property
+    def unresolved_issues(self):
+        if not self.previous_diff:
+            raise BadRequest("No previous diff was found to compare issues")
+        return self.distinct_issues(
+            self.known_issues.filter(known=False).annotate(
+                unresolved=Exists(
+                    IssueLink.objects.filter(
+                        diff=self.previous_diff,
+                        issue=OuterRef("issue"),
+                    )
+                )
+            )
+        )
+
+    @property
+    def closed_issues(self):
+        if not self.previous_diff:
+            raise BadRequest("No previous diff was found to compare issues")
+        return self.distinct_issues(
+            self.previous_diff.issue_links.annotate(
+                closed=~Exists(
+                    IssueLink.objects.filter(
+                        diff=self.diff,
+                        issue=OuterRef("issue"),
+                    )
+                )
+            )
+        )
+
+
+urls = [
+    path(
+        "diff/<int:diff_id>/issues/<str:mode>/",
+        IssueList.as_view(),
+        name="issue-list",
+    ),
+]

backend/code_review_backend/issues/v2/serializers.py

@@ -0,0 +1,19 @@
+from rest_framework import serializers
+
+from code_review_backend.issues.models import IssueLink
+
+
+class IssueLinkHashSerializer(serializers.ModelSerializer):
+    """Serialize an Issue hash from the IssueLink M2M"""
+
+    id = serializers.UUIDField(source="issue_id", read_only=True)
+    hash = serializers.CharField(source="issue__hash", max_length=32, read_only=True)
+
+    class Meta:
+        model = IssueLink
+        fields = ("id", "hash")
+
+
+class DiffIssuesSerializer(serializers.Serializer):
+    previous_diff_id = serializers.UUIDField(allow_null=True, read_only=True)
+    issues = IssueLinkHashSerializer(many=True)

backend/code_review_backend/issues/v2/tests/test_list_issues.py

@@ -0,0 +1,175 @@
+from rest_framework import status
+from rest_framework.test import APITestCase
+
+from code_review_backend.issues.models import Issue, Repository
+
+
+class ListIssuesTestCase(APITestCase):
+    def setUp(self):
+        # Create the main repository
+        self.repo_main = Repository.objects.create(
+            slug="mozilla-central", url="https://hg.mozilla.org/mozilla-central"
+        )
+        self.repo_try = Repository.objects.create(
+            slug="myrepo-try", url="http://repo.test/try"
+        )
+
+        self.issues = Issue.objects.bulk_create(
+            [
+                Issue(**attrs)
+                for attrs in [
+                    {"hash": "0" * 32},
+                    {"hash": "1" * 32},
+                    {"hash": "2" * 32},
+                    {"hash": "3" * 32},
+                ]
+            ]
+        )
+
+        # Create historic revision with some issues
+        self.revision_main = self.repo_main.head_revisions.create(
+            phabricator_id=456,
+            phabricator_phid="PHID-REV-XXX",
+            title="Main revision",
+            base_repository=self.repo_main,
+            head_repository=self.repo_main,
+        )
+        self.revision_main.issue_links.create(issue=self.issues[0])
+        self.revision_main.issue_links.create(issue=self.issues[1])
+
+        # Create a new revions, with two successive diffs
+        self.revision_last = self.repo_try.head_revisions.create(
+            phabricator_id=1337,
+            phabricator_phid="PHID-REV-YYY",
+            title="Bug XXX - Yet another bug",
+            base_repository=self.repo_main,
+            head_repository=self.repo_try,
+        )
+        self.diff1 = self.revision_last.diffs.create(
+            id=1,
+            repository=self.repo_try,
+            review_task_id="deadbeef123",
+            phid="PHID-DIFF-xxx",
+        )
+        self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[0])
+        self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[2])
+        self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[3])
+
+        self.diff2 = self.revision_last.diffs.create(
+            id=2,
+            repository=self.repo_try,
+            review_task_id="coffee12345",
+            phid="PHID-DIFF-yyy",
+        )
+        self.diff2.issue_links.create(revision=self.revision_last, issue=self.issues[0])
+        self.diff2.issue_links.create(revision=self.revision_last, issue=self.issues[2])
+
+    def test_list_issues_invalid_mode(self):
+        with self.assertNumQueries(0):
+            response = self.client.get(
+                f"/v2/diff/{self.diff2.id}/issues/test/", format="json"
+            )
+            self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertListEqual(
+            response.json(),
+            ["mode argument must be one of ['unresolved', 'known', 'closed']"],
+        )
+
+    def test_list_issues_invalid_diff(self):
+        with self.assertNumQueries(1):
+            response = self.client.get("/v2/diff/424242/issues/known/")
+            self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+
+    def test_list_issues_known(self):
+        with self.assertNumQueries(2):
+            response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/known/")
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.maxDiff = None
+        self.assertDictEqual(
+            response.json(),
+            {
+                "previous_diff_id": None,
+                "issues": [
+                    {"id": str(self.issues[0].id), "hash": self.issues[0].hash},
+                ],
+            },
+        )
+
+    def test_list_issues_unresolved_first_diff(self):
+        """No issue can be marked as unresolved on a new diff"""
+        with self.assertNumQueries(2):
+            response = self.client.get(
+                f"/v2/diff/{self.diff1.id}/issues/unresolved/", format="json"
+            )
+            self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+
+    def test_list_issues_unresolved(self):
+        with self.assertNumQueries(3):
+            response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/unresolved/")
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertDictEqual(
+            response.json(),
+            {
+                "previous_diff_id": "1",
+                "issues": [
+                    {"id": str(self.issues[2].id), "hash": self.issues[2].hash},
+                ],
+            },
+        )
+
+    def test_list_issues_closed_first_diff(self):
+        """No issue can be marked as closed on a new diff"""
+        with self.assertNumQueries(2):
+            response = self.client.get(f"/v2/diff/{self.diff1.id}/issues/closed/")
+            self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+
+    def test_list_issues_closed(self):
+        with self.assertNumQueries(3):
+            response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/closed/")
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertDictEqual(
+            response.json(),
+            {
+                "previous_diff_id": "1",
+                "issues": [{"id": str(self.issues[3].id), "hash": self.issues[3].hash}],
+            },
+        )
+
+    def test_list_issues_new_diff_reopen(self):
+        """
+        Open a new diff with 1 known issue and one that has been closed in the previous diff.
+        Both are listed as new, and the two issues of the previous diff listed as closed.
+        """
+        diff = self.revision_last.diffs.create(
+            id=3,
+            repository=self.repo_try,
+            review_task_id="42",
+            phid="PHID-DIFF-42",
+        )
+        new_issue = Issue.objects.create(hash="4" * 32)
+        diff.issue_links.create(revision=self.revision_last, issue=new_issue)
+        diff.issue_links.create(revision=self.revision_last, issue=self.issues[3])
+        self.assertDictEqual(
+            self.client.get(f"/v2/diff/{diff.id}/issues/unresolved/").json(),
+            {
+                "previous_diff_id": "2",
+                "issues": [],
+            },
+        )
+        self.assertDictEqual(
+            self.client.get(f"/v2/diff/{diff.id}/issues/known/").json(),
+            {
+                "previous_diff_id": None,
+                "issues": [],
+            },
+        )
+        self.assertDictEqual(
+            self.client.get(f"/v2/diff/{diff.id}/issues/closed/").json(),
+            {
+                "previous_diff_id": "2",
+                "issues": [
+                    {"id": str(self.issues[0].id), "hash": self.issues[0].hash},
+                    {"id": str(self.issues[2].id), "hash": self.issues[2].hash},
+                ],
+            },
+        )

bot/code_review_bot/backend.py

@@ -6,6 +6,7 @@
 
 import requests
 import structlog
+from requests.exceptions import HTTPError
 
 from code_review_bot import taskcluster
 from code_review_bot.config import GetAppUserAgent, settings
@@ -187,6 +188,32 @@ def list_diff_issues(self, diff_id):
         """
         return list(self.paginate(f"/v1/diff/{diff_id}/issues/"))
 
+    def list_diff_issues_v2(self, diff_id, mode):
+        """
+        List issues for a given diff, applying specific filters.
+        Returns:
+          * The ID of the previous diff if it exists (always null for `known` mode).
+          * A list of issues (dict serializing ID and hash) corresponding to the filter.
+        """
+        assert mode in ("known", "unresolved", "closed")
+        try:
+            data = self.get(f"/v2/diff/{diff_id}/issues/{mode}")
+        except HTTPError as e:
+            if e.response.status_code != 404:
+                logger.warning(
+                    f"Could not list {mode} issues from the bot: {e}. Skipping"
+                )
+            else:
+                logger.info(f"Diff not found in code review backend: {diff_id}")
+            return None, []
+        return data["previous_diff_id"], data["issues"]
+
+    def get(self, url):
+        auth = (self.username, self.password)
+        resp = requests.get(url, auth=auth, headers=GetAppUserAgent())
+        resp.raise_for_status()
+        return resp.json()
+
     def paginate(self, url_path):
         """
         Yield results from a paginated API one by one