Status Update
Comments
ya...@gmail.com <ya...@gmail.com> #2
yb...@google.com <yb...@google.com> #3
Thanks. Let us know when you have some API design (not looking for a doc, just commenting here is also fine, whichever is easier for you).
One tricky bit about this one is the "transactionality" of it.
e.g. we need to handle the case where we called onPopuplated but application crashed before onPopulated
could succeed. We need to re-run it in the next run.
We might do this by keeping a record in room_master_table
(there might be a better option, this is the first thing that comes to my mind)
ya...@gmail.com <ya...@gmail.com> #4
My idea is to:
1. Add RoomDatabase.Callback#onPopulated method
2. SQLiteCopyOpenHelper already has access to a list of callbacks (List<RoomDatabase.Callback>) through mDatabaseConfiguration.callbacks
3. Modify getWritableDatabase() and getReadableDatabase() so that after the file is copied I can iterate over those callbacks and call onPopulated. I would pass mDelegate.getWritableDatabase() and mDelegate.getReadableDatabase() as a parameter to onPopulated.
4. Add tests in DatabaseCallbackTest
I asked Danny about it and he said that I might need to temporarily open the DB with a vanilla open helper just to invoke onPopulated close it and then let Room re-open it through the normal path. Not sure how would I do that yet so I'm still reading and trying to get better understanding of the code.
This probably also doesn't take this "transactionality" thing into account so I need to look into it. I think I'll try to create some tests that will help me verify if it works or not.
I'm also wondering if there is a way to build the lib locally so I can test it on a sample project. Can I somehow publish it to local maven or something?
yb...@gmail.com <yb...@gmail.com> #5
For transactionality, we do actually have a table called `room_master_table` that we can use. I made a mistake on the design of that table by not having proper keys :facepalm: instead it only has 1 id column. It is possible to create a migration for it within room but for this task, we can just add another magic number (we use 42 for identity hash) to check if we did successfully call onPopuplated (and also, we need to call on populated as we propbably don't want to call it if it is not created from assets).
For testing, TestApp is probably sufficient.
If you want to build a maven repo, you can execute:
```
cd room && DIST_DIR=$(pwd)/../repo ./gradlew createDiffArchiveForAndroidxRoom
```
This is a special task that checks the versions in
ya...@gmail.com <ya...@gmail.com> #6
When it comes to naming, I was considering onPopulatedFromAssets but there is also createFromFile method in Room database builder so I think we should use something more generic. What do you think?
yb...@google.com <yb...@google.com> #7
yea makes sense. Lets go with your first suggestion for now, maybe Dany will have a better idea.
da...@google.com <da...@google.com> #8
I think onPopulated
is a good starting point. Maybe a more specific name could be onOpenPrepackagedDatabase
, we mention 'pre-package databases' a few in the documentation so maybe we can use that same term.
Meanwhile, let me clarify what I meant by 'opening the DB with a vanilla helper'. In SQLiteCopyOpenHelper
once delegate.getWritableDatabase()
is invoked it will actually open the DB and go through all the onCreate
, migrations and onOpen
callbacks which means if you add logic to call onPopulated
it would have already been too late. My feeling is onPopulated
should be called around the same time as onCreate
, before migrations and before onOpen
so that users get a chance to modify the pre-package DB, possibly making schemas changes to comply with Room's validation. Therefore, I think we need to temporarily open the DB just to pass it so onPopulated
, we can do this with a new FrameworkSQLiteOpenHelper
, then close the DB and let the delegate open it, which would go through the rest of Room's flow. Hope this makes sense!
yb...@google.com <yb...@google.com> #9
do you mean doing it in the copying helper? Maybe even we can let createFromAsset
builder methods to receive a callback which includes onPopulated?
da...@google.com <da...@google.com> #10
Yeah, doing it on copying helper, it's the most accurate place where we know a pre-packaged DB was used / copied. The callback itself can also be receive in createFromAsset
/ createFromFile
, or just a new callback in RoomDatabase.Callback
, I still think the tricky part is giving a DB in the callback itself (onPopulated(SuppoertSqliteDatabase db)
) which requires opening the DB in the middle of the 'opening sequence'.
yb...@google.com <yb...@google.com> #11
thats why i thought about scoping it to createFromFile(file, callback)
so that it would be responsibility of the createFromFile
to open the database and call the callback and then close it.
now it makes me wonder if that callback needs to be full fledged open helper callback instead of a special on populated? What if we just let them pass the regular callback there and if that callback exists, they can do whatever.
Another problem is though, we don't know the version so how are we going to open it in the first place :/.
da...@google.com <da...@google.com> #12
Seems reasonable to scope it and not be a full fledge callback. However, I still think it needs to be lazy called (when the DB is opened for the first time, usually on the first query done) and not when building the DB (via the builder). The callback is really only useful is it receives the DB as a param so the user gets a chance to do something, so still think we need to open the DB and close it.
By the way, we can read the DB version without opening the DB:
ya...@gmail.com <ya...@gmail.com> #13
But that means that we would be adding 2 new methods to the builder - createFromAsset/createFromFile but with additional parameter, right? So that we don't break backwards compatibility. I'll play with this ideas tomorrow.
But what about "transactionality"? Does putting this in copying helper make any difference? Do you think we still should use Room master table to detect if this new onPopulated callback should be called or not?
yb...@google.com <yb...@google.com> #14
ugh thats a good point.
Danny knows better but my first instinct is to let SQLiteCopyOpenHelper
take care of calling it.
It also needs to know that it is expected to open the database :/ so needs to keep track of that information as well not to open the database when it doesn't need to.
we might possibly go a bit loose on transactionality and just keep an additional file next to the database to keep this information.
the downside is that we'll be creating more junk that we can never delete.
ap...@google.com <ap...@google.com> #15
Branch: androidx-master-dev
commit 52c2e0b99eb8e3638a7fb8645772a529d42653f8
Author: mzgreen <yairobbe@gmail.com>
Date: Fri Aug 07 20:22:02 2020
[GH] Add onOpenPrepackagedDatabase callback
This patch adds a new builder methods that accept new
PrepackagedCallback for creating a database. The new PrepackagedCallback
contains a single method onOpenPrepackagedDatabase(SupportSQLiteDatabase)
which is called right after creating a db from Asset/File/InputStream.
onOpenPrepackagedDatabase(SupportSQLiteDatabase) callback is transactional
which means that it'll be called only once after creating a db from
Asset/File/InputStream. If an exception is thrown inside onOpenPrepackagedDatabase
callback then the next time this database is opened, it should be created from
Asset/File/InputStream again and onOpenPrepackagedDatabase should also be called
again.
Relnote: "Add a onOpenPrepackagedDatabase callback for when a prepackaged
DB is copied."
Test: Added a unit tests to PrepackageTest.
Fixes:
This is an imported pull request from
Resolves #55
Github-Pr-Head-Sha: 330c068dda27ff3ee3edc5c9166a1f3218e6acf8
GitOrigin-RevId: 1771ccf4458fdd2c9318ba413e4a1576da3c0024
Change-Id: I1ba740f679565f5c97548f6683de40b0f40f9024
M room/integration-tests/testapp/src/androidTest/java/androidx/room/integration/testapp/test/PrepackageTest.java
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/DatabaseConfiguration.java
M room/runtime/src/main/java/androidx/room/RoomDatabase.java
M room/runtime/src/main/java/androidx/room/SQLiteCopyOpenHelper.java
M room/testing/src/main/java/androidx/room/testing/MigrationTestHelper.java
ap...@google.com <ap...@google.com> #16
Branch: androidx-master-dev
commit 52c2e0b99eb8e3638a7fb8645772a529d42653f8
Author: mzgreen <yairobbe@gmail.com>
Date: Fri Aug 07 20:22:02 2020
[GH] Add onOpenPrepackagedDatabase callback
This patch adds a new builder methods that accept new
PrepackagedCallback for creating a database. The new PrepackagedCallback
contains a single method onOpenPrepackagedDatabase(SupportSQLiteDatabase)
which is called right after creating a db from Asset/File/InputStream.
onOpenPrepackagedDatabase(SupportSQLiteDatabase) callback is transactional
which means that it'll be called only once after creating a db from
Asset/File/InputStream. If an exception is thrown inside onOpenPrepackagedDatabase
callback then the next time this database is opened, it should be created from
Asset/File/InputStream again and onOpenPrepackagedDatabase should also be called
again.
Relnote: "Add a onOpenPrepackagedDatabase callback for when a prepackaged
DB is copied."
Test: Added a unit tests to PrepackageTest.
Fixes:
This is an imported pull request from
Resolves #55
Github-Pr-Head-Sha: 330c068dda27ff3ee3edc5c9166a1f3218e6acf8
GitOrigin-RevId: 1771ccf4458fdd2c9318ba413e4a1576da3c0024
Change-Id: I1ba740f679565f5c97548f6683de40b0f40f9024
M room/runtime/api/current.txt
M room/integration-tests/testapp/src/androidTest/java/androidx/room/integration/testapp/test/PrepackageTest.java
M room/runtime/api/public_plus_experimental_current.txt
M room/runtime/src/main/java/androidx/room/SQLiteCopyOpenHelper.java
M room/runtime/src/main/java/androidx/room/DatabaseConfiguration.java
M room/runtime/api/restricted_current.txt
M room/runtime/src/main/java/androidx/room/RoomDatabase.java
M room/testing/src/main/java/androidx/room/testing/MigrationTestHelper.java
Description
Version used: 2.2.3
Thanks a lot for long-awaited `createFromAssets\createFromFile` feature!
But since the copied database is not considered a new database, need the possibility do business logic when database actually copied from assets by subscription to special callback `RoomDatabase.Callback#onPopulated`, for instance.
Now, to workaround this we need to do a lot of things.