Opened 2 years ago

Closed 10 months ago

#21554 closed Bug (fixed)

incorrect SQL generated when using multiple inheritance

Reported by: pegler Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: multiple-inheritance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


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

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 

Attachments (1) (1.0 KB) - added by pegler 2 years ago.

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by pegler

comment:1 Changed 2 years ago by mitchell

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I've also stumbled onto this one today.

I've made a gist of a rudimentary module with a quick and dirty monkey patch for Django 1.6.

I'm new to Django so don't feel comfortable submitting a proper patch.

Last edited 2 years ago by mitchell (previous) (diff)

comment:2 Changed 2 years ago by pegler

I can confirm that patch works on 1.6.

comment:3 Changed 2 years ago by timo

  • Triage Stage changed from Unreviewed to Accepted

I haven't investigated but the report looks legitimate. Obviously a proper patch with tests will help get this fixed.

comment:4 Changed 2 years ago by pegler

  • Has patch set

I packaged up mitchell's patch into a pull request here:

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)

comment:5 Changed 2 years ago by charettes

  • Patch needs improvement set

I added comments on PR. Apart from the minor suggested changes the patch looks RFC; full test suite passes on SQLite3 Py2.

comment:6 Changed 2 years ago by pegler

Thanks for reviewing. I made the suggested changes. Tests continue to pass fully.

comment:7 Changed 2 years ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 38e24d680d28b92997def9ab46a961d09bb81dce:

Fixed #21554 -- Incorrect SQL generated when using multiple inheritance.

comment:8 Changed 2 years ago by mitchell

  • Resolution fixed deleted
  • Status changed from closed to new

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:

comment:9 Changed 2 years ago by timo

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

comment:10 Changed 10 months ago by Tim Graham <timograham@…>

In 7eb513ab:

Refs #21554 -- Added some assertions to a model_inheritance_regress test.

comment:11 Changed 10 months ago by timgraham

  • Resolution set to fixed
  • Status changed from new to closed

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!

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