[RFC,3/4] api: modularize patch relations handling

Move function that handles updates for patch relations for REST API
calls outside of DRF serialized so that it can be reused by the patch
relations table through form submission. The function is used in an
upcoming patch that deals with the manual modification of the patch
relations of a given patch.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 patchwork/api/patch.py | 161 +++++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 77 deletions(-)

Made a mistake with making update_patch_relations modular. Tests are
failing. The following changes should be made to this patch to fix:

-    patches = set(related_patches) if related_patches else set()
+    patches = set(related_patches['patches']) if related_patches else set()
     if patch.related is not None:
         patches = patches.union(patch.related.patches.all())
     check_user_maintains_all(user, patches)

     # handle deletion
-    if not related_patches:
+    if not related_patches['patches']:

On Mon, Aug 23, 2021 at 10:58 AM Raxel Gutierrez <raxel@google.com> wrote:
>
> Move function that handles updates for patch relations for REST API
> calls outside of DRF serialized so that it can be reused by the patch
> relations table through form submission. The function is used in an
> upcoming patch that deals with the manual modification of the patch
> relations of a given patch.
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  patchwork/api/patch.py | 161 +++++++++++++++++++++--------------------
>  1 file changed, 84 insertions(+), 77 deletions(-)
>
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index a97a8820..ff7640b8 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -70,6 +70,87 @@ class PatchConflict(APIException):
>      )
>
>
> +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
> +
> +
> +def update_patch_relations(user, patch, data):
> +    # 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:
> +    # Must be maintainer of:
> +    #  - current patch
> +    check_user_maintains_all(user, [patch])
> +    #  - all patches currently in relation
> +    #  - all patches proposed to be in relation
> +    errors = []
> +    related_patches = data.pop('related')
> +    patches = set(related_patches) if related_patches else set()
> +    if patch.related is not None:
> +        patches = patches.union(patch.related.patches.all())
> +    check_user_maintains_all(user, patches)
> +
> +    # handle deletion
> +    if not related_patches:
> +        # do not allow relations with a single patch
> +        if patch.related and patch.related.patches.count() == 2:
> +            patch.related.delete()
> +        patch.related = None
> +        patch.save()
> +        return errors
> +
> +    # break before make
> +    relations = {patch.related for patch in patches if patch.related}
> +    if len(relations) > 1:
> +        return [PatchConflict()]
> +    if relations:
> +        relation = relations.pop()
> +    else:
> +        relation = None
> +    if relation and patch.related is not None:
> +        if patch.related != relation:
> +            return [PatchConflict()]
> +
> +    # apply
> +    if relation is None:
> +        relation = PatchRelation()
> +        relation.save()
> +
> +    for rp in patches:
> +        rp.related = relation
> +        rp.save()
> +    patch.related = relation
> +    patch.save()
> +
> +    return errors
> +
> +
>  class PatchListSerializer(BaseHyperlinkedModelSerializer):
>
>      web_url = SerializerMethodField()
> @@ -184,87 +265,13 @@ class PatchDetailSerializer(PatchListSerializer):
>              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()
> -
> +        errors = update_patch_relations(user, instance, validated_data)
> +        if errors:
> +            raise errors.pop()
>          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 + (
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>