From b34b5bea20ff64f4d1568a4524f797d104419bff Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Sat, 15 Oct 2022 15:43:02 +0200 Subject: [PATCH 1/6] Fix #298 -- create m2m when using `_bulk_create=True` --- CHANGELOG.md | 1 + model_bakery/baker.py | 19 ++++++++++++++++++- tests/test_baker.py | 12 ++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fdd2f2a6..ca76f0e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Fixed a bug with `seq` being passed a tz-aware start value [PR #353](https://github.com/model-bakers/model_bakery/pull/353) - [dev] Use official postgis docker image in CI [PR #355](https://github.com/model-bakers/model_bakery/pull/355) +- Create m2m when using `_bulk_create=True` [PR #354](https://github.com/model-bakers/model_bakery/pull/354) ### Removed diff --git a/model_bakery/baker.py b/model_bakery/baker.py index 930f50b2..f7187c3c 100644 --- a/model_bakery/baker.py +++ b/model_bakery/baker.py @@ -787,4 +787,21 @@ def _save_related_objs(model, objects) -> None: else: manager = baker.model._base_manager - return manager.bulk_create(entries) + created_entries = manager.bulk_create(entries) + # set many-to-many relations from kwargs + for entry in created_entries: + for field in baker.model._meta.many_to_many: + if field.name in kwargs: + through_model = getattr(entry, field.name).through + through_model.objects.bulk_create( + [ + through_model( + **{ + field.remote_field.name: entry, + field.related_model._meta.model_name: obj, + } + ) + for obj in kwargs[field.name] + ] + ) + return created_entries diff --git a/tests/test_baker.py b/tests/test_baker.py index e955f9d6..33e10e2e 100644 --- a/tests/test_baker.py +++ b/tests/test_baker.py @@ -1032,3 +1032,15 @@ def test_annotation_within_manager_get_queryset_are_run_on_make(self): _from_manager="objects", ) assert movie.title == movie.name + + +class TestCreateM2MWhenBulkCreate(TestCase): + @pytest.mark.django_db + def test_create(self): + with self.assertNumQueries(22): + person = baker.make(models.Person) + baker.make( + models.Classroom, students=[person], _quantity=20, _bulk_create=True + ) + c1, c2 = models.Classroom.objects.all()[:2] + assert list(c1.students.all()) == list(c2.students.all()) == [person] From 31b2cfda9286839fee5c777f930681dbd662aa6d Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Sat, 15 Oct 2022 18:27:20 +0200 Subject: [PATCH 2/6] [draft] Fix logic for Django 3.2 (SQLite) --- model_bakery/baker.py | 4 ++++ tests/test_baker.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/model_bakery/baker.py b/model_bakery/baker.py index f7187c3c..dc7c2eb6 100644 --- a/model_bakery/baker.py +++ b/model_bakery/baker.py @@ -787,7 +787,11 @@ def _save_related_objs(model, objects) -> None: else: manager = baker.model._base_manager + existing_entries = list(manager.values_list("pk", flat=True)) created_entries = manager.bulk_create(entries) + # bulk_create in Django < 4.0 does not return ids of created objects + created_entries = manager.exclude(pk__in=existing_entries) + # set many-to-many relations from kwargs for entry in created_entries: for field in baker.model._meta.many_to_many: diff --git a/tests/test_baker.py b/tests/test_baker.py index 33e10e2e..dda0c9b1 100644 --- a/tests/test_baker.py +++ b/tests/test_baker.py @@ -1037,7 +1037,7 @@ def test_annotation_within_manager_get_queryset_are_run_on_make(self): class TestCreateM2MWhenBulkCreate(TestCase): @pytest.mark.django_db def test_create(self): - with self.assertNumQueries(22): + with self.assertNumQueries(24): person = baker.make(models.Person) baker.make( models.Classroom, students=[person], _quantity=20, _bulk_create=True From 8d0900e69651fcc0a5e66f16f1782d08b50c13f0 Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Sun, 16 Oct 2022 21:34:25 +0200 Subject: [PATCH 3/6] Increase query limit for several tests M2M lookups are adding up to the query count, we need to adjust tests to that. --- tests/test_baker.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/test_baker.py b/tests/test_baker.py index dda0c9b1..735c4613 100644 --- a/tests/test_baker.py +++ b/tests/test_baker.py @@ -151,17 +151,17 @@ def test_make_should_create_objects_respecting_quantity_parameter(self): assert all(p.name == "George Washington" for p in people) def test_make_quantity_respecting_bulk_create_parameter(self): - with self.assertNumQueries(1): + with self.assertNumQueries(3): baker.make(models.Person, _quantity=5, _bulk_create=True) assert models.Person.objects.count() == 5 - with self.assertNumQueries(1): + with self.assertNumQueries(3): people = baker.make( models.Person, name="George Washington", _quantity=5, _bulk_create=True ) assert all(p.name == "George Washington" for p in people) - with self.assertNumQueries(1): + with self.assertNumQueries(3): baker.make(models.NonStandardManager, _quantity=3, _bulk_create=True) assert getattr(models.NonStandardManager, "objects", None) is None assert ( @@ -362,7 +362,7 @@ def test_create_multiple_one_to_one(self): assert models.Person.objects.all().count() == 5 def test_bulk_create_multiple_one_to_one(self): - with self.assertNumQueries(6): + with self.assertNumQueries(8): baker.make(models.LonelyPerson, _quantity=5, _bulk_create=True) assert models.LonelyPerson.objects.all().count() == 5 @@ -370,22 +370,22 @@ def test_bulk_create_multiple_one_to_one(self): def test_chaining_bulk_create_reduces_query_count(self): qtd = 5 - with self.assertNumQueries(3): + with self.assertNumQueries(7): baker.make(models.Person, _quantity=qtd, _bulk_create=True) person_iter = models.Person.objects.all().iterator() baker.make( models.LonelyPerson, only_friend=person_iter, - _quantity=5, + _quantity=qtd, _bulk_create=True, ) # 2 bulk create and 1 select on Person - assert models.LonelyPerson.objects.all().count() == 5 - assert models.Person.objects.all().count() == 5 + assert models.LonelyPerson.objects.all().count() == qtd + assert models.Person.objects.all().count() == qtd def test_bulk_create_multiple_fk(self): - with self.assertNumQueries(6): + with self.assertNumQueries(8): baker.make(models.PaymentBill, _quantity=5, _bulk_create=True) assert models.PaymentBill.objects.all().count() == 5 @@ -396,7 +396,7 @@ def test_create_many_to_many_if_flagged(self): assert store.employees.count() == 5 assert store.customers.count() == 5 - def test_regresstion_many_to_many_field_is_accepted_as_kwargs(self): + def test_regression_many_to_many_field_is_accepted_as_kwargs(self): employees = baker.make(models.Person, _quantity=3) customers = baker.make(models.Person, _quantity=3) From 257ef4312f05dcc418b6b4767b343d2a577feccf Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Sun, 16 Oct 2022 21:48:14 +0200 Subject: [PATCH 4/6] Perform extra M2M query for pks only for Django < 4.0 --- model_bakery/baker.py | 7 +++++-- tests/test_baker.py | 31 ++++++++++++++++++------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/model_bakery/baker.py b/model_bakery/baker.py index dc7c2eb6..a5a818aa 100644 --- a/model_bakery/baker.py +++ b/model_bakery/baker.py @@ -13,6 +13,7 @@ overload, ) +import django from django.apps import apps from django.conf import settings from django.contrib import contenttypes @@ -789,8 +790,10 @@ def _save_related_objs(model, objects) -> None: existing_entries = list(manager.values_list("pk", flat=True)) created_entries = manager.bulk_create(entries) - # bulk_create in Django < 4.0 does not return ids of created objects - created_entries = manager.exclude(pk__in=existing_entries) + # bulk_create in Django < 4.0 does not return ids of created objects. + # drop this after 01 Apr 2024 (Django 3.2 LTS end of life) + if django.VERSION < (4, 0): + created_entries = manager.exclude(pk__in=existing_entries) # set many-to-many relations from kwargs for entry in created_entries: diff --git a/tests/test_baker.py b/tests/test_baker.py index 735c4613..69d67890 100644 --- a/tests/test_baker.py +++ b/tests/test_baker.py @@ -3,6 +3,7 @@ from decimal import Decimal from unittest.mock import patch +import django import pytest from django.conf import settings from django.contrib.contenttypes.models import ContentType @@ -151,17 +152,18 @@ def test_make_should_create_objects_respecting_quantity_parameter(self): assert all(p.name == "George Washington" for p in people) def test_make_quantity_respecting_bulk_create_parameter(self): - with self.assertNumQueries(3): + query_count = 2 if django.VERSION >= (4, 0) else 3 + with self.assertNumQueries(query_count): baker.make(models.Person, _quantity=5, _bulk_create=True) assert models.Person.objects.count() == 5 - with self.assertNumQueries(3): + with self.assertNumQueries(query_count): people = baker.make( models.Person, name="George Washington", _quantity=5, _bulk_create=True ) assert all(p.name == "George Washington" for p in people) - with self.assertNumQueries(3): + with self.assertNumQueries(query_count): baker.make(models.NonStandardManager, _quantity=3, _bulk_create=True) assert getattr(models.NonStandardManager, "objects", None) is None assert ( @@ -362,30 +364,32 @@ def test_create_multiple_one_to_one(self): assert models.Person.objects.all().count() == 5 def test_bulk_create_multiple_one_to_one(self): - with self.assertNumQueries(8): + query_count = 7 if django.VERSION >= (4, 0) else 8 + with self.assertNumQueries(query_count): baker.make(models.LonelyPerson, _quantity=5, _bulk_create=True) assert models.LonelyPerson.objects.all().count() == 5 assert models.Person.objects.all().count() == 5 def test_chaining_bulk_create_reduces_query_count(self): - qtd = 5 - with self.assertNumQueries(7): - baker.make(models.Person, _quantity=qtd, _bulk_create=True) + query_count = 5 if django.VERSION >= (4, 0) else 7 + with self.assertNumQueries(query_count): + baker.make(models.Person, _quantity=5, _bulk_create=True) person_iter = models.Person.objects.all().iterator() baker.make( models.LonelyPerson, only_friend=person_iter, - _quantity=qtd, + _quantity=5, _bulk_create=True, ) # 2 bulk create and 1 select on Person - assert models.LonelyPerson.objects.all().count() == qtd - assert models.Person.objects.all().count() == qtd + assert models.LonelyPerson.objects.all().count() == 5 + assert models.Person.objects.all().count() == 5 def test_bulk_create_multiple_fk(self): - with self.assertNumQueries(8): + query_count = 7 if django.VERSION >= (4, 0) else 8 + with self.assertNumQueries(query_count): baker.make(models.PaymentBill, _quantity=5, _bulk_create=True) assert models.PaymentBill.objects.all().count() == 5 @@ -1037,10 +1041,11 @@ def test_annotation_within_manager_get_queryset_are_run_on_make(self): class TestCreateM2MWhenBulkCreate(TestCase): @pytest.mark.django_db def test_create(self): - with self.assertNumQueries(24): + query_count = 13 if django.VERSION >= (4, 0) else 14 + with self.assertNumQueries(query_count): person = baker.make(models.Person) baker.make( - models.Classroom, students=[person], _quantity=20, _bulk_create=True + models.Classroom, students=[person], _quantity=10, _bulk_create=True ) c1, c2 = models.Classroom.objects.all()[:2] assert list(c1.students.all()) == list(c2.students.all()) == [person] From 095ace04f802eacb9c5582ec46c1ac0f9c382012 Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Sun, 16 Oct 2022 22:28:29 +0200 Subject: [PATCH 5/6] Switch to Ubuntu 22 --- .github/workflows/changelog.yml | 2 +- .github/workflows/linter.yml | 2 +- .github/workflows/release.yml | 2 +- .github/workflows/tests.yml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index 6a06232b..dfc3c77d 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -4,7 +4,7 @@ on: pull_request jobs: remind: - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 if: | !contains(github.event.pull_request.body, '[skip changelog]') && (github.actor != 'dependabot[bot]') diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index 06d22964..59e81640 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -9,7 +9,7 @@ on: jobs: tests: name: Python ${{ matrix.python-version }} - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v3 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 5bc80f92..6ce473a1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -5,7 +5,7 @@ on: [release] jobs: package: name: Build & verify package - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v3 diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 4d2294eb..bb40687d 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -9,7 +9,7 @@ on: jobs: tests: name: Python ${{ matrix.python-version }} - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 services: postgis: From 6055f1639b49d45dd421ece848b0f36b0e4e9111 Mon Sep 17 00:00:00 2001 From: Rust Saiargaliev Date: Mon, 17 Oct 2022 16:31:26 +0200 Subject: [PATCH 6/6] Improve Django version import Co-authored-by: Tim Klein --- model_bakery/baker.py | 4 ++-- tests/test_baker.py | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/model_bakery/baker.py b/model_bakery/baker.py index a5a818aa..86dd2a84 100644 --- a/model_bakery/baker.py +++ b/model_bakery/baker.py @@ -13,7 +13,7 @@ overload, ) -import django +from django import VERSION as DJANGO_VERSION from django.apps import apps from django.conf import settings from django.contrib import contenttypes @@ -792,7 +792,7 @@ def _save_related_objs(model, objects) -> None: created_entries = manager.bulk_create(entries) # bulk_create in Django < 4.0 does not return ids of created objects. # drop this after 01 Apr 2024 (Django 3.2 LTS end of life) - if django.VERSION < (4, 0): + if DJANGO_VERSION < (4, 0): created_entries = manager.exclude(pk__in=existing_entries) # set many-to-many relations from kwargs diff --git a/tests/test_baker.py b/tests/test_baker.py index 69d67890..d777d763 100644 --- a/tests/test_baker.py +++ b/tests/test_baker.py @@ -3,8 +3,8 @@ from decimal import Decimal from unittest.mock import patch -import django import pytest +from django import VERSION as DJANGO_VERSION from django.conf import settings from django.contrib.contenttypes.models import ContentType from django.db.models import Manager @@ -152,7 +152,7 @@ def test_make_should_create_objects_respecting_quantity_parameter(self): assert all(p.name == "George Washington" for p in people) def test_make_quantity_respecting_bulk_create_parameter(self): - query_count = 2 if django.VERSION >= (4, 0) else 3 + query_count = 2 if DJANGO_VERSION >= (4, 0) else 3 with self.assertNumQueries(query_count): baker.make(models.Person, _quantity=5, _bulk_create=True) assert models.Person.objects.count() == 5 @@ -364,7 +364,7 @@ def test_create_multiple_one_to_one(self): assert models.Person.objects.all().count() == 5 def test_bulk_create_multiple_one_to_one(self): - query_count = 7 if django.VERSION >= (4, 0) else 8 + query_count = 7 if DJANGO_VERSION >= (4, 0) else 8 with self.assertNumQueries(query_count): baker.make(models.LonelyPerson, _quantity=5, _bulk_create=True) @@ -372,7 +372,7 @@ def test_bulk_create_multiple_one_to_one(self): assert models.Person.objects.all().count() == 5 def test_chaining_bulk_create_reduces_query_count(self): - query_count = 5 if django.VERSION >= (4, 0) else 7 + query_count = 5 if DJANGO_VERSION >= (4, 0) else 7 with self.assertNumQueries(query_count): baker.make(models.Person, _quantity=5, _bulk_create=True) person_iter = models.Person.objects.all().iterator() @@ -388,7 +388,7 @@ def test_chaining_bulk_create_reduces_query_count(self): assert models.Person.objects.all().count() == 5 def test_bulk_create_multiple_fk(self): - query_count = 7 if django.VERSION >= (4, 0) else 8 + query_count = 7 if DJANGO_VERSION >= (4, 0) else 8 with self.assertNumQueries(query_count): baker.make(models.PaymentBill, _quantity=5, _bulk_create=True) @@ -1041,7 +1041,7 @@ def test_annotation_within_manager_get_queryset_are_run_on_make(self): class TestCreateM2MWhenBulkCreate(TestCase): @pytest.mark.django_db def test_create(self): - query_count = 13 if django.VERSION >= (4, 0) else 14 + query_count = 13 if DJANGO_VERSION >= (4, 0) else 14 with self.assertNumQueries(query_count): person = baker.make(models.Person) baker.make(