commit 38221ed0973ad63e3b47effa39f297f4af0012ae
Author: Karsten Loesing <karsten.loesing(a)gmx.net>
Date: Mon May 22 22:15:09 2017 +0200
Simplify and avoid repetition in parse helper methods.
Implements #22279.
---
CHANGELOG.md | 1 +
.../descriptor/impl/BridgeNetworkStatusImpl.java | 2 +-
.../torproject/descriptor/impl/KeyValueMap.java | 71 ++++++++
.../descriptor/impl/NetworkStatusEntryImpl.java | 2 +-
.../torproject/descriptor/impl/ParseHelper.java | 178 ++++-----------------
.../impl/RelayNetworkStatusConsensusImpl.java | 4 +-
.../impl/RelayNetworkStatusVoteImpl.java | 4 +-
.../impl/ExtraInfoDescriptorImplTest.java | 20 ++-
8 files changed, 118 insertions(+), 164 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c907cc6..3ab6955 100644
--- a/CHANGELOG.md
+++ b/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
diff --git a/src/main/java/org/torproject/descriptor/impl/BridgeNetworkStatusImpl.java b/src/main/java/org/torproject/descriptor/impl/BridgeNetworkStatusImpl.java
index 88411e6..6036e50 100644
--- a/src/main/java/org/torproject/descriptor/impl/BridgeNetworkStatusImpl.java
+++ b/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()) {
diff --git a/src/main/java/org/torproject/descriptor/impl/KeyValueMap.java b/src/main/java/org/torproject/descriptor/impl/KeyValueMap.java
new file mode 100644
index 0000000..7b918dc
--- /dev/null
+++ b/src/main/java/org/torproject/descriptor/impl/KeyValueMap.java
@@ -0,0 +1,71 @@
+/* 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;
+ }
+
+}
diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
index cb4eca8..fd4b3e3 100644
--- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
+++ b/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 + "'.");
}
diff --git a/src/main/java/org/torproject/descriptor/impl/ParseHelper.java b/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
index d5ce50f..cfd3724 100644
--- a/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
+++ b/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
diff --git a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java
index 4570931..43212f3 100644
--- a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusConsensusImpl.java
+++ b/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;
diff --git a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java b/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java
index c645928..e5bd052 100644
--- a/src/main/java/org/torproject/descriptor/impl/RelayNetworkStatusVoteImpl.java
+++ b/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)
diff --git a/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java b/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java
index b6e1343..2e7bbc6 100644
--- a/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java
+++ b/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");
