#20448 closed Cleanup/optimization (wontfix)
Make Model.__repr__() safe
Reported by: | Patryk Zawadzki | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
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 |
Description
__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:
`
Foo.objects.create()
Foo(id=1)
`
Attachments (1)
Change History (6)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 11 years ago
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 by , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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 by , 11 years ago
Triage Stage: | Accepted → Unreviewed |
---|
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?
https://docs.djangoproject.com/en/dev/ref/django-admin/#shell
I'm quite open to making this more clear in:
https://docs.djangoproject.com/en/dev/ref/models/instances/#unicode
and have take a brief stab at it.
by , 11 years ago
Attachment: | 20448.diff added |
---|
comment:5 by , 11 years ago
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.
For example this would make an additional query:
Calling the the
__repr__
method onBook
will make an additional query to the database.