From a240266106bf99780a48d86883499dbebd20fbda Mon Sep 17 00:00:00 2001 From: zachh Date: Fri, 12 Jan 2018 15:19:00 -0800 Subject: Cleaned up wording around "valid" and "formattable". We don't actually parition by "formattable", we parition by "valid". An invalid number like 456-7890 can be formatted to E164 ("+14567890") but what ParitionedNumbers actually does is parition by valid/invalid (and then converts the valid numbers to E164). Also added a new sharded test suite for phonenumberproto tests which had occasionally been timing out on TAP. Test: existing PiperOrigin-RevId: 181800443 Change-Id: Id64fc32c893025b0115dd350dd87e3277607f21c --- .../DialerBlockedNumberPhoneLookup.java | 9 +-- .../phonelookup/cp2/Cp2LocalPhoneLookup.java | 74 +++++++++++----------- .../phonenumberproto/DialerPhoneNumberUtil.java | 6 +- .../phonenumberproto/PartitionedNumbers.java | 51 ++++++++------- .../android/dialer/telecom/TelecomCallUtil.java | 18 +++--- 5 files changed, 80 insertions(+), 78 deletions(-) (limited to 'java') diff --git a/java/com/android/dialer/phonelookup/blockednumber/DialerBlockedNumberPhoneLookup.java b/java/com/android/dialer/phonelookup/blockednumber/DialerBlockedNumberPhoneLookup.java index 54df3995c..e6c15e8d9 100644 --- a/java/com/android/dialer/phonelookup/blockednumber/DialerBlockedNumberPhoneLookup.java +++ b/java/com/android/dialer/phonelookup/blockednumber/DialerBlockedNumberPhoneLookup.java @@ -134,7 +134,8 @@ public final class DialerBlockedNumberPhoneLookup implements PhoneLookup { if (TextUtils.isEmpty(rawNumber)) { return Cp2Info.getDefaultInstance(); } - Optional e164 = TelecomCallUtil.getE164Number(appContext, call); + Optional validE164 = TelecomCallUtil.getValidE164Number(appContext, call); Set cp2ContactInfos = new ArraySet<>(); // Note: It would make sense to use PHONE_LOOKUP for E164 numbers as well, but we use PHONE to // ensure consistency when the batch methods are used to update data. try (Cursor cursor = - e164.isPresent() + validE164.isPresent() ? queryPhoneTableBasedOnE164( - Cp2Projections.getProjectionForPhoneTable(), ImmutableSet.of(e164.get())) + Cp2Projections.getProjectionForPhoneTable(), ImmutableSet.of(validE164.get())) : queryPhoneLookup(Cp2Projections.getProjectionForPhoneLookupTable(), rawNumber)) { if (cursor == null) { LogUtil.w("Cp2LocalPhoneLookup.lookupInternal", "null cursor"); @@ -154,7 +154,7 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { @Override public ListenableFuture isDirty(ImmutableSet phoneNumbers) { PartitionedNumbers partitionedNumbers = new PartitionedNumbers(phoneNumbers); - if (partitionedNumbers.unformattableNumbers().size() > MAX_SUPPORTED_INVALID_NUMBERS) { + if (partitionedNumbers.invalidNumbers().size() > MAX_SUPPORTED_INVALID_NUMBERS) { // If there are N invalid numbers, we can't determine determine dirtiness without running N // queries; since running this many queries is not feasible for the (lightweight) isDirty // check, simply return true. The expectation is that this should rarely be the case as the @@ -239,15 +239,14 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { List>> queryFutures = new ArrayList<>(); - // First use the E164 numbers to query the NORMALIZED_NUMBER column. + // First use the valid E164 numbers to query the NORMALIZED_NUMBER column. queryFutures.add( queryPhoneTableForContactIdsBasedOnE164(partitionedNumbers.validE164Numbers())); // Then run a separate query for each invalid number. Separate queries are done to accomplish // loose matching which couldn't be accomplished with a batch query. - Assert.checkState( - partitionedNumbers.unformattableNumbers().size() <= MAX_SUPPORTED_INVALID_NUMBERS); - for (String invalidNumber : partitionedNumbers.unformattableNumbers()) { + Assert.checkState(partitionedNumbers.invalidNumbers().size() <= MAX_SUPPORTED_INVALID_NUMBERS); + for (String invalidNumber : partitionedNumbers.invalidNumbers()) { queryFutures.add(queryPhoneLookupTableForContactIdsBasedOnRawNumber(invalidNumber)); } return Futures.transform( @@ -527,10 +526,9 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { ImmutableMap existingInfoMap) { ArraySet unprocessableNumbers = new ArraySet<>(); PartitionedNumbers partitionedNumbers = new PartitionedNumbers(existingInfoMap.keySet()); - if (partitionedNumbers.unformattableNumbers().size() > MAX_SUPPORTED_INVALID_NUMBERS) { - for (String invalidNumber : partitionedNumbers.unformattableNumbers()) { - unprocessableNumbers.addAll( - partitionedNumbers.dialerPhoneNumbersForUnformattable(invalidNumber)); + if (partitionedNumbers.invalidNumbers().size() > MAX_SUPPORTED_INVALID_NUMBERS) { + for (String invalidNumber : partitionedNumbers.invalidNumbers()) { + unprocessableNumbers.addAll(partitionedNumbers.dialerPhoneNumbersForInvalid(invalidNumber)); } } return unprocessableNumbers; @@ -652,39 +650,39 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { return Futures.immediateFuture(new ArrayMap<>()); } - // Divide the numbers into those we can format to E164 and those we can't. Issue a single - // batch query for the E164 numbers against the PHONE table, and in parallel issue - // individual queries against PHONE_LOOKUP for each non-E164 number. + // Divide the numbers into those that are valid and those that are not. Issue a single + // batch query for the valid numbers against the PHONE table, and in parallel issue + // individual queries against PHONE_LOOKUP for each invalid number. // TODO(zachh): These queries are inefficient without a lastModified column to filter on. PartitionedNumbers partitionedNumbers = new PartitionedNumbers(ImmutableSet.copyOf(updatedNumbers)); - ListenableFuture>> e164Future = + ListenableFuture>> validNumbersFuture = batchQueryForValidNumbers(partitionedNumbers.validE164Numbers()); - List>> nonE164FuturesList = new ArrayList<>(); - for (String invalidNumber : partitionedNumbers.unformattableNumbers()) { - nonE164FuturesList.add(individualQueryForInvalidNumber(invalidNumber)); + List>> invalidNumbersFuturesList = new ArrayList<>(); + for (String invalidNumber : partitionedNumbers.invalidNumbers()) { + invalidNumbersFuturesList.add(individualQueryForInvalidNumber(invalidNumber)); } - ListenableFuture>> nonE164Future = - Futures.allAsList(nonE164FuturesList); + ListenableFuture>> invalidNumbersFuture = + Futures.allAsList(invalidNumbersFuturesList); Callable>> computeMap = () -> { // These get() calls are safe because we are using whenAllSucceed below. - Map> e164Result = e164Future.get(); - List> non164Results = nonE164Future.get(); + Map> validNumbersResult = validNumbersFuture.get(); + List> invalidNumbersResult = invalidNumbersFuture.get(); Map> map = new ArrayMap<>(); - // First update the map with the E164 results. - for (Entry> entry : e164Result.entrySet()) { - String e164Number = entry.getKey(); + // First update the map with the valid number results. + for (Entry> entry : validNumbersResult.entrySet()) { + String validNumber = entry.getKey(); Set cp2ContactInfos = entry.getValue(); Set dialerPhoneNumbers = - partitionedNumbers.dialerPhoneNumbersForE164(e164Number); + partitionedNumbers.dialerPhoneNumbersForValidE164(validNumber); addInfo(map, dialerPhoneNumbers, cp2ContactInfos); @@ -694,12 +692,12 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { updatedNumbers.removeAll(dialerPhoneNumbers); } - // Next update the map with the non-E164 results. + // Next update the map with the invalid results. int i = 0; - for (String unformattableNumber : partitionedNumbers.unformattableNumbers()) { - Set cp2Infos = non164Results.get(i++); + for (String invalidNumber : partitionedNumbers.invalidNumbers()) { + Set cp2Infos = invalidNumbersResult.get(i++); Set dialerPhoneNumbers = - partitionedNumbers.dialerPhoneNumbersForUnformattable(unformattableNumber); + partitionedNumbers.dialerPhoneNumbersForInvalid(invalidNumber); addInfo(map, dialerPhoneNumbers, cp2Infos); @@ -717,32 +715,32 @@ public final class Cp2LocalPhoneLookup implements PhoneLookup { } return map; }; - return Futures.whenAllSucceed(e164Future, nonE164Future) + return Futures.whenAllSucceed(validNumbersFuture, invalidNumbersFuture) .call(computeMap, lightweightExecutorService); }, lightweightExecutorService); } private ListenableFuture>> batchQueryForValidNumbers( - Set e164Numbers) { + Set validE164Numbers) { return backgroundExecutorService.submit( () -> { Map> cp2ContactInfosByNumber = new ArrayMap<>(); - if (e164Numbers.isEmpty()) { + if (validE164Numbers.isEmpty()) { return cp2ContactInfosByNumber; } try (Cursor cursor = queryPhoneTableBasedOnE164( - Cp2Projections.getProjectionForPhoneTable(), e164Numbers)) { + Cp2Projections.getProjectionForPhoneTable(), validE164Numbers)) { if (cursor == null) { LogUtil.w("Cp2LocalPhoneLookup.batchQueryForValidNumbers", "null cursor"); } else { while (cursor.moveToNext()) { - String e164Number = Cp2Projections.getNormalizedNumberFromCursor(cursor); - Set cp2ContactInfos = cp2ContactInfosByNumber.get(e164Number); + String validE164Number = Cp2Projections.getNormalizedNumberFromCursor(cursor); + Set cp2ContactInfos = cp2ContactInfosByNumber.get(validE164Number); if (cp2ContactInfos == null) { cp2ContactInfos = new ArraySet<>(); - cp2ContactInfosByNumber.put(e164Number, cp2ContactInfos); + cp2ContactInfosByNumber.put(validE164Number, cp2ContactInfos); } cp2ContactInfos.add( Cp2Projections.buildCp2ContactInfoFromCursor(appContext, cursor)); diff --git a/java/com/android/dialer/phonenumberproto/DialerPhoneNumberUtil.java b/java/com/android/dialer/phonenumberproto/DialerPhoneNumberUtil.java index 8cb4557cb..8969737d4 100644 --- a/java/com/android/dialer/phonenumberproto/DialerPhoneNumberUtil.java +++ b/java/com/android/dialer/phonenumberproto/DialerPhoneNumberUtil.java @@ -132,13 +132,13 @@ public class DialerPhoneNumberUtil { * Formats the provided number to E164 format or return a normalized version of the raw number if * the number is not valid according to {@link PhoneNumberUtil#isValidNumber(PhoneNumber)}. * - * @see #formatToE164(DialerPhoneNumber) + * @see #formatToValidE164(DialerPhoneNumber) * @see PhoneNumberUtils#normalizeNumber(String) */ @WorkerThread public String normalizeNumber(DialerPhoneNumber number) { Assert.isWorkerThread(); - return formatToE164(number) + return formatToValidE164(number) .or(PhoneNumberUtils.normalizeNumber(number.getRawInput().getNumber())); } @@ -154,7 +154,7 @@ public class DialerPhoneNumberUtil { * @see PhoneNumberUtils#formatNumberToE164(String, String) */ @WorkerThread - public Optional formatToE164(DialerPhoneNumber number) { + public Optional formatToValidE164(DialerPhoneNumber number) { Assert.isWorkerThread(); if (number.hasDialerInternalPhoneNumber()) { PhoneNumber phoneNumber = Converter.protoToPojo(number.getDialerInternalPhoneNumber()); diff --git a/java/com/android/dialer/phonenumberproto/PartitionedNumbers.java b/java/com/android/dialer/phonenumberproto/PartitionedNumbers.java index 372f21ee8..4c8ac2f21 100644 --- a/java/com/android/dialer/phonenumberproto/PartitionedNumbers.java +++ b/java/com/android/dialer/phonenumberproto/PartitionedNumbers.java @@ -30,14 +30,14 @@ import java.util.Map; import java.util.Set; /** - * Divides a set of {@link DialerPhoneNumber DialerPhoneNumbers} by those that can be formatted to - * E164 and those that cannot. + * Divides a set of {@link DialerPhoneNumber DialerPhoneNumbers} according to those that are valid + * according to libphonenumber, and those that are not. */ public final class PartitionedNumbers { private final ImmutableMap> e164NumbersToDialerPhoneNumbers; private final ImmutableMap> - unformattableNumbersToDialerPhoneNumbers; + invalidNumbersToDialerPhoneNumbers; @WorkerThread public PartitionedNumbers(@NonNull ImmutableSet dialerPhoneNumbers) { @@ -45,12 +45,12 @@ public final class PartitionedNumbers { DialerPhoneNumberUtil dialerPhoneNumberUtil = new DialerPhoneNumberUtil(PhoneNumberUtil.getInstance()); Map> e164MapBuilder = new ArrayMap<>(); - Map> unformattableMapBuilder = new ArrayMap<>(); + Map> invalidMapBuilder = new ArrayMap<>(); for (DialerPhoneNumber dialerPhoneNumber : dialerPhoneNumbers) { - Optional e164 = dialerPhoneNumberUtil.formatToE164(dialerPhoneNumber); - if (e164.isPresent()) { - String validE164 = e164.get(); + Optional optValidE164 = dialerPhoneNumberUtil.formatToValidE164(dialerPhoneNumber); + if (optValidE164.isPresent()) { + String validE164 = optValidE164.get(); Set currentNumbers = e164MapBuilder.get(validE164); if (currentNumbers == null) { currentNumbers = new ArraySet<>(); @@ -58,49 +58,52 @@ public final class PartitionedNumbers { } currentNumbers.add(dialerPhoneNumber); } else { - String unformattableNumber = dialerPhoneNumber.getRawInput().getNumber(); - Set currentNumbers = unformattableMapBuilder.get(unformattableNumber); + String invalidNumber = dialerPhoneNumber.getRawInput().getNumber(); + Set currentNumbers = invalidMapBuilder.get(invalidNumber); if (currentNumbers == null) { currentNumbers = new ArraySet<>(); - unformattableMapBuilder.put(unformattableNumber, currentNumbers); + invalidMapBuilder.put(invalidNumber, currentNumbers); } currentNumbers.add(dialerPhoneNumber); } } e164NumbersToDialerPhoneNumbers = makeImmutable(e164MapBuilder); - unformattableNumbersToDialerPhoneNumbers = makeImmutable(unformattableMapBuilder); + invalidNumbersToDialerPhoneNumbers = makeImmutable(invalidMapBuilder); } - /** Returns the set of formatted number from the original DialerPhoneNumbers */ + /** Returns the set of invalid numbers from the original DialerPhoneNumbers */ @NonNull - public ImmutableSet unformattableNumbers() { - return unformattableNumbersToDialerPhoneNumbers.keySet(); + public ImmutableSet invalidNumbers() { + return invalidNumbersToDialerPhoneNumbers.keySet(); } - /** Returns the set of raw number that is unformattable from the original DialerPhoneNumbers */ + /** Returns the set of valid, E164 formatted numbers from the original DialerPhoneNumbers */ @NonNull public ImmutableSet validE164Numbers() { return e164NumbersToDialerPhoneNumbers.keySet(); } /** - * Returns the corresponding set of original DialerPhoneNumber that maps to the e.164 number, or - * an empty set if the number is not found. + * Returns the corresponding set of original DialerPhoneNumbers that map to the valid E164 number + * from {@link #validE164Numbers()}. + * + * @throws NullPointerException if there are no numbers found */ @NonNull - public ImmutableSet dialerPhoneNumbersForE164(String e164) { - return Assert.isNotNull(e164NumbersToDialerPhoneNumbers.get(e164)); + public ImmutableSet dialerPhoneNumbersForValidE164(String validE164) { + return Assert.isNotNull(e164NumbersToDialerPhoneNumbers.get(validE164)); } /** - * Returns the corresponding set of original DialerPhoneNumber that maps to the unformattable - * number returned by {@link #unformattableNumbers()}, or an empty set if the number is not found. + * Returns the corresponding set of original DialerPhoneNumbers that map to the invalid number + * from {@link #invalidNumbers()}. + * + * @throws NullPointerException if there are no numbers found */ @NonNull - public ImmutableSet dialerPhoneNumbersForUnformattable( - String unformattableNumber) { - return Assert.isNotNull(unformattableNumbersToDialerPhoneNumbers.get(unformattableNumber)); + public ImmutableSet dialerPhoneNumbersForInvalid(String invalidNumber) { + return Assert.isNotNull(invalidNumbersToDialerPhoneNumbers.get(invalidNumber)); } private static ImmutableMap> makeImmutable( diff --git a/java/com/android/dialer/telecom/TelecomCallUtil.java b/java/com/android/dialer/telecom/TelecomCallUtil.java index 7d71b4b90..3ae952357 100644 --- a/java/com/android/dialer/telecom/TelecomCallUtil.java +++ b/java/com/android/dialer/telecom/TelecomCallUtil.java @@ -82,9 +82,9 @@ public class TelecomCallUtil { public static Optional getNormalizedNumber(Context appContext, Call call) { Assert.isWorkerThread(); - Optional e164 = getE164Number(appContext, call); - if (e164.isPresent()) { - return e164; + Optional validE164 = getValidE164Number(appContext, call); + if (validE164.isPresent()) { + return validE164; } String rawNumber = getNumber(call); if (TextUtils.isEmpty(rawNumber)) { @@ -94,14 +94,14 @@ public class TelecomCallUtil { } /** - * Formats the number of the {@code call} to E.164. The country of the SIM associated with the - * call is used to determine the country. + * Formats the number of the {@code call} to E.164 if it is valid. The country of the SIM + * associated with the call is used to determine the country. * - *

If the number cannot be formatted (because for example the country cannot be determined), - * returns {@link Optional#absent()}. + *

If the number cannot be formatted (because for example it is invalid or the country cannot + * be determined), returns {@link Optional#absent()}. */ @WorkerThread - public static Optional getE164Number(Context appContext, Call call) { + public static Optional getValidE164Number(Context appContext, Call call) { Assert.isWorkerThread(); String rawNumber = getNumber(call); if (TextUtils.isEmpty(rawNumber)) { @@ -109,7 +109,7 @@ public class TelecomCallUtil { } Optional countryCode = getCountryCode(appContext, call); if (!countryCode.isPresent()) { - LogUtil.w("TelecomCallUtil.getE164Number", "couldn't find a country code for call"); + LogUtil.w("TelecomCallUtil.getValidE164Number", "couldn't find a country code for call"); return Optional.absent(); } return Optional.fromNullable(PhoneNumberUtils.formatNumberToE164(rawNumber, countryCode.get())); -- cgit v1.2.3