fix: add missing columns

[betodealmeida]

SUMMARY

The migration script b56500de1855_add_uuid_column_to_import_mixin.py has a try/except block when adding the uuid column to tables; if the operation fails the script might succeed depending on the error. Because of this, people who have ran the migration are having problems running Superset because of missing columns.

I fixed the script by removing the try/except block, and adding a new script that adds the column wherever necessary.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

1. Ran migration, nothing happened as expected (since all my tables have the uuid column)
2. Downgraded to 070c043f2fdb and deleted the uuid column from the dashboards table
3. Ran migration, and verified that uuid was added to dashboards and populated.

ADDITIONAL INFORMATION

• Has associated issue: Error after install and upgrade of Superset DB apache-superset 1.0.1  #13038
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[ktmud]

LGTM. Was this user-reported? If so, do you mind linking the GitHub issue if it exists?

I think another way to do this is do the column check in add_uuid_column_to_import_mixin.py, then simply re-run the same upgrade again in add_missing_uuid_column.py. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.

[betodealmeida]

LGTM. Was this user-reported? If so, do you mind linking the GitHub issue if it exists?

Good call. I saw the reports on Slack and Stackoverflow, but there's also a ticket which I just added.

I think another way to do this is do the column check in add_uuid_column_to_import_mixin.py, then simply re-run the same upgrade again in add_missing_uuid_column.py. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

[ktmud]

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.

[betodealmeida]

The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.

I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.

Ah, that makes sense. Sorry I misunderstood.

[mistercrunch]

This LGTM.

People do tend to get tangled in "partially executed database migrations", where say if the constraint creation fails (step 2), then step 1 won't run anymore on re-attempts. Then you have people reporting the stack trace from step one failing "column already exist" on subsequent execution, which is a red herring. Solution is to either go in and delete the column in the database manually, or commenting step 1 in the source code, both of which are super confusing for people not super familiar with this type of issues.

Eventually we could have more robust migrations that tend to fall back on their feet more automatically (check if column exists, ...). I don't think that's in-scope for this PR but wanted to point this out.