openwisp / openwisp/openwisp-utils
[bug] ValidatedModelSerializer validates a copy, discarding Model.clean() corrections
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 93
- Forks
- 104
- Avg merge
- 14h 10m
- Merged PRs (30d)
- 22
Description
Describe the bug
45931f7f579e (#633, released in 1.3) changed ValidatedModelSerializer.validate()
to validate a copy of the instance on the update path:
else:
# Validate incoming PUT/PATCH data without mutating the DB instance.
instance = copy(instance)
Applying the incoming data before validation is a real fix and 1.2.x was wrong
not to do it. But validating a copy silently breaks models whose clean()
corrects state by assignment: DRF's ModelSerializer.update() saves the
original instance with no update_fields, so under 1.2.x those assignments
reached the database, while under 1.3 they are written to a throwaway and lost.
openwisp-controller's own Template.clean() depends on this in three places
(openwisp_controller/config/base/template.py, 1.2.3):
elif self.type != "vpn":
self.vpn = None
self.auto_cert = False
...
if self.required and not self.default:
self.default = True
Steps To Reproduce
- Create a template with
type="vpn"and a VPN attached. PATCH /api/v1/controller/template/<pk>/with
{"type": "generic", "config": {"interfaces": []}}→200.PATCH /api/v1/controller/template/<pk>/with{"name": "renamed"}→200.- Re-read the row and inspect
vpn.
Measured on identical code, varying only ValidatedModelSerializer.validate
(the 1.3 body applied via mock.patch):
| after step 2 | after step 3 | |
|---|---|---|
| utils 1.2.2 | type=generic, vpn=<uuid> |
vpn=None — corrected |
| utils 1.3 | type=generic, vpn=<uuid> |
vpn=<uuid> — never corrected |
PATCH {"required": true} on any template shows the same shape: default stays
False permanently instead of being forced to True.
Expected behavior
A write should be validated against the state it will save, and any correction
clean() makes to that state should be what gets persisted. Note neither
version validates the submitted state at the moment of the write — step 2 is
validated while the row is still type=vpn and saves an inconsistent row under
both versions. The difference is that 1.2.x recovered on the next write and 1.3
does not recover at all.
Three possible directions:
- apply the data to the real instance, accepting
clean()-as-normalisation,
which is what callers already rely on; - keep the copy, but copy cleaned values back onto the instance for concrete
local fields; - declare that
clean()must not mutate, and fix the affected
openwisp-controllermodels — cleanest contract, but breaking for
downstreams.
We hit this on a downstream model with a type-scoped permission field: under the
1.3 body, a template converted away from that type keeps granting device access
indefinitely, because the clean() that strips the generated credentials only
ever runs against a discarded copy. Fail-unsafe, and invisible — every status
code stays 200.
openwisp-controller 1.2.3 (latest on PyPI) pins openwisp-utils~=1.2.0, so
this pairing isn't reachable from a released controller yet, which is why it
seems worth catching before a 1.3-compatible controller ships.
Screenshots
N/A.
System Informatioon:
- OS: Pop!_OS 24.04 LTS
- Python Version: Python 3.13.5
- Django Version: Django 5.2.16
- Browser and Browser Version (if applicable): N/A (REST API)
Also: openwisp-utils 1.2.2 (1.3 behaviour reproduced by patching in the 1.3
validate body), openwisp-controller 1.2.3, djangorestframework 3.16.1.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at ValidatedModelSerializer.validate(), especially the update path that copies the instance, and compare it with DRF ModelSerializer.update(). Reproduce the behavior using openwisp-controller's Template.clean() in openwisp_controller/config/base/template.py. Done means corrected values from clean() are persisted on update, with regression coverage for the type/vpn and required/default cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- django, python
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100