[SQLite] implement C++ code to get keyservers
ClosedPublic
Actions

Authored by kamil on Jan 22 2024, 1:29 PM.

Details

Reviewers

inka
marcin

Commits

rCOMM9885469a8000: [SQLite] implement C++ code to get keyservers

Summary

Code for native ops

Depends on D10776

Test Plan

Call this code:

let data = await commCoreModule.getClientDBStore();
console.log(data.keyservers);
await commCoreModule.processKeyserverStoreOperations([
  {
    type: 'replace_keyserver',
    payload: {
      id: '1',
      keyserverInfo: 'fdfd',
    },
  },
  {
    type: 'replace_keyserver',
    payload: {
      id: '2',
      keyserverInfo: 'fdfd',
    },
  },
]);

data = await commCoreModule.getClientDBStore();
console.log(data.keyservers);
await commCoreModule.processKeyserverStoreOperations([
  {
    type: 'replace_keyserver',
    payload: {
      id: '2',
      keyserverInfo: 'fdfd45454545',
    },
  },
]);

data = await commCoreModule.getClientDBStore();
console.log(data.keyservers);
await commCoreModule.processKeyserverStoreOperations([
  {
    type: 'remove_keyserver',
    payload: {
      id: '1',
    },
  },
]);

data = await commCoreModule.getClientDBStore();
console.log(data.keyservers);
await commCoreModule.processKeyserverStoreOperations([
  {
    type: 'remove_all_keyservers',
  },
]);

data = await commCoreModule.getClientDBStore();
console.log(data.keyservers);

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Jan 22 2024, 1:29 PM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 22 2024, 1:29 PM

kamil added a child revision: D10781: [SQLite] implement processing keyserver store ops on worker.Jan 22 2024, 1:32 PM

Harbormaster completed remote builds in B26022: Diff 35956.Jan 22 2024, 1:48 PM

kamil published this revision for review.Jan 22 2024, 2:39 PM

kamil edited the test plan for this revision. (Show Details)

ashoat added inline comments.Jan 23 2024, 4:56 PM

web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	A bit confused about this function and its name. There appears to be another `getClientStore` in `web/database/utils/store.js`. What's the difference between the two? Should one be renamed? Also wondering why this function returns a `ClientDBStore` with `threads`, `drafts`, and `reports` populated, but the other fields are empty.

ashoat added inline comments.Jan 23 2024, 4:57 PM

web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	Regarding my second question, it looks like the `keyservers` property is populated in the next diff (D10781)

We discussed this offline and agreed that it would be good if someone else had a look as well - I don't feel confident reviewing this code.

kamil added inline comments.Jan 27 2024, 6:28 PM

web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	A bit confused about this function and its name. There appears to be another `getClientStore` in `web/database/utils/store.js`. What's the difference between the two? Should one be renamed? One is executed on the main thread, and the second is executed on a worker. I could rename this but I think that directory path is enough to distinguish. Also wondering why this function returns a `ClientDBStore` with `threads`, `drafts`, and `reports` populated, but the other fields are empty. Other fields are empty because we don't support `messages` and `users` on the web (`users` are currently in the testing process). Regarding my second question, it looks like the `keyservers` property is populated in the next diff (D10781) That's correct

ashoat added inline comments.Jan 27 2024, 11:15 PM

web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	One is executed on the main thread, and the second is executed on a worker. I could rename this but I think that directory path is enough to distinguish. If they weren't being exported I think it'd be fine, but since they're both exported I think it would be good to change both of their names to be different (and more specific)

kamil added inline comments.Jan 29 2024, 11:51 AM

web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	makes sense, I'll do that in a separate diff

marcin added inline comments.Feb 1 2024, 10:45 AM

native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/DataStores/KeyserverStore.cpp
26 ↗	(On Diff #35956)	As far as I understand for all store types we parse JSON in C++ so that returned `ClientDB<...>Info` is a proper JS object. Here we just return stringified JSON to the JS and probably expect that parsing will be done on JS side. Is there a string reason not to treat keyserver store in the same manner as other store types?

kamil added a child revision: D10917: [web] update `getClientStore` name.Feb 1 2024, 8:43 PM

kamil added inline comments.Feb 1 2024, 8:45 PM

native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/DataStores/KeyserverStore.cpp
26 ↗	(On Diff #35956)	As far as I understand for all store types we parse JSON in C++ so that returned `ClientDB<...>Info` is a proper JS object. Here we just return stringified JSON to the JS and probably expect that parsing will be done on JS side. Is there a strong reason not to treat keyserver store in the same manner as other store types? I don't think we do that. If we do some parsing in C++ it's notifs related. We always return data that align with the database structure and do parsing on the JS side: avatar in thread user info etc.
web/database/worker/process-operations.js
161 ↗	(On Diff #35956)	D10917

kamil removed a child revision: D10781: [SQLite] implement processing keyserver store ops on worker.Feb 2 2024, 11:13 AM

marcin accepted this revision.Feb 2 2024, 12:17 PM

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/DataStores/KeyserverStore.cpp
26 ↗	(On Diff #35956)	Parsing in C++ is done for `MessageStore`, `DraftStore` and `ThreadStore`. But on the other hand it is not done in C++ for `ReportStore` and `UserStore` so I was wrong telling that `KeyserverStore` breaks the pattern - it has been broken since `ReportStore` and `UserStore` (not sure which one was first). We might consider to unify approach in future but this is out of the scope of this diff.

This revision is now accepted and ready to land.Feb 2 2024, 12:17 PM

kamil added inline comments.Feb 2 2024, 12:38 PM

native/cpp/CommonCpp/NativeModules/PersistentStorageUtilities/DataStores/KeyserverStore.cpp
26 ↗	(On Diff #35956)	Parsing in C++ is done for `MessageStore`, `DraftStore` and `ThreadStore`. Could link where or what parsing you mean? For `ThreadStore` we map the database result for the JSI object (exactly the same as here), and if we have stringified JSON somewhere, like for `avatar` we pass it as string and parse in JS. I cannot find any JSON parsing for either of these stores in C++ (other than some utils notif function linked in comment above) - but maybe I am missing something obvious.