Fix: nullish keys not removed/merged in IDB-Keyval (web)

[chrispader]

@tgolen @marcaaron

Details

On web in the new IDB-Keyval implementation, when merging null or undefined, the keys are not removed from storage, like it's done on native with SQLite.

Additionally, top-level nullish keys in an object should also be removed when merging. E.g. when merging an existing value {a: 1, b: 2} with {a: null} or {a: undefined} the resulting value should be {b: 2}.

This PR aims to fix caused problems like this one.

Related Issues

Expensify/App#26627

Automated Tests

Linked PRs

[tgolen]

When would it be possible for value to be undefined? Wouldn't that mean that one of the pairs has only a key?

[marcaaron]

The value part of the pair could also be undefined like ['something', undefined].

[marcaaron]

I am sort of wondering how/why that happened though. It seems inconsistent with the merge behavior. Setting something to null has a special significance (i.e. delete me). Setting it to undefined does not mean anything.

I would guess that we would just throw these values out vs. allow them to overwrite / delete any keys.

[chrispader]

Just added a [HOLD on #334] tag to this PR. The SQLite crash PR actually fixes two things that are expected for this PR as well.

• Allow undefined values to be passed to fastMerge
• Remove key in Onyx.merge if null is passed

So we'll first want to merge the other PR in order for this to continue 👍

[chrispader]

This way, the extra condition in IDBKeyVal.multiMerge is not needed anymore

[marcaaron]

The value part of the pair could also be undefined like ['something', undefined].

[marcaaron]

I am sort of wondering how/why that happened though. It seems inconsistent with the merge behavior. Setting something to null has a special significance (i.e. delete me). Setting it to undefined does not mean anything.

I would guess that we would just throw these values out vs. allow them to overwrite / delete any keys.

[mountiny]

@chrispader Can we make sure the tests cover this scenario and make sure that setting key to null or undefined will remove it with either storage? I am surprised this was not caught with automated tests

[chrispader]

@chrispader Can we make sure the tests cover this scenario and make sure that setting key to null or undefined will remove it with either storage? I am surprised this was not caught with automated tests

Yes, i'll update the tests accordingly 👍

Btw, over here, we decided not to remove keys when the value is set to undefined, but instead only remove the value/set it to undefined. Setting the value to null will still remove the key from storage

[chrispader]

@marcaaron another thought... When we merge an object with another object, where every top-level key is set to null, it will result in {}. Should we instead set the value to undefined in this case or leave the empty object in storage as it is?

[chrispader]

When we merge an object with another object, where every top-level key is set to null, it will result in {}. Should we instead set the value to undefined in this case or leave the empty object in storage as it is?

I am not sure why it would make a difference (apart from less space taken up). One thought is that if the "cache" knows the object doesn't exist then on the next attempt to "read" we should not need try and read it from storage. So, there is probably an optimization to consider with that if we would do that vs. having the "cache" know that the value doesn't exist vs. not sure if it exists or not and have to read it again.

I see your point. If we set the value to undefined instead of leaving it as {}, Onyx will think there is no value in cache and therefore try to read it from storage, right?

In this case i also think it would be better to leave the {} in storage. I think we should favor less computing rather than a bit less disk space..

Wdyt @tgolen ?

[tgolen]

It would be great if we could have both performance and less storage. My suggestion would be to remove it from storage, but leave something in the cache that signals "no, don't try to read this value from storage, it's been deleted, so just return nothing".

That does make the caching layer a bit out-of-sync from what's in storage though.

If we don't want to make a change like that, then I think sticking with {} is OK for now. We don't need to preoptimize it until it's a problem.

[chrispader]

Agree. Also in favor of leaving {} in storage for now.

The current implementation achieves exactly this, so this should be ready for merging 👍🏼

[tgolen]

I think it's just waiting on @marcaaron for this one. I wasn't sure that all of Marc's concerns were addressed so I didn't want to dismiss his review and merge this yet.

[marcaaron]

Heads up, I'm probably not going to get to this today - so if you need to move forward without me, please go ahead.

[chrispader]

gonna have to resolve conflicts tomorrow morning anyway, so you can still chime in if you want @marcaaron

[chrispader]

gonna have to resolve conflicts tomorrow morning anyway, so you can still chime in if you want @marcaaron

merge conflicts resolved 👍

[chrispader]

@tgolen ill leave it up to you if you want to wait for one more review by @marcaaron

To get it merged.

[pradeepmdk]

@chrispader This is not yet resolved because when const prev = values[index]; has the value means still taking the old value. not remove the key from Onyx when we pass null in the merge command.
https://github.com/margelo/react-native-onyx/blob/cad1b560828d935aff83d3f16c919e125135ff03/lib/storage/providers/IDBKeyVal.js#L57

solution:
Option 1) just we need to check this here Utils.removeNullObjectValues in IDBKeyVal.js place
and need removed from lib/Onyx.js on the merge function
Option 2) we need pick the Object.keys from new value and based on keys only we need the select the key from prev value

and removeNullObjectValues funciton currently we are checking the null only we missed to check undefined case. we need change isNull => isNil

[pradeepmdk]

adding ref video

85f1d156-bde3-4446-b7c7-edeaac0e67a6.mp4

[chrispader]

and removeNullObjectValues funciton currently we are checking the null only we missed to check undefined case. we need change isNull => isNil

This is actually intended as described by @marcaaron in this GH comment. @marcaaron should we maybe overthink this? Currently, we'll leave undefined values in storage for top-level object keys

[chrispader]

@chrispader This is not yet resolved because when const prev = values[index]; has the value means still taking the old value. not remove the key from Onyx when we pass null in the merge command.

I agree, that this isn't optimal yet. This might be part of a bigger change though, because we don't really need to read the existing value from storage in Onyx.merge anymore, since we already read it in the "storage layer" for web (SQLite) and native (IDBKeyVal).

I'm gonna change this so that IDBKeyval.multiMerge isn't merging the same data a second time.

[chrispader]

@chrispader This is not yet resolved because when const prev = values[index]; has the value means still taking the old value. not remove the key from Onyx when we pass null in the merge command. https://github.com/margelo/react-native-onyx/blob/cad1b560828d935aff83d3f16c919e125135ff03/lib/storage/providers/IDBKeyVal.js#L57

solution: Option 1) just we need to check this here Utils.removeNullObjectValues in IDBKeyVal.js place and need removed from lib/Onyx.js on the merge function Option 2) we need pick the Object.keys from new value and based on keys only we need the select the key from prev value

and removeNullObjectValues funciton currently we are checking the null only we missed to check undefined case. we need change isNull => isNil

Fixed this issue in this PR: #359