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

[Raxel Gutierrez]

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 at 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 at 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
>