Update Onyx.clear to accept passed in keys to preserve, and use multiSet to clear storage #214

yuwenmemon · 2022-12-08T18:49:40Z

@neil-marcellini please review
cc @marcaaron @tgolen

Details

Add a keysToPreserve param to clear to allow it to accept passed-in keys to preserve when clearing Onyx. To better support this, clear Onyx by using multiSet to set all keys to null (except the ones we want to preserve)

Related Issues

Expensify/App#13062

Automated Tests

Added tests in onyxClearWebStorageTest.js and onyxClearNativeStorageTest.js

Additionally, ran some quick performance tests on Android to make sure there were no regressions:

main:
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 208 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 273 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 260 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 230 - ""

yuwen-additionalDefaultsMultiSet:
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 206 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 215 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 215 - ""
DEBUG  [info] YUWEN clearStorageAndRedirect timing: 218 - ""

Linked PRs

Expensify/App#13384

…cache entry

yuwenmemon · 2022-12-08T18:51:04Z

tests/unit/onyxClearNativeStorageTest.js

@@ -73,15 +74,12 @@ describe('Set data while storage is clearing', () => {
        Onyx.clear();
        return waitForPromisesToResolve()
            .then(() => {
-                // Then the value in Onyx and the cache is the default key state
+                // Then the value in Onyx, the cache, and Storage is the default key state


A nice side effect of this change: the cache and Storage are actually in sync when clearing.

marcaaron

Changes look good to me. I haven't had a chance to test though.

lib/Onyx.js

marcaaron · 2022-12-08T19:28:05Z

tests/unit/onyxClearNativeStorageTest.js

@@ -73,15 +74,12 @@ describe('Set data while storage is clearing', () => {
        Onyx.clear();
        return waitForPromisesToResolve()
            .then(() => {
-                // Then the value in Onyx and the cache is the default key state
+                // Then the value in Onyx, the cache, and Storage is the default key state


… keys

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

tgolen · 2022-12-08T19:48:36Z

lib/Onyx.js

@@ -1025,17 +1028,22 @@ function initializeWithDefaultKeyStates() {
 * Storage.setItem() from Onyx.clear() will have already finished and the merged
 * value will be saved to storage after the default value.
 *
+ * @param {Array} keysToPreserve ONYXKEYS which values you want to keep


Suggested change

* @param {Array} keysToPreserve ONYXKEYS which values you want to keep

* @param {Array} keysToPreserve is a list of ONYXKEYS which values you don't want to be cleared with the rest of the data

The original description read a little funny to me and I think this is more clear

That's a good point Tim, I find the wording odd too, especially "which". What about this?

* @param {Array} keysToPreserve is a list of ONYXKEYS that should not be cleared with the rest of the data

lib/Onyx.js

tgolen · 2022-12-08T19:50:31Z

lib/Onyx.js

+            const defaultKeys = _.keys(defaultKeyStates);
+            const keysToClear = _.without(keys, ..._.union(keysToPreserve, defaultKeys));
+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));


Same here, why a spread operator?

Same as above

Suggested change

const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

tgolen · 2022-12-08T19:53:02Z

lib/Onyx.js

+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
+            const newStoreKeyValues = _.union(keyValuesToClear, defaultKeyValues);
+            _.each(newStoreKeyValues, (keyValue) => {


Are we still sure that we want this to be done before it's actually cleared in Onyx? Could we instead move this to multiSet().then() so that there is no possibility of a weird race condition where a subscriber has gotten a value, but the storage still hasn't cleared it yet?

Yeah so switching this around breaks the test here:

react-native-onyx/tests/unit/onyxClearNativeStorageTest.js

Lines 46 to 66 in 335aca0

it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => {

expect.assertions(3);

// Given that Onyx is completely clear

// When Onyx.clear() is called

Onyx.clear();

// When merge is called between the cache and storage clearing, on a key with a default key state

Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);

return waitForPromisesToResolve()

.then(() => {

// Then the value in Onyx, the cache, and the storage is the merged value

expect(onyxValue).toBe(MERGED_VALUE);

const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);

expect(cachedValue).toBe(MERGED_VALUE);

// Use parseInt to convert the string value from the storage mock back to an integer

return Storage.getItem(ONYX_KEYS.DEFAULT_KEY)

.then(storedValue => expect(parseInt(storedValue, 10)).toBe(MERGED_VALUE));

});

});

Where the onyxValue ends up being the DEFAULT_VALUE instead. What I think is happening is that because we're calling notifySubscribersOnNextTick only after clearing the storage that tick happens at the same time / after the merge call and this thus the DEFAULT_VALUE is preserved.

@neil-marcellini thoughts on this? I know you dealt with this weird race condition and clear before...

Yeah so the problem we had with Onyx.clear() for the transition flows is that after Onyx cleared and the previous user was signed out, we would sign in the new transitioning user (with an Onyx merge), and then the default Onyx value would get set for the session key, which would sign the user out again.

So yes we need to make sure, that clear() followed by merge() on a key with a default state results in the merged value getting saved.

OK, that's a nasty race condition, and I think notifySubscribersOnNextTick really bites us in that instance. It would be great to move a lot of the context from these comments into the code comment.

@neil-marcellini feel free to take a look at the updated comment I added above this and see if you agree 👍

tgolen · 2022-12-08T19:54:40Z

lib/Onyx.js

+            const keysToClear = _.without(keys, ..._.union(keysToPreserve, defaultKeys));
+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
+            const newStoreKeyValues = _.union(keyValuesToClear, defaultKeyValues);


It's a bit confusing at this point what exactly is being done with all this logic. Maybe adding some comments will help? Maybe explaining why we need to keep doing stuff with default keys, and why we are doing all the keys(), without(), map(), pairs(), union(). It's all just a tad confusing.

Thanks for the additional comments! I think what is still confusing is some of the variable names.

keyValuesToClear sounds like it would be the thing that eventually is passed to multiSet() just by the name of it

newStoreKeyValues is what actually gets passed to multiSet() but the name doesn't really imply that at all. "New" is pretty meaningless in the name of this variable

What I would suggest is to start with an empty variable:

const keyValuesToClear = [];

Then as you progress through the logic, things will get added to that array.

keyValuesToClear.push(defaultKeyValues);

Eventually, that's what gets passed to multiSet

Storage.multiSet(keyValuesToClear);

tgolen · 2022-12-08T19:59:47Z

tests/unit/onyxClearNativeStorageTest.js

+        return waitForPromisesToResolve()
+            .then(() => {
+                // Then the value in Onyx and the cache for the default key is the default key state
+                expect(onyxValue).toBe(DEFAULT_VALUE);


It appears like these assertions are already done in the test above. Why is it necessary to test them again? Just to ensure that passing a list of keys doesn't change the default behavior? If so, maybe we can DRY this up a little bit.

Just to ensure that passing a list of keys doesn't change the default behavior?

Yeah, that was the thought. Although yeah I think it's kind of redundant just thinking about the code. I think it'll be clearer just to remove this in hindsight 👍

tests/unit/onyxClearNativeStorageTest.js

tgolen · 2022-12-08T20:02:17Z

tests/unit/onyxClearNativeStorageTest.js

+    });
+
+    it('should preserve the value of any keysToPreserve passed in', () => {
+        expect.assertions(6);


Is there a purpose for this?

Yes, it helps to make sure that assertions within async callbacks are actually called during the test. In my experience it's really easy to mess up and have an assertion that jest does not wait for. https://jestjs.io/docs/expect#expectassertionsnumber

Huh, interesting. I've never used it, but the point about async assertions makes sense.

tgolen · 2022-12-08T20:03:18Z

tests/unit/onyxClearNativeStorageTest.js

+        Onyx.set(ONYX_KEYS.REGULAR_KEY, SET_VALUE);
+
+        // When clear is called with a key to preserve
+        Onyx.clear([ONYX_KEYS.REGULAR_KEY]);


I think you want to be sure that the clear() doesn't happen until set() is done, so I would chain those together.

tgolen · 2022-12-08T20:03:56Z

tests/unit/onyxMultiMergeWebStorageTest.js

@@ -141,7 +141,11 @@ describe('Onyx.mergeCollection() amd WebStorage', () => {
    });

    it('setItem() and multiMerge()', () => {
-        expect(localforageMock.storageMap).toEqual({});
+        expect(localforageMock.storageMap).toEqual({
+            test_1: null,


Why this change?

I'm pretty sure he made this change because now instead of removing all of the keys and values when clearing Storage, the keys remain but all the values are set to null.

ahhh, OK. Could you add a code comment about that please?

neil-marcellini

This approach looks great. I have two small comments.

tests/unit/onyxClearNativeStorageTest.js

yuwenmemon · 2022-12-08T23:08:09Z

Updated!

tgolen

Sorry for being such a stickler for all the comments! This code is really complex and it's very important that intention and logic are super clear.

lib/Onyx.js

tgolen · 2022-12-09T16:02:53Z

lib/Onyx.js

+            const defaultKeys = _.keys(defaultKeyStates);
+            const keysToClear = _.without(keys, ..._.union(keysToPreserve, defaultKeys));
+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));


Suggested change

const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

tgolen · 2022-12-09T16:05:26Z

lib/Onyx.js

+            // If keysToPreserve contains a key that also has a default key state, we'll preserve it rather than reset
+            // it to the default.


Can you please update this comment to replace the two instances of "it" with what the reference is? It's a little ambiguous and makes it hard to understand. The comment could also do a little bit better job of explaining why we want this behavior and the intention behind it. Why do we handle the conflict between keysToPreserve and defaultKeyStates this way as opposed to some other way?

tgolen · 2022-12-09T16:10:10Z

lib/Onyx.js

+            const keysToClear = _.without(keys, ..._.union(keysToPreserve, defaultKeys));
+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
+            const newStoreKeyValues = _.union(keyValuesToClear, defaultKeyValues);


Thanks for the additional comments! I think what is still confusing is some of the variable names.

keyValuesToClear sounds like it would be the thing that eventually is passed to multiSet() just by the name of it

newStoreKeyValues is what actually gets passed to multiSet() but the name doesn't really imply that at all. "New" is pretty meaningless in the name of this variable

What I would suggest is to start with an empty variable:

const keyValuesToClear = [];

Then as you progress through the logic, things will get added to that array.

keyValuesToClear.push(defaultKeyValues);

Eventually, that's what gets passed to multiSet

Storage.multiSet(keyValuesToClear);

tgolen · 2022-12-09T16:13:09Z

lib/Onyx.js

+            const keyValuesToClear = _.map(keysToClear, key => [key, null]);
+            const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
+            const newStoreKeyValues = _.union(keyValuesToClear, defaultKeyValues);
+            _.each(newStoreKeyValues, (keyValue) => {


OK, that's a nasty race condition, and I think notifySubscribersOnNextTick really bites us in that instance. It would be great to move a lot of the context from these comments into the code comment.

tests/unit/onyxClearNativeStorageTest.js

tests/unit/onyxMultiMergeWebStorageTest.js

Co-authored-by: Tim Golen <tgolen@expensify.com>

yuwenmemon · 2022-12-09T19:30:56Z

Updated!

lib/Onyx.js

neil-marcellini · 2022-12-09T19:36:45Z

lib/Onyx.js

-            // Combine the pairs of keys we're clearing and keys we're setting back to default for our multiSet call
-            const newStoreKeyValues = _.union(keyValuesToClear, defaultKeyValues);
+            // Add the default key value pairs to the keyValuesToClear so that they get set back to their default values when we clear Onyx
+            keyValuesToClear.push(...defaultKeyValuePairs);


Nitpick: what about keyValuesToReset? We aren't clearing the defaultKeys we are actually resetting them.

Co-authored-by: Neil Marcellini <neil@expensify.com>

lib/Onyx.js

neil-marcellini · 2022-12-09T19:51:14Z

lib/Onyx.js

+            // We do this before clearing Storage so that any call to clear() followed by merge() on a key with a default state results in the merged value getting saved, since the update
+            // from the merge() call would happen on the tick after the update from this clear()


I like this 👍

yuwenmemon · 2022-12-09T20:00:54Z

Updated!

tgolen

Looks great now, thanks for all the attention on those comments!

yuwenmemon added 11 commits December 6, 2022 12:01

Update Onyx.clear to accept passed in default values

4b6ec53

Add in tests

317a617

Fix copy pasta mistake

c660897

Fix typo in docs

6fe4b3e

Swtich to keyStatesToPreserve

af6f771

Update test comments/linting

9a82d54

Restore cleared storage values in addition to skipping resetting the …

56c674b

…cache entry

Update tests

f239a7f

Preserve additional defaults using multiset

2c2daab

Update tests because we'll now always have storage match the cache

2c5b82b

Update docs

29ba481

yuwenmemon self-assigned this Dec 8, 2022

yuwenmemon requested a review from a team as a code owner December 8, 2022 18:49

melvin-bot bot requested review from amyevans and removed request for a team December 8, 2022 18:50

yuwenmemon commented Dec 8, 2022

View reviewed changes

yuwenmemon requested a review from neil-marcellini December 8, 2022 18:53

Update tests

851da7d

yuwenmemon force-pushed the yuwen-additionalDefaultsMultiSet branch from e98f078 to 851da7d Compare December 8, 2022 19:04

marcaaron previously approved these changes Dec 8, 2022

View reviewed changes

Update to make sure that keysToPreserve takes precedence over default…

335aca0

… keys

yuwenmemon dismissed marcaaron’s stale review via 335aca0 December 8, 2022 19:36

yuwenmemon and others added 2 commits December 8, 2022 11:38

Update lib/Onyx.js

885dd11

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Remove blank line

1f54370

tgolen requested changes Dec 8, 2022

View reviewed changes

neil-marcellini requested changes Dec 8, 2022

View reviewed changes

tests/unit/onyxClearNativeStorageTest.js Outdated Show resolved Hide resolved

tests/unit/onyxClearNativeStorageTest.js Outdated Show resolved Hide resolved

PR comments

7f7f06b

yuwenmemon force-pushed the yuwen-additionalDefaultsMultiSet branch from 85b0a43 to 7f7f06b Compare December 8, 2022 21:50

tgolen requested changes Dec 9, 2022

View reviewed changes

yuwenmemon and others added 4 commits December 9, 2022 11:02

Update lib/Onyx.js

d1a14b2

Co-authored-by: Tim Golen <tgolen@expensify.com>

Update lib/Onyx.js

3bfcc8c

Co-authored-by: Tim Golen <tgolen@expensify.com>

Update tests/unit/onyxMultiMergeWebStorageTest.js

5203294

Co-authored-by: Tim Golen <tgolen@expensify.com>

Make method more clear with both variable names and comments

220a8ee

neil-marcellini reviewed Dec 9, 2022

View reviewed changes

Update lib/Onyx.js

420e677

Co-authored-by: Neil Marcellini <neil@expensify.com>

neil-marcellini reviewed Dec 9, 2022

View reviewed changes

Update comments / var names

c26f6e5

neil-marcellini approved these changes Dec 9, 2022

View reviewed changes

tgolen approved these changes Dec 9, 2022

View reviewed changes

tgolen merged commit 5686d24 into main Dec 9, 2022

tgolen deleted the yuwen-additionalDefaultsMultiSet branch December 9, 2022 22:13

yuwenmemon mentioned this pull request Dec 9, 2022

Use Onyx.clear to preserve state when logging out Expensify/App#13384

Merged

48 tasks

tgolen mentioned this pull request Dec 14, 2022

Calling Onyx.clear() does not send the right data to collection callbacks using waitForCollectionCallback Expensify/App#13584

Closed

	* @param {Array} keysToPreserve ONYXKEYS which values you want to keep
	* @param {Array} keysToPreserve is a list of ONYXKEYS which values you don't want to be cleared with the rest of the data

	const defaultKeyValues = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));
	const defaultKeyValuePairs = _.pairs(_.omit(defaultKeyStates, ...keysToPreserve));

	it('should persist the value of Onyx.merge when called between the cache and storage clearing', () => {
	expect.assertions(3);

	// Given that Onyx is completely clear
	// When Onyx.clear() is called
	Onyx.clear();

	// When merge is called between the cache and storage clearing, on a key with a default key state
	Onyx.merge(ONYX_KEYS.DEFAULT_KEY, MERGED_VALUE);
	return waitForPromisesToResolve()
	.then(() => {
	// Then the value in Onyx, the cache, and the storage is the merged value
	expect(onyxValue).toBe(MERGED_VALUE);
	const cachedValue = cache.getValue(ONYX_KEYS.DEFAULT_KEY);
	expect(cachedValue).toBe(MERGED_VALUE);

	// Use parseInt to convert the string value from the storage mock back to an integer
	return Storage.getItem(ONYX_KEYS.DEFAULT_KEY)
	.then(storedValue => expect(parseInt(storedValue, 10)).toBe(MERGED_VALUE));
	});
	});

		// If keysToPreserve contains a key that also has a default key state, we'll preserve it rather than reset
		// it to the default.

		// We do this before clearing Storage so that any call to clear() followed by merge() on a key with a default state results in the merged value getting saved, since the update
		// from the merge() call would happen on the tick after the update from this clear()

Update Onyx.clear to accept passed in keys to preserve, and use multiSet to clear storage #214

Update Onyx.clear to accept passed in keys to preserve, and use multiSet to clear storage #214

Conversation

yuwenmemon commented Dec 8, 2022 • edited Loading

Details

Related Issues

Automated Tests

Linked PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

yuwenmemon commented Dec 8, 2022

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenmemon commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenmemon commented Dec 9, 2022

tgolen left a comment

Choose a reason for hiding this comment

yuwenmemon commented Dec 8, 2022 •

edited

Loading