This diff introduces a function to convert folly dynamic thet represents media JSON object received with notification into SQLite database Media enitities.
Details
Details
No testing plan. Future diffs will provide a code that uses those functions on iOS and Android respectively
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
|---|---|---|
| 13 ↗ | (On Diff #12485) | What happens when there's nothing under a given key? Have you tested this case? Are you sure we won't encounter UB/crash here? asString may be then invoked on null or some random value from memory? I'm not sure. |
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
|---|---|---|
| 13 ↗ | (On Diff #12485) | According to folly documentation on dynamic: https://github.com/facebook/folly/blob/main/folly/docs/Dynamic.md and its implementation: https://github.com/facebook/folly/blob/main/folly/dynamic.cpp an attempt to access non-existing key throws std::out_of_range error and attempt to perform invalid conversion (e.g. asString() on some random memory ) throws folly::TypeError. Both of this can be try{} catch{}. This function is not intended to be used independently (we expect it to be used in translateRawMessageInfoToClientDBMessageInfo introduced in the next diff), so my proposal here is to make it private, let it crash in case of malicious operation and perform appropriate error handling in translateRawMessageInfoToClientDBMessageInfo where it is used to create std::vector<Media> that may potentially be empty. |
Comment Actions
An idea for a test plan might be to write a code that uses this function. This code could be invoked in any place of our app, or even as a separate main function.
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
|---|---|---|
| 6 ↗ | (On Diff #12485) | Do we use this import? |
| 23–30 ↗ | (On Diff #12485) | Are we sure it is a good idea to move these? We don't want to move when returning, but just wondering if we apply the same rule here. |
| native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp | ||
|---|---|---|
| 13 ↗ | (On Diff #12485) | By "crash" you mean throw, right? Thanks for the research, if folly throws on its own when accessing non-existent keys, then we're good here, I think. |