[fix] Fixed ValidatedModelSerializer to handle update requests #633

Fixes #633

Author: pandafy

openwisp_utils/api/serializers.py

@@ -1,3 +1,5 @@
+from copy import copy
+
 from django.core.exceptions import ImproperlyConfigured
 from django.db import models
 
@@ -19,16 +21,12 @@ def validate(self, data):
         Allows to avoid having to duplicate model validation logic in the
         REST API.
         """
-        instance = self.instance
-        # if instance is empty (eg: creation)
-        # simulate for validation purposes
-        if not instance:
-            Model = self.Meta.model
-            instance = Model()
-            for key, value in data.items():
-                # avoid direct assignment for m2m (not allowed)
-                if not isinstance(Model._meta.get_field(key), models.ManyToManyField):
-                    setattr(instance, key, value)
+        Model = self.Meta.model
+        instance = copy(self.instance) if self.instance else Model()
+        for key, value in data.items():
+            # avoid direct assignment for m2m (not allowed)
+            if not isinstance(Model._meta.get_field(key), models.ManyToManyField):
+                setattr(instance, key, value)
         # perform model validation
         instance.full_clean(exclude=self.exclude_validation)
         return data

tests/test_project/api/urls.py

@@ -1,4 +1,4 @@
-from django.urls import re_path
+from django.urls import path, re_path
 
 from . import views
 
@@ -7,5 +7,11 @@
         r"^receive_project/(?P<pk>[^/\?]+)/$",
         views.receive_project,
         name="receive_project",
-    )
+    ),
+    path("shelves/", views.shelf_list_create_view, name="shelf_list"),
+    path(
+        "shelves/<uuid:pk>/",
+        views.shelf_retrieve_update_destroy_view,
+        name="shelf_detail",
+    ),
 ]

tests/test_project/api/views.py

@@ -1,8 +1,10 @@
 from django.http import JsonResponse
 from django.utils.translation import gettext_lazy as _
 from django.views import View
+from rest_framework import generics
+from test_project.serializers import ShelfSerializer
 
-from ..models import Project
+from ..models import Project, Shelf
 
 
 class ReceiveProjectView(View):
@@ -23,4 +25,16 @@ def get(self, request, pk):
         return JsonResponse({"detail": _("ok"), "name": project.name}, status=200)
 
 
+class ShelfListCreateView(generics.ListCreateAPIView):
+    queryset = Shelf.objects.all()
+    serializer_class = ShelfSerializer
+
+
+class ShelfRetrieveUpdateDestroyView(generics.RetrieveUpdateDestroyAPIView):
+    queryset = Shelf.objects.all()
+    serializer_class = ShelfSerializer
+
+
 receive_project = ReceiveProjectView.as_view()
+shelf_list_create_view = ShelfListCreateView.as_view()
+shelf_retrieve_update_destroy_view = ShelfRetrieveUpdateDestroyView.as_view()

tests/test_project/tests/test_api.py

@@ -1,6 +1,9 @@
 from django.conf import settings
-from django.core.exceptions import ValidationError
+from django.core.exceptions import ValidationError as DjangoValidationError
 from django.test import TestCase
+from django.urls import reverse
+from rest_framework import status
+from rest_framework.exceptions import ValidationError
 from test_project.serializers import ShelfSerializer
 
 from ..models import Shelf
@@ -11,15 +14,9 @@ class TestApi(CreateMixin, TestCase):
     shelf_model = Shelf
     operator_permission_filter = [{"codename__endswith": "shelf"}]
 
-    def test_validator_pass(self):
-        s1 = self._create_shelf(name="shelf1")
-        serializer = ShelfSerializer(s1)
-        result = serializer.validate(s1)
-        self.assertIsInstance(result, Shelf)
-
     def test_validator_data_dict(self):
         s1 = self._create_shelf(name="shelf1")
-        data = s1.__dict__
+        data = s1.__dict__.copy()
         to_delete = [
             "_state",
             "id",
@@ -30,27 +27,26 @@ def test_validator_data_dict(self):
         for key in to_delete:
             del data[key]
         data["writers"] = [1]
-        serializer = ShelfSerializer()
+        serializer = ShelfSerializer(instance=s1, data=data)
         data = serializer.validate(data)
 
     def test_validator_fail(self):
-        with self.assertRaises(ValidationError):
+        with self.assertRaises(DjangoValidationError):
             self._create_shelf(name="Intentional_Test_Fail")
 
         s1 = self._create_shelf(name="shelf1")
-        s1.name = "Intentional_Test_Fail"
-        serializer = ShelfSerializer(s1)
+        serializer = ShelfSerializer(
+            instance=s1, data={"name": "Intentional_Test_Fail"}
+        )
         with self.assertRaises(ValidationError):
-            serializer.validate(s1)
+            serializer.is_valid(raise_exception=True)
 
     def test_exclude_validation(self):
         s1 = self._create_shelf(name="shelf1")
-        s1.books_type = "madeup"
-        serializer = ShelfSerializer(s1)
+        serializer = ShelfSerializer(instance=s1, data={"books_type": "invalid"})
         with self.assertRaises(ValidationError):
-            serializer.validate(s1)
+            serializer.is_valid(raise_exception=True)
         serializer.exclude_validation = ["books_type"]
-        serializer.validate(s1)
 
     def test_rest_framework_settings_override(self):
         drf_conf = getattr(settings, "REST_FRAMEWORK", {})
@@ -64,3 +60,60 @@ def test_rest_framework_settings_override(self):
                 "TEST": True,
             },
         )
+
+    def test_crud_shelf(self):
+        list_url = reverse("shelf_list")
+        with self.subTest("Create"):
+            resp = self.client.post(list_url, {"name": "shelf1"}, format="json")
+            self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
+            self.assertEqual(Shelf.objects.count(), 1)
+            pk = resp.data["id"]
+
+        shelf = Shelf.objects.get(pk=pk)
+        detail_url = reverse("shelf_detail", args=[pk])
+        with self.subTest("List"):
+            resp = self.client.get(list_url)
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+            self.assertIn(pk, [s["id"] for s in resp.data])
+
+        with self.subTest("Retrieve"):
+            resp = self.client.get(detail_url)
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+
+        with self.subTest("Update"):
+            resp = self.client.patch(
+                detail_url, {"name": "shelf2"}, content_type="application/json"
+            )
+            self.assertEqual(resp.status_code, status.HTTP_200_OK)
+            # ensure the DB reflects the change
+            shelf.refresh_from_db()
+            self.assertEqual(shelf.name, "shelf2")
+
+        with self.subTest("Delete"):
+            resp = self.client.delete(detail_url)
+            self.assertEqual(resp.status_code, status.HTTP_204_NO_CONTENT)
+            self.assertEqual(Shelf.objects.count(), 0)
+
+    def test_model_validation_create_and_update(self):
+        list_url = reverse("shelf_list")
+        # Creating with invalid name should fail due to model.clean()
+        resp = self.client.post(
+            list_url, {"name": "Intentional_Test_Fail"}, format="json"
+        )
+        self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST)
+        # Create valid
+        resp = self.client.post(list_url, {"name": "valid"}, format="json")
+        self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
+
+        pk = resp.data["id"]
+        detail_url = reverse("shelf_detail", args=[pk])
+        # Updating to invalid should fail
+        resp = self.client.patch(
+            detail_url,
+            {"name": "Intentional_Test_Fail"},
+            content_type="application/json",
+        )
+        self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST)
+        # Ensure DB value unchanged
+        shelf = Shelf.objects.get(pk=pk)
+        self.assertEqual(shelf.name, "valid")