yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

persistWithOnConflict() returns true if the row already exists in the Table. #160

Closed AkshayRaj closed 8 years ago

AkshayRaj commented 8 years ago

According to the current persistWithOnConflict() method documentation, this method will return false if the current Model object is not inserted in the table. But it returns true.

The reason is that [insertWithOnConflict in SQLiteDatabase](http://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#insertWithOnConflict%28java.lang.String, java.lang.String, android.content.ContentValues, int%29) returns the rowId of the existing record and not -1 if the row contained in the Squidb model object already exists. Try the CONFLICT.IGNORE Conflict Algorithm.

sbosley commented 8 years ago

I'm not sure I quite understand what you're saying, and I think in fact you might find that it's the SQLiteDatabase documentation that is incorrect. We have a unit test that in fact tests this behavior: https://github.com/yahoo/squidb/blob/master/squidb-tests/src/com/yahoo/squidb/data/SquidDatabaseTest.java#L549. In all of our testing, we've found that when using the IGNORE conflict algorithm, -1 is returned for the rowId on conflict. You can see in this unit test (which does pass) that we call assertFalse on the result of a call to persistWithOnConflict using the IGNORE algorithm. Can you write a unit test that exhibits consistently different behavior on a wide variety of devices?

AkshayRaj commented 8 years ago

After CONFLICT_IGNORE, if any constraint violations occur, SQLiteDatabase returns the rowId of the record where the violation occurs. It does not return -1, according to the [official doc](http://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#insertWithOnConflict%28java.lang.String, java.lang.String, android.content.ContentValues, int%29). I used squidb in my project, and persistWithOnConflict returned true when CONFLICT_IGNORE was used.

I cannot point you to the actual test, as it is in a private repo.

sbosley commented 8 years ago

Yeah, I understand the official docs have said it does not return -1, but we've found that the official doc is incorrect. As you can see from the unit test I linked to, that method does return false as documented. We run our unit test suite on a wide variety of Android API levels/emulators and the behavior appears consistent across all of them.

If you can write a separate (non-private) test case using the squidb test suite that does exhibit the problem you're describing, that will help us track down whether or not this is an edge case that we somehow haven't considered or if it's an issue specific to the device/API level you're running on.

sbosley commented 8 years ago

I dug into this a bit more just to be sure, and after examining the Android source code I feel pretty confident that the Android SQLite classes do return -1 when a conflict occurs during an insert using the IGNORE conflict algorithm, even though this is not documented correctly in their official documentation. I'll outline my findings here.

When executing a SQL INSERT statement, the Android classes eventually call SQLiteConnection.executeForLastInsertedRowId, which in turn calls and returns the value from the native method nativeExecuteForLastInsertedRowId. The source code for this native method can be found here. The line that determines the return value is this:

return err == SQLITE_DONE && sqlite3_changes(connection->db) > 0
            ? sqlite3_last_insert_rowid(connection->db) : -1;

In the case of the IGNORE conflict algorithm, if a conflict had occurred attempting to execute the insert, the value returned by sqlite3_changes would be 0, since no changes would have occurred executing the most recent insert statement. That would make the overall boolean condition false, triggering a return of -1.

I don't doubt that at some point you saw persistWithOnConflict return true, but I think it may be more likely that there were simply some unexpected conditions in your DB access code that caused the call to perhaps do an update instead of an insert (which would be more likely to succeed and return true), or for some reason there wasn't actually any conflict in the DB to resolve. Since our unit tests are confirming that the method works as documented and the Android source code seems to back up the behavior our unit tests expect, I'm inclined to consider this issue resolved. However, if you feel like I missed something in my explanation or if you are able to create a unit test that can duplicate the problem, feel free to reply and we can try to figure out why you saw the unexpected behavior you did.

sbosley commented 8 years ago

Going to close this issue, but feel free to reopen if you have additional information to add or want to clarify the issue at all.