[Django] #28110: Model inheritance mistakenly associates child fields with parent (when running checks)
Django
running checks)
-------------------------------------+-------------------------------------
Reporter: Matthew | Owner: nobody
Schinckel |
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I have a model structure that generates a `models.E006` error under Django
1.9 onwards:
{{{
from django.db import models
class OrgUnit(models.Model):
name = models.CharField(max_length=128)
class Organisation(OrgUnit):
config = models.TextField(default='{}')
class Region(OrgUnit):
organisation = models.ForeignKey(Organisation, related_name='regions')
class SubRegion(OrgUnit):
region = models.ForeignKey(Region, related_name='sub_regions')
}}}
When running `./manage.py check`, this results in:
{{{
SystemCheckError: System check identified some issues:
ERRORS:
org.Region.organisation: (models.E006) The field 'organisation' clashes
with the field 'organisation' from model 'org.orgunit'.
org.SubRegion.region: (models.E006) The field 'region' clashes with the
field 'region' from model 'org.orgunit'.
System check identified 2 issues (0 silenced).
}}}
However, I believe these are incorrect: the model OrgUnit does not have
the fields listed.
This model structure worked fine in 1.8, but started failing in 1.9 (and
still fails under 1.11).
I have a repository containing a minimal project at
https://bitbucket.org/schinckel/inheritance-test
--
Ticket URL: <https://code.djangoproject.com/ticket/28110>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
* component: Database layer (models, ORM) => Core (System checks)
Comment:
Bisected to 4bc00defd0cf4de3baf90cad43da62e531987459. I think the warning
is correct as there is a `Region.organisation` accessor that your foreign
key is overriding. Using the models you provided but without with foreign
keys:
{{{
u = OrgUnit.objects.create()
o = Organisation.objects.create(orgunit_ptr=u)
r = Region.objects.create(orgunit_ptr=u)
r.organisation == o
}}}
I think the warning's wording must be corrected to something like "The
field 'organisation' clashes with the related accessor 'organisation' from
model 'org.orgunit'." but I'm not certain about this analysis. It might be
possible to continue to allow the overriding you've been doing -- I'm not
sure if a patch is feasible.
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:1>
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
For your situation, I wonder if there's any reason not to make `OrgUnit`
abstract?
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:2>
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Besides the fact that it's existing production code, and using an Abstract
model would change the database structure, there actually is one reason.
A different model has a relationship to OrgUnit, and from the perspective
of that model, it's important that it doesn't need to know which of the
OrgUnit subclasses it relates to (i.e., as long as it relates to _any_
OrgUnit, that's fine).
Having said that, I'm not entirely happy with that model structure, and I
plan at some point to move it to a better structure (I have a similar
setup that allows an arbitrary number of levels of hierarchy, for
instance). However, I can't change that //right now//.
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:3>
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Ah, I think (thanks to your pointer) I understand a little better now:
it's because a parent model always gets an accessor for each of it's
subclasses? Is there a way to change that accessor name, I wonder?
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:4>
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
Hmm. The documentation at
https://docs.djangoproject.com/en/1.11/topics/db/models/#inheritance-and-
reverse-relations suggests that it should already give a more informative
error.
Reading more carefully, that seems to be a M2M relationship to the parent,
however in this case it's an FK to a "sibling" model (even though in this
specific case there is a hierarchy, but I think that's irrelevant).
I think that this error message (or something very similar) should be used
for the situation I managed to find myself in.
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:5>
Django
accessors as fields
--------------------------------------+------------------------------------
Reporter: Matthew Schinckel | Owner: nobody
Component: Core (System checks) | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Paul Kenjora):
This ticket breaks abstraction, user defined fields now clash with
framework defined fields. In addition the framework fields are inferred
not explicit, giving the user no reasonable way to know ahead of time what
the conflict may be, especially for 3rd party models that may end up
clashing. Can we raise the severity of this since it forces a significant
project rewrite ( rename all clashing fields ) from version 1.8 to 1.9+?
I'll continue investigating, for the curious the clash is caught at:
/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-
packages/django/db/models/base.py" [readonly] line 1453
Search for E006 in file. Looks like there is already an exception for id
clash, do we add one for inherited models?
--
Ticket URL: <https://code.djangoproject.com/ticket/28110#comment:6>