Page MenuHomePhabricator

[lib] Add integrity store ops
ClosedPublic

Authored by will on Mar 5 2024, 1:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 6, 11:04 PM
Unknown Object (File)
Sun, May 5, 11:51 AM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Unknown Object (File)
Apr 7 2024, 8:15 PM
Subscribers

Details

Summary

Add integrity store ops for sqlite migration

Test Plan

flow. Functionality tested later in stack

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

will held this revision as a draft.
will published this revision for review.Mar 5 2024, 9:02 PM
kamil requested changes to this revision.Mar 7 2024, 4:04 AM
kamil added inline comments.
lib/ops/integrity-store-ops.js
23 ↗(On Diff #37863)

this is incorrect, in D11249 you defined threadHash as TEXT so here you should have string

73–74 ↗(On Diff #37863)

nit: I think this is more readable

75–78 ↗(On Diff #37863)

you can use entries() and map(), example is here

This revision now requires changes to proceed.Mar 7 2024, 4:04 AM
will added inline comments.
lib/ops/integrity-store-ops.js
23 ↗(On Diff #37863)

Thanks. This is from thinking we'd need to store the threadHash with the threadHashStatus.

I think this is then best kept as a number as I'll change the sqlite table to make threadHash a INTEGER type.

review feedback and add remove operation

kamil added a subscriber: atul.
kamil added inline comments.
lib/ops/integrity-store-ops.js
23 ↗(On Diff #37863)

Wondering if this is safe... Looks like for hashing we use string-hash which returns a "number between 0 and 4294967295 (inclusive)" so should be safe for SQLite's INTEGER but I am worried about JSI/WASM parsing, we should use unsigned int to fit maximum number, and this could vary on different platforms.

If I remember correctly @atul some time ago suggested always use string in cases like here and then parsing it if we want to store numbers in SQLite.

@atul what do you think?

will retitled this revision from [sqlite] Add integrity store ops to [lib] Add integrity store ops.Mar 11 2024, 3:10 PM
kamil requested changes to this revision.Mar 12 2024, 10:21 AM

Putting back to your queue to address threadHash type

This revision now requires changes to proceed.Mar 12 2024, 10:21 AM
lib/ops/integrity-store-ops.js
23 ↗(On Diff #37863)

I think we should prefer string vs. number/int in this situation.

JS number is internally represented as floating point which isn't 1:1 w/ C++ int/unsigned int etc. There could possibly be issues if like 10.5 (valid JS number) was passed to C++ side and not handled properly, etc.

Probably more compelling argument is that using string gives us more flexibility with encoding. We ran into this with eg threadIDs which now are keyserverID|threadID. Similarly could imagine we had hashes encoded with some additional info in the future? Maybe a stretch but we might include information about hash algorithm or something? Like sha512|blah or superQuantumFuturisticHash|blah something?

Is there a reason to prefer number/INTEGER for this situation? I think any performance difference should be imperceptible

lib/ops/integrity-store-ops.js
23 ↗(On Diff #37863)

Then I think it makes sense to convert it to a string.

The original reason was just to avoid parsing between number and string and keep the sqlite consistent with our javascript, but what you outlined makes sense.

I'll convert from number to string in the next rebase.

review feedback. convert thread_hash to string

This revision is now accepted and ready to land.Mar 18 2024, 6:16 AM

add remove to process store ops

rename ClientDBIntegrityThreadHashesOperation to ClientDBIntegrityStoreOperation for consistency

will marked 2 inline comments as done.Mar 25 2024, 1:22 PM
This revision was automatically updated to reflect the committed changes.