Skip to content
This repository was archived by the owner on Feb 11, 2022. It is now read-only.

Fix update behaviour in content provider implementation #62

Merged
joetimmins merged 5 commits into from
Jul 1, 2016
Merged

Fix update behaviour in content provider implementation #62

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
360e1ec
Add test to showcase broken insert behaviour
Jun 30, 2016
e1d5273
Fix incorrect behavior of update in content provider implementation
Jun 30, 2016
42db835
Remove redundant modifier in interface declaration
Jun 30, 2016
93f6626
Rename column name
Jul 1, 2016
344a624
Remove unnecessary call to super.tearDown()
Jul 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions core/src/androidTest/java/novoda/lib/sqliteprovider/ContentProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@

package novoda.lib.sqliteprovider;

import android.content.ContentValues;
import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.net.Uri;
import android.test.AndroidTestCase;

import java.io.IOException;

import novoda.lib.sqliteprovider.sqlite.ExtendedSQLiteOpenHelper;

public class ContentProviderTest extends AndroidTestCase {

private static final String TABLE_NAME = "test";
private static final String COLUMN_PRIMARY_KEY = "column_primary_key";
private static final String COLUMN2 = "column2";
Copy link
Contributor

@takecare takecare Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a better name or is there a specific reason for this one?

Copy link

@lgvalle lgvalle Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason for this one but we couldn't find a better name because the data there is really not relevant for the test.
Suggestions welcome!
A_COLUMN
ANOTHER_COLUMN
A_COLUMN_HAS_NO_NAME
?

Copy link
Contributor

@takecare takecare Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if it's not relevant you can name it ANY_COLUMN or ANY_COLUMN_NAME

Copy link
Contributor

@joetimmins joetimmins Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for A_COLUMN_HAS_NO_NAME 😂

Copy link
Contributor

@frapontillo frapontillo Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't I think of that before? 👍 for A_COLUMN_HAS_NO_NAME

Copy link
Contributor Author

@eduardb eduardb Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 93f6626. The column has a name 😛


public ContentProviderTest() {
super();
}
Expand All @@ -18,4 +28,46 @@ public void testMapping() throws Exception {
assertTrue(cursor.getColumnIndexOrThrow("childs__id") > -1);
assertTrue(cursor.getColumnIndexOrThrow("parents_name") > -1);
}

public void test_GivenATableWithData_When_UpdatingNonexistentRow_Then_NumberOfAffectedRowsShouldBeZero() throws Exception {
Copy link
Contributor

@blundell blundell Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

givenATableWithData();

ContentValues values = new ContentValues(1);
values.put(COLUMN2, 1);

int numRows = new ExtendedSQLiteOpenHelper(getContext())
.getWritableDatabase()
.update(TABLE_NAME, values, COLUMN_PRIMARY_KEY + "=?", new String[]{"2"});

assertEquals(0, numRows);

numRows = getContext().getContentResolver().update(
Uri.parse("content://novoda.lib.sqliteprovider.test/" + TABLE_NAME),
values,
COLUMN_PRIMARY_KEY + "=?",
new String[]{"2"});

assertEquals(0, numRows);
}

private void givenATableWithData() throws IOException {
ExtendedSQLiteOpenHelper helper = new ExtendedSQLiteOpenHelper(getContext());
SQLiteDatabase db = helper.getWritableDatabase();
db.execSQL("CREATE TABLE IF NOT EXISTS `" + TABLE_NAME + "` (" +
"`" + COLUMN_PRIMARY_KEY + "` INTEGER PRIMARY KEY," +
"`" + COLUMN2 + "` INTEGER UNIQUE)");

ContentValues values = new ContentValues(2);
values.put(COLUMN_PRIMARY_KEY, 1);
values.put(COLUMN2, 1);
helper.getWritableDatabase().insert(TABLE_NAME, null, values);
}

@Override
protected void tearDown() throws Exception {
super.tearDown();
Copy link
Contributor

@blundell blundell Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpic: you don't actually have to call this, but what I was originally going to say was LIFO call this last line of hte method :)

Copy link
Contributor Author

@eduardb eduardb Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 344a624

ExtendedSQLiteOpenHelper helper = new ExtendedSQLiteOpenHelper(getContext());
SQLiteDatabase db = helper.getWritableDatabase();
db.execSQL("DROP TABLE IF EXISTS `" + TABLE_NAME + "`");
}
}
2 changes: 1 addition & 1 deletion core/src/androidTest/java/novoda/lib/sqliteprovider/sqlite/InsertHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ private void onQueryOf(String tableName, CursorOperations cursorOperations) {
}
}

private static interface CursorOperations {
private interface CursorOperations {
Copy link
Contributor

@takecare takecare Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

void doOperationsOn(Cursor cursor);
}
}
11 changes: 4 additions & 7 deletions core/src/main/java/novoda/lib/sqliteprovider/provider/SQLiteContentProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import android.content.Context;
import android.database.Cursor;
import android.database.DatabaseUtils;
import android.database.SQLException;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteOpenHelper;
import android.net.Uri;
Expand Down Expand Up @@ -96,14 +95,12 @@ private Uri insertSilently(Uri uri, ContentValues values) {
protected int updateInTransaction(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
ContentValues insertValues = (values != null) ? new ContentValues(values) : new ContentValues();
Copy link
Contributor

@takecare takecare Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know this was already here but afaik we don't need to do this and can actually pass the null to update()

Copy link
Contributor Author

@eduardb eduardb Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following exception is thrown if I let null in there:

java.lang.IllegalArgumentException: Empty values
at android.database.sqlite.SQLiteDatabase.updateWithOnConflict(SQLiteDatabase.java:1542)
at android.database.sqlite.SQLiteDatabase.update(SQLiteDatabase.java:1522)
at novoda.lib.sqliteprovider.provider.SQLiteContentProviderImpl.updateInTransaction(SQLiteContentProviderImpl.java:98)

Copy link
Contributor

@takecare takecare Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 weird. the docs state that null is a possible value but...
screen shot 2016-07-01 at 10 15 41
oh well...

Copy link
Contributor Author

@eduardb eduardb Jul 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that the null guard is useless, as using new ContentValues() will also throw.
But maybe this was changed in a recent version of the OS, and the documentation is outdated.

Copy link
Contributor Author

@eduardb eduardb Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that by "null is a possible value" they mean that a null value in the map is possible, not that the values itself can be null.

Copy link
Contributor

@takecare takecare Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, maybe it's that 👍 weird choice of words, still.


int rowId = getWritableDatabase().update(UriUtils.getItemDirID(uri), insertValues, selection, selectionArgs);
int rowsAffected = getWritableDatabase().update(UriUtils.getItemDirID(uri), insertValues, selection, selectionArgs);

if (rowId > 0) {
Uri insertUri = ContentUris.withAppendedId(uri, rowId);
notifyUriChange(insertUri);
return rowId;
if (rowsAffected > 0) {
notifyUriChange(uri);
}
throw new SQLException("Failed to update row into " + uri + " because it does not exists.");
return rowsAffected;
}

@Override
Expand Down