replace PR #155 to avoid unneed reading files into memory during import/upload

Author: djay

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 DX 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/interfaces.py

@@ -10,8 +10,7 @@
 
 HAVE_BLOBS = True
 
-
-class IFile(Interface):
+class ITyped(Interface):
 
     contentType = schema.NativeStringLine(
         title="Content Type",
@@ -21,6 +20,8 @@ class IFile(Interface):
         missing_value="",
     )
 
+class IFile(Interface):
+
     data = schema.Bytes(
         title="Data",
         description="The actual content of the object.",
@@ -83,12 +84,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,44 @@ 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
+
+        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)
+
+        with unittest.mock.patch.object(ZODB.blob.Blob, 'open', count_open):
+            data = getFile("image.gif")
+            f = tempfile.NamedTemporaryFile(delete=False)
+            try:
+                path = f.name
+                f.write(data)
+                f.close()
+                with open(path, "rb") as f:
+                    fi = NamedBlobFile(f, filename="image.gif")
+            finally:
+                if os.path.exists(path):
+                    os.remove(path)
+            self.assertEqual(303, fi.getSize())
+            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")