Skip to content
Merged
8 changes: 7 additions & 1 deletion lib/Onyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,13 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):

try {
const validChanges = mergeQueue[key].filter((change) => {
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue);
const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(change, existingValue);
if (isEmptyArrayCoercion) {
// Merging an object into an empty array isn't semantically correct, but we allow it
// in case we accidentally encoded an empty object as an empty array in PHP. If you're
// looking at a bugbot from this message, we're probably missing that key in OnyxKeys::KEYS_REQUIRING_EMPTY_OBJECT
Logger.logAlert(`[ENSURE_BUGBOT] Onyx merge called on key "${key}" whose existing value is an empty array. Will coerce to object.`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB: Is this enough data for someone to tell what went wrong and fix it? Having the key is nice, but I feel like having the value would almost be necessary. Though, I don't love the idea of logging the value since there is not a good way to know if it's sensitive or not. Is there any other data that we can include that would be helpful? I think probably the most useful thing would be the requestID (if one exists), but that sounds difficult to grab.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't see a great way to get the requestID that delivered the update. We could maybe log in SaveResponseInOnyx for any SET [] operation, but that would be potentially noisier than we want, and that wouldn't cover pusher updates. I also don't want to log the value for the reason you stated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log statements that make it to the backend will have a user email and a datetime. It'll take a bit of legwork but it's probably not impossible to work backwards from that and the onyx key and figure out what the user was doing

}
if (!isCompatible) {
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'merge', existingValueType, newValueType));
}
Expand Down
21 changes: 18 additions & 3 deletions lib/OnyxUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,13 @@ function setWithRetry<TKey extends OnyxKey>({key, value, options}: SetParams<TKe
}

// Check if the value is compatible with the existing value in the storage
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(value, existingValue);
const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(value, existingValue);
if (isEmptyArrayCoercion) {
// Setting an object over empty array isn't semantically correct, but we allow it
// in case we accidentally encoded an empty object as an empty array in PHP. If you're
// looking at a bugbot from this message, we're probably missing that key in OnyxKeys::KEYS_REQUIRING_EMPTY_OBJECT
Logger.logAlert(`[ENSURE_BUGBOT] Onyx setWithRetry called on key "${key}" whose existing value is an empty array. Will coerce to object.`);
}
if (!isCompatible) {
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'set', existingValueType, newValueType));
return Promise.resolve();
Expand Down Expand Up @@ -1576,8 +1582,17 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>(
const cachedCollectionForExistingKeys = getCachedCollection(collectionKey, existingKeys);

const existingKeyCollection = existingKeys.reduce((obj: OnyxInputKeyValueMapping, key) => {
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(resultCollection[key], cachedCollectionForExistingKeys[key]);

const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(
resultCollection[key],
cachedCollectionForExistingKeys[key],
);

if (isEmptyArrayCoercion) {
// Merging an object into an empty array isn't semantically correct, but we allow it
// in case we accidentally encoded an empty object as an empty array in PHP. If you're
// looking at a bugbot from this message, we're probably missing that key in OnyxKeys::KEYS_REQUIRING_EMPTY_OBJECT
Logger.logAlert(`[ENSURE_BUGBOT] Onyx mergeCollection called on key "${key}" whose existing value is an empty array. Will coerce to object.`);
}
if (!isCompatible) {
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType));
return obj;
Expand Down
17 changes: 16 additions & 1 deletion lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,27 @@ function formatActionName(method: string, key?: OnyxKey): string {
}

/** validate that the update and the existing value are compatible */
function checkCompatibilityWithExistingValue(value: unknown, existingValue: unknown): {isCompatible: boolean; existingValueType?: string; newValueType?: string} {
function checkCompatibilityWithExistingValue(
value: unknown,
existingValue: unknown,
): {isCompatible: boolean; existingValueType?: string; newValueType?: string; isEmptyArrayCoercion?: boolean} {
if (!existingValue || !value) {
return {
isCompatible: true,
};
}

// PHP's associative arrays cannot distinguish between an empty list and an
// empty object, so it encodes both as []. A key that should hold an
// object may arrive from the server as [] and be stored that way. If
// we then try to MERGE an object into that key, the array-vs-object type check
// would normally block it. Since an empty array carries no data worth
// preserving, we treat it as compatible with an object update and coerce it.
const isObjectValue = typeof value === 'object' && !Array.isArray(value);
if (Array.isArray(existingValue) && existingValue.length === 0 && isObjectValue) {
return {isCompatible: true, isEmptyArrayCoercion: true};
}

const existingValueType = Array.isArray(existingValue) ? 'array' : 'non-array';
const newValueType = Array.isArray(value) ? 'array' : 'non-array';
if (existingValueType !== newValueType) {
Expand Down
70 changes: 70 additions & 0 deletions tests/unit/onyxTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,76 @@ describe('Onyx', () => {
});
});

it('should merge an object into an empty array (treating [] as {})', async () => {
let testKeyValue: unknown;

connection = Onyx.connect({
key: ONYX_KEYS.TEST_KEY,
initWithStoredValues: false,
callback: (value) => {
testKeyValue = value;
},
});

await Onyx.set(ONYX_KEYS.TEST_KEY, []);
expect(testKeyValue).toStrictEqual([]);
await Onyx.merge(ONYX_KEYS.TEST_KEY, {test: 'value'});
expect(testKeyValue).toStrictEqual({test: 'value'});
});

it('should still reject merging an object into a non-empty array', async () => {
let testKeyValue: unknown;

connection = Onyx.connect({
key: ONYX_KEYS.TEST_KEY,
initWithStoredValues: false,
callback: (value) => {
testKeyValue = value;
},
});

await Onyx.merge(ONYX_KEYS.TEST_KEY, ['existing']);
expect(testKeyValue).toStrictEqual(['existing']);
await Onyx.merge(ONYX_KEYS.TEST_KEY, {test: 'value'});
expect(testKeyValue).toStrictEqual(['existing']);
});

it('should mergeCollection an object into an empty array (treating [] as {})', async () => {
const collectionKey = `${ONYX_KEYS.COLLECTION.TEST_KEY}1`;
let testKeyValue: unknown;

connection = Onyx.connect({
key: ONYX_KEYS.COLLECTION.TEST_KEY,
initWithStoredValues: false,
waitForCollectionCallback: true,
callback: (value) => {
testKeyValue = value;
},
});

await Onyx.set(collectionKey, []);
expect((testKeyValue as Record<string, unknown>)?.[collectionKey]).toStrictEqual([]);
await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {[collectionKey]: {test: 'value'}});
expect((testKeyValue as Record<string, unknown>)?.[collectionKey]).toStrictEqual({test: 'value'});
});

it('should set an object over an empty array (treating [] as {})', async () => {
let testKeyValue: unknown;

connection = Onyx.connect({
key: ONYX_KEYS.TEST_KEY,
initWithStoredValues: false,
callback: (value) => {
testKeyValue = value;
},
});

await Onyx.set(ONYX_KEYS.TEST_KEY, []);
expect(testKeyValue).toStrictEqual([]);
await Onyx.set(ONYX_KEYS.TEST_KEY, {test: 'value'});
expect(testKeyValue).toStrictEqual({test: 'value'});
});

it('should notify subscribers when data has been cleared', () => {
let testKeyValue: unknown;
connection = Onyx.connect({
Expand Down
Loading