Opened 3 years ago

Closed 3 years ago

#22199 closed Bug (fixed)

Incorrect handling of max_length in FileField.deconstruct

Reported by: Julian Brost Owned by: Andrew Godwin
Component: Migrations Version: master
Severity: Release blocker Keywords: FileField, ImageField, max_length
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

max_length is handled incorrectly in FileField.deconstruct which results in all FileFields created as VARCHAR(100) in the database.

In order to reproduce the bug, just create a model like this:

class ExampleModel(models.Model):
    f = models.FileField(max_length=255)

Next run makemigrations and migrate. This will create a database table like this one:

CREATE TABLE "foo_examplemodel" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "f" varchar(100) NOT NULL);

The field f has the wrong length which is the default value instead of the max_length attribute passed to the FileField constructor.

Attachments (1)

filefield-deconstruct-max-length.patch (626 bytes) - added by Julian Brost 3 years ago.

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by Julian Brost

comment:1 Changed 3 years ago by Baptiste Mispelon

Needs documentation: unset
Needs tests: set
Patch needs improvement: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Hi,

This does appear to be a problem and your patch seems like it fixes the issue.

However, a full patch will also need some tests in order to get merged.

Thanks.

comment:2 in reply to:  1 Changed 3 years ago by Julian Brost

Replying to bmispelon:

However, a full patch will also need some tests in order to get merged.

What exactly should such a testcase cover? Just this attribute for the FileField class or for all Field classes that support max_length?

comment:3 Changed 3 years ago by Baptiste Mispelon

After a quick look, it seems this bug is only present for FileField so you can probably only add a test for it.

However, it'd be useful to see whether we actually have any tests for this behavior and add some if that's not the case.
The problematic fields are those that set a value for max_length in their __init__() method:

  • EmailField (this one might be special-cased. see 48493cff73fd01870306965f1c48193602754a78)
  • FilePathField
  • IPAddressField
  • GenericIPAddressField (you should be able to simplify the deconstruct() method on this one since max_length is always set to 39)
  • SlugField
  • URLField
  • And of course, FileField.

comment:4 Changed 3 years ago by Andrew Godwin

Owner: changed from nobody to Andrew Godwin
Status: newassigned

comment:5 Changed 3 years ago by Andrew Godwin <andrew@…>

Resolution: fixed
Status: assignedclosed

In cd7a2a077ed5dc4dd04e55d60e72fd15d71e928a:

Fixed #22199: Bad max_length deconstruction for FileField

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