Repository: netty/netty
Issue: Add unescapeCsv method to StringUtil.
Labels: improvement

For the solution for #3237 involved adding a method named `escapeCsv` to StringUtil. We should also add the counter part of this, `unescapeCsv`, to StringUtil.


Issue comments:
Thanks for capturing this issue @danbev!

Just as #3271 added the ability to automatically combine and CSV escape values associated with the same header name; we should also add the ability to automatically unescape values returned by the accessor methods.  For example `CharSequence get(Charsequence name)` in `DefaultTextHeaders` should be unescaping the values that are returned (under some condition...which may be the same condition that activates escaping).

@danbev - Any updates on this issue?

Sorry this parenting thing was more time consuming than I expected. I don't
think I'll be of much use before I return to work in July I'm afraid.

onsdag 8 april 2015 skrev Scott Mitchell notifications@github.com:

> @danbev https://github.com/danbev - Any updates on this issue?
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/netty/netty/issues/3435#issuecomment-90806591.

@danbev - O wow I had no idea...congratulations!  Take care of your family and forget about this ;)

@danbev Congratulations! :-D

Thanks guys!

fredag 10 april 2015 skrev Trustin Lee notifications@github.com:

> @danbev https://github.com/danbev Congratulations! :-D
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/netty/netty/issues/3435#issuecomment-91419961.

Thanks @windie for taking care of this! The second part of this issue was moved to https://github.com/netty/netty/issues/4855.


Pull request review comments:
++i would be better.

It doesn't look like the `or the value unchanged` is honored like in `escapeCsv`

Doesn't really matter. Javac generates the same bytes for both `i++` and `++i` if the return value is not used.

Fixed. Now just return the value if it's not escaped.

Remove the else

nit: import static for `unescapeCsv` and `escapeCsv`

if `quoted == FALSE` we are just validating proper CSV format and not doing any unescaping, right? Does it make sense to split these two functions up?

``` java
public static void unescapeCsv(CharSequence seq) {
  boolean quoted = ...;
  if (quoted) {
    validateCsvFormat(seq);
    return seq;
  }
  StringBuilder builder = ...;
  for(/**/) {
    // unescape seq into builder
  }
  return builder.toString();
}

public static void validateCsvFormat(CharSequence seq) {
..
// if invalid throw exception
}
```

Done

There is an `escapeCsv` method in `StringUtilTest`. So I just used the static import for unescapeCsv.

- Could you extract `new IllegalArgumentException(...)` into a new private static factory method to remove the duplication?
- Exception message: `invalid escaped CSV field:`

Done

How about `newInvalidEscapedCsvFieldException`? `new` and `CSV` -> `Csv`

Done

Consider adding an argument for an index to give more context as to where the problem is.

Consider removing temp variable. It is only used 1 time.

Consider removing explicit null check ... You "dereference" on this line anyways and NPE will already be thrown. There is also only 1 argument so I don't think much context will be lost by not having "value" in the exception message.

I think it's still useful to have this check for any user-calling public methods.

done

done

okey, I will keep it.


Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/StringUtil.java b/common/src/main/java/io/netty/util/internal/StringUtil.java
index 473ba597d1f..e60c734bb00 100644
--- a/common/src/main/java/io/netty/util/internal/StringUtil.java
+++ b/common/src/main/java/io/netty/util/internal/StringUtil.java
@@ -387,6 +387,67 @@ public static CharSequence escapeCsv(CharSequence value) {
                 escaped.append(DOUBLE_QUOTE) : value;
     }
 
+    /**
+     * Unescapes the specified escaped CSV field, if necessary according to
+     * <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>.
+     *
+     * @param value The escaped CSV field which will be unescaped according to
+     *              <a href="https://tools.ietf.org/html/rfc4180#section-2">RFC-4180</a>
+     * @return {@link CharSequence} the unescaped value if necessary, or the value unchanged
+     */
+    public static CharSequence unescapeCsv(CharSequence value) {
+        int length = checkNotNull(value, "value").length();
+        if (length == 0) {
+            return value;
+        }
+        int last = length - 1;
+        boolean quoted = isDoubleQuote(value.charAt(0)) && isDoubleQuote(value.charAt(last)) && length != 1;
+        if (!quoted) {
+            validateCsvFormat(value);
+            return value;
+        }
+        StringBuilder unescaped = InternalThreadLocalMap.get().stringBuilder();
+        for (int i = 1; i < last; i++) {
+            char current = value.charAt(i);
+            if (current == DOUBLE_QUOTE) {
+                if (isDoubleQuote(value.charAt(i + 1)) && (i + 1) != last) {
+                    // Followed by a double-quote but not the last character
+                    // Just skip the next double-quote
+                    i++;
+                } else {
+                    // Not followed by a double-quote or the following double-quote is the last character
+                    throw newInvalidEscapedCsvFieldException(value, i);
+                }
+            }
+            unescaped.append(current);
+        }
+        return unescaped.toString();
+    }
+
+    /**
+     * Validate if {@code value} is a valid csv field without double-quotes.
+     *
+     * @throws IllegalArgumentException if {@code value} needs to be encoded with double-quotes.
+     */
+    private static void validateCsvFormat(CharSequence value) {
+        int length = value.length();
+        for (int i = 0; i < length; i++) {
+            switch (value.charAt(i)) {
+                case DOUBLE_QUOTE:
+                case LINE_FEED:
+                case CARRIAGE_RETURN:
+                case COMMA:
+                    // If value contains any special character, it should be enclosed with double-quotes
+                    throw newInvalidEscapedCsvFieldException(value, i);
+                default:
+            }
+        }
+    }
+
+    private static IllegalArgumentException newInvalidEscapedCsvFieldException(CharSequence value, int index) {
+        return new IllegalArgumentException("invalid escaped CSV field: " + value + " index: " + index);
+    }
+
     /**
      * Get the length of a string, {@code null} input is considered {@code 0} length.
      */


Test patch:
diff --git a/common/src/test/java/io/netty/util/internal/StringUtilTest.java b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
index 66229dacb17..3084094f00a 100644
--- a/common/src/test/java/io/netty/util/internal/StringUtilTest.java
+++ b/common/src/test/java/io/netty/util/internal/StringUtilTest.java
@@ -321,6 +321,61 @@ private static void escapeCsv(CharSequence value, CharSequence expected) {
         }
     }
 
+    @Test
+    public void testUnescapeCsv() {
+        assertEquals("", unescapeCsv(""));
+        assertEquals("\"", unescapeCsv("\"\"\"\""));
+        assertEquals("\"\"", unescapeCsv("\"\"\"\"\"\""));
+        assertEquals("\"\"\"", unescapeCsv("\"\"\"\"\"\"\"\""));
+        assertEquals("\"netty\"", unescapeCsv("\"\"\"netty\"\"\""));
+        assertEquals("netty", unescapeCsv("netty"));
+        assertEquals("netty", unescapeCsv("\"netty\""));
+        assertEquals("\r", unescapeCsv("\"\r\""));
+        assertEquals("\n", unescapeCsv("\"\n\""));
+        assertEquals("hello,netty", unescapeCsv("\"hello,netty\""));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvWithSingleQuote() {
+        unescapeCsv("\"");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvWithOddQuote() {
+        unescapeCsv("\"\"\"");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvWithCRAndWithoutQuote() {
+        unescapeCsv("\r");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvWithLFAndWithoutQuote() {
+        unescapeCsv("\n");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void unescapeCsvWithCommaAndWithoutQuote() {
+        unescapeCsv(",");
+    }
+
+    @Test
+    public void escapeCsvAndUnEscapeCsv() {
+        assertEscapeCsvAndUnEscapeCsv("");
+        assertEscapeCsvAndUnEscapeCsv("netty");
+        assertEscapeCsvAndUnEscapeCsv("hello,netty");
+        assertEscapeCsvAndUnEscapeCsv("hello,\"netty\"");
+        assertEscapeCsvAndUnEscapeCsv("\"");
+        assertEscapeCsvAndUnEscapeCsv(",");
+        assertEscapeCsvAndUnEscapeCsv("\r");
+        assertEscapeCsvAndUnEscapeCsv("\n");
+    }
+
+    private void assertEscapeCsvAndUnEscapeCsv(String value) {
+        assertEquals(value, unescapeCsv(StringUtil.escapeCsv(value)));
+    }
+
     @Test
     public void testSimpleClassName() throws Exception {
         testSimpleClassName(String.class);