#24231 closed Bug (wontfix)
Regression in availability of `_meta.get_field()` before app registry is fully populated
Reported by: | Carl Meyer | Owned by: | nobody |
---|---|---|---|
Component: | Documentation | Version: | 1.8alpha1 |
Severity: | Normal | Keywords: | 1.8-beta |
Cc: | pirosb3 | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
In Django versions prior to 1.8, it was possible to call _meta.get_field(...)
on a model class before the app registry was fully populated -- for instance, in a class_prepared
signal handler that adjusts a model's fields or adds new field(s). (Real-world example: https://github.com/carljm/django-model-utils/blob/master/model_utils/models.py#L54-L94)
In 1.8, with the meta refactor, this now raises an AppRegistryError
.
This change makes sense, because get_field
also gets reverse related fields, and those can't be populated until all models are done loading. But it's still a regression in a reasonable use case (even though the meta API was technically private before, it was de facto public).
I think the issue of "when are these methods safe to call?" should be discussed in the _meta refactor porting guide, because it has changed from 1.7, even for methods like get_field()
that otherwise remain similar.
I also think there should be some public supported, documented API that is safe to use before the app registry is populated, that accesses only fields declared on the local model. Perhaps an API equivalent to what you can currently do with _meta._get_fields(reverse=False)
. (I note that this last is already used several places in Django internally where local fields need to be accessed before the app registry is ready.)
Change History (16)
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
comment:2 by , 10 years ago
Cc: | added |
---|
comment:3 by , 10 years ago
Hi pirosb3!
I agree on the doc changes to clarify which methods require a complete app registry. Not sure about modifying the error - I don't think Options
should re-wrap the AppRegistryError
- I guess the base AppRegistryError
could point to the app-loading docs, but that seems like a separate issue.
Regarding adding new public API, I don't have any problem with using a list comprehension. The problem I have is that there is currently _no_ public API that can be used before the app registry is fully loaded; neither get_field()
nor get_fields()
is usable. The only option AFAICS is _get_fields(reverse=False)
(which is what you use internally as well when you need to introspect before app cache is ready), but that's private API.
You may be right that 99% of developers will never hit this issue, but I am not sure of that. Django provides the class_prepared
signal as public API, and basically any use of class_prepared
will involve working with a model class before the app registry is fully loaded.
More conceptually speaking, it seems wrong to me that there is no public API at all to introspect a single model class on its own terms (i.e. only fields declared on that model class), without relying on the rest of the system.
Actually, I think the local_fields
attribute might cover this need well enough, but it's also currently undocumented/private. So I think maybe just documenting that would take care of it.
comment:4 by , 10 years ago
I've created a pull request (https://github.com/django/django/pull/4006) with my suggested documentation updates. Thoughts?
comment:5 by , 10 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Has patch: | set |
Triage Stage: | Unreviewed → Ready for checkin |
Dan, please take a look, but this looks good to me.
comment:7 by , 10 years ago
FTR, django-mptt is also concerned by this API change: https://github.com/django-mptt/django-mptt/issues/356
comment:8 by , 10 years ago
In IRC, PirosB3 and I agreed on a new forward_fields
cached property which is equivalent to _get_fields(reverse=False)
. I will add that to my PR.
comment:9 by , 10 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
comment:10 by , 10 years ago
Version: | 1.7 → 1.8alpha1 |
---|
comment:11 by , 10 years ago
Keywords: | 1.8-beta added |
---|
Does the new proposed fix for #24146 address this issue?
comment:12 by , 10 years ago
I've spent the past few days wavering on that question myself :-) The proposed fix for #24146 makes get_field()
and get_fields()
usable during AppConfig.ready()
, but not in a class_prepared
signal handler. On the one hand:
- People should generally be using
AppConfig.ready()
rather thanclass_prepared
signal handlers, andclass_prepared
, though documented, is already sort of pseudo-private; the docs say "Django uses this signal internally; it’s not generally used in third-party applications."
So in purely practical terms, I'm not sure there's a strong case for this ticket anymore.
On the other hand, it still seems wrong to me that a single model class provides no public API that can be used to introspect its own declared fields safely, regardless of whether the entire app registry is ready or not.
So I'm still willing to update the PR for this to provide such an API, but if others think it should be closed, I won't argue.
comment:13 by , 10 years ago
Deprecating class_prepared
, as discussed on IRC and suggested in #24313, would eliminate the remaining use case.
To me it feels right to conclude there's no use case for introducing an API that works before the app registry is ready. That would mean the app-loading abstraction is leaking. I'd like to keep that to a minimum :-)
comment:14 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Ok, I'll go with that for now. It seems to effectively be saying "a Django model class is not something you are permitted to work with outside the context of a prepared app registry."
comment:15 by , 10 years ago
Just noting that this change in behaviour is a major PITA for developers of libraries that dynamically generate fields during model construction.
In django-mptt I worked around this via hacks involving iterating superclasses and looking at local_fields
(which is apparently not a public API, but it's the only way to do it in django 1.8)
Code is here, hopefully it will help someone else: https://github.com/django-mptt/django-mptt/commit/b9ecddc7b22330abc073916fd6b35c222ad5538d
comment:16 by , 10 years ago
@craigds is it not an option for django-mptt to delay adding the fields until AppConfig.ready()
instead of doing it immediately on class construction? The wontfixing of this ticket is basically saying that that's what you should be doing instead.
Hi carljm
Thanks for flagging the issue.
We could maybe do the following:
But happy to discuss otherwise!