ImperialCollegeLondon / django-drf-filepond

A Django app providing a server implemention for the Filepond file upload library
BSD 3-Clause "New" or "Revised" License
105 stars 40 forks source link

DJANGO_DRF_FILEPOND_FILE_STORE_PATH required #97

Closed browze closed 11 months ago

browze commented 1 year ago

It appears that if DJANGO_DRF_FILEPOND_FILE_STORE_PATH isn't set, an error is thrown by DjangoDrfFilepondConfig, which shows up in the console as follows:

django.core.exceptions.ImproperlyConfigured: You are using local file storage so you must set the base file storage path using DJANGO_DRF_FILEPOND_FILE_STORE_PATH

The documentation for this lib makes it clear that the app that integrates it can handle the movement to permanent storage itself, though I don't see anywhere in the docs, or in the codebase, a setting to indicate that intention (explicitly setting DJANGO_DRF_FILEPOND_FILE_STORE_PATH to None doesn't do that). So currently, there appears to be no way to prevent the error except to define a setting you don't intend to use (which can be done of course as a temporary work around to avoid forking over such a minor issue).

jcohen02 commented 1 year ago

I assume you are using the approch to store files as noted in "Manual handling of file storage" in the docs?

DJANGO_DRF_FILEPOND_FILE_STORE_PATH is set by default to None in drf_filepond_settings.py (L61).

However, as you may have seen, the logic in apps.py is set up to throw the error you're seeing if no alternative storage backend is set via DJANGO_DRF_FILEPOND_STORAGES_BACKEND. This seems to have slipped through since there should clearly be a way to allow you to not specify either a FILE_STORE_PATH or STORAGES_BACKEND.

Thanks for highlighting this.

I guess the issue here is that the docs also state that the two approaches to storing uploaded files (manual or using the API to store files to a specified backend) "are not mutually exclusive and you can choose to use one approach for some files and the other approach for other files if you wish.". The current set up works fine if you want to use both approaches because you'll have a storages backend or file store path set and won't see the error you're getting.

If you're doing everything manually and therefore don't want to use the store_upload, get_stored_upload, delete_stored_upload API calls, that's fine but they'll of course fail if there is any attempt to use them when you haven't set STORAGES_BACKEND or FILE_STORE_PATH.

To resolve this, there will need to be a check at the start of the implementation of these functions to ensure that either FILE_STORE_PATH or STORAGES_BACKEND is set, if not, an exception will be thrown. I could add new boolean setting, something like MANUAL_FILE_STORAGE to make the configuration more explicit but that might complicate things if users of the library using a mix of API-based or manual file storage. I'm reluctant to allow a "manual" option for STORAGES_BACKEND since this is currently intended to only accept a valid django-storages class name.

On that basis, I guess that the most simple solution here is to allow the application to be configured without either FILE_STORE_PATH or STORAGES_BACKEND set and to then check at the start of any API calls that require these settings that they are indeed present. If they're not, a call to them will result in an exception - this will prevent any unexpected behaviour from uninetentionally calling these functions if you're intending to manually handle file permanent file storage.

If you have any thoughts/suggestions on improvements/alternatives to this approach, let me know, otherwise I'll go ahead and implement this.

browze commented 1 year ago

Hi @jcohen02,

I assume you are using the approch to store files as noted in "Manual handling of file storage" in the docs

Yes

I guess that the most simple solution here is to allow the application to be configured without either FILE_STORE_PATH or STORAGES_BACKEND set and to then check at the start of any API calls that require these settings that they are indeed present

Yes, that sounds like the best bet to me. I think most users will probably realise that they'll need to define those settings if they intend to use the storage API and your docs make that quite clear too - so I'd say it's reasonable to only catch it in the 'breach' as it were - at run time. As you also said, adopters of this lib might combine both 'manual' and 'API' usage, so otherwise the setting would need to be something like 'MANUAL_FILE_STORAGE_ONLY' to be unambiguous, which would seem a bit contrived. That indicates to me that the check fits most naturally at an API 'function' specific level.

Thanks!

jcohen02 commented 1 year ago

Hi @browze, thanks for the feedback. I'm working on some updates to the library at present so I'll get this fixed as soon as possible and included in the next release. Thanks for raising this issue.

jcohen02 commented 11 months ago

Closing as fixed in #107 and scheduled for release in upcoming v0.6.0. Thanks for reporting this @browze.