-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto: Add curve_key and sender_data_type columns to inboundgroupsessions table/store #3797
crypto: Add curve_key and sender_data_type columns to inboundgroupsessions table/store #3797
Conversation
// Open and return the DB (we know it's at the latest version) | ||
Ok(IdbDatabase::open_u32(name, 11)?.await?) | ||
Ok(IdbDatabase::open_u32(name, 12)?.await?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: this will mean the application release can't be rolled back without breaking everyone's sessions: we need to make sure this is called out, loudly, in changelogs all the way up the stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside: I wonder if, for future changes, there is a way we can do this that doesn't actually break compatibility. The addition of an index doesn't mean that the new database won't work with the old application, but the indexeddb version mechanism doesn't know that.
What if we did something like this:
- Promise that any future schema versions between 12 and (say) 19 will be backwards-compatible with applications expecting version 12.
- Replace this line with:
if old_version > 19 { return Err("db too new"); } Ok(IdbDatabase::open(name)?.await?)
Then, if we make a schema change that is actually breaking, we bump the version to 20 (there is no reason that schema versions have to be sequential).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3812 for Rich's follow-up on this idea.
let mut params = IdbIndexParameters::new(); | ||
params.unique(false); | ||
object_store.create_index_with_params(name, &IdbKeyPath::str_sequence(key_path), ¶ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a strong opinion, but it's worth noting that this whole thing can equivalently be written:
let mut params = IdbIndexParameters::new(); | |
params.unique(false); | |
object_store.create_index_with_params(name, &IdbKeyPath::str_sequence(key_path), ¶ms) | |
object_store.create_index(name, key_path.into()) |
... at which point, I really question whether the helper function has any value.
(even if you want to make the nonunique bit explicit, that's not much longer, because it can be written: IdbIndexParameters::new().unique(false)
)
We weren't updating the database schema version immediately after the v10 -> v11 migration. This was fine in practice, because (a) for now, there is no v12 migration so we ended up setting the schema version immediately anyway; (b) the migration is idempotent. However, it's inconsistent with the other migrations and confusing, and is about to make my test fail, so let's clean it up.
... so that next time we make a non-breaking change to the schema, it doesn't break rollback
…sions table/store
9c80560
to
b08403f
Compare
No description provided.