Merge pull request #377 from jarekwg/bugfix/rollback-after-db-exception

Let Resource rollback if import throws exception

.travis.yml

@@ -3,6 +3,7 @@ python:
   - "2.7"
   - "3.3"
   - "3.4"
+  - "3.5"
 env:
   - DJANGO="Django>=1.5,<1.6"
   - DJANGO="Django>=1.6,<1.7"

import_export/resources.py

@@ -1,27 +1,26 @@
 from __future__ import unicode_literals
 
 import functools
-from copy import deepcopy
 import sys
+import tablib
 import traceback
+from copy import deepcopy
 
-import tablib
 from diff_match_patch import diff_match_patch
 
 from django import VERSION
-from django.utils.safestring import mark_safe
-from django.utils import six
+from django.conf import settings
+from django.db import transaction
 from django.db.models.fields import FieldDoesNotExist
 from django.db.models.query import QuerySet
 from django.db.transaction import TransactionManagementError
-from django.conf import settings
+from django.utils import six
+from django.utils.safestring import mark_safe
 
-from .results import Error, Result, RowResult
+from . import widgets
 from .fields import Field
-from import_export import widgets
-from .instance_loaders import (
-    ModelInstanceLoader,
-)
+from .instance_loaders import ModelInstanceLoader
+from .results import Error, Result, RowResult
 
 try:
     from django.db.transaction import atomic, savepoint, savepoint_rollback, savepoint_commit  # noqa
@@ -47,7 +46,7 @@
     from django.utils.datastructures import SortedDict as OrderedDict
 
 # Set default logging handler to avoid "No handler found" warnings.
-import logging
+import logging # isort:skip
 try:  # Python 2.7+
     from logging import NullHandler
 except ImportError:
@@ -375,7 +374,8 @@ def import_data(self, dataset, dry_run=False, raise_errors=False,
                     if self.skip_row(instance, original):
                         row_result.import_type = RowResult.IMPORT_TYPE_SKIP
                     else:
-                        self.save_instance(instance, real_dry_run)
+                        with transaction.atomic():
+                            self.save_instance(instance, real_dry_run)
                         self.save_m2m(instance, row, real_dry_run)
                         # Add object info to RowResult for LogEntry
                         row_result.object_repr = force_text(instance)

tests/core/tests/resources_tests.py

@@ -1,32 +1,21 @@
 from __future__ import unicode_literals
 
-from decimal import Decimal
-from datetime import date
+import tablib
 from copy import deepcopy
-from unittest import (
-    skip,
-)
+from datetime import date
+from decimal import Decimal
+from unittest import skip
 
-from django.db import models
+from django.contrib.auth.models import User
 from django.db.models import Count
 from django.db.models.fields import FieldDoesNotExist
-from django.test import (
-    skipUnlessDBFeature,
-    TestCase,
-    TransactionTestCase,
-    )
+from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature
 from django.utils.html import strip_tags
-from django.contrib.auth.models import User
-
-import tablib
 
-from import_export import resources
-from import_export import fields
-from import_export import widgets
-from import_export import results
+from import_export import fields, resources, results, widgets
 from import_export.instance_loaders import ModelInstanceLoader
 
-from core.models import Book, Author, Category, Entry, Profile, WithDefault, WithDynamicDefault
+from ..models import Author, Book, Category, Entry, Profile, WithDefault, WithDynamicDefault
 
 try:
     from django.utils.encoding import force_text
@@ -122,6 +111,12 @@ class Meta:
         exclude = ('imported', )
 
 
+class ProfileResource(resources.ModelResource):
+    class Meta:
+        model = Profile
+        exclude = ('user', )
+
+
 class WithDefaultResource(resources.ModelResource):
     class Meta:
         model = WithDefault
@@ -199,8 +194,7 @@ def test_get_instance_with_missing_field_data(self):
         dataset = tablib.Dataset(headers=['name', 'author_email', 'price'])
         dataset.append(['Some book', 'test@example.com', "10.25"])
         with self.assertRaises(KeyError) as cm:
-            instance = self.resource.get_instance(instance_loader,
-                                                  dataset.dict[0])
+            self.resource.get_instance(instance_loader, dataset.dict[0])
         self.assertEqual(u"Column 'id' not found in dataset. Available columns "
                          "are: %s" % [u'name', u'author_email', u'price'],
                          cm.exception.args[0])
@@ -359,7 +353,7 @@ class Meta:
                 fields = ('published',)
                 widgets = {
                     'published': {'format': '%d.%m.%Y'},
-                    }
+                }
 
         resource = B()
         self.book.published = date(2012, 8, 13)
@@ -503,15 +497,15 @@ def before_import(self, dataset, dry_run, **kwargs):
 
     def test_link_to_nonexistent_field(self):
         with self.assertRaises(FieldDoesNotExist) as cm:
-            class BrokenBook(resources.ModelResource):
+            class BrokenBook1(resources.ModelResource):
                 class Meta:
                     model = Book
                     fields = ('nonexistent__invalid',)
         self.assertEqual("Book.nonexistent: Book has no field named 'nonexistent'",
                          cm.exception.args[0])
 
         with self.assertRaises(FieldDoesNotExist) as cm:
-            class BrokenBook(resources.ModelResource):
+            class BrokenBook2(resources.ModelResource):
                 class Meta:
                     model = Book
                     fields = ('author__nonexistent',)
@@ -520,15 +514,15 @@ class Meta:
 
     def test_link_to_nonrelation_field(self):
         with self.assertRaises(KeyError) as cm:
-            class BrokenBook(resources.ModelResource):
+            class BrokenBook1(resources.ModelResource):
                 class Meta:
                     model = Book
                     fields = ('published__invalid',)
         self.assertEqual("Book.published is not a relation",
                          cm.exception.args[0])
 
         with self.assertRaises(KeyError) as cm:
-            class BrokenBook(resources.ModelResource):
+            class BrokenBook2(resources.ModelResource):
                 class Meta:
                     model = Book
                     fields = ('author__name__invalid',)
@@ -547,7 +541,7 @@ def field_from_django_field(self, field_name, django_field,
                 if field_name == 'published':
                     return {'sound': 'quack'}
 
-        resource = B()
+        B()
         self.assertEqual({'sound': 'quack'}, B.fields['published'])
 
     def test_readonly_annotated_field_import_and_export(self):
@@ -612,7 +606,7 @@ class Meta:
         self.assertTrue(callable(DynamicDefaultResource.fields['name'].default))
 
         resource = DynamicDefaultResource()
-        dataset = tablib.Dataset(headers=['id', 'name',])
+        dataset = tablib.Dataset(headers=['id', 'name', ])
         dataset.append([1, None])
         dataset.append([2, None])
         resource.import_data(dataset, raise_errors=False)
@@ -621,36 +615,52 @@ class Meta:
 
 
 class ModelResourceTransactionTest(TransactionTestCase):
-
-    def setUp(self):
-        self.resource = BookResource()
-
     @skipUnlessDBFeature('supports_transactions')
     def test_m2m_import_with_transactions(self):
+        resource = BookResource()
         cat1 = Category.objects.create(name='Cat 1')
         headers = ['id', 'name', 'categories']
         row = [None, 'FooBook', "%s" % cat1.pk]
         dataset = tablib.Dataset(row, headers=headers)
 
-        result = self.resource.import_data(dataset, dry_run=True,
-                                           use_transactions=True)
+        result = resource.import_data(
+            dataset, dry_run=True, use_transactions=True
+        )
 
         row_diff = result.rows[0].diff
-        fields = self.resource.get_fields()
+        fields = resource.get_fields()
 
-        id_field = self.resource.fields['id']
+        id_field = resource.fields['id']
         id_diff = row_diff[fields.index(id_field)]
         # id diff should exists because in rollbacked transaction
         # FooBook has been saved
         self.assertTrue(id_diff)
 
-        category_field = self.resource.fields['categories']
+        category_field = resource.fields['categories']
         categories_diff = row_diff[fields.index(category_field)]
         self.assertEqual(strip_tags(categories_diff), force_text(cat1.pk))
 
         # check that it is really rollbacked
         self.assertFalse(Book.objects.filter(name='FooBook'))
 
+    @skipUnlessDBFeature('supports_transactions')
+    def test_m2m_import_with_transactions_error(self):
+        resource = ProfileResource()
+        headers = ['id', 'user']
+        # 'user' is a required field, the database will raise an error.
+        row = [None, None]
+        dataset = tablib.Dataset(row, headers=headers)
+
+        result = resource.import_data(
+            dataset, dry_run=True, use_transactions=True
+        )
+
+        # Ensure the error raised by the database has been saved.
+        self.assertTrue(result.has_errors())
+
+        # Ensure the rollback has worked properly.
+        self.assertEqual(Profile.objects.count(), 0)
+
 
 class ModelResourceFactoryTest(TestCase):
 

tox.ini

@@ -1,78 +1,84 @@
 [tox]
-envlist = py27-1.5, py27-1.6, py33-1.6, py27-1.7, py33-1.7, py34-1.7, py27-1.8, py33-1.8, py34-1.8, py27-1.9, py34-1.9, py27-dev, py34-dev
+envlist =
+    py{27,33}-1.6,
+    py{27,33,34}-1.7,
+    py{27,33,34}-1.8,
+    py{27,34,35}-1.9,
+    py{27,34,35}-dev
 
 [testenv]
 commands=python {toxinidir}/tests/manage.py test core
 deps = openpyxl
 
-[testenv:py27-1.5]
-basepython = python2.7
-deps = 
-    django==1.5.3
-    {[testenv]deps}
-
 [testenv:py27-1.6]
 basepython = python2.7
-deps = 
+deps =
     django==1.6
     {[testenv]deps}
 
 [testenv:py33-1.6]
 basepython = python3.3
-deps = 
+deps =
     django==1.6.1
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
 
 [testenv:py27-1.7]
 basepython = python2.7
-deps = 
+deps =
     django==1.7.0
     {[testenv]deps}
 
 [testenv:py33-1.7]
 basepython = python3.3
-deps = 
+deps =
     django==1.7.0
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
 
 [testenv:py34-1.7]
 basepython = python3.4
-deps = 
+deps =
     django==1.7.0
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
 
 [testenv:py27-1.8]
 basepython = python2.7
-deps = 
+deps =
     django==1.8
     {[testenv]deps}
 
 [testenv:py33-1.8]
 basepython = python3.3
-deps = 
+deps =
     django==1.8
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
 
 [testenv:py34-1.8]
 basepython = python3.4
-deps = 
+deps =
     django==1.8
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
 
 [testenv:py27-1.9]
 basepython = python2.7
-deps = 
+deps =
     django==1.9
     {[testenv]deps}
 
 [testenv:py34-1.9]
 basepython = python3.4
-deps = 
+deps =
+    django==1.9
+    -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
+    {[testenv]deps}
+
+[testenv:py35-1.9]
+basepython = python3.5
+deps =
     django==1.9
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
@@ -89,3 +95,10 @@ deps =
     https://github.com/django/django/archive/master.tar.gz
     -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
     {[testenv]deps}
+
+[testenv:py35-dev]
+basepython = python3.5
+deps =
+    https://github.com/django/django/archive/master.tar.gz
+    -egit+https://github.com/kennethreitz/tablib.git#egg=tablib
+    {[testenv]deps}