react-native-async-storage / async-storage

An asynchronous, persistent, key-value storage system for React Native.
https://react-native-async-storage.github.io/async-storage/
MIT License
4.61k stars 459 forks source link

`multiMerge` will *overwrite* key on iOS for '{}' #1046

Closed irisjae closed 4 months ago

irisjae commented 6 months ago

What happened?

According to documentation, the function of multiMerge is Multiple merging of existing and new values in a batch. Assumes that values are stringified JSON. Once completed, invokes callback with errors (if any).

One expects that the empty object {} is the identity of the operation of merging objects, and that merging the JSON-ified object "{}" into any value that is already an object will produce no effect.

This is the case on Android and not the case on iOS (tested with 1.19.3). The following snippet reproduces this, producing '{ "x": 1 }' for key "a" on Android, and producing '{}' on iOS.

let { default: { multiMerge, getAllKeys, multiGet } } = require ('@react-native-async-storage/async-storage')

Promise .resolve ()
.then (_ => multiMerge ([ [ 'a', '{ "x": 1 }' ] ] ))
.then (_ => multiMerge ([ [ 'a', '{}' ] ] ))
.then (_ => getAllKeys ())
.then (keys => multiGet (keys))
.then (entries => Object .fromEntries (entries))
.then (result => console .log (result))

The bug is caused by this line https://github.com/react-native-async-storage/async-storage/blob/9d9bb1aa157ce77c362e6f24639c9f2995f56e15/packages/default-storage-backend/ios/RNCAsyncStorage.mm#L791 Presumably, the line is intended to overwrite the previous value if the provided input is not an object (i.e. not mergeable). What is actually specified is overwriting of the previous value if the provided input has no keys, which unfortunately produces this bug. The corresponding implementation in Android https://github.com/react-native-async-storage/async-storage/blob/9d9bb1aa157ce77c362e6f24639c9f2995f56e15/packages/default-storage-backend/android/src/main/java/com/reactnativecommunity/asyncstorage/next/StorageSupplier.kt#L61 properly (it seems) merges the values instead, and handles the case correctly.

Version

1.19.3

What platforms are you seeing this issue on?

System Information

.

Steps to Reproduce

Provided above

github-actions[bot] commented 4 months ago

This issue has been marked as stale due to inactivity. Please respond or otherwise resolve the issue within 7 days or it will be closed.

irisjae commented 4 months ago

I've worked around this, but this is not stale; I've attached a test case and the relevant code that appears to cause the issue.

This issue potentially causes data loss.

irisjae commented 4 months ago

I don't know about the maintainers of this project, but to me, unexpected data loss issues are not very encouraging signs. I'm happy to provide a PR if maintainers show interest.

krizzu commented 4 months ago

@irisjae Thanks for the details explanation, I'll try to find some time later next week to look into that 🙏

AsyncStorageBot commented 4 months ago

:tada: This issue has been resolved in version 1.22.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: