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):
         """

[Maarten ter Huurne]

Replying to Jacob Walls:

Thank you very much for looking into this further!

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.

The current implementation of model_class() dates back to commit ​ba7206cd from late 2013 (Django 1.7). It preserves the old "return None" behavior that model_class() inherited from apps.get_model(), when apps.get_model() was changed to raise LookupError instead. So changing model_class() to raise LookupError would be consistent with past design decisions, but it seems even 12 years ago it was considered too risky to do that without a deprecation cycle.

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.

I don't think having stale content types in the database should be considered a programming error: it would either be a deployment error (like forgetting to run migrations) or not an error at all. I assumed that removing stale content types was only a way to tidy up the database, not a requirement for correct operation.

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:

Indeed: I don't consider a missing model to be significantly different from a missing instance. However, if that difference would be considered significant, it would still be better to raise a specific and documented exception type instead of AttributeError.

Maybe a compromise is possible by introducing a ModelDoesNotExist exception type that either inherits from ObjectDoesNotExist or both inheriting from the same base class? Then it's possible to catch only one exception type when the difference is relevant or catch both when the difference is irrelevant.

Wouldn't you rather have this feedback (admin crashing) instead of glossing over the data problem?

I'm not convinced yet it is a data problem (see above). But if it is, there are probably more graceful ways to handle it. For example, logging an error or adding a startup system check to detect stale content types.

If the index page on the admin site crashes on one record, all other records on the same page also become inaccessible to admins. We have admins that don't have the knowledge or access to remove stale content types from the database, so they are blocked in their work until someone else fixes the issue.

[Jacob Walls]

Grepping for get_object_for_this_type() in django/contrib/admin, I only get a hit in LogEntry.get_edited_object. Is that the context you found this in? That seems to buttress your case that the admin should gracefully fall back to something. I see the use case now for having stale content types being referred to by admin data.

[Jacob Walls]

Oh, sorry, you mentioned it was just your use of GenericForeignKey that surfaced this.

Could you distill this into a forum post? I'd like to get more input on whether to teach the admin to gracefully handle data for missing models, or whether we'd be removing a meaningful source of developer feedback.