Fix nullable deleted column in API Keys table

[davelopez]

By default, the deleted column is nullable even if it has a default value of False.

There is a chance, some external scripts that create API keys for users directly in the database might not set the deleted value explicitly causing possible incongruences with multiple API keys for the user not being explicitly deleted=False.

This restricts those cases by forcing a non-null value.

We could also try to handle possible null values in the API logic, I can change the approach if this is more desirable than altering the database schema at this point.

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.

[mvdbeek]

Doesn't this potentially make old (and thus supposedly inactive) API keys valid ? Can we just fix those external scripts and call it a day?

[mvdbeek]

See also #14489 for context

[nsoranzo]

Would you also need to add , nullable=False to https://github.com/galaxyproject/galaxy/blob/release_23.0/lib/galaxy/model/__init__.py#L9700 ?

Edit: I initially wrote , nullable=True by mistake here!

[davelopez]

Doesn't this potentially make old (and thus supposedly inactive) API keys valid ?

If I remember correctly with #14489, only the latest API that is not deleted will be the valid one, so I guess the only problem will be with a new API key that is deleted=NULL

Can we just fix those external scripts and call it a day?

I'm completely fine with that, ping @hexylena in case she has some opinions.

Would you also need to add , nullable=True to https://github.com/galaxyproject/galaxy/blob/release_23.0/lib/galaxy/model/__init__.py#L9700 ?

Apparently, this is the default when you don't explicitly set the nullable property, so it won't change anything I guess.

[mvdbeek]

Apparently, this is the default when you don't explicitly set the nullable property, so it won't change anything I guess.

I guess @nsoranzo meant that it needs to be changed to nullable=False, no ?

[nsoranzo]

I guess @nsoranzo meant that it needs to be changed to nullable=False, no ?

Indeed, sorry!

[mvdbeek]

only the latest API that is not deleted will be the valid one, so I guess the only problem will be with a new API key that is deleted=NULL

right ... so if an admin set deleted=False on the latest API key your old key (which is now the latest non-deleted one) becomes active ... it just seems like there is too much room for error here. It may not work that way, but the fact that we can't talk about this without tripping up makes me think we should migrate deleted=null to deleted=true in the migration to be on the safe side.

Or not do anything and fix the script.

[mvdbeek]

what happens when you declare a column as non-nullable in the migration but you have null values in the database ? if that only blocks inserting new null values then that would also be a valid option.

[davelopez]

what happens when you declare a column as non-nullable in the migration but you have null values in the database ?

The migration will fail with:

sqlalchemy.exc.IntegrityError: (psycopg2.errors.NotNullViolation) column "deleted" of relation "api_keys" contains null values

Agree, I will change the delete=NULL existing rows to delete=True as suggested, feels much safer, in the worst case the user will need to recreate the API key because the one that may be used was discarded in the migration.

[bgruening]

This probably impacts only EU. If it's all easier, then let's fix this on dev and don't introduce a migration in a stable branch.

[hexylena]

Can we just fix those external scripts and call it a day?

I'm completely fine with that, ping @hexylena in case she has some opinions.

I mean, yea, we can. We are doing that separately. That said, this column allowed nulls which semantically do not make sense, and we should fix that underlying issue as well at some point, how that happens (23.0 vs dev) all is fine for me. But now that it 'fails closed', this looks better to me.

[mvdbeek]

Looks good to me, @jdavcs can you also take a look ?

[jdavcs]

Both alter statements (upgrade and downgrade) should be wrapped in a batch operations context manager:

with op.batch_alter_table(table_name) as batch_op:
    batch_op.alter_column(column_name, existing_type=sa.Boolean(), nullable=True, server_default=None)

Note that we do not need to pass table name to alter_column, and we call it on batch_op, not op.

The reason for that is that SQLite has very limited support for ALTER statements. The batch mode uses the "move and copy" approach.

The rest looks fine.

[jdavcs]

Also, not a dealbreaker, but would it be possible to merge this after #15811? Otherwise, it would have to be edited after that PR is merged. With 15811, this revision script would look like this:

with transaction():
   op.execute(...)
   alter_column(...)

In addition, this revision would be tested (that PR adds a test to the CI so that all revisions go through a downgrade>upgrade test sequence on both postgresql and sqlite).

[nsoranzo]

Also, not a dealbreaker, but would it be possible to merge this after #15811? Otherwise, it would have to be edited after that PR is merged. With 15811, this revision script would look like this:

This seems another reason to target dev instead of 23.0 ?

[davelopez]

Yes, I will retarget dev and update the PR with John's suggestions as soon as #15811 is merged. I will keep it in draft for now and rename the title :)

[davelopez]

Rebased and retargeted to dev.
@jdavcs a final review to see if I included your requested changes properly would be much appreciated 🍻

[jdavcs]

Rebased and retargeted to dev. @jdavcs a final review to see if I included your requested changes properly would be much appreciated beers

@davelopez my suggestion is to do the same as we do with all other DDL statements: have a helper like this. That way they are all routed through the base DDLOperation class that handles all checks, makes --repair mode possible, etc. I'll add one today.

[jdavcs]

@davelopez I've added the utility in #16009. With that, instead of with op.batch... you can just call alter_column( passing the table name + the same arguments you passed to the batch op function.

[jdavcs]

Test failures: server_default cannot be a bool. None or str. https://docs.sqlalchemy.org/en/20/core/metadata.html#sqlalchemy.schema.Column.params.server_default

[jdavcs]

Thank you, @davelopez and everyone for such a thorough review!

[mvdbeek]

this definitely needs to go into the release announcement, since it'll invalidate old api keys