Fix minor issues after introducing incremental JSON_PATCH merge

[chrispader]

@marcaaron

Details

Fixing some issues i encountered while testing the JSON_PATCH merge changes in the Expensify/App repo in this PR

When batching the changes from mergeQueue, top-level null values may not be omitted. Only when merging the final value, we can remove these null values.

Also added back a log line, that was removed in the other PR

Related Issues

Automated Tests

Linked PRs

[chrispader]

@marcaaron @tgolen all review comments adressed! 👍

[tgolen]

Thanks for those changes. You'll get the hang of the comments in time, so keep working on them!

[marcaaron]

+1 to @tgolen's comments about comments :)

don't we actually want to drop those keys from storage and not just set them to "null" or is it the same thing as far as SQLite is concerned?

Also, I think this is a part that could be better explained in the comments as well. JSON_PATCH() behavior will delete a top level key when the value is null like in this example...

[chrispader]

Also, I think this is a part that could be better explained in the comments as well. JSON_PATCH() behavior will delete a top level key when the value is null like in this example...

Ah i understand, didn't know, that that's why we added this manually for web. Gonna further explain this in the comments then!

[chrispader]

If we are using this method to build the "batch of changes" then don't we actually want to drop those keys from storage and not just set them to "null" or is it the same thing as far as SQLite is concerned?

When building the batch of changes, we don't want to omit the null keys, because then this would happen:

Changes like [{key1: null, key2: null}, {key3: null}] turn into modifiedData: {}. Merging the existing object value with {} will not update it though, because merging {} is the same as merging nothing.

For creating the modifiedData we'll want to omit the keys, so they get removed from storage.

[tgolen]

Getting close!

[marcaaron]

One small thing that needs to be addressed. Looks good - thanks for the changes!

[marcaaron]

One last thing 🥲

[chrispader]

One last thing 🥲

fixed!

[mountiny]

Thanks @tgolen and @marcaaron for pushing this forwards.

To unblock the other onyx caching PR, I am going to approve and merge this, given Tim already gave approval and the last changes requested by Marc has been addressed and they were just comments.

Addressed