Fix migrations so that migrate knox zero is possible #379
Conversation
|
Hey 👋 The only thing that should be done to this PR is to add a reverse_code function to the corresponding migration, right? I’m generally confused here so please help me clarify the intent 😊 |
| @@ -4,6 +4,22 @@ | |||
| from django.db import migrations, models | |||
|
|
|||
|
|
|||
| def populate_salt_field_with_dummy_data(apps, schema_editor): | |||
There was a problem hiding this comment.
This here is a function that when reverting the salt field migration and it gets created, this function populates it with just the index of the row so that it is not null
There was a problem hiding this comment.
The function is never used in the file and and you are modifying a migration that most users of the lib will have run already, meaning they will never benefit from this code.
There was a problem hiding this comment.
Sorry, yes forgot to use it, I just updated the code. this code is meant to run when they try to undo the migrations not when running from the beginning, they will benifit from this code when they try to run "manage.py migrate knox zero" as shown in issue #368
| token.save(update_fields=["salt"]) | ||
|
|
||
|
|
||
| def delete_all_rows(apps, schema_editor): |
There was a problem hiding this comment.
Since the digest field is the primary key of the tokens it is read only and I cannot override it, so when the reverse migrations happen here it transform the max_length property on the digest field from 128 to 64, which will trigger a integrity error when it finds inside the cell a values that exceeds 64 in length. so the only way it runs smoothly is to delete all rows
| @@ -19,6 +35,6 @@ class Migration(migrations.Migration): | |||
| migrations.AlterField( | |||
| model_name='authtoken', | |||
| name='salt', | |||
| field=models.CharField(unique=True, max_length=16), | |||
| field=models.CharField(max_length=16, null=True), | |||
There was a problem hiding this comment.
The salt field is deleted, when the migration is reverted it looks for the field in migration files and recreates it, I don't want the field to be recreated with unique=True because when it gets all populated with null it will also trigger an Integrity error, so I removed unique=True and added null=True so it defaults to null without a problem
| @@ -3,6 +3,18 @@ | |||
| from django.db import migrations, models | |||
|
|
|||
|
|
|||
| def clear_token_keys(apps, schema_editor): | |||
There was a problem hiding this comment.
Same thing here the token_key field max_length property gets shrinked in reversing the migrations from 25 to 8 which trigger Integrity Error, so i clear it in all rows.
|
Hello, |
You need to consider the case where someone initialized the database without your change and then tries to revert with your changes. Pretty sure it will not work. Also I’m against modifying an existing migration’s “forward” change - this is a massive breaking change for existing users. |
|
I need to have another look when I get on my computer but something is bothering me 😊 Thanks for the details already ! |
|
Sure no problem, and if you need we can schedule a meet call to discuss it. |
Following this issue #368 I wanted to help since it was not possible to reverse migrations especially when there is rows in the database