Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25858 closed Bug (fixed)

Deriving from abstract model with foreign key to model in same app broken on Django 1.9

Reported by: Karl Hobley Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.9
Severity: Release blocker Keywords:
Cc: Karl Hobley, Simon Charette 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

As of Django 1.9, it is no longer possible to derive a new model from abstract model in another app if that abstract model has a local foreign key to a model in the same app.

See example project here: https://github.com/kaedroho/djangobugtest

For example, I have two apps: app1 and app2

app1 contains a concrete model and an abstract model. The abstract model has a foreign key to the concrete one (note the Foreign key is defined as a string and doesn't include the app label):

class ConcreteModel(models.Model):
    pass


class AbstractModel(models.Model):
    foreign_key = models.ForeignKey('ConcreteModel')

    class Meta:
        abstract = True

In app2, we have another model that derives from the abstract model in app 1

class DerivedModel(app1.AbstractModel):
    pass

When running makemigrations, the following system check error occurs:

app2.DerivedModel.foreign_key: (fields.E300) Field defines a relation with model 'ConcreteModel', which is either not installed, or is abstract.

For apps upgrading to Django 1.9 and already have migrations, the following error occurs when running tests (after the database has been initialised but before the first test runs. It looks like it's happening while serialising the database):

ValueError: Related model u'ConcreteModel' cannot be resolved

Traceback from an actual site: https://gist.github.com/kaedroho/01b29332308018abfa29

This issue only occurs when the ForeignKey is defined with a string that doesn't contain the app label. So referencing the model class directly or using "app_label.ModelName" works.

Change History (14)

comment:1 Changed 4 years ago by Karl Hobley

Cc: Karl Hobley added

comment:3 Changed 4 years ago by Tim Graham

Cc: Simon Charette added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Not sure if the issue can/should be fixed, but we could at least document the requirement and backwards incompatibility.

comment:4 Changed 4 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

I think we should try fixing this. It can lead to confusing errors if a second ConcreteModel is defined in app2 for example. I'll see if I can come up with a non-invasive patch.

comment:5 Changed 4 years ago by Tim Graham

I guess there's a small design decision about whether or not it could possibly be a useful "feature" to have the option of using the model name only in the ForeignKey to be able to refer to a local model in the app in which the abstract model is actually used and to require an app_label in the ForeignKey otherwise?

comment:6 Changed 4 years ago by Simon Charette

Indeed, I've been trying to reach Carl to talk about it but I guess I should simply write to @developpers.

If we were to consider this change as a feature I guess we should go through a deprecation period.

comment:8 Changed 4 years ago by Simon Charette

Has patch: set

PR based on input from Markus, Shai and Aymeric.

comment:9 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In bc7d201b:

Fixed #25858 -- Bound abstract model application relative relationships.

Thanks to Karl Hobley for the report and Markus, Shai, Aymeric for their input
and Tim for the review.

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

In 27ef6403:

[1.9.x] Fixed #25858 -- Bound abstract model application relative relationships.

Thanks to Karl Hobley for the report and Markus, Shai, Aymeric for their input
and Tim for the review.

Backport of bc7d201bdbaeac14a49f51a9ef292d6312b4c45e from master

comment:12 Changed 4 years ago by Simon Charette

In order to fix #26186 the patch for this ticket is planed to be reverted in Django 1.9.3.

The suggested approach for the issue reported here is to bound your app relative reference to an application (app_label.ConcreteModel in your reported case) because what we initially thought as a new behavior introduced in Django 1.9 has been possible since at least Django 1.2 given the appropriate setup.

In order to support both use cases it was decided that app relative lazy relationships should be resolved against their concrete model's app_label and an explicit app_label should be added to the lazy relationship if the abstract model was meant to be used in a foreign application while maintaining a relationship to a specific application's model.

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

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:14 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