Skip to content

feat(android): Use Room library for persistent storage #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Apr 2, 2021

Conversation

krizzu
Copy link
Member

@krizzu krizzu commented Jan 19, 2021

Summary

Current implementation of Sqlite access is cumbersome, error prone and produces some hard to reproduce errors to users. This PR aims to improve our native implementation of SQLite access and thread control.

About

This PR introduced the "next" storage implementation. Room has been used for database access, and Coroutines to handle asynchronous work.

I tried to reuse old database file, but due to it having a SQLite bug (where primary key can be null, which is a SQLite violation that Room is guarded against) I had to provide a migration path to copy over values to new database. This operation is done only once, the very first time feature is switched on.

No developer migration is necessary to start using this feature. It means that, once switched on, the same data stored so far will be available to use. No API change on JS side is involved. One notable change that can be somehow breaking is how this feature handle AsyncStorage rules violation - if key used is not string (or null) OR value is not a string, then error with proper message is thrown. No more cryptic sqlite errors.

As part of this PR and feature, some new enhancement:

  • Updated the docs, to describe usage of the feature and also added an Android section to Brownfield integration doc, to show how one can access storage from native side.
  • added new section in test app, called Basic, to test all basic test for AsyncStorage (get/set/remove, merge, getKeys, clearing db and error validation)
  • added native unit tests to validate Storage access (since it can be used from native side as well).

Test plan

Green CI, manual test on the Example App.

@krizzu krizzu force-pushed the feat/android-room branch 4 times, most recently from ec73170 to 69a6c95 Compare February 15, 2021 10:13
@krizzu krizzu marked this pull request as ready for review February 15, 2021 10:14
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

LGTM, but I haven't tested it yet. I'll try to find some time to do so but will tentatively approve in the meantime. 😄

@krizzu krizzu force-pushed the feat/android-room branch 2 times, most recently from 249dfbd to a62ce70 Compare February 26, 2021 10:28
@krizzu krizzu force-pushed the feat/android-room branch 4 times, most recently from 27ed528 to a74d613 Compare March 6, 2021 19:34
@krizzu krizzu force-pushed the feat/android-room branch 2 times, most recently from 86114f9 to b388e87 Compare March 6, 2021 20:59
@krizzu krizzu force-pushed the feat/android-room branch from b388e87 to 70de3b0 Compare March 7, 2021 14:06
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

LGTM

@krizzu krizzu merged commit 75c571b into master Apr 2, 2021
@krizzu krizzu deleted the feat/android-room branch April 2, 2021 07:46
@krizzu
Copy link
Member Author

krizzu commented Apr 2, 2021

🎉 This PR is included in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@krizzu krizzu added the released label Apr 2, 2021
Ashoat added a commit to CommE2E/comm that referenced this pull request Apr 30, 2021
Summary:
Android wasn't persisting the Redux store when logged in with the "ashoat" user. I saw the "Row too big to fit into CursorWindow" error, so I Googled around and found this hack in a [GitHub issue](react-native-async-storage/async-storage#537).

The long-term fix is for us to move the database stuff to SQLite. For completeness, here are some other fixes I considered:

1. [react-native-mmkv](https://github.com/mrousavy/react-native-mmkv) as storage for `redux-persist` (not confident it will handle huge pages better though)
2. [Separating reducers](rt2zz/redux-persist#1265 (comment)) into distinct `AsyncStorage` keys (not sure what the distribution of size by keys is, and might require a migration)
3. Upgrading `AsyncStorage` and investing [this](https://react-native-async-storage.github.io/async-storage/docs/advanced/db_size) (people on the issue said it didn't help)
4. [Moving](react-native-async-storage/async-storage#528) off of SQLite storage onto Android Room storage (people on the issue said it didn't help)

I think using this hack for now is a good stopgap.

Test Plan: Make sure Android is loading the persisted store correctly when the app starts

Reviewers: palys-swm

Reviewed By: palys-swm

Subscribers: KatPo, Adrian, atul

Differential Revision: https://phabricator.ashoat.com/D1069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants