[Django] #24104: Refs #23987 - SQLite schema should to look for internal type of field instead of class instance when choosing a default for created fields
Django
instead of class instance when choosing a default for created fields
----------------------------------------------+----------------------
Reporter: coldmind | Owner: coldmind
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------
See alex/django-taggit#285 for example where this issue can be present.
Some custom related fields can not have a default, but in the same time
they will be not inheritors of ManyToManyField.
--
Ticket URL: <https://code.djangoproject.com/ticket/24104>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
instead of class instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* severity: Normal => Release blocker
* needs_better_patch: => 1
* needs_tests: => 0
* needs_docs: => 0
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:1>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I haven't looked into this issue, but just wanted to note that we also
have the
[https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.Field.many_to_many
Field.many_to_many] attribute (though it's new in 1.8) which may be
appropriate for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:2>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Description changed by claudep:
Old description:
> See alex/django-taggit#285 for example where this issue can be present.
> Some custom related fields can not have a default, but in the same time
> they will be not inheritors of ManyToManyField.
New description:
See [[https://github.com/alex/django-taggit/issues/285|alex/django-
taggit#285]] for example where this issue can be present.
Some custom related fields can not have a default, but in the same time
they will be not inheritors of ManyToManyField.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:3>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version:
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* needs_better_patch: 1 => 0
* version: 1.7 =>
Comment:
Patch needs review.
After review I'll create patch to backported to 1.7
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:4>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* version: => master
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:5>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: master => 1.7
Comment:
This issue affects 1.7 and needs to be backported as you indicated, so
please don't change the version.
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:6>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
Some conversation logs:
{{{
<MarkusH> Hey timograham
<MarkusH> What are the implications if we force people to have a
get_internal_type() returning "ManyToMany" for their fields thought they
are not m2m fields?
<MarkusH> /cc truecoldmind ---^
<truecoldmind> MarkusH, we do not force people. For now e.g. django-taggit
with its m2m-fields will not work as expected, because they do not inherit
ManyToManyField. So, if they want to do something custom, but with the
same behavior, they should have ability to do this. For now they does not
have this ability due to `isinstance` checks
<MarkusH> I get that
<MarkusH> The problem they run into on SQLite is the way Django creates
the new table and fills in the values for the new columns
<MarkusH> That is, the default value
<MarkusH> right?
<truecoldmind> MarkusH,
https://github.com/django/django/pull/3865#issuecomment-69796289
<collinanderson> I think isinstance(field, ManyToManyField) should be
"field.many_to_many"
<MarkusH> collinanderson: yes, but not on 1.7
<collinanderson> MarkusH: yes. true
<MarkusH> taggit runs into a problem on >=1.7.2
<truecoldmind> MarkusH, what problems with internal type checks ?
<truecoldmind> for 1.7
<MarkusH> I don't know where they are used internally in the ORM
<MarkusH> it probably affects the lookups
<timograham> the effective_default solution seems a bit closer to what we
had before, I think I'd favor that, at least for 1.7, unless there is some
problem with it
<truecoldmind> timograham, which solution ?
<truecoldmind> timograham, please take a look for
https://github.com/django/django/pull/3865#issuecomment-69796289
<truecoldmind> if it will not call `_remake_table`, it will not reach this
stuff with effective default, right?
<timograham> I didn't look into it. Markus said that fix makes the problem
go away -- are you saying it doesn't?
<truecoldmind> It will fix problem if we will don't fix check in
`add_field`. If field is m2mlike, it should call another method,
`create_table`, not `_remake_table`. So if I understand right it will work
incorrect at least for django-taggit
<truecoldmind> * `create_model`
<truecoldmind> but for example from taggit issue it should call
`_remake_table`, since `field.rel.through._meta.auto_created == False`
(i'm about this check
https://github.com/django/django/blob/cbbe6a6abba6510716e25b7ee9364274334ffcfe/django/db/backends/sqlite3/schema.py#L175)
<truecoldmind> Fix that MarkusH proposed will work for this concrete
issue, but None can be as effective default
<truecoldmind> but with that fix None will be skipped and not added to
mapping
<truecoldmind> so it is not correct
<MarkusH> truecoldmind: can you give me a test that shows the erroneous
behavior?
<truecoldmind> which behavior? where?
<MarkusH> the one you just described
<truecoldmind> if you look in source of effective_default
(https://github.com/django/django/blob/stable/1.7.x/django/db/backends/schema.py#L182)
you can see that None can be chosen as default, but with check you wrote
`if default is not None` it will skip this and will not add to mapping
<MarkusH> right
<MarkusH> but do we need the mapping?
<truecoldmind> you mean in case if default is None?
<MarkusH> ahh, got you: mapping[field.column] =
self.effective_default(field) should be enough?
<MarkusH> mapping[field.column] =
self.quote_value(self.effective_default(field))
<MarkusH> should be sufficient
<truecoldmind> well, not, because for ManyToMany it will choose None as
default, which is not correct
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:7>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:8>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Markus Holtermann <info@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"38c17871bb6dafd489367f6fe8bc56199223adb8"]:
{{{
#!CommitTicketReference repository=""
revision="38c17871bb6dafd489367f6fe8bc56199223adb8"
Fixed #24104 -- Fixed check to look on field.many_to_many instead of class
instance
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:9>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"11a5e45b96c3a15826927f5d0e50472767b937f1"]:
{{{
#!CommitTicketReference repository=""
revision="11a5e45b96c3a15826927f5d0e50472767b937f1"
[1.8.x] Fixed #24104 -- Fixed check to look on field.many_to_many instead
of class instance
Backport of 38c17871bb6dafd489367f6fe8bc56199223adb8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:10>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* status: closed => new
* resolution: fixed =>
Comment:
Backport for 1.7 will be opened as a separate PR including release notes
for 1.7. Release notes can then be added to the 1.8 and 1.9 branches.
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:11>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 1 => 0
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:12>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* has_patch: 0 => 1
Comment:
Port previous patch to 1.7
https://github.com/django/django/pull/3982
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:13>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Markus Holtermann <info@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"3d4a826174b7a411a03be39725e60c940944a7fe"]:
{{{
#!CommitTicketReference repository=""
revision="3d4a826174b7a411a03be39725e60c940944a7fe"
[1.7.x] Fixed #24104 -- Fixed check to look on field.get_internal_type()
instead of class instance
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:14>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"da224d6be0d55811d448ed6d57ac828c18cd1a80"]:
{{{
#!CommitTicketReference repository=""
revision="da224d6be0d55811d448ed6d57ac828c18cd1a80"
Refs #24104 -- Added missing release notes
Forwardport of 3d4a826174b7a411a03be39725e60c940944a7fe from stable/1.7.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:15>
Django
instance when choosing a default for created fields
Reporter: coldmind | Owner: coldmind
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"645fe136c4354ce313be5a150864ad046c227a22"]:
{{{
#!CommitTicketReference repository=""
revision="645fe136c4354ce313be5a150864ad046c227a22"
[1.8.x] Refs #24104 -- Added missing release notes
Forwardport of 3d4a826174b7a411a03be39725e60c940944a7fe from stable/1.7.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24104#comment:16>