ContentType.get_object_for_this_type() raises inappropriate exception type for removed models

[Maarten ter Huurne]

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:

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.

[Jacob Walls]

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

            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 ​Forum (internals) first. Thanks.

[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:

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.

[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 ​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:

django/contrib/contenttypes/models.py

@@ -1 +1 @@
 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 +177 @@
         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):
         """

tests/contenttypes_tests/test_fields.py

@@ -2 +2 @@
 
 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 +27 @@
         ):
             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?

[Jacob Walls]

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.

django/contrib/contenttypes/models.py

@@ -1 +1 @@
 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 +177 @@
         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):
         """