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

mmurphy@, do you have a sample implementation that i can check?
I'm considering creating a new API rather than mapping the framework so seeing an alternative implementation reference would be helpful.

Attached is a sample app demonstrating using Room with SQLCipher for Android.

It is quite possibly the worst Room implementation that you have seen. It's effectively a straight-up port of an old book example of mine that used SQLite directly. The key word there being "old". Also, the old book example used to pre-load the database with a bunch of values; I have not ported that over. When you run the app, click the + action bar item to get a dialog to add a numeric constant by name and value to the database and show it in the ListView.

It has an alternative implementation of the SupportSQLite* classes, separate from the Framework* ones, that integrates with the current release of SQLCipher for Android.

The sticky wickets:

- The aforementioned UnsupportedOperationExceptions that I throw, mostly for things that SQLCipher for Android does not have direct support for.

- Passphrase management probably needs work. The pattern with SQLCipher for Android is to collect the passphrase, use it immediately with getWritableDatabase(), then do what you can to destroy the passphrase (e.g., zero out the elements of the char[]). This being a modified book sample, the passphrase is hardcoded. My SupportSQLiteOpenHelper implementation for SQLCipher for Android caches the passphrase until the first call to getReadableDatabase() or getWritableDatabase(), then zeros out the passphrase. This means that a SupportSQLiteOpenHelper instance can be used to open a database, but not re-open the database after closing it. If Room needs the unfettered ability to open and close databases at will, then we have a problem. If, OTOH, Room will only close a database when the developer calls close() on the RoomDatabase, then we should be OK, probably with some tweaks to my passphrase management and nice loud warnings to developers about the effects of closing the RoomDatabase.

This is proof-of-concept-grade code at best. I just wanted to hack something together to see how well the SupportSQLite* classes and SQLCipher for Android worked together.

I'm missing copyright headers on some Java classes. With the power vested in me as me, I hereby declare that this code is licensed under the Apache Software License 2.0.

awesome, thanks!

BTW, I had commented out the two android.database.sqlite.* imports from my SupportSQLiteDatabase implementation (com.commonsware.android.saferoom.Database) as part of checking where those classes were used. You'll need to uncomment those imports to get that class to compile. Sorry about that!

Regarding at-will opening & closing of Database instances: A possible solution might be to reduce how much behavior is "guaranteed" by documentation, especially for SupportSQLiteOpenHelper.getWritableDatabase()? Caching of the Database instance until it is closed, after which for subsequent calls a new Database instance is created, is in my opinion a detail of the platform implementation.
In the SafeRoom-PoC, if there is a constraint that a Helper instance can only create exactly one Database instance which cached for subsequent getWritableDatabase() calls, then the Database class might implement close() as a no-op, instead providing a package-private reallyClose() method that is called by Helper's close() method.

OpenHelper actually keeps the database open until closed. After closed, if re-accessed, it re-opens the database.

For alpha 2, I've removed a bunch of methods (factory methods, million argument methods).
It is still WIP  but significantly reduced API (still depends on android).

Btw, this abstraction is not totally free and there might be some conflicts with the lazy loading project. Knowing that the returned Cursor is an SQLiteCursor may allows us to optimize the windowing (SQLiteCursor is an AbstractWindowedCursor). On the other hand, it is not great for alternative implementations.

tl;dr; these APIs may change (maybe we'll introduce a windowed cursor interface or implement a less efficient fallback).
Hopefully, alpha2 should be cleaner to implement for your custom implementation.

> OpenHelper actually keeps the database open until closed.

To clarify, do you mean "keeps the database open until explicitly closed via a call to close()"?

Thanks!

In alpha2, we've reduced the API surface and I've even changed some methods to be more correct (e.g. queries receive Object[] instead of String[] for bind arguments).
This is nowhere final but would be nice to get your feedback on recent changes. We are more liberal in changing these APIs since less developers are exposed to them.

I'd also tried to create bridge for sqlcipher. Faced problem with passing arguments to query. In Android Architecture Components samples BasicSample there is a call to Cursor query(SupportSQLiteQuery supportQuery) for room alpha2 when opening certain item from list. For sqlite it passes empty array of arguments to rawQueryWithFactory method. Tried to call same method for sqlcipher. Sqlciper's SQLiteQuery doesn't parse sql for arguments (or arguments count) and private field with argument values is zero-length, it sets that field from constructor's parameter only. I don't know which side assumptions are wrong - Room or SQLiteQuery, but for now to get it work I had to use reflection on both sides: get count of arguments from supportQuery and fill SQLiteQuery's private field with new array...

You can see those tries here https://github.com/ivanovsuper/android-architecture-components/blob/master/BasicSample/app/src/main/java/com/example/android/persistence/db/cipher/CipherSQLiteDatabase.java#L159

There is two commits for room alpha1 and alpha 2.

I can reproduce the query(SupportSQLiteQuery) problem from #14.

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).

BTW, other than this hiccup, the changes to the API (the original point behind this issue) certainly simplify matters and reduce how many things SQLCipher can't handle in its present incarnation.

I'm pretty sure this is wrong on our side because we lie to the framework sqlite.

We had to to this to workaround an android sqlite API bug where if you are trying to bind an int, converting it a string would confuse SQLite so query would not return what you expect.

Having a getter in SupportSQLite query will be kind of ugly but that might be the solution.

Maybe we can introduce a get parameter count?
In your code:
https://github.com/ivanovsuper/android-architecture-components/blob/master/BasicSample/app/src/main/java/com/example/android/persistence/db/cipher/CipherSQLiteDatabase.java#L159

You could've extracted the count when query is called and then call the `mDelegate.rawQueryWithFactory` with String[count] and then in the bind method, you would just call bind directly?

Btw, idk much about SQLiteDatabase but since it tries to replicate android APIs, maybe if we don't try to replicate, there might be a better solution?

It can't be just a plain String[], as SQLCipher actually binds the values in there (since that's really what the String[] is supposed to be).

Now, if I supply a String[] whose values are all set to "", things seem to work, insofar as I get the proper values back from queries, even where the real selection arguments are not all "". Apparently, SQLCipher binds once with the String[], and then we re-bind ourselves in the CursorFactory (modeled after the FrameworkSQLiteDatabase code).

This seems somewhat scary.

FWIW, attached is an updated version of my SQLCipher for Android/Room proof-of-concept, updated for alpha2, using ivanovsuper's reflection hack modified as I mention above.

Sorry to jump in, but it looks like that while reducing the API surface on alpha2, FrameworkSQLiteDatabase loose the capacity to bind Boolean arguments. SimpleSQLiteQuery.bind(SupportSQLiteProgram statement, int index, Object arg) should probably support Boolean arg.

Let me know if I should open an other issue.

The workaround with passing array with empty strings may work or may not work depending on implementation. For instance it can check for int values for int arguments samehow and throw for empty string like it throws on array with nulls. With current version of sqlcipher it works ok as in #17 code, but this behavior can be changed in future versions...

I think right way is ability to get args array (String[] or Object[]?) with actual bindings not only count. And to avoid sqlite bug with "int as string" rebind it inside factory like now as needed.

The API is already Object[] so if sqlchipher can do it, it should work fine.
But SupportSQLiteQuery does not have a getArgs : Object[] method, maybe that is what i need to add to support your case.

for the default framework implementation, using the factory works so we prefer to use bind method instead.

Sorry if partially off-topic, I can create another issue if needed, but should SupportSQLiteProgram extend Closeable? Statements need to be closed in all of the database implementations I've used. In the Android SDK case, SQLiteProgram extends SQLiteClosable which implements Closeable.

Create a separate issue please.

Created https://issuetracker.google.com/issues/63131997