[Django] #21554: incorrect SQL generated when using multiple inheritance
Django
----------------------------------------------+----------------------------
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords: multiple-
Triage Stage: Unreviewed | inheritance
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
----------------------------------------------+----------------------------
Our application makes use of multiple-inheritance and we've just come
across an interesting bug. There is a test case attached and it fails
against the latest 1.4, 1.5, 1.6, and master versions. Though the reason
changes between versions 1.5 and 1.6.
The models:
{{{
class Person(models.Model):
name = models.CharField(max_length=50)
class Politician(models.Model):
politician_id = models.AutoField(primary_key=True)
title = models.CharField(max_length=50)
class Congressman(Person, Politician):
state = models.CharField(max_length=2)
class Senator(Congressman):
pass
}}}
The statement {{{ Senator.objects.get(politician_id=1) }}} produces
incorrect SQL for all versions, but is different between versions.
1.4.10 and 1.5.5 result in the error "DoesNotExist: Senator matching query
does not exist." due to it trying to join demo_politician onto
demo_senator via demo_senator.congressman_ptr_id instead of
demo_politician.politician_id
{{{
SELECT "demo_person"."id", "demo_person"."name", T5."politician_id",
T5."title", "demo_congressman"."politician_ptr_id",
"demo_congressman"."person_ptr_id", "demo_congressman"."state",
"demo_senator"."congressman_ptr_id"
FROM "demo_senator"
INNER JOIN "demo_congressman" ON ("demo_senator"."congressman_ptr_id" =
"demo_congressman"."person_ptr_id")
INNER JOIN "demo_person" ON ("demo_senator"."congressman_ptr_id" =
"demo_person"."id")
INNER JOIN "demo_politician" T5 ON ("demo_senator"."congressman_ptr_id" =
T5."politician_id")
WHERE "demo_congressman"."politician_ptr_id" = 1
}}}
1.6 and master results in the error "OperationalError: no such column:
demo_congressman.politician_id". It incorrectly thinks that politician_id
is on demo_congressman instead of demo_politician.
{{{
SELECT "demo_person"."id", "demo_person"."name",
"demo_congressman"."politician_id", "demo_congressman"."title",
"demo_congressman"."politician_ptr_id",
"demo_congressman"."person_ptr_id", "demo_congressman"."state",
"demo_senator"."congressman_ptr_id"
FROM "demo_senator"
INNER JOIN "demo_congressman" ON ( "demo_senator"."congressman_ptr_id" =
"demo_congressman"."person_ptr_id" )
INNER JOIN "demo_person" ON ( "demo_congressman"."person_ptr_id" =
"demo_person"."id" )
WHERE "demo_congressman"."politician_ptr_id" = 1
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21554>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: multiple- | Unreviewed
inheritance | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by mitchell):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I've also stumbled onto this one today.
I've made [https://gist.github.com/mitchellrj/7792290 a gist] of a
rudimentary module with a quick and dirty monkey patch for Django 1.6.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:1>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: multiple- | Unreviewed
inheritance | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by pegler):
I can confirm that patch works on 1.6.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:2>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* stage: Unreviewed => Accepted
Comment:
I haven't investigated but the report looks legitimate. Obviously a proper
patch with tests will help get this fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:3>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I packaged up mitchell's patch into a pull request here:
https://github.com/django/django/pull/2034
I don't know enough about Django to know the implications of making that
change to get_default_columns, but I guess that's why I don't have that
commit bit. The test suite did pass after the change:
{{{
Ran 6295 tests in 325.761s
OK (skipped=419, expected failures=8)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:4>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multiple- | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
I added comments on PR. Apart from the minor suggested changes the patch
looks RFC; full test suite passes on SQLite3 Py2.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:5>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by pegler):
Thanks for reviewing. I made the suggested changes. Tests continue to
pass fully.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:6>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"38e24d680d28b92997def9ab46a961d09bb81dce"]:
{{{
#!CommitTicketReference repository=""
revision="38e24d680d28b92997def9ab46a961d09bb81dce"
Fixed #21554 -- Incorrect SQL generated when using multiple inheritance.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:7>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
Test needs to be extended to test that the title is correct. Currently the
patch replaces all fields from non-primary classes with the ID. Also, the
second base was not being joined to the query correctly.
Second pull request here: https://github.com/django/django/pull/2059
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:8>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: multiple- | Needs documentation: 0
inheritance | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
I left comments for improvement on PR. Please uncheck "Patch needs
improvement" when you update it, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:9>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: multiple- | Triage Stage: Accepted
inheritance |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"7eb513ab0f761fe88aa4d3579fdf8706741f2239" 7eb513ab]:
{{{
#!CommitTicketReference repository=""
revision="7eb513ab0f761fe88aa4d3579fdf8706741f2239"
Refs #21554 -- Added some assertions to a model_inheritance_regress test.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:10>
Django
Reporter: pegler | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: multiple- | Triage Stage: Accepted
inheritance |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
I guess this issue has since been fixed (unless the regression tests in
the second pull request don't capture the issue). Please open a new ticket
if issues remain, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/21554#comment:11>