Merge 0921105 into e7b6318

docs/changelog.rst

@@ -10,15 +10,17 @@ 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.
+    - 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.
+
+- Refactor admin import to include encoding param (#1306)
+    - Admin user interface: If an exception is thrown when attempting to read a file then this error is presented as a form error, instead of being written directly back in the response HTML.  If you have any code or process which checks the HTML response for the error (i.e. wrapped in H1 HTML tags) then this will need to be updated to handle the errors which are now returned as form errors.
+    - This change also refactors the `tmp_storages` interface.  If you have made any changes which call the `tmp_storages` interface, then these will need to be updated.
 
 - 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.
+    - ``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.
 
 - Add support for multiple resources in ModelAdmin. (#1223)
-
    - The `*Mixin.resource_class` accepting single resource has been deprecated (will work for few next versions) and the new `*Mixin.resource_classes` accepting subscriptable type (list, tuple, ...) has been added.
-
    - Same applies to all of the `get_resource_class`, `get_import_resource_class` and `get_export_resource_class` methods.
 
 - Deprecated `exceptions.py` - this module will be removed in a future release (#1372)
@@ -27,6 +29,7 @@ Enhancements
 ############
 
 - Updated import.css to support dark mode (#1370)
+- Admin site: Refactored reading bytes from temporary storage to address decoding issues (#1306)
 
 Development
 ###########

docs/getting_started.rst

@@ -340,10 +340,14 @@ mixins (:class:`~import_export.admin.ImportMixin`,
 
     admin.site.register(Book, BookAdmin)
 
+.. _change-screen-figure:
+
 .. figure:: _static/images/django-import-export-change.png
 
    A screenshot of the change view with Import and Export buttons.
 
+.. _confirm-import-figure:
+
 .. figure:: _static/images/django-import-export-import.png
 
    A screenshot of the import view.
@@ -478,6 +482,29 @@ as well as importing customizations.
     :doc:`/api_admin`
         available mixins and options.
 
+Import confirmation
+~~~~~~~~~~~~~~~~~~~
+
+Importing in the Admin site is a two step process.
+
+#. Choose the file to import (:ref:`screenshot<change-screen-figure>`).
+#. Review changes and confirm import (:ref:`screenshot<confirm-import-figure>`).
+
+To support this, uploaded data is written to temporary storage after step 1, and read
+back for final import after step 2.
+
+There are three mechanisms for temporary storage.
+
+#. Temporary file storage on the host server (default).  This is suitable for development only.
+   Use of temporary filesystem storage is not recommended for production sites.
+
+#. The `Django cache <https://docs.djangoproject.com/en/dev/topics/cache/>`_.
+
+#. `Django storage <https://docs.djangoproject.com/en/dev/ref/files/storage/>`_.
+
+To modify which storage mechanism is used, please refer to :ref:`IMPORT_EXPORT_TMP_STORAGE_CLASS`.
+
+
 Using multiple resources
 ------------------------
 

docs/installation.rst

@@ -65,6 +65,8 @@ of losing an audit trail.
 Can be overridden on a ``ModelAdmin`` class inheriting from ``ImportMixin`` by
 setting the ``skip_admin_log`` class attribute.
 
+.. _IMPORT_EXPORT_TMP_STORAGE_CLASS:
+
 ``IMPORT_EXPORT_TMP_STORAGE_CLASS``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

import_export/admin.py

@@ -10,7 +10,6 @@
 from django.template.response import TemplateResponse
 from django.urls import path, reverse
 from django.utils.decorators import method_decorator
-from django.utils.encoding import force_str
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
 from django.views.decorators.http import require_POST
@@ -19,7 +18,7 @@
 from .mixins import BaseExportMixin, BaseImportMixin
 from .results import RowResult
 from .signals import post_export, post_import
-from .tmp_storages import TempFolderStorage
+from .tmp_storages import MediaStorage, TempFolderStorage
 
 
 class ImportExportMixinBase:
@@ -41,7 +40,7 @@ class ImportMixin(BaseImportMixin, ImportExportMixinBase):
     #: template for import view
     import_template_name = 'admin/import_export/import.html'
     #: import data encoding
-    from_encoding = "utf-8"
+    from_encoding = "utf-8-sig"
     skip_admin_log = None
     # storage class for saving temporary files
     tmp_storage_class = None
@@ -103,13 +102,15 @@ def process_import(self, request, *args, **kwargs):
             import_formats = self.get_import_formats()
             input_format = import_formats[
                 int(confirm_form.cleaned_data['input_format'])
-            ]()
-            tmp_storage = self.get_tmp_storage_class()(name=confirm_form.cleaned_data['import_file_name'])
-            data = tmp_storage.read(input_format.get_read_mode())
-            if not input_format.is_binary() and self.from_encoding:
-                data = force_str(data, self.from_encoding)
-            dataset = input_format.create_dataset(data)
+            ](encoding=self.from_encoding)
+            encoding = None if input_format.is_binary() else self.from_encoding
+            tmp_storage = self.get_tmp_storage_class()(
+                name=confirm_form.cleaned_data['import_file_name'],
+                encoding=encoding
+            )
 
+            data = tmp_storage.read()
+            dataset = input_format.create_dataset(data)
             result = self.process_dataset(dataset, confirm_form, request, *args, **kwargs)
 
             tmp_storage.remove()
@@ -214,12 +215,22 @@ def get_import_data_kwargs(self, request, *args, **kwargs):
         return {}
 
     def write_to_tmp_storage(self, import_file, input_format):
-        tmp_storage = self.get_tmp_storage_class()()
+        encoding = None
+        read_mode = 'rb'
+        if not input_format.is_binary():
+            encoding = self.from_encoding
+            read_mode = 'r'
+
+        tmp_storage_cls = self.get_tmp_storage_class()
+        if tmp_storage_cls == MediaStorage:
+            # files are always persisted to MediaStorage as binary
+            read_mode = 'rb'
+        tmp_storage = tmp_storage_cls(encoding=encoding, read_mode=read_mode)
         data = bytes()
         for chunk in import_file.chunks():
             data += chunk
 
-        tmp_storage.save(data, input_format.get_read_mode())
+        tmp_storage.save(data)
         return tmp_storage
 
     def import_action(self, request, *args, **kwargs):
@@ -243,52 +254,56 @@ def import_action(self, request, *args, **kwargs):
                          request.FILES or None,
                          **form_kwargs)
 
+        resources = list()
         if request.POST and form.is_valid():
             input_format = import_formats[
                 int(form.cleaned_data['input_format'])
             ]()
+            if not input_format.is_binary():
+                input_format.encoding = self.from_encoding
+
             import_file = form.cleaned_data['import_file']
             # first always write the uploaded file to disk as it may be a
             # memory file or else based on settings upload handlers
             tmp_storage = self.write_to_tmp_storage(import_file, input_format)
 
-            # then read the file, using the proper format-specific mode
-            # warning, big files may exceed memory
             try:
-                data = tmp_storage.read(input_format.get_read_mode())
-                if not input_format.is_binary() and self.from_encoding:
-                    data = force_str(data, self.from_encoding)
+                # then read the file, using the proper format-specific mode
+                # warning, big files may exceed memory
+                data = tmp_storage.read()
                 dataset = input_format.create_dataset(data)
-            except UnicodeDecodeError as e:
-                return HttpResponse(_(u"<h1>Imported file has a wrong encoding: %s</h1>" % e))
             except Exception as e:
-                return HttpResponse(_(u"<h1>%s encountered while trying to read file: %s</h1>" % (type(e).__name__, import_file.name)))
-
-            # prepare kwargs for import data, if needed
-            res_kwargs = self.get_import_resource_kwargs(request, form=form, *args, **kwargs)
-            resource = self.choose_import_resource_class(form)(**res_kwargs)
-            resources = [resource]
-
-            # prepare additional kwargs for import_data, if needed
-            imp_kwargs = self.get_import_data_kwargs(request, form=form, *args, **kwargs)
-            result = resource.import_data(dataset, dry_run=True,
-                                          raise_errors=False,
-                                          file_name=import_file.name,
-                                          user=request.user,
-                                          **imp_kwargs)
-
-            context['result'] = result
-
-            if not result.has_errors() and not result.has_validation_errors():
-                initial = {
-                    'import_file_name': tmp_storage.name,
-                    'original_file_name': import_file.name,
-                    'input_format': form.cleaned_data['input_format'],
-                    'resource': request.POST.get('resource', ''),
-                }
-                confirm_form = self.get_confirm_import_form()
-                initial = self.get_form_kwargs(form=form, **initial)
-                context['confirm_form'] = confirm_form(initial=initial)
+                form.add_error('import_file',
+                               _(f"'{type(e).__name__}' encountered while trying to read file. "
+                                 "Ensure you have chosen the correct format for the file. "
+                                 f"{str(e)}"))
+
+            if not form.errors:
+                # prepare kwargs for import data, if needed
+                res_kwargs = self.get_import_resource_kwargs(request, form=form, *args, **kwargs)
+                resource = self.choose_import_resource_class(form)(**res_kwargs)
+                resources = [resource]
+
+                # prepare additional kwargs for import_data, if needed
+                imp_kwargs = self.get_import_data_kwargs(request, form=form, *args, **kwargs)
+                result = resource.import_data(dataset, dry_run=True,
+                                              raise_errors=False,
+                                              file_name=import_file.name,
+                                              user=request.user,
+                                              **imp_kwargs)
+
+                context['result'] = result
+
+                if not result.has_errors() and not result.has_validation_errors():
+                    initial = {
+                        'import_file_name': tmp_storage.name,
+                        'original_file_name': import_file.name,
+                        'input_format': form.cleaned_data['input_format'],
+                        'resource': request.POST.get('resource', ''),
+                    }
+                    confirm_form = self.get_confirm_import_form()
+                    initial = self.get_form_kwargs(form=form, **initial)
+                    context['confirm_form'] = confirm_form(initial=initial)
         else:
             res_kwargs = self.get_import_resource_kwargs(request, form=form, *args, **kwargs)
             resource_classes = self.get_import_resource_classes()

import_export/formats/base_formats.py

@@ -56,6 +56,9 @@ class TablibFormat(Format):
     TABLIB_MODULE = None
     CONTENT_TYPE = 'application/octet-stream'
 
+    def __init__(self, encoding=None):
+        self.encoding = encoding
+
     def get_format(self):
         """
         Import and returns tablib module.
@@ -96,6 +99,12 @@ def can_export(self):
 
 
 class TextFormat(TablibFormat):
+
+    def create_dataset(self, in_stream, **kwargs):
+        if isinstance(in_stream, bytes) and self.encoding:
+            in_stream = in_stream.decode(self.encoding)
+        return super().create_dataset(in_stream, **kwargs)
+
     def get_read_mode(self):
         return 'r'
 
@@ -107,9 +116,6 @@ class CSV(TextFormat):
     TABLIB_MODULE = 'tablib.formats._csv'
     CONTENT_TYPE = 'text/csv'
 
-    def create_dataset(self, in_stream, **kwargs):
-        return super().create_dataset(in_stream, **kwargs)
-
 
 class JSON(TextFormat):
     TABLIB_MODULE = 'tablib.formats._json'

import_export/tmp_storages.py

@@ -9,13 +9,15 @@
 
 class BaseStorage:
 
-    def __init__(self, name=None):
+    def __init__(self, name=None, read_mode='r', encoding=None):
         self.name = name
+        self.read_mode = read_mode
+        self.encoding = encoding
 
-    def save(self, data, mode='w'):
+    def save(self, data):
         raise NotImplementedError
 
-    def read(self, read_mode='r'):
+    def read(self):
         raise NotImplementedError
 
     def remove(self):
@@ -24,20 +26,12 @@ def remove(self):
 
 class TempFolderStorage(BaseStorage):
 
-    def open(self, mode='r'):
-        if self.name:
-            return open(self.get_full_path(), mode)
-        else:
-            tmp_file = tempfile.NamedTemporaryFile(delete=False)
-            self.name = tmp_file.name
-            return tmp_file
-
-    def save(self, data, mode='w'):
-        with self.open(mode=mode) as file:
+    def save(self, data):
+        with self._open(mode='w') as file:
             file.write(data)
 
-    def read(self, mode='r'):
-        with self.open(mode=mode) as file:
+    def read(self):
+        with self._open(mode=self.read_mode) as file:
             return file.read()
 
     def remove(self):
@@ -49,6 +43,14 @@ def get_full_path(self):
             self.name
         )
 
+    def _open(self, mode='r'):
+        if self.name:
+            return open(self.get_full_path(), mode, encoding=self.encoding)
+        else:
+            tmp_file = tempfile.NamedTemporaryFile(delete=False)
+            self.name = tmp_file.name
+            return tmp_file
+
 
 class CacheStorage(BaseStorage):
     """
@@ -57,12 +59,12 @@ class CacheStorage(BaseStorage):
     CACHE_LIFETIME = 86400
     CACHE_PREFIX = 'django-import-export-'
 
-    def save(self, data, mode=None):
+    def save(self, data):
         if not self.name:
             self.name = uuid4().hex
         cache.set(self.CACHE_PREFIX + self.name, data, self.CACHE_LIFETIME)
 
-    def read(self, read_mode='r'):
+    def read(self):
         return cache.get(self.CACHE_PREFIX + self.name)
 
     def remove(self):
@@ -72,13 +74,16 @@ def remove(self):
 class MediaStorage(BaseStorage):
     MEDIA_FOLDER = 'django-import-export'
 
-    def save(self, data, mode=None):
+    def __init__(self, name=None, read_mode='rb', encoding=None):
+        super().__init__(name, read_mode=read_mode, encoding=encoding)
+
+    def save(self, data):
         if not self.name:
             self.name = uuid4().hex
         default_storage.save(self.get_full_path(), ContentFile(data))
 
-    def read(self, read_mode='rb'):
-        with default_storage.open(self.get_full_path(), mode=read_mode) as f:
+    def read(self):
+        with default_storage.open(self.get_full_path(), mode=self.read_mode) as f:
             return f.read()
 
     def remove(self):

tests/core/exports/books-ISO-8859-1.csv

@@ -0,0 +1,2 @@
+id,name,author_email
+1,Merci � toi,test@example.com