Code

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#9610 closed (fixed)

File upload duplicate filename check mechanism fail

Reported by: fadlytabrani Owned by: fadlytabrani
Component: File uploads/storage Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The file upload duplicate filename check mechanism will fail if the upload directory contains a dot and when the filename does not.

django.core.files.storage:Storage

def get_available_name(self, name):
        """
        Returns a filename that's free on the target storage system, and
        available for new content to be written to.
        """
        # If the filename already exists, keep adding an underscore to the name
        # of the file until the filename doesn't exist.
        while self.exists(name):
            try:
                dot_index = name.rindex('.')
            except ValueError: # filename has no dot
                name += '_'
            else:
                name = name[:dot_index] + '_' + name[dot_index:]
        return name

# Failure example, uploading duplicate file "testlog" will create new directory upload_.10122008
# Correct result would be "testlog" would be uploaded with the filename testlog_
get_available_name("c:\\django\\media\\upload.10122008\\testlog")

Using os.path would solve the problem

def get_available_name(self, name):
        while self.exists(name):
            dname, fname = os.path.split(name)
            try:
                dot_index = fname.rindex('.')
            except ValueError:
                fname += '_'
            else:
                fname = fname[:dot_index] + '_' + fname[dot_index:]
                
            return os.path.join(dname, fname)

Attachments (3)

storage.py.patch (478 bytes) - added by fadlytabrani 6 years ago.
storage.py.2.patch (477 bytes) - added by fadlytabrani 6 years ago.
9610.diff (2.2 KB) - added by adurdin 5 years ago.
Added tests, and updated the patch to unified diff format.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by fadlytabrani

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Initial submitted fix doesn't work, this does

def get_available_namex(self, name):
        while self.exists(name):
            dname, fname = os.path.split(name)
            try:
                dot_index = fname.rindex('.')
            except ValueError:
                fname += '_'
            else:
                fname = fname[:dot_index] + '_' + fname[dot_index:]
            name = os.path.join(dname, fname)
        return name

comment:2 Changed 6 years ago by dc

Little improvement:

def get_available_namex(self, name):
    dir_name, file_name = os.path.split(name) 
    while self.exists(name):
        try:
            dot_index = file_name.rindex('.')
        except ValueError:
            file_name += '_'
        else:
            file_name = file_name[:dot_index] + '_' + file_name[dot_index:]
        name = os.path.join(dir_name, file_name)
    return name

Changed 6 years ago by fadlytabrani

comment:3 Changed 6 years ago by fadlytabrani

  • Owner changed from nobody to fadlytabrani
  • Status changed from new to assigned

Patch attached

The line :

dir_name, file_name = os.path.split(name)

should be within the while loop to acommodate checking more than once

def get_available_name(self, name): 
    while self.exists(name):
		dir_name, file_name = os.path.split(name)
        try:
            dot_index = file_name.rindex('.')
        except ValueError:
            file_name += '_'
        else:
            file_name = file_name[:dot_index] + '_' + file_name[dot_index:]
        name = os.path.join(dir_name, file_name)
    return name

comment:4 follow-up: Changed 6 years ago by fadlytabrani

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:5 Changed 6 years ago by fadlytabrani

  • Triage Stage changed from Ready for checkin to Design decision needed

comment:6 Changed 6 years ago by fadlytabrani

  • Resolution set to worksforme
  • Status changed from assigned to closed
  • Triage Stage changed from Design decision needed to Unreviewed

comment:7 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by dc

  • Patch needs improvement set

Replying to fadlytabrani:

The line should be within the while loop to acommodate checking more than once

Nope. dir_name never changes and it should be calculated once. But your version calls unneeded split every iteration. Just check in debugger (or something like that) if you unsure.

Changed 6 years ago by fadlytabrani

comment:8 in reply to: ↑ 7 Changed 6 years ago by fadlytabrani

Replying to dc:

Replying to fadlytabrani:

The line should be within the while loop to acommodate checking more than once

Nope. dir_name never changes and it should be calculated once. But your version calls unneeded split every iteration. Just check in debugger (or something like that) if you unsure.

Thanks dc, you're correct :) I missed that one

Have uploaded correct path

comment:9 Changed 6 years ago by fadlytabrani

  • Needs tests set
  • Patch needs improvement unset

comment:10 Changed 6 years ago by dc

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Still a bug which must be fixed.

comment:11 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:12 Changed 5 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by adurdin

Added tests, and updated the patch to unified diff format.

comment:13 Changed 5 years ago by adurdin

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 5 years ago by gwilson

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [10702]) [1.0.X]: Fixed #9610 -- Fixed duplicate uploaded file name mangling when directory contained a dot and file didn't. Based on patches from fadlytabrani and adurdin.

Backport of r10701 from trunk.

comment:15 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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.