From e1f679bfd2a3a815673d62eee56355f29f2b51ee Mon Sep 17 00:00:00 2001
From: David Beaumont <dbeaumont@google.com>
Date: Wed, 5 Sep 2012 16:02:01 +0000
Subject: [PATCH] JAVA: Better exception handling during metadata loading.

---
 .../i18n/phonenumbers/PhoneNumberUtil.java    | 37 +++++++++++++------
 .../phonenumbers/PhoneNumberUtilTest.java     | 24 ++++++++++++
 java/release_notes.txt                        |  4 ++
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java b/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
index b33231830..7e4e6bb5d 100644
--- a/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
+++ b/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
@@ -558,25 +558,38 @@ public class PhoneNumberUtil {
     nanpaRegions.addAll(countryCallingCodeToRegionCodeMap.get(NANPA_COUNTRY_CODE));
   }
 
-  private void loadMetadataFromFile(String filePrefix, String regionCode, int countryCallingCode) {
+  // @VisibleForTesting
+  void loadMetadataFromFile(String filePrefix, String regionCode, int countryCallingCode) {
     boolean isNonGeoRegion = REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCode);
-    InputStream source = isNonGeoRegion
-        ? PhoneNumberUtil.class.getResourceAsStream(filePrefix + "_" + countryCallingCode)
-        : PhoneNumberUtil.class.getResourceAsStream(filePrefix + "_" + regionCode);
+    String fileName = filePrefix + "_" +
+        (isNonGeoRegion ? String.valueOf(countryCallingCode) : regionCode);
+    InputStream source = PhoneNumberUtil.class.getResourceAsStream(fileName);
+    if (source == null) {
+      LOGGER.log(Level.SEVERE, "missing metadata: " + fileName);
+      throw new RuntimeException("missing metadata: " + fileName);
+    }
     ObjectInputStream in = null;
     try {
       in = new ObjectInputStream(source);
       PhoneMetadataCollection metadataCollection = new PhoneMetadataCollection();
       metadataCollection.readExternal(in);
-      for (PhoneMetadata metadata : metadataCollection.getMetadataList()) {
-        if (isNonGeoRegion) {
-          countryCodeToNonGeographicalMetadataMap.put(countryCallingCode, metadata);
-        } else {
-          regionToMetadataMap.put(regionCode, metadata);
-        }
+      List<PhoneMetadata> metadataList = metadataCollection.getMetadataList();
+      if (metadataList.isEmpty()) {
+        LOGGER.log(Level.SEVERE, "empty metadata: " + fileName);
+        throw new RuntimeException("empty metadata: " + fileName);
+      }
+      if (metadataList.size() > 1) {
+        LOGGER.log(Level.WARNING, "invalid metadata (too many entries): " + fileName);
+      }
+      PhoneMetadata metadata = metadataList.get(0);
+      if (isNonGeoRegion) {
+        countryCodeToNonGeographicalMetadataMap.put(countryCallingCode, metadata);
+      } else {
+        regionToMetadataMap.put(regionCode, metadata);
       }
     } catch (IOException e) {
-      LOGGER.log(Level.WARNING, e.toString());
+      LOGGER.log(Level.SEVERE, "cannot load/parse metadata: " + fileName, e);
+      throw new RuntimeException("cannot load/parse metadata: " + fileName, e);
     } finally {
       close(in);
     }
@@ -587,7 +600,7 @@ public class PhoneNumberUtil {
       try {
         in.close();
       } catch (IOException e) {
-        LOGGER.log(Level.WARNING, e.toString());
+        LOGGER.log(Level.WARNING, "error closing input stream (ignored)", e);
       }
     }
   }
diff --git a/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java b/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
index 59f3f7059..714429c11 100644
--- a/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
+++ b/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
@@ -111,6 +111,30 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase {
     assertTrue(phoneUtil.getSupportedRegions().size() > 0);
   }
 
+  public void testGetInstanceLoadBadMetadata() {
+    assertNull(phoneUtil.getMetadataForRegion("No Such Region"));
+    assertNull(phoneUtil.getMetadataForNonGeographicalRegion(-1));
+  }
+
+  public void testMissingMetadataFileThrowsRuntimeException() {
+    // In normal usage we should never get a state where we are asking to load metadata that doesn't
+    // exist. However if the library is packaged incorrectly in the jar, this could happen and the
+    // best we can do is make sure the exception has the file name in it.
+    try {
+      phoneUtil.loadMetadataFromFile("no/such/file", "XX", -1);
+      fail("expected exception");
+    } catch (RuntimeException e) {
+      assertTrue("Unexpected error: " + e, e.toString().contains("no/such/file_XX"));
+    }
+    try {
+      phoneUtil.loadMetadataFromFile(
+          "no/such/file", PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY, 123);
+      fail("expected exception");
+    } catch (RuntimeException e) {
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_123"));
+    }
+  }
+
   public void testGetInstanceLoadUSMetadata() {
     PhoneMetadata metadata = phoneUtil.getMetadataForRegion(RegionCode.US);
     assertEquals("US", metadata.getId());
diff --git a/java/release_notes.txt b/java/release_notes.txt
index 20f29c738..ab720e898 100644
--- a/java/release_notes.txt
+++ b/java/release_notes.txt
@@ -1,3 +1,7 @@
+Sep 5, 2010: libphonenumber-5.1.1
+* Code changes:
+  - Added better logging/exception handling for catching cases where metadata is invalid/missing.
+
 Sep 3, 2012: libphonenumber-5.1
 * Code changes:
  - Inserting a space after the national prefix in the AsYouTypeFormatter when formatting numbers in