Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20448 closed Cleanup/optimization (wontfix)

Make Model.__repr__() safe

Reported by: Patryk Zawadzki Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


__repr__ is mainly used in the debugging context and it calling __unicode__ can yield unwanted consequences such as executing a database query. I also propose having it include the primary key of the object so it's easy to track the record in the database. __unicode__ is not really suitable for lookup, even if it returns contents of an actual database field in most cases it will be an unindexed column.

Please note that while having a more verbose __repr__ can be useful, you can always overload this method in your models.

There is a pull request on GitHub that implements __repr__ the following way:




Attachments (1)

20448.diff (863 bytes) - added by Preston Holmes 3 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 3 years ago by Marc Egli

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

For example this would make an additional query:

class Author(models.Model):
    name = models.CharField(max_length=50)

    def __unicode__(self):

class Book(models.Model):
    author = models.ForeignKey(Author)
    title = models.CharField(max_length=50)

    def __unicode__(self):
        return u"%s - %s" % (self.title,

Calling the the __repr__ method on Book will make an additional query to the database.

comment:2 Changed 3 years ago by Patryk Zawadzki

To clarify: is this bug really in Accepted state? It seems to be a misclick given the feedback on the devel mailing list.

comment:3 Changed 3 years ago by Karen Tracey

Resolution: wontfix
Status: newclosed

I agree with Anssi said on the mailing list, I think this should stay as-is. repr being "unsafe" seems to be an exceptional condition to me, and catering to that unusual case by removing the informative data you'll generally get from the current implementation strikes me as a step backwards for the normal case.

comment:4 Changed 3 years ago by Preston Holmes

Triage Stage: AcceptedUnreviewed

I'm also in agreement with Anssi and Karen, even looking to see if the docs could clarify this I didn't see a huge opportunity. I think the current documentation of __unicode__ being used by Django " in a number of places" already implies that you should expect this to be called at any time from anywhere. As pointed out in the ticket description, if you have a need to remove this behavior, you can always override __repr__ in an abstract base model class.

Perhaps the issues around both model and queryset repr could be mentioned in the shell docs?

I'm quite open to making this more clear in:

and have take a brief stab at it.

Changed 3 years ago by Preston Holmes

Attachment: 20448.diff added

comment:5 Changed 3 years ago by Patryk Zawadzki

The problem is that most often you don't control the model in question. If reusable apps are expected to override this then it should be documented.

The way I feel about this is that having the PK displayed instead is not only safe but also more useful. We work with databases containing tens of millions of rows and lookup by things like title is out of question even if __unicode__ is safe.

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