Before splitting D3941 into smaller diffs @karol-bisztyga asked an important question:

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?

We can always iterate in foreach manner over folly::dynamic provided it is an Object or an Array. In the first case the iteration works as iteration over dictionary (key-value) pairs and as iteration over vector in the the case on an Array. 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++?

(I responded on D3941 because I saw that first... here is my response copy-pasted:)

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.

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?

I think we can skip processing the corrupted notif if it is malformed. Since we don't advance messagesCurrentAsOf in response to messages saved via notif, we will still end up downloading the same message using the standard in-app mechanism. So if we skip processing a corrupted notif, it should have a minimal impact.

Of course, it's likely that if the RawMessageInfo in the notif is corrupted, it will probably be corrupted when downloaded via the standard in-app mechanism.

The main exception I can think of – what happens if the RawMessageInfo is really long? Is it possible that it gets truncated by the notif service (APNs or FCM)? We should probably make sure we don't crash if the JSON parse fails

It looks like inconsistency I accidentally made. fields that are std::string should not be moved. We should only move fields that are std::unique_ptr<>.

According to doc: https://github.com/facebook/folly/blob/main/folly/docs/Dynamic.md, to use find() we would need to check ti against .items().end():
json.find("key") != json.items().end() not just json.find("key") != json.end() as we would do with std::map. .count() method returns 0 if and only if there is no object under the given key and it shorter to use in if statements. That is why I preferred to use count()

Additionally we use also a regular find() != end() on std::map in this function so it might be confusing for someone to see find() being checked against end() and items().end() somewhere else.

Actually we cant use reset this way since reset requires a pointer as an argument. We could use reset here in one of three ways:
localID.reset((std::make_unique<std::string>(rawMessageInfo["localID"].asString())).get());
or
localID.reset(&(std::string() = rawMessageInfo["localID"].asString()));
or declare a string variable above and use its address as an argument:
std::string localIDvalue = rawMessageInfo["localID"].asString();
localID.reset(&localIDValue);
The second version is shortest but the third one is probably the most readable. I would opt for one of these two.