Fix for nodes_closure migration when node name matches parent

[canismarko]

Modifies the alembic migration that builds the initial nodes_closure table to handle a catalog with edge cases. See the linked issue for details.

Fixes:

• Failure in migrating database when upgrading past Tiled v0.1.0b32 #1114

Checklist

• Add a Changelog entry
• Add the ticket number which this PR closes to the comment section

[canismarko]

Need to update so that SQLite still works.

[canismarko]

This worked on my database:

$ tiled catalog upgrade-database "postgresql+asyncpg://***:***@***/catalog"
Starting migration to add closure table...
Added 'parent' column and foreign key to 'nodes' table.
Created 'nodes_closure' table with uniqueness constraint.
Inserted root node with id=0.
Maximum depth of ancestors found: 3
Initialized parent of each node as 0 and set depth in 'nodes_closure'.
Updated 'parent' column and 'nodes_closure' for depth 1.
Updated 'parent' column and 'nodes_closure' for depth 2.
Updated 'parent' column and 'nodes_closure' for depth 3.
Completed updating 'parent' column recursively.
Inserted self-referential records into 'nodes_closure' for each node.
Updated index in the 'nodes' table.
Created unique constraint on (key, parent) pairs in 'nodes' table.
Dropped 'ancestors' column from the 'nodes' table.
Added triggers to maintain the closure table.
Migration to add closure table completed successfully.

[danielballan]

Thanks for upstreaming this fix, @canismarko! I leave it to @genematx's review.

[genematx]

Thanks for the update, @canismarko! Just to be clear, the node that caused problems has the following path in the catalog: 7e5cd568-6a87-4995-bfcf-81903b0dcd9c/streams/ge_13element/ge_13element, right? (i.e. an array within a container with the same name). I need to double-check if changing the order of operations would fix this, or if there's a deeper issue here.

[canismarko]

@genematx

the node that caused problems has the following path in the catalog: 7e5cd568-6a87-4995-bfcf-81903b0dcd9c/streams/ge_13element/ge_13element, right?

Yes, that's right.

Here is the SQL row: https://gist.github.com/canismarko/4b6244f6ee9bd95b473db20cbb88d78f#file-sql
And what it looks like in a Tiled client: https://gist.github.com/canismarko/4b6244f6ee9bd95b473db20cbb88d78f#file-tiled-client

[genematx]

I think this should fix it, @canismarko. We don't need to change the order of migration steps, but we do need to check if the child has the same name (key) as its parent.
The self-referential rows added in Step 4 are meant to support the closure table logic; I am afraid that when we simply change the order of operations, even though the error is not being raised anymore, the resulting database is not correct with parent and child ids being mixed up.
Thank you again for noticing this and helping to fix it!

[canismarko]

Great, thanks. I'll take a look.

[canismarko]

@genematx I restored to original DB from backup and ran the updated migration you sent. It ran without errors. ✅

I think this is ready to review and/or merge.