-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[BUG] Two reproducible issues in DRF nested serializers with many to many t… #8627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # Generated by Django 3.1.13 on 2022-01-23 09:00 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| initial = True | ||
|
|
||
| dependencies = [ | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='Item', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('name', models.CharField(max_length=10)), | ||
| ], | ||
| ), | ||
| migrations.CreateModel( | ||
| name='ItemAmount', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('amount', models.IntegerField()), | ||
| ('item', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='issue.item')), | ||
| ], | ||
| ), | ||
| migrations.CreateModel( | ||
| name='Summary', | ||
| fields=[ | ||
| ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('items', models.ManyToManyField(through='issue.ItemAmount', to='issue.Item')), | ||
| ], | ||
| ), | ||
| migrations.AddField( | ||
| model_name='itemamount', | ||
| name='summary', | ||
| field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='issue.summary'), | ||
| ), | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| from django.db import models | ||
|
|
||
|
|
||
| class Item(models.Model): | ||
| name = models.CharField(max_length=10) | ||
|
|
||
|
|
||
| class ItemAmount(models.Model): | ||
| summary = models.ForeignKey('Summary', on_delete=models.CASCADE) | ||
| item = models.ForeignKey(Item, on_delete=models.CASCADE) | ||
| amount = models.IntegerField() | ||
|
|
||
|
|
||
| class Summary(models.Model): | ||
| items = models.ManyToManyField(Item, through=ItemAmount) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| from rest_framework import serializers | ||
|
|
||
| from . import models | ||
|
|
||
|
|
||
| class ItemAmountSerializer(serializers.ModelSerializer): | ||
| item = serializers.PrimaryKeyRelatedField(queryset=models.Item.objects.all()) | ||
|
|
||
| class Meta: | ||
| model = models.ItemAmount | ||
| fields = ('item', 'amount') | ||
|
|
||
|
|
||
| class SummarySerializer(serializers.ModelSerializer): | ||
| items = ItemAmountSerializer(source='itemamount_set', many=True) | ||
|
|
||
| def create(self, validated_data): | ||
| items = validated_data.pop('itemamount_set') | ||
| instance = super().create(validated_data) | ||
| for item in items: | ||
| instance.items.add( | ||
| item['item'], through_defaults=dict( | ||
| amount=item['amount'] | ||
| ) | ||
| ) | ||
| return instance | ||
|
|
||
| class Meta: | ||
| model = models.Summary | ||
| fields = ('items', ) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||
| from django.test import TestCase, override_settings | ||||||
| from django.urls import reverse | ||||||
|
|
||||||
| from rest_framework.test import APIClient | ||||||
| from rest_framework.utils import json | ||||||
|
|
||||||
| from . import models, serializers | ||||||
|
|
||||||
|
|
||||||
| class TestSerializer(TestCase): | ||||||
| def test_create(self): | ||||||
| item = models.Item.objects.create(name='test') | ||||||
| data = { | ||||||
| "items": [ | ||||||
| { | ||||||
| "item": item.id, | ||||||
| "amount": 100, | ||||||
| } | ||||||
| ], | ||||||
| } | ||||||
| serializer = serializers.SummarySerializer(data=data) | ||||||
| serializer.is_valid(raise_exception=True) | ||||||
| expected_data = { | ||||||
| "itemamount_set": [ | ||||||
| { | ||||||
| "item": item, | ||||||
| "amount": 100, | ||||||
| } | ||||||
| ], | ||||||
| } | ||||||
| assert serializer.validated_data == expected_data | ||||||
| serializer.save() | ||||||
| assert models.Summary.objects.count() == 1 | ||||||
|
|
||||||
|
|
||||||
| @override_settings(ROOT_URLCONF='tests.issue.urls') | ||||||
| class TestIssueViewSet(TestCase): | ||||||
| def test_create(self): | ||||||
| api_client = APIClient() | ||||||
| item = models.Item.objects.create(name='test') | ||||||
| data = { | ||||||
| "items": [ | ||||||
| { | ||||||
| "item": item.id, | ||||||
| "amount": 100, | ||||||
| } | ||||||
| ], | ||||||
| } | ||||||
| response = api_client.post(reverse('summary-list'), data) | ||||||
|
||||||
| response = api_client.post(reverse('summary-list'), data) | |
| response = api_client.post(reverse('summary-list'), data, format='json') |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from rest_framework.routers import DefaultRouter | ||
|
|
||
| from . import views | ||
|
|
||
| app_name = "issue" | ||
| router = DefaultRouter() | ||
|
|
||
| router.register(r'summary', views.SummaryViewSet, basename='summary') | ||
|
|
||
| urlpatterns = router.urls |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,13 @@ | ||||||
| from rest_framework import viewsets | ||||||
| from rest_framework.permissions import AllowAny | ||||||
|
|
||||||
| from . import models, serializers | ||||||
|
|
||||||
|
|
||||||
| class SummaryViewSet(viewsets.ModelViewSet): | ||||||
| """ | ||||||
| A simple ViewSet for viewing and editing accounts. | ||||||
|
||||||
| A simple ViewSet for viewing and editing accounts. | |
| A simple ViewSet for viewing and editing summaries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.