Opened 5 years ago

Closed 5 years ago

#31064 closed Bug (fixed)

Migration doesn't detect precision changes in fields that ManyToMany points to.

Reported by: zapililirad Owned by: Simon Charette
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Simon Charette 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

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

Change History (13)

comment:1 by Mariusz Felisiak, 5 years ago

Component: Database layer (models, ORM)Migrations
Summary: Wrong db migration in case of change PK which in many to many relationshipMigration doesn't detect precision changes in fields that ManyToMany points to.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Thanks I was able to reproduce this issue on SQLite (ForeignKeys are not affected).

comment:2 by Dart, 5 years ago

Owner: changed from nobody to Dart
Status: newassigned

in reply to:  1 comment:3 by Sanskar Jaiswal, 5 years ago

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 felixxm:

Thanks I was able to reproduce this issue on SQLite (ForeignKeys are not affected).

comment:4 by Simon Charette, 5 years ago

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

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

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

The re-pointing logic currently lives in BaseDatabaseSchemaEditor._alter_field but it's possible that it's only 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 this particular line.

in reply to:  4 ; comment:5 by Sanskar Jaiswal, 5 years ago

Replying to Simon Charette:

The re-pointing logic currently lives in BaseDatabaseSchemaEditor._alter_field but it's possible that it's only 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 this particular line.

After I did a bit of digging, it seems like 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.

Last edited 5 years ago by Sanskar Jaiswal (previous) (diff)

comment:6 by Simon Charette, 5 years ago

Cc: Simon Charette added

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.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:7 by Simon Charette, 5 years ago

Owner: changed from Dart to Simon Charette

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.

in reply to:  5 comment:8 by Sanskar Jaiswal, 5 years ago

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

comment:9 by Simon Charette, 5 years ago

Has patch: set
Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:10 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:11 by Simon Charette, 5 years ago

Patch needs improvement: unset

comment:12 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In a5482808:

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.

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