From 9c48bec292c3b720f0996ccd09012e1699021ab7 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 25 Mar 2026 15:07:06 +0530 Subject: [PATCH 1/2] [fix] Fixed ValidatedModelSerializer to handle update requests #633 Fixes #633 --- openwisp_utils/api/serializers.py | 18 +++--- tests/test_project/api/urls.py | 10 +++- tests/test_project/api/views.py | 16 ++++- tests/test_project/tests/test_api.py | 87 ++++++++++++++++++++++------ 4 files changed, 101 insertions(+), 30 deletions(-) diff --git a/openwisp_utils/api/serializers.py b/openwisp_utils/api/serializers.py index 29a58cce..080d4c2b 100644 --- a/openwisp_utils/api/serializers.py +++ b/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 diff --git a/tests/test_project/api/urls.py b/tests/test_project/api/urls.py index 0fdc80ae..0f78eb6a 100644 --- a/tests/test_project/api/urls.py +++ b/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[^/\?]+)/$", views.receive_project, name="receive_project", - ) + ), + path("shelves/", views.shelf_list_create_view, name="shelf_list"), + path( + "shelves//", + views.shelf_retrieve_update_destroy_view, + name="shelf_detail", + ), ] diff --git a/tests/test_project/api/views.py b/tests/test_project/api/views.py index c73c1afb..ad7e05c0 100644 --- a/tests/test_project/api/views.py +++ b/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() diff --git a/tests/test_project/tests/test_api.py b/tests/test_project/tests/test_api.py index a862d0b5..8956b62c 100644 --- a/tests/test_project/tests/test_api.py +++ b/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") From 85acd07ee2f84fcc4197a3fe76f4aa0841f7ff93 Mon Sep 17 00:00:00 2001 From: Gagan Deep Date: Wed, 25 Mar 2026 15:41:15 +0530 Subject: [PATCH 2/2] [fix] Fixes by @coderabbitai --- tests/test_project/tests/test_api.py | 124 +++++++++++++++++++-------- 1 file changed, 90 insertions(+), 34 deletions(-) diff --git a/tests/test_project/tests/test_api.py b/tests/test_project/tests/test_api.py index 8956b62c..11ed4dcb 100644 --- a/tests/test_project/tests/test_api.py +++ b/tests/test_project/tests/test_api.py @@ -64,56 +64,112 @@ def test_rest_framework_settings_override(self): 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) + response = self.client.post( + list_url, + { + "name": "Fiction Shelf", + "books_type": "FANTASY", + "books_count": 3, + "locked": False, + }, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.assertEqual(Shelf.objects.count(), 1) - pk = resp.data["id"] + pk = response.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]) + response = self.client.get(list_url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data[0]["id"], pk) with self.subTest("Retrieve"): - resp = self.client.get(detail_url) - self.assertEqual(resp.status_code, status.HTTP_200_OK) + response = self.client.get(detail_url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data["id"], pk) - with self.subTest("Update"): - resp = self.client.patch( - detail_url, {"name": "shelf2"}, content_type="application/json" + with self.subTest("Update with PATCH"): + response = self.client.patch( + detail_url, {"name": "Shelf - Updated"}, content_type="application/json" ) - self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(response.status_code, status.HTTP_200_OK) # ensure the DB reflects the change shelf.refresh_from_db() - self.assertEqual(shelf.name, "shelf2") + self.assertEqual(shelf.name, "Shelf - Updated") + + with self.subTest("Update with PUT"): + payload = { + "name": "Shelf PUT Full", + "books_type": "FACTUAL", + "books_count": 5, + "locked": False, + } + response = self.client.put( + detail_url, payload, content_type="application/json" + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + shelf.refresh_from_db() + self.assertEqual(shelf.name, "Shelf PUT Full") with self.subTest("Delete"): - resp = self.client.delete(detail_url) - self.assertEqual(resp.status_code, status.HTTP_204_NO_CONTENT) + response = self.client.delete(detail_url) + self.assertEqual(response.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) + with self.subTest("Create (invalid)"): + response = self.client.post( + list_url, {"name": "Intentional_Test_Fail"}, format="json" + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + with self.subTest("Create (valid)"): + response = self.client.post( + list_url, + { + "name": "Reference Shelf", + "books_type": "FACTUAL", + "books_count": 7, + "locked": True, + }, + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + pk = response.data["id"] - 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") + with self.subTest("Update - PATCH (invalid)"): + response = self.client.patch( + detail_url, + {"name": "Intentional_Test_Fail"}, + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + with self.subTest("Update - PUT (valid)"): + payload = { + "name": "Valid PUT Shelf", + "books_type": "FACTUAL", + "books_count": 10, + "locked": True, + } + response = self.client.put( + detail_url, payload, content_type="application/json" + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + with self.subTest("Update - PUT (invalid)"): + invalid_payload = {"books_count": 7} + response = self.client.put( + detail_url, invalid_payload, content_type="application/json" + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + with self.subTest("DB value unchanged after failed updates"): + shelf = Shelf.objects.get(pk=pk) + self.assertEqual(shelf.name, "Valid PUT Shelf") + self.assertEqual(shelf.books_count, 10) + self.assertEqual(shelf.locked, True)