Opened 6 years ago

Closed 6 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 Changed 6 years ago by Paul Dejean

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

Also I might submit a patch for this *fingers crossed*

Last edited 6 years ago by Paul Dejean (previous) (diff)

comment:2 Changed 6 years ago by Claude Paroz

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 Changed 6 years ago by Paul Dejean

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 Changed 6 years ago by Claude Paroz

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 Changed 6 years ago by Carl Meyer

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 Changed 6 years ago by Tim Graham

I agree with Carl.

comment:8 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

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

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