humazed / RoomAsset

A helper library to help using Room with existing pre-populated database [DEPRECATED].
Apache License 2.0
135 stars 23 forks source link

RoomAsset will only properly initialize one database? #9

Open VerumCH opened 6 years ago

VerumCH commented 6 years ago

I was having an issue with RoomAsset not migrating the contents of my pre-loaded database to the usable Room database, so I dug through the code and found what I believe to be the culprit:

/**
* Open the database and copy it to data folder using [SQLiteAssetHelper]
*/
private fun openDb(context: Context, name: String, storageDirectory: String?, factory: SQLiteDatabase.CursorFactory?) {
    val instantiated = "instantiated"
    val sharedPref = context.defaultSharedPreferences

    if (!sharedPref.getBoolean(instantiated, false)) {
        SQLiteAssetHelper(context, name, storageDirectory, factory, 1).writableDatabase.close()
        sharedPref.edit().putBoolean(instantiated, true).apply()
        Log.w(TAG, "RoomAsset is ready ")
    }
}

This method checks if there's a boolean value stored in SharedPreferences with the key "instantiated", and only migrates the contents if that value is false or does not yet exist. Now, my problem did not technically arise from trying to use multiple pre-populated databases, but rather from trying to overwrite the old one with one with a different name. However, I plan to have multiple in the future and can see the same problem arising - namely, that the database simply does not get filled in because RoomAsset thinks its job is already done.

I managed to work around it in my current situation by recording the name of my database myself in SharedPreferences, and resetting the "instantiated" key to false any time the name changes. However, it will become much more of a pain once I have multiple databases included, since I will have to keep resetting before initially migrating each one.

The problem stems from the quite non-descriptive and entirely non-unique key used for recording whether RoomAsset has initialized the database yet. Rather than simply "instantiated", it should be changed to something unique (both to RoomAsset, to prevent potential conflicts form other non-unique key values, and to the database, to allow multiple pre-populated databases without janky workarounds). For example, particularly since the name of the database is already passed to the method, this line:

val instantiated = "instantiated"

could be changed to something like:

val instantiated = "RoomAsset.instantiated." + name

This would be a super easy change, and if I had the time I'd just do a pull request for it but I don't for now.

humazed commented 6 years ago

Thank you, this seems a good solution, I will make the change you suggested, but I'm afraid that it won't be backward compatible.

VerumCH commented 6 years ago

Hmm. I see what your concern is - that applications using the current iteration will, upon updating to the newer version of RoomAsset, have their database re-initialized, potentially losing user data if the user is also allowed to add/save data to the pre-populated database.

However, I can't tell if that will actually be a problem? As far as I can see, once the database is initialized and handed over to Room, it continues to save to the same File location. Therefore, even if RoomAsset were to try and initialize it again, it would have all the saved data in it. If the app has had multiple versions of the database complete with migrations, that could be a problem, but I don't think RoomAsset would be involved at that point (at least, not if the creator is handling things properly).

The only concern I would have is - how do Android updates handle overwriting database files already on the device with database files packaged in the APK? From my anecdotal experience testing in the emulator on Android Studio, it seems to favor the files already on the device. Any time I change my database and want to check the changes in the emulator, I have to either change the name of the file or uninstall and reinstall the app, or else no changes are reflected. If this is always the case, which I am definitely not certain of, then it should be safe to push this change without worrying about backwards-compatibility, given the situation mentioned in the paragraph above.

If the above is not always the case, or if you simply think it's too risky to rely on, I can think of one decent "workaround" option - add a boolean flag to the constructor of the databaseBuilder to indicate whether multi-DB support should be enabled, with a default of false. If that value is false, leave the SharedPreferences key the same as it currently is. If that value is true, save/check the SharedPreferences key specific to the passed-in database name. I'm not super familiar with Kotlin, but I think it would look something like the following:

(constructor)

fun <T : RoomDatabase> databaseBuilder(
    context: Context,
    klass: Class<T>,
    name: String,
    multiDb: Boolean? = false,
    storageDirectory: String? = null,
    factory: SQLiteDatabase.CursorFactory? = null)
    : RoomDatabase.Builder<T> {

(openDb method call/signature)

private fun openDb(context: Context, name: String, multiDb: Boolean, storageDirectory: String?, factory: SQLiteDatabase.CursorFactory?)

openDb(context, name, multiDb, storageDirectory, factory)

(key variable initialization)

val instantiated = if (multiDb) ("RoomAsset.instantiated." + name) else "instantiated"

Then, existing users that have no problems with the current implementation can leave everything as-is with no worries even after updating their RoomAsset dependency, while those looking to do something like I am in my situation can use the multiDb option from the start.

VerumCH commented 6 years ago

@humazed Tagging just in case you missed my last response.

humazed commented 6 years ago

I'm really sorry, but I'm very busy these two weeks, having a big project with a tight deadline that is taking all my time.

I have seen your suggestion and it's very good, I will implement it and update the library as soon as I finish this project.