Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 53
Introducing emails and pubkeys stores instead of contacts#3445
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
77aa1ac10a987639ebd04909473a86ccffb69c56c392875480c6996bb39a1d397e5b388146f55164d2f05ab5b0f866a36d8a17520aae19a303cf2a02881ddaa0c19ef2135e3a1e33c0c91cf81ecd07570b0e8856635946ff0a392007f4f612ebcaaea91184c0611e8236d4f548e7d995128b3300b26e0dc276b375616f64218ba7664b7ac8731576772b7865b76cd11af415f1056f12d5c9b97f4d66e2e7782744f048458f3c38e0759f1a348d72baa4a1d11280ae2f51edae273b603d439fa9cbdef90793a91d4440bd2974f4531a575c61288b1d335c32e1bc51f5bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -50,16 +50,10 @@ export type PubkeyResult = { pubkey: Key, email: string, isMine: boolean }; | ||
| export type Contact = { | ||
| email: string; | ||
| name: string | null; | ||
| pubkey: Key | null; | ||
| pubkey: Key | undefined; | ||
| has_pgp: 0 | 1; | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are doing a migration anyway, could we rename these to be consistently camelCase? To use ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a purely DTO class. We can of course refactor it in this PR, making it grow even bigger. | ||
| searchable: string[]; | ||
| client: string | null; | ||
| fingerprint: string | null; | ||
| longid: string | null; | ||
| longids: string[]; | ||
| pending_lookup: number; | ||
| last_use: number | null; | ||
| pubkey_last_sig: number | null; | ||
| pubkey_last_check: number | null; | ||
| expiresOn: number | null; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -56,8 +56,8 @@ export class SmimeKey { | ||
| expiration: SmimeKey.dateToNumber(certificate.validity.notAfter), | ||
| fullyDecrypted: true, | ||
| fullyEncrypted: false, | ||
| isPublic: false, // the way isPublic is currently used is as opposite of isPrivate, even if the Key contains both | ||
| isPrivate: true, | ||
| isPublic: certificate.publicKey && !certificate.privateKey, | ||
| isPrivate: !!certificate.privateKey, | ||
rrrooommmaaa marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } as Key; | ||
| const headers = PgpArmor.headers('pkcs12'); | ||
| (key as unknown as { raw: string }).raw = `${headers.begin}\n${forge.util.encode64(bytes)}\n${headers.end}`; | ||
| @@ -107,8 +107,8 @@ export class SmimeKey { | ||
| expiration: SmimeKey.dateToNumber(certificate.validity.notAfter), | ||
| fullyDecrypted: false, | ||
| fullyEncrypted: false, | ||
| isPublic: true, | ||
| isPrivate: true, | ||
| isPublic: certificate.publicKey && !certificate.privateKey, | ||
| isPrivate: !!certificate.privateKey, | ||
| } as Key; | ||
| (key as unknown as { rawArmored: string }).rawArmored = text; | ||
| return key; | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
@tomholub this is the piece of code I was referring to when talking about contacts migration.
It would be more convenient to perform migration inside
dbOpen()call. Thus we'll be able to deletecontactsstore right after successful conversion toemailsandpubkeys.We can, for example, pass a list of functions to
dbOpento nicely inject transformations so they don't clutterContactStorecode?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.
or, perhaps,
ContactStore.updatewill cause errors when called from inside upgrade transactionThere 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.
We can do all of this in
ContactStoreitself, no need to bring the migration out to here.Uh oh!
There was an error while loading. Please reload this page.
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.
Looks like there is a problem with this approach. IDTransaction docs say that transaction is automatically completed when it goes out of scope, and this disallows usage of async function in
onupgradeneededetc.. They are fired alright, but the transaction is closed by the time the actual processing occurs (before waiting for the promise returned byonupgradeneededhandler to be resolved). I searched the github for projects where an async function is used as event handler, found only a small project https://github.com/SourceCodeBot/crudodb (I suspect they haven't tested it properly).The general approach for transactional operations is (though I haven't found this exact requirement in IndexedDB specs) -- a handler must immediately open a new request, otherwise the transaction will be auto-completed.
We can't convert contacts to pubkeys/emails in a completely synchronized way because of async
KeyUtil.parsefunction.(This is a point for another discussion -- KeyUtil.parse is async only because it uses a Stream API approach for dearmoring of potentially large messages. As the keys are small, we may think about a sync implementation of KeyUtil.parse)
Still, maybe it's user-unfriendly to "hang" (will it hang?) the browser window for the process of database upgrade, converting multiple contacts, so it makes sense to keep the conversion out of the upgrade handler. The drawback is that the empty
contactstable will be lying around until the next upgrade.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 will possibly hang the whole browser when we do something like this in background page. Also depends on contacts count. For 50 contacts, not a problem. For 1000 contacts - possible problem.
I think our most reasonable chance is to do a migration of database structure itself, fire off async migration of the actual database, and hope for the best.
If the actual migration does not finish successfully, then on next migration it will be retried (like a browser restart). I think that's ok as long as there is this retry in place.
Best would be to then not migrate all contacts in one gigantic transaction, but one by one. If one of the transactions throws an error because whatever (parsing, concurency, etc), it would not fail the whole migration. 100 keys already migrated, key 101 failed, 50 more to go - the failed one gets catched and skipped, and it will chug along for the remaining 50 keys. Then next time it only has to retry one.
Uh oh!
There was an error while loading. Please reload this page.
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.
I have implemented migration in batches of 50, but it's going to throw on an error and crash the extension.
If we skip the errored batch, this portion of contacts won't be available for search -- is this what you're after?
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.
When there is an error, it's better to skip a batch then stop half way through the migration.
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.
Will the skipped batch get retried (without any special retry logic) next time I restart the browser?
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.
The conversion stop at the failed batch with the current implementation.
as
contactsstore objects are deleted within the same transaction as populating new stores, the contacts from the failed batch will remain in the database, thus they will be retried on subsequent browser restart.