[fix] Fixed ValidatedModelSerializer to handle update requests #633#634
[fix] Fixed ValidatedModelSerializer to handle update requests #633#634
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
📝 WalkthroughWalkthroughThis PR fixes validation in ValidatedModelSerializer by copying the existing instance (using copy) and applying incoming data to that copy before calling instance.full_clean(), so model validation runs against the merged (current + incoming) state for update operations. It also adds DRF List/Create and Retrieve/Update/Destroy views, URLs, and tests for a Shelf model exercising create/list/retrieve/update/delete and model validation behavior for POST/PUT/PATCH. Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant View as API View
participant Serializer as ValidatedModelSerializer
participant Model as Model Instance (copy)
participant DB as Database
Client->>View: HTTP POST/PUT/PATCH/GET/DELETE
View->>Serializer: instantiate with instance (if update) and data
Serializer->>Serializer: if update -> copy(self.instance)
Serializer->>Serializer: apply incoming data to copied instance (skip M2M)
Serializer->>Model: call full_clean() on copied instance
Model->>Model: run model.clean() and field validation
Model->>DB: (validation may query DB for uniqueness/constraints)
Serializer-->>View: validation result (valid/raises)
View->>DB: create/update/delete/query as appropriate
View-->>Client: HTTP response (201/200/400/204/etc.)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
pandafy
force-pushed
the
issues/633-ValidatedModelSerializer
branch
from
a574e1a to
9c48bec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_api.py`:
- Around line 83-87: Add explicit PUT-path validation tests alongside the
existing PATCH subtests: within the same test block that uses detail_url and
resp, add a self.subTest("Update - PUT (valid)") that issues
self.client.put(detail_url, <full_valid_payload>,
content_type="application/json") and asserts resp.status_code ==
status.HTTP_200_OK, and a self.subTest("Update - PUT (invalid)") that issues
self.client.put(detail_url,
<invalid_payload_missing_required_fields_or_bad_types>,
content_type="application/json") and asserts resp.status_code ==
status.HTTP_400_BAD_REQUEST; mirror the same additions for the other instance
around lines 111-116 so both update paths exercise valid and invalid PUT
behavior (use the same detail_url, resp variable, and status.HTTP_200_OK /
status.HTTP_400_BAD_REQUEST symbols).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2de449c-0efe-4456-bab6-2c3576b45ce0
📒 Files selected for processing (4)
openwisp_utils/api/serializers.pytests/test_project/api/urls.pytests/test_project/api/views.pytests/test_project/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_utils/api/serializers.pytests/test_project/api/urls.pytests/test_project/tests/test_api.pytests/test_project/api/views.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior
📚 Learning: 2026-03-14T20:44:14.568Z
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
tests/test_project/tests/test_api.py
🔇 Additional comments (3)
openwisp_utils/api/serializers.py (1)
24-31: Merged-state validation flow looks correct.Using a copied instance for updates and validating via
full_cleanafter applying incoming non-M2M fields matches the intended behavior and avoids mutating the original object during validation.tests/test_project/api/views.py (1)
28-40: Shelf CRUD view additions are clean and consistent.The list/create and retrieve/update/destroy views are correctly defined and exposed for URL wiring and test coverage.
tests/test_project/api/urls.py (1)
11-16: URL routing for shelf endpoints is correctly wired.Both collection and detail routes are aligned with the new shelf views and naming used by tests.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_project/tests/test_api.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c727bbc7-c8ec-4bc9-8ab3-3c1eddebdab2
📒 Files selected for processing (1)
tests/test_project/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/test_project/tests/test_api.py
🧠 Learnings (3)
📚 Learning: 2026-03-14T20:44:14.568Z
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Changes: Update tests to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
tests/test_project/tests/test_api.py
📚 Learning: 2026-03-14T20:44:14.568Z
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Applied to files:
tests/test_project/tests/test_api.py
📚 Learning: 2026-03-14T20:44:14.568Z
Learnt from: CR
Repo: openwisp/openwisp-utils PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T20:44:14.568Z
Learning: Features: Add tests for new features and ensure coverage does not decrease significantly; prefer Selenium browser tests for UI-impacting features
Applied to files:
tests/test_project/tests/test_api.py
🔇 Additional comments (6)
tests/test_project/tests/test_api.py (6)
1-16: LGTM!Clean import organization with clear distinction between Django's
ValidationErrorand DRF'sValidationError. The aliasing (DjangoValidationError) aids readability when testing both validation layers.
17-31: LGTM!The test verifies that validation passes without raising an exception, which is a valid pattern. The M2M field (
writers) is included in the data dict, exercising the serializer's handling of such fields.
33-42: LGTM!Excellent coverage testing both Django model validation (
DjangoValidationErroron create) and DRF serializer validation (ValidationErrorviais_valid(raise_exception=True)on update). This aligns with the PR objective of ensuring model validation runs against incoming data.
51-62: LGTM!Configuration verification test is straightforward and correct.
64-119: LGTM!Comprehensive CRUD test covering Create, List, Retrieve, PATCH, PUT, and Delete operations. The inclusion of both PATCH and PUT update tests aligns with the PR objectives. Good practice verifying database state changes via
refresh_from_db().
121-175: LGTM!Excellent test coverage for the core PR objective. Tests validate:
- Invalid/valid create operations
- Invalid PATCH updates (model validation against merged state)
- Valid/invalid PUT updates
- Database integrity after failed validation attempts
This directly exercises the fix ensuring
ValidatedModelSerializervalidates against the merged instance+data state for updates.Based on learnings: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs – this test covers happy paths (valid create/update), error paths (invalid payloads), and verifies no side effects.
| 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"] |
There was a problem hiding this comment.
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.
|
We should not merge this PR yet. The changes cause several tests in openwisp-controller to fail, which suggests there may be compatibility issues with projects that rely on ValidatedModelSerializer. Before merging, we should test this patch against all OpenWISP packages that use ValidatedModelSerializer and ensure the required updates are prepared in those repositories. Once the dependent repositories are ready, we can proceed with the merge. Merging prematurely would likely result in multiple failing CI pipelines across the ecosystem. |
pandafy
added
the
Blocked
|
The current PR does not include tests for the exclude_validation logic. Due to limited time, I’m unable to continue working on this PR at the moment. I will close this PR for now, as leaving it open may discourage other contributors from addressing #633. |
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
2 participants
Checklist
Reference to Existing Issue
Fixes #633