[Django] #31064: Wrong db migration in case of change PK which in many to many relationship
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
zapililirad |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In my case was:
models.py:
{{{
class Vulnerability(models.Model):
cve_id = models.CharField(max_length=15, primary_key=True)
app = models.ManyToManyField(AppVersion)
class Meta:
managed = True
}}}
Later, i changed cve_id max_length to 100 and did migration:
{{{
operations = [
migrations.AlterField(
model_name='vulnerability',
name='cve_id',
field=models.CharField(max_length=100, primary_key=True,
serialize=False),
),
]
}}}
In result:
cve_id field length was changed, but vulnerability_id field length in
table vulnerability_app remain unchanged
--
Ticket URL: <https://code.djangoproject.com/ticket/31064>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
to.
-----------------------------+------------------------------------
Reporter: zapililirad | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* version: 2.2 => master
* component: Database layer (models, ORM) => Migrations
* stage: Unreviewed => Accepted
Comment:
Thanks I was able to reproduce this issue on SQLite (`ForeignKey`s are not
affected).
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:1>
Django
to.
-----------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dart):
* status: new => assigned
* owner: nobody => Dart
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:2>
Django
to.
-----------------------------+------------------------------------
Reporter: zapililirad | Owner: Dart
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
I have not been able to reproduce this on my machine. Could you kindly
provide me with a minimal code sample, so that I could get a hint of
what's going wrong here?
Replying to [comment:1 felixxm]:
> Thanks I was able to reproduce this issue on SQLite (`ForeignKey`s are
not affected).
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:3>
Django
to.
-----------------------------+------------------------------------
Reporter: zapililirad | Owner: Dart
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Sanskar it's possible the issue is only present on SQLite because it deals
with column alterations in a different way since it doesn't support `ALTER
TABLE`.
The idea is that is you have the follow models
{{{#!python
class Author(models.Model):
pass
class Book(models.Model):
isbn = models.CharField(max_length=10, primary_key=True)
authors = models.ManyToManyField(Author)
}}}
Django will automatically created a `trough` model
{{{#!python
# Automatically created by Django
class Book_Authors(models.Model):
book= models.ForeignKey(Book) # This is a
models.CharField(max_length=10) referring to Book.isbn
author = models.ForeignKey(Author)
}}}
Now the reported issue is that if you If you change `Book.isbn.max_length`
to 13 the `Book_Authors.book` field in the intermediary table that backs
the `Book.authors` many-to-many relationship currently doesn't change to
`max_length=13`.
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/base/schema.py#L757
The re-pointing logic currently lives] in
`BaseDatabaseSchemaEditor._alter_field` but it's possible that it's only
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/sqlite3/schema.py#L348-L365
an issue with SQLite schema editor].
By a quick glance at the code it looks like it could be a SQLite only
issue due to
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/sqlite3/schema.py#L364
this particular line].
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:4>
Django
to.
-----------------------------+------------------------------------
Reporter: zapililirad | Owner: Dart
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
Replying to [comment:4 Simon Charette]:
>
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/base/schema.py#L757
The re-pointing logic currently lives] in
`BaseDatabaseSchemaEditor._alter_field` but it's possible that it's only
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/sqlite3/schema.py#L348-L365
an issue with SQLite schema editor].
>
> By a quick glance at the code it looks like it could be a SQLite only
issue due to
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/sqlite3/schema.py#L364
this particular line].
After I did a bit of digging, it seems like
[https://github.com/django/django/blob/a4881f5e5d7ee38b7e83301331a0b4962845ef8a/django/db/backends/sqlite3/schema.py#L364
this particular line] was indeed the culprit. Instead of calling
`related_objects` on `new_field.model._meta`, if we call
`get_fields(forward=False, reverse=True, include_hidden=True)` on the
same, the `ForeignKey` in the `through` Model changes its type according
to the `ManyToManyField`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:5>
Django
to.
-----------------------------+------------------------------------
Reporter: zapililirad | Owner: Dart
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
Comment:
That makes sense, good old model level cache biting us again. If
`related_objects` gets out of sync it's likely because we forgot to clear
something in `AlterField.state_forwards` and my money is on
`is_referenced_by_foreign_key` not taking into account `ManyToManyField`
defined without an explicit `through`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:6>
Django
to.
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: Dart => Simon Charette
Comment:
Dart I'll assign this to me if you don't mind since I have an idea of how
to address this issue thanks to Sanskar's investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:7>
Django
to.
-----------------------------+------------------------------------------
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
Replying to [comment:6 Simon Charette]:
> That makes sense, good old model level cache biting us again. If
`related_objects` gets out of sync it's likely because we forgot to clear
something in `AlterField.state_forwards` and my money is on
`is_referenced_by_foreign_key` not taking into account `ManyToManyField`
defined without an explicit `through` since that won't create any entry in
`state.models`.
Hey Simon,
I don't know if this helps, but while I was playing around with this
issue, calling `related_objects` on `new_field.model._meta`, always gave
me an empty `ImmutableList`, but calling `_get_fields(forward=False,
reverse=True, include_hidden=True)` gave me an `ImmutableList` of a
`ManyToOneRel`. If we look at
[https://github.com/django/django/blob/master/django/db/models/options.py#L508
related_objects] right now, it only returns Relation objects which are not
hidden or their field's `many_to_many` attribute is `True`. I am not quite
sure whether this is intended or a bug though.
Cheers
Sanskar
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:8>
Django
to.
-----------------------------+------------------------------------------
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:9>
Django
to.
-----------------------------+------------------------------------------
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:10>
Django
to.
-----------------------------+------------------------------------------
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:11>
Django
to.
Reporter: zapililirad | Owner: Simon Charette
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:12>
Django
to.
-----------------------------+---------------------------------------------
Reporter: zapililirad | Owner: Simon Charette
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a54828085732d5d2f020c2fb52fe1cbe1a20a6c9" a5482808]:
{{{
#!CommitTicketReference repository=""
revision="a54828085732d5d2f020c2fb52fe1cbe1a20a6c9"
Fixed #31064 -- Recreated auto-created many-to-many tables on primary key
data type change on SQLite.
Both local and remote auto-created many-to-many relationships were
affected.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31064#comment:13>