Merge branch 'release-3-x' into check-manytomany-changes

docs/changelog.rst

@@ -7,18 +7,18 @@ Changelog
 Breaking changes
 ################
 
+This release makes the following changes to the API.  You may need to update your implementation to
+accommodate these changes.
+
 - Check value of ManyToManyField in skip_row() (#1271)
    - This fixes an issue where ManyToMany fields are not checked correctly in `skip_row()`.
      This means that `skip_row()` now takes `row` as a mandatory arg.
      If you have overridden `skip_row()` in your own implementation, you will need to add `row`
      as an arg.
 
-
-2.7.2 (unreleased)
-------------------
-
-- Nothing changed yet.
-
+- Use 'create' flag instead of instance.pk (#1362)
+   - ``import_export.resources.save_instance()`` now takes an additional mandatory argument: `is_create`.
+     If you have over-ridden `save_instance()` in your own code, you will need to add this new argument.
 
 2.7.1 (2021-12-23)
 ------------------

import_export/resources.py

@@ -455,18 +455,24 @@ def validate_instance(self, instance, import_validation_errors=None, validate_un
         if errors:
             raise ValidationError(errors)
 
-    def save_instance(self, instance, using_transactions=True, dry_run=False):
+    def save_instance(self, instance, is_create, using_transactions=True, dry_run=False):
         """
         Takes care of saving the object to the database.
 
         Objects can be created in bulk if ``use_bulk`` is enabled.
+
+        :param instance: The instance of the object to be persisted.
+        :param is_create: A boolean flag to indicate whether this is a new object
+        to be created, or an existing object to be updated.
+        :param using_transactions: A flag to indicate whether db transactions are used.
+        :param dry_run: A flag to indicate dry-run mode.
         """
         self.before_save_instance(instance, using_transactions, dry_run)
         if self._meta.use_bulk:
-            if instance.pk:
-                self.update_instances.append(instance)
-            else:
+            if is_create:
                 self.create_instances.append(instance)
+            else:
+                self.update_instances.append(instance)
         else:
             if not using_transactions and dry_run:
                 # we don't have transactions and we want to do a dry_run
@@ -704,7 +710,7 @@ def import_row(self, row, instance_loader, using_transactions=True, dry_run=Fals
                     row_result.import_type = RowResult.IMPORT_TYPE_SKIP
                 else:
                     self.validate_instance(instance, import_validation_errors)
-                    self.save_instance(instance, using_transactions, dry_run)
+                    self.save_instance(instance, new, using_transactions, dry_run)
                     self.save_m2m(instance, row, using_transactions, dry_run)
                     row_result.add_instance_info(instance)
                 if not skip_diff:

tests/core/migrations/0010_uuidbook.py

@@ -0,0 +1,21 @@
+# Generated by Django 2.2.7 on 2021-05-02 07:46
+import uuid
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0009_auto_20211111_0807'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='UUIDBook',
+            fields=[
+                ('id', models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)),
+                ('name', models.CharField(max_length=100, verbose_name='Book name')),
+            ],
+        ),
+    ]

tests/core/models.py

@@ -1,5 +1,6 @@
 import random
 import string
+import uuid
 
 from django.core.exceptions import ValidationError
 from django.db import models
@@ -104,3 +105,9 @@ class EBook(Book):
     """Book proxy model to have a separate admin url access and name"""
     class Meta:
         proxy = True
+
+
+class UUIDBook(models.Model):
+    """A model which uses a UUID pk (issue 1274)"""
+    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
+    name = models.CharField('Book name', max_length=100)

tests/core/tests/test_resources.py

@@ -30,6 +30,7 @@
     Person,
     Profile,
     Role,
+    UUIDBook,
     WithDefault,
     WithDynamicDefault,
     WithFloatField,
@@ -652,8 +653,8 @@ def before_save_instance(self, instance, using_transactions, dry_run):
                     self.before_save_instance_dry_run = True
                 else:
                     self.before_save_instance_dry_run = False
-            def save_instance(self, instance, using_transactions=True, dry_run=False):
-                super().save_instance(instance, using_transactions, dry_run)
+            def save_instance(self, instance, new, using_transactions=True, dry_run=False):
+                super().save_instance(instance, new, using_transactions, dry_run)
                 if dry_run:
                     self.save_instance_dry_run = True
                 else:
@@ -679,7 +680,7 @@ def after_save_instance(self, instance, using_transactions, dry_run):
     @mock.patch("core.models.Book.save")
     def test_save_instance_noop(self, mock_book):
         book = Book.objects.first()
-        self.resource.save_instance(book, using_transactions=False, dry_run=True)
+        self.resource.save_instance(book, is_create=False, using_transactions=False, dry_run=True)
         self.assertEqual(0, mock_book.call_count)
 
     @mock.patch("core.models.Book.save")
@@ -1594,10 +1595,10 @@ class Meta:
         rows = [('book_name',)] * 10
         self.dataset = tablib.Dataset(*rows, headers=['name'])
 
-    def init_update_test_data(self):
-        [Book.objects.create(name='book_name') for _ in range(10)]
-        self.assertEqual(10, Book.objects.count())
-        rows = Book.objects.all().values_list('id', 'name')
+    def init_update_test_data(self, model=Book):
+        [model.objects.create(name='book_name') for _ in range(10)]
+        self.assertEqual(10, model.objects.count())
+        rows = model.objects.all().values_list('id', 'name')
         updated_rows = [(r[0], 'UPDATED') for r in rows]
         self.dataset = tablib.Dataset(*updated_rows, headers=['id', 'name'])
 
@@ -1625,6 +1626,25 @@ class Meta:
         mock_bulk_create.assert_called_with(mock.ANY, batch_size=5)
         self.assertEqual(10, result.total_rows)
 
+    @mock.patch('core.models.UUIDBook.objects.bulk_create')
+    def test_bulk_create_uuid_model(self, mock_bulk_create):
+        """Test create of a Model which defines uuid not pk (issue #1274)"""
+        class _UUIDBookResource(resources.ModelResource):
+            class Meta:
+                model = UUIDBook
+                use_bulk = True
+                batch_size = 5
+                fields = (
+                    'id',
+                    'name',
+                )
+
+        resource = _UUIDBookResource()
+        result = resource.import_data(self.dataset)
+        self.assertEqual(2, mock_bulk_create.call_count)
+        mock_bulk_create.assert_called_with(mock.ANY, batch_size=5)
+        self.assertEqual(10, result.total_rows)
+
     @mock.patch('core.models.Book.objects.bulk_create')
     def test_bulk_create_no_batch_size(self, mock_bulk_create):
         class _BookResource(resources.ModelResource):
@@ -1933,6 +1953,33 @@ class Meta:
             self.assertEqual(e, raised_exc)
 
 
+@skipIf(django.VERSION[0] == 2 and django.VERSION[1] < 2, "bulk_update not supported in this version of django")
+class BulkUUIDBookUpdateTest(BulkTest):
+
+    def setUp(self):
+        super().setUp()
+        self.init_update_test_data(model=UUIDBook)
+
+    @mock.patch('core.models.UUIDBook.objects.bulk_update')
+    def test_bulk_update_uuid_model(self, mock_bulk_update):
+        """Test update of a Model which defines uuid not pk (issue #1274)"""
+        class _UUIDBookResource(resources.ModelResource):
+            class Meta:
+                model = UUIDBook
+                use_bulk = True
+                batch_size = 5
+                fields = (
+                    'id',
+                    'name',
+                )
+
+        resource = _UUIDBookResource()
+        result = resource.import_data(self.dataset)
+        self.assertEqual(2, mock_bulk_update.call_count)
+        self.assertEqual(10, result.total_rows)
+        self.assertEqual(10, result.totals["update"])
+
+
 class BulkDeleteTest(BulkTest):
     class DeleteBookResource(resources.ModelResource):
         def for_delete(self, row, instance):
@@ -2049,3 +2096,25 @@ class Meta:
         with self.assertRaises(Exception) as raised_exc:
             resource.import_data(self.dataset, raise_errors=True)
             self.assertEqual(e, raised_exc)
+
+
+class BulkUUIDBookDeleteTest(BulkTest):
+    class DeleteBookResource(resources.ModelResource):
+        def for_delete(self, row, instance):
+            return True
+
+        class Meta:
+            model = UUIDBook
+            use_bulk = True
+            batch_size = 5
+
+    def setUp(self):
+        super().setUp()
+        self.resource = self.DeleteBookResource()
+        self.init_update_test_data(model=UUIDBook)
+
+    def test_bulk_delete_batch_size_of_5(self):
+        self.assertEqual(10, UUIDBook.objects.count())
+        self.resource.import_data(self.dataset)
+        self.assertEqual(0, UUIDBook.objects.count())
+