Create JSI functions that may be called from js as well as cpp. setDeviceID calls a rust function that generates the id, and then calls a cpp function that sets 'device_id' entry in Metadata table of SQLite database to the generated
value.
Details
Details
comment out native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp set_encryption_key function to turn off the database encryption, Add the following to onClick function of ThreadsettingsPromoteSidebar in
native/chat/settings/thread-settings-promote-sidebar.react.js:
console.log(commCoreModule.setDeviceID('WEB'));
commCoreModule.getDeviceID().then(id => console.log(id));And see by setting breakpoints before and after these lines that in the SQLite Metadata table for the simulator there appears device_id entry with a value subject to "^(ks|mobile|web):[a-zA-Z0-9]{64}$" in the data column.
See that this value gets logged to the console twice.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 2 ↗ | (On Diff #17483) | In my opinion it is better to avoid including this rust-cpp bindings header and using ::rust namespace types directly in CommCoreModule. This is why I created C++ wrappers for ma native blob client. But this is rather my personal feeling about code cleanness so I am not going to block this revision on this. You don't need to address this comment anyhow if you don't feel like it. |
| 1001 ↗ | (On Diff #17483) | This variable is not used anywhere |
| 1004 ↗ | (On Diff #17483) | We could create a static std::map<std::string, DeviceType> in this function that maps strings into enumeration values and call generate_device_id only once. Code we be shorter and more readable. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1004 ↗ | (On Diff #17483) | I started wandering if maybe, since this code will be called only from native, I should just always call generate_device_id(DeviceType::MOBILE). And do the same on web. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1004 ↗ | (On Diff #17483) | Or actually have the functions on web and native, responsible for generating the device id, generate only the one type that they will ever be called with anyways. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1004 ↗ | (On Diff #17483) | I have been informed that this code might be used on the desktop app, so I cannot do that. Please ignore my previous comments. |
Comment Actions
CI fails because we haven't yet made rust work on android on CI. This is tracked in ENG-1852
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1012 ↗ | (On Diff #17637) | Setting device id is a database operation that can fail. If the code above fails the app will crash with vague and uninformative stacktrace (I have dealt with crashes caused by uncaught exception from our ORM and they were hard to debug). Moreover, everywhere in CommCoreModule we make sure that any database operation placed on the database thread (GlobalDBSingleton now) is executed inside try catch statement so that we can either:
Please change the code so that it follows the same pattern. |
| 1017 ↗ | (On Diff #17637) | There are two things here:
|
Comment Actions
Address code review - add try-catch around GlobalDBSingleton::instance.scheduleOrRun and make getDeviceID async and use GlobalDBSingleton.
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1021–1042 ↗ | (On Diff #17805) | This function is almost identical to getCurrentUserID. I think it would be good to extract the common code and have getCurrentUserID and getDeviceID be left as wrapper functions. But first this code needs to be approved I think |
Comment Actions
Accepting, but kindly please make sure you test it on Android and iOS physical devices before landing if you haven't done it already. My experience is that there are some C++ bugs that reveal themselves only on physical devices.
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1021–1042 ↗ | (On Diff #17805) | Extracting a common function is a good idea |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1021–1042 ↗ | (On Diff #17805) | After some research I think I'd rather leave it. result = DatabaseManager::getQueryExecutor().getCurrentUserID(); and the second one calls result = DatabaseManager::getQueryExecutor().getDeviceID(); So to extract the common code I'd have to pass getCurrentUserID/getDeviceID as an argument. This can be done. It would look something like this: jsi::Value getFunctionResultAsPromise(jsi::Runtime &rt, std::string (SQLiteQueryExecutor::*func)(), CommCoreModule *commCoreModule){
. . .
}
jsi::Value CommCoreModule::getCurrentUserID(jsi::Runtime &rt) {
return getFunctionResultAsPromise(rt, &SQLiteQueryExecutor::getCurrentUserID, this);
}But because we get the instance of SQLiteQueryExecutor via DatabaseManager::getQueryExecutor(), which return value is DatabaseQueryExecutor I cannot call a function passed like this on this instance. I cannot make my function take std::string (DatabaseQueryExecutor::*func)() as an argument either, because DatabaseQueryExecutor is an abstract class so its functions don't have implementations. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1021–1042 ↗ | (On Diff #17805) | Ok, this makes sense - it should be possible to simplify that but not sure if worth it now. Also, we have a task https://linear.app/comm/issue/ENG-325/refactor-commcoremodule-c which can also include this refactoring. |
| native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp | ||
|---|---|---|
| 1021–1042 ↗ | (On Diff #17805) | I'll add this to ENG-325 when this diff is landed then |