Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#24104 closed Cleanup/optimization (fixed)

SQLite schema should to look for internal type of field instead of class instance when choosing a default for created fields

Reported by: Andriy Sokolovskiy Owned by: Andriy Sokolovskiy
Component: Database layer (models, ORM) Version: 1.7
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Claude Paroz)

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.

Change History (16)

comment:1 by Tim Graham, 10 years ago

Has patch: set
Patch needs improvement: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:2 by Tim Graham, 10 years ago

Summary: Refs #23987 - SQLite schema should to look for internal type of field instead of class instance when choosing a default for created fieldsSQLite schema should to look for internal type of field instead of class instance when choosing a default for created fields

I haven't looked into this issue, but just wanted to note that we also have the Field.many_to_many attribute (though it's new in 1.8) which may be appropriate for this.

comment:3 by Claude Paroz, 10 years ago

Description: modified (diff)

comment:4 by Andriy Sokolovskiy, 10 years ago

Patch needs improvement: unset
Version: 1.7

Patch needs review.
After review I'll create patch to backported to 1.7

comment:5 by Andriy Sokolovskiy, 10 years ago

Version: master

comment:6 by Tim Graham, 10 years ago

Version: master1.7

This issue affects 1.7 and needs to be backported as you indicated, so please don't change the version.

comment:7 by Andriy Sokolovskiy, 10 years ago

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

comment:8 by Markus Holtermann, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Markus Holtermann <info@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 38c17871bb6dafd489367f6fe8bc56199223adb8:

Fixed #24104 -- Fixed check to look on field.many_to_many instead of class instance

comment:10 by Markus Holtermann <info@…>, 10 years ago

In 11a5e45b96c3a15826927f5d0e50472767b937f1:

[1.8.x] Fixed #24104 -- Fixed check to look on field.many_to_many instead of class instance

Backport of 38c17871bb6dafd489367f6fe8bc56199223adb8 from master

comment:11 by Markus Holtermann, 10 years ago

Resolution: fixed
Status: closednew

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.

comment:12 by Tim Graham, 10 years ago

Has patch: unset
Triage Stage: Ready for checkinAccepted

comment:13 by Andriy Sokolovskiy, 10 years ago

Has patch: set

comment:14 by Markus Holtermann <info@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 3d4a826174b7a411a03be39725e60c940944a7fe:

[1.7.x] Fixed #24104 -- Fixed check to look on field.get_internal_type() instead of class instance

comment:15 by Markus Holtermann <info@…>, 10 years ago

In da224d6be0d55811d448ed6d57ac828c18cd1a80:

Refs #24104 -- Added missing release notes

Forwardport of 3d4a826174b7a411a03be39725e60c940944a7fe from stable/1.7.x

comment:16 by Markus Holtermann <info@…>, 10 years ago

In 645fe136c4354ce313be5a150864ad046c227a22:

[1.8.x] Refs #24104 -- Added missing release notes

Forwardport of 3d4a826174b7a411a03be39725e60c940944a7fe from stable/1.7.x

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