Fixed
Status Update
No update yet.
Comments
yb...@google.com
ugh seems like i made a mistake and added it to RoomSQLiteQuery :/.
i'll pull it to the base class, sorry.
i'll pull it to the base class, sorry.
yb...@google.com
ok i take it back a bit, well i mean looks like it was not a mistake.
Given that there is getSql and bindArgs; getArgCount seems very one off API.
Btw, instead of reflection, you can use a logging bind args object to take out the parameters.
I'm not saying we should not do this, just not super convinced that this is the right API.
Given that there is getSql and bindArgs; getArgCount seems very one off API.
Btw, instead of reflection, you can use a logging bind args object to take out the parameters.
I'm not saying we should not do this, just not super convinced that this is the right API.
mm...@commonsware.com
> Given that there is getSql and bindArgs
Here is the 1.0.0-beta1 implementation of SupportSQLiteQuery:
/*
* Copyright (C) 2017 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
*http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package android.arch.persistence.db;
/**
* A query with typed bindings. It is better to use this API instead of
* {@link android.database.sqlite.SQLiteDatabase#rawQuery(String, String[])} because it allows
* binding type safe parameters.
*/
public interface SupportSQLiteQuery {
/**
* The SQL query. This query can have placeholders(?) for bind arguments.
*
* @return The SQL query to compile
*/
String getSql();
/**
* Callback to bind the query parameters to the compiled statement.
*
* @param statement The compiled statement
*/
void bindTo(SupportSQLiteProgram statement);
}
(I would have linked to it, but AFAIK the code is not published on the Web just yet)
So... what is the bindArgs that you are referring to?
> Btw, instead of reflection, you can use a logging bind args object to take out the parameters.
Sorry, but I do not understand what you mean here.
Here is the 1.0.0-beta1 implementation of SupportSQLiteQuery:
/*
* Copyright (C) 2017 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
*
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package android.arch.persistence.db;
/**
* A query with typed bindings. It is better to use this API instead of
* {@link android.database.sqlite.SQLiteDatabase#rawQuery(String, String[])} because it allows
* binding type safe parameters.
*/
public interface SupportSQLiteQuery {
/**
* The SQL query. This query can have placeholders(?) for bind arguments.
*
* @return The SQL query to compile
*/
String getSql();
/**
* Callback to bind the query parameters to the compiled statement.
*
* @param statement The compiled statement
*/
void bindTo(SupportSQLiteProgram statement);
}
(I would have linked to it, but AFAIK the code is not published on the Web just yet)
So... what is the bindArgs that you are referring to?
> Btw, instead of reflection, you can use a logging bind args object to take out the parameters.
Sorry, but I do not understand what you mean here.
yb...@google.com
bindArgs -> bindTo
here is my reflection-free recommendation.
/**
* an sqlite program implementation that logs the bind args
*/
class MySupportSQLiteProgram implements SupportSQLiteProgram {
void bindLong(int index, long value) {
args.put(index, value);
}
}
then in your query method, you can call
MySupportSQLiteProgram fakeProgram = new MySupportSQLiteProgram();
supportSQLiteQuery.bindTo(fakeProgram);
// now you have all args in the fakePrgram that you can pass down to the cypher.
here is my reflection-free recommendation.
/**
* an sqlite program implementation that logs the bind args
*/
class MySupportSQLiteProgram implements SupportSQLiteProgram {
void bindLong(int index, long value) {
args.put(index, value);
}
}
then in your query method, you can call
MySupportSQLiteProgram fakeProgram = new MySupportSQLiteProgram();
supportSQLiteQuery.bindTo(fakeProgram);
// now you have all args in the fakePrgram that you can pass down to the cypher.
yb...@google.com
mm...@commonsware.com
I would argue that the correct status is "Intended Behavior" or some other won't-fix variant. "Fixed" implies that SupportSQLiteQuery has getArgCount(), and it doesn't, at least according to the JavaDocs: https://developer.android.com/reference/android/arch/persistence/db/SupportSQLiteQuery.html
yb...@google.com
actually i have a pending CL that adds getArgCount method. Not merged yet but if all go well, should be out in the next room alpha.
yb...@google.com
will be fix in 1.1 alpha3.
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.