Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#26998 closed Bug (fixed)

admin.E013 check false positive on django-taggit

Reported by: Aymeric Augustin Owned by: nobody
Component: contrib.admin Version: 1.10
Severity: Release blocker Keywords:
Cc: nate-djangoproject@…, cmawebsite@…, jonathan.morgan.007@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (22)

comment:1 Changed 2 years ago by Aymeric Augustin

Description: modified (diff)

comment:2 Changed 2 years ago by 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.

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

comment:4 Changed 2 years ago by 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.

comment:5 Changed 2 years ago by 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.

comment:6 Changed 2 years ago by Nathan Shafer

Cc: nate-djangoproject@… added

comment:7 Changed 2 years ago by Aymeric Augustin

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

comment:8 Changed 2 years ago by Tim Graham

Resolution: duplicate
Status: newclosed
Summary: E013 check is over-zealousadmin.E013 check false positive on django-taggit

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

comment:9 Changed 2 years ago by Collin Anderson

Cc: cmawebsite@… added
Resolution: duplicate
Status: closednew

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

comment:10 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

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

comment:11 Changed 2 years ago by 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.

comment:12 Changed 2 years ago by Tim Graham

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

comment:13 Changed 2 years ago by Jonathan Morgan

Cc: jonathan.morgan.007@… added

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.

comment:14 Changed 2 years ago by 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

comment:15 Changed 2 years ago by 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

comment:16 Changed 2 years ago by 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

comment:17 Changed 2 years ago by Collin Anderson

Has patch: set

comment:18 Changed 2 years ago by Collin Anderson

Severity: NormalRelease blocker

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

comment:19 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:20 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 384f89f8:

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

This partially reverts 983c158da7723eb00a376bd31db76709da4d0260

comment:21 Changed 2 years ago by 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

comment:22 Changed 2 years ago by 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

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