In ticket #36846, the central question is what the expected behavior is when ContentType.get_object_for_this_type() is called when the ContentType represents a model that doesn’t exist in the code base anymore.
There is a remove_stale_contenttypes admin command to remove stale ContentType records. The documentation does not mention whether the use of this command is an optional way to clean up the database or a required step to be run on each deployment to ensure correct operation:
- If running
remove_stale_contenttypesis required, I’d argue that there should be a system check on startup that issues a warning if there are stale content types in the database, similar to the warning about migrations that haven’t run. - If running
remove_stale_contenttypesis optional, the question is what should happen at runtime when a stale content type is accessed.
The current behavior is that an AttributeError is raised when a manager attribute is looked up on a None object. This behavior is undocumented and seems accidental rather than designed, so it is probably not the desired behavior. The impact of the current behavior is that the admin site crashes if one record in an overview table is linked to a model that no longer exists via a GenericForeignKey.
The behavior that I expected was for ObjectDoesNotExist to be raised, similar to the situation where the model does exist but the object does not. This exception is caught by the admin site and the result is that - is displayed instead of the linked object.
In the ticket discussion, Jacob Walls mentioned a concern that using the same exception type (ObjectDoesNotExist) for non-existing instances and non-existing models would deprive developers of useful information. The underlying question is whether the removal of the model itself is meaningfully different from the removal of all instances of that model (no table vs empty table).
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.
The unhandled None is returned by ContentType.model_class(), the current implementation of which dates back to commit ba7206cd from late 2013 (Django 1.7). It preserves the old “return None” behavior when apps.get_model() was changed to raise LookupError instead. Having ContentType.model_class() raise LookupError as well would be consistent, but this was considered too invasive of a change even 12 years ago (as evidenced by model_class() retaining the old behavior for compatibility), so it would likely require a deprecation cycle.
To summarize, the discussed behavior options for ContentType.get_object_for_this_type() when called for a model that doesn’t exist anymore are:
- require that the database never contains stale content types
- raise
AttributeError: current behavior, a bug in my opinion - raise
LookupError: the new behavior ofapps.get_model() - raise
ObjectDoesNotExist: handle a missing model like a missing instance - raise
ModelDoesNotExist: introduce a new exception type
Note that ObjectDoesNotExist does not inherit LookupError, even though it is raised when a lookup fails. Maybe it should?
In all cases except forbidding stale content types and raising ObjectDoesNotExist the admin site code would have to be updated to handle the selected behavior.
It would be good to get some more perspectives on what the expected/desired behavior would be.