Page MenuHomePhabricator

[SQLite] add methods to operate on `keyservers` table
ClosedPublic

Authored by kamil on Jan 22 2024, 3:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 6:48 PM
Unknown Object (File)
Thu, Oct 24, 5:48 AM
Unknown Object (File)
Oct 15 2024, 7:57 AM
Unknown Object (File)
Oct 15 2024, 7:57 AM
Unknown Object (File)
Oct 15 2024, 7:57 AM
Unknown Object (File)
Oct 6 2024, 5:14 AM
Unknown Object (File)
Oct 5 2024, 11:15 AM
Unknown Object (File)
Oct 2 2024, 5:37 PM

Details

Summary
  1. Representation of data row.
  2. Methods to modify the table.
  3. Emscripten bindings.

Note: This could heavily conflict with @marcin's work on deprecating sqlite_orm - we will coordinate to make this smooth and avoid blocking each other.

Depends on D10774

Test Plan

Calling those methods from JS and making sure it works.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jan 22 2024, 6:38 AM
kamil edited the summary of this revision. (Show Details)
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1341

Wondering if method taking a vector of id's to remove multiple keyservers at once wouldn't be more practical. Actually this is what we do in the rest of the code.

I agree with @marcin

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
986

Can we remove this empty line?

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
74

Can we remove this empty line?

Before landing I think you should:

  1. Refactor removeKeyserver to removeKeyservers that takes std::vector<std::string> ids instead of std::string id. It is more practical and matches convention.
  2. Implement some unit tests for the KeyserverStore in the same manner that you did for other stores that have already migrated to the database on web.
This revision is now accepted and ready to land.Jan 23 2024, 12:44 AM

address review and implement tests