Fixed
Status Update
No update yet.
Comments
yb...@google.com
i'm still skeptical about changing the default to crash because I'm worried that some apps won't handle migrating from older versions properly.
But creating a builder option totally makes sense so that more avare developers can turn it on.
Btw, another nice thing about drop by default is the development time. during development, you can easily increment it and pass the check and then just provide the migration from latest release to current release when shipping the app.
But creating a builder option totally makes sense so that more avare developers can turn it on.
Btw, another nice thing about drop by default is the development time. during development, you can easily increment it and pass the check and then just provide the migration from latest release to current release when shipping the app.
mm...@commonsware.com
If some apps won't handle migrating and it is convenient for them to drop user data, it should not become a problem for those who respect user data. Respecting user data is more important that convenience for *some* lazy devs who can't write JUST ONE LINE to drop tables.
yb...@google.com
Dropping a user data is not always a bad thing, even for offline first apps.
Usually, local modifications are stored in a separate queue that is independent from the data in the db (mostly because data might be overridden).
So even if the data is nuked, local modifications can be restored. It is a fair compromise. In the common case scenario, the device probably had enough time to run any pending jobs before the app is updated so it is rarely a case where app is updated but it had local modifications. Even in that case, data is not lost since the modifications stay in a separate queue.
Worst case scenario of someone failing to write a migration and the app crash looping on the user's device is really bad. It is actually very easy to end up in that situation if the developer simply uninstalls the app to get around the migration because they were focused on solving another problem.
I think the best action for us is to add a "forceMigrations" method into the builder that will always enforce migrations. This way, more aware developers can turn it on (they are also more likely to read the docs).
Usually, local modifications are stored in a separate queue that is independent from the data in the db (mostly because data might be overridden).
So even if the data is nuked, local modifications can be restored. It is a fair compromise. In the common case scenario, the device probably had enough time to run any pending jobs before the app is updated so it is rarely a case where app is updated but it had local modifications. Even in that case, data is not lost since the modifications stay in a separate queue.
Worst case scenario of someone failing to write a migration and the app crash looping on the user's device is really bad. It is actually very easy to end up in that situation if the developer simply uninstalls the app to get around the migration because they were focused on solving another problem.
I think the best action for us is to add a "forceMigrations" method into the builder that will always enforce migrations. This way, more aware developers can turn it on (they are also more likely to read the docs).
yb...@google.com
mm...@commonsware.com
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.
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.
yb...@google.com
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.
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.
yb...@google.com
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.
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:
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.
Description
Version used: 1.0.0-beta1
Devices/Android versions reproduced on: n/a
This work got lost when that issue was marked as fixed.
We really need SupportSQLiteQuery to expose a getArgCount() (or the equivalent). In principle, we need the arguments themselves to be exposed, but so far we have been getting away without those.
Quoting myself from the above comment:
"The docs for rawQueryWithFactory() on SQLiteDatabase say the following for the selectionArgs parameter: "You may include ?s in where clause in the query, which will be replaced by the values from selectionArgs. The values will be bound as Strings."
"The FrameworkSQLiteDatabase implementation passes an empty String[] to rawQueryWithFactory()... then assumes that it can actually bind selection arguments later, despite saying (via the empty String[]) that there aren't any selection arguments.
"Perhaps that works with the native SQLiteDatabase, but it would appear that SQLCipher for Android assumes that the code calling rawQueryWithFactory() isn't lying about the selection arguments. Hence, we need those arguments (or perhaps just the count) before calling rawQueryWithFactory() on SQLCipher's SQLiteDatabase, but mBindArgs is private in SimpleSQLiteQuery, and in some cases, we are not the ones creating the SimpleSQLiteQuery instance (so we can't fork SimpleSQLiteQuery and provide our own implementation)."
So, for example, lacking a getArgCount(), we have to use reflection to try to get that value from some stock SupportSQLiteQuery implementations:
@Override
public Cursor query(final SupportSQLiteQuery supportQuery,
CancellationSignal signal) {
int count=0;
try {
if (supportQuery instanceof RoomSQLiteQuery) {
Field argCount = RoomSQLiteQuery.class.getDeclaredField("mArgCount");
argCount.setAccessible(true);
count = argCount.getInt(supportQuery);
}
else if (supportQuery instanceof SimpleSQLiteQuery) {
Field bindArgs = SimpleSQLiteQuery.class.getDeclaredField("mBindArgs");
bindArgs.setAccessible(true);
Object[] bindArgsValue = (Object[]) bindArgs.get(supportQuery);
count = bindArgsValue!=null?bindArgsValue.length:0;
}
else {
throw new IllegalArgumentException("Unexpected SupportSQLiteQuery type: "
+supportQuery.getClass().getCanonicalName());
}
}
catch (Exception e) {
throw new IllegalStateException("Um, ick", e);
}
String[] fakeArgs=new String[count];
for (int i=0;i<count;i++) {
fakeArgs[i]="";
}
return(safeDb.rawQueryWithFactory(
new net.sqlcipher.database.SQLiteDatabase.CursorFactory() {
@Override
public net.sqlcipher.Cursor newCursor(
net.sqlcipher.database.SQLiteDatabase db,
SQLiteCursorDriver masterQuery, String editTable,
SQLiteQuery query) {
supportQuery.bindTo(new Program(query));
return new SQLiteCursor(db, masterQuery, editTable, query);
}
}, supportQuery.getSql(), fakeArgs, null));
}
Adding getArgCount() would eliminate the reflection. We still wind up lying to SQLCipher for Android (and possibly other SQLite implementations) about the arguments, so it'll still be ugly, just less ugly.