Stop setting the national number pattern to NA when no data exists (#1751)

cpp/CMakeLists.txt

@@ -459,6 +459,7 @@ endif ()
 set (TEST_SOURCES
   "test/phonenumbers/asyoutypeformatter_test.cc"
   "test/phonenumbers/logger_test.cc"
+  "test/phonenumbers/matcher_test.cc"
   "test/phonenumbers/phonenumberutil_test.cc"
   "test/phonenumbers/regexp_adapter_test.cc"
   "test/phonenumbers/regexp_cache_test.cc"

cpp/src/phonenumbers/phonenumberutil.cc

@@ -285,8 +285,7 @@ bool DescHasData(const PhoneNumberDesc& desc) {
   // since if this is the only thing that's present we don't really support the
   // type at all: no type-specific methods will work with only this data.
   return desc.has_example_number() || DescHasPossibleNumberData(desc) ||
-         (desc.has_national_number_pattern() &&
-          !(desc.national_number_pattern() == "NA"));
+         desc.has_national_number_pattern();
 }
 
 // Returns the types we have metadata for based on the PhoneMetadata object

cpp/src/phonenumbers/regex_based_matcher.cc

@@ -37,24 +37,32 @@ RegexBasedMatcher::RegexBasedMatcher()
 RegexBasedMatcher::~RegexBasedMatcher() {}
 
 bool RegexBasedMatcher::MatchNationalNumber(
-    const string& national_number, const PhoneNumberDesc& number_desc,
+    const string& number,
+    const PhoneNumberDesc& number_desc,
     bool allow_prefix_match) const {
-  return Match(national_number, number_desc.national_number_pattern(),
-               allow_prefix_match);
+  const string& national_number_pattern = number_desc.national_number_pattern();
+  // We don't want to consider it a prefix match when matching non-empty input
+  // against an empty pattern.
+  if (national_number_pattern.empty()) {
+    return false;
+  }
+  return Match(number, national_number_pattern, allow_prefix_match);
 }
 
-bool RegexBasedMatcher::Match(const string& national_number,
-                              const string& number_pattern,
-                              bool allow_prefix_match) const {
+bool RegexBasedMatcher::Match(
+    const string& number,
+    const string& number_pattern,
+    bool allow_prefix_match) const {
   const RegExp& regexp(regexp_cache_->GetRegExp(number_pattern));
 
-  if (allow_prefix_match) {
-    const scoped_ptr<RegExpInput> normalized_number_input(
-        regexp_factory_->CreateInput(national_number));
-    return regexp.Consume(normalized_number_input.get());
-  } else {
-    return regexp.FullMatch(national_number);
+  if (regexp.FullMatch(number)) {
+    return true;
   }
+  const scoped_ptr<RegExpInput> normalized_number_input(
+      regexp_factory_->CreateInput(number));
+  return regexp.Consume(normalized_number_input.get())
+      ? allow_prefix_match
+      : false;
 }
 
 }  // namespace phonenumbers

cpp/test/phonenumbers/matcher_test.cc

@@ -0,0 +1,109 @@
+// Copyright (C) 2017 The Libphonenumber Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Tests that all implementations of MatcherApi are consistent.
+
+#include "phonenumbers/matcher_api.h"
+
+#include <string>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#include "phonenumbers/regex_based_matcher.h"
+#include "phonenumbers/phonemetadata.pb.h"
+
+namespace i18n {
+namespace phonenumbers {
+
+namespace {
+
+  void ExpectMatched(
+      const MatcherApi& matcher,
+      const string& number,
+      const PhoneNumberDesc& desc) {
+    EXPECT_TRUE(matcher.MatchNationalNumber(number, desc, false));
+    EXPECT_TRUE(matcher.MatchNationalNumber(number, desc, true));
+  }
+
+  void ExpectInvalid(
+      const MatcherApi& matcher,
+      const string& number,
+      const PhoneNumberDesc& desc) {
+    EXPECT_FALSE(matcher.MatchNationalNumber(number, desc, false));
+    EXPECT_FALSE(matcher.MatchNationalNumber(number, desc, true));
+  }
+
+  void ExpectTooLong(
+      const MatcherApi& matcher,
+      const string& number,
+      const PhoneNumberDesc& desc) {
+    EXPECT_FALSE(matcher.MatchNationalNumber(number, desc, false));
+    EXPECT_TRUE(matcher.MatchNationalNumber(number, desc, true));
+  }
+
+}  // namespace
+
+class MatcherTest : public testing::Test {
+ protected:
+  void CheckMatcherBehavesAsExpected(const MatcherApi& matcher) const {
+    PhoneNumberDesc desc;
+
+    desc = CreateDesc("");
+    // Test if there is no matcher data.
+    ExpectInvalid(matcher, "1", desc);
+
+    // Matcher data generated by MatcherCompiler.
+    desc = CreateDesc("9\\d{2}");
+    ExpectInvalid(matcher, "91", desc);
+    ExpectInvalid(matcher, "81", desc);
+    ExpectMatched(matcher, "911", desc);
+    ExpectInvalid(matcher, "811", desc);
+    ExpectTooLong(matcher, "9111", desc);
+    ExpectInvalid(matcher, "8111", desc);
+
+    desc = CreateDesc("\\d{1,2}");
+    ExpectMatched(matcher, "2", desc);
+    ExpectMatched(matcher, "20", desc);
+
+    desc = CreateDesc("20?");
+    ExpectMatched(matcher, "2", desc);
+    ExpectMatched(matcher, "20", desc);
+
+    desc = CreateDesc("2|20");
+    ExpectMatched(matcher, "2", desc);
+    // Subtle case where lookingAt() and matches() result in different end()s.
+    ExpectMatched(matcher, "20", desc);
+  }
+
+ private:
+  // Helper method to set national number fields in the PhoneNumberDesc proto.
+  // Empty fields won't be set.
+  PhoneNumberDesc CreateDesc(
+      const string& national_number_pattern) const {
+    PhoneNumberDesc desc;
+    if (!national_number_pattern.empty()) {
+      desc.set_national_number_pattern(national_number_pattern);
+    }
+    return desc;
+  }
+};
+
+TEST_F(MatcherTest, RegexBasedMatcher) {
+  RegexBasedMatcher matcher;
+  CheckMatcherBehavesAsExpected(matcher);
+}
+
+}  // namespace phonenumbers
+}  // namespace i18n

cpp/test/phonenumbers/phonenumberutil_test.cc

@@ -239,8 +239,9 @@ TEST_F(PhoneNumberUtilTest, GetInstanceLoadUSMetadata) {
   // separately in the toll free element as well.
   EXPECT_EQ(0, metadata->toll_free().possible_length_size());
   EXPECT_EQ("900\\d{7}", metadata->premium_rate().national_number_pattern());
-  // No shared-cost data is available, so it should be initialised to "NA".
-  EXPECT_EQ("NA", metadata->shared_cost().national_number_pattern());
+  // No shared-cost data is available, so its national number data should not be
+  // set.
+  EXPECT_FALSE(metadata->shared_cost().has_national_number_pattern());
 }
 
 TEST_F(PhoneNumberUtilTest, GetInstanceLoadDEMetadata) {

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

@@ -1001,7 +1001,7 @@ public class PhoneNumberUtil {
     // support the type at all: no type-specific methods will work with only this data.
     return desc.hasExampleNumber()
         || descHasPossibleNumberData(desc)
-        || (desc.hasNationalNumberPattern() && !desc.getNationalNumberPattern().equals("NA"));
+        || desc.hasNationalNumberPattern();
   }
 
   /**