Bug Report: RenameField and RenameModel operations don't work

[ThomasAitken]

First of all, thank you again for this library, and thank you for your recent changes to support MySQL. A follow up on my earlier issue is that I ended up creating a version of this where I alter Django's default dependency graph so that pre-deploy migrations can always be run on their own, even if out of sequence. I am not planning to submit a pull request for this, although please let me know if you are interested in this concept and I can explain more (it relies on the concept that a pre-deploy migration need never actually rely on a post-deploy migration).

Anyhow, this issue is a bug report

First bug

This is a bug that causes migrations containing RenameField and (I suspect) RenameModel operations to fail completely.

Steps to reproduce:

1. Rename a field
2. Run makemigrations
3. Confirm field renaming in questioning flow
4. Select one of the two latter options

Now try to plan/run the newly generated migration using python manage.py migrate. You get an error trace like this:

  File "/Users/admin/ClipsalRepos/helios/clippy/migrations/0156_rename_nem3_state_tariffrate_nem4_state.py", line 7, in <module>
    class Migration(migrations.Migration):
  File "/Users/admin/ClipsalRepos/helios/clippy/migrations/0156_rename_nem3_state_tariffrate_nem4_state.py", line 14, in Migration
    syzygy.operations.RenameField(),
  File "/Users/admin/Library/Caches/pypoetry/virtualenvs/helios-EL_XmWmL-py3.10/lib/python3.10/site-packages/syzygy/operations.py", line 309, in __init__
    self.stage = kwargs.pop("stage")
KeyError: 'stage'

Commentary

Line 120 in autodetector.py is meant to prevent this: operation = RenameField.for_stage(operation, stage) by re-initialising the operation class with the appropriate stage

But the generated migration uses a new instance of this class, so this line does nothing.

Second Bug

The questioner flow uses 0-based indexing for the choices but it should be 1-based indexing because they're displayed as 1, 2, 3, 4.

Apologies if I somehow misunderstood something with regard to either of these two bugs

[charettes]

Thanks for your report @ThomasAitken! There are effectively serialization issues with StagedOperation subclasses which I'm unsure how they flew under the radar for so long. I have a patch almost ready but I want to add some tests as well to make sure things don't regress.

There's is effectively a off-by-one error in the questioning logic as well. I wonder if something changed on the Django side though as I'm surprised this also was broken.

[charettes]

@ThomasAitken #54 should be addressing both of your rename related issues. LMK if it resolves them.

As for the the pre-deploy migrations can always be run on their own I'd be curious to see it discussed in a different thread as I believe there might be soundness issues with that? There has been some improvements on this front though in 30b348b

[charettes]

Solved by #54.

[ThomasAitken]

@ThomasAitken #54 should be addressing both of your rename related issues. LMK if it resolves them.

As for the the pre-deploy migrations can always be run on their own I'd be curious to see it discussed in a different thread as I believe there might be soundness issues with that? There has been some improvements on this front though in 30b348b

Regarding soundness issues with allowing pre-deploy migrations to run on their own, there are definitely some possible issues of this kind. For example, suppose Migration 1 contains a single AddField operation with a database default (a pre-deploy migration), Migration 2 contains a RemoveField operation for that same field (a post-deploy migration) and Migration 3 is identical to Migration 1. In my current implementation, this fails. Migration 3 will depend only on Migration 1, and if they are run in the expected order (1 then 3), then Migration 3 will error out from trying to create a duplicate column. More importantly, it's clearly not the developer intention in this case to run Migration 2 last.

My current implementation (which suffers the above issue) builds the altered dependency graph as follows:

1. A new pre-deploy migration depends on the most recent pre-deploy migration in the app, whether a leaf or not.

2. A new post-deploy migration depends on the current leaves in the app.

I used "leaves" plural for the second condition because this new proposal does give rise to the possibility of two leaves in a single app (a pre-deploy leaf and a post-deploy leaf), but I have made a slight adjustment to the detect_conflicts method of the MigrationLoader so that it allows this particular scenario.

As indicated, this implementation fails in the above scenario I described. I think the solution would be to alter the first condition in the above proposal as follows:

1. A new pre-deploy migration X depends on the most recent pre-deploy migration in the app Y, whether a leaf or not, unless one of the operations in X updates a resource that is also updated in a post-deploy migration Z that has been created after Y. Then X depends on Z instead (and we are forced to do a deployment before executing X).

I think this would not be a typical scenario so it would still be worthwhile to create separate pre- and post-deploy life cycles. With that said, I'm not sure how trivial it will be to implement this amendment.

I have not created a discussion thread because this is still in the experimental phase. But that was my thought dump.