[$250] Add some basic runtime type validation in Onyx

[roryabraham]

coming from https://github.com/Expensify/Expensify/issues/363913...

Problem

It is possible to queue incorrect Onyx updates from Auth. Twice now, we have seen an array used in place of an object in an Onyx update in Auth, which when merged completely removes the object and replaces it with an array.

Solution

While we recognize that there a number of more elegant solutions to this (compile time type validation of Onyx updates in the back-end being a prime candidate), we should apply a simple solution to prevent simple mistakes from having catastrophic effects, as we have seen occur in production twice already.

What this means is - let's update Onyx such that:

• it will not apply any updates where we try to merge an array into an object, or vice-versa
• it will log an alert if we do try to do that (except on dev, of course)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0152270829f1a6f929
• Upwork Job ID: 1777413537488273408
• Last Price Increase: 2024-04-08

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0152270829f1a6f929

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot[bot]]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We need to validate if existing items and new changes have the same type before merging in Onyx.

What is the root cause of that problem?

New change.

What changes do you think we should make in order to solve the problem?

Below is the pseudo code which needs to be added in merge method in react-native-onyx:

OnyxUtils.get(key).then(existingValue => {
  const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
  const incomingValueType = Array.isArray(changes) ? 'array' : 'object';
  if (existingValueType !== incomingValueType) {
      // Log an alert for the mismatched types except on dev
      return Promise.resolve();
  }

   // Continue with the merge

}).catch(exception => {
    // Log the exception here
    return Promise.resolve();
});
       

For set:

function set<TKey extends OnyxKey>(key: TKey, value: OnyxEntry<KeyValueMapping[TKey]>): Promise<void[]> {
  OnyxUtils.get(key).then(existingValue => {
    const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
    const incomingValueType = Array.isArray(changes) ? 'array' : 'object';

    if (existingValueType != incomingValueType) {
        // Log an alert for the mismatched types except on dev
        return Promise.resolve();
    }

   // Continue with existing logic
  })

}

For mergeCollection, change the existingKeys logic to below:

const existingKeys = []

keys.forEach((key) => {
    if (persistedKeys.has(key)) {
        OnyxUtils.get(key).then((existingValue) => {
            const existingValueType = Array.isArray(existingValue) ? 'array' : 'object';
            const incomingValueType = Array.isArray(mergedCollection[key]) ? 'array' : 'object';
            if (existingValueType === incomingValueType) {
                existingKeys.push(key)
            } else {
                // Log an alert for the mismatched types except on dev
            }
        }).catch(exception => {
            // Log the exception here
        });
    }
});

For multiSet, this would be the updated method:

function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void[]> {
    const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data);

    const updatePromises = keyValuePairs.map(([key, value]) => {
        const prevValue = cache.getValue(key, false);

        const existingValueType = Array.isArray(prevValue) ? 'array' : 'object';
        const incomingValueType = Array.isArray(value) ? 'array' : 'object';
        if (existingValueType !== incomingValueType) {
            // Log an alert for the mismatched types except on dev
            return null
        }

        // Update cache and optimistically inform subscribers on the next tick
        cache.set(key, value);
        return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue);
    });

    return Storage.multiSet(keyValuePairs)
        .catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, data))
        .then(() => {
            OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, data);
            return Promise.all(updatePromises.filter(promise => promise !== null));
        });
}

[mananjadhav]

@ShridharGoel Wouldn't we need to check if it's an Array for the existingValue as well? I am assuming we're using the existing block with OnyxUtils.get(key).then(existingValue.

@roryabraham

1. The following is true for all/any environment right?

it will not apply any updates where we try to merge an array into an object, or vice-versa

2. Would Logger.logAlert suffice for the mismatch? do we need any sendActionToDevTools?

[ShridharGoel]

Proposal

Updated to use similar check for existing value and changes.

[dominictb]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want to:

• It will not apply any updates where we try to merge an array into an object, or vice-versa
• It will log an alert if we do try to do that (except on dev, of course)

What is the root cause of that problem?

We don't have such functionality in Onyx yet

What changes do you think we should make in order to solve the problem?

A. Updates for Onyx.merge:

First there're a few considerations:

• There're 3 cases possible for a change:
  • Nullish (null, undefined)
  • Object
  • Array If existing value is Array, it can be used with Nullish/Array change, but not Object change. If existing value is Object, it can be used with Nullish/Object change, but not Array change.

So just checking Array.isArray and compare both is not enough, we need to check the Nullish case as well.

We can define a method isUpdateCompatibleWithExistingValue to validate that the update and the existing value are compatible:

function isUpdateCompatibleWithExistingValue(value, existingValue): boolean {
    if (existingValue && value && Array.isArray(existingValue) !== Array.isArray(value)) {
        return false;
    }
    return true;
}

• The changes are batched into an array, we should only remove the invalid changes, not discard the full batch just because one of the update happens to be of the wrong type.

With the above considerations, here're the pseudo code of the changes we can make in Onyx here to filter out the invalid change, Log an alert, and check the change type compatibility properly:

const validChanges = mergeQueue[key].filter((change) => isUpdateCompatibleWithExistingValue(change, existingValue));

if (validChanges.length !== mergeQueue[key].length) {
    Logger.logAlert(`A warning occurred while applying merge for key: ${key}, Warning: Trying to merge array to object or vice versa`);
}

if (!validChanges.length) {
    return undefined;
}

Then below that, when we use mergeQueue[key] like in here, we should change to use validChanges instead.

B. Updates for Onyx.set In here, we should get the existing value, then check if the new update is compatible with the existing value, if not, early return. (we can alternatively get the existing value from cache.getValue so it can be synchronous)

OnyxUtils.get(key).then(existingValue => {
        if (!isUpdateCompatibleWithExistingValue(value, existingValue)) {
            Logger.logAlert(`A warning occurred while applying set for key: ${key}, Warning: Trying to set array to object or vice versa`);
            return Promise.resolve();
        }
        
        // now we do the Onyx.set as now

C. Updates for Onyx.mergeCollection

• In here, modify so getCachedCollection accepts an optional collectionMemberKeys param, if it's passed in, we'll use it, if not, we'll use our own existing collectionMemberKeys here
• In here, get the collection value of existingKeys by using getCachedCollection and pass existingKeys as collectionMemberKeys
• In here, we check that mergedCollection[key] is compatible with the existing value from existingKeys above, if they are not compatible, Log a warning by using Logger.logAlert, and do not set to obj (so that update will be ignored)

What alternative solutions did you explore? (Optional)

For A., an alternative is to check update compatibility right in here before we push the changes to the mergeQueue. We can get the existing value by using cache.getValue and check the compatibility using isUpdateCompatibleWithExistingValue, if they are not compatible, log a warning using Logger.logAlert and early return, so the changes will never end up in the merge queue.

[melvin-bot[bot]]

📣 @dominictb! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[judy-hopps-12345]

### Proposal

Please re-state the problem that we are trying to solve in this issue. Add some basic runtime type validation in Onyx #39852

What is the root cause of that problem? The validation of the datatype for Onyx update will be done in the backend but double validation in the frontend is helpful in order to prevent simple mistakes.

What changes do you think we should make in order to solve the problem? The functions for Onyx update such as Onyx.set(), Onyx.update() are performed by write(..., onyxData, ...) function in the @libs/API/index.ts. As the parameter for Onyx, we can see a variable - onyxData(which is consist of OptimisticData and onyxDataWithoutOptimisticData). Here, OptimisticData is enough to compare the types of old and new.

There is the example of optimisticData variable below. (refer to tunction signIn())

const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.ACCOUNT,
            value: {
                ...CONST.DEFAULT_ACCOUNT_DATA,
                isLoading: true,
                loadingForm: twoFactorAuthCode ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM,
            },
        },
    ];

We can get the old value from "key" property, and compare it with "value" property.

const promise1 = new Promise((resolve, reject) => {
    if (oldValueType !== newValueType) {
        Logger.logAlert();
        Promise.reject();
    } else {
        // Do the original action
        Promise.resolve();
    }
});
Promise.all([promise1]).then((values) => {
    console.log(values);
});

What alternative solutions did you explore? (Optional) N/A

[melvin-bot[bot]]

📣 @sea1375! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[judy-hopps-12345]

Contributor details My Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01e06e26f8cd83d24f

[mananjadhav]

I think @ShridharGoel's proposal makes sense. Pending response from @roryabraham .

🎀 👀 🎀 C+ reviewed.

[melvin-bot[bot]]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[dominictb]

@mananjadhav That proposal won't work in many cases, as mentioned in my proposal.

Some examples where that solution will cause regressions:

• When we try to clear an Array key (we won't be able to clear)
• When there're many updates in update array, but only one of them is invalid
• It will only work with the first changes, since it uses the changes variable, not the mergeQueue[key] (which is the batch of changes that we apply)

Can you double check on that?

cc @roryabraham

[ShridharGoel]

When there're many updates in update array, but only one of them is invalid

At once we pass either an object or array to merge. So, if the existing value is an object and the new value is not, we'll not continue with the merge, and vice-versa.

[ShridharGoel]

We don't have to discard specific array elements.

[dominictb]

At once we pass either an object or array to merge. So, if the existing value is an object and the new value is not, we'll not continue with the merge, and vice-versa.

@ShridharGoel So is it correct that you're suggesting to run OnyxUtils.get(key).then(existingValue => to check the existingValue for every change? (without batching changes)

[james-tindal]

Any solution is likely to conflict with https://github.com/Expensify/react-native-onyx/pull/523

[melvin-bot[bot]]

📣 @james-tindal! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[roryabraham]

The following is true for all/any environment right?

correct

Would Logger.logAlert suffice for the mismatch?

yes

[roryabraham]

Sorry if this wasn't clear, but this change should apply not only to Onyx.merge, but also to Onyx.set and Onyx.mergeCollection. As such, I don't think any of the current proposals are sufficient

[judy-hopps-12345]

I think that the write() function in the @libs/API/index.ts performs all the commands such as merge(), set(), mergeCollection(), and so on by following the command type. The first parameter of write() function is command such as "ONYXKEYS.MERGE". Even though the parameter is MERGE_COLLECTION, it works.

[dominictb]

Proposal updated to handle the Onyx.set and Onyx.mergeCollection too, based on this expectation update

[kadiealexander]

@mananjadhav bump for review please!

[mananjadhav]

Will review this today/tomorrow. Going through different methods and scenarios myself.

[ShridharGoel]

Proposal

Updated

[roryabraham]

Going to move forward with @dominictb's proposal - I like how it abstracted isUpdateCompatibleWithExistingValue.

[melvin-bot[bot]]

📣 @dominictb You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖