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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> 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 description:
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.
Pull request at https://github.com/django/django/pull/3984
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:1>
* cc: cmawebsite@… (added)
* version: 1.8alpha1 => master
Comment:
Hi, Thanks for the patch. It's super clear to me what problem you're
trying to solve.
A test or simpler pull request could make it more clear.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:2>
Comment (by AlexHill):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:3>
Comment (by collinanderson):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:4>
Comment (by AlexHill):
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
[https://github.com/stephenmcd/mezzanine/blob/8e0ff285c9761642cc8544f59b583a75b7821fff/mezzanine/accounts/models.py
wait on both a user model and a profile model] which are both string
references. It's more defensive there than 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. I've done
a quick search on GitHub but
[https://github.com/search?l=python&q=add_lazy_relation&type=Code&utf8=%E2%9C%93
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 [https://docs.djangoproject.com/en/1.7/ref/applications
/#application-registry 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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:5>
Comment (by akaariai):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:6>
Comment (by AlexHill):
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
[https://github.com/django/django/pull/3984/files#diff-
3010fc5a498b7171c342520f34507968R283 RelatedField.contribute_to_class] and
[https://github.com/django/django/pull/3984/files#diff-
3010fc5a498b7171c342520f34507968R2022
create_many_to_many_intermediary_model] (the latter now takes advantage of
the first bullet point)
* I've
[https://github.com/AlexHill/django/commit/d27869e3dcb03eccf7c3aa09d911b5206bda9d0a
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:7>
Comment (by AlexHill):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:8>
Comment (by charettes):
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 [https://github.com/django/django/pull/4115 this PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:9>
Comment (by AlexHill):
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
([https://github.com/AlexHill/django/commit/3e7b896617a9fe1f315b757a45f92a58d707575f
see commit here]). Good to see we're thinking along the same lines.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:10>
Comment (by AlexHill):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:11>
Comment (by charettes):
We're talking about deprecating the `class_prepared` signal altogether
(#24313) so this should closed in the process.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:12>
Comment (by AlexHill):
That sounds sensible. Thanks for bringing that ticket to my attention.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:13>
Comment (by akaariai):
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:14>
Comment (by MarkusH):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:15>
Comment (by Simon Charette <charette.s@…>):
In [changeset:"9239f1dda7b94f53d21efb8b5e4d056e24f4e906"]:
{{{
#!CommitTicketReference repository=""
revision="9239f1dda7b94f53d21efb8b5e4d056e24f4e906"
Refs #24215 -- Prevented pending lookup pollution by abstract models.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:16>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:17>
Comment (by AlexHill):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:18>
Old description:
> 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.
>
> Pull request at https://github.com/django/django/pull/3984
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:19>
Comment (by AlexHill):
I've rebased this on master - new pull request at
https://github.com/django/django/pull/4130
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:20>
Comment (by AlexHill):
Current status of this issue: [https://github.com/django/django/pull/4130
the main pull request, #4130] has been reviewed extensively but is waiting
on [https://github.com/django/django/pull/4122 another pull request,
#4122], which makes some changes to SQLite schema alterations slightly.
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:21>
* needs_better_patch: 0 => 1
Comment:
PR #4122 is merged. I left some minor comments on #4130, but suggest we
wait until #4241 to avoid creating more work for Anssi.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:22>
* needs_better_patch: 1 => 0
Comment:
PR4241 is in and I've rebased on top of that and squashed.
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:23>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"720ff740e70e649a97fcf0232fec132569a52c7e" 720ff74]:
{{{
#!CommitTicketReference repository=""
revision="720ff740e70e649a97fcf0232fec132569a52c7e"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:24>
Comment (by Tim Graham <timograham@…>):
In [changeset:"25c157e4ccc27702883b0c3a53e0e9b44e19757d" 25c157e4]:
{{{
#!CommitTicketReference repository=""
revision="25c157e4ccc27702883b0c3a53e0e9b44e19757d"
Refs #24215 -- Improved error message for unhandled lazy model operations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:25>
Comment (by Tim Graham <timograham@…>):
In [changeset:"7506616f164a0e2a63fd8bf5ca044ddb78f9b22a" 7506616f]:
{{{
#!CommitTicketReference repository=""
revision="7506616f164a0e2a63fd8bf5ca044ddb78f9b22a"
Refs #24215 -- Fixed Python 3.5 compatiblity for unhandled lazy ops error.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:26>
Comment (by Tim Graham <timograham@…>):
In [changeset:"b2ffbb00a5b4526ab51c62a620f819c96a44902c" b2ffbb00]:
{{{
#!CommitTicketReference repository=""
revision="b2ffbb00a5b4526ab51c62a620f819c96a44902c"
Refs #24215 -- Removed add_lazy_relation() per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24215#comment:27>