[Django] #29893: Unexpected behavior of DetailView on a model defining __len__() returning False
Django
False
-----------------------------------------+------------------------
Reporter: burrscurr | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
== Description
When using Django's DetailView on a model defining a {{{__len__()}}}
method, the model object is not always added to context. This is the case,
when {{{__len__()}}} of the model object returns zero. The behavior is
caused by {{{BaseDetailView.get()}}} calling
{{{SingleObjectMixin.get_context_data()}}} which checks like this, if
{{{self.object}}} was set before by{{{SingleObjectMixin.get_object()}}}:
{{{
if self.object:
# adds self.object to context
}}}
Because this check is happening implicitly and not against {{{None}}},
pythons default implementation calling {{{self.object.__len__()}}}
([https://docs.python.org/3/library/stdtypes.html#truth-value-testing])
causes this condition to evaluate to False, at least if {{{__len__()}}}
returns zero. This leads to not having the wanted object in context data.
== Steps to reproduce:
Create an empty model defining a {{{__len__}}} method returning zero and a
DetailView like this:
{{{
class TestModel(models.Model):
def __len__(self):
return 0
class TestModelDetailView(DetailView):
model = TestModel
template_name = 'test.html' # some template
}}}
After creating a TestModel object, the described behavior can be
reproduced when calling the Detailview (url pattern like this
{{{/test/<int:pk>/}}}).
== Proposed solution
Simply checking if self.object is the default value None prevents this not
easy to find bug.
{{{SingleObjectMixin.get_queryset()}}} also has a similar check
{{{
if self.model:
# some action
}}}
which does not seem to cause problems, because {{{self.model}}} is a class
and no object. It should be considered to also check against the default
value for self.model here, which is None.
--
Ticket URL: <https://code.djangoproject.com/ticket/29893>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
False
Reporter: Max | Owner: Max
Type: Bug | Status: assigned
Component: Generic views | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Max):
* cc: Max (added)
* owner: nobody => Max
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29893#comment:1>
Django
False
Reporter: Max | Owner: Max
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I guess adding an `is None` would do for this case but I wouldn't be
surprised if a lot of other cases still break. For example, there's
probably a few model forms or admin APIs that do something along `if
self.instance` and would need to be adjusted as well.
Given defining `__len__()` on a model instance is an edge case and your
issue can be worked around by defining a `def __bool__(self): return True`
I'm tempted to close this issue as _wont fix_ but I'll let other
contributors chime in. I feel like the burden of making sure all APIs
dealing with optional model instances truthiness appropriately is not
worth the small benefit here.
--
Ticket URL: <https://code.djangoproject.com/ticket/29893#comment:2>
Django
False
Reporter: Max | Owner: Max
Type: Bug | Status: assigned
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Max):
I agree that the case of having __len__ defined on a model may be rare,
but i would strongly propose doing the modification of this check.
Although this may not be exhaustive (you mentioned cases like
{{{self.instance}}}), i think it is worth it, because the current
situation leads to highly unexpected behavior, which can be definitely
avoided in this particular case.
--
Ticket URL: <https://code.djangoproject.com/ticket/29893#comment:3>
Django
False
Reporter: Max | Owner: Max
Component: Generic views | Version: 2.1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => wontfix
Comment:
I agree with Simon that trying to adapt the Django ecosystem to this
convention is likely an unreasonable burden. You can write to the
DevelopersMailingList if you want to try to convince the community
otherwise. It would certainly help your argument if you explained why you
want to define `__len__()` as such. Others may suggest a better solution
for the problem you're trying to solve.
--
Ticket URL: <https://code.djangoproject.com/ticket/29893#comment:4>