search

jrief / django-formset

The missing widgets and form manipulation library for Django
https://django-formset.fly.dev/
MIT License
317 stars 30 forks source link

Upload not compatible with generic django storage class #99

Closed tissieres closed 9 months ago

tissieres commented 9 months ago

Hi @jrief

In upload.py, the base_location of the default_storage is accessed: https://github.com/jrief/django-formset/blob/38d20ca75daa915584c13fd673e4554f8b3ba44d/formset/upload.py#L13

This attribute is only valid for the FileSystemStorage class but is not compatible with the generic django storage class defined here. An exception is raised when using django-formset with a S3 storage for example.

Thanks!

jrief commented 9 months ago

Unfortunately I do not have any configuration using S3. Can you please point me onto some documentation where I can emulate the S3_BOTO behaviour for my unit tests. I don't want to use AWS in my unit tests.

How would you fix that?

tissieres commented 9 months ago

For example I'm using django-minio-storage and their implementation of the Storage class is defined here. I would adapt your implementation to use the file storage API described in the docs instead of relying on Pathlib.

https://github.com/jrief/django-formset/blob/38d20ca75daa915584c13fd673e4554f8b3ba44d/formset/upload.py#L109-L118 This part could probably simplified by using the File storage API instead of doing these operations by yourself on the filesystem. Your unit test would only use this API as well and it would make your package compatible with any storage implementation. What do you think?

jrief commented 9 months ago

Could you please check with HEAD from main branch.

I did not try django-minio-storage or any other storage class. However I now use the proper file storage API. Thanks for reporting.

Would you make the storage class for this file upload configurable, or just keep the current default_storage?

tissieres commented 9 months ago

It's working with django-minio-storage, but I dot not explicitely use the file upload feature so far. That was the import that was failing for me.

I never use multiple storage backends so no need for it to be configurable for me.

Thank you very much for your fast fix :+1:

tuky commented 2 months ago

Would you make the storage class for this file upload configurable, or just keep the current default_storage?

May I chip in on this older topic: I would highly propose to use the storage, that was configured to be used with the respective model field. We are in a multi bucket environment with the default media bucket configured to be publicly accessible and with user data buckets containing private data. It would be very unfortunate to temporarily save private files to the default public bucket. at least we can make the upload_temp folder private, but this is not documented and nobody guarantees the folder will always be named upload_temp. Alternatively I would also welcome the solution of django-import-export with their customizable storage for temp files (confirmation step IMPORT_EXPORT_TMP_STORAGE_CLASS): https://django-import-export.readthedocs.io/en/latest/admin_integration.html#import-confirmation / https://django-import-export.readthedocs.io/en/latest/api_tmp_storages.html. We are using that one in conjunction with a cache storage which seems more fitting than S3 for such temp data. Thank you!

jrief commented 2 months ago

Hi Tobias, I'm perfectly fine having a configurable storage for the temporary upload folder. If you can transfer the logic from django-import-export to django-formset, I will merge this into the current project.