[Django] #24227: isinstance checks on ManyToManyField should be replaced with field.many_to_many
Django
field.many_to_many
--------------------------------------+--------------------
Reporter: coldmind | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
`\bisinstance\b\([a-zA-Z._-]*.\s[a-zA-Z.]*?\bManyToManyField\b` gives me
15 occurrences (I will check manually too).
As a followup to https://code.djangoproject.com/ticket/24104, it might be
good to check and replace (where we can do this) to look more cleaner and
allow custom m2m-fields to behave in the same way like original
`ManyToManyField` do.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
field.many_to_many
Reporter: coldmind | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* cc: cmawebsite@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:1>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* status: new => assigned
* owner: nobody => coldmind
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:2>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* component: Uncategorized => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:3>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* cc: MarkusH (added)
* stage: Unreviewed => Accepted
Comment:
This change should eventually happen at some point. But I don't think we
are there yet. If we are going to replace a bunch of `isinstance()` calls
in the migration / schema / ORM layers, we should also think about
`ForeignKey` and foreignkey-like fields, where `field.one_to_many` and
`field.one_to_one` join the game.
This ticket will require some deeper digging than the one referenced
(#24104).
As soon as composite fields and composite foreign keys are a thing, this
ticket will certainly be related.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:4>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by pirosb3):
Hi
I agree. Now that we have field flags {{{isinstance()}}} checks can be
replaced, possibly also removing an import statement. As far as I remember
(@freakyboy and @timgraham let me know if this is not the case) we wanted
to do this in stages.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:5>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Yea, as Loic said in IRC, "The thing is in practice we probably aren't
ready. If isinstance is followed by reaching private attributes of
ManyToManyField, checking for many_to_many isn't going to work, but in any
case it'd be good to identify where we have this kind of issues."
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:6>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6236 PR]
It's not always obvious to know if `isinstance(field, ForeignKey)` should
be replaced by `field.many_to_one` or by `field.many_to_one or
field.one_to_one`, as OneToOneField is a ForeignKey subclass. Trusting the
test suite at this regard.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:7>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
It's not obvious to me if all of the replacements as proposed are better
than `isinstance`, especially where we hardcode class names like
`ForeignKey` in error messages. I would be more comfortable not making any
changes until we have specific use cases where the current checks are
problematic, but I'd like to hear other opinions. If we could find some
third-party fields that are making use of these flags, I think that would
be quite useful for evaluating whether this change is useful. Did you have
a specific motivation for completing this?
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:8>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I think, but unfortunately don't remember exactly when, that I have been
bitten at some point by the `isinstance(f, ManyToManyField)` in the
`django.forms.models.model_to_dict` function (I had a branch with that
change).
In any case, duck typing is clearly a better pattern than `isinstance`
calls in most cases, that's why I think we should go forward with this.
Wrt error messages, we could for instance change `ForeignKey` by `foreign
key` when appropriate.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:9>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:10>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by claudep):
I've got a +1 from Collin to commit the `ManyToMany` part of the patch
([https://github.com/django/django/pull/6265 PR #6265]). If noone opposes
within one day, I'll commit that one and then rebase the patch with the
`ForeignKey` changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:11>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Claude Paroz <claude@…>):
In [changeset:"983c158da7723eb00a376bd31db76709da4d0260" 983c158d]:
{{{
#!CommitTicketReference repository=""
revision="983c158da7723eb00a376bd31db76709da4d0260"
Refs #24227 -- Replaced M2M isinstance checks by field.many_to_many
Thanks Markus Holtermann, Collin Anderson and Tim Graham for the reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:12>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 1 => 0
Comment:
Pull request updated after committing the M2M part.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:13>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:14>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jpic):
Could we have extended documentation of what behaviors this changes
exactly ?
I'm seeing that django-taggit doesn't have its formfield rendered anymore
(even though it has the many_to_many flag) and I'm also investigating an
exception raised when the admin enforces a RelatedWidgetWrapper around
django-gm2m's GM2MField widget - even though it's **not** a concrete model
field with a formfield. I mean that having the GM2MField's name in
ModelForm.Meta.fields will raise an exception about it not being an
"available" field, but RelatedWidgetWrapper is enforced there anyway.
{{{
Traceback (most recent call last):
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/core/handlers/base.py", line 150, in get_response
response = self.process_exception_by_middleware(e, request)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/core/handlers/base.py", line 148, in get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/options.py", line 542, in wrapper
return self.admin_site.admin_view(view)(*args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/utils/decorators.py", line 149, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/views/decorators/cache.py", line 57, in _wrapped_view_func
response = view_func(request, *args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/sites.py", line 209, in inner
return view(request, *args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/options.py", line 1493, in add_view
return self.changeform_view(request, None, form_url, extra_context)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/utils/decorators.py", line 67, in _wrapper
return bound_func(*args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/utils/decorators.py", line 149, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/utils/decorators.py", line 63, in bound_func
return func.__get__(self, type(self))(*args2, **kwargs2)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/utils/decorators.py", line 185, in inner
return func(*args, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/options.py", line 1423, in changeform_view
ModelForm = self.get_form(request, obj)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/options.py", line 640, in get_form
return modelform_factory(self.model, **defaults)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-packages/django/forms/models.py",
line 556, in modelform_factory
return type(form)(class_name, (form,), form_class_attrs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-packages/django/forms/models.py",
line 257, in __new__
opts.field_classes)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-packages/django/forms/models.py",
line 180, in fields_for_model
formfield = formfield_callback(f, **kwargs)
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/options.py", line 165, in
formfield_for_dbfield
formfield.widget, db_field.remote_field, self.admin_site,
**wrapper_kwargs
File "/home/jpic/env/src/dal/.tox/base-
py27-django110-sqlite/lib/python2.7/site-
packages/django/contrib/admin/widgets.py", line 258, in __init__
self.choices = widget.choices
AttributeError: 'TextInput' object has no attribute 'choices'
}}}
I'm also investigating why it gets a TextInput now, probably because it
inherits from Field's .formfield method, with this form:
{{{
class TestForm(autocomplete.FutureModelForm):
test = autocomplete.GM2MQuerySetSequenceField(
queryset=autocomplete.QuerySetSequence(
Group.objects.all(),
TestModel.objects.all(),
),
required=False,
widget=autocomplete.QuerySetSequenceSelect2Multiple(
'select2_gm2m'),
)
class Meta:
model = TestModel
fields = ('name',)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:15>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
As I said in comment:8, it's not so clear what changes are expected. I
think we need the community to provide such documentation about what
changes they see in their applications.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:16>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
I have a pull request here which I believe reverts the problematic parts
of the original change. https://github.com/django/django/pull/6582
There are 2 code branches which are basically just hacks to make the
current ManyToManyFields work properly:
- in forms/models.py, `model_to_dict` converts a `qs` into a list of
`pk`s.
- in admin/options.py, `get_changeform_initial_data` splits a string on
commas `','`
Custom form fields should handle this behavior themselves (if they want
this behavior, which shouldn't be assumed).
The other code paths are for wrapping the form field in
`ManyToManyRawIdWidget` or other widgets, and I a custom field will want a
custom widget.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:17>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Removing the special case in `model_to_dict()` seems feasible:
[https://github.com/django/django/pull/6585 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:18>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"67d984413c9540074e4fe6aa033081a35cf192bc" 67d98441]:
{{{
#!CommitTicketReference repository=""
revision="67d984413c9540074e4fe6aa033081a35cf192bc"
Refs #24227 -- Removed ManyToManyField special casing in model_to_dict().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:19>
Django
field.many_to_many
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"38c43b2a5c9e47f2fabb60521c1224825ebcc833" 38c43b2a]:
{{{
#!CommitTicketReference repository=""
revision="38c43b2a5c9e47f2fabb60521c1224825ebcc833"
Refs #24227 -- Partially reverted replacement of M2M isinstance checks by
field.many_to_many.
This fixes django-taggit and reflects some places where duck-typing may
not
be appropriate.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:20>
Django
field.many_to_one/field.many_to_many
-------------------------------------+-------------------------------------
Reporter: coldmind | Owner: coldmind
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Claude abandoned [https://github.com/django/django/pull/6236 the PR] to
change instances of `isinstance(field, ForeignKey)` to
`field.many_to_one`. If someone sees an advantage to those changes, feel
free to send a new PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:21>
Django
field.many_to_one/field.many_to_many
-------------------------------------+-------------------------------------
Reporter: coldmind | Owner:
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* owner: coldmind =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:22>
Django
field.many_to_one/field.many_to_many
-------------------------------------+-------------------------------------
Reporter: coldmind | Owner:
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"a4c20ae85b40c49e28d1b2227208e4f00d7820df" a4c20ae8]:
{{{
#!CommitTicketReference repository=""
revision="a4c20ae85b40c49e28d1b2227208e4f00d7820df"
Refs #24227 -- Fixed crash of ManyToManyField.value_from_object() on
unsaved model instances.
This behavior was removed in 67d984413c9540074e4fe6aa033081a35cf192bc
but is needed to prevent a crash in formtools.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:23>
Django
field.many_to_one/field.many_to_many
-------------------------------------+-------------------------------------
Reporter: coldmind | Owner:
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"f529d0cb5855db2dfc8d0d39850dbb4793bc75c2" f529d0cb]:
{{{
#!CommitTicketReference repository=""
revision="f529d0cb5855db2dfc8d0d39850dbb4793bc75c2"
[1.10.x] Refs #24227 -- Fixed crash of ManyToManyField.value_from_object()
on unsaved model instances.
This behavior was removed in 67d984413c9540074e4fe6aa033081a35cf192bc
but is needed to prevent a crash in formtools.
Backport of a4c20ae85b40c49e28d1b2227208e4f00d7820df from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24227#comment:24>