#22199 closed Bug (fixed)

Incorrect handling of max_length in FileField.deconstruct

Reported by: julianbrost Owned by: andrewgodwin
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 julianbrost 16 months ago.

Download all attachments as: .zip

Change History (6)

Changed 16 months ago by julianbrost

comment:1 follow-up: Changed 16 months ago by bmispelon

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 16 months ago by julianbrost

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 16 months ago by bmispelon

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 16 months ago by andrewgodwin

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

comment:5 Changed 16 months ago by Andrew Godwin <andrew@…>

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

In cd7a2a077ed5dc4dd04e55d60e72fd15d71e928a:

Fixed #22199: Bad max_length deconstruction for FileField

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