This differential introduces method to clean Secure Store and SQLite to 'DatabaseQueryExecutor' API and implements it in its derived class 'SQLiteQueryExecutor'. This method firstly attempts to delete database (if it exists), iverwrite encryption key in secure store and finally run migrate' method to create new database. The last step is essential. This method is expected to be called from react component, so it is important that it leaves application in a state that other components can use. In particular core-data-provider` component tries to do some database queries even if the user is logged out.
Details
Details
This function is not expected to be used outside of react-component so it might be hard to provide test plan prior to introducing diffs that implement actual components. But for the purpose of unit testing, someone might: 1) place this function somwhere in AppDelegate 2) open the app without any user logged in 3) execute this function setp by step in the debugger and monitor database file size.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 785–790 ↗ | (On Diff #15260) | This small code fragment is duplicated. What do you think about creating a new function that handles generating and setting the new encryption key? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 785–790 ↗ | (On Diff #15260) | I think it is a good practice that duplicated code gets extracted into a function. |
Comment Actions
Couple of questions inline. The one that really matters is making sure we're handling the return code of std::remove properly in the conditional on line 780.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 780 ↗ | (On Diff #15260) | I might be mistaken, but on successful deletion doesn't std::remove(...) return 0? Should we instead explicitly check if the return code is 0? eg std::remove(SQLiteQueryExecutor::sqliteFilePath.c_str()) == 0 ? As an aside, looking at the std::remove entry in CPPReference (https://en.cppreference.com/w/cpp/io/c/remove) it looks like they check for successful deletion with the following std::remove("file1.txt"); // delete file
bool failed = !std::ifstream("file1.txt");
if(failed) { std::perror("Error opening deleted file"); return 1; }I haven't looked too closely at it, but I wonder if that's a more "robust" check to ensure the file was properly deleted? |
| 781–782 ↗ | (On Diff #15260) | Elsewhere in the C++ codebase we handle string interpolation/appending with a pattern that looks like: std::ostringstream stringStream; stringStream << "Error creating '" << tableName << "' table: " << error; Logger::log(stringStream.str()); Could we do something like that here? eg std::ostringstream errorStream; errorStream << "Failed to delete database file. Details: " << strerror(errno); throw std::system_error(errno, std::generic_category(), errorStream.str()); ? |
| 785 ↗ | (On Diff #15260) | Can we use braced initialization here? CommSecureStore commSecureStore{}; |
| 788 ↗ | (On Diff #15260) | If I understand correctly (it's been months since I've looked at C++ so I'm probably rusty)
Is there a reason we can't use static functions instead and avoid instantiating a CommSecureStore object? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 780 ↗ | (On Diff #15260) |
0 in C++ always evaluates to boolean false and any non-zero integer always evaluates to true. That said value returned from std::remove can be directly used in a boolean expression without explicit comparison against 0. This if statement is intended to check for unsuccessful deletion , so we could write:
I think authors of this documentation simply wanted some example of code that shows that std::remove indeed removes the file, so that you can not open it with ifstream again. Documentation for POSIX remove (called by std::remove) explicitly states it will return non-zero integer upon failure. I am not sure why this information shouldn't be enough to detect unsuccessful file deletion. |
| 781–782 ↗ | (On Diff #15260) | Ok, I will stick to the convention and do the refactor. |
| 785 ↗ | (On Diff #15260) | Sure, I will introduce it. |
| 788 ↗ | (On Diff #15260) | CommSecureStore is our cross-platform interface that has platform-specific implementations that call native (Java or Objective-C) code depending on platform. Its method are defined as non-static, so currently we are forced to instantiate it. However it's implementation on both platforms is stateless so there is no reason not to make them static. But it would require a dozen of changes that are out of the scope of this differential. Taking into account the fact that this differential solves high priority Linear issue we probably do not want to delay it by introducing additional diffs into the stack. I think it would be best to create Linear task to make CommSecureStore methods static and to update relevant places in the codebase. Probably it would be a good starter task since it is relatively easy and quick so one of our interns could pick it up. |
Comment Actions
Refactor: curly brackets for CommSecureStore intiantiation, common logic extraction into a method, using string stream to concantenate strings instead of addition.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 780 ↗ | (On Diff #15260) | Ah okay sorry completely disregard what I said, I totally misread the conditional. I still think explicitly writing std::remove(SQLiteQueryExecutor::sqliteFilePath.c_str()) != 0 might be a little clearer, but up to you |
| 788 ↗ | (On Diff #15260) | Yeah that makes complete sense to me |
Comment Actions
Newest change for this diff relies on the following docs: https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.h#L95. SQLiteQueryExecutor::clearSensitiveData() is expected to be called in CommCoreModule on databaseThread. In order to access CommSecureStore on Android from databaseThread we need to attach databaseThread to JVM for the time when this function is executed. This differential introduces this functionality so entire function can be scheduled on databaseThread like in D4729. It must be noted that facebook::jni::ThreadScope::WithClassLoader could also be used directly in D4729. Both approaches are acceptable. I decided to do so in SQLiteQueryExecutor since it seems for me to be more low-level class while CommCoreModule is high-level and platform agnostic. But I am open to change it and use facebook::jni::ThreadScope::WithClassLoader in CommCoreModule just before call to clearSensitiveData is scheduled on databaseThread.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 788 ↗ | (On Diff #15260) | |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 518 | Unclear why we use snake case here | |