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

Authored by kamil on Jan 22 2024, 3:44 AM.

Details

Reviewers

inka
marcin

Commits

rCOMMc2979e5a2646: [SQLite] add methods to operate on `keyservers` table

Summary

Representation of data row.
Methods to modify the table.
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 created this revision.Jan 22 2024, 3:44 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 22 2024, 3:44 AM

Harbormaster failed remote builds in B26003: Diff 35935!Jan 22 2024, 4:03 AM

rebase

kamil added a child revision: D10776: [SQLite] implement C++ keyserver store ops.Jan 22 2024, 4:32 AM

Harbormaster completed remote builds in B26017: Diff 35951.Jan 22 2024, 4:54 AM

kamil published this revision for review.Jan 22 2024, 6:38 AM

kamil edited the summary of this revision. (Show Details)

marcin added inline comments.Jan 22 2024, 7:16 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1341 ↗	(On Diff #35951)	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 ↗	(On Diff #35951)	Can we remove this empty line?
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
74 ↗	(On Diff #35951)	Can we remove this empty line?

Before landing I think you should:

Refactor removeKeyserver to removeKeyservers that takes std::vector<std::string> ids instead of std::string id. It is more practical and matches convention.
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.