faster uploads by avoiding unneed large blob reads

news/167.bugfix

@@ -0,0 +1 @@
+Adjusted schema validation to prevent file/image loading during upload/import

plone/namedfile/field.py

@@ -10,6 +10,7 @@
 from plone.namedfile.interfaces import INamedFileField
 from plone.namedfile.interfaces import INamedImage
 from plone.namedfile.interfaces import INamedImageField
+from plone.namedfile.interfaces import INamedTyped
 from plone.namedfile.interfaces import IPluggableFileFieldValidation
 from plone.namedfile.interfaces import IPluggableImageFieldValidation
 from plone.namedfile.utils import get_contenttype
@@ -138,7 +139,17 @@ class NamedBlobFile(NamedField):
     accept = ()
 
     def validate(self, value):
-        super().validate(value, IPluggableFileFieldValidation)
+        # Bit of a hack but we avoid loading the .data into memory
+        # because schema validation checks the property exists
+        # which loads the entire file into memory for no reaon.
+        # This can slow down imports and uploads a lot.
+        # TODO: better fix might be get zope.schema to check for a decorator first 
+        # - https://stackoverflow.com/questions/16169948/check-if-something-is-an-attribute-or-decorator-in-python
+        self.schema = INamedTyped
+        try:
+            super().validate(value, IPluggableFileFieldValidation)
+        finally:
+            self.schema = INamedBlobFile
 
 
 @implementer(INamedBlobImageField)
@@ -150,4 +161,9 @@ class NamedBlobImage(NamedField):
     accept = ("image/*",)
 
     def validate(self, value):
-        super().validate(value, IPluggableImageFieldValidation)
+        # see NamedBlobFile comment
+        self.schema = INamedTyped
+        try:
+            super().validate(value, IPluggableImageFieldValidation)
+        finally:
+            self.schema = INamedBlobImage

plone/namedfile/file.py

@@ -390,7 +390,8 @@ def __init__(self, data=b"", contentType="", filename=None):
         # Allow override of the image sniffer
         if contentType:
             self.contentType = contentType
-        exif_data = get_exif(self.data)
+        with self.open("r") as fp:
+            exif_data = get_exif(fp, self.contentType, self._width, self._height)
         if exif_data is not None:
             log.debug(
                 "Image contains Exif Informations. "
@@ -428,7 +429,7 @@ def _setData(self, data):
             res = getImageInfo(firstbytes)
         contentType, self._width, self._height = res
         if contentType:
-            self.contentType = contentType
+            self.contentType = contentType        
 
     data = property(NamedBlobFile._getData, _setData)
 

plone/namedfile/interfaces.py

@@ -11,7 +11,7 @@
 HAVE_BLOBS = True
 
 
-class IFile(Interface):
+class ITyped(Interface):
 
     contentType = schema.NativeStringLine(
         title="Content Type",
@@ -21,6 +21,9 @@ class IFile(Interface):
         missing_value="",
     )
 
+
+class IFile(Interface):
+
     data = schema.Bytes(
         title="Data",
         description="The actual content of the object.",
@@ -83,12 +86,15 @@ class INamed(Interface):
 
     filename = schema.TextLine(title="Filename", required=False, default=None)
 
+class INamedTyped(INamed, ITyped):
+    """An item with a filename and contentType"""
+
 
-class INamedFile(INamed, IFile):
+class INamedFile(INamedTyped, IFile):
     """A non-BLOB file with a filename"""
 
 
-class INamedImage(INamed, IImage):
+class INamedImage(INamedTyped, IImage):
     """A non-BLOB image with a filename"""
 
 

plone/namedfile/tests/test_storable.py

@@ -18,6 +18,8 @@
 from OFS.Image import Pdata
 from plone.namedfile.file import FileChunk
 from plone.namedfile.file import NamedBlobImage
+from plone.namedfile.file import NamedBlobFile
+from plone.namedfile import field
 from plone.namedfile.testing import PLONE_NAMEDFILE_FUNCTIONAL_TESTING
 from plone.namedfile.tests import getFile
 
@@ -56,3 +58,54 @@ def test_opened_file_storable(self):
             if os.path.exists(path):
                 os.remove(path)
         self.assertEqual(303, fi.getSize())
+
+    def test_upload_no_read(self):
+        # ensure we don't read the whole file into memory
+
+        import ZODB.blob
+
+        old_open = ZODB.blob.Blob.open
+        blob_read = 0
+        blob_write = 0
+        old_read = ZODB.blob.BlobFile.read
+        read_bytes = 0
+
+        def count_open(self, mode="r"):
+            nonlocal blob_read, blob_write
+            blob_read += 1 if "r" in mode else 0
+            blob_write += 1 if "w" in mode else 0
+            return old_open(self, mode)
+
+        def count_reads(self, size=-1):
+            nonlocal read_bytes
+            res = old_read(self, size)
+            read_bytes += len(res)
+            return res
+
+        with unittest.mock.patch.object(ZODB.blob.Blob, 'open', count_open), unittest.mock.patch.object(ZODB.blob.BlobFile, 'read', count_reads):
+            data = getFile("image.jpg")
+            f = tempfile.NamedTemporaryFile(delete=False)
+            try:
+                path = f.name
+                f.write(data)
+                f.close()
+                with open(path, "rb") as f:
+                    fi = NamedBlobImage(f, filename="image.gif")
+            finally:
+                if os.path.exists(path):
+                    os.remove(path)
+            self.assertEqual(3641, fi.getSize())
+            self.assertIn('Exif', fi.exif)
+            self.assertLess(read_bytes, 3641, "Images should not need to read all data to get exif and size etc")
+            #self.assertEqual(blob_read, 1, "blob should have only been opened to get size")
+            self.assertEqual(
+                blob_write,
+                1,
+                "Slow write to blob instead of os rename. Should be only 1 on init",
+            )
+            blob_read = 0
+
+            blob_field = field.NamedBlobFile()
+            blob_field.validate(fi)
+
+            self.assertEqual(blob_read, 0, "Validation is reading the whole blob in memory")

plone/namedfile/utils/__init__.py

@@ -227,17 +227,19 @@ def getImageInfo(data):
     return content_type, width, height
 
 
-def get_exif(image):
+def get_exif(image, content_type=None, width=None, height=None):
     #
     exif_data = None
-    image_data = _ensure_data(image)
 
-    content_type, width, height = getImageInfo(image_data)
+    if None in (content_type, width, height):
+        # if we already got the image info don't read the while file into memory
+        content_type, width, height = getImageInfo(_ensure_data(image))
     if content_type in ["image/jpeg", "image/tiff"]:
         # Only this two Image Types could have Exif informations
         # see http://www.cipa.jp/std/documents/e/DC-008-2012_E.pdf
         try:
-            exif_data = piexif.load(image_data)
+            # if possible pass filename in instead to prevent reading all data into memory
+            exif_data = piexif.load(image.name if getattr(image, "name") else _ensure_data(image))
         except Exception as e:
             # TODO: determ wich error really happens
             # Should happen if data is to short --> first_bytes