Skip to content
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

refactor(sql): recreate config and keypairs table #5064

Closed
wants to merge 2 commits into from

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Nov 30, 2023

The reason for the change is that I also want to recreate keypairs table for #1177 and turn is_default into a config row pointing to the currently used key.

Current way of selecting my primary keypair is tricky:
https://github.com/deltachat/deltachat-core-rust/blob/1c9662a8f275bb6c57809ada43b9e4b0a3eea210/src/key.rs#L86-L88

Combined with inserting a new key and making sure it is unique is even more tricky, seems more reliable to add UNIQUE constraint once and then it is easy to insert/update config rows in a migration.

@link2xt link2xt force-pushed the link2xt/config-upsert branch 2 times, most recently from 6a27de3 to 780743c Compare November 30, 2023 20:21
@link2xt link2xt marked this pull request as draft November 30, 2023 20:33
@link2xt link2xt force-pushed the link2xt/config-upsert branch 2 times, most recently from 3044450 to 15398de Compare November 30, 2023 20:59
@link2xt link2xt marked this pull request as ready for review November 30, 2023 21:01
.await?;
}
self.execute(
"INSERT OR REPLACE INTO config (keyname, value) VALUES (?, ?)",
Copy link
Collaborator Author

@link2xt link2xt Nov 30, 2023

Choose a reason for hiding this comment

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

There are unfortunately migrations 50, 59, 67, 71 which use set_raw_config. This prevents use of more specific ON CONFLICT keyname as this is not allowed if there is no UNIQUE constraint yet. But non-specific ON CONFLICT DO UPDATE or INSERT OR REPLACE works.

Generally it is not a good idea to use anything other than SQL statements in migrations as functions called will be changed over time and it essentially changes migrations which should be immutable.

@link2xt link2xt force-pushed the link2xt/config-upsert branch 2 times, most recently from 284e0c9 to ce1daf2 Compare December 1, 2023 00:59
@link2xt link2xt changed the title refactoring(sql): recreate config table with UNIQUE constraint refactor(sql): recreate config and keypairs table Dec 1, 2023
if dbversion < 107 {
sql.execute_migration(
"CREATE TABLE new_keypairs (
id INTEGER PRIMARY KEY AUTOINCREMENT,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With id of the keypair being AUTOINCREMENT I can point to it from acpeerstates when I need to store what I think is the current verified key of me from the contact's perspective. If it points to the current key, I have a backward verification. Then if I also have a verified key for the contact corresponding to the autocrypt key, I have forward verification. If the contact is both forward and backward verified, I am going to display green checkmark for the contact, the chat with the contact becomes verified and contact can be added to verified chats.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, if the contact is only forward-verified, the green checkmark can already be displayed (we already verified it, so why not display that?) and we even can add the contact to verified chats, but before adding we should retry sending v*-request-with-auth because maybe it was lost and that's the reason of the absent backward verification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the contact is only forward-verified, if we add member to a verified chat they will receive a square bracket error "The message was sent with non-verified encryption. See 'Info' for more details.". As for automatic retry of Securejoin messages, I plan to keep it simple and not add green checkmark, so user will notice verification did not work out and can simply rescan to retry the whole process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the contact is only forward-verified, if we add member to a verified chat they will receive a square bracket error "The message was sent with non-verified encryption. See 'Info' for more details."

Yes, that's why i suggested to retry v*-request-with-auth and be optimistic in that it reaches the contact and assume that we can add them to verified chats. But i agree, not showing the green checkmark and letting the user to retry on their own looks simpler.

@link2xt link2xt force-pushed the link2xt/config-upsert branch 2 times, most recently from 6cd5fda to 59aaf74 Compare December 1, 2023 02:32
@link2xt link2xt force-pushed the link2xt/config-upsert branch 2 times, most recently from 16b17bd to cdf74ce Compare December 1, 2023 18:58
@link2xt link2xt marked this pull request as draft December 1, 2023 22:56
@link2xt
Copy link
Collaborator Author

link2xt commented Dec 1, 2023

This is not needed right now and too dangerous for bugfix releases, but nice that it is reviewed and I can build on top of it.

@link2xt link2xt force-pushed the link2xt/config-upsert branch 5 times, most recently from 004c9b7 to f6ae7de Compare December 6, 2023 17:06
Removed unused `addr` and `created` field.
`is_default` boolean flag is moved into `config` row
pointing to the current default key.
@link2xt link2xt force-pushed the link2xt/config-upsert branch from f6ae7de to 5577ca6 Compare December 6, 2023 19:59
@link2xt
Copy link
Collaborator Author

link2xt commented Dec 6, 2023

Closing this, moved commits to #5089

@link2xt link2xt closed this Dec 6, 2023
@link2xt
Copy link
Collaborator Author

link2xt commented Dec 11, 2023

Recreated this as #5099 with a config table cache fixup.

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