Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26186 closed Bug (fixed)

When extending from abstract model, ForeignKey points to wrong application

Reported by: skyjur Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.9
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by skyjur)

This might be related to https://code.djangoproject.com/ticket/25858

I have following code:

app_A/abstract.py:

class AbstractModel1(Model):
    class Meta:
        abstract = True


class AbstractModel2(Model):
    model1 = ForeignKey('Model1')

    class Meta:
          abstract = True

app_A/models.py:

from .abstract import AbstractModel1, AbstractModel12

class Model1(AbstractModel1):
    pass

class Model2(AbstractModel12):
    pass

app_B/models.py:

from app_A.abstract import AbstractModel1, AbstractModel2

class Model1(AbstractModel1):
    pass

class Model2(AbstractModel12):
    pass

in Django 1.8, the app_B.Model2.model1 would point to app_B.Model1, but in Django 1.9.2 it points to app_A.Model1 so my code no longer works.

Test case can be found: https://github.com/skyjur/django-ticketing/tree/master/ticket_26186

Change History (15)

comment:1 Changed 4 years ago by Simon Charette

Resolution: invalid
Status: newclosed

Hi skyjur,

While I can reproduce on Django 1.9 and 1.9.1 (where it was considered a regression #25858) I cannot reproduce on any version of Django 1.8.

I even tried swapping the order of 'app_A' and 'app_B' in INSTALLED_APPS with no success.

You can find the test applications I used in my attempts to reproduce here.

Are you sure you managed to reproduce against Django 1.8? If it's the case please reopen this ticket with more details.

comment:2 Changed 4 years ago by skyjur

I have updated your test case:
https://github.com/charettes/django-ticketing/pull/1

In my app my abstract models are defined in a/abstract.py and then imported into a/models.py, I didn't realise this was important.

comment:3 Changed 4 years ago by skyjur

Description: modified (diff)
Resolution: invalid
Status: closednew

comment:4 Changed 4 years ago by skyjur

Description: modified (diff)

comment:5 Changed 4 years ago by skyjur

I have added another test case showing that in 1.9 it's impossible to extend from abstract models if they contain foreign keys to abstract model that is not inside an installed app.

In 1.8 this was possible:

https://github.com/skyjur/django-ticketing/tree/master/ticket_26186_2

tox output:

django189 runtests: commands[0] | django-admin test
Creating test database for alias 'default'...
.
----------------------------------------------------------------------
Ran 1 test in 0.000s

OK
Destroying test database for alias 'default'...
django192 runtests: commands[0] | django-admin test
Creating test database for alias 'default'...
Traceback (most recent call last):
    <...>
    self._related_fields = self.resolve_related_fields()
  File "django-ticketing/ticket_26186/.tox/django192/local/lib/python2.7/site-packages/django/db/models/fields/related.py", line 567, in resolve_related_fields
    raise ValueError('Related model %r cannot be resolved' % self.remote_field.model)
ValueError: Related model u'None.Model1' cannot be resolved
ERROR: InvocationError: 'django-ticketing/ticket_26186_2/.tox/django192/bin/django-admin test'
ERROR: InvocationError: 'django-ticketing/ticket_26186_2/.tox/django192/bin/django-admin test'
______________summary__________________________ __________
  django189: commands succeeded
ERROR:   django192: commands failed

comment:6 Changed 4 years ago by Tim Graham

Does it raise a deprecation warning in 1.8? There was a deprecation that ended in 1.9that may be relevant -- from the release notes: "All models need to be defined inside an installed application or declare an explicit app_label." To confirm this, can you bisect to the commit in Django where the behavior changed?

comment:7 Changed 4 years ago by Simon Charette

In your example both app_B.Model2.model1 and app_A.Model2.model1 are pointing to app_B.Model1 because app_B's models module was imported first (because of the order of INSTALLED_APPS) and triggered the import of app_A.abstract.

No version of Django allowed you to make Model2 point to their respective Model1. The only thing that changed in Django 1.9.2 is that lazy related references defined on abstract models are now bound to the application in which they are defined.

comment:8 Changed 4 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

Well I stand corrected. It looks like the reported behavior is possible as long as the application containing the abstract module is not yet imported.

Here's what happens on Django 1.8.

The b application is imported by the app loading mechanism and triggers and the import of a.abstract. At this point the abstract models are not bound to any application and thus the AbstractModel2.model1 field is not resolved to any model which allows the b.Model2.model1 field to resolves to b.Model1. When the a application is loaded the abstract models are bound to it (because they are defined in a a submodule) and thus a.Model2 automatically resolves to a.Model1.

In this case if the a application is loaded before the b application b.Model2 will point to a.Model1 because the abstract models were bound to the a application when b.models will import a.abstract (which was imported by a.models first).

Here's what happen before #25858 was fixed (1.9, 1.9.1):

App relative lazy relationships defined on abstract models were never resolved thus the behavior described above if the b application appeared before the a one on Django 1.8 was always present and not dependent on ÌNSTALLED_APPS ordering.

Here's what happen since #25858 is fixed (1.9.2 , master):

It's not possible anymore as app relative lazy relationships defined on abstract models are bound to the application in which they are defined.

I'll take this to the ML again to gather feedback but I'm inclined to close this as wontfix because the behavior was only achievable in 1.8 through import side effects.

comment:9 Changed 4 years ago by Simon Charette

Posted to the mailing list.

I suggested we revert the fix for #25858 and instead document how app relative relationships defined on
abstract models are resolved.

comment:10 Changed 4 years ago by Simon Charette

Has patch: set

Here's a PR with the suggested changes.

comment:11 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Left comments for improvement.

comment:12 Changed 4 years ago by Simon Charette

Patch needs improvement: unset

comment:13 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:14 Changed 4 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 0223e213:

Fixed #26186 -- Documented how app relative relationships of abstract models behave.

This partially reverts commit bc7d201bdbaeac14a49f51a9ef292d6312b4c45e.

Thanks Tim for the review.

Refs #25858.

comment:15 Changed 4 years ago by Simon Charette <charette.s@…>

In 48cf7516:

[1.9.x] Fixed #26186 -- Documented how app relative relationships of abstract models behave.

This partially reverts commit bc7d201bdbaeac14a49f51a9ef292d6312b4c45e.

Thanks Tim for the review.

Refs #25858.

Backport of 0223e213dd690b6b6e0669f836a20efb10998c83 from master

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