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

Bug / Integer primary key conflict should upsert #60

Merged
merged 9 commits into from Jun 29, 2016

Conversation

jonreeve
Copy link
Contributor

Problem

InsertHelper claims it will perform an "upsert" - that is, update if present, insert if not - but this only works for tables with explicitly defined uniqueness constraints that will result in explicit indexes.

There is a common pattern of usage with SQLite to use a synthetic primary key column named _id, and for that column to be declared as an INTEGER PRIMARY KEY. As per the SQLite docs:

If a table contains a column of type INTEGER PRIMARY KEY, then that column becomes an alias for the ROWID

When this happens, there will not be an explicit unique index created for this column. None is needed; it is an alias for the ROWID, which is inherently unique and faster than any index. In the case that this is the only thing constrained to be unique in a table, the library fails to detect this constraint and thus does not switch to an update. It instead attempts an insert, which fails.

Solution

Functionality has been added to DbUtils to detect an INTEGER PRIMARY KEY on a table, and if found, to add this to the list of constraints it will find for a table. With the implicit constraint made apparent, the library performs correctly.

Tests

  • A test has been added for DbUtils to verify that it will now include a constraint of this type if present.
  • The one existing test for InsertHelper that was testing two things has been split into two, and some efforts made to increase readability. It's rather tricky to verify things with a cursor in a readable way, without some other testing library, but I've settled on something that I think works and keeps it quite simple (i.e. not so complicated the tests would need tests!). This especially benefits from IDE collapsing of Java boilerplate, though I don't think it's too bad without.
  • One more test has then been added for InsertHelper to verify this specific case of "upsert". The unfortunate widespread usage of static functions makes decoupling tests on InsertHelper from also relying on DbUtils impossible, and the goal here isn't to restructure the architecture of the library, so this is left as is for now.

Need to put a function on here to find this implicit constraint, then feed that back to the InsertHelper.
Due to all the static functions I can't easily decouple and make that one pass before this one...
…ry key.

Implementation copied across from the workaround I put on CCleaner and hope to soon remove.
@stefanhoth
Copy link
Contributor

Can one of the admins review this PR?

final Cursor pragmas = db.rawQuery(String.format(PRGAMA_INDEX_LIST, table), null);
while (pragmas.moveToNext()) {
int isUnique = pragmas.getInt(2);
final Cursor indices = queryIndexListForTable(table, db);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't want to fix it in the deprecated version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's deprecated... so, no? If someone updated the lib to get the fix, they could switch to using the non-deprecated version.

@blundell
Copy link
Contributor

💯 what about adding a demo of an upsert? Another PR to split the noise probably

@friedger
Copy link
Contributor

:shipit:

@@ -1,2 +1,4 @@
CREATE TABLE 'parents' (_id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT UNIQUE, description TEXT, latitude REAL, longitude REAL, createdAt INTEGER);
CREATE TABLE 'childs' (_id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, parent_id INTEGER NOT NULL, FOREIGN KEY(parent_id) REFERENCES parent(_id));
-- This table intentionally has no Foreign Keys, no UNIQUE fields, no indexes, apart from the implicit index from the INTEGER PRIMARY KEY.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jonreeve
Copy link
Contributor Author

jonreeve commented Jun 29, 2016

@blundell the demos seem kind of like low-grade tests - i.e. it just runs a bunch of stuff in onCreate while pointing out that no-one should do that, so I figured well expressed tests would demonstrate it better.

Ah, admittedly I think it was demo-simple I looked at. Perhaps the other is better. I'd honestly ditch demo-simple in favour of nicely self-documenting tests.

I might look at adding a demo to the demo-extended thing, if I can see a meaningful way to show it. But yeah, as you said, another PR...

List<String> tables = getTables(db);
List<String> foreignTables = new ArrayList<String>(5);
String name;
String tableName;
while (cur.moveToNext()) {
name = cur.getString(cur.getColumnIndexOrThrow("name"));
name = cur.getString(cur.getColumnIndexOrThrow(COLUMN_NAME));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@frapontillo
Copy link
Contributor

Looks legit 👍

@frapontillo
Copy link
Contributor

2 👍 = :shipit: in my book

@frapontillo frapontillo merged commit 2474157 into master Jun 29, 2016
@frapontillo frapontillo deleted the bug/integer_primary_key_conflict_should_upsert branch June 29, 2016 15:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants