Repository: codingjoe/django-pictures (language: python)

Issue:
djanog-pictures not compatible wiht django-storages
I finally did test `django-pictures` with `django-storages` (using s3) and they are not compatible.
When creating or updating an image I do get `The file cannot be reopened.`

I *think*  this is due to https://github.com/codingjoe/django-pictures/blob/main/pictures/tasks.py#L15 trying to open the (closed) file again. From my understanding `django-storages` is using in-memory for file processing, which does not allow it to be reopened.

I also looked at `django-stdimage` and this implementation is opening the file from storage:
https://github.com/codingjoe/django-stdimage/blob/4c7194a88fc970cb0d910f444a2de29911d4a630/stdimage/models.py#L72

❓ Maybe adopting this would fix this? 
❓ Or not closing it in the first place before processing finished?
djanog-pictures not compatible wiht django-storages
I finally did test `django-pictures` with `django-storages` (using s3) and they are not compatible.
When creating or updating an image I do get `The file cannot be reopened.`

I *think*  this is due to https://github.com/codingjoe/django-pictures/blob/main/pictures/tasks.py#L15 trying to open the (closed) file again. From my understanding `django-storages` is using in-memory for file processing, which does not allow it to be reopened.

I also looked at `django-stdimage` and this implementation is opening the file from storage:
https://github.com/codingjoe/django-stdimage/blob/4c7194a88fc970cb0d910f444a2de29911d4a630/stdimage/models.py#L72

❓ Maybe adopting this would fix this? 
❓ Or not closing it in the first place before processing finished?


Discussion / hints:
@vchrisb interesting. I have been using the async methods in a life system only. I believe we should still try to open the files, but handle the exception. I use a custom storage form `django-s3file` that doesn't touch files during the upload, therefore the would be closed.


Do you want to do the honors of adding a try bock around this?
From my understanding it is not possible to open the files again when they are in-memory representations. 
The files are already closed somewhere else, hence it will fail in anyway.
What about:
```python
def _process_picture(field_file: PictureFieldFile) -> None:
    with field_file.storage.open(field_file.name) as file:
        with Image.open(file) as img:
            for ratio, sources in field_file.aspect_ratios.items():
                for file_type, srcset in sources.items():
                    for width, picture in srcset.items():
                        picture.save(img)
```
If they are open or in memory, we can just read from them? Loading it into memory twice might not to be so memory efficient.
So I did dig into it a bit more. Apparently S3boto is calling `close` on the file after upload: https://github.com/boto/boto3/issues/3150
Another try. 😁 
```python
    # TemporaryUploadedFile can't be reopened
    if field_file.closed and getattr(field_file, "_file", None) is not None and isinstance(field_file._file, TemporaryUploadedFile):
        field_file._file = None

    with field_file.open() as file:
        with Image.open(file) as img:
            for ratio, sources in field_file.aspect_ratios.items():
                for file_type, srcset in sources.items():
                    for width, picture in srcset.items():
                        picture.save(img)                        
```

If `_file` is `None`, file will be reload from storage: https://github.com/django/django/blob/0dd29209091280ccf34e07c9468746c396b7778e/django/db/models/fields/files.py#L77
That looks like a lot of ifs. Maybe we just try to open the file, catch the exception and open it from storage if need be. Maybe that's more EAFP?
I further looked into it:
When uploading a file, it is a subclass of `UploadedFile` https://github.com/django/django/blob/stable/4.0.x/django/core/files/uploadedfile.py#L21.

boto3 is calling (unwanted) `close()` on this `UploadedFile` https://github.com/boto/boto3/issues/3150, which gets deleted (either from memory or tmp disk)

With `_process_picture` being called in the upload request, `_file` is still an `UploadedFile`, but due to boto3 it is closed.
Therefore it can't be reopened.

`django-storages` actually introduced an adapted storage class (https://github.com/jschneier/django-storages/blob/master/storages/backends/s3boto3.py#L612) when using `ManifestFilesMixin`, which is copying the temporary file before passing it to s3boto. https://github.com/jschneier/django-storages/issues/382

Long story short, creating a custom `S3Boto3Storage` class like the following, does handle the issue:
```
class MediaStorage(S3Boto3Storage):
    def _save(self, name, content):
        content.seek(0)
        with tempfile.SpooledTemporaryFile() as tmp:
            tmp.write(content.read())
            return super()._save(name, tmp)
```

Besides that, I think `_process_picture` should be updated to use `with field_file.open() as file:`, so the file gets closed at the end?
OK, you seem to have put a lot of research into this. I don't know if a custom storage is feasible for most. I would probably default to open the file from storage (always) and of course close it at the end as you suggested.
I think for `django-storages` it is not too uncommon to create a storage class: https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html#overriding-the-default-storage-class
It is e.g. necessary for configuring different buckets for static and media.
With `_process_picture` being called in the upload request I share your opinion, that loading the file again from storage isn't very efficient and makes the request longer than necessary.

With that said, I can add a section to the README for `django-storages` and also update `_process_picture` to use `with field_file.open() as file:`.
Hm… yes, but it's still going to raise many questions aka tickets. Besides, we'd leave all files open, even if they are not post processesed, this seems wrong. I believe storages is doing the right thing. Opening a file from storage seems more plausible. It needs to be done in all async cases anyway. So it would be a more consistent behavior across all implementations.
If someone is using django-storages with ManifestFilesMixin, they will also get a similar exception and need to look at the docs.
I also checked that the file gets closed anyway. So the `with` clause is actually not needed in the sync task.

Anyway, I'm happy with adapting the storage class and you might want to merge the addition to the Readme. ;) 
Hi @vchrisb,

I understand how that works for you. But you also seem like a very talented software engineer. That aside, I don't really know what side effects no closing the files might have on other packages. Soooo, I would be more reluctant to have this package work regardless of the storage implementation and without interfering with other 3rd party code.

I am happy to do the changes myself, but I feel like you deserve the credit here.

Best Joe
I'm just good at googling. You are the engineer who created this great package! :) 
I still feel we are not on the same page yet. There are two distinct things:

1. Closing the file in the sync task is not necessary. From my testing, close will be called somewhere later on the file. I could try to dig out where.

2. Only for django-storages with s3boto, it is necessary to modify the storage class. From my point of view this acceptable, because this is a known limitation of s3boto in django-storages. Hence there is a specific storage class when using `ManifestStaticFilesStorage`. I was thinking to do a PR to django-storages to add another storage class similar to `storages.backends.s3boto3.S3ManifestStaticStorage`.
@vchrisb interesting. I have been using the async methods in a life system only. I believe we should still try to open the files, but handle the exception. I use a custom storage form `django-s3file` that doesn't touch files during the upload, therefore the would be closed.


Do you want to do the honors of adding a try bock around this?
From my understanding it is not possible to open the files again when they are in-memory representations. 
The files are already closed somewhere else, hence it will fail in anyway.
What about:
```python
def _process_picture(field_file: PictureFieldFile) -> None:
    with field_file.storage.open(field_file.name) as file:
        with Image.open(file) as img:
            for ratio, sources in field_file.aspect_ratios.items():
                for file_type, srcset in sources.items():
                    for width, picture in srcset.items():
                        picture.save(img)
```
If they are open or in memory, we can just read from them? Loading it into memory twice might not to be so memory efficient.
So I did dig into it a bit more. Apparently S3boto is calling `close` on the file after upload: https://github.com/boto/boto3/issues/3150
Another try. 😁 
```python
    # TemporaryUploadedFile can't be reopened
    if field_file.closed and getattr(field_file, "_file", None) is not None and isinstance(field_file._file, TemporaryUploadedFile):
        field_file._file = None

    with field_file.open() as file:
        with Image.open(file) as img:
            for ratio, sources in field_file.aspect_ratios.items():
                for file_type, srcset in sources.items():
                    for width, picture in srcset.items():
                        picture.save(img)                        
```

If `_file` is `None`, file will be reload from storage: https://github.com/django/django/blob/0dd29209091280ccf34e07c9468746c396b7778e/django/db/models/fields/files.py#L77
That looks like a lot of ifs. Maybe we just try to open the file, catch the exception and open it from storage if need be. Maybe that's more EAFP?
I further looked into it:
When uploading a file, it is a subclass of `UploadedFile` https://github.com/django/django/blob/stable/4.0.x/django/core/files/uploadedfile.py#L21.

boto3 is calling (unwanted) `close()` on this `UploadedFile` https://github.com/boto/boto3/issues/3150, which gets deleted (either from memory or tmp disk)

With `_process_picture` being called in the upload request, `_file` is still an `UploadedFile`, but due to boto3 it is closed.
Therefore it can't be reopened.

`django-storages` actually introduced an adapted storage class (https://github.com/jschneier/django-storages/blob/master/storages/backends/s3boto3.py#L612) when using `ManifestFilesMixin`, which is copying the temporary file before passing it to s3boto. https://github.com/jschneier/django-storages/issues/382

Long story short, creating a custom `S3Boto3Storage` class like the following, does handle the issue:
```
class MediaStorage(S3Boto3Storage):
    def _save(self, name, content):
        content.seek(0)
        with tempfile.SpooledTemporaryFile() as tmp:
            tmp.write(content.read())
            return super()._save(name, tmp)
```

Besides that, I think `_process_picture` should be updated to use `with field_file.open() as file:`, so the file gets closed at the end?
OK, you seem to have put a lot of research into this. I don't know if a custom storage is feasible for most. I would probably default to open the file from storage (always) and of course close it at the end as you suggested.
I think for `django-storages` it is not too uncommon to create a storage class: https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html#overriding-the-default-storage-class
It is e.g. necessary for configuring different buckets for static and media.
With `_process_picture` being called in the upload request I share your opinion, that loading the file again from storage isn't very efficient and makes the request longer than necessary.

With that said, I can add a section to the README for `django-storages` and also update `_process_picture` to use `with field_file.open() as file:`.
Hm… yes, but it's still going to raise many questions aka tickets. Besides, we'd leave all files open, even if they are not post processesed, this seems wrong. I believe storages is doing the right thing. Opening a file from storage seems more plausible. It needs to be done in all async cases anyway. So it would be a more consistent behavior across all implementations.
If someone is using django-storages with ManifestFilesMixin, they will also get a similar exception and need to look at the docs.
I also checked that the file gets closed anyway. So the `with` clause is actually not needed in the sync task.

Anyway, I'm happy with adapting the storage class and you might want to merge the addition to the Readme. ;) 
Hi @vchrisb,

I understand how that works for you. But you also seem like a very talented software engineer. That aside, I don't really know what side effects no closing the files might have on other packages. Soooo, I would be more reluctant to have this package work regardless of the storage implementation and without interfering with other 3rd party code.

I am happy to do the changes myself, but I feel like you deserve the credit here.

Best Joe
I'm just good at googling. You are the engineer who created this great package! :) 
I still feel we are not on the same page yet. There are two distinct things:

1. Closing the file in the sync task is not necessary. From my testing, close will be called somewhere later on the file. I could try to dig out where.

2. Only for django-storages with s3boto, it is necessary to modify the storage class. From my point of view this acceptable, because this is a known limitation of s3boto in django-storages. Hence there is a specific storage class when using `ManifestStaticFilesStorage`. I was thinking to do a PR to django-storages to add another storage class similar to `storages.backends.s3boto3.S3ManifestStaticStorage`.

Fix patch:
diff --git a/pictures/tasks.py b/pictures/tasks.py
index 8d9fba2..70e2fb3 100644
--- a/pictures/tasks.py
+++ b/pictures/tasks.py
@@ -12,12 +12,14 @@
 
 
 def _process_picture(field_file: PictureFieldFile) -> None:
-    field_file.open()  # the file needs to be open
-    with Image.open(field_file.file) as img:
-        for ratio, sources in field_file.aspect_ratios.items():
-            for file_type, srcset in sources.items():
-                for width, picture in srcset.items():
-                    picture.save(img)
+    # field_file.file may already be closed and can't be reopened.
+    # Therefore, we always open it from storage.
+    with field_file.storage.open(field_file.name) as fs:
+        with Image.open(fs) as img:
+            for ratio, sources in field_file.aspect_ratios.items():
+                for file_type, srcset in sources.items():
+                    for width, picture in srcset.items():
+                        picture.save(img)
 
 
 process_picture = _process_picture


Test patch:
diff --git a/tests/test_tasks.py b/tests/test_tasks.py
new file mode 100644
index 0000000..fc4b13a
--- /dev/null
+++ b/tests/test_tasks.py
@@ -0,0 +1,18 @@
+from unittest.mock import Mock
+
+import pytest
+
+from pictures import tasks
+from tests.testapp.models import SimpleModel
+
+
+@pytest.mark.django_db
+def test_process_picture__file_cannot_be_reopened(image_upload_file):
+    # regression https://github.com/codingjoe/django-pictures/issues/26
+    obj = SimpleModel.objects.create(picture=image_upload_file)
+    setattr(
+        obj.picture.file,
+        "open",
+        Mock(side_effect=ValueError("The file cannot be reopened.")),
+    )
+    tasks._process_picture(obj.picture)