From a16242b9f82023749b61a0306e2620720b6294c3 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 5 Dec 2019 11:41:47 -0500 Subject: [PATCH] Fix issues with RecipientDatabase#update(). --- .../securesms/database/RecipientDatabase.java | 29 ++---- .../thoughtcrime/securesms/util/SqlUtil.java | 44 +++++++++ .../securesms/database/SqliteUtilTest.java | 95 +++++++++++++++++++ 3 files changed, 147 insertions(+), 21 deletions(-) create mode 100644 test/unitTest/java/org/thoughtcrime/securesms/database/SqliteUtilTest.java diff --git a/src/org/thoughtcrime/securesms/database/RecipientDatabase.java b/src/org/thoughtcrime/securesms/database/RecipientDatabase.java index 2791d6230c..eed2535793 100644 --- a/src/org/thoughtcrime/securesms/database/RecipientDatabase.java +++ b/src/org/thoughtcrime/securesms/database/RecipientDatabase.java @@ -8,6 +8,7 @@ import android.text.TextUtils; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.annimon.stream.Stream; import com.google.android.gms.common.util.ArrayUtils; @@ -26,9 +27,11 @@ import org.thoughtcrime.securesms.util.Base64; import org.thoughtcrime.securesms.util.FeatureFlags; import org.thoughtcrime.securesms.util.GroupUtil; import org.thoughtcrime.securesms.util.IdentityUtil; +import org.thoughtcrime.securesms.util.SqlUtil; import org.thoughtcrime.securesms.util.Util; import org.whispersystems.libsignal.IdentityKey; import org.whispersystems.libsignal.InvalidKeyException; +import org.whispersystems.libsignal.util.Pair; import org.whispersystems.libsignal.util.guava.Optional; import org.whispersystems.signalservice.api.push.SignalServiceAddress; import org.whispersystems.signalservice.api.util.UuidUtil; @@ -1164,29 +1167,13 @@ public class RecipientDatabase extends Database { * query such that this will only return true if a row was *actually* updated. */ private boolean update(@NonNull RecipientId id, ContentValues contentValues) { - SQLiteDatabase database = databaseHelper.getWritableDatabase(); - StringBuilder qualifier = new StringBuilder(); - Set> valueSet = contentValues.valueSet(); - String[] args = new String[valueSet.size() + 1]; + SQLiteDatabase database = databaseHelper.getWritableDatabase(); + String selection = ID + " = ?"; + String[] args = new String[]{id.serialize()}; - args[0] = id.serialize(); + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, contentValues); - int i = 0; - - for (Map.Entry entry : valueSet) { - qualifier.append(entry.getKey()).append(" != ?"); - - if (i != valueSet.size() - 1) { - qualifier.append(" OR "); - } - - args[i + 1] = String.valueOf(entry.getValue()); - - i++; - } - - - return database.update(TABLE_NAME, contentValues, ID + " = ? AND (" + qualifier + ")", args) > 0; + return database.update(TABLE_NAME, contentValues, result.first(), result.second()) > 0; } private @NonNull Optional getByColumn(@NonNull String column, String value) { diff --git a/src/org/thoughtcrime/securesms/util/SqlUtil.java b/src/org/thoughtcrime/securesms/util/SqlUtil.java index e3019b341c..5dad5cc49d 100644 --- a/src/org/thoughtcrime/securesms/util/SqlUtil.java +++ b/src/org/thoughtcrime/securesms/util/SqlUtil.java @@ -1,11 +1,20 @@ package org.thoughtcrime.securesms.util; +import android.content.ContentValues; import android.database.Cursor; import androidx.annotation.NonNull; import net.sqlcipher.database.SQLiteDatabase; +import org.whispersystems.libsignal.util.Pair; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Set; + public final class SqlUtil { private SqlUtil() {} @@ -31,4 +40,39 @@ public final class SqlUtil { return false; } + + /** + * Returns an updated query and args pairing that will only update rows that would *actually* + * change. In other words, if {@link SQLiteDatabase#update(String, ContentValues, String, String[])} + * returns > 0, then you know something *actually* changed. + */ + public static @NonNull Pair buildTrueUpdateQuery(@NonNull String selection, + @NonNull String[] args, + @NonNull ContentValues contentValues) + { + StringBuilder qualifier = new StringBuilder(); + Set> valueSet = contentValues.valueSet(); + List fullArgs = new ArrayList<>(args.length + valueSet.size()); + + fullArgs.addAll(Arrays.asList(args)); + + int i = 0; + + for (Map.Entry entry : valueSet) { + if (entry.getValue() != null) { + qualifier.append(entry.getKey()).append(" != ? OR ").append(entry.getKey()).append(" IS NULL"); + fullArgs.add(String.valueOf(entry.getValue())); + } else { + qualifier.append(entry.getKey()).append(" NOT NULL"); + } + + if (i != valueSet.size() - 1) { + qualifier.append(" OR "); + } + + i++; + } + + return new Pair<>("(" + selection + ") AND (" + qualifier + ")", fullArgs.toArray(new String[0])); + } } diff --git a/test/unitTest/java/org/thoughtcrime/securesms/database/SqliteUtilTest.java b/test/unitTest/java/org/thoughtcrime/securesms/database/SqliteUtilTest.java new file mode 100644 index 0000000000..7e144b8d7d --- /dev/null +++ b/test/unitTest/java/org/thoughtcrime/securesms/database/SqliteUtilTest.java @@ -0,0 +1,95 @@ +package org.thoughtcrime.securesms.database; + +import android.app.Application; +import android.content.ContentValues; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; +import org.thoughtcrime.securesms.util.SqlUtil; +import org.whispersystems.libsignal.util.Pair; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE, application = Application.class) +public class SqliteUtilTest { + + @Test + public void buildTrueUpdateQuery_simple() { + String selection = "_id = ?"; + String[] args = new String[]{"1"}; + + ContentValues values = new ContentValues(); + values.put("a", 2); + + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, values); + + assertEquals("(_id = ?) AND (a != ? OR a IS NULL)", result.first()); + assertArrayEquals(new String[] { "1", "2" }, result.second()); + } + + @Test + public void buildTrueUpdateQuery_complexSelection() { + String selection = "_id = ? AND (foo = ? OR bar != ?)"; + String[] args = new String[]{"1", "2", "3"}; + + ContentValues values = new ContentValues(); + values.put("a", 4); + + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, values); + + assertEquals("(_id = ? AND (foo = ? OR bar != ?)) AND (a != ? OR a IS NULL)", result.first()); + assertArrayEquals(new String[] { "1", "2", "3", "4" }, result.second()); + } + + @Test + public void buildTrueUpdateQuery_multipleContentValues() { + String selection = "_id = ?"; + String[] args = new String[]{"1"}; + + ContentValues values = new ContentValues(); + values.put("a", 2); + values.put("b", 3); + values.put("c", 4); + + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, values); + + assertEquals("(_id = ?) AND (a != ? OR a IS NULL OR b != ? OR b IS NULL OR c != ? OR c IS NULL)", result.first()); + assertArrayEquals(new String[] { "1", "2", "3", "4"}, result.second()); + } + + @Test + public void buildTrueUpdateQuery_nullContentValue() { + String selection = "_id = ?"; + String[] args = new String[]{"1"}; + + ContentValues values = new ContentValues(); + values.put("a", (String) null); + + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, values); + + assertEquals("(_id = ?) AND (a NOT NULL)", result.first()); + assertArrayEquals(new String[] { "1" }, result.second()); + } + + @Test + public void buildTrueUpdateQuery_complexContentValue() { + String selection = "_id = ?"; + String[] args = new String[]{"1"}; + + ContentValues values = new ContentValues(); + values.put("a", (String) null); + values.put("b", 2); + values.put("c", 3); + values.put("d", (String) null); + values.put("e", (String) null); + + Pair result = SqlUtil.buildTrueUpdateQuery(selection, args, values); + + assertEquals("(_id = ?) AND (a NOT NULL OR b != ? OR b IS NULL OR c != ? OR c IS NULL OR d NOT NULL OR e NOT NULL)", result.first()); + assertArrayEquals(new String[] { "1", "2", "3" }, result.second()); + } +}