Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New smart API to list issues #2534

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions backend/code_review_backend/app/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)",
Expand Down
Empty file.
127 changes: 127 additions & 0 deletions backend/code_review_backend/issues/v2/api.py
Original file line number Diff line number Diff line change
@@ -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",
),
]
19 changes: 19 additions & 0 deletions backend/code_review_backend/issues/v2/serializers.py
Original file line number Diff line number Diff line change
@@ -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)
Empty file.
175 changes: 175 additions & 0 deletions backend/code_review_backend/issues/v2/tests/test_list_issues.py
Original file line number Diff line number Diff line change
@@ -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},
],
},
)