Rework update noop

Author: mdellweg

CHANGES/6896.feature

@@ -1 +1 @@
-Modified the update Domain API so that it does not dispatch a task if no change is needed.
+Modified the API so that updates do not dispatch a task if no change is needed.

CHANGES/6897.feature

@@ -1 +1 @@
-Modified the update Remote API so that it does not dispatch a task if no change is needed.
+Modified the API so that updates do not dispatch a task if no change is needed.

pulpcore/app/util.py

@@ -678,16 +678,3 @@ def normalize_http_status(status):
         return "5xx"
     else:
         return ""
-
-
-def resource_modified(resource, request, **kwargs):
-    """Check if the request data has values different from the current resource instance."""
-    partial = kwargs.pop("partial", False)
-    entity = resource.get_object()
-    serializer = resource.get_serializer(entity, data=request.data, partial=partial)
-    serializer.is_valid(raise_exception=True)
-
-    for request_field, request_field_value in serializer.validated_data.items():
-        if getattr(entity, request_field) != request_field_value:
-            return True
-    return False

pulpcore/app/viewsets/base.py

@@ -13,7 +13,7 @@
 from rest_framework.decorators import action
 from rest_framework.generics import get_object_or_404
 from rest_framework.response import Response
-from pulpcore.openapi import PulpAutoSchema
+from pulpcore.openapi import PulpAutoSchema, InheritSerializer
 from rest_framework.serializers import ValidationError as DRFValidationError, ListField, CharField
 
 from pulpcore.app import tasks
@@ -482,27 +482,31 @@ class AsyncUpdateMixin(AsyncReservedObjectMixin):
     ALLOW_NON_BLOCKING_UPDATE = True
 
     @extend_schema(
-        description="Trigger an asynchronous update task",
-        responses={202: AsyncOperationResponseSerializer},
+        description="Update the entity and trigger an asynchronous task if necessary",
+        responses={200: InheritSerializer, 202: AsyncOperationResponseSerializer},
     )
     def update(self, request, pk, **kwargs):
         partial = kwargs.pop("partial", False)
         instance = self.get_object()
         serializer = self.get_serializer(instance, data=request.data, partial=partial)
         serializer.is_valid(raise_exception=True)
-        app_label = instance._meta.app_label
-        task = dispatch(
-            tasks.base.ageneral_update,
-            exclusive_resources=self.async_reserved_resources(instance),
-            args=(pk, app_label, serializer.__class__.__name__),
-            kwargs={"data": request.data, "partial": partial},
-            immediate=self.ALLOW_NON_BLOCKING_UPDATE,
-        )
-        return OperationPostponedResponse(task, request)
+
+        if all(getattr(instance, key) == value for key, value in serializer.validated_data.items()):
+            return Response(serializer.data)
+        else:
+            app_label = instance._meta.app_label
+            task = dispatch(
+                tasks.base.ageneral_update,
+                exclusive_resources=self.async_reserved_resources(instance),
+                args=(pk, app_label, serializer.__class__.__name__),
+                kwargs={"data": request.data, "partial": partial},
+                immediate=self.ALLOW_NON_BLOCKING_UPDATE,
+            )
+            return OperationPostponedResponse(task, request)
 
     @extend_schema(
-        description="Trigger an asynchronous partial update task",
-        responses={202: AsyncOperationResponseSerializer},
+        description="Update the entity partially and trigger an asynchronous task if necessary",
+        responses={200: InheritSerializer, 202: AsyncOperationResponseSerializer},
     )
     def partial_update(self, request, *args, **kwargs):
         kwargs["partial"] = True

pulpcore/app/viewsets/domain.py

@@ -1,7 +1,7 @@
 from gettext import gettext as _
 
 from drf_spectacular.utils import extend_schema
-from rest_framework import mixins, response
+from rest_framework import mixins
 from rest_framework.decorators import action
 from rest_framework.exceptions import ValidationError
 
@@ -14,7 +14,6 @@
     AsyncOperationResponseSerializer,
 )
 from pulpcore.app.tasks import migrate_backend
-from pulpcore.app.util import resource_modified
 from pulpcore.app.viewsets import NamedModelViewSet, AsyncRemoveMixin, AsyncUpdateMixin, LabelsMixin
 from pulpcore.app.viewsets.base import NAME_FILTER_OPTIONS
 from pulpcore.app.viewsets.custom_filters import LabelFilter
@@ -110,19 +109,14 @@ class DomainViewSet(
 
     @extend_schema(
         description="Trigger an asynchronous update task",
-        responses={202: AsyncOperationResponseSerializer, 204: None},
+        responses={200: DomainSerializer, 202: AsyncOperationResponseSerializer},
     )
     def update(self, request, pk, **kwargs):
         """Prevent trying to update the default domain."""
         instance = self.get_object()
         if instance.name == "default":
             raise ValidationError(_("Default domain can not be updated."))
 
-        # check if the update operation is going to change anything and
-        # if no actual change is needed return a 204 No Content.
-        if not resource_modified(self, request, **kwargs):
-            return response.Response(None, status=204)
-
         return super().update(request, pk, **kwargs)
 
     @extend_schema(

pulpcore/app/viewsets/repository.py

@@ -5,7 +5,7 @@
 from django.urls.base import Resolver404, resolve
 from django_filters import Filter
 from drf_spectacular.utils import extend_schema
-from rest_framework import mixins, response, serializers
+from rest_framework import mixins, serializers
 from rest_framework.decorators import action
 from rest_framework.viewsets import GenericViewSet
 from urllib.parse import urlparse
@@ -43,7 +43,7 @@
 from pulpcore.app.viewsets.custom_filters import LabelFilter, WithContentFilter, WithContentInFilter
 from pulpcore.tasking.tasks import dispatch
 from pulpcore.filters import HyperlinkRelatedFilter
-from pulpcore.app.util import resolve_prn, resource_modified
+from pulpcore.app.util import resolve_prn
 
 
 class RepositoryContentFilter(Filter):
@@ -368,21 +368,6 @@ class RemoteViewSet(
     queryset = Remote.objects.all()
     filterset_class = RemoteFilter
 
-    @extend_schema(
-        description="Trigger an asynchronous update task",
-        responses={202: AsyncOperationResponseSerializer, 204: None},
-    )
-    def update(self, request, pk, **kwargs):
-        """
-        Check if the update operation is going to change anything and if:
-        - no actual change is needed return a 204 No Content
-        - the request contains modifications, trigger an asynchronous update task
-        """
-        if not resource_modified(self, request, **kwargs):
-            return response.Response(None, status=204)
-
-        return super().update(request, pk, **kwargs)
-
 
 # We have to use GenericViewSet as NamedModelViewSet causes
 # get_viewset_for_model() to match multiple viewsets.

pulpcore/openapi/__init__.py

@@ -23,8 +23,8 @@
 )
 from drf_spectacular.settings import spectacular_settings
 from drf_spectacular.types import OpenApiTypes
-from drf_spectacular.utils import OpenApiParameter, extend_schema_field
-from drf_spectacular.extensions import OpenApiAuthenticationExtension
+from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_field
+from drf_spectacular.extensions import OpenApiViewExtension, OpenApiAuthenticationExtension
 from rest_framework import mixins, serializers
 from rest_framework.exceptions import ParseError
 from rest_framework.request import Request
@@ -48,6 +48,10 @@
 extend_schema_field(OpenApiTypes.INT64)(serializers.IntegerField)
 
 
+class InheritSerializer(serializers.Serializer):
+    """This is a dummy tracer to allow mixins to refer to the natural serializer of a viewset."""
+
+
 class PulpAutoSchema(AutoSchema):
     """Pulp Auto Schema."""
 
@@ -238,6 +242,7 @@ def _get_response_bodies(self):
         """
         Handle response status code.
         """
+        # DRF handles this most of the time. But it seems set_label still needs it.
         response = super()._get_response_bodies()
         if (
             self.method == "POST"
@@ -248,6 +253,15 @@ def _get_response_bodies(self):
 
         return response
 
+    def _get_response_for_code(
+        self, serializer, status_code, media_types=None, direction="response"
+    ):
+        """Hack to replace the InheritSerializer with the real deal."""
+        if serializer == InheritSerializer:
+            serializer = self._get_serializer()
+
+        return super()._get_response_for_code(serializer, status_code, media_types, direction)
+
 
 class PulpSchemaGenerator(SchemaGenerator):
     """Pulp Schema Generator."""

pulpcore/tests/functional/api/test_crud_domains.py

@@ -48,8 +48,8 @@ def test_crud_domains(pulpcore_bindings, monitor_task):
     response = pulpcore_bindings.DomainsApi.partial_update_with_http_info(
         domain.pulp_href, update_body
     )
-    assert response.status_code == 204
-    assert response.data is None
+    assert response.status_code == 200
+    assert response.data == domain
 
     # Delete the domain
     response = pulpcore_bindings.DomainsApi.delete(domain.pulp_href)

pulpcore/tests/functional/api/using_plugin/test_crud_repos.py

@@ -210,8 +210,8 @@ def _compare_results(data, received):
 
     # An update request with no changes should return a 200 OK (without dispatching a task)
     response = file_bindings.RemotesFileApi.partial_update_with_http_info(remote.pulp_href, data)
-    assert response.status_code == 204
-    assert response.data is None
+    assert response.status_code == 200
+    _compare_results(data, response.data)
 
     # Test that a password can be updated with a PUT request.
     temp_remote = file_remote_factory(
@@ -231,6 +231,7 @@ def _compare_results(data, received):
     assert exc.stdout.rstrip("\n") == "changed"
 
     # Test that password doesn't get unset when not passed with a PUT request.
+    # QUESTION: Why not? PUT is supposed to replace the whole entity in place.
     temp_remote = file_remote_factory(url="http://", password="new")
     href = temp_remote.pulp_href
     uuid = re.search(r"/api/v3/remotes/file/file/([\w-]+)/", href).group(1)
@@ -241,8 +242,8 @@ def _compare_results(data, received):
     # test a PUT request without a password
     remote_update = {"name": temp_remote.name, "url": "http://"}
     response = file_bindings.RemotesFileApi.update_with_http_info(href, remote_update)
-    assert response.status_code == 204
-    assert response.data is None
+    assert response.status_code == 200
+    _compare_results(remote_update, response.data)
     exc = run(["pulpcore-manager", "shell", "-c", shell_cmd], text=True, capture_output=True)
     assert exc.stdout.rstrip("\n") == "new"