Re: Lazy operations refactor regression with abstract models #25858
Hi everyone, I submited a PR[1] that partially reverts the initial fix and document how app relative lazy relationships behave when defined on abstract models subclassed as concrete models in foreign applications. Reviews welcome! Simon [1] https://github.com/django/django/pull/6178 Le mercredi 10 février 2016 16:29:52 UTC-5, charettes a écrit : > > I should have mentioned that this behavior is reproducible since at least > Django 1.6 and has not been introduced by Django 1.8. I wouldn't be > surprised > if it has always been working before the fix was introduced. > > Still, as you mentionned the conditions required to achieve this were > really > convoluted before the lazy operations refactor. > > Simon > > Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : >> >> On Tuesday 09 February 2016 23:33:50 charettes wrote: >> > Hi everyone, >> > >> > The chosen fix[1] unfortunately introduced a new regression[2]. >> > >> > It looks like the behavior described in the previous ticket was >> possible >> > with >> > Django 1.8 under certain circumstances[3] where the abstract models >> defined >> > in a foreign application were derived as concrete models in another >> > applications >> > before the abstract models relations are resolved (if they are ever >> > resolved): >> > >> >> The explanation is complex enough, the case where it works edgy enough, >> that >> I'd be content to call this a bug in 1.8. I'm pretty sure the specifics >> of this >> resolution are not documented, and my sense from reading the ticket is >> that if >> you'd try to document the behavior you'll reach the conclusion that it >> probably isn't intentional. >> >> My 2 cents, >> Shai. >> >> > >> > [1] >> https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 >> > [2] https://code.djangoproject.com/ticket/26186 >> > [3] https://code.djangoproject.com/ticket/26186#comment:8 >> > [4] https://code.djangoproject.com/ticket/24215 >> > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/fa69a047-a210-4a7a-a62c-26298eef4005%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
Hi Simon, Nope, you're not missing anything. I agree with reverting the fix in #25858 and going with approach #2 as outlined in your initial post. Sorry that wasn't clear - I meant to address my response to the thread as a whole, in light of that fix being found to cause another regression. Cheers, Alex On Tue, Feb 16, 2016 at 4:42 AM, charettes wrote: > Hi Alex, thanks for chiming in. > > Just to make sure I understand your second proposed option; I'm having a > hard > time understanding how it's different from simply reverting the fix for > #25858[1]. > > Are you suggesting that the abstract model's foreign key be resolved or the > copy.copy()'ied field attached to the concrete model be. If the former is > ever resolved it will breaks the behavior reported in #26186 as the second > concrete model subclassing the abstract one will point to the first > application > referred model. If only the latter are resolved this will be equivalent to > reverting the fix for #25858[1]. > > Am I missing something? > > Simon > > [1] https://code.djangoproject.com/ticket/25858 > [2] https://code.djangoproject.com/ticket/26186 > > > Le mercredi 10 février 2016 22:27:40 UTC-5, Alex Hill a écrit : >> >> It looks like we agree that this depending on import order is not on, so >> we have no choice but to break behaviour in some cases. >> >> Option 1 (don't allow relative references) removes support for relative >> references in abstract models for everyone. >> >> Option 2 (resolve references relative to the first concrete inheriting >> class) doesn't remove anybody's existing behaviour, just formalises it and >> clarifies how it's specified. The most anybody has to do to keep doing what >> they're doing is add an app label to a reference. >> >> Option 3 (always resolve references relative to the abstract model) makes >> impossible something that is currently possible: resolving relative >> references in the concrete class's app. >> >> I like option 2. Being able to define a "floating" reference to a >> to-be-implemented model in a different app is a useful feature in abstract >> models. >> >> If we can't agree on that, I'd go with option 1. I don't really see >> abstract models as belonging to an app in the same way concrete models do, >> and that's reflected in Django in various ways. They're not recognised as >> models by the app registry, fields in their concrete subclasses belong to >> the inheriting class (unlike in concrete inheritance), we have app label >> placeholders for related names, etc. I see them more as a blueprint or >> template than a model. Options 1 and 2 both reinforce that distinction, >> option 3 muddies it a bit. >> >> Cheers, >> Alex >> >> >> On Thursday, February 11, 2016 at 5:29:52 AM UTC+8, charettes wrote: >>> >>> I should have mentioned that this behavior is reproducible since at least >>> Django 1.6 and has not been introduced by Django 1.8. I wouldn't be >>> surprised >>> if it has always been working before the fix was introduced. >>> >>> Still, as you mentionned the conditions required to achieve this were >>> really >>> convoluted before the lazy operations refactor. >>> >>> Simon >>> >>> Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : On Tuesday 09 February 2016 23:33:50 charettes wrote: > Hi everyone, > > The chosen fix[1] unfortunately introduced a new regression[2]. > > It looks like the behavior described in the previous ticket was possible > with > Django 1.8 under certain circumstances[3] where the abstract models defined > in a foreign application were derived as concrete models in another > applications > before the abstract models relations are resolved (if they are ever > resolved): > The explanation is complex enough, the case where it works edgy enough, that I'd be content to call this a bug in 1.8. I'm pretty sure the specifics of this resolution are not documented, and my sense from reading the ticket is that if you'd try to document the behavior you'll reach the conclusion that it probably isn't intentional. My 2 cents, Shai. > > [1] https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 > [2] https://code.djangoproject.com/ticket/26186 > [3] https://code.djangoproject.com/ticket/26186#comment:8 > [4] https://code.djangoproject.com/ticket/24215 >>> -- > You received this message because you are subscribed to a topic in the > Google Groups "Django developers (Contributions to Django itself)" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/django-developers/jRut8aYEet0/unsubscribe > . > To unsubscribe from this group and all its topics, send an email to > django-developers+unsubscr...@googlegroups.com. > To post to this group, send email to django-developers@googlegroups.com. > Visit this group at https://groups.google
Re: Lazy operations refactor regression with abstract models #25858
Hi Alex, thanks for chiming in. Just to make sure I understand your second proposed option; I'm having a hard time understanding how it's different from simply reverting the fix for #25858[1]. Are you suggesting that the abstract model's foreign key be resolved or the copy.copy()'ied field attached to the concrete model be. If the former is ever resolved it will breaks the behavior reported in #26186 as the second concrete model subclassing the abstract one will point to the first application referred model. If only the latter are resolved this will be equivalent to reverting the fix for #25858[1]. Am I missing something? Simon [1] https://code.djangoproject.com/ticket/25858 [2] https://code.djangoproject.com/ticket/26186 Le mercredi 10 février 2016 22:27:40 UTC-5, Alex Hill a écrit : > > It looks like we agree that this depending on import order is not on, so > we have no choice but to break behaviour in some cases. > > Option 1 (don't allow relative references) removes support for relative > references in abstract models for everyone. > > Option 2 (resolve references relative to the first concrete inheriting > class) doesn't remove anybody's existing behaviour, just formalises it and > clarifies how it's specified. The most anybody has to do to keep doing what > they're doing is add an app label to a reference. > > Option 3 (always resolve references relative to the abstract model) makes > impossible something that is currently possible: resolving relative > references in the concrete class's app. > > I like option 2. Being able to define a "floating" reference to a > to-be-implemented model in a different app is a useful feature in abstract > models. > > If we can't agree on that, I'd go with option 1. I don't really see > abstract models as belonging to an app in the same way concrete models do, > and that's reflected in Django in various ways. They're not recognised as > models by the app registry, fields in their concrete subclasses belong to > the inheriting class (unlike in concrete inheritance), we have app label > placeholders for related names, etc. I see them more as a blueprint or > template than a model. Options 1 and 2 both reinforce that distinction, > option 3 muddies it a bit. > > Cheers, > Alex > > > On Thursday, February 11, 2016 at 5:29:52 AM UTC+8, charettes wrote: >> >> I should have mentioned that this behavior is reproducible since at least >> Django 1.6 and has not been introduced by Django 1.8. I wouldn't be >> surprised >> if it has always been working before the fix was introduced. >> >> Still, as you mentionned the conditions required to achieve this were >> really >> convoluted before the lazy operations refactor. >> >> Simon >> >> Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : >>> >>> On Tuesday 09 February 2016 23:33:50 charettes wrote: >>> > Hi everyone, >>> > >>> > The chosen fix[1] unfortunately introduced a new regression[2]. >>> > >>> > It looks like the behavior described in the previous ticket was >>> possible >>> > with >>> > Django 1.8 under certain circumstances[3] where the abstract models >>> defined >>> > in a foreign application were derived as concrete models in another >>> > applications >>> > before the abstract models relations are resolved (if they are ever >>> > resolved): >>> > >>> >>> The explanation is complex enough, the case where it works edgy enough, >>> that >>> I'd be content to call this a bug in 1.8. I'm pretty sure the specifics >>> of this >>> resolution are not documented, and my sense from reading the ticket is >>> that if >>> you'd try to document the behavior you'll reach the conclusion that it >>> probably isn't intentional. >>> >>> My 2 cents, >>> Shai. >>> >>> > >>> > [1] >>> https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 >>> > [2] https://code.djangoproject.com/ticket/26186 >>> > [3] https://code.djangoproject.com/ticket/26186#comment:8 >>> > [4] https://code.djangoproject.com/ticket/24215 >>> >> -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/b4d36464-1b16-441f-a45e-f8c5ec31f9ac%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
Would love seeing Option 2. I've been relying on this pattern since as far back as django 1.2 on several projects. https://github.com/skyjur/django-ticketing/blob/master/ticket_26186_2/tox.ini https://github.com/skyjur/django-ticketing/blob/master/ticket_26186_2/test-result.txt On Thursday, 11 February 2016 03:27:40 UTC, Alex Hill wrote: > > It looks like we agree that this depending on import order is not on, so > we have no choice but to break behaviour in some cases. > > Option 1 (don't allow relative references) removes support for relative > references in abstract models for everyone. > > Option 2 (resolve references relative to the first concrete inheriting > class) doesn't remove anybody's existing behaviour, just formalises it and > clarifies how it's specified. The most anybody has to do to keep doing what > they're doing is add an app label to a reference. > > Option 3 (always resolve references relative to the abstract model) makes > impossible something that is currently possible: resolving relative > references in the concrete class's app. > > I like option 2. Being able to define a "floating" reference to a > to-be-implemented model in a different app is a useful feature in abstract > models. > > If we can't agree on that, I'd go with option 1. I don't really see > abstract models as belonging to an app in the same way concrete models do, > and that's reflected in Django in various ways. They're not recognised as > models by the app registry, fields in their concrete subclasses belong to > the inheriting class (unlike in concrete inheritance), we have app label > placeholders for related names, etc. I see them more as a blueprint or > template than a model. Options 1 and 2 both reinforce that distinction, > option 3 muddies it a bit. > > Cheers, > Alex > > > On Thursday, February 11, 2016 at 5:29:52 AM UTC+8, charettes wrote: >> >> I should have mentioned that this behavior is reproducible since at least >> Django 1.6 and has not been introduced by Django 1.8. I wouldn't be >> surprised >> if it has always been working before the fix was introduced. >> >> Still, as you mentionned the conditions required to achieve this were >> really >> convoluted before the lazy operations refactor. >> >> Simon >> >> Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : >>> >>> On Tuesday 09 February 2016 23:33:50 charettes wrote: >>> > Hi everyone, >>> > >>> > The chosen fix[1] unfortunately introduced a new regression[2]. >>> > >>> > It looks like the behavior described in the previous ticket was >>> possible >>> > with >>> > Django 1.8 under certain circumstances[3] where the abstract models >>> defined >>> > in a foreign application were derived as concrete models in another >>> > applications >>> > before the abstract models relations are resolved (if they are ever >>> > resolved): >>> > >>> >>> The explanation is complex enough, the case where it works edgy enough, >>> that >>> I'd be content to call this a bug in 1.8. I'm pretty sure the specifics >>> of this >>> resolution are not documented, and my sense from reading the ticket is >>> that if >>> you'd try to document the behavior you'll reach the conclusion that it >>> probably isn't intentional. >>> >>> My 2 cents, >>> Shai. >>> >>> > >>> > [1] >>> https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 >>> > [2] https://code.djangoproject.com/ticket/26186 >>> > [3] https://code.djangoproject.com/ticket/26186#comment:8 >>> > [4] https://code.djangoproject.com/ticket/24215 >>> >> -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/114a2c06-95da-49cc-a6ba-3ecfb877b293%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
It looks like we agree that this depending on import order is not on, so we have no choice but to break behaviour in some cases. Option 1 (don't allow relative references) removes support for relative references in abstract models for everyone. Option 2 (resolve references relative to the first concrete inheriting class) doesn't remove anybody's existing behaviour, just formalises it and clarifies how it's specified. The most anybody has to do to keep doing what they're doing is add an app label to a reference. Option 3 (always resolve references relative to the abstract model) makes impossible something that is currently possible: resolving relative references in the concrete class's app. I like option 2. Being able to define a "floating" reference to a to-be-implemented model in a different app is a useful feature in abstract models. If we can't agree on that, I'd go with option 1. I don't really see abstract models as belonging to an app in the same way concrete models do, and that's reflected in Django in various ways. They're not recognised as models by the app registry, fields in their concrete subclasses belong to the inheriting class (unlike in concrete inheritance), we have app label placeholders for related names, etc. I see them more as a blueprint or template than a model. Options 1 and 2 both reinforce that distinction, option 3 muddies it a bit. Cheers, Alex On Thursday, February 11, 2016 at 5:29:52 AM UTC+8, charettes wrote: > > I should have mentioned that this behavior is reproducible since at least > Django 1.6 and has not been introduced by Django 1.8. I wouldn't be > surprised > if it has always been working before the fix was introduced. > > Still, as you mentionned the conditions required to achieve this were > really > convoluted before the lazy operations refactor. > > Simon > > Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : >> >> On Tuesday 09 February 2016 23:33:50 charettes wrote: >> > Hi everyone, >> > >> > The chosen fix[1] unfortunately introduced a new regression[2]. >> > >> > It looks like the behavior described in the previous ticket was >> possible >> > with >> > Django 1.8 under certain circumstances[3] where the abstract models >> defined >> > in a foreign application were derived as concrete models in another >> > applications >> > before the abstract models relations are resolved (if they are ever >> > resolved): >> > >> >> The explanation is complex enough, the case where it works edgy enough, >> that >> I'd be content to call this a bug in 1.8. I'm pretty sure the specifics >> of this >> resolution are not documented, and my sense from reading the ticket is >> that if >> you'd try to document the behavior you'll reach the conclusion that it >> probably isn't intentional. >> >> My 2 cents, >> Shai. >> >> > >> > [1] >> https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 >> > [2] https://code.djangoproject.com/ticket/26186 >> > [3] https://code.djangoproject.com/ticket/26186#comment:8 >> > [4] https://code.djangoproject.com/ticket/24215 >> > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ac07321f-aa6a-47dc-b0f1-d49ba9b71d1c%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
I should have mentioned that this behavior is reproducible since at least Django 1.6 and has not been introduced by Django 1.8. I wouldn't be surprised if it has always been working before the fix was introduced. Still, as you mentionned the conditions required to achieve this were really convoluted before the lazy operations refactor. Simon Le mercredi 10 février 2016 16:11:43 UTC-5, Shai Berger a écrit : > > On Tuesday 09 February 2016 23:33:50 charettes wrote: > > Hi everyone, > > > > The chosen fix[1] unfortunately introduced a new regression[2]. > > > > It looks like the behavior described in the previous ticket was possible > > with > > Django 1.8 under certain circumstances[3] where the abstract models > defined > > in a foreign application were derived as concrete models in another > > applications > > before the abstract models relations are resolved (if they are ever > > resolved): > > > > The explanation is complex enough, the case where it works edgy enough, > that > I'd be content to call this a bug in 1.8. I'm pretty sure the specifics of > this > resolution are not documented, and my sense from reading the ticket is > that if > you'd try to document the behavior you'll reach the conclusion that it > probably isn't intentional. > > My 2 cents, > Shai. > > > > > [1] > https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 > > [2] https://code.djangoproject.com/ticket/26186 > > [3] https://code.djangoproject.com/ticket/26186#comment:8 > > [4] https://code.djangoproject.com/ticket/24215 > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/cbf4295f-9ee0-4c82-bcd2-af6de8031a7f%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
On Tuesday 09 February 2016 23:33:50 charettes wrote: > Hi everyone, > > The chosen fix[1] unfortunately introduced a new regression[2]. > > It looks like the behavior described in the previous ticket was possible > with > Django 1.8 under certain circumstances[3] where the abstract models defined > in a foreign application were derived as concrete models in another > applications > before the abstract models relations are resolved (if they are ever > resolved): > The explanation is complex enough, the case where it works edgy enough, that I'd be content to call this a bug in 1.8. I'm pretty sure the specifics of this resolution are not documented, and my sense from reading the ticket is that if you'd try to document the behavior you'll reach the conclusion that it probably isn't intentional. My 2 cents, Shai. > > [1] https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6 > [2] https://code.djangoproject.com/ticket/26186 > [3] https://code.djangoproject.com/ticket/26186#comment:8 > [4] https://code.djangoproject.com/ticket/24215
Re: Lazy operations refactor regression with abstract models #25858
Hi everyone,
The chosen fix[1] unfortunately introduced a new regression[2].
It looks like the behavior described in the previous ticket was possible
with
Django 1.8 under certain circumstances[3] where the abstract models defined
in a foreign application were derived as concrete models in another
applications
before the abstract models relations are resolved (if they are ever
resolved):
# abstract_app/models.py
class AbstractReferent(models.Model):
refered = models.ForeignKey('Refered')
class Meta:
abstract = True
# app1/models.py
from abstract_app.models import Referent
class Refered(models.Model):
pass
class Referent(AbstractReferent):
pass
Before Django 1.9.2 `Referent.refered` resolved to `Refered` but since the
fix
is applied it attempts to resolve to `abstract_app.Refered` which never
resolves
to any model and results in a check failure.
In the light of this new report I think we should revert the fix as it makes
this "blue print" feature of abstract models unusable. The only thing Django
1.9's refactor of lazy operations[4] changed was to make this feature less
dependent on import side effects and more reliable.
Instead, I suggest we document how app relative relationships are defined on
abstract models.
Thoughts?
Simon
[1]
https://github.com/django/django/commit/bc7d201bdbaeac14a49f51a9ef292d6312b4c45e
[2] https://code.djangoproject.com/ticket/26186
[3] https://code.djangoproject.com/ticket/26186#comment:8
[4] https://code.djangoproject.com/ticket/24215
Le vendredi 8 janvier 2016 16:16:33 UTC-5, charettes a écrit :
>
> During the refactor of lazy operations[1] abstract models stopped resolving
> their related field model reference by themselves[2][3] in order to prevent
> pending lookup pollution. This was necessary in order to warn the users
> about
> unresolved relationships in a reliable way.
>
> This change introduced a subtle regression[4] when an abstract model with a
> lazy app relative (missing an app_label suffix) relationship is derived as
> a concrete model in another application:
>
> # app1/models.py
>
> class Refered(models.Model):
> pass
>
> class AbstractReferent(models.Model):
> refered = models.ForeignKey('Refered')
>
> class Meta:
> abtract = True
>
> # app2/models.py
>
> from app1.models import AbstractReferent
>
> class Refered(models.Model):
> pass
>
> class Referent(AbstractReferent):
> pass
>
> Now that relationships defined on abstract models are only resolved on
> definition of concrete subclasses the `app2.Referent.refered` points to
> `app2.Refered` when it used to point to `app1.Refered`.
>
> Here are the solutions I had in mind:
>
> 1) Refuse the temptation to guess; raise an exception on `app2.Referent`
> definition about the ambigous relationship and point to resolution options.
>
> 2) As abstract models should really just be considered "placeholders for
> fields"
> consider this new behavior the correct one. This could be considered a new
> feature and a deprecation path would have to be thought of.
>
> 3) Revert to the previous behavior by converting app relative lazy
> relationships
> to the "app_label.Model" form on `RelatedField.contribute_to_class`.
>
> I'd favor the first option over the others because I feel like this is
> really
> an edge case where both behaviors have merit (and a bad pratice i'd like to
> prevent) but the third one is definitely the safest option.
>
> Thanks for your feedback,
> Simon
>
> [1] https://code.djangoproject.com/ticket/24215
> [2]
> https://groups.google.com/d/msg/django-developers/U5pdY-WVTes/lqfv9cm9bPAJ
> [3] https://github.com/django/django/pull/4115
> [4] https://code.djangoproject.com/ticket/25858
>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/3e766894-44fd-4180-9cdf-f3ee55a9a4ca%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
Thanks for your input everyone. You convinced me we should with the third option[1] for now. Markus, nothing prevent us from changing this behavior in the future if there's a need for it but since this is a regression and we don't have a clear backward compatible upgrade path defined we should go with the safe alternative for now. Simon https://github.com/django/django/pull/5964 Le samedi 9 janvier 2016 16:13:15 UTC-5, Aymeric Augustin a écrit : > > Hello, > > On 9 janv. 2016, at 19:51, Shai Berger > > wrote: > > > So, I say, go for option 3. A field defined in a model in app1 takes > app- > > relative references in app1. > > I agree. > > We’re discussing an unintended regression. The discussion shows possible > improvements in this area but the correct course of action isn't obvious. > > In such circumstances, we should fix the regression by restoring the > historical behavior. If someone wants to tackle the improvements, it's > always > possible at a later point, after considering the general situation > carefully. > > You may want to leave a comment pointing to these hypothetical > improvements > when you write the patch. > > Best regards, > > -- > Aymeric. > > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2e1d50e6-d13f-4b19-b20d-9dcbec990947%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
Hello, On 9 janv. 2016, at 19:51, Shai Berger wrote: > So, I say, go for option 3. A field defined in a model in app1 takes app- > relative references in app1. I agree. We’re discussing an unintended regression. The discussion shows possible improvements in this area but the correct course of action isn't obvious. In such circumstances, we should fix the regression by restoring the historical behavior. If someone wants to tackle the improvements, it's always possible at a later point, after considering the general situation carefully. You may want to leave a comment pointing to these hypothetical improvements when you write the patch. Best regards, -- Aymeric. -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/690A8FEB-71C5-4D4E-ABED-EED6B5C9532B%40polytechnique.org. For more options, visit https://groups.google.com/d/optout.
Re: Lazy operations refactor regression with abstract models #25858
On Saturday 09 January 2016 00:15:32 Markus Holtermann wrote: > That's a nice one, Simon. > Yep. [Simon said: 1: error out, 2: take model from child's app; 3: take model from abstract parent's app] > tl;dr: I favor 2; am OK with 1 but against 3. > But here I no longer agree. > I was favoring 1) as well. But then thought that app relative relationships > actually make sense and the current behavior adds a nice new API feature. > This way a 3rd party app can provide an abstract model and require you to > implement 2 models: the inheriting and the referred with a specific name. > This makes little sense to me. It makes sense for an abstract model to require the concrete model to have some FK-like attribute -- you do that by using said attribute in the abstract model's methods; looking for a definition of another model specifically is forcing implementation details. Also, there have been sevaral discussions in the past about allowing inheritors to override an abstract model's fields; that feature can be useful, but introducing a vaguely-specified, very partial implementation of it as an afterthought to a different feature does not strike me as a good way to go. I don't like option 1 either -- it is slightly backwards-incompatible, and reeks of "action at a distance" (I am not allowed to name a model in app2 "Refered" because a model by that name exists in app1 and is referred to by a model in app1, which I happen to inherit in app2...). So, I say, go for option 3. A field defined in a model in app1 takes app- relative references in app1. My 2 cents, Shai.
Re: Lazy operations refactor regression with abstract models #25858
That's a nice one, Simon.
tl;dr: I favor 2; am OK with 1 but against 3.
I was favoring 1) as well. But then thought that app relative relationships
actually make sense and the current behavior adds a nice new API feature. This
way a 3rd party app can provide an abstract model and require you to implement
2 models: the inheriting and the referred with a specific name.
Furthermore, if you want a specific model to be used, be explicit about it. Use
the full app.Model name. I think we should emphasize that in our docs and
suggest to use the full name at all times.
The problem we'll be running into with 2 would be a sensible migration path. I
can't think of one right now.
Cheers
/Markus
On January 9, 2016 8:16:32 AM GMT+11:00, charettes wrote:
>During the refactor of lazy operations[1] abstract models stopped
>resolving
>their related field model reference by themselves[2][3] in order to
>prevent
>pending lookup pollution. This was necessary in order to warn the users
>
>about
>unresolved relationships in a reliable way.
>
>This change introduced a subtle regression[4] when an abstract model
>with a
>lazy app relative (missing an app_label suffix) relationship is derived
>as
>a concrete model in another application:
>
># app1/models.py
>
>class Refered(models.Model):
>pass
>
>class AbstractReferent(models.Model):
>refered = models.ForeignKey('Refered')
>
>class Meta:
>abtract = True
>
># app2/models.py
>
>from app1.models import AbstractReferent
>
>class Refered(models.Model):
>pass
>
>class Referent(AbstractReferent):
>pass
>
>Now that relationships defined on abstract models are only resolved on
>definition of concrete subclasses the `app2.Referent.refered` points to
>`app2.Refered` when it used to point to `app1.Refered`.
>
>Here are the solutions I had in mind:
>
>1) Refuse the temptation to guess; raise an exception on
>`app2.Referent`
>definition about the ambigous relationship and point to resolution
>options.
>
>2) As abstract models should really just be considered "placeholders
>for
>fields"
>consider this new behavior the correct one. This could be considered a
>new
>feature and a deprecation path would have to be thought of.
>
>3) Revert to the previous behavior by converting app relative lazy
>relationships
>to the "app_label.Model" form on `RelatedField.contribute_to_class`.
>
>I'd favor the first option over the others because I feel like this is
>really
>an edge case where both behaviors have merit (and a bad pratice i'd
>like to
>prevent) but the third one is definitely the safest option.
>
>Thanks for your feedback,
>Simon
>
>[1] https://code.djangoproject.com/ticket/24215
>[2]
>https://groups.google.com/d/msg/django-developers/U5pdY-WVTes/lqfv9cm9bPAJ
>[3] https://github.com/django/django/pull/4115
>[4] https://code.djangoproject.com/ticket/25858
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/333AF605-822B-46BD-BB2B-CF9040692236%40markusholtermann.eu.
For more options, visit https://groups.google.com/d/optout.
Lazy operations refactor regression with abstract models #25858
During the refactor of lazy operations[1] abstract models stopped resolving
their related field model reference by themselves[2][3] in order to prevent
pending lookup pollution. This was necessary in order to warn the users
about
unresolved relationships in a reliable way.
This change introduced a subtle regression[4] when an abstract model with a
lazy app relative (missing an app_label suffix) relationship is derived as
a concrete model in another application:
# app1/models.py
class Refered(models.Model):
pass
class AbstractReferent(models.Model):
refered = models.ForeignKey('Refered')
class Meta:
abtract = True
# app2/models.py
from app1.models import AbstractReferent
class Refered(models.Model):
pass
class Referent(AbstractReferent):
pass
Now that relationships defined on abstract models are only resolved on
definition of concrete subclasses the `app2.Referent.refered` points to
`app2.Refered` when it used to point to `app1.Refered`.
Here are the solutions I had in mind:
1) Refuse the temptation to guess; raise an exception on `app2.Referent`
definition about the ambigous relationship and point to resolution options.
2) As abstract models should really just be considered "placeholders for
fields"
consider this new behavior the correct one. This could be considered a new
feature and a deprecation path would have to be thought of.
3) Revert to the previous behavior by converting app relative lazy
relationships
to the "app_label.Model" form on `RelatedField.contribute_to_class`.
I'd favor the first option over the others because I feel like this is
really
an edge case where both behaviors have merit (and a bad pratice i'd like to
prevent) but the third one is definitely the safest option.
Thanks for your feedback,
Simon
[1] https://code.djangoproject.com/ticket/24215
[2]
https://groups.google.com/d/msg/django-developers/U5pdY-WVTes/lqfv9cm9bPAJ
[3] https://github.com/django/django/pull/4115
[4] https://code.djangoproject.com/ticket/25858
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/f4afd52b-ff54-42cb-a222-e421a6536c5e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
