From 4ee24a9492c7c83952f59ecc54071c31aa68fa86 Mon Sep 17 00:00:00 2001 From: zachh Date: Thu, 11 Jan 2018 14:05:10 -0800 Subject: 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 --- .../composite/CompositePhoneLookup.java | 14 ++- .../PhoneLookupHistoryContentProvider.java | 101 ++++++++++++--------- .../contract/PhoneLookupHistoryContract.java | 10 ++ 3 files changed, 79 insertions(+), 46 deletions(-) (limited to 'java/com/android/dialer/phonelookup') 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 public ListenableFuture isDirty(ImmutableSet phoneNumbers) { List> 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 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"; -- cgit v1.2.3