bug-2037388: Migration to add preferred_upload_api_version column to tokens table.#3310
bug-2037388: Migration to add preferred_upload_api_version column to tokens table.#3310smarnach wants to merge 1 commit into
Conversation
9cebc86 to
5f3b7f4
Compare
biancadanforth
left a comment
There was a problem hiding this comment.
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.
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? |
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 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 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.
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
The plan is something like this:
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. |
5f3b7f4 to
8c62795
Compare
|
I updated both PRs with the There's now a database default for the new column: I think this should work as intended now. |
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_tokentable 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_versionfield 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:I suggest reviewing this PR together #3311.