From f1a87518e1e63f5c77e1a5a9316d26f67a5e55de Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Fri, 16 Jul 2021 13:53:17 -0300 Subject: [PATCH] Fix contact search query returning outdated or bad recipients. --- .../securesms/database/RecipientDatabase.java | 260 +++++++++++++----- .../ContactSearchSelectionBuilderTest.kt | 72 +++++ 2 files changed, 256 insertions(+), 76 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/database/ContactSearchSelectionBuilderTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java index e093fc841..335200af1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientDatabase.java +++ b/app/src/main/java/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.protobuf.ByteString; @@ -2314,20 +2315,13 @@ public class RecipientDatabase extends Database { } public @Nullable Cursor getSignalContacts(boolean includeSelf) { - String selection = BLOCKED + " = ? AND " + - REGISTERED + " = ? AND " + - GROUP_ID + " IS NULL AND " + - "(" + SYSTEM_JOINED_NAME + " NOT NULL OR " + PROFILE_SHARING + " = ?) AND " + - "(" + SORT_NAME + " NOT NULL OR " + USERNAME + " NOT NULL)"; - String[] args; - - if (includeSelf) { - args = new String[] { "0", String.valueOf(RegisteredState.REGISTERED.getId()), "1" }; - } else { - selection += " AND " + ID + " != ?"; - args = new String[] { "0", String.valueOf(RegisteredState.REGISTERED.getId()), "1", Recipient.self().getId().serialize() }; - } + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withRegistered(true) + .withGroups(false) + .excludeId(includeSelf ? null : Recipient.self().getId()) + .build(); + String selection = searchSelection.getWhere(); + String[] args = searchSelection.getArgs(); String orderBy = SORT_NAME + ", " + SYSTEM_JOINED_NAME + ", " + SEARCH_PROFILE_NAME + ", " + USERNAME + ", " + PHONE; return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, selection, args, null, null, orderBy); @@ -2336,23 +2330,14 @@ public class RecipientDatabase extends Database { public @Nullable Cursor querySignalContacts(@NonNull String query, boolean includeSelf) { query = buildCaseInsensitiveGlobPattern(query); - String selection = BLOCKED + " = ? AND " + - REGISTERED + " = ? AND " + - GROUP_ID + " IS NULL AND " + - "(" + SYSTEM_JOINED_NAME + " NOT NULL OR " + PROFILE_SHARING + " = ?) AND " + - "(" + - PHONE + " GLOB ? OR " + - SORT_NAME + " GLOB ? OR " + - USERNAME + " GLOB ?" + - ")"; - String[] args; + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withRegistered(true) + .withGroups(false) + .excludeId(includeSelf ? null : Recipient.self().getId()) + .withSearchQuery(query) + .build(); - if (includeSelf) { - args = new String[]{"0", String.valueOf(RegisteredState.REGISTERED.getId()), "1", query, query, query}; - } else { - selection += " AND " + ID + " != ?"; - args = new String[] { "0", String.valueOf(RegisteredState.REGISTERED.getId()), "1", query, query, query, String.valueOf(Recipient.self().getId().toLong()) }; - } + String selection = searchSelection.getWhere(); + String[] args = searchSelection.getArgs(); String orderBy = SORT_NAME + ", " + SYSTEM_JOINED_NAME + ", " + SEARCH_PROFILE_NAME + ", " + PHONE; @@ -2360,12 +2345,12 @@ public class RecipientDatabase extends Database { } public @Nullable Cursor getNonSignalContacts() { - String selection = BLOCKED + " = ? AND " + - REGISTERED + " != ? AND " + - GROUP_ID + " IS NULL AND " + - SYSTEM_CONTACT_URI + " NOT NULL AND " + - "(" + PHONE + " NOT NULL OR " + EMAIL + " NOT NULL)"; - String[] args = new String[] { "0", String.valueOf(RegisteredState.REGISTERED.getId()) }; + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withNonRegistered(true) + .withGroups(false) + .build(); + + String selection = searchSelection.getWhere(); + String[] args = searchSelection.getArgs(); String orderBy = SYSTEM_JOINED_NAME + ", " + PHONE; return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, selection, args, null, null, orderBy); @@ -2374,61 +2359,42 @@ public class RecipientDatabase extends Database { public @Nullable Cursor queryNonSignalContacts(@NonNull String query) { query = buildCaseInsensitiveGlobPattern(query); - String selection = BLOCKED + " = ? AND " + - REGISTERED + " != ? AND " + - GROUP_ID + " IS NULL AND " + - SYSTEM_CONTACT_URI + " NOT NULL AND " + - "(" + PHONE + " NOT NULL OR " + EMAIL + " NOT NULL) AND " + - "(" + - PHONE + " GLOB ? OR " + - EMAIL + " GLOB ? OR " + - SYSTEM_JOINED_NAME + " GLOB ?" + - ")"; - String[] args = new String[] { "0", String.valueOf(RegisteredState.REGISTERED.getId()), query, query, query }; + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withNonRegistered(true) + .withGroups(false) + .withSearchQuery(query) + .build(); + + String selection = searchSelection.getWhere(); + String[] args = searchSelection.getArgs(); String orderBy = SYSTEM_JOINED_NAME + ", " + PHONE; return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, selection, args, null, null, orderBy); } public @Nullable Cursor getNonGroupContacts(boolean includeSelf) { - String selection = BLOCKED + " = ? AND " + - GROUP_ID + " IS NULL AND " + - "(" + SYSTEM_JOINED_NAME + " NOT NULL OR " + PROFILE_SHARING + " = ?) AND " + - "(" + SORT_NAME + " NOT NULL OR " + USERNAME + " NOT NULL)"; - String[] args; + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withRegistered(true) + .withNonRegistered(true) + .withGroups(false) + .excludeId(includeSelf ? null : Recipient.self().getId()) + .build(); - if (includeSelf) { - args = SqlUtil.buildArgs("0", "1"); - } else { - selection += " AND " + ID + " != ?"; - args = SqlUtil.buildArgs("0", "1", Recipient.self().getId().serialize()); - } + String orderBy = SORT_NAME + ", " + SYSTEM_JOINED_NAME + ", " + SEARCH_PROFILE_NAME + ", " + USERNAME + ", " + PHONE; - String orderBy = SORT_NAME + ", " + SYSTEM_JOINED_NAME + ", " + SEARCH_PROFILE_NAME + ", " + USERNAME + ", " + PHONE; - - return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, selection, args, null, null, orderBy); + return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, searchSelection.where, searchSelection.args, null, null, orderBy); } public @Nullable Cursor queryNonGroupContacts(@NonNull String query, boolean includeSelf) { query = buildCaseInsensitiveGlobPattern(query); - String selection = BLOCKED + " = ? AND " + - GROUP_ID + " IS NULL AND " + - "(" + SYSTEM_JOINED_NAME + " NOT NULL OR " + PROFILE_SHARING + " = ?) AND " + - "(" + - PHONE + " GLOB ? OR " + - SORT_NAME + " GLOB ? OR " + - USERNAME + " GLOB ?" + - ")"; - String[] args; - - if (includeSelf) { - args = SqlUtil.buildArgs("0", "1", query, query, query); - } else { - selection += " AND " + ID + " != ?"; - args = SqlUtil.buildArgs("0", "1", query, query, query, Recipient.self().getId().toLong()); - } + ContactSearchSelection searchSelection = new ContactSearchSelection.Builder().withRegistered(true) + .withNonRegistered(true) + .withGroups(false) + .excludeId(includeSelf ? null : Recipient.self().getId()) + .withSearchQuery(query) + .build(); + String selection = searchSelection.getWhere(); + String[] args = searchSelection.getArgs(); String orderBy = SORT_NAME + ", " + SYSTEM_JOINED_NAME + ", " + SEARCH_PROFILE_NAME + ", " + PHONE; return databaseHelper.getReadableDatabase().query(TABLE_NAME, SEARCH_PROJECTION, selection, args, null, null, orderBy); @@ -3589,4 +3555,146 @@ public class RecipientDatabase extends Database { this.neededInsert = neededInsert; } } + + @VisibleForTesting + static final class ContactSearchSelection { + + static final String FILTER_GROUPS = " AND " + GROUP_ID + " IS NULL"; + static final String FILTER_ID = " AND " + ID + " != ?"; + static final String FILTER_BLOCKED = " AND " + BLOCKED + " = ?"; + + static final String NON_SIGNAL_CONTACT = REGISTERED + " != ? AND " + + SYSTEM_CONTACT_URI + " NOT NULL AND " + + "(" + PHONE + " NOT NULL OR " + EMAIL + " NOT NULL)"; + + static final String QUERY_NON_SIGNAL_CONTACT = NON_SIGNAL_CONTACT + + " AND (" + + PHONE + " GLOB ? OR " + + EMAIL + " GLOB ? OR " + + SYSTEM_JOINED_NAME + " GLOB ?" + + ")"; + + static final String SIGNAL_CONTACT = REGISTERED + " = ? AND " + + "(" + SYSTEM_JOINED_NAME + " NOT NULL OR " + PROFILE_SHARING + " = ?) AND " + + "(" + SORT_NAME + " NOT NULL OR " + USERNAME + " NOT NULL)"; + + static final String QUERY_SIGNAL_CONTACT = SIGNAL_CONTACT + " AND (" + + PHONE + " GLOB ? OR " + + SORT_NAME + " GLOB ? OR " + + USERNAME + " GLOB ?" + + ")"; + + private final String where; + private final String[] args; + + private ContactSearchSelection(@NonNull String where, @NonNull String[] args) { + this.where = where; + this.args = args; + } + + String getWhere() { + return where; + } + + String[] getArgs() { + return args; + } + + @VisibleForTesting + static final class Builder { + + private boolean includeRegistered; + private boolean includeNonRegistered; + private RecipientId excludeId; + private boolean excludeGroups; + private String searchQuery; + + @NonNull Builder withRegistered(boolean includeRegistered) { + this.includeRegistered = includeRegistered; + return this; + } + + @NonNull Builder withNonRegistered(boolean includeNonRegistered) { + this.includeNonRegistered = includeNonRegistered; + return this; + } + + @NonNull Builder excludeId(@Nullable RecipientId recipientId) { + this.excludeId = recipientId; + return this; + } + + @NonNull Builder withGroups(boolean includeGroups) { + this.excludeGroups = !includeGroups; + return this; + } + + @NonNull Builder withSearchQuery(@NonNull String searchQuery) { + this.searchQuery = searchQuery; + return this; + } + + @NonNull ContactSearchSelection build() { + if (!includeRegistered && !includeNonRegistered) { + throw new IllegalStateException("Must include either registered or non-registered recipients in search"); + } + + StringBuilder stringBuilder = new StringBuilder("("); + List args = new LinkedList<>(); + + if (includeRegistered) { + stringBuilder.append("("); + + args.add(RegisteredState.REGISTERED.id); + args.add(1); + + if (Util.isEmpty(searchQuery)) { + stringBuilder.append(SIGNAL_CONTACT); + } else { + stringBuilder.append(QUERY_SIGNAL_CONTACT); + args.add(searchQuery); + args.add(searchQuery); + args.add(searchQuery); + } + + stringBuilder.append(")"); + } + + if (includeRegistered && includeNonRegistered) { + stringBuilder.append(" OR "); + } + + if (includeNonRegistered) { + stringBuilder.append("("); + args.add(RegisteredState.REGISTERED.id); + + if (Util.isEmpty(searchQuery)) { + stringBuilder.append(NON_SIGNAL_CONTACT); + } else { + stringBuilder.append(QUERY_SIGNAL_CONTACT); + args.add(searchQuery); + args.add(searchQuery); + args.add(searchQuery); + } + + stringBuilder.append(")"); + } + + stringBuilder.append(")"); + stringBuilder.append(FILTER_BLOCKED); + args.add(0); + + if (excludeGroups) { + stringBuilder.append(FILTER_GROUPS); + } + + if (excludeId != null) { + stringBuilder.append(FILTER_ID); + args.add(excludeId.serialize()); + } + + return new ContactSearchSelection(stringBuilder.toString(), args.stream().map(Object::toString).toArray(String[]::new)); + } + } + } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/database/ContactSearchSelectionBuilderTest.kt b/app/src/test/java/org/thoughtcrime/securesms/database/ContactSearchSelectionBuilderTest.kt new file mode 100644 index 000000000..08b9b719e --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/database/ContactSearchSelectionBuilderTest.kt @@ -0,0 +1,72 @@ +package org.thoughtcrime.securesms.database + +import org.junit.Assert +import org.junit.Test +import org.thoughtcrime.securesms.recipients.RecipientId +import org.thoughtcrime.securesms.util.SqlUtil + +class ContactSearchSelectionBuilderTest { + @Test(expected = IllegalStateException::class) + fun `Given non registered and registered are false, when I build, then I expect an IllegalStateException`() { + RecipientDatabase.ContactSearchSelection.Builder() + .withNonRegistered(false) + .withRegistered(false) + .build() + } + + @Test + fun `Given registered, when I build, then I expect SIGNAL_CONTACT`() { + val result = RecipientDatabase.ContactSearchSelection.Builder() + .withRegistered(true) + .build() + + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.SIGNAL_CONTACT)) + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.FILTER_BLOCKED)) + Assert.assertArrayEquals(SqlUtil.buildArgs(RecipientDatabase.RegisteredState.REGISTERED.id, 1, 0), result.args) + } + + @Test + fun `Given exclude id, when I build, then I expect FILTER_ID`() { + val result = RecipientDatabase.ContactSearchSelection.Builder() + .withRegistered(true) + .excludeId(RecipientId.from(12)) + .build() + + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.FILTER_ID)) + } + + @Test + fun `Given all non group contacts, when I build, then I expect both CONTACT and FILTER_GROUP`() { + val result = RecipientDatabase.ContactSearchSelection.Builder() + .withRegistered(true) + .withNonRegistered(true) + .withGroups(false) + .build() + + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.SIGNAL_CONTACT)) + Assert.assertTrue(result.where.contains(") OR (")) + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.NON_SIGNAL_CONTACT)) + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.FILTER_GROUPS)) + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.FILTER_BLOCKED)) + Assert.assertArrayEquals( + SqlUtil.buildArgs( + RecipientDatabase.RegisteredState.REGISTERED.id, 1, + RecipientDatabase.RegisteredState.REGISTERED.id, + 0 + ), + result.args + ) + } + + @Test + fun `Given a query, when I build, then I expect QUERY_SIGNAL_CONTACT`() { + val result = RecipientDatabase.ContactSearchSelection.Builder() + .withRegistered(true) + .withGroups(false) + .withSearchQuery("query") + .build() + + Assert.assertTrue(result.where.contains(RecipientDatabase.ContactSearchSelection.QUERY_SIGNAL_CONTACT)) + Assert.assertTrue(result.args.contains("query")) + } +}