openwisp / openwisp/openwisp-utils

[bug] ValidatedModelSerializer validates a copy, discarding Model.clean() corrections

Open
#760 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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

  1. Create a template with type="vpn" and a VPN attached.
  2. PATCH /api/v1/controller/template/<pk>/ with
    {"type": "generic", "config": {"interfaces": []}}200.
  3. PATCH /api/v1/controller/template/<pk>/ with {"name": "renamed"}200.
  4. 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-controller models — 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.