Page MenuHomePhabricator

Implement interface for translation from raw message string into SQLite database entities
ClosedPublic

Authored by marcin on May 6 2022, 2:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 3:58 AM
Unknown Object (File)
Sat, Nov 9, 9:47 AM
Unknown Object (File)
Sat, Nov 9, 7:49 AM
Unknown Object (File)
Fri, Nov 8, 11:56 AM
Unknown Object (File)
Sat, Nov 2, 2:29 PM
Unknown Object (File)
Sat, Nov 2, 12:28 PM
Unknown Object (File)
Sat, Nov 2, 12:28 PM
Unknown Object (File)
Sat, Nov 2, 11:02 AM

Details

Summary

Defines in C++ functions that are implemented in message-ops-utils.js

Test Plan

No testing plan. Future diffs will introduce code that was used on iOS and Andrdoid to parse raw message strings and store them in database.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This diff is quite big, it might be a good idea to add each function in a separate diff.

native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp
17 ↗(On Diff #12415)

why do you use snake instead of camel case?

32 ↗(On Diff #12415)

How does this work with folly::dynamic? Don't we need to check whether it is a collection and if the field is present in the first place?

44–47 ↗(On Diff #12415)

camel case

native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h
26 ↗(On Diff #12415)

new line missing

This revision now requires changes to proceed.May 9 2022, 8:07 AM
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp
32 ↗(On Diff #12415)

We can always iterate in foreach manner over folly::dynamic provided it is an Object or an Array. In the first case the iteration is similar to iteration over dictionary (key-value) pairs and similar to iteration over vector in the latter. I understand there are several places in this code where I assume that a certain key is present or the value under certain key is an Object or an Array. On one hand it would be justifiable to add sanity checks in those places and throw some error/log and return if the condition is not fulfilled. On the other this code is meant to mirror functionality implemented in message-ops-utils.js where assumptions about a presence of a certain key or character ot the value are not additionally verified. Therefore I assumed it might be an overkill to introduce such sanity checks in C++ version of the code. However it is not an issue to provide such checks. @ashoat, @palys-swm what do you think? Can I assume that if presence of a key or value character in notification is not verified in JS then it does not need to be verified in C++?

Use renamed message Specs dictionary, use camel case for local variables

Reserve this for interface definition

marcin retitled this revision from Implement translation from raw message string into SQLite database entities to Implement interface for translation from raw message string into SQLite database entities.May 10 2022, 2:44 AM
marcin edited the summary of this revision. (Show Details)
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp
32 ↗(On Diff #12415)

On the other this code is meant to mirror functionality implemented in message-ops-utils.js where assumptions about a presence of a certain key or character ot the value are not additionally verified.

In our JS codebase, I think these assumptions are generally enforced statically by the Flow typechecker. But I don't think data returned from the server is verified at runtime, so the assumptions made by the typechecker could be wrong.

I think it's probably okay to make these assumptions since we do the same in JS, but I guess it would be better to do some sort of runtime checks when the data is received from the server. I guess that's probably outside the scope of this diff?

I honestly don't feel too strongly, open to other perspectives.

native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp
32 ↗(On Diff #12415)

I wouldn't say it is out of the scope of this diff. Instead of optimistically accessing keys we could use if statements or ternary operators to check for presence of given key or whether value we expect under the given key is indeed and Object or an Array so that it can be iterated over. The only difficulty I see in performing runtime checks is how should we react when those checks fail? Probably we should throw an error or log error message and return, but how do we make sure that corrupted notification is re-send in a correct form, or is at least downloaded in its correct form upon app launch?

native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.cpp
32 ↗(On Diff #12415)

It may be good to create a helper function for this. It would assign a value if it is present or throw/do sth else when it is not present.

karol added 1 blocking reviewer(s): tomek.
tomek requested changes to this revision.May 11 2022, 4:07 AM

I think it's fine to don't have a test plan for a diff which introduces just the interface. We could also mention that e.g. it compiles.

native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h
12 ↗(On Diff #12477)

Why do we have a class with all the functions static? What is the benefit of this approach compared to plain functions in comm namespace?

14 ↗(On Diff #12477)

It might be a good idea to have e.g. a typedef for it - it would be more readable to see what std::pair<Message, std::vector<Media>> actually means for us

22 ↗(On Diff #12477)

This function returns a vector, so maybe we should use plural messageInfos in the name?

This revision now requires changes to proceed.May 11 2022, 4:07 AM
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h
12 ↗(On Diff #12477)

I am about to make some of those functions private since they may throw exception that will be handled in public methods of this class. Another reason to have this functionality contained within a class is that we are going to use them in Java on Android in CommNotificationsHandler.java

Use typedef for pair of Message and vector of Media. Use private access specifier for functions that may crash.

tomek requested changes to this revision.May 12 2022, 1:27 AM
tomek added inline comments.
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h
14

All the members of a class are private by default, so there's no need to specify that

24

THis code doesn't seem to be properly formatted. We probably need to update clangPaths

24–25

As suggested in other diff, should we take these by reference?

This revision now requires changes to proceed.May 12 2022, 1:27 AM

Pass message string by reference

tomek added inline comments.
native/cpp/CommonCpp/NativeModules/MessageOperationsUtilities/MessageOperationsUtilities.h
24

Do we know if this is properly formatted?

This revision is now accepted and ready to land.May 16 2022, 10:00 AM
ashoat added inline comments.
native/android/app/CMakeLists.txt
75–76 ↗(On Diff #12725)

Would be nice if there was a way to just tell CMake to recursively include a directory and all of its subdirectories. @karol-bisztyga found something like this, but not for include_directories I think. @jimpo, any ideas?

Renamse storeNotification to storeMessageInfos

ashoat added inline comments.
native/android/app/CMakeLists.txt
75–76 ↗(On Diff #12725)

Ping on this @marcinwasowicz @karol-bisztyga @geekbrother @jimpo any way to recursively include a directory in CMake?

native/android/app/CMakeLists.txt
75–76 ↗(On Diff #12725)

Something like

file(GLOB_RECURSE SOURCE_CODE "./src/*.cpp")

works for finding files but here we need to find directories here.
In services, we explicitly add every subdirectory. I'm not sure if there's a solution to do this recursively. Maybe there is but I'm not aware of it.

Move to persistent storage utilities