[Django] #25320: ManyToManyField.null returns True on 1.8, False on 1.7.
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.8
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
According to report on PR
https://github.com/django/django/pull/4241#issuecomment-135492016 we
unintentionally changed the flag's value in 1.8. The null flag for
ManyToManyField isn't that well specified to begin with (does it mean can
the joins generate NULL values (True), or can the returned list be None
(False)?). But I don't see any reason to change the flag's value in 1.8,
so fix for 1.8+ seems appropriate.
--
Ticket URL: <https://code.djangoproject.com/ticket/25320>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> According to report on PR
> https://github.com/django/django/pull/4241#issuecomment-135492016 we
> unintentionally changed the flag's value in 1.8. The null flag for
> ManyToManyField isn't that well specified to begin with (does it mean can
> the joins generate NULL values (True), or can the returned list be None
> (False)?). But I don't see any reason to change the flag's value in 1.8,
> so fix for 1.8+ seems appropriate.
New description:
According to report on PR
https://github.com/django/django/pull/4241#issuecomment-135492016 we
unintentionally changed the flag's value in 1.9. The null flag for
ManyToManyField isn't that well specified to begin with (does it mean can
the joins generate NULL values (True), or can the returned list be None
(False)?). But I don't see any reason to change the flag's value in 1.9,
so fix for the upcoming 1.9 release seems appropriate.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:1>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by tomchristie):
I've amended affected version on this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:2>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.8 => master
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:3>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:4>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
If the flag is reverted then it seems we need to
[https://github.com/django/django/pull/4241/files#diff-
44f2b5094911a07c146e3a7d43a0fde6L215 add back complicated logic] in the
admin list filters to get the tests passing. I'll take a closer look, but
I thought maybe this would remind Anssi why he added the flags in the
first place.
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:5>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by tomchristie):
Cheap hack example implementation, if we did want to stick with `null =
False`...
Have a private `_to_many = True` on these cases - then easy to get the
same switching logic as is now used in master.
`if field.null or getattr(field, '_to_many', False)`
I assume there'd be nicer ways to get the right switching behavior, but
just presenting this as a "it doesn't have to be difficult" example.
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:6>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
See the [https://github.com/django/django/pull/5202 pull request] for the
test failures. If we remove `ForeignObjectRel.null` (it isn't present in
1.8) instead of changing it to `False` as done in the PR, we'll get this
traceback:
{{{
======================================================================
ERROR: test_relatedfieldlistfilter_reverse_relationships
(admin_filters.tests.ListFiltersTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tim/code/django/tests/admin_filters/tests.py", line 478, in
test_relatedfieldlistfilter_reverse_relationships
changelist = self.get_changelist(request, User, modeladmin)
File "/home/tim/code/django/tests/admin_filters/tests.py", line 252, in
get_changelist
modeladmin.list_select_related, modeladmin.list_per_page,
modeladmin.list_max_show_all, modeladmin.list_editable, modeladmin)
File "/home/tim/code/django/django/contrib/admin/views/main.py", line
78, in __init__
self.queryset = self.get_queryset(request)
File "/home/tim/code/django/django/contrib/admin/views/main.py", line
321, in get_queryset
filters_use_distinct) = self.get_filters(request)
File "/home/tim/code/django/django/contrib/admin/views/main.py", line
135, in get_filters
if spec and spec.has_output():
File "/home/tim/code/django/django/contrib/admin/filters.py", line 180,
in has_output
if self.field.null:
AttributeError: 'ManyToOneRel' object has no attribute 'null'
}}}
(Tom, I'll take a look at your suggestion -- composed this message and
then received your response.)
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:7>
Django
Reporter: akaariai | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
The test failures are because `null=False` hides the option to show
"(None)" in the admin's list filter. Personally, I think `null=True` makes
sense for the affected fields/relations (i.e. I wouldn't make any
changes), but if we must revert, I've created a
[https://github.com/django/django/pull/5202 pull request] using the
approach suggested by Tom.
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:8>
Django
Reporter: akaariai | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"064d4b1cb01cbab0461469254a11c918e6b15cbd" 064d4b1]:
{{{
#!CommitTicketReference repository=""
revision="064d4b1cb01cbab0461469254a11c918e6b15cbd"
Fixed #25320 -- Reverted ManyToManyField.null to False for backwards
compatibility.
Thanks Tom Christie for the report and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25320#comment:9>