#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 <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/0107019b9e0c3d20-2faebdcc-8b3b-425b-9b43-c0aa36c45ab6-000000%40eu-central-1.amazonses.com.
