Page MenuHomePhabricator

[sqlite/lib] Split keyserver ops for backed and non-backed up data
ClosedPublic

Authored by michal on Mar 29 2024, 1:43 PM.
Tags
None
Referenced Files
F3378613: D11481.id38776.diff
Wed, Nov 27, 11:51 AM
F3378138: D11481.diff
Wed, Nov 27, 9:13 AM
Unknown Object (File)
Sun, Nov 3, 7:54 AM
Unknown Object (File)
Oct 12 2024, 8:33 PM
Unknown Object (File)
Oct 12 2024, 8:33 PM
Unknown Object (File)
Oct 12 2024, 8:32 PM
Unknown Object (File)
Sep 18 2024, 6:45 AM
Unknown Object (File)
Aug 29 2024, 7:50 PM
Subscribers

Details

Summary

ENG-6928 : Return the default data after restoration
Modify the keyserver ops to split the keyserver data into two stringied jsons -> one of them will be backed up and the other won't. When fetching the keyserver data the two tables are joined and the ops merge the data into one. If the non-synced (non-backed up) data isn't there (e.g. after a backup restore), the data will be filled in with defaults.

Depends on D11473

Test Plan

Run the keyservers-queries.test.js. Make sure that keyserver tables are filled in with data correctly. Check that if the non-synced data doesn't have any data, the ops return default values.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Accepting but I am of strong opinion that it would be better to create two new structs: NonSyncedKeyserverInfo and SyncedKeyserverInfo that would look exactly the same as original KeyserverInfo. In SQLiteQueryExecutor methods we would just create instances of each class and could rely on simple SQL queries without named parameters. The code would be easier to understand and maintain. It would be two small classes doing simple logic instead of one bigger doing complex logic.

This revision is now accepted and ready to land.Apr 2 2024, 3:39 AM

Accepting but I am of strong opinion that it would be better to create two new structs: NonSyncedKeyserverInfo and SyncedKeyserverInfo that would look exactly the same as original KeyserverInfo. In SQLiteQueryExecutor methods we would just create instances of each class and could rely on simple SQL queries without named parameters. The code would be easier to understand and maintain. It would be two small classes doing simple logic instead of one bigger doing complex logic.

Agree this breaks our convention a bit but not sure if this is more maintainable - I think it's better to have one entity which is some sort of shared interface on how both C++ and JS understand data, the only difference is that we have two tables for it, but on each operation, we modify both and when querying we joined them.

I also think named parameters are better overall, we should implement them that way, now probably there will be no time to prioritize them.

lib/ops/keyserver-store-ops.js
14 ↗(On Diff #38556)

can be merged

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1650–1654 ↗(On Diff #38556)

nit: I think you can improve formatting

native/cpp/CommonCpp/DatabaseManagers/entities/KeyserverInfo.h
24 ↗(On Diff #38556)

TBH I like this approach even more than this we can use right now

Small fixes

I agree with @kamil, these two tables are bundled together logically and the operations should be made on both of them, so splitting ops in two would make it easier to introduce bugs and subtle differences.