Lazy operations refactor regression with abstract models #25858

194 views
Skip to first unread message

charettes

unread,
Jan 8, 2016, 4:16:33 PM1/8/16
to Django developers (Contributions to Django itself)
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

Markus Holtermann

unread,
Jan 8, 2016, 5:19:14 PM1/8/16
to django-d...@googlegroups.com
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

Shai Berger

unread,
Jan 9, 2016, 1:52:02 PM1/9/16
to django-d...@googlegroups.com
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.

Aymeric Augustin

unread,
Jan 9, 2016, 4:13:15 PM1/9/16
to django-d...@googlegroups.com
Hello,

On 9 janv. 2016, at 19:51, Shai Berger <sh...@platonix.com> 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.

charettes

unread,
Jan 9, 2016, 6:17:34 PM1/9/16
to Django developers (Contributions to Django itself)
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

charettes

unread,
Feb 9, 2016, 4:33:50 PM2/9/16
to Django developers (Contributions to Django itself)
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

Shai Berger

unread,
Feb 10, 2016, 4:11:43 PM2/10/16
to django-d...@googlegroups.com
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

charettes

unread,
Feb 10, 2016, 4:29:52 PM2/10/16
to Django developers (Contributions to Django itself)
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

Alex Hill

unread,
Feb 10, 2016, 10:27:40 PM2/10/16
to Django developers (Contributions to Django itself)
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

skyjur

unread,
Feb 12, 2016, 6:45:47 AM2/12/16
to Django developers (Contributions to Django itself)
Would love seeing Option 2. I've been relying on this pattern since as far back as django 1.2 on several projects.

charettes

unread,
Feb 15, 2016, 3:42:04 PM2/15/16
to Django developers (Contributions to Django itself)
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

Alexander Hill

unread,
Feb 15, 2016, 8:19:15 PM2/15/16
to django-d...@googlegroups.com
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

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@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.

charettes

unread,
Feb 22, 2016, 5:16:58 PM2/22/16
to Django developers (Contributions to Django itself)
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
Reply all
Reply to author
Forward
0 new messages