Opened 9 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 Collin Anderson, 9 years ago

Cc: cmawebsite@… added

comment:2 by Andriy Sokolovskiy, 9 years ago

Owner: changed from nobody to Andriy Sokolovskiy
Status: newassigned

comment:3 by Collin Anderson, 9 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:4 by Markus Holtermann, 9 years ago

Cc: Markus Holtermann added
Triage Stage: UnreviewedAccepted

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.

comment:5 by pirosb3, 9 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 Tim Graham, 9 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 Claude Paroz, 8 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 Tim Graham, 8 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 Claude Paroz, 8 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 Tim Graham, 8 years ago

Patch needs improvement: set

comment:11 by Claude Paroz, 8 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:12 by Claude Paroz <claude@…>, 8 years ago

In 983c158d:

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

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

comment:13 by Claude Paroz, 8 years ago

Patch needs improvement: unset

Pull request updated after committing the M2M part.

comment:14 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:15 by James Pic, 8 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 Tim Graham, 8 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 Collin Anderson, 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 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.

Last edited 8 years ago by Collin Anderson (previous) (diff)

comment:18 by Tim Graham, 8 years ago

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

comment:19 by Tim Graham <timograham@…>, 8 years ago

In 67d98441:

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

comment:20 by Tim Graham <timograham@…>, 8 years ago

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.

comment:21 by Tim Graham, 8 years ago

Patch needs improvement: set
Summary: isinstance checks on ManyToManyField should be replaced with field.many_to_manyisinstance checks on ForeignKey/ManyToManyField should be replaced with field.many_to_one/field.many_to_many
Triage Stage: Ready for checkinAccepted

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 Tim Graham, 8 years ago

Owner: Andriy Sokolovskiy removed
Status: assignednew

comment:23 by Tim Graham <timograham@…>, 8 years ago

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.

comment:24 by Tim Graham <timograham@…>, 8 years ago

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

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