#36846: ContentType.get_object_for_this_type() does not handle removed models
-------------------------------------+-------------------------------------
Reporter: Maarten ter Huurne | Owner: Vishy
Type: | Algo
Cleanup/optimization | Status: closed
Component: | Version: dev
contrib.contenttypes |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Maarten ter Huurne):
Maybe I should add some context to explain why I think this is an actual
bug and not an API cleanup proposal:
We have an application where samples can be registered taken from various
kinds of goods. As those goods use different models, we use
`GenericForeignKey` to link the sample to the good being sampled. At some
point, I removed the models for some types of goods that we don't need
anymore, but didn't remove the old samples. Then the admin site (made
using `django.contrib.admin`) started crashing (uncaught exception on page
generation) whenever a sample linking to a removed model was being
displayed in an index table, because the lookup of the `GenericForeignKey`
hit an unhandled `None` model.
When `get_object_for_this_type()` raises `ObjectDoesNotExist`, the admin
site will display "-" instead, which for this use case is the desired
behavior. There is a subtle difference between the situation where all
instances of a model have been removed and the model itself having been
removed, but I'm not sure that distinction is important enough to require
a separate exception. In what scenario would the caller of
`get_object_for_this_type()` handle "model does not exist" differently
from "instance does not exist"?
If you insist on having separate exception types, having
`get_object_for_this_type()` raise `LookupError` without changing
`model_class()` would also be an option, as `model_class()` explicitly
swallows `LookupError`:
{{{#!python
def model_class(self):
"""Return the model class for this type of content."""
try:
return apps.get_model(self.app_label, self.model)
except LookupError:
return None
}}}
So if `get_object_for_this_type()` would call `apps.get_model()` directly
instead of going through `model_class()`, it would raise `LookupError` on
removed models. This would avoid a deprecation cycle. However, the admin
site would also have to be updated then to catch `LookupError` in addition
to `ObjectDoesNotExist`.
In any case, the current behavior of raising `AttributeError` seems like
the worst alternative: it is not the right exception type and it's not
documented either. In my opinion, any of the alternatives
(`get_object_for_this_type()` raising `ObjectDoesNotExist`,
`get_object_for_this_type()` raising `LookupError`, or `model_class()`
raising `LookupError`) would be preferable over keeping the current
behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019b9d8e6dca-4f2a517e-007d-4836-9ac4-417f6d6c75dd-000000%40eu-central-1.amazonses.com.
