Fix: SQLite causes crash when trying to merge undefined

[chrispader]

@tgolen @marcaaron

Details

When you run Onyx.merge with the value undefined on native platforms, SQLite will crash the app, because we have a NOT NULL constraint for all values in the database.

Proposal

When undefined is passed as the value in Onyx.merge we manually remove the key from storage with Storage.removeItem and remove it from the merge delta.

Related Issues

https://expensify.slack.com/archives/C01GTK53T8Q/p1693404147209609

Automated Tests

Linked PRs

[chrispader]

@tgolen what's the desired behaviour of Onyx if a key is set to undefined? Previously, setting it to/ merging it with null will remove the key from storage.

Should we also remove the key when setting/merging undefined?

[tgolen]

Can you please expand this comment to explain why only the last objects in the batch should be merged? I'm not sure I understand the scenario or what nullish and array values have to do with it.

[MonilBhavsar]

+1
May be we want to merge the very last value in the batch - irrespective of if it is null or not null

[tgolen]

I'm not quite getting this... what about objects at the beginning or the middle of the array?

[marcaaron]

When undefined is passed as the value in Onyx.merge we manually remove the key from storage with Storage.removeItem and remove it from the merge delta.

I don't think we should do that. Just throw the value out. Probably they wanted to merge something. Not delete it and it just happened to be undefined. I think we should just act like it didn't happen. They will figure out quickly that it's the wrong method to use if they really want to remove a key.

[marcaaron]

I think this new behavior is not desirable. I propose we ignore any merging of undefined values. We never said anywhere that you can do this. People might not intentionally be doing this. It is much more likely that a value would be unintentionally undefined than unintentionally null.

[chrispader]

I think this new behavior is not desirable. I propose we ignore any merging of undefined values. We never said anywhere that you can do this. People might not intentionally be doing this. It is much more likely that a value would be unintentionally undefined than unintentionally null.

Alright 👍 Makes sense! Just wanted to be on the same page on that. Will instead just set the value to undefined. Setting/merging null will still delete the key though

[chrispader]

Please wait with reviews, fixing tests right now...

[chrispader]

@tgolen @marcaaron i moved the improvements i made to a separate PR, as this became kind of a scope creep already.

#339

[chrispader]

This PR now only contains the lines of code needed in SQLiteStorage.js to fix the crash...

[tgolen]

Thanks for decreasing the scope of this!