Opened 9 years ago

Closed 9 years ago

#23679 closed Bug (fixed)

varchar(255) converted into varchar(255) NOT NULL after inspectdb followed by migrate

Reported by: Paul Dejean Owned by: nobody
Component: Core (Management commands) Version: 1.7
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

show create table on the original database yields:

device_mac_ethernet varchar(255) DEFAULT NULL

inspectdb convert this into:

models.CharField(max_length=255, blank=True)

makemigrations followed by migrate converts that into:

device_mac_ethernet varchar(255) NOT NULL

Change History (9)

comment:1 by Paul Dejean, 9 years ago

If I had to guess this is a case of: hey "varchar(255) .* NULL" totally matches "varchar(255) NOT NULL"

Version 0, edited 9 years ago by Paul Dejean (next)

comment:2 by Claude Paroz, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

The fact that inspectdb doesn't add null=True on CharFields/TextFields is based on a Django "good practice", explained in https://docs.djangoproject.com/en/stable/ref/models/fields/#null

Now the question is, should inspectdb reflect the current database structure as faithfully as possible, or should it entice users to follow good practices? At a minimum, Django should add a comment that it has not respected the NULL flag on the introspected field. Accepted the ticket on that base.

comment:3 by Paul Dejean, 9 years ago

How about adding a command line option to inspectdb so the user can choose. Good practice being enforced can be the default but users who need to deviate for some reason can do so without having to hack the inspectdb source as I did.

comment:4 by Claude Paroz, 9 years ago

inspectdb output has to be reviewed in all cases. For me, the choice is:

  • keep the current output and add a comment like: # Field was detected as NULL, but we didn't add null=True, see https://docs.djangoproject.com/en/stable/ref/models/fields/#null
  • change the output to let null=True and add the comment: # Read https://docs.djangoproject.com/en/stable/ref/models/fields/#null and consider removing null=True

I'm currently favoring the second option.

comment:5 by Carl Meyer, 9 years ago

Given those two choices, I'm in favor of the second option as well, though I don't even think the comment is really necessary.

I think Django's advice re null CharFields is questionable, but even if we accept that advice, a CharField(null=True) is still a valid, functional configuration, and there is no reason for inspectdb to prohibit it and intentionally generate models that don't match the source database.

comment:6 by Tim Graham, 9 years ago

I agree with Carl.

comment:8 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Claude Paroz <claude@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 4fe6824ffd319b199140e291f0f742e7fe1f4615:

Fixed #23679 -- Fixed null introspection for char/text fields

Thanks Paul Dejean for the report.

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