openfoodfacts / openfoodfacts-androidapp

(Legacy) Native version of Open Food Facts on Android - Coders & Decoders welcome 🤳🥫
https://android.openfoodfacts.org
Apache License 2.0
774 stars 447 forks source link

RFC: Migrate from greenDAO to ObjectBox #3756

Open VaiTon opened 3 years ago

VaiTon commented 3 years ago

As a follow up to this announcement, I'd like to migrate our DB system to ObjectBox.

The advantages are multiple and they suit our scope perfectly. From the migration page:

  • Much faster: ObjectBox is up to 10 times faster than SQLite (check this open source benchmark app)
  • Strong support for relations: ObjectBox offers change tracking, cascading puts, and flexible loading strategies (eager and lazy)
  • No need to master SQL: ObjectBox is easier to use without the need to learn a “foreign” language
  • Modern APIs: ObjectBox has simplified APIs and comes with its own reactive queries and support for RxJava 2/3 (with real change observing)
  • Cleaner entity code: ObjectBox has invisible code generation and does not generate sources into your files
  • Kotlin support: ObjectBox supports Kotlin including data classes
  • Sync coming soon: Keeping data in sync is hard to get right based on typical REST networking approaches. ObjectBox Sync makes that simple.

I'd like to emphatize the point "Kotlin support", "Strong support for relations" and "Modern Api (RxJava)".

Also: https://docs.objectbox.io/faq#on-which-platforms-does-objectbox-run

Let me know what you think. @teolemon @stephanegigandet @philippeauriach

kartikaysharma01 commented 3 years ago

I would like to implement this.

VaiTon commented 3 years ago

I would like to implement this.

Thank you for your interest, although we still need to see if the other members agree or not on this one. I'd like to wait expecially for @stephanegigandet, @philippeauriach and all @openfoodfacts/openfoodfacts-server about this change.

philippeauriach commented 3 years ago

From what I read, we could test it as it seems to have a backward compatible api by replacing just an import ? This way we could make a first pass to check if things work well.

What about the state of the database, does it get migrated ? Or this new database would "start fresh", and therefore what data would be lost for the user?

Also, we should double check that we are compatible with all supported android versions.

VaiTon commented 3 years ago

From what I read, we could test it as it seems to have a backward compatible api by replacing just an import ? This way we could make a first pass to check if things work well.

Sure we could, we could start using the new api working with the old database.

What about the state of the database, does it get migrated ? Or this new database would "start fresh", and therefore what data would be lost for the user?

From what I understood, the database can be migrated via migration task the first time the DB is initialized. See https://greenrobot.org/greendao/documentation/objectbox-compat/

Also, we should double check that we are compatible with all supported android versions.

Already checked. https://docs.objectbox.io/faq#on-which-platforms-does-objectbox-run

naivekook commented 3 years ago

ObjectBox looks promising! I like the NoSql approach, and looks like we don't need features from the relation database (such as table joining or heavy filtering).

One thing we should consider, that ObjectBox has a trick to achieve such performance. Under the hood, it has 2 databases - in-memory and disk one. So when you save the entity to ObjectBox it puts the object to memory cache and returns back the successful event. After that ObjectBox syncs the in-memory version with the disc version. so potentially we have a chance to miss the data And also, ObjectBox doesn't support coroutines from the box. But looks like creating a coroutine wrapper no such a big deal.

@VaiTon What if, with database migrating, we also make the database a separate module? It will be a good step forward to clean architecture. WDYT?

Anyway, If you need help with this task, I'd like to work on it!

VaiTon commented 3 years ago

so potentially we have a chance to miss the data

Indeed there is, although we use the db to sync data with the server and keep offline data available for later use/upload. We actually do not use it a lot for atomicity of operations etc...

And also, ObjectBox doesn't support coroutines from the box. But looks like creating a coroutine wrapper no such a big deal.

There is a standard implementation to create a flow from a query: https://github.com/objectbox/objectbox-java/pull/990

@VaiTon What if, with database migrating, we also make the database a separate module? It will be a good step forward to clean architecture. WDYT?

What would the module look like?

naivekook commented 3 years ago

What would the module look like?

just pure kotlin module with database and database entities only

I remember that at least half a year ago some database entities were used not just for storing purposes, but also in business logic and for API calls. For me, it is a quite dirty approach that is hard to test and hard to fix issues because of high coupling. I would like to see some separation between database, API and business logic, because sometimes we have a mess with 1k lines of code.

Changing the database requires refactoring so maybe we can consider also some refactoring in architecture?