IssueTracker

since these are in public API (:/) we need to do this in 1.2

Opening diff shortly

Project: platform/frameworks/support
Branch: androidx-master-dev

commit d576cbdc911cba16638a44fd8223391a90a07ef7
Author: Mike Nakhimovich <digitalbuddha@users.noreply.github.com>
Date:   Tue Aug 11 09:30:34 2020

    [GH] Hide deprecated internal API.
   
    ## Proposed Changes
   
    * `RoomDatabase.java` has protected `mCallbacks` field which is leaking in the API docs, we should @Hide it.
   
    ## Testing
   
    Test: Ran unit tests locally
   
    ## Issues Fixed
   
    Fixes: 76109329
   
    This is an imported pull request from https://github.com/androidx/androidx/pull/61.
   
    Resolves #61
    Github-Pr-Head-Sha: 6440daa3a63752c7f9d5ba2a390248cd85bc634f
    GitOrigin-RevId: fe92d8466a59b44b218b6ca3cbd57dcda17992f7
   
    Change-Id: Id599cdf5b02b32bdae0166266fb7da967598fe92

A       room/runtime/api/current.ignore
M       room/runtime/api/current.txt
M       room/runtime/api/public_plus_experimental_current.txt
M       room/runtime/api/restricted_current.txt
M       room/runtime/src/main/java/androidx/room/RoomDatabase.java

https://android-review.googlesource.com/1396827

There are two offline scenarios IMHO: one is a pure cache mode and you basically do not care about local data, but the other one is where a user generates some content offline. In that scenario it's essential that the data is never deleted. Assuming that all developers respect their user data it is still possible to make a mistake and having user data dropped can be an expensive mistake in that case.

What is the issue with having a default migration that will fix up the schema to have the correct tables + columns? E.g. not dropping tables or columns, just adding them and updating indices etc. That could still fail when the data does not match the constraints set, but at least it prevents data to be lost and sends a signal during development as opposed to silently dropping the whole schema.

Library deciding to *delete* something is a bad idea. At least by default.

How about the scenario when in versionCode=2 I forgot to add the migration, but oh god it rolled out overnight to 1000 users, so at morning I quickly create and upload a versionCode=3 to include the needed migration - only to find that the precious data has been deleted? hardly a good idea.

First of all, thanks a lot for all the feedback. This is why we label these as alpha and I'm glad it is working.

To be honest, I'm quite torn on this issue.

Let me first address the #6, the offline issue. You don't need to be persisting the user input in the database to make the app work offline. In fact, if you do that, you need to deal with synchronization between the local database and the job that uploads it to ensure that you always upload what is necessary etc.
An easier solution to that problem is to use a work queue for each user modification and de-couple it from the database. This also makes it very easy to architect the rest of the app (e.g. you can freely trim the database when it grows w/o caring about pending data).
That is actually what we did in my previous job and it worked very well. We initially had a queue on top of Square Tape, then wrote Android Priority JobQueue. Mind you it is 5-6 years old but here is something I wrote about it later on: http://www.birbit.com/a-recipe-for-writing-responsive-rest-clients-on-android. By the time, this solution was quite common among the very few offline ready apps.

About auto-migrations, it is something we wanted to do but not very trivial so we postponed to post 1.0. It is also likely to be a tool inside Studio to generate the migration instead of auto-magic migrations.

About #7, same scenario is as worse in the crash case where you'll get thousands of crash loops and uninstalls. It gets just worse if you didn't realize it next morning.

Auto-crashing is also annoying during development, though we can work around it by providing out of the box NukeMigration which will nuke the db and recreate.

Allowing a forceMigrations builder parameter seems like a proper sacrifice to me since it allows the more aware developer to put more strict rules for themselves while allowing the less aware developer to go with their flow. Also makes the common case easier.

Alternatively, we can default to crash but I feel like it will be a decision we'll regret as we receive more bugs that apps which use Room crash after update.

A third option might be to enforce force migration builder parameter, which avoids making a decision. It is an ugly API but will leave the decision to the developer in a more explicit way.

Please keep the comments coming, it is important to get this right for us and we really value your feedback.

Re #6 If you want to show the content in the app while it's not synced to the network, it makes sense to store it in the database and additionally keep some sync data. I've built a couple of apps where this was the case: the user adds some content and doesn't really care if it is synced, just that it is synced eventually. By adding it to the database and marking it as "pending" somehow the content can now show up with the rest of the content which in fact simplifies the architecture.

One example is a photo app, where you just want to keep adding your photos to albums and once you are on wifi, everything is synced up to the cloud. Meanwhile you can review and work with your albums without change.

I recently ran into an app doing something similar (adding to the db + sync marker + scheduling a job to sync) so it's not just me doing that :D
It probably boils down to what you are describing, but without the need to consolidate the work queue with the data from the db, because that data in the db is already optimistically updated. Without debating which one is better, I think it's a valid approach.

Nuking the DB for development isn't great either. That same app did not have a way to fetch the previously synced data (for reasons...) and nuking the data during development would therefore not be something that would be very convenient.

Re #7: Of course crashing isn't great either, but if I have the choice of a crashing app for which I can stop roll out and provide an update vs telling users that I trashed their data, I know what I'd pick.

I'd prefer an automatic migration and I think that automatic migration should be at runtime (what if you forget to generate your migration in AS?). For the apps that I wrote, and for users using my library, it seems to match the 90% case for migrations where new entities are added and no data migration is required. If that's not possible then I guess an explicit choice would be best, because ultimately the dev has to indicate someway that it's OK for Room to nuke user data as a convenience or not.

To clarify: I wrote "That same app", which is not true, I scrapped a second example to keep my reply shorter, but it was a different app :)

Yea, it is always a trade-offs case :). We did the same, just double write into the queue and the database.
As far as I can see, the majority is in favor of crashing in which case we would ahead and change the API to crash by default and add a builder parameter to nuke if migration is missing.

In the next version, Room will throw by default.
This behavior can be changed by calling fallbackToDestructiveMigration in the builder.

Thanks for all the feedback.