klis87 / django-cloudinary-storage

Django package that provides Cloudinary storages for both media and static files as well as management commands for removing unnecessary files.
MIT License
130 stars 26 forks source link

override `use_filename` #8

Closed dbinetti closed 7 years ago

dbinetti commented 7 years ago

Is there a particular reason that you are going with use_filename=True here: https://github.com/klis87/django-cloudinary-storage/blob/master/cloudinary_storage/storage.py#L60

I'm trying to access the latest version through a canonical form that will ease discovery; I can always fork the repo and flip that bit but was wondering if there's something I've missed.

dbinetti commented 7 years ago

so I should change the title to "allow unique_filename=False`, which actually accomplishes my goal. but in general is there a way to pass optional params to the upload function?

klis87 commented 7 years ago

use_filename=True was implemented because I wanted model files to be based on true file names. I strongly advise against using allow unique_filename=False. Just imagine what could happen if someone uploaded a new file with the same name. Cloudinary caches all files for 30 days, that's why unique url is so important. You could upload with invalidate=True, but I find it a little not reliable as sometimes cache wasn't invalidated for a really long time. Secondly, usually users would like to bypass the cache immediately after a new file is uploaded, only unique filenames can achieve that. For the same reason StaticCloudinaryStorage is discourged and StaticHashedCloudinaryStorage is recommended, because the latter adds hashes to all static files based on their content.

In general you cannot pass optional params to upload function, because I cannot see use case for this. And if we had some use case, I think easier for users would be to provide a new settings for this package. But I could consider it for a valid reason.

Please let me know whether current defaults you mentioned in this issue are acceptable for you, I refer to your problem:

I'm trying to access the latest version through a canonical form that will ease discovery

dbinetti commented 7 years ago

First, thanks for such a useful library.

Second, thanks for being responsive to my concerns!

Third, with respect, developers will want to do what they want to do, and so deciding for them what is a bad idea is IMHO counter-productive. They will simply fork to accomplish their goal (see #1 -- thank you for even making that possible! :-).

With that, let me explain my use case more fully. I am producing files server-side that follow a particular naming convention that is based in part on the resource's name and PK (via a class passed to upload_to). The users download them and then use them for importing into other programs. With consistency naming the users can then just hit the endpoint directly without needing to know anything other than the meta data of the resource itself. Obviously this form of discovery will not be possible when random characters are appended to the file.

Secondly, files in cloudinary are not overwritten and aren't garbage-collected using the deleteorphanmedia command. Perhaps this is just a bug, but the net effect is that I get a lot of cruft that not only increases storage costs but makes management difficult.

When using the native CloudinaryField from pycloudinary, all this was handled. I could rename the file, which would overwrite the reference to the latest version (through the public_id) and then keep versioned copies of prior uploads. I am not sure how cacheing would impact this, but it seemed to work flawlessly for me. However, I couldn't use the native FileField tools via Django and found your approach compelling. But I need to consider the tradeoffs and might need to go back.

What might be happening here is a disconnect between using a filename v. using a public_id. The public_id seems to abstract some of these problems away, leaving the management to cloudinary (which is good that's what I pay them for). Does that make sense?

klis87 commented 7 years ago

Thank you for kind words @dbinetti !

Actually forced use of CloudinaryField by pycloudinary was one of the reason this package was created :)

Your use case of course makes sense, so I think I could adjust upload so it could be possible to pass optional option dictionary which would extend default options. So if someone wants to have custom behaviour, one could subclass MediaCloudinaryStorage to pass options like allow unique_filename=False . Do you think this could be a good way to do it? Something like:

def _upload(self, name, content, custom_options=None):
    options = {'use_filename': True, 'resource_type': self._get_resource_type(name), 'tags': self.TAG}
    if custom_options is not None:
        options.update(custom_options)
    ...

Regarding deleteorphanmedia, it really should work. It should get a list of all images attached to objects in db and remove the rest. If it does not work for you, could you please raise an issue?

dbinetti commented 7 years ago

yes, that sounds like a perfect solution.

I'll investigate what's happening with deleteorphanmedia and will open an issue if it's a thing

On Fri, Jun 30, 2017 at 12:28 AM, Konrad Lisiczyński < notifications@github.com> wrote:

Thank you for kind words @dbinetti https://github.com/dbinetti !

Actually forced use of CloudinaryField by pycloudinary was one of the reason this package was created :)

Your use case of course makes sense, so I think I could adjust upload so it could be possible to pass optional option dictionary which would extend default options. So if someone wants to have custom behaviour, one could subclass MediaCloudinaryStorage to pass options like allow unique_filename=False . Do you think this could be a good way to do it? Something like:

def _upload(self, name, content, custom_options=None): options = {'use_filename': True, 'resource_type': self._get_resource_type(name), 'tags': self.TAG} if custom_options is not None: options.update(custom_options) ...

Regarding deleteorphanmedia, it really should work. It should get a list of all images attached to objects in db and remove the rest. If it does not work for you, could you please raise an issue?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/klis87/django-cloudinary-storage/issues/8#issuecomment-312196314, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ3uoshmJnXRNzGm7djhFT5yivTzRH0ks5sJKOygaJpZM4OIazE .

dbinetti commented 7 years ago

So a question: can we just submit public_id and let cloudinary worry about the rest?

klis87 commented 7 years ago

You mean to use public_id in upload? That would mean that you are in control of setting public id, and actually you are the one who needs to worry about cache invalidation etc for files with the same public id. I cannot set this package to work like this as a default, because generally people want to have unique files names for each upload. Even Django won't allow you to upload 2 media files with the same name. If you try, Django will add additional random text to file name.

That's why I can allow you to override upload options by subclassing MediaCloudinaryStorage, then you will be able to do whatever you need for your use case.

dbinetti commented 7 years ago

Ok. I may be overthinking this...

On Jul 3, 2017, 01:33 -0700, Konrad Lisiczyński notifications@github.com, wrote:

You mean to use public_id in upload? That would mean that you are in control of setting public id, and actually you are the one who needs to worry about cache invalidation etc for files with the same public id. I cannot set this package to work like this as a default, because generally people want to have unique files names for each upload. Even Django won't allow you to upload 2 media files with the same name. If you try, Django will add additional random text to file name. That's why I can allow you to override upload options by subclassing MediaCloudinaryStorage, then you will be able to do whatever you need for your use case. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

klis87 commented 7 years ago

So, to sum up, if you still would like to customize cloudinary options in upload method, I will add this feature. But I need your confirmation that you still need it.

dbinetti commented 7 years ago

I'll need to consider it. Please give me a few days, I'm at a conference. many thanks for being so generous with your offer!

On Wed, Jul 5, 2017 at 12:10 AM, Konrad Lisiczyński < notifications@github.com> wrote:

So, to sum up, if you still would like to customize cloudinary options in upload method, I will add this feature. But I need your confirmation that you still need it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/klis87/django-cloudinary-storage/issues/8#issuecomment-313020321, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ3uqaqG0PAMHii3gWT8H9Fet-qNQZwks5sKzbogaJpZM4OIazE .

klis87 commented 7 years ago

Ok, no problem. Have a good conference :)

klis87 commented 7 years ago

Closing due to lack of activity. Please make a comment if you still need this.