[23.1] Add missing TS migration

[jdavcs]

This migration appears to be missing. It was the last migration script on sqlalchemy migrate (https://github.com/galaxyproject/galaxy/pull/14489/files#diff-fa487f1639fd05014a3ec90f54d78393aa5ae32290b65e3a33d746863d2e91d9) which did not get applied. With the move to alembic it got lost.

This migration combines 2 migrations in the galaxy db: e0e3bb173ee6_add_column_deleted_to_api_keys.py and b855b714e8b8_make_api_keys_deleted_non_nullable.py.

This should fix #18242 and the psycopg2.errors.UndefinedColumn error on sentry https://sentry.galaxyproject.org/share/issue/04adfef3d6cc4370bfe2a5bfca671b7c/

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jdavcs]

Failing tests are unrelated, due to Python 3.7.

[mvdbeek]

Have you reviewed the logic, in particular that we're not going to enable API keys that aren't the current API key ? See #15956 (comment) which applies here as well. I think we should either default the deleted column to true and invalidate all old API keys, which is a little disruptive, or have a separate manager class for the TS. I would prefer the latter.

[mvdbeek]

So I'm afraid that we probably need to also mark non-latest api keys as deleted. What I would suggest is something like https://github.com/galaxyproject/galaxy/pull/15956/files#diff-3636e105c5faeac8c33accca40eea93a836d856101d6eebed9bd2a24a32c3c65, but maybe we can be more fine grained and keep the latest api key as deleted = False

[mvdbeek]

I've taken the liberty to implement the deleted logic in the migration, please have a look if that looks OK to you. I've tested this manually on my tool shed instance and I see that only the latest api key is marked as not deleted.

[mvdbeek]

Thank @jdavcs!

[jdavcs]

@mvdbeek thank you - of course I overlooked all that logic!

I think we can simplify the migration (see my latest commit): the column does not exist before we run the revision script, so we can follow these steps:

1. Create column as non-nullable with default=false. That will add the column with deleted=false to all rows.
2. Update all rows to deleted=true. All keys are deleted now.
3. Update the latest user keys to deleted=false.

This way it does exactly what the previous version did, but it doesn't alter the column to set nullability. I've tested this locally, it works as expected.

If this migration looks right, I'll squash the migration changes into one commit to avoid future confusion if we ever dig through this commit history.

By the way, if you test it locally using manage_toolshed_db.sh and a non-default database (not database/community.sqlite), it won't read the config. I think I need to backport #18215 to 23.1.

[mvdbeek]

By the way, if you test it locally using manage_toolshed_db.sh and a non-default database (not database/community.sqlite), it won't read the config. I think I need to backport #18215 to 23.1.

I know, that's how I migrated the tool shed.

Looks good, let's merge, I don't think the tests are telling us anything.

[mvdbeek]

@jdavcs can you handle the forward merge ? I think there's another migration later on so this needs some handling

[jdavcs]

Have you reviewed the logic, in particular that we're not going to enable API keys that aren't the current API key ? See #15956 (comment) which applies here as well. I think we should either default the deleted column to true and invalidate all old API keys, which is a little disruptive, or have a separate manager class for the TS. I would prefer the latter.

Also, re: ee0c72c. I agree that having a separate manager class (and not reusing galaxy's logic here) is a better solution. I think keeping models and db schemas in-sync just for the sake of reusing business logic across 2 apps (even closely related apps) is not great. (i.e., if we must change the db schema in one app to accommodate "generic" business logic that we import from the other, that logic is no longer generic.) I think next time we run into a case like this, before adjusting the model we should try decoupling the logic first.

[jdavcs]

@jdavcs can you handle the forward merge ? I think there's another migration later on so this needs some handling

Done. The migrations were for the galaxy db, which has its own versioning tree, so no special handling was needed (the ts db had only the initial migration). I've added the new revision tags to TS's script in #18271.