Page MenuHomePhabricator

[native] support operations failure in error handling
ClosedPublic

Authored by kamil on Feb 13 2023, 6:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 7:17 PM
Unknown Object (File)
Mon, Mar 25, 2:04 PM
Unknown Object (File)
Mon, Mar 25, 2:04 PM
Unknown Object (File)
Mon, Mar 25, 2:03 PM
Unknown Object (File)
Tue, Mar 12, 11:49 AM
Unknown Object (File)
Wed, Feb 28, 6:16 PM
Unknown Object (File)
Wed, Feb 28, 6:15 PM
Unknown Object (File)
Wed, Feb 28, 6:15 PM
Subscribers

Details

Summary

Follow up to D5672.
If there will be an error while processing store operations we want to note this in secure store and kill app, after opening again database will be recreated.

The problem is that operation failure might occur when migration/creating database works properly - that being said after error in reducer and entry this in SecureStore, user opens app and QueryExecutor is properly created, which results in setting database status as workable.
This diff resolves that problem by adding one more possible state.

Test Plan

Test if this not break previous functionality of error handling. Check if calling reportDBOperationsFailure set value in SecureStore

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
64 ↗(On Diff #22413)

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 published this revision for review.Feb 13 2023, 6:25 AM
native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
87 ↗(On Diff #22413)

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?

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
87 ↗(On Diff #22413)

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

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
64 ↗(On Diff #22413)

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?

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.h
14 ↗(On Diff #22413)

I would prefer to name this parameter as afterClearingSensitiveData. We never clear SQLite exclusively. However up to you.

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
87 ↗(On Diff #22413)

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 ;) ).

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
64 ↗(On Diff #22413)

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?
native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.h
14 ↗(On Diff #22413)

yea you're right, I will update this before landing

native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
64 ↗(On Diff #22413)
  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.
native/cpp/CommonCpp/DatabaseManagers/DatabaseManager.cpp
64 ↗(On Diff #22413)
  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

This revision is now accepted and ready to land.Feb 27 2023, 3:05 AM