Skip to content

bug-2037388: Migration to add preferred_upload_api_version column to tokens table.#3310

Open
smarnach wants to merge 1 commit into
mainfrom
token_upload_api_version_migration
Open

bug-2037388: Migration to add preferred_upload_api_version column to tokens table.#3310
smarnach wants to merge 1 commit into
mainfrom
token_upload_api_version_migration

Conversation

@smarnach
Copy link
Copy Markdown
Contributor

@smarnach smarnach commented May 6, 2026

This is the first of two PRs to implement the new /upload/auth_info/ endpoint. This PR only contains the database migration. It needs to be applied first in stage and prod before we can merge the follow-up PR containing the actual functionality.

The migration only affects the tokens_token table in the database, which has 68 rows on the production environment, so it can be applied with any database downtime. (In Postgres, adding a column with a default value only changes the table metadata anyway, so the amount of data in the table doesn't even matter in this case.)

The new preferred_upload_api_version field sets the preferred upload API version per token. I initially planned setting this per user, but setting it per token instead has a few advantages:

  • When performing testing of the CLI, I can have tokens for both API versions without needing to switch between different accounts.
  • We use the stock django-auth user model. If we wanted to add a field to that model, we'd need to completely switch to a custom user model, which is possible, but more cumbersome. The alternative would be using groups or permissions as flags, but that leads to awkward code as well and is more work to clean up again once we are done with the migration.

I suggest reviewing this PR together #3311.

@smarnach smarnach requested a review from a team as a code owner May 6, 2026 18:17
@smarnach smarnach force-pushed the token_upload_api_version_migration branch 2 times, most recently from 9cebc86 to 5f3b7f4 Compare May 6, 2026 20:53
Copy link
Copy Markdown
Contributor

@biancadanforth biancadanforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is missing the model update to add the preferred_upload_api_version field to Token? It looks like it's in your other PR, #3311 .

I know the other PR is to follow this one, so there probably wouldn't be a large period of time in between, and given the last DB migration was in 2017, it seems unlikely we'd actually hit a footgun with this, but it still seems like the right separation.

@biancadanforth
Copy link
Copy Markdown
Contributor

biancadanforth commented May 8, 2026

In Postgres, adding a column with a default value only changes the table metadata anyway, so the amount of data in the table doesn't even matter in this case.

IIUC this is true for defaults that are always the same value, but not for defaults that have a different value with each call.

I'm curious: I can see how having this field on the token is easier for testing during the project, but what is the plan for updating V1 tokens after the migration? Default token validity is 1 year, change the default field value to 2, and the field gets updated if/when the token is rotated?

@smarnach
Copy link
Copy Markdown
Contributor Author

smarnach commented May 8, 2026

I think this PR is missing the model update to add the preferred_upload_api_version field to Token? It looks like it's in your other PR, #3311 .

The entire reason that I filed two PRs is that the migration and the model change can't be in the same PR, at least not for our model of applying migrations. I think you are still right that there is a small problem here.

Some people apply migrations on each deployment, before the application gets deployed. We don't apply migrations automatically at all. Instead, we have a Helm chart to manually apply them, and this can only happen after the app was deployed.

Django's ORM is fine with having more columns in the actual database schema than in the model. When querying rows, it will always include explicit row names in the query, so the additional column simply won't show up. When creating rows it will provide values for the columns it knows about, and the remaining ones will be set to either the database default for the column or NULL.

This is where the problem is. I just double-checked the schema of the tokens_token table after applying the migration, and it is this:

tecken=# \d tokens_token
                                            Table "public.tokens_token"
            Column            |           Type           | Collation | Nullable |             Default              
------------------------------+--------------------------+-----------+----------+----------------------------------
 id                           | integer                  |           | not null | generated by default as identity
 key                          | character varying(32)    |           | not null | 
 expires_at                   | timestamp with time zone |           | not null | 
 notes                        | text                     |           | not null | 
 created_at                   | timestamp with time zone |           | not null | 
 user_id                      | integer                  |           | not null | 
 preferred_upload_api_version | integer                  |           | not null | 
Indexes:
    "tokens_token_pkey" PRIMARY KEY, btree (id)
    "tokens_token_user_id_7bb25a61" btree (user_id)
Foreign-key constraints:
    "tokens_token_user_id_7bb25a61_fk_auth_user_id" FOREIGN KEY (user_id) REFERENCES auth_user(id) DEFERRABLE INITIALLY DEFERRED
Referenced by:
    TABLE "tokens_token_permissions" CONSTRAINT "tokens_token_permissions_token_id_c4e3e001_fk_tokens_token_id" FOREIGN KEY (token_id) REFERENCES tokens_token(id) DEFERRABLE INITIALLY DEFERRED

So there actually isn't a database default set for the column, and it's not nullable, so creating tokens will fail after applying the migration and before deploying the updated Token model. It probably wouldn't be a huge issue if Tecken can't create new tokens for a few minutes, but it would still be an unintentional and unnecessary mistake.

If we had a field in the model that doesn't yet exist in the database, however, the app wouldn't even start up. In other words, if we included the model changes in the same PR, we wouldn't be able to deploy that change. It would still technically be possible to apply the migration in that case, since the old version would continue running while the new replica set is crash looping. We could then manually apply the migration, and the new version would eventually start up. It seems more controlled to separate the model change from the migration, though, and that's also the usual strategy for this kind of change.

In Postgres, adding a column with a default value only changes the table metadata anyway, so the amount of data in the table doesn't even matter in this case.

IIUC this is true for defaults that are always the same value, but not for defaults that have a different value with each call.

Yes, that's true. And it's also only true for actual database-level defaults, and it turns out the the default I defined is only an ORM-level default. There's a seperate option called db_default to set the database default, and I need to include that as well.

I'm curious: I can see how having this field on the token is easier for testing during the project, but what is the plan for updating V1 tokens after the migration? Default token validity is 1 year, change the default field value to 2, and the field gets updated if/when the token is rotated?

The plan is something like this:

  • Initially, all tokens have the version set to 1.
  • During my own testing, I'll set some of my tokens to version 2.
  • When testing on Taskcluster with Gabriele, we'll also set some tokens to version 2, or we create new ones with version 2.
  • During the actual migration, we change the active tokens one by one manually. There are only about ten tokens in active use.
  • Once every actively used token is migrated, we have many options. We could change the default version to 2 and then modify all existing tokens with a data migration. It's probably easier, though, to simply remove the preferred_upload_api_version field from Token model again and always return version 2 in the /update/auth_info/ endpoint.
  • Eventually, we'll completely remove the old upload API from both the server and the client and stop returning a version number in the /upload/auth_info/ endpoint.

This isn't necessarily a fixed plan. We'll have a lot of flexibility with a per-token version number, and we can adapt our approach depending on what obstacles we encounter.

@smarnach smarnach force-pushed the token_upload_api_version_migration branch from 5f3b7f4 to 8c62795 Compare May 8, 2026 11:22
@smarnach
Copy link
Copy Markdown
Contributor Author

smarnach commented May 8, 2026

I updated both PRs with the db_default parameter, and verified manually that it's possible to create tokens after applying the migration, but before changing the model:

app@b90b84d5ca50:/app$ ./manage.py shell
9 objects imported automatically (use -v 2 for details).

Python 3.11.14 (main, Feb  3 2026, 03:12:21) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> user = User.objects.get()  # There's only one user, so we can use get().
>>> token = Token.objects.create(user=user)
>>> token.id
3

There's now a database default for the new column:

tecken=# \d tokens_token
                                            Table "public.tokens_token"
            Column            |           Type           | Collation | Nullable |             Default              
------------------------------+--------------------------+-----------+----------+----------------------------------
 id                           | integer                  |           | not null | generated by default as identity
 key                          | character varying(32)    |           | not null | 
 expires_at                   | timestamp with time zone |           | not null | 
 notes                        | text                     |           | not null | 
 created_at                   | timestamp with time zone |           | not null | 
 user_id                      | integer                  |           | not null | 
 preferred_upload_api_version | integer                  |           | not null | 1
Indexes:
    "tokens_token_pkey" PRIMARY KEY, btree (id)
    "tokens_token_user_id_7bb25a61" btree (user_id)
Foreign-key constraints:
    "tokens_token_user_id_7bb25a61_fk_auth_user_id" FOREIGN KEY (user_id) REFERENCES auth_user(id) DEFERRABLE INITIALLY DEFERRED
Referenced by:
    TABLE "tokens_token_permissions" CONSTRAINT "tokens_token_permissions_token_id_c4e3e001_fk_tokens_token_id" FOREIGN KEY (token_id) REFERENCES tokens_token(id) DEFERRABLE INITIALLY DEFERRED

I think this should work as intended now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants