Improve parsing logic to be smarter about national-prefix detection & stripping. (#1894)

cpp/src/phonenumbers/phonenumberutil.cc

@@ -316,9 +316,6 @@ void GetSupportedTypesForMetadata(
 
 // Helper method to check a number against possible lengths for this number
 // type, and determine whether it matches, or is too short or too long.
-// Currently, if a number pattern suggests that numbers of length 7 and 10 are
-// possible, and a number in between these possible lengths is entered, such as
-// of length 8, this will return TOO_LONG.
 PhoneNumberUtil::ValidationResult TestNumberLength(
     const string& number, const PhoneMetadata& metadata,
     PhoneNumberUtil::PhoneNumberType type) {
@@ -397,9 +394,7 @@ PhoneNumberUtil::ValidationResult TestNumberLength(
 
 // Helper method to check a number against possible lengths for this region,
 // based on the metadata being passed in, and determine whether it matches, or
-// is too short or too long. Currently, if a number pattern suggests that
-// numbers of length 7 and 10 are possible, and a number in between these
-// possible lengths is entered, such as of length 8, this will return TOO_LONG.
+// is too short or too long.
 PhoneNumberUtil::ValidationResult TestNumberLength(
     const string& number, const PhoneMetadata& metadata) {
   return TestNumberLength(number, metadata, PhoneNumberUtil::UNKNOWN);
@@ -2184,8 +2179,11 @@ PhoneNumberUtil::ErrorType PhoneNumberUtil::ParseHelper(
     // and carrier code be long enough to be a possible length for the region.
     // Otherwise, we don't do the stripping, since the original number could be
     // a valid short number.
-    if (TestNumberLength(potential_national_number, *country_metadata) !=
-        TOO_SHORT) {
+    ValidationResult validation_result =
+        TestNumberLength(potential_national_number, *country_metadata);
+    if (validation_result != TOO_SHORT &&
+        validation_result != IS_POSSIBLE_LOCAL_ONLY &&
+        validation_result != INVALID_LENGTH) {
       normalized_national_number.assign(potential_national_number);
       if (keep_raw_input && !carrier_code.empty()) {
         temp_number.set_preferred_domestic_carrier_code(carrier_code);

cpp/test/phonenumbers/phonenumberutil_test.cc

@@ -3662,6 +3662,17 @@ TEST_F(PhoneNumberUtilTest, ParseNationalNumber) {
   EXPECT_EQ(PhoneNumberUtil::NO_PARSING_ERROR,
             phone_util_.Parse("12", RegionCode::NZ(), &test_number));
   EXPECT_EQ(short_number, test_number);
+
+  // Test for short-code with leading zero for a country which has 0 as
+  // national prefix. Ensure it's not interpreted as national prefix if the
+  // remaining number length is local-only in terms of length. Example: In GB,
+  // length 6-7 are only possible local-only.
+  short_number.set_country_code(44);
+  short_number.set_national_number(123456);
+  short_number.set_italian_leading_zero(true);
+  EXPECT_EQ(PhoneNumberUtil::NO_PARSING_ERROR,
+            phone_util_.Parse("0123456", RegionCode::GB(), &test_number));
+  EXPECT_EQ(short_number, test_number);
 }
 
 TEST_F(PhoneNumberUtilTest, ParseNumberWithAlphaCharacters) {

java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java

@@ -2478,9 +2478,7 @@ public class PhoneNumberUtil {
 
   /**
    * Helper method to check a number against possible lengths for this region, based on the metadata
-   * being passed in, and determine whether it matches, or is too short or too long. Currently, if a
-   * number pattern suggests that numbers of length 7 and 10 are possible, and a number in between
-   * these possible lengths is entered, such as of length 8, this will return TOO_LONG.
+   * being passed in, and determine whether it matches, or is too short or too long.
    */
   private ValidationResult testNumberLength(CharSequence number, PhoneMetadata metadata) {
     return testNumberLength(number, metadata, PhoneNumberType.UNKNOWN);
@@ -2488,9 +2486,7 @@ public class PhoneNumberUtil {
 
   /**
    * Helper method to check a number against possible lengths for this number type, and determine
-   * whether it matches, or is too short or too long. Currently, if a number pattern suggests that
-   * numbers of length 7 and 10 are possible, and a number in between these possible lengths is
-   * entered, such as of length 8, this will return TOO_LONG.
+   * whether it matches, or is too short or too long.
    */
   private ValidationResult testNumberLength(
       CharSequence number, PhoneMetadata metadata, PhoneNumberType type) {
@@ -3222,7 +3218,10 @@ public class PhoneNumberUtil {
       // We require that the NSN remaining after stripping the national prefix and carrier code be
       // long enough to be a possible length for the region. Otherwise, we don't do the stripping,
       // since the original number could be a valid short number.
-      if (testNumberLength(potentialNationalNumber, regionMetadata) != ValidationResult.TOO_SHORT) {
+      ValidationResult validationResult = testNumberLength(potentialNationalNumber, regionMetadata);
+      if (validationResult != ValidationResult.TOO_SHORT
+          && validationResult != ValidationResult.IS_POSSIBLE_LOCAL_ONLY
+          && validationResult != ValidationResult.INVALID_LENGTH) {
         normalizedNationalNumber = potentialNationalNumber;
         if (keepRawInput && carrierCode.length() > 0) {
           phoneNumber.setPreferredDomesticCarrierCode(carrierCode.toString());

java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java

@@ -2121,6 +2121,13 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase {
     PhoneNumber shortNumber = new PhoneNumber();
     shortNumber.setCountryCode(64).setNationalNumber(12L);
     assertEquals(shortNumber, phoneUtil.parse("12", RegionCode.NZ));
+
+    // Test for short-code with leading zero for a country which has 0 as national prefix. Ensure
+    // it's not interpreted as national prefix if the remaining number length is local-only in
+    // terms of length. Example: In GB, length 6-7 are only possible local-only.
+    shortNumber.setCountryCode(44).setNationalNumber(123456)
+        .setItalianLeadingZero(true);
+    assertEquals(shortNumber, phoneUtil.parse("0123456", RegionCode.GB));
   }
 
   public void testParseNumberWithAlphaCharacters() throws Exception {

javascript/i18n/phonenumbers/phonenumberutil.js

@@ -3313,9 +3313,7 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.isPossibleNumberForType =
 /**
  * Helper method to check a number against possible lengths for this region,
  * based on the metadata being passed in, and determine whether it matches, or
- * is too short or too long. Currently, if a number pattern suggests that
- * numbers of length 7 and 10 are possible, and a number in between these
- * possible lengths is entered, such as of length 8, this will return TOO_LONG.
+ * is too short or too long.
  *
  * @param {string} number
  * @param {i18n.phonenumbers.PhoneMetadata} metadata
@@ -3331,10 +3329,7 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.testNumberLength_ =
 
 /**
  * Helper method to check a number against a particular pattern and determine
- * whether it matches, or is too short or too long. Currently, if a number
- * pattern suggests that numbers of length 7 and 10 are possible, and a number
- * in between these possible lengths is entered, such as of length 8, this will
- * return TOO_LONG.
+ * whether it matches, or is too short or too long.
  *
  * @param {string} number
  * @param {i18n.phonenumbers.PhoneMetadata} metadata
@@ -4215,9 +4210,14 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.parseHelper_ =
     // carrier code be long enough to be a possible length for the region.
     // Otherwise, we don't do the stripping, since the original number could be
     // a valid short number.
-    if (this.testNumberLength_(potentialNationalNumber.toString(),
-            regionMetadata) !=
-        i18n.phonenumbers.PhoneNumberUtil.ValidationResult.TOO_SHORT) {
+    var validationResult = this.testNumberLength_(
+        potentialNationalNumber.toString(), regionMetadata);
+    if (validationResult !=
+        i18n.phonenumbers.PhoneNumberUtil.ValidationResult.TOO_SHORT &&
+	validationResult !=
+        i18n.phonenumbers.PhoneNumberUtil.ValidationResult.IS_POSSIBLE_LOCAL_ONLY &&
+	validationResult !=
+        i18n.phonenumbers.PhoneNumberUtil.ValidationResult.INVALID_LENGTH) {
       normalizedNationalNumber = potentialNationalNumber;
       if (keepRawInput && carrierCode.toString().length > 0) {
         phoneNumber.setPreferredDomesticCarrierCode(carrierCode.toString());

javascript/i18n/phonenumbers/phonenumberutil_test.js

@@ -2759,6 +2759,15 @@ function testParseNationalNumber() {
   shortNumber.setCountryCode(64);
   shortNumber.setNationalNumber(12);
   assertTrue(shortNumber.equals(phoneUtil.parse('12', RegionCode.NZ)));
+
+  // Test for short-code with leading zero for a country which has 0 as
+  // national prefix. Ensure it's not interpreted as national prefix if the
+  // remaining number length is local-only in terms of length. Example: In GB,
+  // length 6-7 are only possible local-only.
+  shortNumber.setCountryCode(44);
+  shortNumber.setNationalNumber(123456);
+  shortNumber.setItalianLeadingZero(true);
+  assertTrue(shortNumber.equals(phoneUtil.parse('0123456', RegionCode.GB)));
 }
 
 function testParseNumberWithAlphaCharacters() {

pending_code_changes.txt

@@ -1,2 +1,6 @@
 Code changes:
+ - Improve parsing logic to be smarter about national-prefix detection &
+   stripping based on possible-lengths (IS_POSSIBLE_LOCAL_ONLY and
+   INVALID_LENGTH). Enables e.g. adding Iran short-codes starting with "096"
+   without the need to hack IR's national prefix parsing config.
  - Logging changes: Don't log client-provided phone numbers.