Skip to content

Commit 109b22e

Browse files
tgolentgolen
authored
Merge pull request Expensify#754 from Expensify/chuckdries/allow-merge-empty-array
Allow MERGE operations into empty array values
2 parents 434871e + 9ce1bc7 commit 109b22e

4 files changed

Lines changed: 111 additions & 5 deletions

File tree

‎lib/Onyx.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,13 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
237237

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

‎lib/OnyxUtils.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,7 +1343,13 @@ function setWithRetry<TKey extends OnyxKey>({key, value, options}: SetParams<TKe
13431343
}
13441344

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

15781584
const existingKeyCollection = existingKeys.reduce((obj: OnyxInputKeyValueMapping, key) => {
1579-
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(resultCollection[key], cachedCollectionForExistingKeys[key]);
1580-
1585+
const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(
1586+
resultCollection[key],
1587+
cachedCollectionForExistingKeys[key],
1588+
);
1589+
1590+
if (isEmptyArrayCoercion) {
1591+
// Merging an object into an empty array isn't semantically correct, but we allow it
1592+
// in case we accidentally encoded an empty object as an empty array in PHP. If you're
1593+
// looking at a bugbot from this message, we're probably missing that key in OnyxKeys::KEYS_REQUIRING_EMPTY_OBJECT
1594+
Logger.logAlert(`[ENSURE_BUGBOT] Onyx mergeCollection called on key "${key}" whose existing value is an empty array. Will coerce to object.`);
1595+
}
15811596
if (!isCompatible) {
15821597
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'mergeCollection', existingValueType, newValueType));
15831598
return obj;

‎lib/utils.ts‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,27 @@ function formatActionName(method: string, key?: OnyxKey): string {
207207
}
208208

209209
/** validate that the update and the existing value are compatible */
210-
function checkCompatibilityWithExistingValue(value: unknown, existingValue: unknown): {isCompatible: boolean; existingValueType?: string; newValueType?: string} {
210+
function checkCompatibilityWithExistingValue(
211+
value: unknown,
212+
existingValue: unknown,
213+
): {isCompatible: boolean; existingValueType?: string; newValueType?: string; isEmptyArrayCoercion?: boolean} {
211214
if (!existingValue || !value) {
212215
return {
213216
isCompatible: true,
214217
};
215218
}
219+
220+
// PHP's associative arrays cannot distinguish between an empty list and an
221+
// empty object, so it encodes both as []. A key that should hold an
222+
// object may arrive from the server as [] and be stored that way. If
223+
// we then try to MERGE an object into that key, the array-vs-object type check
224+
// would normally block it. Since an empty array carries no data worth
225+
// preserving, we treat it as compatible with an object update and coerce it.
226+
const isObjectValue = typeof value === 'object' && !Array.isArray(value);
227+
if (Array.isArray(existingValue) && existingValue.length === 0 && isObjectValue) {
228+
return {isCompatible: true, isEmptyArrayCoercion: true};
229+
}
230+
216231
const existingValueType = Array.isArray(existingValue) ? 'array' : 'non-array';
217232
const newValueType = Array.isArray(value) ? 'array' : 'non-array';
218233
if (existingValueType !== newValueType) {

‎tests/unit/onyxTest.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,76 @@ describe('Onyx', () => {
182182
});
183183
});
184184

185+
it('should merge an object into an empty array (treating [] as {})', async () => {
186+
let testKeyValue: unknown;
187+
188+
connection = Onyx.connect({
189+
key: ONYX_KEYS.TEST_KEY,
190+
initWithStoredValues: false,
191+
callback: (value) => {
192+
testKeyValue = value;
193+
},
194+
});
195+
196+
await Onyx.set(ONYX_KEYS.TEST_KEY, []);
197+
expect(testKeyValue).toStrictEqual([]);
198+
await Onyx.merge(ONYX_KEYS.TEST_KEY, {test: 'value'});
199+
expect(testKeyValue).toStrictEqual({test: 'value'});
200+
});
201+
202+
it('should still reject merging an object into a non-empty array', async () => {
203+
let testKeyValue: unknown;
204+
205+
connection = Onyx.connect({
206+
key: ONYX_KEYS.TEST_KEY,
207+
initWithStoredValues: false,
208+
callback: (value) => {
209+
testKeyValue = value;
210+
},
211+
});
212+
213+
await Onyx.merge(ONYX_KEYS.TEST_KEY, ['existing']);
214+
expect(testKeyValue).toStrictEqual(['existing']);
215+
await Onyx.merge(ONYX_KEYS.TEST_KEY, {test: 'value'});
216+
expect(testKeyValue).toStrictEqual(['existing']);
217+
});
218+
219+
it('should mergeCollection an object into an empty array (treating [] as {})', async () => {
220+
const collectionKey = `${ONYX_KEYS.COLLECTION.TEST_KEY}1`;
221+
let testKeyValue: unknown;
222+
223+
connection = Onyx.connect({
224+
key: ONYX_KEYS.COLLECTION.TEST_KEY,
225+
initWithStoredValues: false,
226+
waitForCollectionCallback: true,
227+
callback: (value) => {
228+
testKeyValue = value;
229+
},
230+
});
231+
232+
await Onyx.set(collectionKey, []);
233+
expect((testKeyValue as Record<string, unknown>)?.[collectionKey]).toStrictEqual([]);
234+
await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {[collectionKey]: {test: 'value'}});
235+
expect((testKeyValue as Record<string, unknown>)?.[collectionKey]).toStrictEqual({test: 'value'});
236+
});
237+
238+
it('should set an object over an empty array (treating [] as {})', async () => {
239+
let testKeyValue: unknown;
240+
241+
connection = Onyx.connect({
242+
key: ONYX_KEYS.TEST_KEY,
243+
initWithStoredValues: false,
244+
callback: (value) => {
245+
testKeyValue = value;
246+
},
247+
});
248+
249+
await Onyx.set(ONYX_KEYS.TEST_KEY, []);
250+
expect(testKeyValue).toStrictEqual([]);
251+
await Onyx.set(ONYX_KEYS.TEST_KEY, {test: 'value'});
252+
expect(testKeyValue).toStrictEqual({test: 'value'});
253+
});
254+
185255
it('should notify subscribers when data has been cleared', () => {
186256
let testKeyValue: unknown;
187257
connection = Onyx.connect({

0 commit comments

Comments
 (0)