[Django] #23679: varchar(255) converted into varchar(255) NOT NULL after inspectdb followed by migrate
Django
by migrate
--------------------------------------------+--------------------
Reporter: paulcdejean | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Management commands) | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------------+--------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/23679>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Uncategorized | Status: new
commands) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by paulcdejean):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
If I had to guess this is a case of: hey "varchar(255) .* NULL" totally
matches "varchar(255) NOT NULL"
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:1>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
The fact that `inspectdb` doesn't add `null=True` on
`CharField`s/`TextField`s 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:2>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by paulcdejean):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:3>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by claudep):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:4>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:5>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timgraham):
I agree with Carl.
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:6>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/3395
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:7>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 1.7
commands) | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:8>
Django
by migrate
Reporter: paulcdejean | Owner: nobody
Type: Bug | Status: closed
Component: Core (Management | Version: 1.7
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"4fe6824ffd319b199140e291f0f742e7fe1f4615"]:
{{{
#!CommitTicketReference repository=""
revision="4fe6824ffd319b199140e291f0f742e7fe1f4615"
Fixed #23679 -- Fixed null introspection for char/text fields
Thanks Paul Dejean for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23679#comment:9>