Simplify and avoid repetition in parse helper methods.

Implements #22279.

CHANGELOG.md

@@ -4,6 +4,7 @@
    - Turn keyword strings into enums and use the appropriate enum sets
      and maps to avoid repeating string literals and to use more speedy
      collection types.
+   - Simplify and avoid repetition in parse helper methods.
 
 
 # Changes in version 1.7.0 - 2017-05-17

src/main/java/org/torproject/descriptor/impl/BridgeNetworkStatusImpl.java

@@ -110,7 +110,7 @@ public class BridgeNetworkStatusImpl extends NetworkStatusImpl
           + line + "'.");
     }
     SortedMap<String, String> flagThresholds =
-        ParseHelper.parseKeyValueStringPairs(line, parts, 1, "=");
+        ParseHelper.parseKeyValueStringPairs(line, parts, 1);
     try {
       for (Map.Entry<String, String> e : flagThresholds.entrySet()) {
         switch (e.getKey()) {

src/main/java/org/torproject/descriptor/impl/KeyValueMap.java

+/* Copyright 2017 The Tor Project
+ * See LICENSE for licensing information */
+
+package org.torproject.descriptor.impl;
+
+import org.torproject.descriptor.DescriptorParseException;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.TreeMap;
+
+public class KeyValueMap<T> extends TreeMap<String, T> {
+
+  private Class<T> clazz;
+
+  public KeyValueMap(Class<T> clazz) {
+    super();
+    this.clazz = clazz;
+  }
+
+  private void putPair(String key, T value, String line, String listElement,
+      int keyLength) throws DescriptorParseException {
+    if (this.keySet().contains(key)) {
+      throw new DescriptorParseException("Line '" + line + "' contains "
+          + "duplicate key '" + key + "'.");
+    }
+    if (null == key || key.isEmpty()
+        || (keyLength > 0 && key.length() != keyLength)) {
+      throw new DescriptorParseException("Line '" + line + "' contains an "
+          + "illegal key in list element '" + listElement + "'.");
+    }
+    if (null == value) {
+      throw new DescriptorParseException("Line '" + line + "' contains an "
+          + "illegal value in list element '" + listElement + "'.");
+    }
+    this.put(key, value);
+  }
+
+  /** Extract key value maps of numbers and verify the key-value pairs. */
+  public KeyValueMap<T> parseKeyValueList(String line, String[] partsNoOpt,
+      int startIndex, int keyLength, String separatorPattern)
+      throws DescriptorParseException {
+    if (startIndex >= partsNoOpt.length) {
+      return this;
+    }
+    String[] keysAndValues = " ".equals(separatorPattern) ? partsNoOpt
+        : partsNoOpt[startIndex].split(separatorPattern, -1);
+    for (int i = " ".equals(separatorPattern) ? startIndex : 0;
+        i < keysAndValues.length; i++) {
+      String listElement = keysAndValues[i];
+      String[] keyAndValue = listElement.split("=");
+      String key = keyAndValue[0];
+      T value = null;
+      if (keyAndValue.length == 2) {
+        try {
+          Method method = clazz.getMethod("valueOf", String.class);
+          value = (T) method.invoke(clazz, keyAndValue[1]);
+        } catch (IllegalAccessException | SecurityException e) {
+          throw new RuntimeException("This shouldn't happen.", e);
+        } catch (IllegalArgumentException | InvocationTargetException e) {
+          value = null;
+        } catch (NoSuchMethodException e) { // use the String value
+          value = (T)keyAndValue[1];
+        }
+      }
+      this.putPair(key, value, line, listElement, keyLength);
+    }
+    return this;
+  }
+
+}

src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java

@@ -201,7 +201,7 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
       throws DescriptorParseException {
     this.parsedAtMostOnceKey(Key.W);
     SortedMap<String, Integer> pairs =
-        ParseHelper.parseKeyValueIntegerPairs(line, parts, 1, "=");
+        ParseHelper.parseKeyValueIntegerPairs(line, parts, 1);
     if (pairs.isEmpty()) {
       throw new DescriptorParseException("Illegal line '" + line + "'.");
     }

src/main/java/org/torproject/descriptor/impl/ParseHelper.java

@@ -10,7 +10,6 @@ import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.LinkedHashMap;
 import java.util.Locale;
 import java.util.Map;
 import java.util.SortedMap;
@@ -243,37 +242,17 @@ public class ParseHelper {
   }
 
   protected static SortedMap<String, String> parseKeyValueStringPairs(
-      String line, String[] parts, int startIndex, String separatorString)
+      String line, String[] parts, int startIndex)
       throws DescriptorParseException {
-    SortedMap<String, String> result = new TreeMap<>();
-    for (int i = startIndex; i < parts.length; i++) {
-      String pair = parts[i];
-      String[] pairParts = pair.split(separatorString);
-      if (pairParts.length != 2) {
-        throw new DescriptorParseException("Illegal key-value pair in "
-            + "line '" + line + "'.");
-      }
-      result.put(pairParts[0], pairParts[1]);
-    }
-    return result;
+    return (new KeyValueMap<String>(String.class))
+        .parseKeyValueList(line, parts, startIndex, 0, " ");
   }
 
   protected static SortedMap<String, Integer> parseKeyValueIntegerPairs(
-      String line, String[] parts, int startIndex, String separatorString)
+      String line, String[] parts, int startIndex)
       throws DescriptorParseException {
-    SortedMap<String, Integer> result = new TreeMap<>();
-    SortedMap<String, String> keyValueStringPairs =
-        ParseHelper.parseKeyValueStringPairs(line, parts, startIndex,
-        separatorString);
-    for (Map.Entry<String, String> e : keyValueStringPairs.entrySet()) {
-      try {
-        result.put(e.getKey(), Integer.parseInt(e.getValue()));
-      } catch (NumberFormatException ex) {
-        throw new DescriptorParseException("Illegal value in line '"
-            + line + "'.");
-      }
-    }
-    return result;
+    return (new KeyValueMap<Integer>(Integer.class))
+        .parseKeyValueList(line, parts, startIndex, 0, " ");
   }
 
   private static Pattern nicknamePattern =
@@ -331,48 +310,22 @@ public class ParseHelper {
         .toUpperCase();
   }
 
-  private static Map<Integer, Pattern>
-      commaSeparatedKeyValueListPatterns = new HashMap<>();
-
   protected static String parseCommaSeparatedKeyIntegerValueList(
       String line, String[] partsNoOpt, int index, int keyLength)
       throws DescriptorParseException {
-    String result = "";
-    if (partsNoOpt.length < index) {
-      throw new DescriptorParseException("Line '" + line + "' does not "
-          + "contain a key-value list at index " + index + ".");
-    } else if (partsNoOpt.length > index) {
-      if (!commaSeparatedKeyValueListPatterns.containsKey(keyLength)) {
-        String keyPattern = "[0-9a-zA-Z?<>\\-_]"
-            + (keyLength == 0 ? "+" : "{" + keyLength + "}");
-        String valuePattern = "\\-?[0-9]{1,9}";
-        String patternString = String.format("^%s=%s(,%s=%s)*$",
-            keyPattern, valuePattern, keyPattern, valuePattern);
-        commaSeparatedKeyValueListPatterns.put(keyLength,
-            Pattern.compile(patternString));
-      }
-      Pattern pattern = commaSeparatedKeyValueListPatterns.get(
-          keyLength);
-      if (pattern.matcher(partsNoOpt[index]).matches()) {
-        result = partsNoOpt[index];
-      } else {
-        throw new DescriptorParseException("Line '" + line + "' "
-            + "contains an illegal key or value.");
-      }
-    }
-    return result;
+    return parseStringKeyIntegerList(line, partsNoOpt, index, keyLength);
   }
 
   protected static SortedMap<String, Integer>
       convertCommaSeparatedKeyIntegerValueList(String validatedString) {
-    SortedMap<String, Integer> result = null;
-    if (validatedString != null) {
-      result = new TreeMap<>();
-      if (validatedString.contains("=")) {
-        for (String listElement : validatedString.split(",", -1)) {
-          String[] keyAndValue = listElement.split("=");
-          result.put(keyAndValue[0], Integer.parseInt(keyAndValue[1]));
-        }
+    KeyValueMap<Integer> result = new KeyValueMap<>(Integer.class);
+    if (!validatedString.isEmpty()) {
+      try {
+        result.parseKeyValueList(validatedString,
+            new String[]{ validatedString }, 0, 0, ",");
+      } catch (DescriptorParseException e) {
+        throw new RuntimeException("Should have been caught in earlier "
+            + "validation step, but wasn't. ", e);
       }
     }
     return result;
@@ -382,34 +335,8 @@ public class ParseHelper {
       parseCommaSeparatedKeyLongValueList(String line,
       String[] partsNoOpt, int index, int keyLength)
       throws DescriptorParseException {
-    SortedMap<String, Long> result = new TreeMap<>();
-    if (partsNoOpt.length < index) {
-      throw new DescriptorParseException("Line '" + line + "' does not "
-          + "contain a key-value list at index " + index + ".");
-    } else if (partsNoOpt.length > index) {
-      String[] listElements = partsNoOpt[index].split(",", -1);
-      for (String listElement : listElements) {
-        String[] keyAndValue = listElement.split("=");
-        String key = null;
-        long value = -1;
-        if (keyAndValue.length == 2 && (keyLength == 0
-            || keyAndValue[0].length() == keyLength)) {
-          try {
-            value = Long.parseLong(keyAndValue[1]);
-            key = keyAndValue[0];
-          } catch (NumberFormatException e) {
-            /* Handle below. */
-          }
-        }
-        if (null == key || key.isEmpty()) {
-          throw new DescriptorParseException("Line '" + line + "' "
-              + "contains an illegal key or value in list element '"
-              + listElement + "'.");
-        }
-        result.put(key, value);
-      }
-    }
-    return result;
+    return (new KeyValueMap<Long>(Long.class))
+        .parseKeyValueList(line, partsNoOpt, index, keyLength, ",");
   }
 
   protected static Integer[] parseCommaSeparatedIntegerValueList(
@@ -464,70 +391,27 @@ public class ParseHelper {
       parseSpaceSeparatedStringKeyDoubleValueMap(String line,
       String[] partsNoOpt, int startIndex)
       throws DescriptorParseException {
-    Map<String, Double> result = new LinkedHashMap<>();
-    if (partsNoOpt.length < startIndex) {
-      throw new DescriptorParseException("Line '" + line + "' does not "
-          + "contain a key-value list starting at index " + startIndex
-          + ".");
-    }
-    for (int i = startIndex; i < partsNoOpt.length; i++) {
-      String listElement = partsNoOpt[i];
-      String[] keyAndValue = listElement.split("=");
-      String key = null;
-      Double value = null;
-      if (keyAndValue.length == 2) {
-        try {
-          value = Double.parseDouble(keyAndValue[1]);
-          key = keyAndValue[0];
-        } catch (NumberFormatException e) {
-          /* Handle below. */
-        }
-      }
-      if (null == key || key.isEmpty()) {
-        throw new DescriptorParseException("Line '" + line + "' contains "
-            + "an illegal key or value in list element '" + listElement
-            + "'.");
-      }
-      result.put(key, value);
-    }
-    return result;
+    return (new KeyValueMap<Double>(Double.class))
+        .parseKeyValueList(line, partsNoOpt, startIndex, -1, " ");
   }
 
   protected static Map<String, Long>
       parseSpaceSeparatedStringKeyLongValueMap(String line,
       String[] partsNoOpt, int startIndex)
       throws DescriptorParseException {
-    Map<String, Long> result = new LinkedHashMap<>();
-    if (partsNoOpt.length < startIndex) {
-      throw new DescriptorParseException("Line '" + line + "' does not "
-          + "contain a key-value list starting at index " + startIndex
-          + ".");
-    }
-    for (int i = startIndex; i < partsNoOpt.length; i++) {
-      String listElement = partsNoOpt[i];
-      String[] keyAndValue = listElement.split("=");
-      String key = null;
-      Long value = null;
-      if (keyAndValue.length == 2) {
-        try {
-          value = Long.parseLong(keyAndValue[1]);
-          key = keyAndValue[0];
-        } catch (NumberFormatException e) {
-          /* Handle below. */
-        }
-      }
-      if (null == key || key.isEmpty()) {
-        throw new DescriptorParseException("Line '" + line + "' contains "
-            + "an illegal key or value in list element '" + listElement
-            + "'.");
-      }
-      if (result.keySet().contains(key)) {
-        throw new DescriptorParseException("Line '" + line + "' contains "
-            + "an already defined key '" + key + "'.");
-      }
-      result.put(key, value);
+    return (new KeyValueMap<Long>(Long.class))
+        .parseKeyValueList(line, partsNoOpt, startIndex, -1, " ");
+  }
+
+  private static String parseStringKeyIntegerList(String line,
+      String[] partsNoOpt, int startIndex, int keyLength)
+      throws DescriptorParseException {
+    if (startIndex >= partsNoOpt.length) {
+      return "";
     }
-    return result;
+    KeyValueMap<Integer> result = new KeyValueMap<>(Integer.class);
+    result.parseKeyValueList(line, partsNoOpt, startIndex, keyLength, ",");
+    return partsNoOpt[startIndex];
   }
 
   protected static String

src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java

@@ -354,7 +354,7 @@ public class RelayNetworkStatusConsensusImpl extends NetworkStatusImpl
   private void parseParamsLine(String line, String[] parts)
       throws DescriptorParseException {
     this.consensusParams = ParseHelper.parseKeyValueIntegerPairs(line,
-        parts, 1, "=");
+        parts, 1);
   }
 
   private void parseSharedRandPreviousValueLine(String line, String[] parts)
@@ -390,7 +390,7 @@ public class RelayNetworkStatusConsensusImpl extends NetworkStatusImpl
   private void parseBandwidthWeightsLine(String line, String[] parts)
       throws DescriptorParseException {
     this.bandwidthWeights = ParseHelper.parseKeyValueIntegerPairs(line,
-        parts, 1, "=");
+        parts, 1);
   }
 
   private String consensusDigest;

src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java

@@ -420,7 +420,7 @@ public class RelayNetworkStatusVoteImpl extends NetworkStatusImpl
           + line + "'.");
     }
     SortedMap<String, String> flagThresholds =
-        ParseHelper.parseKeyValueStringPairs(line, parts, 1, "=");
+        ParseHelper.parseKeyValueStringPairs(line, parts, 1);
     try {
       for (Map.Entry<String, String> e : flagThresholds.entrySet()) {
         switch (e.getKey()) {
@@ -467,7 +467,7 @@ public class RelayNetworkStatusVoteImpl extends NetworkStatusImpl
   private void parseParamsLine(String line, String[] parts)
       throws DescriptorParseException {
     this.consensusParams = ParseHelper.parseKeyValueIntegerPairs(line,
-        parts, 1, "=");
+        parts, 1);
   }
 
   private void parseDirSourceLine(String line, String[] parts)

src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java

@@ -1278,12 +1278,9 @@ public class ExtraInfoDescriptorImplTest {
         + "gb=208,");
   }
 
-  @Test()
+  @Test(expected = DescriptorParseException.class)
   public void testGeoipClientOriginsDuplicate()
       throws DescriptorParseException {
-    /* dir-spec.txt doesn't say anything about duplicate country codes, so
-     * this line is valid, even though it leads to a somewhat undefined
-     * parse result. */
     GeoipStatsBuilder.createWithGeoipClientOriginsLine(
         "geoip-client-origins de=1152,de=952,cn=896,us=712,it=504,"
         + "ru=352,fr=208,gb=208,ir=200");
@@ -1293,8 +1290,8 @@ public class ExtraInfoDescriptorImplTest {
   public void testGeoipClientOriginsExtraArg()
       throws DescriptorParseException {
     GeoipStatsBuilder.createWithGeoipClientOriginsLine(
-        "geoip-client-origins de=1152,de=952,cn=896,us=712,it=504 "
-            + "ru=352 fr=208 gb=208 ir=200");
+        "geoip-client-origins de=1152,cn=896,us=712,it=504 ru=352 fr=208 "
+        + "gb=208 ir=200");
   }
 
   @Test()
@@ -1387,7 +1384,8 @@ public class ExtraInfoDescriptorImplTest {
       throws DescriptorParseException {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage(
-        "Line 'dirreq-v3-resp =10848' contains an illegal key or value.");
+        "Line 'dirreq-v3-resp =10848' contains an illegal key in list element "
+        + "'=10848'.");
     DirreqStatsBuilder.createWithDirreqV3RespLine("dirreq-v3-resp =10848");
   }
 
@@ -1638,7 +1636,7 @@ public class ExtraInfoDescriptorImplTest {
       throws DescriptorParseException {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage("Line 'exit-kibibytes-written =74647' contains "
-        + "an illegal key or value in list element '=74647'.");
+        + "an illegal key in list element '=74647'.");
     ExitStatsBuilder.createWithExitKibibytesWrittenLine(
         "exit-kibibytes-written =74647");
   }
@@ -1846,7 +1844,7 @@ public class ExtraInfoDescriptorImplTest {
   public void testPaddingCountsNoKey() throws DescriptorParseException {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage(Matchers
-        .allOf(Matchers.containsString("illegal key or value in list element"),
+        .allOf(Matchers.containsString("illegal key in list element"),
         Matchers.containsString("=7")));
     DescriptorBuilder.createWithPaddingCountsLine("padding-counts 2017-05-10 "
         + "01:48:43 (86400 s) write-total=9 write-drop=10000 =7 x=8");
@@ -1856,7 +1854,7 @@ public class ExtraInfoDescriptorImplTest {
   public void testPaddingCountsNoValue() throws DescriptorParseException {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage(Matchers
-        .allOf(Matchers.containsString("illegal key or value in list element"),
+        .allOf(Matchers.containsString("illegal value in list element"),
         Matchers.containsString("'write-drop='")));
     DescriptorBuilder.createWithPaddingCountsLine("padding-counts 2017-05-10 "
         + "01:48:43 (86400 s) write-total=7 write-drop= bin-size=10000 ");
@@ -1866,7 +1864,7 @@ public class ExtraInfoDescriptorImplTest {
   public void testPaddingCountsKeyRepeated() throws DescriptorParseException {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage(Matchers
-        .allOf(Matchers.containsString("contains an already defined key"),
+        .allOf(Matchers.containsString("contains duplicate key"),
         Matchers.containsString("'a'")));
     DescriptorBuilder.createWithPaddingCountsLine("padding-counts 2017-05-10 "
         + "01:48:43 (86400 s) a=1 b=2 a=3 b=4");