Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#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 Carl Meyer)

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 Carl Meyer, 10 years ago

Description: modified (diff)

comment:2 by pirosb3, 10 years ago

Cc: pirosb3 added

Hi carljm

Thanks for flagging the issue.

  • Related to docs: You are right! there is nothing stating when this API is ready to use. I think a note should be added.
  • Related to adding another public API: I think that 99% of the developers will have the Apps registry warmed by the time they touch the interpreter, or by the time the server is running. Also, getting a field by name can be done with a simple list comprehension making it close to a one-liner. Do you think this is worth the trouble?

We could maybe do the following:

  • Document better this potential error message, and provide a simple alternative snippet
  • Modify the AppRegistryError to point to the docs, saying there is an alternative.

But happy to discuss otherwise!

comment:3 by Carl Meyer, 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 Carl Meyer, 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 Tim Graham, 10 years ago

Component: Database layer (models, ORM)Documentation
Has patch: set
Triage Stage: UnreviewedReady for checkin

Dan, please take a look, but this looks good to me.

comment:6 by pirosb3, 10 years ago

Hi,

Just wrote a few comments on the PR. But anyways thanks to both!

comment:7 by Claude Paroz, 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 Carl Meyer, 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 Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:10 by Carl Meyer, 10 years ago

Version: 1.71.8alpha1

comment:11 by Tim Graham, 10 years ago

Keywords: 1.8-beta added

Does the new proposed fix for #24146 address this issue?

comment:12 by Carl Meyer, 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 than class_prepared signal handlers, and class_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 Aymeric Augustin, 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 Carl Meyer, 10 years ago

Resolution: wontfix
Status: newclosed

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 Craig de Stigter, 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 Carl Meyer, 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.

Note: See TracTickets for help on using tickets.
Back to Top