Opened 12 months ago

Closed 3 weeks ago

#35038 closed Cleanup/optimization (fixed)

Changing violation_error_message on constraints cause a remove/add operation in migration

Reported by: David Owned by: Salvo Polizzi
Component: Migrations Version: 4.1
Severity: Normal Keywords:
Cc: Adrienne Franke Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Changing the violation_error_message of a constraint and running makemigrations causes the migration to be written with two operations RemoveConstraint and AddConstraint .

Suppose we start with this situation:

from django.db import models

class MyModel(models.Model):
    counter = models.IntegerField(defalt=0)
    max_value = models.IntegerField(defalt=0)
    name = models.CharField(max_length=4)

    class Meta:
        constraints = (
            models.CheckConstraint(
                check=models.Q(counter__gte=models.F('max_value')),
                name='counter_lower_than_max',
            ),
            models.UniqueConstraint(
                fields=('name', 'max_value')
                name='uniq_name_and_max',
            ),
        )

If we add a custom violation_error_message and run makemigrations it will output

$ ./manage.py makemigrations mysample -n update_violation_msgs
Migrations for 'mysample':
  mysample/migrations/0002_update_violation_msgs.py
    - Remove constraint counter_lower_than_max from model mymodel
    - Create constraint counter_lower_than_max on model mymodel
    - Remove constraint uniq_name_and_maxfrom model mymodel
    - Create constraint uniq_name_and_maxon model mymodel

This will cause the database tu run useless commands, since nothing on database side has changed!
This may be a particular problem when applied to UniqueConstraints since the database needs to destroy and re-create an index.

Change History (24)

comment:1 by David Sanders, 12 months ago

Triage Stage: UnreviewedAccepted

Thanks for the report 🏆

Aside from running useless commands, dropping & adding constraints on a production database may be undesirable.

comment:3 by Mariusz Felisiak, 12 months ago

Has patch: unset

comment:4 by Salvo Polizzi, 12 months ago

Owner: changed from nobody to Salvo Polizzi
Status: newassigned

comment:5 by Salvo Polizzi, 12 months ago

Owner: Salvo Polizzi removed
Status: assignednew

comment:6 by Nathaniel Conroy, 12 months ago

Owner: set to Nathaniel Conroy
Status: newassigned

comment:7 by Nathaniel Conroy, 11 months ago

Owner: Nathaniel Conroy removed
Status: assignednew

comment:8 by Adrienne Franke, 11 months ago

Owner: set to Adrienne Franke
Status: newassigned

comment:9 by Adrienne Franke, 10 months ago

Cc: Adrienne Franke added
Has patch: set

comment:10 by Mariusz Felisiak, 10 months ago

Needs tests: set
Patch needs improvement: set

Marking as needs improvement pending a discussion.

comment:11 by Shubham Singh Sugara, 4 months ago

Hi team ,
Is anyone working on this i think another issue (ticket ) is also waiting for this to resolve (#35305)
willing to have a crack at it, open to dicussion
old PR closed with this comment on commit , maybe we can start from here , like what is the correct way to implement this patch ?
https://github.com/django/django/pull/17644/commits/fe9fa7ff28abc5a5a5bd70b93a1d8452ec1b5332#r1435873636

comment:12 by Jacob Walls, 4 months ago

I think the latest consensus is that we need a new Operation for AlterConstraint that is documented to be a no-op (emits no SQL) and that the migration framework will produce in these cases in order to avoid dropping and recreating constraints on the database.

comment:13 by Salvo Polizzi, 3 months ago

Owner: changed from Adrienne Franke to Salvo Polizzi

comment:14 by Salvo Polizzi, 3 months ago

Needs documentation: set

comment:15 by Salvo Polizzi, 3 months ago

Needs tests: unset

comment:16 by Salvo Polizzi, 3 months ago

Patch needs improvement: unset

comment:17 by Salvo Polizzi, 3 months ago

Needs documentation: unset

comment:18 by Sarah Boyce, 8 weeks ago

Patch needs improvement: set

comment:19 by Jacob Walls, 6 weeks ago

Patch needs improvement: unset

comment:20 by Sarah Boyce, 6 weeks ago

Patch needs improvement: set

comment:21 by Jacob Walls, 3 weeks ago

Needs tests: set
Patch needs improvement: unset

comment:22 by Jacob Walls, 3 weeks ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:23 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In b92511b4:

Refs #35038 -- Added test for drop and recreation of a constraint.

comment:24 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In b82f8090:

Fixed #35038 -- Created AlterConstraint operation.

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