isinstance checks on ForeignKey/ManyToManyField should be replaced with field.many_to_one/field.many_to_many

[Andriy Sokolovskiy]

\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.

[Markus Holtermann]

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.

[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.

[Tim Graham]

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."

[Claude Paroz]

​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.

[Tim Graham]

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?

[Claude Paroz]

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.

[Claude Paroz]

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.

[Claude Paroz <claude@…>]

In 983c158d:

Refs #24227 -- Replaced M2M isinstance checks by field.many_to_many

Thanks Markus Holtermann, Collin Anderson and Tim Graham for the reviews.

[Claude Paroz]

Pull request updated after committing the M2M part.

[James Pic]

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',)

[Tim Graham]

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.

[Collin Anderson]

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 pks.
• 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.

[Tim Graham]

Removing the special case in model_to_dict() seems feasible: ​PR

[Tim Graham <timograham@…>]

In 67d98441:

Refs #24227 -- Removed ManyToManyField special casing in model_to_dict().

[Tim Graham <timograham@…>]

In 38c43b2a:

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.

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In a4c20ae8:

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.

[Tim Graham <timograham@…>]

In f529d0cb:

[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