Fix migrations 1 #1446
Fix migrations 1 #1446
Conversation
| // don't allow it | ||
| // | ||
| // Use Int for Bools | ||
| const val DEFAULT_THEME = 0 |
There was a problem hiding this comment.
I really wanted to be able to use the Enums and ordinals here, but Room makes you use constants for their currentValue annotations.
app/src/main/java/com/jerboa/ui/components/settings/lookandfeel/LookAndFeelActivity.kt
Show resolved
Hide resolved
|
There is a test in AndroidTest that goes through the migrations to ensure it doesn't fail on migrating between versions but I will test it regardless. |
| """ | ||
| CREATE TABLE IF NOT EXISTS AppSettingsBackup ( | ||
| `id` INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, | ||
| `font_size` INTEGER NOT NULL DEFAULT ${DEFAULT_FONT_SIZE}, |
There was a problem hiding this comment.
You can't do something like this, the migrations need to stay exactly the same.
If we make migration version 33, say we change a default.
We change the constant and now we have also changed migration of 32.
Room will rightly throw a error that the checksum of migration 32 mismatches. Because that means that some users their db schema 32 is different. That can be very problematic
There was a problem hiding this comment.
Alright, can change them.
|
Which columns where in the wrong place? edit: I see how I did the order wrong, I was basing the order on the order of the alter statements that added the columns But If I look into the schemas, they are completely different. We should always explicitly set which columns maps to which to prevent subtle bugs like this to occur again. |
| db.execSQL("ALTER TABLE AppSettingsBackup RENAME to AppSettings") | ||
|
|
||
| // Reset a few messups to default | ||
| db.execSQL("UPDATE AppSettings SET post_actionbar_mode = $DEFAULT_POST_ACTION_BAR_MODE") |
There was a problem hiding this comment.
There are probably more but only the enums matter, bc if they get assigned a value bigger than their enum size (due swapping between two enums which have different size and that user having the biggest value) it would cause a NPE when we map the int to enum
There was a problem hiding this comment.
Imo this should be sufficient unless you want to reset more enums.
|
iirc the two out of place, were |
|
K, I'll get a release out after its merged. |
Okay, I strongly typed all our default values, and double checked the column order (2 were in the wrong place iirc).
It'd be good if you could test this also, just to be sure it doesn't break existing installs.