Skip to content

[fix] Fixed ValidatedModelSerializer to handle update requests #633 #634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pandafy wants to merge 2 commits into from
+157 −30
Closed

[fix] Fixed ValidatedModelSerializer to handle update requests #633 #634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c48bec
[fix] Fixed ValidatedModelSerializer to handle update requests #633
pandafy Mar 25, 2026
85acd07
[fix] Fixes by @coderabbitai
pandafy Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions openwisp_utils/api/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from copy import copy

from django.core.exceptions import ImproperlyConfigured
from django.db import models

Expand All @@ -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
10 changes: 8 additions & 2 deletions tests/test_project/api/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from django.urls import re_path
from django.urls import path, re_path

from . import views

Expand All @@ -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",
),
]
16 changes: 15 additions & 1 deletion tests/test_project/api/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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):
Expand All @@ -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()
143 changes: 126 additions & 17 deletions tests/test_project/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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",
Expand All @@ -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"]
Comment on lines 44 to 49
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Incomplete test: exclude_validation setting is not actually tested.

Line 49 sets serializer.exclude_validation = ["books_type"] after the with self.assertRaises block completes, making it dead code. The test never verifies that excluding a field from validation actually allows the data to pass.

🛠️ Proposed fix to complete the test 
     def test_exclude_validation(self):
         s1 = self._create_shelf(name="shelf1")
-        serializer = ShelfSerializer(instance=s1, data={"books_type": "invalid"})
-        with self.assertRaises(ValidationError):
-            serializer.is_valid(raise_exception=True)
-        serializer.exclude_validation = ["books_type"]
+        # Without exclude_validation, invalid books_type should fail
+        serializer1 = ShelfSerializer(instance=s1, data={"books_type": "invalid"})
+        with self.assertRaises(ValidationError):
+            serializer1.is_valid(raise_exception=True)
+
+        # With exclude_validation set, the same data should pass validation
+        serializer2 = ShelfSerializer(instance=s1, data={"books_type": "invalid"})
+        serializer2.exclude_validation = ["books_type"]
+        self.assertTrue(serializer2.is_valid())
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/test_project/tests/test_api.py` around lines 44 - 49, The
test_exclude_validation currently sets serializer.exclude_validation after the
failure assertion, so it never verifies the bypass behavior; move or add a
second validation attempt where you set serializer.exclude_validation =
["books_type"] on the same ShelfSerializer instance (or a new instance) before
calling serializer.is_valid(raise_exception=True) and then assert it does not
raise (e.g., is_valid() is True) and/or that serializer.errors does not contain
"books_type"; reference the ShelfSerializer instance named serializer and the
is_valid method to implement this check.

serializer.validate(s1)

def test_rest_framework_settings_override(self):
drf_conf = getattr(settings, "REST_FRAMEWORK", {})
Expand All @@ -64,3 +60,116 @@ def test_rest_framework_settings_override(self):
"TEST": True,
},
)

def test_crud_shelf(self):
list_url = reverse("shelf_list")
with self.subTest("Create"):
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 = response.data["id"]

shelf = Shelf.objects.get(pk=pk)
detail_url = reverse("shelf_detail", args=[pk])
with self.subTest("List"):
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"):
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 with PATCH"):
response = self.client.patch(
detail_url, {"name": "Shelf - Updated"}, content_type="application/json"
)
self.assertEqual(response.status_code, status.HTTP_200_OK)
# ensure the DB reflects the change
shelf.refresh_from_db()
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"):
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")
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"]

detail_url = reverse("shelf_detail", args=[pk])
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)
Loading