Code

Opened 7 years ago

Closed 5 years ago

#4339 closed (duplicate)

Override an existing file, using Model.save_FIELD_file method,

Reported by: elaatifi@… Owned by: Gulopine
Component: Database layer (models, ORM) Version: master
Severity: Keywords: FileField db fs-rf-docs
Cc: densetsu.no.ero.sennin@…, marc.garcia@…, anthony@…, miracle2k Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

When we use save_FIELD_file, If the filename already exists, Django keep adding an underscore to the name of
the file until the filename doesn't exist.
But if I want to override it ?
I think it would be handy to use save_FILED_file(..., override=True) (and keep it False by default)

Attachments (4)

override_existing_files_patch.diff (2.5 KB) - added by elaatifi@… 7 years ago.
overwrite_existing_files.diff (3.3 KB) - added by elaatifi@… 7 years ago.
file_field_delete_file_on_update.diff (1.2 KB) - added by lakin.wecker@… 6 years ago.
Patch which deletes a file when a new one is upload, but only if we're the only record which references that file.
file_field_delete_file_on_update.2.diff (1.2 KB) - added by lakin.wecker@… 6 years ago.

Download all attachments as: .zip

Change History (32)

Changed 7 years ago by elaatifi@…

comment:1 Changed 7 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Override an existing file, using Model.save_FIELD_file method to Fixed #3426, Override an existing file, using Model.save_FIELD_file method,

comment:2 Changed 7 years ago by mtredinnick

  • Summary changed from Fixed #3426, Override an existing file, using Model.save_FIELD_file method, to Override an existing file, using Model.save_FIELD_file method,

Undo what I presume is an accidental title change, since it makes no sense.

comment:3 Changed 7 years ago by Marc Fargas <telenieko@…>

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Maybe the thing should be called "overwrite" not "override"
Also, documentation about that should be available somewhere ;)

comment:4 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design decision needed

Umm... I'm not sure this is an automatic "accepted", since we've had hesitation in the past about automatically overriding files (it has security and annoyance implications, for a start). Moving to "design decision needed" until we've had time to think about it.

comment:5 Changed 7 years ago by Marc Fargas <telenieko@…>

I marked as accepted because this patch does not automatically overwrite a file, you need to override a method on your model, in which case you are supposed to know what you are doing and pass the right parameter to the super() method.

On the other side, not having this means that anybody that wants to overwrite files needs to write his/her own method checking for file existence, etc. Which doesn't seem very DRY.

Just my 0.02, well 0.002 :P

Changed 7 years ago by elaatifi@…

comment:6 Changed 7 years ago by elaatifi@…

Now I can use

class UserProfile(models.Model):
    avatar = models.ImageField(blank=True, upload_to='users/avatars/', overwrite=True)

the other use is to set explicitly overwrite to True,

userprofile.save_avatar_file(filename, content, overwrite=True)

comment:7 follow-up: Changed 7 years ago by Seamus

When is this patch going to be acccepted into the django development version? This is a very useful feature, there is no automatic overwriting of files, it simply gives more flexibility to the programmer. What is the problem with this exactly?

comment:8 Changed 7 years ago by Simon G. <dev@…>

when some of core developers have time to decide whether it's a good idea or not (see Malcolm's comments above). If you want to hurry it up, then feel free to post a message to django-developers pointing out the good points of this.

comment:9 in reply to: ↑ 7 Changed 7 years ago by ubernostrum

Replying to Seamus:

When is this patch going to be acccepted into the django development version? This is a very useful feature, there is no automatic overwriting of files, it simply gives more flexibility to the programmer. What is the problem with this exactly?

There are currently over three hundred tickets at the stage of "design decision needed", and that stage, by its nature, requires a fair amount of attention from multiple developers. As a result, it can take quite some time for a more or less "normal" process of discussion to get around to any particular ticket unless someone steps up -- as Chris suggested -- and gets the ball rolling. The place to do that is the django-developers list (and please note that a short "this ticket needs discussion" or "I need this feature, why don't you add it already" message is generally frowned upon -- to get discussion going, mention the ticket and provide an argument for or against the proposed solution).

comment:10 Changed 7 years ago by Densetsu no Ero-sennin <densetsu.no.ero.sennin@…>

  • Cc densetsu.no.ero.sennin@… added

I was really surprised when discovered that the is no simple way to overwrite an uploaded file in Django. So I'm already using this patch and hope it will be merged soon.

comment:11 Changed 6 years ago by garcia_marc

  • Cc marc.garcia@… added

I agree that this patch should be merged (and I started a topic in django-devel), but I disagree that there is no simple way to do it in Django.

Check this:

def _save_FIELD_file(self, field, filename, raw_contents, save=True):
        fullpath_filename = os.path.join(settings.MEDIA_ROOT, field.get_filename(filename))
        if os.path.exists(fullpath_filename):
                os.remove(fullpath_filename)
        super(MyModel, self)._save_FIELD_file(field, filename, raw_contents, save)

Changed 6 years ago by lakin.wecker@…

Patch which deletes a file when a new one is upload, but only if we're the only record which references that file.

comment:12 Changed 6 years ago by lakin.wecker@…

I added a patch which takes a different approach on this subject. The reasoning is simple. The current code defines the following behavior: when an instance of a model is deleted, the file will also be deleted if the deleted model instance was the only instance which referenced this file. Hence, the curent code is already destructive. To me, it follows logically that the code should do the same thing when a new file is uploaded. If the new file upload is named the same as the old one, it'll be an effective overwrite. If not, it's just remove a file from the filesystem that would otherwise be unreferenceable and not manageable from the admin. Additionally, if there are other records that reference the file, the original one will still be preserved.

Changed 6 years ago by lakin.wecker@…

comment:13 Changed 6 years ago by Gulopine

  • Keywords fs-rf-docs added

comment:14 Changed 6 years ago by Gulopine

  • milestone set to 1.0 beta

comment:15 Changed 6 years ago by aptiko

  • Cc anthony@… added

comment:16 Changed 6 years ago by mir

  • milestone changed from 1.0 beta to post-1.0

Since this is not regarded a bug, there is no reason to give this to milestone 1.0 beta.

comment:17 Changed 6 years ago by Gulopine

  • milestone changed from post-1.0 to 1.0 beta
  • Owner changed from nobody to Gulopine
  • Status changed from new to assigned

Bugs aren't the only things planned for 1.0 beta. This is being addressed as part of #5361, which is planned to make it into 1.0 beta. Even though it won't be fixed directly, the behavior requested in this ticket will be quite possible once that goes in. Per Jacob, all such related tickets should be placed in the 1.0 beta milestone.

comment:18 Changed 6 years ago by jacob

  • Resolution set to wontfix
  • Status changed from assigned to closed

File storage is in as of [8244], so I'm marking this as wontfix -- as Marty says, it's very easy to write a custom file storage backend that works exactly how you like it.

comment:19 Changed 6 years ago by lakin@…

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I'm re-opening this because I respectfully disagree with Marty. Don't get me wrong, I much prefer the new FileStorage backend interface, but according to the current docs - 1 the file storage object is never given any context with which to decide whether or not to overwrite the file.

Here is my dilemna, I have an Image model, that allows customers to upload images. Inevitably, the customers want to replace a specific image with a new one, so they re-upload a new version of the file and it gets a new name. So things break, like CSS background image urls. etc. In the old monkey-patch, I could query the models to find out if the current model-instance was the only row that referred to the image, and if so it would overwrite it. If not, it would rename the file as normal.

With the current system, my custom storage instance has no access to the necessary request specific context, like model-instance in order to decide whether or not to overwrite the file. So it must generally make a decision of "always overwrite files even when they have the same name" or "always generate a new name, even if the old filename will be orphaned (no more references to it in the database.)"

comment:20 Changed 6 years ago by mtredinnick

  • milestone 1.0 beta deleted

comment:21 Changed 5 years ago by olau

Actually, with the default backend you've got a DoS entry if you allow your users to upload a profile picture with an ImageField (even if you check the size of the stuff they upload) - since it will leave the orphaned images behind. The attacker just needs to reupload files to fill up the available disk space which may be scarce on shared hosting.

In any case, the current behaviour doesn't really make a lot of sense when you override upload_to to set a filename (e.g. using the db id) instead of relying on the name from the browser.

I think it should work this way: when you reupload a file, it should be the same as first deleting the old file and then writing the new one (maybe in reverse order, with a bit of code to handle the case where the names are identical). What do you think?

BTW, there's a snippet here with a custom backend that always overwrites:

http://www.djangosnippets.org/snippets/976/

comment:22 Changed 5 years ago by elaatifi

With Django's new file access API there is more flexibity to deal with such problems.

I think it's not a good idea to override get_available_name since it's a public methode
that shouldn't have a side effect (I suppose) due to his name

{{{class OverwriteStorage(FileSystemStorage):

def _save(self, name, content):

if self.exists(name):

self.delete(name)

return super(OverwriteStorage, self)._save(name, content)

}}}

Thank you

comment:23 Changed 5 years ago by miracle2k

  • Cc miracle2k added
  • Owner changed from Gulopine to miracle2k
  • Status changed from reopened to new

Without having tried it, I don't think the code given my elaatifi (overriding _save) will work, since at this point get_available_name

comment:24 Changed 5 years ago by miracle2k

  • Owner changed from miracle2k to Gulopine

Apparently I screwed up that last comment pretty badly - sorry. Not sure why the status changed.

comment:25 Changed 5 years ago by nwelch

Overriding both _save and get_available_name seems to do the trick for me. It *seems* that it should be unnecessary to override both, but overwriting _save wasn't enough, and only overriding get_available_name resulted in an infinite loop since _save demands that the file not exist (i.e. it won't do an actual overwrite -- but rather a delete, then a write).

from django.core.files.storage import FileSystemStorage
class OverwriteStorage(FileSystemStorage):
    def _save(self, name, content):
        if self.exists(name):
            self.delete(name)
        return super(OverwriteStorage, self)._save(name, content)
    def get_available_name(self, name):
        return name

I disclaim any copyright to the above code. (I'm guessing it would be considered too trivial to be copyrighted anyway, but just in case)

Also, while this is maybe a bit more likely to cause race conditions, nothing particularly harmful happens as a result. The thread will just have to try again until it succeeds.

comment:26 Changed 5 years ago by rizumu

Just a quick thought from a UI standpoint in the Admin, it sure would be nice when you add an image for there to be a pop-up that showed you the image and proposed three options. A similar pop-up for an image could post some meta-data.

Would you like to:
Overwrite the existing image with the new one? (sitewide)
Save the new image under a different name? (affects only this instance)
Use the existing image? (No need to create a new one with an _ in the name)

comment:27 Changed 5 years ago by olau

rizumu: This bug is not particular to the admin. Also what you're talking about is the situation where the user is supposed to control the filename. This bug is about the case where the programmer is supposed to control the filename (where currently there's no simple knob in Django to say "this filename, and I mean it, no underscores").

comment:28 Changed 5 years ago by SmileyChris

  • Resolution set to duplicate
  • Status changed from new to closed

Since the original issue is very out of date after filesystem refactor, I'll close this as a duplicate of the new ticket #11663

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.