diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 2b836bb1..3da6a4de 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -237,7 +237,13 @@ function merge(key: TKey, changes: OnyxMergeInput): 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.`); + } if (!isCompatible) { Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'merge', existingValueType, newValueType)); } diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 3e884e2b..1a9b11bb 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1343,7 +1343,13 @@ function setWithRetry({key, value, options}: SetParams( 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; diff --git a/lib/utils.ts b/lib/utils.ts index d7d29653..d057bf41 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -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) { diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 67e6d1cb..bfafd076 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -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)?.[collectionKey]).toStrictEqual([]); + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {[collectionKey]: {test: 'value'}}); + expect((testKeyValue as Record)?.[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({