Refactor of lazy model operations

[Alex Hill]

I dealt with add_lazy_relation() a few months ago working on Mezzanine and I thought it could use some detangling.

Now that the list of pending operations is stored in the Apps class, it makes sense to put the related methods on that class as well. Running a function when a model is loaded seems an appropriate job for the app registry object.

I've introduced a more generic API, whereby a user-supplied function can be called once any number of models are ready, with those freshly-loaded models as its arguments (plus optional kwargs), a helper function for related models, and the old add_lazy_relation() reimplemented in terms of the new API with a deprecation warning.

New pull request, now targeting master rather than 1.8, at ​https://github.com/django/django/pull/4130

[Collin Anderson]

Hi, Thanks for the patch. It's not super clear to me what problem you're trying to solve.

A test or simpler pull request could make it more clear.

[Alex Hill]

Hey Collin, thanks for having a look at this. I'll try to explain myself a bit better.

add_lazy_relation is essentially "do something with this related field and its two related models when both models are ready". That's just a special case of the more general "do something when all of these n models are ready", so I've reimplemented it as such.

The important part is waiting until models are loaded, and running some code at that time. So I've pulled that part out and made it as generic as possible: you provide a callback and any number of models or model names, and when they're all loaded, your code runs with access to the actual model classes.

I reckon that's a generally useful piece of functionality for Apps to provide. So that's the first part.

The second part is to introduce a new two-model wrapper function, lazy_related_operation, to replace add_lazy_relation itself. A wrapper function like this remains useful as a place to resolve "self" references and model names without app_labels, and for identifying the relevant apps object. There are a few reasons I'd like to deprecate add_lazy_relation:

• it doesn't do what it says - it doesn't add lazy relations at all, the callback you supply does that (or doesn't).
• the argument order is inconsistent. it takes class, field, related_class, callback but the callback's signature has to be field, related_class, class. The API introduced in this patch is consistent with most other partial/lazy functional APIs: function, *args, **kwargs.
• it still requires the field argument, which can be handled by a more general kwargs implementation (or simply by closing over it)
• it's inconsistent with the new underlying API

add_lazy_relation is still there with its existing signature in case anybody's using it outside of Django, just reimplemented using the new API and a deprecation warning.

The test suite passes with this patch in place, but I'd gladly write some more tests that demonstrate the new API in a narrower way.

Alex

[Collin Anderson]

Hi Alex,

Do you have example of why you would need two models? What real-world problem are you trying to solve? :)

I think add_lazy_relations is one of those private APIs that may have a lot of use, so if we're going to change it I just want to make sure it's worth it. :)

Thanks,
Collin

[Alex Hill]

Hey Collin,

Sorry about the delay, I didn't get an email notification of your comment. Thanks for replying.

I use it this in Mezzanine to ​wait on both a user model and a profile model which are both string references. That usage is more defensive than it is strictly necessary, but it demonstrates the point. I leave more creative uses to future generations. :)

I hear you about add_lazy_relation being used outside of Django (I do it). If it is used a lot, I see that as a good reason to clean up and document the API, though with an extended deprecation timeline – pave the cowpaths. I've done a quick search on GitHub but ​most of the results are Django builds in other people's repos, so it's not all that helpful. Perhaps I should poll django-users to get a sense of how widely it's used?

I propose:

• Move the core generic functionality into the Apps registry and reimplement add_lazy_relation accordingly as in this PR.
• Include Apps.lazy_model_operation in the public app registry API and document it ​here.

Then either:

• Scrap the new lazy_model_operation helper and don't deprecate add_lazy_relation at all – simply re-implement it while retaining the existing signature, or
• Proceed as per my plan but push removal of add_lazy_relation back to the next LTS. Possibly go the whole hog and document lazy_model_operation as well.

Now wishing this had come up a bit earlier and snuck into 1.8!

Cheers,
Alex

[Anssi Kääriäinen]

I like the general idea of refactoring the add_lazy_relation functionality. I think we also want to guard against unhandled lazy relations, which seems to be missing from current code. So, +1 from me.

I'm not going to set this as accepted, as I'm not sure if we need this as public API (I'm not against that either, I just don't know).

[Alex Hill]

Hi Anssi,

Thanks for your feedback.

I have made some changes to the patch, notably:

• lazy_related_operation now takes multiple related model arguments.
• I've simplified and removed some now-unnecessary type checks in ​RelatedField.contribute_to_class and ​create_many_to_many_intermediary_model (the latter now takes advantage of the first bullet point)
• I've ​made a change to how SQLite schema alterations work due to a bit of friction in the above. This is a small change in terms of code and I believe for the better, but will want some eyes on it.

Regarding unhandled relations: if someone waits for a model that doesn't exist the signal will never fire. I suppose at the end of the app registry's loading phase would be a good place to check for those. Issuing a warning might be the go, as it's always possible (though unlikely) that the user intends to define a model later...

While looking into this, ran into #21175. Currently abstract models don't fire class_prepared, so lazy operations involving abstract models remain unhandled. In practise it doesn't matter because their concrete children work fine, but it would be worth fixing.

Cheers,
Alex

[Alex Hill]

Actually, it's a bit more complicated than just having abstract models fire class_prepared.

We try get_registered_model to see if a model is ready or not, which will always fail with abstract models because they don't appear in the registry. In the case that the abstract model really isn't ready, the callback will still fire when it's constructed (assuming #21175 is fixed), but only the first time.

Is there a reason the app registry doesn't keep track of abstract models?

Note these inconsistencies are present in Django now, they aren't introduced by this patch. Again in practise it doesn't really matter – in fact you get errors if some of the lazy operations are run with abstract models. However if we want to be able to provide warnings for unhandled lazy operations, we have to sort something out with abstract models or they'll raise false alarms.

Cheers,
Alex

[Simon Charette]

As pointed out by Carl on the mailing list abstract model should only be considered as field and method placeholders.

The real issue here is related fields declared on abstract models that add themselves to the apps _pending_lookups.

See ​this PR.

[Alex Hill]

Yep, after my last post in django-developers I basically did the same thing as you in my pull request, just hadn't updated this ticket yet (​see commit here). Good to see we're thinking along the same lines.

[Alex Hill]

Incidentally, what do you think of #21175 - having abstract models fire class_prepared?

I'm thinking that in keeping with what we're talking about here, maybe they shouldn't.

[Simon Charette]

We're talking about deprecating the class_prepared signal altogether (#24313) so this should closed in the process.

[Alex Hill]

That sounds sensible. Thanks for bringing that ticket to my attention.

[Anssi Kääriäinen]

I'm working on refactoring the way relation fields are implemented in Django (#24317). One part of the work is to store directly the remote parts of fields in the remote model's _meta. This isn't that complex to do when first creating the models, but when doing partial re-renders for migrations we need some way to re-fire the equivalent of class-prepared signal. The ticket contains a larger explanation of what I'm planning to do. The last point in the plan is the troublesome case. Ideas welcome!

[Markus Holtermann]

Hey Anssi,

as part of #24225 and #24264 I re-implemented the model reloading process in PR 4097. For now it recursively looks at a model's references, removes all of them from the app registry and adds them again. In case that might be worth knowing.

[Simon Charette <charette.s@…>]

In 9239f1dda7b94f53d21efb8b5e4d056e24f4e906:

Refs #24215 -- Prevented pending lookup pollution by abstract models.

[Alex Hill]

PR now in much better shape thanks to review from Collin, Anssi, Simon and Carl. Notably:

• I'm no longer using the class_prepared signal. Instead, Apps.do_pending_lookups is called directly at the end of Apps.register_model.
• Apps.lazy_model_operation no longer accepts keyword arguments. Functions passed to it should expect a number of models as positional arguments, and nothing else.
• The lazy_related_operation convenience wrapper still supports keyword arguments, but they're applied to the supplied function with partial before it's passed to Apps.lazy_model_operation.

[Alex Hill]

I've rebased this on master - new pull request at ​https://github.com/django/django/pull/4130

[Alex Hill]

Current status of this issue: ​the main pull request, #4130 has been reviewed extensively but is waiting on ​another pull request, #4122, which makes some changes to SQLite schema alterations.

My last change to the SQLite schema alteration branch explicitly disables FK constraints during schema alterations in order to guarantee the expected behaviour. This change hasn't been reviewed yet; if anybody has a few minutes, I would really appreciate a review: ​https://github.com/django/django/pull/4122

Cheers,
Alex

[Tim Graham]

PR #4122 is merged. I left some minor comments on #4130, but suggest we wait until #4241 to avoid creating more work for Anssi.

[Alex Hill]

PR4241 is in and I've rebased on top of that and squashed.

[Tim Graham <timograham@…>]

In 720ff74:

Fixed #24215 -- Refactored lazy model operations

This adds a new method, Apps.lazy_model_operation(), and a helper function,
lazy_related_operation(), which together supersede add_lazy_relation() and
make lazy model operations the responsibility of the App registry. This
system no longer uses the class_prepared signal.

[Tim Graham <timograham@…>]

In 25c157e4:

Refs #24215 -- Improved error message for unhandled lazy model operations.

[Tim Graham <timograham@…>]

In 7506616f:

Refs #24215 -- Fixed Python 3.5 compatiblity for unhandled lazy ops error.

[Tim Graham <timograham@…>]

In b2ffbb00:

Refs #24215 -- Removed add_lazy_relation() per deprecation timeline.