[PATCH v2 2/2] REST: Add patch relations

Stephen Finucane stephen at that.guru
Fri Feb 28 10:29:32 AEDT 2020


From: Mete Polat <metepolat2000 at gmail.com>

View relations and add/update/delete them as a maintainer. Maintainers
can only create relations of patches which are part of a project they
maintain. Because this is a writable many-many nested relationship, it
behaves a little unusually. In short:

- All operations use PATCH to the 'related' field of a patch

- To relate a patch to another patch, say 7 to 19, either:

    PATCH /api/patch/7  related := [19]
    PATCH /api/patch/19 related := [7]

- To delete a patch from a relation, say 1, 21 and 42 are related but we
  only want it to be 1 and 42:

    PATCH /api/patch/21 related := []

  * You _cannot_ remove a patch from a relation by patching another
    patch in the relation: I'm trying to avoid read-modify-write loops.

  * Relations that would be left with just 1 patch are deleted. This is
    only ensured in the API - the admin interface will let you do this.

- Break-before-make: if you have [1, 12, 24] and [7, 15, 42] and you want
  to end up with [1, 12, 15, 42], you have to remove 15 from the second
  relation first:

    PATCH /api/patch/1 related := [15] will fail with 409 Conflict.

  Instead do:

    PATCH /api/patch/15 related := []
    PATCH /api/patch/1  related := [15]
       -> 200 OK, [1, 12, 15, 42] and [7, 42] are the resulting relations

Signed-off-by: Mete Polat <metepolat2000 at gmail.com>
Signed-off-by: Daniel Axtens <dja at axtens.net>
Signed-off-by: Stephen Finucane <stephen at that.guru>
---
 docs/api/schemas/latest/patchwork.yaml        |  44 +++
 docs/api/schemas/patchwork.j2                 |  60 ++++
 docs/api/schemas/v1.1/patchwork.yaml          |  18 +
 docs/api/schemas/v1.2/patchwork.yaml          |  44 +++
 patchwork/api/embedded.py                     |  25 ++
 patchwork/api/event.py                        |   3 +
 patchwork/api/patch.py                        | 121 ++++++-
 patchwork/tests/api/test_relation.py          | 319 ++++++++++++++++++
 patchwork/tests/utils.py                      |  11 +
 .../add-patch-relations-c96bb6c567b416d8.yaml |  11 +
 10 files changed, 653 insertions(+), 3 deletions(-)
 create mode 100644 patchwork/tests/api/test_relation.py
 create mode 100644 releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml

diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
index 4cee1513..13cdc9cd 100644
--- a/docs/api/schemas/latest/patchwork.yaml
+++ b/docs/api/schemas/latest/patchwork.yaml
@@ -352,6 +352,7 @@ paths:
               - patch-created
               - patch-completed
               - patch-state-changed
+              - patch-relation-changed
               - patch-delegated
               - check-created
               - series-created
@@ -390,6 +391,7 @@ paths:
                     - $ref: '#/components/schemas/EventPatchCreated'
                     - $ref: '#/components/schemas/EventPatchCompleted'
                     - $ref: '#/components/schemas/EventPatchStateChanged'
+                    - $ref: '#/components/schemas/EventPatchRelationChanged'
                     - $ref: '#/components/schemas/EventPatchDelegated'
                     - $ref: '#/components/schemas/EventCheckCreated'
                     - $ref: '#/components/schemas/EventSeriesCreated'
@@ -403,6 +405,8 @@ paths:
                         '#/components/schemas/EventPatchCompleted'
                       patch-state-changed: >
                         '#/components/schemas/EventPatchStateChanged'
+                      patch-relation-changed: >
+                        '#/components/schemas/EventPatchRelationChanged'
                       patch-delegated: >
                         '#/components/schemas/EventPatchDelegated'
                       check-created: '#/components/schemas/EventCheckCreated'
@@ -552,6 +556,12 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
       tags:
         - patches
     put:
@@ -595,6 +605,12 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
       tags:
         - patches
   /api/patches/{id}/comments/:
@@ -1721,6 +1737,24 @@ components:
                 current_state:
                   title: Current state
                   type: string
+    EventPatchRelationChanged:
+      allOf:
+        - $ref: '#/components/schemas/EventBase'
+        - type: object
+          properties:
+            category:
+              enum:
+                - patch-relation-changed
+            payload:
+              properties:
+                patch:
+                  $ref: '#/components/schemas/PatchEmbedded'
+                previous_relation:
+                  title: Previous relation
+                  type: string
+                current_relation:
+                  title: Current relation
+                  type: string
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'
@@ -1893,6 +1927,11 @@ components:
           items:
             type: string
           readOnly: true
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
     PatchDetail:
       allOf:
         - $ref: '#/components/schemas/PatchList'
@@ -1943,6 +1982,11 @@ components:
           title: Delegate
           type: integer
           nullable: true
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
     Person:
       type: object
       properties:
diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
index c2f5ea6a..bd714d5e 100644
--- a/docs/api/schemas/patchwork.j2
+++ b/docs/api/schemas/patchwork.j2
@@ -359,6 +359,9 @@ paths:
               - patch-created
               - patch-completed
               - patch-state-changed
+{% if version >= (1, 2) %}
+              - patch-relation-changed
+{% endif %}
               - patch-delegated
               - check-created
               - series-created
@@ -397,6 +400,9 @@ paths:
                     - $ref: '#/components/schemas/EventPatchCreated'
                     - $ref: '#/components/schemas/EventPatchCompleted'
                     - $ref: '#/components/schemas/EventPatchStateChanged'
+{% if version >= (1, 2) %}
+                    - $ref: '#/components/schemas/EventPatchRelationChanged'
+{% endif %}
                     - $ref: '#/components/schemas/EventPatchDelegated'
                     - $ref: '#/components/schemas/EventCheckCreated'
                     - $ref: '#/components/schemas/EventSeriesCreated'
@@ -410,6 +416,10 @@ paths:
                         '#/components/schemas/EventPatchCompleted'
                       patch-state-changed: >
                         '#/components/schemas/EventPatchStateChanged'
+{% if version >= (1, 2) %}
+                      patch-relation-changed: >
+                        '#/components/schemas/EventPatchRelationChanged'
+{% endif %}
                       patch-delegated: >
                         '#/components/schemas/EventPatchDelegated'
                       check-created: '#/components/schemas/EventCheckCreated'
@@ -561,6 +571,14 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+{% if version >= (1, 2) %}
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
+{% endif %}
       tags:
         - patches
     put:
@@ -604,6 +622,14 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+{% if version >= (1, 2) %}
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
+{% endif %}
       tags:
         - patches
   /api/{{ version_url }}patches/{id}/comments/:
@@ -1777,6 +1803,26 @@ components:
                 current_state:
                   title: Current state
                   type: string
+{% if version >= (1, 1) %}
+    EventPatchRelationChanged:
+      allOf:
+        - $ref: '#/components/schemas/EventBase'
+        - type: object
+          properties:
+            category:
+              enum:
+                - patch-relation-changed
+            payload:
+              properties:
+                patch:
+                  $ref: '#/components/schemas/PatchEmbedded'
+                previous_relation:
+                  title: Previous relation
+                  type: string
+                current_relation:
+                  title: Current relation
+                  type: string
+{% endif %}
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'
@@ -1955,6 +2001,13 @@ components:
           items:
             type: string
           readOnly: true
+{% if version >= (1, 2) %}
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
+{% endif %}
     PatchDetail:
       allOf:
         - $ref: '#/components/schemas/PatchList'
@@ -2005,6 +2058,13 @@ components:
           title: Delegate
           type: integer
           nullable: true
+{% if version >= (1, 2) %}
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
+{% endif %}
     Person:
       type: object
       properties:
diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml
index babc972a..6b497aba 100644
--- a/docs/api/schemas/v1.1/patchwork.yaml
+++ b/docs/api/schemas/v1.1/patchwork.yaml
@@ -1551,6 +1551,24 @@ components:
                 current_state:
                   title: Current state
                   type: string
+    EventPatchRelationChanged:
+      allOf:
+        - $ref: '#/components/schemas/EventBase'
+        - type: object
+          properties:
+            category:
+              enum:
+                - patch-relation-changed
+            payload:
+              properties:
+                patch:
+                  $ref: '#/components/schemas/PatchEmbedded'
+                previous_relation:
+                  title: Previous relation
+                  type: string
+                current_relation:
+                  title: Current relation
+                  type: string
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'
diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml
index be79e386..db2ed122 100644
--- a/docs/api/schemas/v1.2/patchwork.yaml
+++ b/docs/api/schemas/v1.2/patchwork.yaml
@@ -352,6 +352,7 @@ paths:
               - patch-created
               - patch-completed
               - patch-state-changed
+              - patch-relation-changed
               - patch-delegated
               - check-created
               - series-created
@@ -390,6 +391,7 @@ paths:
                     - $ref: '#/components/schemas/EventPatchCreated'
                     - $ref: '#/components/schemas/EventPatchCompleted'
                     - $ref: '#/components/schemas/EventPatchStateChanged'
+                    - $ref: '#/components/schemas/EventPatchRelationChanged'
                     - $ref: '#/components/schemas/EventPatchDelegated'
                     - $ref: '#/components/schemas/EventCheckCreated'
                     - $ref: '#/components/schemas/EventSeriesCreated'
@@ -403,6 +405,8 @@ paths:
                         '#/components/schemas/EventPatchCompleted'
                       patch-state-changed: >
                         '#/components/schemas/EventPatchStateChanged'
+                      patch-relation-changed: >
+                        '#/components/schemas/EventPatchRelationChanged'
                       patch-delegated: >
                         '#/components/schemas/EventPatchDelegated'
                       check-created: '#/components/schemas/EventCheckCreated'
@@ -552,6 +556,12 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
       tags:
         - patches
     put:
@@ -595,6 +605,12 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '409':
+          description: Conflict
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
       tags:
         - patches
   /api/1.2/patches/{id}/comments/:
@@ -1721,6 +1737,24 @@ components:
                 current_state:
                   title: Current state
                   type: string
+    EventPatchRelationChanged:
+      allOf:
+        - $ref: '#/components/schemas/EventBase'
+        - type: object
+          properties:
+            category:
+              enum:
+                - patch-relation-changed
+            payload:
+              properties:
+                patch:
+                  $ref: '#/components/schemas/PatchEmbedded'
+                previous_relation:
+                  title: Previous relation
+                  type: string
+                current_relation:
+                  title: Current relation
+                  type: string
     EventPatchDelegated:
       allOf:
         - $ref: '#/components/schemas/EventBase'
@@ -1893,6 +1927,11 @@ components:
           items:
             type: string
           readOnly: true
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
     PatchDetail:
       allOf:
         - $ref: '#/components/schemas/PatchList'
@@ -1943,6 +1982,11 @@ components:
           title: Delegate
           type: integer
           nullable: true
+        related:
+          title: Relations
+          type: array
+          items:
+            type: string
     Person:
       type: object
       properties:
diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index de4f3116..85a30cae 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -14,6 +14,7 @@ from collections import OrderedDict
 from rest_framework.serializers import CharField
 from rest_framework.serializers import SerializerMethodField
 from rest_framework.serializers import PrimaryKeyRelatedField
+from rest_framework.serializers import ValidationError
 
 from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import CheckHyperlinkedIdentityField
@@ -138,6 +139,30 @@ class PatchSerializer(SerializedRelatedField):
             }
 
 
+class PatchRelationSerializer(BaseHyperlinkedModelSerializer):
+    """Hide the PatchRelation model, just show the list"""
+    patches = PatchSerializer(many=True)
+
+    def to_internal_value(self, data):
+        if not isinstance(data, type([])):
+            raise ValidationError(
+                "Patch relations must be specified as a list of patch IDs"
+            )
+        result = super(PatchRelationSerializer, self).to_internal_value(
+            {'patches': data}
+        )
+        return result
+
+    def to_representation(self, instance):
+        data = super(PatchRelationSerializer, self).to_representation(instance)
+        data = data['patches']
+        return data
+
+    class Meta:
+        model = models.PatchRelation
+        fields = ('patches',)
+
+
 class PersonSerializer(SerializedRelatedField):
 
     class _Serializer(BaseHyperlinkedModelSerializer):
diff --git a/patchwork/api/event.py b/patchwork/api/event.py
index 44c34520..d7a99c7d 100644
--- a/patchwork/api/event.py
+++ b/patchwork/api/event.py
@@ -13,6 +13,7 @@ from rest_framework.serializers import SlugRelatedField
 from patchwork.api.embedded import CheckSerializer
 from patchwork.api.embedded import CoverLetterSerializer
 from patchwork.api.embedded import PatchSerializer
+from patchwork.api.embedded import PatchRelationSerializer
 from patchwork.api.embedded import ProjectSerializer
 from patchwork.api.embedded import SeriesSerializer
 from patchwork.api.embedded import UserSerializer
@@ -33,6 +34,8 @@ class EventSerializer(ModelSerializer):
     current_delegate = UserSerializer()
     created_check = SerializerMethodField()
     created_check = CheckSerializer()
+    previous_relation = PatchRelationSerializer(read_only=True)
+    current_relation = PatchRelationSerializer(read_only=True)
 
     _category_map = {
         Event.CATEGORY_COVER_CREATED: ['cover'],
diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index 1a3ce905..9983bb62 100644
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -1,27 +1,34 @@
 # Patchwork - automated patch tracking system
 # Copyright (C) 2016 Linaro Corporation
+# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
+# Copyright (C) 2020, IBM Corporation
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 import email.parser
 
+from django.core.exceptions import ValidationError
 from django.utils.text import slugify
 from django.utils.translation import ugettext_lazy as _
+from rest_framework import status
+from rest_framework.exceptions import APIException
+from rest_framework.exceptions import PermissionDenied
 from rest_framework.generics import ListAPIView
 from rest_framework.generics import RetrieveUpdateAPIView
 from rest_framework.relations import RelatedField
 from rest_framework.reverse import reverse
 from rest_framework.serializers import SerializerMethodField
-from rest_framework.serializers import ValidationError
 
 from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import PatchworkPermission
 from patchwork.api.filters import PatchFilterSet
+from patchwork.api.embedded import PatchRelationSerializer
 from patchwork.api.embedded import PersonSerializer
 from patchwork.api.embedded import ProjectSerializer
 from patchwork.api.embedded import SeriesSerializer
 from patchwork.api.embedded import UserSerializer
 from patchwork.models import Patch
+from patchwork.models import PatchRelation
 from patchwork.models import State
 from patchwork.parser import clean_subject
 
@@ -54,6 +61,15 @@ class StateField(RelatedField):
         return State.objects.all()
 
 
+class PatchConflict(APIException):
+    status_code = status.HTTP_409_CONFLICT
+    default_detail = (
+        'At least one patch is already part of another relation. You have to '
+        'explicitly remove a patch from its existing relation before moving '
+        'it to this one.'
+    )
+
+
 class PatchListSerializer(BaseHyperlinkedModelSerializer):
 
     web_url = SerializerMethodField()
@@ -67,6 +83,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
     check = SerializerMethodField()
     checks = SerializerMethodField()
     tags = SerializerMethodField()
+    related = PatchRelationSerializer()
 
     def get_web_url(self, instance):
         request = self.context.get('request')
@@ -109,6 +126,16 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
         # will be removed in API v2
         data = super(PatchListSerializer, self).to_representation(instance)
         data['series'] = [data['series']] if data['series'] else []
+
+        # stop the related serializer returning this patch in the list of
+        # related patches. Also make it return an empty list, not null/None
+        if 'related' in data:
+            if data['related']:
+                data['related'] = [p for p in data['related']
+                                   if p['id'] != instance.id]
+            else:
+                data['related'] = []
+
         return data
 
     class Meta:
@@ -116,13 +143,13 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
         fields = ('id', 'url', 'web_url', 'project', 'msgid',
                   'list_archive_url', 'date', 'name', 'commit_ref', 'pull_url',
                   'state', 'archived', 'hash', 'submitter', 'delegate', 'mbox',
-                  'series', 'comments', 'check', 'checks', 'tags')
+                  'series', 'comments', 'check', 'checks', 'tags', 'related',)
         read_only_fields = ('web_url', 'project', 'msgid', 'list_archive_url',
                             'date', 'name', 'hash', 'submitter', 'mbox',
                             'series', 'comments', 'check', 'checks', 'tags')
         versioned_fields = {
             '1.1': ('comments', 'web_url'),
-            '1.2': ('list_archive_url',),
+            '1.2': ('list_archive_url', 'related',),
         }
         extra_kwargs = {
             'url': {'view_name': 'api-patch-detail'},
@@ -151,6 +178,94 @@ class PatchDetailSerializer(PatchListSerializer):
     def get_prefixes(self, instance):
         return clean_subject(instance.name)[1]
 
+    def update(self, instance, validated_data):
+        # d-r-f cannot handle writable nested models, so we handle that
+        # specifically ourselves and let d-r-f handle the rest
+        if 'related' not in validated_data:
+            return super(PatchDetailSerializer, self).update(
+                instance, validated_data)
+
+        related = validated_data.pop('related')
+
+        # Validation rules
+        # ----------------
+        #
+        # Permissions: to change a relation:
+        #   for all patches in the relation, current and proposed,
+        #     the user must be maintainer of the patch's project
+        #  Note that this has a ratchet effect: if you add a cross-project
+        #  relation, only you or another maintainer across both projects can
+        #  modify that relationship in _any way_.
+        #
+        # Break before Make: a patch must be explicitly removed from a
+        #   relation before being added to another
+        #
+        # No Read-Modify-Write for deletion:
+        #   to delete a patch from a relation, clear _its_ related patch,
+        #   don't modify one of the patches that are to remain.
+        #
+        # (As a consequence of those two, operations are additive:
+        #   if 1 is in a relation with [1,2,3], then
+        #   patching 1 with related=[2,4] gives related=[1,2,3,4])
+
+        # Permissions:
+        # Because we're in a serializer, not a view, this is a bit clunky
+        user = self.context['request'].user.profile
+        # Must be maintainer of:
+        #  - current patch
+        self.check_user_maintains_all(user, [instance])
+        #  - all patches currently in relation
+        #  - all patches proposed to be in relation
+        patches = set(related['patches']) if related else {}
+        if instance.related is not None:
+            patches = patches.union(instance.related.patches.all())
+        self.check_user_maintains_all(user, patches)
+
+        # handle deletion
+        if not related['patches']:
+            # do not allow relations with a single patch
+            if instance.related and instance.related.patches.count() == 2:
+                instance.related.delete()
+            instance.related = None
+            return super(PatchDetailSerializer, self).update(
+                instance, validated_data)
+
+        # break before make
+        relations = {patch.related for patch in patches if patch.related}
+        if len(relations) > 1:
+            raise PatchConflict()
+        if relations:
+            relation = relations.pop()
+        else:
+            relation = None
+        if relation and instance.related is not None:
+            if instance.related != relation:
+                raise PatchConflict()
+
+        # apply
+        if relation is None:
+            relation = PatchRelation()
+            relation.save()
+        for patch in patches:
+            patch.related = relation
+            patch.save()
+        instance.related = relation
+        instance.save()
+
+        return super(PatchDetailSerializer, self).update(
+            instance, validated_data)
+
+    @staticmethod
+    def check_user_maintains_all(user, patches):
+        maintains = user.maintainer_projects.all()
+        if any(s.project not in maintains for s in patches):
+            detail = (
+                'At least one patch is part of a project you are not '
+                'maintaining.'
+            )
+            raise PermissionDenied(detail=detail)
+        return True
+
     class Meta:
         model = Patch
         fields = PatchListSerializer.Meta.fields + (
diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py
new file mode 100644
index 00000000..d48c62bc
--- /dev/null
+++ b/patchwork/tests/api/test_relation.py
@@ -0,0 +1,319 @@
+# Patchwork - automated patch tracking system
+# Copyright (C) 2020, IBM Corporation
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import unittest
+
+from django.conf import settings
+from django.urls import reverse
+
+from patchwork.models import Patch
+from patchwork.models import PatchRelation
+from patchwork.tests.api import utils
+from patchwork.tests.utils import create_maintainer
+from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patches
+from patchwork.tests.utils import create_project
+from patchwork.tests.utils import create_relation
+from patchwork.tests.utils import create_user
+
+if settings.ENABLE_REST_API:
+    from rest_framework import status
+
+
+ at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
+class TestRelationSimpleAPI(utils.APITestCase):
+    @staticmethod
+    def api_url(item=None, version=None):
+        kwargs = {}
+        if version:
+            kwargs['version'] = version
+
+        if item is None:
+            return reverse('api-patch-list', kwargs=kwargs)
+        kwargs['pk'] = item
+        return reverse('api-patch-detail', kwargs=kwargs)
+
+    def setUp(self):
+        self.project = create_project()
+        self.normal_user = create_user()
+        self.maintainer = create_maintainer(self.project)
+
+    def test_no_relation(self):
+        patch = create_patch(project=self.project)
+        resp = self.client.get(self.api_url(item=patch.pk))
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(resp.data['related'], [])
+
+    @utils.store_samples('relation-list')
+    def test_list_two_patch_relation(self):
+        relation = create_relation(2, project=self.project)
+        patches = relation.patches.all()
+
+        # nobody
+        resp = self.client.get(self.api_url(item=patches[0].pk))
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        self.assertIn('related', resp.data)
+        self.assertEqual(len(resp.data['related']), 1)
+        self.assertEqual(resp.data['related'][0]['id'], patches[1].pk)
+
+        resp = self.client.get(self.api_url(item=patches[1].pk))
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        self.assertIn('related', resp.data)
+        self.assertEqual(len(resp.data['related']), 1)
+        self.assertEqual(resp.data['related'][0]['id'], patches[0].pk)
+
+    @utils.store_samples('relation-create-forbidden')
+    def test_create_two_patch_relation_nobody(self):
+        patches = create_patches(2, project=self.project)
+
+        resp = self.client.patch(
+            self.api_url(item=patches[0].pk), {'related': [patches[1].pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN)
+
+    def test_create_two_patch_relation_user(self):
+        patches = create_patches(2, project=self.project)
+
+        self.client.force_authenticate(user=self.normal_user)
+        resp = self.client.patch(
+            self.api_url(item=patches[0].pk), {'related': [patches[1].pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN)
+
+    @utils.store_samples('relation-create-maintainer')
+    def test_create_two_patch_relation_maintainer(self):
+        patches = create_patches(2, project=self.project)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=patches[0].pk), {'related': [patches[1].pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        # reload and verify
+        patches = Patch.objects.all()
+        self.assertIsNotNone(patches[0].related)
+        self.assertIsNotNone(patches[1].related)
+        self.assertEqual(patches[1].related, patches[0].related)
+
+    def test_delete_two_patch_relation_nobody(self):
+        relation = create_relation(project=self.project)
+        patch = relation.patches.all()[0]
+
+        self.assertEqual(PatchRelation.objects.count(), 1)
+
+        resp = self.client.patch(self.api_url(item=patch.pk), {'related': []})
+        self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN)
+        self.assertEqual(PatchRelation.objects.count(), 1)
+
+    @utils.store_samples('relation-delete')
+    def test_delete_two_patch_relation_maintainer(self):
+        relation = create_relation(project=self.project)
+        patch = relation.patches.all()[0]
+
+        self.assertEqual(PatchRelation.objects.count(), 1)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(self.api_url(item=patch.pk), {'related': []})
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        self.assertEqual(PatchRelation.objects.count(), 0)
+        self.assertEqual(
+            Patch.objects.filter(related__isnull=False).exists(), False
+        )
+
+    def test_create_three_patch_relation(self):
+        patches = create_patches(3, project=self.project)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=patches[0].pk),
+            {'related': [patches[1].pk, patches[2].pk]},
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        # reload and verify
+        patches = Patch.objects.all()
+        self.assertIsNotNone(patches[0].related)
+        self.assertIsNotNone(patches[1].related)
+        self.assertIsNotNone(patches[2].related)
+        self.assertEqual(patches[0].related, patches[1].related)
+        self.assertEqual(patches[1].related, patches[2].related)
+
+    def test_delete_from_three_patch_relation(self):
+        relation = create_relation(3, project=self.project)
+        patch = relation.patches.all()[0]
+
+        self.assertEqual(PatchRelation.objects.count(), 1)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(self.api_url(item=patch.pk), {'related': []})
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertIsNone(Patch.objects.get(id=patch.pk).related)
+        self.assertEqual(PatchRelation.objects.count(), 1)
+        self.assertEqual(PatchRelation.objects.first().patches.count(), 2)
+
+    @utils.store_samples('relation-extend-through-new')
+    def test_extend_relation_through_new(self):
+        relation = create_relation(project=self.project)
+        existing_patch_a = relation.patches.first()
+
+        new_patch = create_patch(project=self.project)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=new_patch.pk), {'related': [existing_patch_a.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(relation, Patch.objects.get(pk=new_patch.pk).related)
+        self.assertEqual(relation.patches.count(), 3)
+
+    def test_extend_relation_through_old(self):
+        relation = create_relation(project=self.project)
+        existing_patch_a = relation.patches.first()
+
+        new_patch = create_patch(project=self.project)
+
+        # maintainer
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=existing_patch_a.pk), {'related': [new_patch.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(relation, Patch.objects.get(id=new_patch.id).related)
+        self.assertEqual(relation.patches.count(), 3)
+
+    def test_extend_relation_through_new_two(self):
+        relation = create_relation(project=self.project)
+        existing_patch_a = relation.patches.first()
+
+        new_patch_a = create_patch(project=self.project)
+        new_patch_b = create_patch(project=self.project)
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=new_patch_a.pk),
+            {'related': [existing_patch_a.pk, new_patch_b.pk]},
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(
+            relation, Patch.objects.get(id=new_patch_a.id).related
+        )
+        self.assertEqual(
+            relation, Patch.objects.get(id=new_patch_b.id).related
+        )
+        self.assertEqual(relation.patches.count(), 4)
+
+    @utils.store_samples('relation-extend-through-old')
+    def test_extend_relation_through_old_two(self):
+        relation = create_relation(project=self.project)
+        existing_patch_a = relation.patches.first()
+
+        new_patch_a = create_patch(project=self.project)
+        new_patch_b = create_patch(project=self.project)
+
+        # maintainer
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=existing_patch_a.pk),
+            {'related': [new_patch_a.pk, new_patch_b.pk]},
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(
+            relation, Patch.objects.get(id=new_patch_a.id).related
+        )
+        self.assertEqual(
+            relation, Patch.objects.get(id=new_patch_b.id).related
+        )
+        self.assertEqual(relation.patches.count(), 4)
+
+    def test_remove_one_patch_from_relation_bad(self):
+        relation = create_relation(3, project=self.project)
+        keep_patch_a = relation.patches.all()[1]
+        keep_patch_b = relation.patches.all()[2]
+
+        # this should do nothing - it is interpreted as
+        # _adding_ keep_patch_b again which is a no-op.
+
+        # maintainer
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=keep_patch_a.pk), {'related': [keep_patch_b.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(relation.patches.count(), 3)
+
+    def test_remove_one_patch_from_relation_good(self):
+        relation = create_relation(3, project=self.project)
+        target_patch = relation.patches.all()[0]
+
+        # maintainer
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=target_patch.pk), {'related': []}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertIsNone(Patch.objects.get(id=target_patch.id).related)
+        self.assertEqual(relation.patches.count(), 2)
+
+    @utils.store_samples('relation-forbid-moving-between-relations')
+    def test_forbid_moving_patch_between_relations(self):
+        """Test the break-before-make logic"""
+        relation_a = create_relation(project=self.project)
+        relation_b = create_relation(project=self.project)
+
+        patch_a = relation_a.patches.first()
+        patch_b = relation_b.patches.first()
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=patch_a.pk), {'related': [patch_b.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_409_CONFLICT)
+
+        resp = self.client.patch(
+            self.api_url(item=patch_b.pk), {'related': [patch_a.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_409_CONFLICT)
+
+    def test_cross_project_different_maintainers(self):
+        patch_a = create_patch(project=self.project)
+
+        project_b = create_project()
+        patch_b = create_patch(project=project_b)
+
+        # maintainer a, patch in own project
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=patch_a.pk), {'related': [patch_b.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN)
+
+        # maintainer a, patch in project b
+        resp = self.client.patch(
+            self.api_url(item=patch_b.pk), {'related': [patch_a.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN)
+
+    def test_cross_project_relation_super_maintainer(self):
+        patch_a = create_patch(project=self.project)
+
+        project_b = create_project()
+        patch_b = create_patch(project=project_b)
+
+        project_b.maintainer_project.add(self.maintainer.profile)
+        project_b.save()
+
+        self.client.force_authenticate(user=self.maintainer)
+        resp = self.client.patch(
+            self.api_url(item=patch_a.pk), {'related': [patch_b.pk]}
+        )
+        self.assertEqual(resp.status_code, status.HTTP_200_OK)
+        self.assertEqual(
+            Patch.objects.get(id=patch_a.id).related,
+            Patch.objects.get(id=patch_b.id).related,
+        )
diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
index 5e6a1b13..7759c8f3 100644
--- a/patchwork/tests/utils.py
+++ b/patchwork/tests/utils.py
@@ -16,6 +16,7 @@ from patchwork.models import Check
 from patchwork.models import Comment
 from patchwork.models import CoverLetter
 from patchwork.models import Patch
+from patchwork.models import PatchRelation
 from patchwork.models import Person
 from patchwork.models import Project
 from patchwork.models import Series
@@ -352,3 +353,13 @@ def create_covers(count=1, **kwargs):
         kwargs (dict): Overrides for various cover letter fields
     """
     return _create_submissions(create_cover, count, **kwargs)
+
+
+def create_relation(count_patches=2, **kwargs):
+    relation = PatchRelation.objects.create()
+    values = {
+        'related': relation
+    }
+    values.update(kwargs)
+    create_patches(count_patches, **values)
+    return relation
diff --git a/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml b/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml
new file mode 100644
index 00000000..44e704c8
--- /dev/null
+++ b/releasenotes/notes/add-patch-relations-c96bb6c567b416d8.yaml
@@ -0,0 +1,11 @@
+---
+features:
+  - |
+    Patches can now be related to other patches (e.g. to cross-reference
+    revisions). Relations can be set via the REST API by maintainers
+    (currently only for patches of projects they maintain). Patches can
+    belong to at most one relation at a time.
+api:
+  - |
+    Relations are available via ``/patches/{patchID}/`` endpoint, in
+    the ``related`` field.
-- 
2.24.1



More information about the Patchwork mailing list