[Django] #30362: Indexes defined on abstract base classes with conditions result in an invalid state due to `name` being required and having to be unique across the database
Django
invalid state due to `name` being required and having to be unique across
the database
-------------------------------------+-------------------------------------
Reporter: Tim | Owner: nobody
Dawborn |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) | Keywords: inheritance index
Severity: Normal | name
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When specifying a `condition` on an `Index`, one has to provide the `name`
of the index as a string:
https://docs.djangoproject.com/en/2.2/ref/models/indexes/#django.db.models.Index.condition
This results in it being impossible to define an `Index` with a
`condition` as a model inheritance base class, as the `name` of the index
needs to be unique across the whole database, but it will be the same for
all subclasses of the base class.
Example:
{{{
class BaseModel(models.Model):
deleted_at = models.DateTimeField(blank=True, null=True)
class Meta:
abstract = True
indexes = [
Index(fields=['id'], condition=Q(deleted_at__isnull=True),
name='id_nondeleted'),
]
class Foo(BaseModel):
pass
class Bar(BaseModel):
pass
}}}
{{{
(ve)12:55:41 ubuntu@local bugreport$ python manage.py makemigrations myapp
Migrations for 'myapp':
myapp/migrations/0001_initial.py
- Create model Bar
- Create model Foo
- Create index id_nondeleted on field(s) id of model foo
- Create index id_nondeleted on field(s) id of model bar
(ve)12:55:42 ubuntu@local bugreport$ python manage.py migrate myapp
Operations to perform:
Apply all migrations: myapp
Running migrations:
Applying myapp.0001_initial...Traceback (most recent call last):
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 82, in _execute
return self.cursor.execute(sql)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/sqlite3/base.py", line 381, in execute
return Database.Cursor.execute(self, query)
sqlite3.OperationalError: index id_nondeleted already exists
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "manage.py", line 21, in <module>
main()
File "manage.py", line 17, in main
execute_from_command_line(sys.argv)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/__init__.py", line 381, in
execute_from_command_line
utility.execute()
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/__init__.py", line 375, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/base.py", line 323, in run_from_argv
self.execute(*args, **cmd_options)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/base.py", line 364, in execute
output = self.handle(*args, **options)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/base.py", line 83, in wrapped
res = handle_func(*args, **kwargs)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/core/management/commands/migrate.py", line 234, in handle
fake_initial=fake_initial,
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/migrations/executor.py", line 117, in migrate
state = self._migrate_all_forwards(state, plan, full_plan, fake=fake,
fake_initial=fake_initial)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/migrations/executor.py", line 147, in
_migrate_all_forwards
state = self.apply_migration(state, migration, fake=fake,
fake_initial=fake_initial)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/migrations/executor.py", line 245, in apply_migration
state = migration.apply(state, schema_editor)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/migrations/migration.py", line 124, in apply
operation.database_forwards(self.app_label, schema_editor, old_state,
project_state)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/migrations/operations/models.py", line 744, in
database_forwards
schema_editor.add_index(model, self.index)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/base/schema.py", line 335, in add_index
self.execute(index.create_sql(model, self), params=None)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/base/schema.py", line 137, in execute
cursor.execute(sql, params)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 99, in execute
return super().execute(sql, params)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 67, in execute
return self._execute_with_wrappers(sql, params, many=False,
executor=self._execute)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 76, in _execute_with_wrappers
return executor(sql, params, many, context)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 84, in _execute
return self.cursor.execute(sql, params)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/utils.py", line 89, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/utils.py", line 82, in _execute
return self.cursor.execute(sql)
File "/tmp/django-test/ve/lib/python3.7/site-
packages/django/db/backends/sqlite3/base.py", line 381, in execute
return Database.Cursor.execute(self, query)
django.db.utils.OperationalError: index id_nondeleted already exists
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30362>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
invalid state due to `name` being required and having to be unique across
the database
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Ian Foote, Mads Jensen (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
I don't see a reasonable solution for inheriting partial indexes (or check
constraints, which are also affected). We should document this restriction
for both.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:1>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:2>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Dawborn):
I personally would like to have seen `name` optionally take a callable
which receives the model object, so that the name of the index could be,
for example, prefixed with the class name of the model in the concrete
class. If there's no desire for that sort of functionality, then adding a
check for this situation sounds like a sensible alternative to prevent
others from falling into this trap.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:3>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ian Foote):
A check seems sensible to me, as does documentation. I'm less sure about
the callable idea, but it could work.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:4>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Dawborn):
Replying to [comment:4 Ian Foote]:
> A check seems sensible to me, as does documentation. I'm less sure about
the callable idea, but it could work.
Well, what I'd actually like is to not have to specify the index name, and
let Django construct it like it does index names for Index's that don't
contain conditions. The callable suggestion was a work around for having
to explicitly specify a name.
Can I ask what the motivation for explicitly requiring an index name was
when providing a condition?
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:5>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
The way we currently deal this class of issue for inherited
`ForeignKey(related_name, related_query_name)` is to
[https://docs.djangoproject.com/en/2.2/topics/db/models/#be-careful-with-
related-name-and-related-query-name allow formatting placeholders].
I suggest we do the same here by allowing `%(app_label)s` and `%(class)s`
to be specified in `name` and that a similar check to the `related_name`
conflicts one be added.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:6>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ian Foote):
The motivation for requiring an explicit name is largely because auto-
generating index names caused a lot of pain in the past. The core devs at
the time decided explicit is better than implicit.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:7>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Dawborn):
Replying to [comment:7 Ian Foote]:
> I like Simon's idea of allowing {{{%(app_label)s}}} and {{{%(class)s}}}
in index and constraint names.
Same, assuming these get evaluated at the the lowest concrete class of an
inheritance hierarchy.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:8>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
IMO in Django 2.2 we should document that partial indexes and check
constraints cannot be used in abstract models with many inheriting models;
and in Django 3.0 we can add a new feature proposed by Simon (separate
ticket).
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:9>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Can Sarıgöl (added)
* has_patch: 0 => 1
Comment:
Hi, I tried to add a control for the fix unique index name. as far as I
could see, the problem is not just about abstract models. So my apporach
was that adding the control into model_checks.py. I tried to add the dict
params in name property as well as mentioned by Simon.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:10>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
Comment:
Please see
[https://code.djangoproject.com/ticket/30362?replyto=10#comment:9
comment]. IMO we will not backport a new feature to the 2.2.x release,
proposed patch requires a separate ticket targeted to the master (3.0)
release. You can add a cross reference to the above discussion.
Replying to [comment:10 Can Sarıgöl]:
> Hi, I tried to add a control for the fix unique index name. as far as I
could see, the problem is not just about abstract models. So my apporach
was that adding the control into model_checks.py. I tried to add the dict
params in name property as well as mentioned by Simon.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:11>
Django
models.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: nobody
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Can Sarıgöl):
I want to emphasize, it is not just about abstract models. even so, will
we just document it or can we proceed with the check code?
For new feature I'm waiting for the new ticket after that I'll separate my
pr.
I'm sorry if I misunderstood :)
Replying to [comment:9 felixxm]:
> IMO in Django 2.2 we should document that partial indexes and check
constraints cannot be used in abstract models with many inheriting models;
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:12>
Django
models.
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => felixxm
Comment:
I added two related tickets for master #30396 and #30397.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:13>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11275 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:14>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Component: Database layer | Version: 2.2
(models, ORM) |
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5df3301aab9e1d1c386799263bef5cf013985c83" 5df3301a]:
{{{
#!CommitTicketReference repository=""
revision="5df3301aab9e1d1c386799263bef5cf013985c83"
Fixed #30362 -- Noted partial indexes and constraints restrictions with
abstract base classes.
Thanks Carlton Gibson for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:15>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f24cf51661af21cfe2d90d7cba6557c56778ef20" f24cf51]:
{{{
#!CommitTicketReference repository=""
revision="f24cf51661af21cfe2d90d7cba6557c56778ef20"
[2.2.x] Fixed #30362 -- Noted partial indexes and constraints restrictions
with abstract base classes.
Thanks Carlton Gibson for the review.
Backport of 5df3301aab9e1d1c386799263bef5cf013985c83 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:16>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alan Justino da Silva):
Replying to [comment:8 Tim Dawborn]:
> Replying to [comment:7 Ian Foote]:
> > I like Simon's idea of allowing {{{%(app_label)s}}} and
{{{%(class)s}}} in index and constraint names.
>
> Same, assuming these get evaluated at the the lowest concrete class of
an inheritance hierarchy.
Just hit the issue. Can a user expect it to be available on the future
versions?
I got the point about not adding a new feature to 2.X branch. For the new
versions, however, not having this new feature means code duplication on
users side.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:17>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Alan Justino da Silva (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:18>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
> Just hit the issue. Can a user expect it to be available on the future
versions?
Yes, it's available in Django 3.0+, see #30397.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:19>
Django
classes.
-------------------------------------+-------------------------------------
Reporter: Tim Dawborn | Owner: felixxm
Type: Bug | Status: closed
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: inheritance index | Triage Stage: Accepted
name |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Alan Justino da Silva):
Replying to [comment:19 felixxm]:
> > Just hit the issue. Can a user expect it to be available on the future
versions?
>
> Yes, it's available in Django 3.0+, see #30397.
Amazing. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/30362#comment:20>