commit 38221ed0973ad63e3b47effa39f297f4af0012ae Author: Karsten Loesing <karsten.loesing@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");