Page MenuHomePhabricator
Differential D6712 Diff 22413 native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp

Changeset View

Standalone View

View Options

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp

#include "DatabaseManager.h" #include "DatabaseManager.h"
#include "../Tools/CommSecureStore.h" #include "../Tools/CommSecureStore.h"
#include "Logger.h" #include "Logger.h"
#include "SQLiteQueryExecutor.h" #include "SQLiteQueryExecutor.h"
namespace comm { namespace comm {
std::once_flag DatabaseManager::initialized; std::once_flag DatabaseManager::initialized;
typedef const std::string DatabaseManagerStatus; typedef const std::string DatabaseManagerStatus;
DatabaseManagerStatus DB_MANAGER_WORKABLE = "WORKABLE"; DatabaseManagerStatus DB_MANAGER_WORKABLE = "WORKABLE";
DatabaseManagerStatus DB_MANAGER_FIRST_FAILURE = "FIRST_FAILURE"; DatabaseManagerStatus DB_MANAGER_FIRST_FAILURE = "FIRST_FAILURE";
DatabaseManagerStatus DB_MANAGER_SECOND_FAILURE = "SECOND_FAILURE"; DatabaseManagerStatus DB_MANAGER_SECOND_FAILURE = "SECOND_FAILURE";
DatabaseManagerStatus DB_OPERATIONS_FAILURE = "DB_OPERATIONS_FAILURE";
const std::string DATABASE_MANAGER_STATUS_KEY = "DATABASE_MANAGER_STATUS"; const std::string DATABASE_MANAGER_STATUS_KEY = "DATABASE_MANAGER_STATUS";
const DatabaseQueryExecutor &DatabaseManager::getQueryExecutor() { const DatabaseQueryExecutor &DatabaseManager::getQueryExecutor() {
// TODO: conditionally create desired type of db manager // TODO: conditionally create desired type of db manager
// maybe basing on some preprocessor flag // maybe basing on some preprocessor flag
thread_local SQLiteQueryExecutor instance; thread_local SQLiteQueryExecutor instance;
// creating an instance means that migration code was executed // creating an instance means that migration code was executed
// and finished without error and database is workable // and finished without error and database is workable
std::call_once(DatabaseManager::initialized, []() { std::call_once(DatabaseManager::initialized, []() {
DatabaseManager::setDatabaseStatusAsWorkable(); DatabaseManager::setDatabaseStatusAsWorkable(false);
}); });
return instance; return instance;
} }
void DatabaseManager::clearSensitiveData() { void DatabaseManager::clearSensitiveData() {
SQLiteQueryExecutor::clearSensitiveData(); SQLiteQueryExecutor::clearSensitiveData();
DatabaseManager::setDatabaseStatusAsWorkable(); DatabaseManager::setDatabaseStatusAsWorkable(true);
} }
void DatabaseManager::initializeQueryExecutor(std::string &databasePath) { void DatabaseManager::initializeQueryExecutor(std::string &databasePath) {
comm::CommSecureStore commSecureStore{}; comm::CommSecureStore commSecureStore{};
try { try {
SQLiteQueryExecutor::initialize(databasePath); SQLiteQueryExecutor::initialize(databasePath);
DatabaseManager::getQueryExecutor(); DatabaseManager::getQueryExecutor();
commSecureStore.set(DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_WORKABLE); DatabaseManager::setDatabaseStatusAsWorkable(false);
Logger::log("Database manager initialized"); Logger::log("Database manager initialized");
} catch (...) { } catch (...) {
folly::Optional<std::string> databaseManagerStatus = folly::Optional<std::string> databaseManagerStatus =
commSecureStore.get(DATABASE_MANAGER_STATUS_KEY); commSecureStore.get(DATABASE_MANAGER_STATUS_KEY);
if (!databaseManagerStatus.hasValue() || if (!databaseManagerStatus.hasValue() ||
databaseManagerStatus.value() == DB_MANAGER_WORKABLE) { databaseManagerStatus.value() == DB_MANAGER_WORKABLE) {
commSecureStore.set( commSecureStore.set(
DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_FIRST_FAILURE); DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_FIRST_FAILURE);
Logger::log("Database manager initialization issue, terminating app"); Logger::log("Database manager initialization issue, terminating app");
throw; throw;
} }
if (databaseManagerStatus.value() == DB_MANAGER_FIRST_FAILURE) { if (databaseManagerStatus.value() == DB_MANAGER_FIRST_FAILURE) {
commSecureStore.set( commSecureStore.set(
DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_SECOND_FAILURE); DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_SECOND_FAILURE);
Logger::log( Logger::log(
"Database manager initialization issue, app proceeding, but " "Database manager initialization issue, app proceeding, but "
"database needs to be deleted"); "database needs to be deleted");
return; return;
} }
} }
} }
void DatabaseManager::setDatabaseStatusAsWorkable() { void DatabaseManager::setDatabaseStatusAsWorkable(bool afterClearingDatabase) {
kamilAuthorUnsubmitted
Done
Inline Actions

we have to distinguish:

  • if we set status after clearing database it's always workable
  • if we set status after creating/migrating means that schema was created properly and without errors, but something might be broken, and executing operations might fail

the second case is something we don't want to override

kamil: we have to distinguish: - if we set status after clearing database it's always workable - if we…
marcinUnsubmitted
Not Done
Inline Actions

If we call this function and DatabaseStatus is DB_OPERATIONS_FAILURE it is a no-op and the caller is not aware of it. If someone tries to set DatabaseStatus as workable when database is indeed not workable it sounds like a programmer mistake and there should be a way to propagate this information. Maybe we should throw or at least make this function return a boolean indicating that marking database as healthy was not successful?

marcin: If we call this function and `DatabaseStatus` is `DB_OPERATIONS_FAILURE` it is a no-op and the…
kamilAuthorUnsubmitted
Done
Inline Actions

Returning the value or throwing an error will not change anything since the caller will have to ignore this. I think naming is the problem, how about this one:
Split this into two functions:

  • setDatabaseStatusAsWorkable which always sets it as workable (called after clearing sensitive data)
  • attemptToSetDatabaseStatusAsWorkable or indicateSchemaCreation or something similar which is called after creating a query executor and will set the database as workable when there was no operations failure?
kamil: Returning the value or throwing an error will not change anything since the caller will have to…
marcinUnsubmitted
Not Done
Inline Actions
  1. Why will the caller have to ignore information that we couldn't set database status because of DB_OPERATIONS_FAILURE?
  2. Assuming the answer to 1 convincing then the approach you described is worth implementing in my opinion.
  3. If you have an idea on how to improve your diffs, you don't have to wait for any reviewer approving you idea. You can just go ahead, implement it and request review afterwards.
marcin: 1. Why will the caller have to ignore information that we couldn't set database status because…
kamilAuthorUnsubmitted
Done
Inline Actions
  1. Why will the caller have to ignore information that we couldn't set database status because of DB_OPERATIONS_FAILURE?

I edited the code and added the comment, should be understandable right now

  1. Assuming the answer to 1 convincing then the approach you described is worth implementing in my opinion.

As we discussed in the office - is convincing

  1. If you have an idea on how to improve your diffs, you don't have to wait for any reviewer approving you idea. You can just go ahead, implement it and request review afterwards.

I wasn't sure about this idea, and wanted to verify with someone else

kamil: > 1. Why will the caller have to ignore information that we couldn't set database status…
comm::CommSecureStore commSecureStore{}; comm::CommSecureStore commSecureStore{};
if (afterClearingDatabase) {
commSecureStore.set(DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_WORKABLE); commSecureStore.set(DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_WORKABLE);
return;
}
folly::Optional<std::string> databaseManagerStatus =
commSecureStore.get(DATABASE_MANAGER_STATUS_KEY);
if (!databaseManagerStatus.hasValue() ||
databaseManagerStatus.value() != DB_OPERATIONS_FAILURE) {
commSecureStore.set(DATABASE_MANAGER_STATUS_KEY, DB_MANAGER_WORKABLE);
}
} }
bool DatabaseManager::checkIfDatabaseNeedsDeletion() { bool DatabaseManager::checkIfDatabaseNeedsDeletion() {
comm::CommSecureStore commSecureStore{}; comm::CommSecureStore commSecureStore{};
folly::Optional<std::string> databaseManagerStatus = folly::Optional<std::string> databaseManagerStatus =
commSecureStore.get(DATABASE_MANAGER_STATUS_KEY); commSecureStore.get(DATABASE_MANAGER_STATUS_KEY);
return databaseManagerStatus.hasValue() && return databaseManagerStatus.hasValue() &&
databaseManagerStatus.value() == DB_MANAGER_SECOND_FAILURE; (databaseManagerStatus.value() == DB_MANAGER_SECOND_FAILURE ||
databaseManagerStatus.value() == DB_OPERATIONS_FAILURE);
}
void DatabaseManager::reportDBOperationsFailure() {
ashoatUnsubmitted
Not Done
Inline Actions

If this is just to call expo-secure-store from JS, wonder if we should use their API directly. I guess the downside is that the constants would have to be redefined in JS?

ashoat: If this is just to call `expo-secure-store` from JS, wonder if we should use their API directly.
kamilAuthorUnsubmitted
Done
Inline Actions

Yea, I had in mind 3 reasons to not use expo-secure-store directly from js:

  • redefining constant (I think we can somehow pass this via CommCoreModule to js but it'll increase complexity)
  • I wanted to keep logic responsible for setting flags (connected to errors) in one consistent place, in this class instead of dividing this between C++ class/some js functions
  • using expo-secure-store is a new pattern in the codebase so I preferred to stick to the approach we already use

But if someone has better arguments against the current solution I am open to changing this

kamil: Yea, I had in mind 3 reasons to not use `expo-secure-store` directly from js: - redefining…
bartekUnsubmitted
Not Done
Inline Actions

Technically, calling expo-secure-store directly from JS doesn't make any difference, so your reasons seem valid to me (maybe besides the last one ;) ).

bartek: Technically, calling `expo-secure-store` directly from JS doesn't make any difference, so your…
comm::CommSecureStore commSecureStore{};
commSecureStore.set(DATABASE_MANAGER_STATUS_KEY, DB_OPERATIONS_FAILURE);
} }
} // namespace comm } // namespace comm