Opened 6 years ago

Closed 6 years ago

#29893 closed Bug (wontfix)

Unexpected behavior of DetailView on a model defining __len__() returning False

Reported by: Max Owned by: Max
Component: Generic views Version: 2.1
Severity: Normal Keywords:
Cc: Max Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (4)

comment:1 by Max, 6 years ago

Cc: Max added
Owner: changed from nobody to Max
Status: newassigned

comment:2 by Simon Charette, 6 years ago

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.

comment:3 by Max, 6 years ago

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.

Version 0, edited 6 years ago by Max (next)

comment:4 by Tim Graham, 6 years ago

Resolution: wontfix
Status: assignedclosed

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.

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