Re: [Django] #36846: ContentType.get_object_for_this_type() raises inappropriate exception type for removed models

[Django]

#36846: ContentType.get_object_for_this_type() raises inappropriate exception type
for 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
-------------------------------------+-------------------------------------
Comment (by Maarten ter Huurne):

Replying to [comment:4 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
[https://github.com/django/django/commit/ba7206cd81458865bace85914905392291b05829
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
[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.

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36846: ContentType.get_object_for_this_type() raises inappropriate exception type
for 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
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:8>

[Django]

#36846: ContentType.get_object_for_this_type() raises inappropriate exception type
for 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
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36846#comment:9>