Opened 17 years ago

Closed 15 years ago

#4339 closed (duplicate)

Override an existing file, using Model.save_FIELD_file method,

Reported by: elaatifi@… Owned by: Marty Alchin
Component: Database layer (models, ORM) Version: dev
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: no UI/UX: no

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@… 17 years ago.
overwrite_existing_files.diff (3.3 KB ) - added by elaatifi@… 17 years ago.
file_field_delete_file_on_update.diff (1.2 KB ) - added by lakin.wecker@… 16 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@… 16 years ago.

Download all attachments as: .zip

Change History (32)

by elaatifi@…, 17 years ago

comment:1 by anonymous, 17 years ago

Summary: Override an existing file, using Model.save_FIELD_file methodFixed #3426, Override an existing file, using Model.save_FIELD_file method,

comment:2 by Malcolm Tredinnick, 17 years ago

Summary: Fixed #3426, Override an existing file, using Model.save_FIELD_file method,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 by Marc Fargas <telenieko@…>, 17 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:4 by Malcolm Tredinnick, 17 years ago

Triage Stage: AcceptedDesign 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 by Marc Fargas <telenieko@…>, 17 years ago

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

by elaatifi@…, 17 years ago

comment:6 by elaatifi@…, 17 years ago

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 by Seamus, 17 years ago

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 by Simon G. <dev@…>, 17 years ago

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.

in reply to:  7 comment:9 by James Bennett, 17 years ago

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 by Densetsu no Ero-sennin <densetsu.no.ero.sennin@…>, 17 years ago

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 by Marc Garcia, 16 years ago

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)

by lakin.wecker@…, 16 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.

comment:12 by lakin.wecker@…, 16 years ago

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.

by lakin.wecker@…, 16 years ago

comment:13 by Marty Alchin, 16 years ago

Keywords: fs-rf-docs added

comment:14 by Marty Alchin, 16 years ago

milestone: 1.0 beta

comment:15 by Antonis Christofides, 16 years ago

Cc: anthony@… added

comment:16 by Michael Radziej, 16 years ago

milestone: 1.0 betapost-1.0

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

comment:17 by Marty Alchin, 16 years ago

milestone: post-1.01.0 beta
Owner: changed from nobody to Marty Alchin
Status: newassigned

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 by Jacob, 16 years ago

Resolution: wontfix
Status: assignedclosed

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 by lakin@…, 16 years ago

Resolution: wontfix
Status: closedreopened

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 by Malcolm Tredinnick, 16 years ago

milestone: 1.0 beta

comment:21 by Ole Laursen, 15 years ago

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 by elaatifi, 15 years ago

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 by miracle2k, 15 years ago

Cc: miracle2k added
Owner: changed from Marty Alchin to miracle2k
Status: reopenednew

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 by miracle2k, 15 years ago

Owner: changed from miracle2k to Marty Alchin

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

comment:25 by nwelch, 15 years ago

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 by Thomas Schreiber, 15 years ago

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 by Ole Laursen, 15 years ago

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 by Chris Beaven, 15 years ago

Resolution: duplicate
Status: newclosed

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

Note: See TracTickets for help on using tickets.
Back to Top