[Django] #24447: Migrations do not execute create_fk_sql() when adding a foreign key to a field
Django
field
----------------------------+--------------------
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
Migrations failed when introducing foreign keys on existing fields.
Pull request: https://github.com/django/django/pull/4235
Branch with fix:
https://github.com/ganwell/django/tree/master_missing_create_fk_sql
Branch with unittest but no fix:
https://github.com/ganwell/django/tree/master_missing_create_fk_sql_fail
Consider following model:
{{{
class ForeignKeyTest(models.Model):
id = models.AutoField(primary_key=True)
customer = models.IntegerField()
class Customer(models.Model):
id = models.AutoField(primary_key=True)
}}}
Then it gets migrated to this:
{{{
class ForeignKeyTest(models.Model):
id = models.AutoField(primary_key=True)
customer = models.ForeignKey('Customer')
class Customer(models.Model):
id = models.AutoField(primary_key=True)
}}}
The second migration won't create the foreign key. This does not fail with
sqlite! Because these migrations are handled differently. It will fail
with MySQL and probably Postgres too. I wrote a unittest and tested it
with MySQL: master_missing_create_fk_sql_fail fails and
master_missing_create_fk_sql is ok.
The code didn't detect this case - if old_field.rel doesn't exist
alter_field() must always create the foreign key and ONLY if .rel exists
it must check .db_constraint, too. Since no .rel also means there was no
foreign key before.
I will of course redo the pull request to get clean history. I also fixed
this in 1.7.
--
Ticket URL: <https://code.djangoproject.com/ticket/24447>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
field
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 1
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:1>
Django
field
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* version: master => 1.7
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:2>
Django
field
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* cc: MarkusH (added)
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:3>
Django
----------------------------+------------------------------------
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Old description:
New description:
Pull request: https://github.com/django/django/pull/4235
Consider following model:
{{{
class ForeignKeyTest(models.Model):
id = models.AutoField(primary_key=True)
customer = models.IntegerField()
class Customer(models.Model):
id = models.AutoField(primary_key=True)
}}}
Then it gets migrated to this:
{{{
class ForeignKeyTest(models.Model):
id = models.AutoField(primary_key=True)
customer = models.ForeignKey('Customer')
class Customer(models.Model):
id = models.AutoField(primary_key=True)
}}}
The second migration won't create the foreign key. This does not fail with
sqlite! Because these migrations are handled differently. It will fail
with MySQL and probably Postgres too.
The code didn't detect this case: if old_field.rel doesn't exist
alter_field() must always create the foreign key and ONLY if .rel exists
it must check .db_constraint, too. Since no .rel also means there was no
foreign key before.
--
Comment (by ganwell):
Removed redundant information and chose clearer title.
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:4>
Django
Reporter: ganwell | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* stage: Accepted => Ready for checkin
Comment:
RFC when remaining style update is done
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:5>
Django
----------------------------+---------------------------------------------
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Markus Holtermann <info@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f4f0060feaee6bbd76a0d575487682bc541111e4"]:
{{{
#!CommitTicketReference repository=""
revision="f4f0060feaee6bbd76a0d575487682bc541111e4"
Fixed #24447 -- Made migrations add FK constraints for existing columns
When altering from e.g. an IntegerField to a ForeignKey, Django didn't
add a constraint.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:6>
Django
----------------------------+---------------------------------------------
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"1ae2df6bfc3afadd7edb29a5887711f70cdb52c6"]:
{{{
#!CommitTicketReference repository=""
revision="1ae2df6bfc3afadd7edb29a5887711f70cdb52c6"
[1.8.x] Fixed #24447 -- Made migrations add FK constraints for existing
columns
When altering from e.g. an IntegerField to a ForeignKey, Django didn't
add a constraint.
Backport of f4f0060feaee6bbd76a0d575487682bc541111e4 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:7>
Django
----------------------------+---------------------------------------------
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"283b630d6324c936cbe42d8f0169f056b3ba59c6"]:
{{{
#!CommitTicketReference repository=""
revision="283b630d6324c936cbe42d8f0169f056b3ba59c6"
Fixed #24447 -- Made migrations add FK constraints for existing columns
When altering from e.g. an IntegerField to a ForeignKey, Django didn't
add a constraint.
Backport of f4f0060feaee6bbd76a0d575487682bc541111e4 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24447#comment:8>