Fixed bug in handling of empty numbers in new call log.

Empty numbers were not being inserted into PhoneLookupHistory because the URI "content://.../PhoneLookupHistory/" is treated the same as "content://.../PhoneLookupHistory" (w/o the trailing slash). This caused the update (i.e. replace) operation to incorrectly update all rows in the table when it should have updated a single row.

The fix for this was to switch to a query parameter, so the empty number URI now looks like "content://.../PhoneLookupHistory?number="

Also improved some logging while debugging this problem.

Bug: 71866050
Test: unit and manual
PiperOrigin-RevId: 181659081
Change-Id: Idec4fb77e74920cd5485620b0a997db03aa8ff9b

6 files changed, 89 insertions, 60 deletions

diff --git a/java/com/android/dialer/calllog/database/MutationApplier.java b/java/com/android/dialer/calllog/database/MutationApplier.java
index eee810eb8..2fb52558e 100644
--- a/java/com/android/dialer/calllog/database/MutationApplier.java
+++ b/java/com/android/dialer/calllog/database/MutationApplier.java
@@ -68,7 +68,7 @@ public class MutationApplier {
 
     if (!mutations.getInserts().isEmpty()) {
       LogUtil.i(
-          "CallLogMutations.applyToDatabase", "inserting %d rows", mutations.getInserts().size());
+          "MutationApplier.applyToDatabase", "inserting %d rows", mutations.getInserts().size());
       for (Entry<Long, ContentValues> entry : mutations.getInserts().entrySet()) {
         long id = entry.getKey();
         ContentValues contentValues = entry.getValue();
@@ -82,7 +82,7 @@ public class MutationApplier {
 
     if (!mutations.getUpdates().isEmpty()) {
       LogUtil.i(
-          "CallLogMutations.applyToDatabase", "updating %d rows", mutations.getUpdates().size());
+          "MutationApplier.applyToDatabase", "updating %d rows", mutations.getUpdates().size());
       for (Entry<Long, ContentValues> entry : mutations.getUpdates().entrySet()) {
         long id = entry.getKey();
         ContentValues contentValues = entry.getValue();
@@ -96,7 +96,7 @@ public class MutationApplier {
 
     if (!mutations.getDeletes().isEmpty()) {
       LogUtil.i(
-          "CallLogMutations.applyToDatabase", "deleting %d rows", mutations.getDeletes().size());
+          "MutationApplier.applyToDatabase", "deleting %d rows", mutations.getDeletes().size());
       String[] questionMarks = new String[mutations.getDeletes().size()];
       Arrays.fill(questionMarks, "?");
 
diff --git a/java/com/android/dialer/calllog/datasources/phonelookup/PhoneLookupDataSource.java b/java/com/android/dialer/calllog/datasources/phonelookup/PhoneLookupDataSource.java
index dca992789..30461a4be 100644
--- a/java/com/android/dialer/calllog/datasources/phonelookup/PhoneLookupDataSource.java
+++ b/java/com/android/dialer/calllog/datasources/phonelookup/PhoneLookupDataSource.java
@@ -21,7 +21,6 @@ import android.content.ContentValues;
 import android.content.Context;
 import android.content.OperationApplicationException;
 import android.database.Cursor;
-import android.net.Uri;
 import android.os.RemoteException;
 import android.support.annotation.MainThread;
 import android.support.annotation.WorkerThread;
@@ -268,12 +267,16 @@ public final class PhoneLookupDataSource
       contentValues.put(PhoneLookupHistory.PHONE_LOOKUP_INFO, phoneLookupInfo.toByteArray());
       contentValues.put(PhoneLookupHistory.LAST_MODIFIED, currentTimestamp);
       operations.add(
-          ContentProviderOperation.newUpdate(numberUri(normalizedNumber))
+          ContentProviderOperation.newUpdate(
+                  PhoneLookupHistory.contentUriForNumber(normalizedNumber))
               .withValues(contentValues)
               .build());
     }
     for (String normalizedNumber : phoneLookupHistoryRowsToDelete) {
-      operations.add(ContentProviderOperation.newDelete(numberUri(normalizedNumber)).build());
+      operations.add(
+          ContentProviderOperation.newDelete(
+                  PhoneLookupHistory.contentUriForNumber(normalizedNumber))
+              .build());
     }
     appContext.getContentResolver().applyBatch(PhoneLookupHistoryContract.AUTHORITY, operations);
     return null;
@@ -596,8 +599,4 @@ public final class PhoneLookupDataSource
     contentValues.put(
         AnnotatedCallLog.CP2_INFO_INCOMPLETE, phoneLookupInfo.getCp2LocalInfo().getIsIncomplete());
   }
-
-  private static Uri numberUri(String number) {
-    return PhoneLookupHistory.CONTENT_URI.buildUpon().appendEncodedPath(number).build();
-  }
 }
diff --git a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
index 34f35311f..68695b7e9 100644
--- a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
+++ b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
@@ -34,6 +34,7 @@ import com.google.common.collect.Maps;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.common.util.concurrent.MoreExecutors;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -88,7 +89,18 @@ public final class CompositePhoneLookup implements PhoneLookup<PhoneLookupInfo>
   public ListenableFuture<Boolean> isDirty(ImmutableSet<DialerPhoneNumber> phoneNumbers) {
     List<ListenableFuture<Boolean>> futures = new ArrayList<>();
     for (PhoneLookup<?> phoneLookup : phoneLookups) {
-      futures.add(phoneLookup.isDirty(phoneNumbers));
+      futures.add(
+          Futures.transform(
+              phoneLookup.isDirty(phoneNumbers),
+              isDirty -> {
+                LogUtil.v(
+                    "CompositePhoneLookup.isDirty",
+                    "isDirty for %s: %b",
+                    phoneLookup.getClass().getSimpleName(),
+                    isDirty);
+                return isDirty;
+              },
+              MoreExecutors.directExecutor()));
     }
     // Executes all child lookups (possibly in parallel), completing when the first composite lookup
     // which returns "true" completes, and cancels the others.
diff --git a/java/com/android/dialer/phonelookup/database/PhoneLookupHistoryContentProvider.java b/java/com/android/dialer/phonelookup/database/PhoneLookupHistoryContentProvider.java
index 5c0c00f81..e20e81369 100644
--- a/java/com/android/dialer/phonelookup/database/PhoneLookupHistoryContentProvider.java
+++ b/java/com/android/dialer/phonelookup/database/PhoneLookupHistoryContentProvider.java
@@ -27,6 +27,7 @@ import android.database.DatabaseUtils;
 import android.database.sqlite.SQLiteDatabase;
 import android.database.sqlite.SQLiteQueryBuilder;
 import android.net.Uri;
+import android.support.annotation.IntDef;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.text.TextUtils;
@@ -34,7 +35,10 @@ import com.android.dialer.common.Assert;
 import com.android.dialer.common.LogUtil;
 import com.android.dialer.phonelookup.database.contract.PhoneLookupHistoryContract;
 import com.android.dialer.phonelookup.database.contract.PhoneLookupHistoryContract.PhoneLookupHistory;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
+import java.util.List;
 
 /**
  * {@link ContentProvider} for the PhoneLookupHistory.
@@ -54,22 +58,17 @@ import java.util.ArrayList;
  */
 public class PhoneLookupHistoryContentProvider extends ContentProvider {
 
-  // For operations against: content://com.android.dialer.phonelookuphistory/PhoneLookupHistory
-  private static final int PHONE_LOOKUP_HISTORY_TABLE_CODE = 1;
-  // For operations against: content://com.android.dialer.phonelookuphistory/PhoneLookupHistory/+123
-  private static final int PHONE_LOOKUP_HISTORY_TABLE_ID_CODE = 2;
-
-  private static final UriMatcher uriMatcher = new UriMatcher(UriMatcher.NO_MATCH);
-
-  static {
-    uriMatcher.addURI(
-        PhoneLookupHistoryContract.AUTHORITY,
-        PhoneLookupHistory.TABLE,
-        PHONE_LOOKUP_HISTORY_TABLE_CODE);
-    uriMatcher.addURI(
-        PhoneLookupHistoryContract.AUTHORITY,
-        PhoneLookupHistory.TABLE + "/*", // The last path component should be a normalized number
-        PHONE_LOOKUP_HISTORY_TABLE_ID_CODE);
+  /**
+   * Can't use {@link UriMatcher} because it doesn't support empty values, and numbers can be empty.
+   */
+  @Retention(RetentionPolicy.SOURCE)
+  @IntDef({UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE, UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE})
+  private @interface UriType {
+    // For operations against: content://com.android.dialer.phonelookuphistory/PhoneLookupHistory
+    int PHONE_LOOKUP_HISTORY_TABLE_CODE = 1;
+    // For operations against:
+    // content://com.android.dialer.phonelookuphistory/PhoneLookupHistory?number=123
+    int PHONE_LOOKUP_HISTORY_TABLE_ID_CODE = 2;
   }
 
   private PhoneLookupHistoryDatabaseHelper databaseHelper;
@@ -98,15 +97,16 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
     SQLiteDatabase db = databaseHelper.getReadableDatabase();
     SQLiteQueryBuilder queryBuilder = new SQLiteQueryBuilder();
     queryBuilder.setTables(PhoneLookupHistory.TABLE);
-    int match = uriMatcher.match(uri);
-    switch (match) {
-      case PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
+    @UriType int uriType = uriType(uri);
+    switch (uriType) {
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
         queryBuilder.appendWhere(
             PhoneLookupHistory.NORMALIZED_NUMBER
                 + "="
-                + DatabaseUtils.sqlEscapeString(uri.getLastPathSegment()));
+                + DatabaseUtils.sqlEscapeString(
+                    Uri.decode(uri.getQueryParameter(PhoneLookupHistory.NUMBER_QUERY_PARAM))));
         // fall through
-      case PHONE_LOOKUP_HISTORY_TABLE_CODE:
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE:
         Cursor cursor =
             queryBuilder.query(db, projection, selection, selectionArgs, null, null, sortOrder);
         if (cursor == null) {
@@ -134,15 +134,16 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
     Assert.checkArgument(values != null);
 
     SQLiteDatabase database = databaseHelper.getWritableDatabase();
-    int match = uriMatcher.match(uri);
-    switch (match) {
-      case PHONE_LOOKUP_HISTORY_TABLE_CODE:
+    @UriType int uriType = uriType(uri);
+    switch (uriType) {
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE:
         Assert.checkArgument(
-            !TextUtils.isEmpty(values.getAsString(PhoneLookupHistory.NORMALIZED_NUMBER)),
+            values.getAsString(PhoneLookupHistory.NORMALIZED_NUMBER) != null,
             "You must specify a normalized number when inserting");
         break;
-      case PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
-        String normalizedNumberFromUri = uri.getLastPathSegment();
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
+        String normalizedNumberFromUri =
+            Uri.decode(uri.getQueryParameter(PhoneLookupHistory.NUMBER_QUERY_PARAM));
         String normalizedNumberFromValues =
             values.getAsString(PhoneLookupHistory.NORMALIZED_NUMBER);
         Assert.checkArgument(
@@ -168,10 +169,8 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
       return null;
     }
     Uri insertedUri =
-        PhoneLookupHistory.CONTENT_URI
-            .buildUpon()
-            .appendEncodedPath(values.getAsString(PhoneLookupHistory.NORMALIZED_NUMBER))
-            .build();
+        PhoneLookupHistory.contentUriForNumber(
+            values.getAsString(PhoneLookupHistory.NORMALIZED_NUMBER));
     if (!isApplyingBatch()) {
       notifyChange(insertedUri);
     }
@@ -182,15 +181,15 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
   public int delete(
       @NonNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) {
     SQLiteDatabase database = databaseHelper.getWritableDatabase();
-    final int match = uriMatcher.match(uri);
-    switch (match) {
-      case PHONE_LOOKUP_HISTORY_TABLE_CODE:
+    @UriType int uriType = uriType(uri);
+    switch (uriType) {
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE:
         break;
-      case PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
         Assert.checkArgument(selection == null, "Do not specify selection when deleting by number");
         Assert.checkArgument(
             selectionArgs == null, "Do not specify selection args when deleting by number");
-        String number = uri.getLastPathSegment();
+        String number = Uri.decode(uri.getQueryParameter(PhoneLookupHistory.NUMBER_QUERY_PARAM));
         Assert.checkArgument(!TextUtils.isEmpty(number), "error parsing number from uri: %s", uri);
         selection = PhoneLookupHistory.NORMALIZED_NUMBER + "= ?";
         selectionArgs = new String[] {number};
@@ -228,9 +227,9 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
     Assert.checkArgument(values != null);
 
     SQLiteDatabase database = databaseHelper.getWritableDatabase();
-    int match = uriMatcher.match(uri);
-    switch (match) {
-      case PHONE_LOOKUP_HISTORY_TABLE_CODE:
+    @UriType int uriType = uriType(uri);
+    switch (uriType) {
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE:
         int rows = database.update(PhoneLookupHistory.TABLE, values, selection, selectionArgs);
         if (rows == 0) {
           LogUtil.w("PhoneLookupHistoryContentProvider.update", "no rows updated");
@@ -240,7 +239,7 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
           notifyChange(uri);
         }
         return rows;
-      case PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
+      case UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
         Assert.checkArgument(
             !values.containsKey(PhoneLookupHistory.NORMALIZED_NUMBER),
             "Do not specify number in values when updating by number");
@@ -248,7 +247,8 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
         Assert.checkArgument(
             selectionArgs == null, "Do not specify selection args when updating by ID");
 
-        String normalizedNumber = uri.getLastPathSegment();
+        String normalizedNumber =
+            Uri.decode(uri.getQueryParameter(PhoneLookupHistory.NUMBER_QUERY_PARAM));
         values.put(PhoneLookupHistory.NORMALIZED_NUMBER, normalizedNumber);
         long result = database.replace(PhoneLookupHistory.TABLE, null, values);
         Assert.checkArgument(result != -1, "replacing PhoneLookupHistory row failed");
@@ -282,10 +282,10 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
       database.beginTransaction();
       for (int i = 0; i < operations.size(); i++) {
         ContentProviderOperation operation = operations.get(i);
-        int match = uriMatcher.match(operation.getUri());
-        switch (match) {
-          case PHONE_LOOKUP_HISTORY_TABLE_CODE:
-          case PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
+        @UriType int uriType = uriType(operation.getUri());
+        switch (uriType) {
+          case UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE:
+          case UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE:
             ContentProviderResult result = operation.apply(this, results, i);
             if (operation.isInsert()) {
               if (result.uri == null) {
@@ -312,4 +312,15 @@ public class PhoneLookupHistoryContentProvider extends ContentProvider {
   private void notifyChange(Uri uri) {
     getContext().getContentResolver().notifyChange(uri, null);
   }
+
+  @UriType
+  private int uriType(Uri uri) {
+    Assert.checkArgument(uri.getAuthority().equals(PhoneLookupHistoryContract.AUTHORITY));
+    List<String> pathSegments = uri.getPathSegments();
+    Assert.checkArgument(pathSegments.size() == 1);
+    Assert.checkArgument(pathSegments.get(0).equals(PhoneLookupHistory.TABLE));
+    return uri.getQueryParameter(PhoneLookupHistory.NUMBER_QUERY_PARAM) == null
+        ? UriType.PHONE_LOOKUP_HISTORY_TABLE_CODE
+        : UriType.PHONE_LOOKUP_HISTORY_TABLE_ID_CODE;
+  }
 }
diff --git a/java/com/android/dialer/phonelookup/database/contract/PhoneLookupHistoryContract.java b/java/com/android/dialer/phonelookup/database/contract/PhoneLookupHistoryContract.java
index f8e108496..a195926ac 100644
--- a/java/com/android/dialer/phonelookup/database/contract/PhoneLookupHistoryContract.java
+++ b/java/com/android/dialer/phonelookup/database/contract/PhoneLookupHistoryContract.java
@@ -30,10 +30,20 @@ public class PhoneLookupHistoryContract {
 
     public static final String TABLE = "PhoneLookupHistory";
 
+    public static final String NUMBER_QUERY_PARAM = "number";
+
     /** The content URI for this table. */
     public static final Uri CONTENT_URI =
         Uri.withAppendedPath(PhoneLookupHistoryContract.CONTENT_URI, TABLE);
 
+    /** Returns a URI for a specific normalized number */
+    public static Uri contentUriForNumber(String normalizedNumber) {
+      return CONTENT_URI
+          .buildUpon()
+          .appendQueryParameter(NUMBER_QUERY_PARAM, Uri.encode(normalizedNumber))
+          .build();
+    }
+
     /** The MIME type of a {@link android.content.ContentProvider#getType(Uri)} single entry. */
     public static final String CONTENT_ITEM_TYPE = "vnd.android.cursor.item/phone_lookup_history";
 
diff --git a/java/com/android/incallui/PhoneLookupHistoryRecorder.java b/java/com/android/incallui/PhoneLookupHistoryRecorder.java
index 667c0d1cc..8517deb65 100644
--- a/java/com/android/incallui/PhoneLookupHistoryRecorder.java
+++ b/java/com/android/incallui/PhoneLookupHistoryRecorder.java
@@ -66,10 +66,7 @@ final class PhoneLookupHistoryRecorder {
             appContext
                 .getContentResolver()
                 .update(
-                    PhoneLookupHistory.CONTENT_URI
-                        .buildUpon()
-                        .appendEncodedPath(normalizedNumber.get())
-                        .build(),
+                    PhoneLookupHistory.contentUriForNumber(normalizedNumber.get()),
                     contentValues,
                     null,
                     null);