Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#30362 closed Bug (fixed)

Note partial indexes and constraints restrictions with abstract base classes.

Reported by: Tim Dawborn Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: 2.2
Severity: Release blocker Keywords: inheritance index name
Cc: Ian Foote, Mads Jensen, Can Sarıgöl, Alan Justino da Silva Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (20)

comment:1 by Mariusz Felisiak, 5 years ago

Cc: Ian Foote Mads Jensen added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

I don't see a reasonable solution for inheriting partial indexes (or check constraints, which are also affected). We should document this restriction and add check for both.

Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:2 by Mariusz Felisiak, 5 years ago

Summary: 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 databasePartial indexes and check constraints crash when inherited from abstract models.

comment:3 by Tim Dawborn, 5 years ago

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.

comment:4 by Ian Foote, 5 years ago

A check seems sensible to me, as does documentation. I'm less sure about the callable idea, but it could work.

in reply to:  4 comment:5 by Tim Dawborn, 5 years ago

Replying to 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?

comment:6 by Simon Charette, 5 years ago

The way we currently deal this class of issue for inherited ForeignKey(related_name, related_query_name) is to 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.

comment:7 by Ian Foote, 5 years ago

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.

Version 0, edited 5 years ago by Ian Foote (next)

in reply to:  7 ; comment:8 by Tim Dawborn, 5 years ago

Replying to 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.

comment:9 by Mariusz Felisiak, 5 years ago

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

comment:10 by Can Sarıgöl, 5 years ago

Cc: Can Sarıgöl added
Has patch: set

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.

in reply to:  10 comment:11 by Mariusz Felisiak, 5 years ago

Has patch: unset

Please see 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 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.

in reply to:  9 comment:12 by Can Sarıgöl, 5 years ago

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 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;

comment:13 by Mariusz Felisiak, 5 years ago

Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

I added two related tickets for master #30396 and #30397.

comment:14 by Mariusz Felisiak, 5 years ago

Has patch: set
Summary: Partial indexes and check constraints crash when inherited from abstract models.Note partial indexes and constraints restrictions with abstract base classes.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 5df3301a:

Fixed #30362 -- Noted partial indexes and constraints restrictions with abstract base classes.

Thanks Carlton Gibson for the review.

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In f24cf51:

[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

in reply to:  8 ; comment:17 by Alan Justino da Silva, 4 years ago

Replying to Tim Dawborn:

Replying to 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.

comment:18 by Alan Justino da Silva, 4 years ago

Cc: Alan Justino da Silva added

in reply to:  17 ; comment:19 by Mariusz Felisiak, 4 years ago

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.

in reply to:  19 comment:20 by Alan Justino da Silva, 4 years ago

Replying to 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.

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