From: Simon Brandhof <simon.brandhof@sonarsource.com>
Date: Tue, 21 Oct 2014 09:20:50 +0000 (+0200)
Subject: SONAR-5768 API: org.sonar.api.utils.KeyValueFormat does not support escaping of values
X-Git-Tag: 4.5.1-RC1~13
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=20f2ddd5f6033d74c9a9de7b865ee82c62ee7c72;p=sonarqube.git

SONAR-5768 API: org.sonar.api.utils.KeyValueFormat does not support escaping of values
---

diff --git a/sonar-plugin-api/pom.xml b/sonar-plugin-api/pom.xml
index 6cdbdbde19f..fce4cdb0856 100644
--- a/sonar-plugin-api/pom.xml
+++ b/sonar-plugin-api/pom.xml
@@ -130,6 +130,11 @@
     </dependency>
 
     <!-- unit tests -->
+    <dependency>
+      <groupId>org.codehaus.sonar</groupId>
+      <artifactId>sonar-testing-harness</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>ch.qos.logback</groupId>
       <artifactId>logback-classic</artifactId>
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValue.java b/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValue.java
deleted file mode 100644
index 9a8045d1bb4..00000000000
--- a/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValue.java
+++ /dev/null
@@ -1,74 +0,0 @@
-/*
- * SonarQube, open source software quality management tool.
- * Copyright (C) 2008-2014 SonarSource
- * mailto:contact AT sonarsource DOT com
- *
- * SonarQube is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 3 of the License, or (at your option) any later version.
- *
- * SonarQube is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public License
- * along with this program; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- */
-package org.sonar.api.utils;
-
-/**
- * A utility class to store a key / value couple of generic types
- *
- * @since 1.10
- * @deprecated in 2.7. Used only in deprecated methods of {@link org.sonar.api.utils.KeyValueFormat}
- */
-@Deprecated
-public class KeyValue<K, V> {
-
-  private K key;
-  private V value;
-
-  /**
-   * Creates a key / value object
-   */
-  public KeyValue(K key, V value) {
-    super();
-    this.key = key;
-    this.value = value;
-  }
-
-  /**
-   * @return the key of the couple
-   */
-  public K getKey() {
-    return key;
-  }
-
-  /**
-   * Sets the key of the couple
-   *
-   * @param key the key
-   */
-  public void setKey(K key) {
-    this.key = key;
-  }
-
-  /**
-   *
-   * @return the value of the couple
-   */
-  public V getValue() {
-    return value;
-  }
-
-  /**
-   * Sets the value of the couple
-   */
-  public void setValue(V value) {
-    this.value = value;
-  }
-
-}
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValueFormat.java b/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValueFormat.java
index 0a90ae1c53f..37c16c9c8f7 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValueFormat.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/utils/KeyValueFormat.java
@@ -19,15 +19,14 @@
  */
 package org.sonar.api.utils;
 
-import com.google.common.collect.LinkedHashMultiset;
 import com.google.common.collect.Maps;
-import com.google.common.collect.Multimap;
 import com.google.common.collect.Multiset;
 import org.apache.commons.collections.Bag;
 import org.apache.commons.lang.StringUtils;
 import org.apache.commons.lang.math.NumberUtils;
 import org.sonar.api.rules.RulePriority;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
 import java.text.ParseException;
@@ -38,15 +37,33 @@ import java.util.Map;
 import java.util.Set;
 
 /**
- * <p>Formats and parses key/value pairs with the string representation : "key1=value1;key2=value2". Conversion
- * of fields is supported and can be extended.</p>
+ * <p>Formats and parses key/value pairs with the text representation : "key1=value1;key2=value2". Field typing
+ * is supported, to make conversion from/to primitive types easier for example.
+ * <p/>
+ * Since version 4.5.1, text keys and values are escaped if they contain the separator characters '=' or ';'.
+ * <p/>
+ * <b>Parsing examples</b>
+ * <pre>
+ *   Map&lt;String,String&gt; mapOfStrings = KeyValueFormat.parse("hello=world;foo=bar");
+ *   Map&lt;String,Integer&gt; mapOfStringInts = KeyValueFormat.parseStringInt("one=1;two=2");
+ *   Map&lt;Integer,String&gt; mapOfIntStrings = KeyValueFormat.parseIntString("1=one;2=two");
+ *   Map&lt;String,Date&gt; mapOfStringDates = KeyValueFormat.parseStringDate("d1=2014-01-14;d2=2015-07-28");
  *
- * <p>This format can easily be parsed with Ruby code: <code>hash=Hash[*(my_string.split(';').map { |elt| elt.split('=') }.flatten)]</code></p>
+ *   // custom conversion
+ *   Map&lt;String,MyClass&gt; mapOfStringMyClass = KeyValueFormat.parse("foo=xxx;bar=yyy",
+ *     KeyValueFormat.newStringConverter(), new MyClassConverter());
+ * </pre>
+ * <p/>
+ * <b>Formatting examples</b>
+ * <pre>
+ *   String output = KeyValueFormat.format(map);
  *
+ *   Map&lt;Integer,String&gt; mapIntString;
+ *   KeyValueFormat.formatIntString(mapIntString);
+ * </pre>
  * @since 1.10
  */
 public final class KeyValueFormat {
-
   public static final String PAIR_SEPARATOR = ";";
   public static final String FIELD_SEPARATOR = "=";
 
@@ -54,58 +71,100 @@ public final class KeyValueFormat {
     // only static methods
   }
 
-  public abstract static class Converter<T> {
-    abstract String format(T type);
-
-    abstract T parse(String s);
+  static class FieldParser {
+    private static final char DOUBLE_QUOTE = '"';
+    private final String csv;
+    private int position = 0;
 
-    boolean requiresEscaping() {
-      return false;
+    FieldParser(String csv) {
+      this.csv = csv;
     }
-  }
-
-  public static final class StringConverter extends Converter<String> {
-    private static final StringConverter INSTANCE = new StringConverter();
 
-    private StringConverter() {
+    @CheckForNull
+    String nextKey() {
+      return next('=');
     }
 
-    @Override
-    String format(String s) {
-      return s;
+    @CheckForNull
+    String nextVal() {
+      return next(';');
     }
 
-    @Override
-    String parse(String s) {
-      return s;
+    @CheckForNull
+    private String next(char separator) {
+      if (position >= csv.length()) {
+        return null;
+      }
+      StringBuilder result = new StringBuilder();
+      boolean escaped = false;
+      char firstChar = csv.charAt(position);
+      char previous = (char) -1;
+      // check if value is escaped by analyzing first character
+      if (firstChar == DOUBLE_QUOTE) {
+        escaped = true;
+        position++;
+        previous = firstChar;
+      }
+
+      boolean end = false;
+      while (position < csv.length() && !end) {
+        char c = csv.charAt(position);
+        if (c == separator && !escaped) {
+          end = true;
+          position++;
+        } else if (c == '\\' && escaped && position < csv.length() + 1 && csv.charAt(position + 1) == DOUBLE_QUOTE) {
+          // on a backslash that escapes double-quotes -> keep double-quotes and jump after
+          previous = DOUBLE_QUOTE;
+          result.append(previous);
+          position += 2;
+        } else if (c == '"' && escaped && previous != '\\') {
+          // on unescaped double-quotes -> end of escaping.
+          // assume that next character is a separator (= or ;). This could be
+          // improved to enforce check.
+          end = true;
+          position += 2;
+        } else {
+          result.append(c);
+          previous = c;
+          position++;
+        }
+      }
+      return result.toString();
     }
+  }
 
-    @Override
-    boolean requiresEscaping() {
-      return true;
+  public abstract static class Converter<T> {
+    abstract String format(T type);
+
+    @CheckForNull
+    abstract T parse(String s);
+
+    String escape(String s) {
+      if (s.contains(FIELD_SEPARATOR) || s.contains(PAIR_SEPARATOR)) {
+        return new StringBuilder()
+          .append(FieldParser.DOUBLE_QUOTE)
+          .append(s.replace("\"", "\\\""))
+          .append(FieldParser.DOUBLE_QUOTE).toString();
+      }
+      return s;
     }
   }
 
-  public static final class UnescapedStringConverter extends Converter<String> {
-    private static final UnescapedStringConverter INSTANCE = new UnescapedStringConverter();
+  public static final class StringConverter extends Converter<String> {
+    private static final StringConverter INSTANCE = new StringConverter();
 
-    private UnescapedStringConverter() {
+    private StringConverter() {
     }
 
     @Override
     String format(String s) {
-      return s;
+      return escape(s);
     }
 
     @Override
     String parse(String s) {
       return s;
     }
-
-    @Override
-    boolean requiresEscaping() {
-      return false;
-    }
   }
 
   public static StringConverter newStringConverter() {
@@ -120,17 +179,12 @@ public final class KeyValueFormat {
 
     @Override
     String format(Object o) {
-      return o.toString();
+      return escape(o.toString());
     }
 
     @Override
     String parse(String s) {
-      throw new IllegalStateException("Can not parse with ToStringConverter: " + s);
-    }
-
-    @Override
-    boolean requiresEscaping() {
-      return true;
+      throw new UnsupportedOperationException("Can not parse with ToStringConverter: " + s);
     }
   }
 
@@ -204,14 +258,6 @@ public final class KeyValueFormat {
   public static class DateConverter extends Converter<Date> {
     private SimpleDateFormat dateFormat;
 
-    /**
-     * @deprecated in version 2.13. Replaced by {@link org.sonar.api.utils.KeyValueFormat#newDateConverter()}
-     */
-    @Deprecated
-    public DateConverter() {
-      this(DateUtils.DATE_FORMAT);
-    }
-
     private DateConverter(String format) {
       this.dateFormat = new SimpleDateFormat(format);
     }
@@ -226,32 +272,39 @@ public final class KeyValueFormat {
       try {
         return StringUtils.isBlank(s) ? null : dateFormat.parse(s);
       } catch (ParseException e) {
-        throw new SonarException("Not a date with format: " + dateFormat.toPattern(), e);
+        throw new IllegalArgumentException("Not a date with format: " + dateFormat.toPattern(), e);
       }
     }
   }
 
   public static DateConverter newDateConverter() {
-    return new DateConverter(DateUtils.DATE_FORMAT);
+    return newDateConverter(DateUtils.DATE_FORMAT);
   }
 
   public static DateConverter newDateTimeConverter() {
-    return new DateConverter(DateUtils.DATETIME_FORMAT);
+    return newDateConverter(DateUtils.DATETIME_FORMAT);
   }
 
   public static DateConverter newDateConverter(String format) {
     return new DateConverter(format);
   }
 
-  public static <K, V> Map<K, V> parse(@Nullable String data, Converter<K> keyConverter, Converter<V> valueConverter) {
+  /**
+   * If input is null, then an empty map is returned.
+   */
+  public static <K, V> Map<K, V> parse(@Nullable String input, Converter<K> keyConverter, Converter<V> valueConverter) {
     Map<K, V> map = Maps.newLinkedHashMap();
-    if (data != null) {
-      String[] pairs = StringUtils.split(data, PAIR_SEPARATOR);
-      for (String pair : pairs) {
-        int indexOfEqualSign = pair.indexOf(FIELD_SEPARATOR);
-        String key = pair.substring(0, indexOfEqualSign);
-        String value = pair.substring(indexOfEqualSign + 1);
-        map.put(keyConverter.parse(key), valueConverter.parse(value));
+    if (input != null) {
+      FieldParser reader = new FieldParser(input);
+      boolean end = false;
+      while (!end) {
+        String key = reader.nextKey();
+        if (key == null) {
+          end = true;
+        } else {
+          String val = StringUtils.defaultString(reader.nextVal(), "");
+          map.put(keyConverter.parse(key), valueConverter.parse(val));
+        }
       }
     }
     return map;
@@ -310,43 +363,6 @@ public final class KeyValueFormat {
     return parse(data, newIntegerConverter(), newDateTimeConverter());
   }
 
-  /**
-   * Value of pairs is the occurrences of the same single key. A multiset is sometimes called a bag.
-   * For example parsing "foo=2;bar=1" creates a multiset with 3 elements : foo, foo and bar.
-   */
-  /**
-   * @since 2.7
-   */
-  public static <K> Multiset<K> parseMultiset(@Nullable String data, Converter<K> keyConverter) {
-    // to keep the same order
-    Multiset<K> multiset = LinkedHashMultiset.create();
-    if (data != null) {
-      String[] pairs = StringUtils.split(data, PAIR_SEPARATOR);
-      for (String pair : pairs) {
-        String[] keyValue = StringUtils.split(pair, FIELD_SEPARATOR);
-        String key = keyValue[0];
-        String value = keyValue.length == 2 ? keyValue[1] : "0";
-        multiset.add(keyConverter.parse(key), new IntegerConverter().parse(value));
-      }
-    }
-    return multiset;
-  }
-
-
-  /**
-   * @since 2.7
-   */
-  public static Multiset<Integer> parseIntegerMultiset(@Nullable String data) {
-    return parseMultiset(data, newIntegerConverter());
-  }
-
-  /**
-   * @since 2.7
-   */
-  public static Multiset<String> parseMultiset(@Nullable String data) {
-    return parseMultiset(data, newStringConverter());
-  }
-
   private static <K, V> String formatEntries(Collection<Map.Entry<K, V>> entries, Converter<K> keyConverter, Converter<V> valueConverter) {
     StringBuilder sb = new StringBuilder();
     boolean first = true;
@@ -373,13 +389,12 @@ public final class KeyValueFormat {
       }
       sb.append(keyConverter.format(entry.getElement()));
       sb.append(FIELD_SEPARATOR);
-      sb.append(new IntegerConverter().format(entry.getCount()));
+      sb.append(newIntegerConverter().format(entry.getCount()));
       first = false;
     }
     return sb.toString();
   }
 
-
   /**
    * @since 2.7
    */
@@ -429,15 +444,6 @@ public final class KeyValueFormat {
     return format(map, newStringConverter(), newIntegerConverter());
   }
 
-  /**
-   * Limitation: there's currently no methods to parse into Multimap.
-   *
-   * @since 2.7
-   */
-  public static <K, V> String format(Multimap<K, V> map, Converter<K> keyConverter, Converter<V> valueConverter) {
-    return formatEntries(map.entries(), keyConverter, valueConverter);
-  }
-
   /**
    * @since 2.7
    */
@@ -446,10 +452,9 @@ public final class KeyValueFormat {
   }
 
   public static String format(Multiset multiset) {
-    return formatEntries(multiset.entrySet(), newToStringConverter());
+    return format(multiset, newToStringConverter());
   }
 
-
   /**
    * @since 1.11
    * @deprecated use Multiset from google collections instead of commons-collections bags
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/utils/DeprecatedKeyValueFormatTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/utils/DeprecatedKeyValueFormatTest.java
deleted file mode 100644
index 6d644688e47..00000000000
--- a/sonar-plugin-api/src/test/java/org/sonar/api/utils/DeprecatedKeyValueFormatTest.java
+++ /dev/null
@@ -1,79 +0,0 @@
-/*
- * SonarQube, open source software quality management tool.
- * Copyright (C) 2008-2014 SonarSource
- * mailto:contact AT sonarsource DOT com
- *
- * SonarQube is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 3 of the License, or (at your option) any later version.
- *
- * SonarQube is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public License
- * along with this program; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- */
-package org.sonar.api.utils;
-
-import com.google.common.collect.Multiset;
-import com.google.common.collect.TreeMultiset;
-import org.apache.commons.collections.bag.TreeBag;
-import org.junit.Test;
-
-import java.util.Map;
-import java.util.TreeMap;
-
-import static junit.framework.Assert.assertEquals;
-import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
-
-public class DeprecatedKeyValueFormatTest {
-
-  @Test
-  public void formatMap() {
-    Map<String, String> map = new TreeMap<String, String>();
-    map.put("hello", "world");
-    map.put("key1", "val1");
-    map.put("key2", "");
-    map.put("key3", "val3");
-    assertThat(KeyValueFormat.format(map), is("hello=world;key1=val1;key2=;key3=val3"));
-  }
-
-  @Test
-  public void formatBag() {
-    TreeBag bag = new TreeBag();
-    bag.add("hello", 1);
-    bag.add("key", 3);
-    assertThat(KeyValueFormat.format(bag, 0), is("hello=1;key=3"));
-  }
-
-  @Test
-  public void formatBagWithVariationHack() {
-    TreeBag bag = new TreeBag();
-    bag.add("hello", 1);
-    bag.add("key", 3);
-    assertThat(KeyValueFormat.format(bag, -1), is("hello=0;key=2"));
-  }
-
-  @Test
-  public void formatMultiset() {
-    Multiset<String> set = TreeMultiset.create();
-    set.add("hello", 1);
-    set.add("key", 3);
-    assertThat(KeyValueFormat.format(set), is("hello=1;key=3"));
-  }
-
-  @Test
-  public void parse() {
-    Map<String, String> map = KeyValueFormat.parse("hello=world;key1=val1;key2=;key3=val3");
-    assertThat(map.size(), is(4));
-    assertEquals("world", map.get("hello"));
-    assertEquals("val1", map.get("key1"));
-    assertEquals("", map.get("key2"));
-    assertEquals("val3", map.get("key3"));
-  }
-}
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/utils/KeyValueFormatTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/utils/KeyValueFormatTest.java
index 2f2cde7b5e7..0d8e537acc8 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/utils/KeyValueFormatTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/utils/KeyValueFormatTest.java
@@ -19,11 +19,17 @@
  */
 package org.sonar.api.utils;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.LinkedHashMultiset;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multiset;
+import com.google.common.collect.TreeMultiset;
+import org.apache.commons.collections.bag.TreeBag;
+import org.fest.assertions.MapAssert;
+import org.junit.Assert;
 import org.junit.Test;
 import org.sonar.api.rules.RulePriority;
+import org.sonar.test.TestUtils;
 
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
@@ -31,16 +37,80 @@ import java.util.Date;
 import java.util.Map;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.hamcrest.Matchers.is;
 
 public class KeyValueFormatTest {
 
   @Test
-  public void shouldFormatMapOfStrings() {
+  public void test_parser() throws Exception {
+    KeyValueFormat.FieldParser reader = new KeyValueFormat.FieldParser("abc=def;ghi=jkl");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("def");
+    assertThat(reader.nextKey()).isEqualTo("ghi");
+    assertThat(reader.nextVal()).isEqualTo("jkl");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=1;ghi=2");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("1");
+    assertThat(reader.nextKey()).isEqualTo("ghi");
+    assertThat(reader.nextVal()).isEqualTo("2");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=;ghi=jkl");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("");
+    assertThat(reader.nextKey()).isEqualTo("ghi");
+    assertThat(reader.nextVal()).isEqualTo("jkl");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=def");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("def");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=\"def\";ghi=\"jkl\"");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("def");
+    assertThat(reader.nextKey()).isEqualTo("ghi");
+    assertThat(reader.nextVal()).isEqualTo("jkl");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("\"abc\"=\"def\";\"ghi\"=\"jkl\"");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("def");
+    assertThat(reader.nextKey()).isEqualTo("ghi");
+    assertThat(reader.nextVal()).isEqualTo("jkl");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=\"def\\\"ghi\"");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("def\"ghi");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("");
+    assertThat(reader.nextKey()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=;def=");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("");
+    assertThat(reader.nextKey()).isEqualTo("def");
+    assertThat(reader.nextVal()).isNull();
+
+    reader = new KeyValueFormat.FieldParser("abc=\"1=2;3\";def=\"4;5=6\"");
+    assertThat(reader.nextKey()).isEqualTo("abc");
+    assertThat(reader.nextVal()).isEqualTo("1=2;3");
+    assertThat(reader.nextKey()).isEqualTo("def");
+    assertThat(reader.nextVal()).isEqualTo("4;5=6");
+    assertThat(reader.nextKey()).isNull();
+  }
+
+  @Test
+  public void keep_order_of_linked_map() {
     Map<String, String> map = Maps.newLinkedHashMap();
     map.put("lucky", "luke");
     map.put("aste", "rix");
     String s = KeyValueFormat.format(map);
-    // same order
     assertThat(s).isEqualTo("lucky=luke;aste=rix");
   }
 
@@ -98,28 +168,51 @@ public class KeyValueFormatTest {
   }
 
   @Test
-  public void shouldParseBlank() {
+  public void helper_parse_methods() throws Exception {
+    assertThat(KeyValueFormat.parseIntDate("1=2014-01-15")).hasSize(1);
+    assertThat(KeyValueFormat.parseIntDateTime("1=2014-01-15T15:50:45+0100")).hasSize(1);
+    assertThat(KeyValueFormat.parseIntDouble("1=3.14")).hasSize(1);
+    assertThat(KeyValueFormat.parseIntInt("1=10")).hasSize(1).includes(MapAssert.entry(1, 10));
+    assertThat(KeyValueFormat.parseIntString("1=one")).hasSize(1).includes(MapAssert.entry(1, "one"));
+    assertThat(KeyValueFormat.parseIntString("1=\"escaped\"")).hasSize(1).includes(MapAssert.entry(1, "escaped"));
+    assertThat(KeyValueFormat.parseStringInt("one=1")).hasSize(1).includes(MapAssert.entry("one", 1));
+    assertThat(KeyValueFormat.parseStringDouble("pi=3.14")).hasSize(1).includes(MapAssert.entry("pi", 3.14));
+  }
+
+  @Test
+  public void helper_format_methods() throws Exception {
+    assertThat(KeyValueFormat.formatIntDateTime(ImmutableMap.of(1, new Date()))).startsWith("1=");
+    assertThat(KeyValueFormat.formatIntDate(ImmutableMap.of(1, new Date()))).startsWith("1=");
+    assertThat(KeyValueFormat.formatIntDouble(ImmutableMap.of(1, 3.14))).startsWith("1=");
+    assertThat(KeyValueFormat.formatIntString(ImmutableMap.of(1, "one"))).isEqualTo("1=one");
+    assertThat(KeyValueFormat.formatStringInt(ImmutableMap.of("one", 1))).isEqualTo("one=1");
+  }
+
+  @Test
+  public void parse_blank() {
     Map<String, String> map = KeyValueFormat.parse("");
-    assertThat(map.size()).isEqualTo(0);
+    assertThat(map).isEmpty();
   }
 
   @Test
-  public void shouldParseNull() {
+  public void parse_null() {
     Map<String, String> map = KeyValueFormat.parse(null);
-    assertThat(map.size()).isEqualTo(0);
+    assertThat(map).isEmpty();
   }
 
   @Test
-  public void shouldParseEmptyFields() {
+  public void parse_empty_values() {
     Map<Integer, Double> map = KeyValueFormat.parseIntDouble("4=4.2;2=;6=6.68");
     assertThat(map.size()).isEqualTo(3);
     assertThat(map.get(4)).isEqualTo(4.2);
+    // key is present but value is null
+    assertThat(map.containsKey(2)).isTrue();
     assertThat(map.get(2)).isNull();
     assertThat(map.get(6)).isEqualTo(6.68);
   }
 
   @Test
-  public void shouldConvertPriority() {
+  public void convert_deprecated_priority() {
     assertThat(KeyValueFormat.newPriorityConverter().format(RulePriority.BLOCKER)).isEqualTo("BLOCKER");
     assertThat(KeyValueFormat.newPriorityConverter().format(null)).isEqualTo("");
 
@@ -128,7 +221,7 @@ public class KeyValueFormatTest {
   }
 
   @Test
-  public void shouldFormatMultiset() {
+  public void format_multiset() {
     Multiset<String> set = LinkedHashMultiset.create();
     set.add("foo");
     set.add("foo");
@@ -137,19 +230,48 @@ public class KeyValueFormatTest {
   }
 
   @Test
-  public void shouldParseMultiset() {
-    Multiset<String> multiset = KeyValueFormat.parseMultiset("foo=2;bar=1;none=");
-    assertThat(multiset.count("foo")).isEqualTo(2);
-    assertThat(multiset.count("bar")).isEqualTo(1);
-    assertThat(multiset.count("none")).isEqualTo(0);
-    assertThat(multiset.contains("none")).isFalse();
+  public void escape_strings() throws Exception {
+    Map<String, String> input = Maps.newLinkedHashMap();
+    input.put("foo", "a=b=c");
+    input.put("bar", "a;b;c");
+    input.put("baz", "double\"quote");
+    String csv = KeyValueFormat.format(input);
+    assertThat(csv).isEqualTo("foo=\"a=b=c\";bar=\"a;b;c\";baz=double\"quote");
+
+    Map<String, String> output = KeyValueFormat.parse(csv);
+    assertThat(output.get("foo")).isEqualTo("a=b=c");
+    assertThat(output.get("bar")).isEqualTo("a;b;c");
+    assertThat(output.get("baz")).isEqualTo("double\"quote");
+  }
+
+  @Test
+  public void not_instantiable() throws Exception {
+    // only static methods. Bad pattern, should be improved.
+    TestUtils.hasOnlyPrivateConstructors(KeyValueFormat.class);
   }
 
   @Test
-  public void shouldKeepOrderWhenParsingMultiset() {
-    Multiset<String> multiset = KeyValueFormat.parseMultiset("foo=2;bar=1");
+  public void formatBag() {
+    TreeBag bag = new TreeBag();
+    bag.add("hello", 1);
+    bag.add("key", 3);
+    Assert.assertThat(KeyValueFormat.format(bag, 0), is("hello=1;key=3"));
+  }
 
-    // first one is foo
-    assertThat(multiset.iterator().next()).isEqualTo("foo");
+  @Test
+  public void formatBagWithVariationHack() {
+    TreeBag bag = new TreeBag();
+    bag.add("hello", 1);
+    bag.add("key", 3);
+    Assert.assertThat(KeyValueFormat.format(bag, -1), is("hello=0;key=2"));
   }
+
+  @Test
+  public void formatMultiset() {
+    Multiset<String> set = TreeMultiset.create();
+    set.add("hello", 1);
+    set.add("key", 3);
+    Assert.assertThat(KeyValueFormat.format(set), is("hello=1;key=3"));
+  }
+
 }