[Django] #26998: E013 check is over-zealous
Django
-----------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by aaugustin:
Old description:
> 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.
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by nshafer):
* cc: nate-djangoproject@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Well django-taggit uses private APIs so liberally that I'm reluctant to
blame any "backwards incompatibility" on Django :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:7>
Django
-------------------------------+--------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.10
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => duplicate
Comment:
Okay, then I'll close this as a duplicate of #12203.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:8>
Django
-------------------------------+--------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* status: closed => new
* cc: cmawebsite@… (added)
* resolution: duplicate =>
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:9>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
I don't mind, feel free to submit a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:10>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jonathanmorgan):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:11>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I don't think a test is needed besides possibly reverting some test
updates in the original commit.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:12>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jonathanmorgan):
* cc: jonathan.morgan.007@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:13>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:14>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jonathanmorgan):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:15>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by nshafer):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:16>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:17>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* severity: Normal => Release blocker
Comment:
marking as release blocker because it's a regression from 1.9. feel free
to change if i'm wrong.
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:18>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.10
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:19>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"384f89f8f843953ac11cf211f85291b5c14baeb9" 384f89f8]:
{{{
#!CommitTicketReference repository=""
revision="384f89f8f843953ac11cf211f85291b5c14baeb9"
Fixed #26998 -- Reverted some admin checks from checking
field.many_to_many back to isinstance(field, models.ManyToManyField).
This partially reverts 983c158da7723eb00a376bd31db76709da4d0260
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:20>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jonathanmorgan):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:21>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.10
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"4073780219258eec2d3c5f7b84f4f0a3c3cc6515" 40737802]:
{{{
#!CommitTicketReference repository=""
revision="4073780219258eec2d3c5f7b84f4f0a3c3cc6515"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26998#comment:22>