Repository: vitalik/django-ninja
Pull request #1232 (2024-07-15 03:30:25)

Fix unchanging field bug when validate assignment true
# Problem
This PR addresses a bug reported here, where if validate_assignment is set to True on a schema, then Pydantic will correctly validate the new values, but the schema object will not persist the new value.

# RCA
This is caused by the fact that [pydantic-core sets these new values on the object (once validated) directly on the object __ dict __](https://github.com/pydantic/pydantic-core/blob/0e6b377d2bef52e744deff22314269c8751f7b00/src/validators/dataclass.rs#L368). As it was, this means that the new value will be set onto the DjangoGetter object, rather than the schema object being validated.

# Solution
Added an extra condition in the root validator of a Schema object that will call `handler` on the original values before the conversion to a DjangoGetter to ensure the new value is persisted properly on the original object. Comment is also updated to reflect this change.

# Testing
Added assertions into an existing test that a reassigned value is persisted when validate_assignment = True, and an additional test to test for the lack of ValidationError when validate_assignment is False or None.

# Open Questions
This seems odd in the following way: I understand the use of DjangoGetter of course to handle resolvable fields. However, it not behaving like a dictionary is causing some churn where we now have to validate the object twice in certain conditions just to ensure fields are persisted/validated properly. I don't know immediately if there's a better solution, or if there's background here I don't know. Welcome to feedback.


Patch:
diff --git a/ninja/schema.py b/ninja/schema.py
index 55a60a8..e51dd43 100644
--- a/ninja/schema.py
+++ b/ninja/schema.py
@@ -215,9 +215,13 @@ class Schema(BaseModel, metaclass=ResolverMetaclass):
     def _run_root_validator(
         cls, values: Any, handler: ModelWrapValidatorHandler[S], info: ValidationInfo
     ) -> Any:
-        # when extra is "forbid" we need to perform default pydantic validation
-        # as DjangoGetter does not act as dict and pydantic will not be able to validate it
-        if cls.model_config.get("extra") == "forbid":
+        # If Pydantic intends to validate against the __dict__ of the immediate Schema
+        # object, then we need to call `handler` directly on `values` before the conversion
+        # to DjangoGetter, since any checks or modifications on DjangoGetter's __dict__
+        # will not persist to the original object.
+        forbids_extra = cls.model_config.get("extra") == "forbid"
+        should_validate_assignment = cls.model_config.get("validate_assignment", False)
+        if forbids_extra or should_validate_assignment:
             handler(values)
 
         values = DjangoGetter(values, cls, info.context)


Test patch:
diff --git a/tests/test_schema.py b/tests/test_schema.py
index 8f92d52..28b2ad0 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -1,4 +1,4 @@
-from typing import List, Optional
+from typing import List, Optional, Union
 from unittest.mock import Mock
 
 import pytest
@@ -197,21 +197,33 @@ def test_django_getter():
     assert repr(dg) == "<DjangoGetter: {'i': 1}>"
 
 
-def test_django_getter_validates_assignment():
+def test_schema_validates_assignment_and_reassigns_the_value():
     class ValidateAssignmentSchema(Schema):
         str_var: str
-
         model_config = {"validate_assignment": True}
 
     schema_inst = ValidateAssignmentSchema(str_var="test_value")
-
-    # Validate we can re-assign the value, this is a test for
-    # a bug where validate_assignment would cause an AttributeError
-    # for __dict__ on the target schema.
     schema_inst.str_var = "reassigned_value"
+    assert schema_inst.str_var == "reassigned_value"
     try:
         schema_inst.str_var = 5
         raise AssertionError()
     except ValidationError:
         # We expect this error, all is okay
         pass
+
+
+@pytest.mark.parametrize("test_validate_assignment", [False, None])
+def test_schema_skips_validation_when_validate_assignment_False(
+    test_validate_assignment: Union[bool, None],
+):
+    class ValidateAssignmentSchema(Schema):
+        str_var: str
+        model_config = {"validate_assignment": test_validate_assignment}
+
+    schema_inst = ValidateAssignmentSchema(str_var="test_value")
+    try:
+        schema_inst.str_var = 5
+        assert schema_inst.str_var == 5
+    except ValidationError as ve:
+        raise AssertionError() from ve