[BUG] Two reproducible issues in DRF nested serializers with many to many t…#8627
[BUG] Two reproducible issues in DRF nested serializers with many to many t…#8627em1208 wants to merge 3 commits intoencode:mainfrom
Conversation
|
I managed to fix the |
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
my questions,
were you able to fix many to many through model issue in DRF? also the name of test app as issue is quite ambigous, do you have any better name in mind? or can we add proposed tests to exiting tests/test apps?
|
@auvipy I believe there is a problem with |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Pull request overview
Adds a new tests.issue Django test app intended to reproduce two reported DRF issues involving nested serializers for a many-to-many relationship with a through model, including both serializer-only and viewset/APIClient-based repro cases.
Changes:
- Introduces a minimal model/serializer/viewset + router setup under
tests/issue/for aSummary↔ItemM2M throughItemAmount. - Adds regression tests that exercise serializer validation and APIClient POST behavior for nested payloads.
- Registers
tests.issuein the test suite’sINSTALLED_APPS.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/issue/models.py |
Adds minimal Item, ItemAmount (through), and Summary models for the repro. |
tests/issue/serializers.py |
Adds nested SummarySerializer writing through ItemAmount via through_defaults. |
tests/issue/views.py |
Adds a ModelViewSet exposing Summary CRUD for API-level reproduction. |
tests/issue/urls.py |
Registers the viewset on a DRF router for test routing. |
tests/issue/test_issue.py |
Adds serializer and APIClient tests for nested write scenarios. |
tests/issue/migrations/0001_initial.py |
Adds initial migration for the new test app models. |
tests/conftest.py |
Adds tests.issue to INSTALLED_APPS so the models/migrations are available in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class SummaryViewSet(viewsets.ModelViewSet): | ||
| """ | ||
| A simple ViewSet for viewing and editing accounts. |
There was a problem hiding this comment.
The viewset docstring says it edits "accounts", but this viewset is for Summary. Updating the docstring (or removing it) will avoid misleading documentation in the test app.
| A simple ViewSet for viewing and editing accounts. | |
| A simple ViewSet for viewing and editing summaries. |
| ], | ||
| } | ||
| response = api_client.post(reverse('summary-list'), data) | ||
| print(response.content) |
There was a problem hiding this comment.
print(response.content) will add noisy output to the test suite and can mask failures in CI logs. Please remove the print and assert on the relevant response content instead (e.g., status + response.data/error details).
| print(response.content) |
| } | ||
| ], | ||
| } | ||
| response = api_client.post(reverse('summary-list'), data) |
There was a problem hiding this comment.
APIClient.post(..., data) without an explicit format uses the default test request format (multipart). DRF's MultiPartRenderer explicitly does not support nested structures, and existing tests expect an AssertionError with a helpful message for nested multipart data (see tests/test_testing.py::test_invalid_multipart_data). This test currently expects a 201, but it should either (a) use format='json'/content_type='application/json' if the intent is to test nested JSON, or (b) assert that an AssertionError (or 400 with a specific error) is raised when trying to send nested data as multipart.
| response = api_client.post(reverse('summary-list'), data) | |
| response = api_client.post(reverse('summary-list'), data, format='json') |
| response = api_client.post(reverse('summary-list'), json.dumps(data), content_type='application/json') | ||
| assert response.status_code == 201 |
There was a problem hiding this comment.
For consistency with the rest of the test suite, prefer api_client.post(..., data, format='json') over manually calling json.dumps + content_type='application/json'. This keeps tests shorter and ensures the DRF test client codepath is exercised consistently.
| # Generated by Django 3.1.13 on 2022-01-23 09:00 | ||
|
|
There was a problem hiding this comment.
This migration includes an auto-generated header with a specific Django version/date ("Generated by Django 3.1.13 on 2022-01-23"). Other test apps' migrations in this repo are kept minimal and don't embed generator metadata (e.g., tests/authentication/migrations/0001_initial.py). Consider removing the header to avoid confusion and unnecessary churn across Django versions.
| # Generated by Django 3.1.13 on 2022-01-23 09:00 |
…hrough model
Note: Before submitting this pull request, please review our contributing guidelines.
Description
I have found two issues when working on a nested serializer with many to many with through model. The first one is related on how
APIClientprepare the request with nested json usingmultipart/form-datawhich is not passed as json to request.data, so in this case the serializer raises, complaining about missingitemswhich is actually passed. The second one is related on howto_representationis called which is raising an attribute error. Any help to find the root cause of these two issues would be much appreciated.