Opened 3 years ago

Last modified 17 months 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: master
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 Changed 3 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:2 Changed 3 years ago by Andriy Sokolovskiy

Owner: changed from nobody to Andriy Sokolovskiy
Status: newassigned

comment:3 Changed 3 years ago by Collin Anderson

Component: UncategorizedDatabase layer (models, ORM)

comment:4 Changed 3 years ago by Markus Holtermann

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 Changed 3 years ago 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.

comment:6 Changed 3 years ago by 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."

comment:7 Changed 20 months ago by Claude Paroz

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 Changed 20 months ago by 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?

comment:9 Changed 20 months ago by 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.

comment:10 Changed 20 months ago by Tim Graham

Patch needs improvement: set

comment:11 Changed 19 months ago by 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.

comment:12 Changed 19 months ago by 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.

comment:13 Changed 19 months ago by Claude Paroz

Patch needs improvement: unset

Pull request updated after committing the M2M part.

comment:14 Changed 19 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:15 Changed 19 months ago by 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',)

comment:16 Changed 19 months ago by 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.

comment:17 Changed 18 months ago by 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/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 18 months ago by Collin Anderson (previous) (diff)

comment:18 Changed 18 months ago by Tim Graham

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

comment:19 Changed 18 months ago by Tim Graham <timograham@…>

In 67d98441:

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

comment:20 Changed 18 months ago by 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.

comment:21 Changed 18 months ago by Tim Graham

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 Changed 18 months ago by Tim Graham

Owner: Andriy Sokolovskiy deleted
Status: assignednew

comment:23 Changed 17 months ago by 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.

comment:24 Changed 17 months ago by 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

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