From 22dfc6231cce451d171a1e6204dd75e815423cad Mon Sep 17 00:00:00 2001
From: Leif Åstrand <leif@vaadin.com>
Date: Sat, 29 Aug 2015 15:53:20 +0300
Subject: Support custom Enum.toString in converter (#17301)

Change-Id: Icd3c164fb252bd048ffcd953f967a9c7acdc4514
---
 WebContent/release-notes.html                      |  2 +
 .../data/util/converter/StringToEnumConverter.java | 71 ++++++++++++++--------
 .../data/converter/StringToEnumConverterTest.java  | 53 ++++++++++++++++
 3 files changed, 101 insertions(+), 25 deletions(-)

diff --git a/WebContent/release-notes.html b/WebContent/release-notes.html
index 463f366069..c95c75e248 100644
--- a/WebContent/release-notes.html
+++ b/WebContent/release-notes.html
@@ -115,6 +115,8 @@
                 on the reasoning behind the change.</li>
             <li>Grid SelectionModels are now Extensions. This update removes all selection related variables and API from 
             GridConnector, GridState, GridServerRpc and GridClientRpc</li>
+            <li>StringToEnumConverter now explicitly supports Enum types with custom toString() implementations.
+                This may affect applications that relied on the undefined behavior in previous versions.</li>
         </ul>
         <h3 id="knownissues">Known Issues and Limitations</h3>
         <ul>
diff --git a/server/src/com/vaadin/data/util/converter/StringToEnumConverter.java b/server/src/com/vaadin/data/util/converter/StringToEnumConverter.java
index 96fdc7baa8..7b2f43f972 100644
--- a/server/src/com/vaadin/data/util/converter/StringToEnumConverter.java
+++ b/server/src/com/vaadin/data/util/converter/StringToEnumConverter.java
@@ -22,8 +22,15 @@ import java.util.Locale;
  * A converter that converts from {@link String} to an {@link Enum} and back.
  * <p>
  * Designed to provide nice human readable strings for {@link Enum} classes
- * where the constants are named SOME_UPPERCASE_WORDS. Will not necessarily work
- * correctly for other cases.
+ * conforming to one of these patterns:
+ * <ul>
+ * <li>The constants are named SOME_UPPERCASE_WORDS and there's no toString
+ * implementation.</li>
+ * <li>toString() always returns the same human readable string that is not the
+ * same as its name() value. Each constant in the enum type returns a distinct
+ * toString() value.</li>
+ * </ul>
+ * Will not necessarily work correctly for other cases.
  * </p>
  * 
  * @author Vaadin Ltd
@@ -63,26 +70,35 @@ public class StringToEnumConverter implements Converter<String, Enum> {
             locale = Locale.getDefault();
         }
 
-        // Foo -> FOO
-        // Foo bar -> FOO_BAR
-        String result = value.replace(" ", "_").toUpperCase(locale);
-        try {
-            return Enum.valueOf(enumType, result);
-        } catch (Exception ee) {
-            // There was no match. Try to compare the available values to see if
-            // the constant is using something else than all upper case
-            try {
-                EnumSet<T> set = EnumSet.allOf(enumType);
-                for (T e : set) {
-                    if (e.name().toUpperCase(locale).equals(result)) {
-                        return e;
-                    }
+        if (!enumType.isEnum()) {
+            throw new ConversionException(enumType.getName()
+                    + " is not an enum type");
+        }
+
+        // First test for the human-readable value since that's the more likely
+        // input
+        String upperCaseValue = value.toUpperCase(locale);
+        T match = null;
+        for (T e : EnumSet.allOf(enumType)) {
+            String upperCase = enumToString(e, locale).toUpperCase(locale);
+            if (upperCase.equals(upperCaseValue)) {
+                if (match != null) {
+                    throw new ConversionException("Both " + match.name()
+                            + " and " + e.name()
+                            + " are matching the input string " + value);
                 }
-            } catch (Exception e) {
+                match = e;
             }
+        }
 
-            // Fallback did not work either, re-throw original exception so
-            // user knows what went wrong
+        if (match != null) {
+            return match;
+        }
+
+        // Then fall back to using a strict match based on name()
+        try {
+            return Enum.valueOf(enumType, upperCaseValue);
+        } catch (Exception ee) {
             throw new ConversionException(ee);
         }
     }
@@ -107,12 +123,17 @@ public class StringToEnumConverter implements Converter<String, Enum> {
         }
 
         String enumString = value.toString();
-        // FOO -> Foo
-        // FOO_BAR -> Foo bar
-        // _FOO -> _foo
-        String result = enumString.substring(0, 1).toUpperCase(locale);
-        result += enumString.substring(1).toLowerCase(locale).replace('_', ' ');
-        return result;
+        if (enumString.equals(value.name())) {
+            // FOO -> Foo
+            // FOO_BAR -> Foo bar
+            // _FOO -> _foo
+            String result = enumString.substring(0, 1).toUpperCase(locale);
+            result += enumString.substring(1).toLowerCase(locale)
+                    .replace('_', ' ');
+            return result;
+        } else {
+            return enumString;
+        }
     }
 
     @Override
diff --git a/server/tests/src/com/vaadin/tests/data/converter/StringToEnumConverterTest.java b/server/tests/src/com/vaadin/tests/data/converter/StringToEnumConverterTest.java
index 59c1ed133a..6660ecc9d5 100644
--- a/server/tests/src/com/vaadin/tests/data/converter/StringToEnumConverterTest.java
+++ b/server/tests/src/com/vaadin/tests/data/converter/StringToEnumConverterTest.java
@@ -14,10 +14,36 @@ public class StringToEnumConverterTest {
         VALUE1, SOME_VALUE, FOO_BAR_BAZ, Bar, nonStandardCase, _HUGH;
     }
 
+    public static enum EnumWithCustomToString {
+        ONE, TWO, THREE;
+
+        @Override
+        public String toString() {
+            return "case " + (ordinal() + 1);
+        }
+    }
+
+    public static enum EnumWithAmbigousToString {
+        FOO, FOOBAR, FOO_BAR;
+
+        @Override
+        public String toString() {
+            return name().replaceAll("_", "");
+        }
+    }
+
     StringToEnumConverter converter = new StringToEnumConverter();
     Converter<Enum, String> reverseConverter = new ReverseConverter<Enum, String>(
             converter);
 
+    private String convertToString(Enum value) {
+        return converter.convertToPresentation(value, String.class, null);
+    }
+
+    public Enum convertToEnum(String string, Class<? extends Enum> type) {
+        return converter.convertToModel(string, type, null);
+    }
+
     @Test
     public void testEmptyStringConversion() {
         Assert.assertEquals(null,
@@ -79,4 +105,31 @@ public class StringToEnumConverterTest {
 
     }
 
+    @Test
+    public void preserveFormattingWithCustomToString() {
+        for (EnumWithCustomToString e : EnumWithCustomToString.values()) {
+            Assert.assertEquals(e.toString(), convertToString(e));
+        }
+    }
+
+    @Test
+    public void findEnumWithCustomToString() {
+        for (EnumWithCustomToString e : EnumWithCustomToString.values()) {
+            Assert.assertSame(e,
+                    convertToEnum(e.toString(), EnumWithCustomToString.class));
+            Assert.assertSame(e,
+                    convertToEnum(e.name(), EnumWithCustomToString.class));
+        }
+    }
+
+    @Test
+    public void unambigousValueInEnumWithAmbigous_succeed() {
+        Assert.assertSame(EnumWithAmbigousToString.FOO,
+                convertToEnum("foo", EnumWithAmbigousToString.class));
+    }
+
+    @Test(expected = ConversionException.class)
+    public void ambigousValue_throws() {
+        convertToEnum("foobar", EnumWithAmbigousToString.class);
+    }
 }
-- 
cgit v1.2.3