admin.E013 check false positive on django-taggit

[Aymeric Augustin]

After upgrading a project to Django 1.10, it failed to start because of a system check:

<class 'blog.admin.PostAdmin'>: (admin.E013) The value of 'fieldsets[2][1]["fields"]' cannot include the many-to-many field 'tags' because that field manually specifies a relationship model.

The corresponding M2M field is managed by django-taggit.

It's declared as follows:

    tags = TaggableManager(
        verbose_name="libellés", blank=True,
        through=TaggedPost, related_name='tag+')

and the manager is just:

class TaggedPost(TaggedItemBase):
    content_object = models.ForeignKey("blog.Post")

I suppose the intent of this check is to prevent developers from using the admin's M2M widget when the through model has extra fields.

However, in my case, the through model doesn't have extra fields, so the check isn't testing the right thing.

This issue didn't happen with Django 1.9 because the test for this check was written slightly differently and django-taggit slipped through the cracks.

It's likely possible to work around this issue with a hack in django-taggit, but it seems to me that this check doesn't use the right logic and could be improved, so I'm filing this ticket.

The corresponding issue django-taggit is ​https://github.com/alex/django-taggit/issues/430.

[Aymeric Augustin]

This is all the more frustrating since TaggableManager — a Field class — implements a custom formfield method to build its admin widget (that's a public API) so a check justified by the admin M2M widget doesn't make sense in any case.

[Tim Graham]

I'd guess 983c158da7723eb00a376bd31db76709da4d0260 is at fault. I was skeptical of the usefulness of that change and don't mind reverting parts of it as needed if an alternative isn't proposed.

[Aymeric Augustin]

Indeed 983c158d is the commit that created the backwards incompatibility for django-taggit. But I think that reverting it would merely address the local symptoms instead of the root cause.

I investigated what invalid condition this check attempts to prevent and what's the most accurate way to test it. The tests for this check point to #12203, which is still open, and also #12237, which is just an extension and which doesn't add much to the present discussion.

Russell correctly pointed out what the most accurate solution would be here: https://code.djangoproject.com/ticket/12203#comment:4. He also described the simpler solution which was eventually implemented, but whose limitations we're hitting now.

Currently #12203 is still open, pending a patch for the more accurate solution. I'm slightly in favor of keeping this ticket open since the history on #12203 is a bit confusing, but we can close it as a duplicate if you prefer.

[Tim Graham]

Do you consider this a regression we should fix for Django 1.10, as far as django-taggit is concerned? Besides addressing #12203 (which might be a bit invasive for a backport), there's the option to revert to isinstance(...., ManyToManyField)until #12203 is fixed.

[Aymeric Augustin]

Well django-taggit uses private APIs so liberally that I'm reluctant to blame any "backwards incompatibility" on Django :-)

[Tim Graham]

Okay, then I'll close this as a duplicate of #12203.

[Collin Anderson]

Hi All, Sorry I'm late to respond on this.

I encouraged the 983c158da7723eb00a376bd31db76709da4d0260 change in hopes of being more liberal in what django _accepts_, but I see now that the many cases the change made django more liberal in what it flags as a problem.

Yes, #12203 is a good new behavior that would solve this problem, but I'd consider the 983c158da7723eb00a376bd31db76709da4d0260 to be a regression, because it breaks things that work fine in 1.9. (Though, yes, you could argue, private APIs, etc.)

I think we should revert the entire checks.py file changes except for admin.E003 and admin.E020. (I think the "must be m2m" checks should allow all many_to_many, and the "can't be m2m" checks should only check ManyToManyField.). That way django allows more custom fields.

(I'm re-opening so this comment doesn't get lost. Feel free to wontfix if we don't want to revert the check.py changes.)

[Tim Graham]

I don't mind, feel free to submit a patch.

[Jonathan Morgan]

I use django-taggit and want it to keep working going forward, and so I am willing to try making a patch for this:

I think we should revert the entire checks.py file changes except for admin.E003 and admin.E020. (I think the "must be m2m" checks should allow all many_to_many, and the "can't be m2m" checks should only check ManyToManyField.). That way django allows more custom fields.

if it is as straightforward as it sounds (revert everything but the changes at lines 107-108 and lines 298-299, then see if that conflicts with other subsequent changes?). I've not contributed before, however, so I might need some guidance as I'm working through the documentation on how to contribute. For example, I don't really understand what is going on here well enough to make a test, and so if a test is needed here as part of the patch, I'm probably not the right person to do this work.

Let me know if you think I can be of help.

[Tim Graham]

I don't think a test is needed besides possibly reverting some test updates in the original commit.

[Jonathan Morgan]

Ok. To start, I'll grab source, run tests, make the change, then run the tests again and see if any fail that passed before. More soon.

[Collin Anderson]

Hi All,

Here's a pull request that I think should fix it. Jonathan or anyone: could you try out my patch?

​https://github.com/django/django/pull/7140

Thanks,
Collin

[Jonathan Morgan]

Happy to test (glad you did this patch rather than I). I created a new virtualenv for testing on my research server and I have it all set up other than django.

What is best way to go about testing? Fork django/django, then install the patch, then install that django from github URL rather than pypi into my virtualenv with all my other stuff? If so, the only thing I'm not familiar with is getting the patch into my fork. If there is a more straightforward way, let me know. And apologies again for not being more up to speed on all this.

Jon

[Nathan Shafer]

Thanks, Collin. I just did a test with your branch and I'm no longer getting the admin.E013 errors on startup. I appreciate your work, and am happy that I'll be able to upgrade to 1.10 now.

Jon: I did this (in my virtualenv with a project I'm using to test just django-taggit with Django):

pip install --upgrade git+https://github.com/collinanderson/django.git@ticket26998#egg=Django

Thank you, everyone. I'm happy to see this fixed!

Nate

[Collin Anderson]

marking as release blocker because it's a regression from 1.9. feel free to change if i'm wrong.

[Tim Graham <timograham@…>]

In 384f89f8:

Fixed #26998 -- Reverted some admin checks from checking field.many_to_many back to isinstance(field, models.ManyToManyField).

This partially reverts 983c158da7723eb00a376bd31db76709da4d0260

[Jonathan Morgan]

I installed django as nshafer directed, and this patch fixed the errors for me, as well.

I also ran the unit tests in my project and they all passed.

So, it looks good to me.

Thanks!

Jon

[Tim Graham <timograham@…>]

In 40737802:

[1.10.x] Fixed #26998 -- Reverted some admin checks from checking field.many_to_many back to isinstance(field, models.ManyToManyField).

This partially reverts 983c158da7723eb00a376bd31db76709da4d0260

Backport of 384f89f8f843953ac11cf211f85291b5c14baeb9 from master