Only use the bulk endpoint for issues creation (#2445)

* Remove non bulk issue publication from the bot

* Simplify bulk size configuration

* Update tests

* Cleanup revision.diff_issues_url

* Disallow creation of a single issue in the backend

Author: vrigal

backend/code_review_backend/issues/api.py

@@ -6,7 +6,6 @@
 from datetime import date, datetime, timedelta
 
 from django.conf import settings
-from django.db import transaction
 from django.db.models import Count, Prefetch, Q
 from django.db.models.functions import TruncDate
 from django.shortcuts import get_object_or_404
@@ -17,12 +16,10 @@
 from rest_framework.exceptions import APIException, ValidationError
 from rest_framework.response import Response
 
-from code_review_backend.issues.compare import detect_new_for_revision
 from code_review_backend.issues.models import (
     LEVEL_ERROR,
     Diff,
     Issue,
-    IssueLink,
     Repository,
     Revision,
 )
@@ -174,7 +171,18 @@ def get_queryset(self):
         return diffs
 
 
-class IssueViewSet(viewsets.ModelViewSet):
+class IssueViewSet(
+    mixins.RetrieveModelMixin,
+    mixins.UpdateModelMixin,
+    mixins.DestroyModelMixin,
+    mixins.ListModelMixin,
+    viewsets.GenericViewSet,
+):
+    """
+    Viewset for Issue management.
+    Creation can only be performed in bulk via the revision.
+    """
+
     serializer_class = IssueSerializer
 
     def get_queryset(self):
@@ -186,22 +194,6 @@ def get_queryset(self):
         # but we use the distinct clause to match the DB state.
         return Issue.objects.filter(diffs=diff).distinct()
 
-    @transaction.atomic
-    def perform_create(self, serializer):
-        # Attach diff to issue created
-        # and detect if the issue is new for the revision
-        diff = get_object_or_404(Diff, id=self.kwargs["diff_id"])
-        issue = serializer.save(
-            new_for_revision=detect_new_for_revision(
-                diff,
-                path=serializer.validated_data["path"],
-                hash=serializer.validated_data["hash"],
-            ),
-        )
-        IssueLink.objects.create(
-            issue=issue, diff_id=diff.id, revision_id=diff.revision_id
-        )
-
 
 class IssueBulkCreate(generics.CreateAPIView):
     """

backend/code_review_backend/issues/tests/test_api.py

@@ -135,7 +135,7 @@ def test_create_diff(self):
         self.assertEqual(diff.mercurial_hash, "coffee12345")
         self.assertEqual(diff.revision, self.revision)
 
-    def test_create_issue(self):
+    def test_create_issue_disabled(self):
         """
         Check we can create a issue through the API
         """
@@ -156,44 +156,12 @@ def test_create_issue(self):
         self.assertEqual(Issue.objects.count(), 0)
         self.client.force_authenticate(user=self.user)
         response = self.client.post("/v1/diff/1234/issues/", data, format="json")
-        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+        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
-        issue_data = response.json()
-        self.assertTrue("id" in issue_data)
-        del issue_data["id"]
-        self.assertDictEqual(
-            issue_data,
-            {
-                "analyzer": "remote-flake8",
-                "char": None,
-                "check": None,
-                "hash": "somemd5hash",
-                "level": "error",
-                "line": 1,
-                "message": None,
-                "nb_lines": None,
-                "new_for_revision": True,
-                "path": "path/to/file.py",
-                "in_patch": True,
-                "publishable": True,
-            },
-        )
-
-        # Check a revision has been created
-        self.assertEqual(Diff.objects.count(), 1)
-        issue = Issue.objects.first()
-        self.assertEqual(issue.path, "path/to/file.py")
-        self.assertEqual(issue.line, 1)
-        self.assertListEqual(
-            list(issue.diffs.values_list("id", flat=True)), [self.diff.id]
+        self.assertEqual(
+            response.content, b'{"detail":"Method \\"POST\\" not allowed."}'
         )
-        self.assertTrue(issue.new_for_revision)
-
-        # The diff now counts an issue
-        response = self.client.get("/v1/diff/1234/")
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertEqual(response.json()["nb_issues"], 1)
 
     def test_create_issue_bulk_methods(self):
         self.client.force_authenticate(user=self.user)

backend/code_review_backend/issues/tests/test_compare.py

@@ -6,7 +6,6 @@
 import random
 
 from django.contrib.auth.models import User
-from rest_framework import status
 from rest_framework.test import APITestCase
 
 from code_review_backend.issues.compare import detect_new_for_revision
@@ -87,30 +86,3 @@ def test_detect_new_for_revision(self):
         # But adding an issue with a different hash on second diff will be set as new
         issue = self.build_issue(2, 12345)
         self.assertTrue(detect_new_for_revision(second_diff, issue.path, issue.hash))
-
-    def test_api_creation(self):
-        """
-        Check the detection is automatically applied through the API
-        """
-        # No issues on second diff at first
-        self.assertFalse(Issue.objects.filter(diffs=2).exists())
-
-        # Adding an issue with same hash on second diff will be set as existing
-        data = {
-            "line": 10,
-            "analyzer": "analyzer-x",
-            "check": "check-y",
-            "level": "error",
-            "path": "path/to/file",
-            "hash": self.build_hash(1),
-        }
-        self.client.force_authenticate(user=self.user)
-        response = self.client.post("/v1/diff/2/issues/", data, format="json")
-        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
-        self.assertFalse(response.json()["new_for_revision"])
-
-        # But adding an issue with a different hash on second diff will be set as new
-        data["hash"] = self.build_hash(456)
-        response = self.client.post("/v1/diff/2/issues/", data, format="json")
-        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
-        self.assertTrue(response.json()["new_for_revision"])

bot/code_review_bot/backend.py

@@ -40,8 +40,7 @@ def enabled(self):
     def publish_revision(self, revision):
         """
         Create a Revision on the backend.
-        In case revision.diff_id exists, also create revision's diff and set
-        revision.diff_issues_url to the URL for issues creation.
+        In case revision.diff_id exists, also create revision's diff.
         """
         if not self.enabled:
             logger.warn("Skipping revision publication on backend")
@@ -106,79 +105,31 @@ def publish_revision(self, revision):
         if backend_diff is None:
             return backend_revision
 
-        # Store the issues url on the revision
-        revision.diff_issues_url = backend_diff["issues_url"]
-
         return backend_revision
 
-    def publish_issues(self, issues, revision, bulk=0):
+    def publish_issues(self, issues, revision):
         """
-        Publish all issues on the backend.
-        If set to an integer, `bulk` parameter allow to use the bulk creation endpoint of the backend.
+        Publish all issues on the backend in bulk.
         """
         if not self.enabled:
             logger.warn("Skipping issues publication on backend")
             return
 
         published = 0
-        if bulk > 0:
-            assert revision.issues_url is not None, (
-                "Missing issues_url on the revision. "
-                "This attribute is required when publishing issues in bulk."
-            )
-            chunks = (issues[i : i + bulk] for i in range(0, len(issues), bulk))
-            for issues_chunk in chunks:
-                # Store valid data as couples of (<issue>, <json_data>)
-                valid_data = []
-                # Build issues' payload for that given chunk
-                for issue in issues_chunk:
-                    if (
-                        isinstance(issue, MozLintIssue)
-                        and issue.linter == "rust"
-                        and issue.path == "."
-                    ):
-                        # Silently ignore issues with path "." from rustfmt, as they cannot be published
-                        # https://github.com/mozilla/code-review/issues/1577
-                        continue
-                    if issue.hash is None:
-                        logger.warning(
-                            "Missing issue hash, cannot publish on backend",
-                            issue=str(issue),
-                        )
-                        continue
-                    valid_data.append((issue, issue.as_dict()))
-
-                if not valid_data:
-                    # May happen when a series of issues are missing a hash
-                    logger.warning(
-                        "No issue is valid over an entire chunk",
-                        head_repository=revision.head_repository,
-                        head_changeset=revision.head_changeset,
-                    )
-                    continue
-
-                response = self.create(
-                    revision.issues_url,
-                    {"issues": [json_data for _, json_data in valid_data]},
-                )
-                if response is None:
-                    # Backend rejected the payload, nothing more to do.
-                    continue
-                created = response.get("issues")
-
-                assert created and len(created) == len(valid_data)
-                for (issue, _), return_value in zip(valid_data, created):
-                    # Set the returned value on each issue
-                    issue.on_backend = return_value
-
-                published += len(valid_data)
-        else:
-            assert revision.diff_issues_url is not None, (
-                "Missing diff_issues_url on the revision. "
-                "This attribute is required when publishing a single issue on a revision's diff."
-            )
-
-            for issue in issues:
+        assert (
+            revision.issues_url is not None
+        ), "Missing issues_url on the revision to publish issues in bulk."
+
+        logger.info(f"Publishing issues in bulk of {settings.bulk_issue_chunks} items.")
+        chunks = (
+            issues[i : i + settings.bulk_issue_chunks]
+            for i in range(0, len(issues), settings.bulk_issue_chunks)
+        )
+        for issues_chunk in chunks:
+            # Store valid data as couples of (<issue>, <json_data>)
+            valid_data = []
+            # Build issues' payload for that given chunk
+            for issue in issues_chunk:
                 if (
                     isinstance(issue, MozLintIssue)
                     and issue.linter == "rust"
@@ -187,21 +138,38 @@ def publish_issues(self, issues, revision, bulk=0):
                     # Silently ignore issues with path "." from rustfmt, as they cannot be published
                     # https://github.com/mozilla/code-review/issues/1577
                     continue
-                try:
-                    assert issue.hash is not None
-                except Exception:
-                    # An exception may occur computing issues hash (e.g. network error)
+                if issue.hash is None:
                     logger.warning(
                         "Missing issue hash, cannot publish on backend",
                         issue=str(issue),
                     )
                     continue
-                payload = issue.as_dict()
-                issue.on_backend = self.create(revision.diff_issues_url, payload)
-                if issue.on_backend is not None:
-                    published += 1
-                else:
-                    logger.warn("Failed backend publication", issue=str(issue))
+                valid_data.append((issue, issue.as_dict()))
+
+            if not valid_data:
+                # May happen when a series of issues are missing a hash
+                logger.warning(
+                    "No issue is valid over an entire chunk",
+                    head_repository=revision.head_repository,
+                    head_changeset=revision.head_changeset,
+                )
+                continue
+
+            response = self.create(
+                revision.issues_url,
+                {"issues": [json_data for _, json_data in valid_data]},
+            )
+            if response is None:
+                # Backend rejected the payload, nothing more to do.
+                continue
+            created = response.get("issues")
+
+            assert created and len(created) == len(valid_data)
+            for (issue, _), return_value in zip(valid_data, created):
+                # Set the returned value on each issue
+                issue.on_backend = return_value
+
+            published += len(valid_data)
 
         total = len(issues)
         if published < total:

bot/code_review_bot/config.py

@@ -50,6 +50,9 @@ def __init__(self):
         self.repositories = []
         self.decision_env_prefixes = []
 
+        # Max number of issues published to the backend at a time during the ingestion of a revision
+        self.bulk_issue_chunks = 100
+
         # Cache to store file-by-file from HGMO Rest API
         self.hgmo_cache = tempfile.mkdtemp(suffix="hgmo")
 
@@ -87,6 +90,9 @@ def setup(self, app_channel, allowed_paths, repositories, mercurial_cache=None):
         if not os.path.isdir(self.taskcluster.results_dir):
             os.makedirs(self.taskcluster.results_dir)
 
+        if "BULK_ISSUE_CHUNKS" in os.environ:
+            self.bulk_issue_chunks = int(os.environ["BULK_ISSUE_CHUNKS"])
+
         # Save allowed paths
         assert isinstance(allowed_paths, list)
         assert all(map(lambda p: isinstance(p, str), allowed_paths))

bot/code_review_bot/revisions.py

@@ -116,8 +116,6 @@ def __init__(
         # the phabricator repository payload for later identification
         self.phabricator_repository = phabricator_repository
 
-        # backend's returned URL to list or create issues on the revision's diff
-        self.diff_issues_url = None
         # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional)
         self.issues_url = None
 

bot/code_review_bot/workflow.py

@@ -30,9 +30,6 @@
 TASKCLUSTER_NAMESPACE = "project.relman.{channel}.code-review.{name}"
 TASKCLUSTER_INDEX_TTL = 7  # in days
 
-# Max number of issues published to the backend at a time during the ingestion of a revision
-BULK_ISSUE_CHUNKS = 100
-
 
 class Workflow:
     """
@@ -212,11 +209,7 @@ def _build_tasks(tasks):
         self.clone_repository(revision)
 
         # Publish issues in the backend
-        self.backend_api.publish_issues(
-            issues,
-            revision,
-            bulk=BULK_ISSUE_CHUNKS,
-        )
+        self.backend_api.publish_issues(issues, revision)
 
     def clone_repository(self, revision):
         """

bot/tests/conftest.py

@@ -57,6 +57,7 @@ def mock_config(mock_repositories):
         del os.environ["TASK_ID"]
     os.environ["TRY_TASK_ID"] = "remoteTryTask"
     os.environ["TRY_TASK_GROUP_ID"] = "remoteTryGroup"
+    os.environ["BULK_ISSUE_CHUNKS"] = "10"
     settings.setup("test", ["dom/*", "tests/*.py", "test/*.c"], mock_repositories)
     return settings