Opened 10 years ago
Last modified 8 years ago
#24227 new Cleanup/optimization
isinstance checks on ForeignKey/ManyToManyField should be replaced with field.many_to_one/field.many_to_many
Reported by: | Andriy Sokolovskiy | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | cmawebsite@…, Markus Holtermann | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
\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.
Change History (24)
comment:1 by , 10 years ago
Cc: | added |
---|
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 10 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|
comment:4 by , 10 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 10 years ago
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.
comment:6 by , 10 years ago
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."
comment:7 by , 9 years ago
Has patch: | set |
---|
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.
comment:8 by , 9 years ago
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?
comment:9 by , 9 years ago
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.
comment:10 by , 9 years ago
Patch needs improvement: | set |
---|
comment:11 by , 9 years ago
I've got a +1 from Collin to commit the ManyToMany
part of the patch (PR #6265). If noone opposes within one day, I'll commit that one and then rebase the patch with the ForeignKey
changes.
comment:13 by , 9 years ago
Patch needs improvement: | unset |
---|
Pull request updated after committing the M2M part.
comment:14 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:15 by , 9 years ago
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',)
comment:16 by , 9 years ago
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.
comment:17 by , 8 years ago
I have a pull request here which I believe reverts the problematic parts of the original change. https://github.com/django/django/pull/6411
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 aqs
into a list ofpk
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.
comment:21 by , 8 years ago
Patch needs improvement: | set |
---|---|
Summary: | isinstance checks on ManyToManyField should be replaced with field.many_to_many → isinstance checks on ForeignKey/ManyToManyField should be replaced with field.many_to_one/field.many_to_many |
Triage Stage: | Ready for checkin → Accepted |
Claude abandoned 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.
comment:22 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
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 aboutForeignKey
and foreignkey-like fields, wherefield.one_to_many
andfield.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.