Code

Opened 2 years ago

Closed 2 years ago

Last modified 19 months ago

#18515 closed Bug (fixed)

Problematic validation code in FileField

Reported by: yoyoma Owned by: nobody
Component: File uploads/storage Version: 1.4
Severity: Normal Keywords: max_length, FileField, fields
Cc: yoyoma Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The following code in django.db.models.fields.files (line # 235 in master @ HEAD) is problematic.

    def validate(self, value, model_instance):
        """
        Validates that the generated file name still fits within max_length.
        """
        # The generated file name stored in the database is generally longer
        # than the uploaded file name. Using the length of generated name in
        # the error message would be confusing. However, in the common case
        # (ie. upload_to='path/to/upload/dir'), the length of the generated
        # name equals the length of the uploaded name plus a constant. Thus
        # we can tell the user how much shorter the name should be (roughly).
        length = len(self.generate_filename(model_instance, value.name))
        if self.max_length and length > self.max_length:
            error_values = {'extra': length - self.max_length}
            raise ValidationError(self.error_messages['max_length'] % error_values)

Given the following:

def oops_asset_path(instance, filename):
    """
    Returns the upload path for a ``Oops.asset``.
    """
    return 'oops/user_{upk}/{dt}/{fn}'.format(
        upk=instance.user_id,
        dt=timezone.now().strftime('%Y_%j_%H_%M_%S_%f'),
        fn=re.sub(r'[^\w\.]', '_', filename).lower())


class Oops(models.Model):
    asset = models.FileField(_(u'asset'), upload_to=oops_asset_path)

When an Oops instance is saved with the asset file name the-best-file-name-ever.jpg, it's converted to something like:

oops/user_3145/2012_178_00_16_02_738638/the_best_file_name_ever.jpg

which is expected. Then, when the Oops is saved again in a form (e.g., an inline model formset I use for multiple uploads) the above validation code runs the existing name through my oops_asset_path, which returns:

oops/user_3145/2012_178_00_16_03_457362/oops_user_3145_2012_178_00_16_02_738638_the_best_file_name_ever.jpg

This causes the max_length validation error, since the new name is clearly too long:

Filename is 7 characters too long.

The file name isn't changed permanently, but these erroneous validation errors are only avoided by increasing the field's max_length.

I can't think of any solution to this that doesn't include either a database query or else some sort of annotation on the FileField by forms... both of which scare me.

Attachments (2)

18515-1.diff (2.3 KB) - added by claudep 2 years ago.
Do not always regenerate filename in FileField validation
18515-2.diff (4.2 KB) - added by claudep 2 years ago.
Regrouped FileField max_length tests

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by yoyoma

  • Cc yoyoma added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 2 years ago by yoyoma

  • Type changed from Bug to Uncategorized

comment:3 Changed 2 years ago by claudep

  • Component changed from Uncategorized to File uploads/storage
  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

Changed 2 years ago by claudep

Do not always regenerate filename in FileField validation

Changed 2 years ago by claudep

Regrouped FileField max_length tests

comment:4 Changed 2 years ago by Claude Paroz <claude@…>

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

In [05d333ba3bb16af024c11966d2072de38fe9f82f]:

Fixed #18515 -- Conditionally regenerated filename in FileField validation

When a FileField value has been saved, a new validation should not
regenerate a new filename when checking the length. Refs #9893.

comment:5 Changed 2 years ago by yoyoma

Holy mackerel! Fixed in 15 hours and committed? Thanks, Claude!

comment:6 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In db278c3bf9177043c42a9ed8b529a5c117938460:

Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.

Refs #9893, #18515.

Thanks Russell for the report.

comment:7 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In 3cb87ec605fb9e7785aa0580bd8220797b622e0c:

[1.5.x] Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.

Refs #9893, #18515.

Thanks Russell for the report.

Backport of db278c3 from master.

comment:8 Changed 19 months ago by Preston Holmes <preston@…>

In 0e431e5dd7ed19aa2119ceba9ebed050c2988844:

[1.5.x] Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.

Refs #9893, #18515.

Thanks Russell for the report.

Backport of db278c3 from master.

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.