Fix: Remove nullish values on set and on merge when no value is in storage

[chrispader]

@tgolen @marcaaron @hannojg

In the native implementation with SQLite, the first call to merge on a key where no value is in storage will act like a regular set since the values are inserted into storage directly.

This is due to this SQLite operation in SQLiteStorage.mergeItem:

INSERT INTO keyvaluepairs (record_key, valueJSON)
VALUES (:key, JSON(:value))
ON CONFLICT DO UPDATE
SET valueJSON = JSON_PATCH(valueJSON, JSON(:value));

In order for this to be consistent with how later merge with JSON_PATCH are working, we need to remove nullish values from the batchedChanged in the first merge operation.

Additionally, we'll want to remove nullish values on set as well, so that the values in cache and storage are always identical.

Details

Related Issues

Automated Tests

Linked PRs

[hannojg]

To give another explanation to the issue at hand:

Calling Onyx.merge for a new key:

• Onyx.merge(newKey, { prop1: 12, prop2: null})
• Calls INSERT INTO as there is no conflict
• In storage we now have: { prop1: 12, prop2: null}

Where as, when...

Calling Onyx.merge for an existing key:

• Onyx.merge(key, { prop1: 12, prop2: null})
• Calls JSON_PATCH as we have a conflict
• In storage we now have: { prop1: 12 }

First of all, this is inconsistent behaviour, and secondly in the app code we don't expect the null values to be persisted, so it creates issues like this one:

• [HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] Home address - Page constantly crashes if insert values and quit the app App#24006

[mountiny]

Looking good but also tagging Marc and Tim for review

[tgolen]

Left some comments to hopefully help the comments to be more clear to others.

[marcaaron]

Code LGTM. Agree with most of Tim's comments and think we could explain this edge case a bit better with some inline comments. @hannojg's description of the problem was very clear to me so maybe it can be incorporated somehow.

[hannojg]

Awaiting another round of review, are we good to continue? This is blocking:

• Reapply onyx upgrade use cache with fixes App#24041

@mountiny @marcaaron @tgolen

[tgolen]

Love the comments. Very clear now. Thank you!