[Django] #36846: ContentType.get_object_for_this_type() does not handle removed models

[Django]

#36846: ContentType.get_object_for_this_type() does not handle removed models
--------------------------+------------------------------------------------
Reporter: mthuurne | Type: Bug
Status: new | Component: contrib.contenttypes
Version: dev | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------+------------------------------------------------
When asked for an object for which the model no longer exists,
`ContentType.get_object_for_this_type()` will raise this exception:
{{{
AttributeError: 'NoneType' object has no attribute '_base_manager'
}}}

The reason for this is that the `model_class()` method will return `None`
when the model cannot be found and the implementation of
`get_object_for_this_type()` does not check for that:
{{{#!python
return self.model_class()._base_manager.using(using).get(**kwargs)
}}}

The behavior I'd expect is that `ObjectDoesNotExist` would be raised,
similar to the situation where the model does exist but the object does
not.

I first noticed this bug in Django 4.2.27, but the latest code in Git has
the same issue.

If raising `ObjectDoesNotExist` is indeed the desired solution, I can
provide a patch for that.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36846: ContentType.get_object_for_this_type() does not handle removed models

-------------------------------------+-------------------------------------
Reporter: Maarten ter Huurne | Owner: Vishy
| Algo
Type: Bug | Status: assigned
Component: | Version: dev
contrib.contenttypes |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Vishy Algo):

* owner: (none) => Vishy Algo
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:1>

[Django]

#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
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: => wontfix
* status: assigned => closed
* type: Bug => Cleanup/optimization

Comment:

Thanks for the proposal. An issue, however, is that the two failures would
become indistinguishable. For instance, we intentionally swallow
`ObjectNotFoundError` in `fetch_one()`:

{{{#!py
try:
rel_obj = ct.get_object_for_this_type(
using=instance._state.db, pk=pk_val
)
except ObjectDoesNotExist:
pass
}}}

... but I doubt we want to intentionally swallow the case where there is a
stale content type.

If `model_class()` let `LookupError` rise instead of returning `None`,
we'd have something nicer, but the method is documented, so we'd need a
deprecation cycle to change it.

Given those reservations, I'm having a hard time seeing the value. If you
think this is worth a deprecation cycle to change, please gather consensus
on the [https://forum.djangoproject.com/c/internals/5 Forum (internals)]
first. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:2>

[Django]

#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]

#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 Jacob Walls):

Thanks for the extra details.

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

This occurred to me as well, but I glossed over it because it felt like a
smell showing that `model_class()` is not doing what it should if we have
to stop delegating to it. So I was trying to weigh doing something that
feels wrong against some concrete added value.

> In what scenario would the caller of get_object_for_this_type() handle
"model does not exist" differently from "instance does not exist"?

Well, at first blush I thought one is an expected error and the other is a
programming error, so we should only catch one. We have a
[https://docs.djangoproject.com/en/6.0/ref/django-admin/#django-contrib-
contenttypes command] to remove stale content types that I would expect to
be run when models are deleted or uninstalled.

Just to be sure all can follow, I sketched out a unit test implementing
your suggestion, it passes on main and fails on your proposal, but I take
it you question the premise of the test:

{{{#!diff
diff --git a/django/contrib/contenttypes/models.py
b/django/contrib/contenttypes/models.py
index 1ae45dea95..71f2655bad 100644
--- a/django/contrib/contenttypes/models.py
+++ b/django/contrib/contenttypes/models.py
@@ -1,6 +1,7 @@
from collections import defaultdict

from django.apps import apps
+from django.core.exceptions import ObjectDoesNotExist
from django.db import models
from django.db.models import Q
from django.utils.translation import gettext_lazy as _
@@ -176,7 +177,9 @@ class ContentType(models.Model):
method. The ObjectNotExist exception, if thrown, will not be
caught,
so code that calls this method should catch it.
"""
- return
self.model_class()._base_manager.using(using).get(**kwargs)
+ if (model_class := self.model_class()) is None:
+ raise ObjectDoesNotExist
+ return model_class._base_manager.using(using).get(**kwargs)

def get_all_objects_for_this_type(self, **kwargs):
"""
diff --git a/tests/contenttypes_tests/test_fields.py
b/tests/contenttypes_tests/test_fields.py
index 76830c3af3..d772a3d717 100644
--- a/tests/contenttypes_tests/test_fields.py
+++ b/tests/contenttypes_tests/test_fields.py
@@ -2,6 +2,7 @@ import json

from django.contrib.contenttypes.fields import GenericForeignKey
from django.contrib.contenttypes.prefetch import GenericPrefetch
+from django.contrib.contenttypes.models import ContentType
from django.db import models
from django.test import TestCase
from django.test.utils import isolate_apps
@@ -26,6 +27,26 @@ class GenericForeignKeyTests(TestCase):
):
field.get_content_type()

+ def
test_generic_relation_distinguishes_missing_object_from_missing_model(self):
+ question = Question.objects.create(text="test")
+ answer = Answer.objects.create(question=question)
+
+ answer.question.delete()
+ self.assertIsNone(answer.question)
+
+ # Start over.
+ question = Question.objects.create(text="test")
+ answer = Answer.objects.create(question=question)
+
+ # Make the content type stale.
+ ct = ContentType.objects.get_for_model(question)
+ ct.model = "badquestion"
+ ct.save()
+
+ answer = Answer.objects.get(pk=answer.pk)
+ with self.assertRaises(Exception):
+ answer.question
+
def test_get_object_cache_respects_deleted_objects(self):
question = Question.objects.create(text="Who?")
post = Post.objects.create(title="Answer", parent=question)
}}}

Wouldn't you rather have this feedback (admin crashing) instead of
glossing over the data problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:4>

[Django]

#36846: ContentType.get_object_for_this_type() does not handle removed models
-------------------------------------+-------------------------------------
Reporter: Maarten ter Huurne | Owner: Vishy
Type: | Algo

Cleanup/optimization | Status: new

Component: | Version: dev
contrib.contenttypes |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: wontfix =>
* stage: Unreviewed => Accepted
* status: closed => new

Comment:

In other words, I'd be happy to consider a PR that changed
`AttributeError` to `LookupError`, but I don't think I'd catch it in the
admin without a wider discussion.

{{{#!diff
diff --git a/django/contrib/contenttypes/models.py
b/django/contrib/contenttypes/models.py

index 1ae45dea95..cba86ac689 100644

--- a/django/contrib/contenttypes/models.py
+++ b/django/contrib/contenttypes/models.py
@@ -1,6 +1,7 @@
from collections import defaultdict

from django.apps import apps
+from django.core.exceptions import ObjectDoesNotExist
from django.db import models
from django.db.models import Q
from django.utils.translation import gettext_lazy as _

@@ -176,7 +177,10 @@ class ContentType(models.Model):

method. The ObjectNotExist exception, if thrown, will not be
caught,
so code that calls this method should catch it.
"""
- return
self.model_class()._base_manager.using(using).get(**kwargs)
+ if (model_class := self.model_class()) is None:

+ # Raise the LookupError.
+ return apps.get_model(self.app_label, self.model)

+ return model_class._base_manager.using(using).get(**kwargs)

def get_all_objects_for_this_type(self, **kwargs):
"""
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:5>