QPID-5721: Improve exception message used when String operand cannot be interpreted as JSON

* Also made AVC consistently return unmodifiable when converting from a JSON string

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1602078 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 224 insertions, 44 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
index d6940655a3..b7b56db15c 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AttributeValueConverter.java
@@ -196,16 +196,7 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
-                ObjectMapper objectMapper = new ObjectMapper();
-                try
-                {
-                    return objectMapper.readValue(interpolated, List.class);
-                }
-                catch (IOException e)
-                {
-                    throw new IllegalArgumentException("Cannot convert String " + interpolated + " to a List");
-                }
+                return Collections.unmodifiableList(convertFromJson((String) value, object, List.class));
             }
             else if(value == null)
             {
@@ -217,6 +208,7 @@ abstract class AttributeValueConverter<T>
             }
         }
     };
+
     static final AttributeValueConverter<Set> SET_CONVERTER = new AttributeValueConverter<Set>()
     {
         @Override
@@ -233,16 +225,7 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
-                ObjectMapper objectMapper = new ObjectMapper();
-                try
-                {
-                    return objectMapper.readValue(interpolated, Set.class);
-                }
-                catch (IOException e)
-                {
-                    throw new IllegalArgumentException("Cannot convert String " + interpolated + " to a List");
-                }
+                return Collections.unmodifiableSet(convertFromJson((String) value, object, Set.class));
             }
             else if(value == null)
             {
@@ -250,7 +233,7 @@ abstract class AttributeValueConverter<T>
             }
             else
             {
-                throw new IllegalArgumentException("Cannot convert type " + value.getClass() + " to a List");
+                throw new IllegalArgumentException("Cannot convert type " + value.getClass() + " to a Set");
             }
         }
     };
@@ -270,16 +253,7 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
-                ObjectMapper objectMapper = new ObjectMapper();
-                try
-                {
-                    return objectMapper.readValue(interpolated, List.class);
-                }
-                catch (IOException e)
-                {
-                    throw new IllegalArgumentException("Cannot convert String " + interpolated + " to a Collection");
-                }
+                return Collections.unmodifiableCollection(convertFromJson((String) value, object, Collection.class));
             }
             else if(value == null)
             {
@@ -287,7 +261,7 @@ abstract class AttributeValueConverter<T>
             }
             else
             {
-                throw new IllegalArgumentException("Cannot convert type " + value.getClass() + " to a List");
+                throw new IllegalArgumentException("Cannot convert type " + value.getClass() + " to a Collection");
             }
         }
     };
@@ -316,24 +290,34 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
-                ObjectMapper objectMapper = new ObjectMapper();
-                try
-                {
-                    return objectMapper.readValue(interpolated, Map.class);
-                }
-                catch (IOException e)
-                {
-                    throw new IllegalArgumentException("Cannot convert String " + interpolated + " to a Map");
-                }
+                return Collections.unmodifiableMap(convertFromJson((String) value, object, Map.class));
             }
             else
             {
                 throw new IllegalArgumentException("Cannot convert type " + value.getClass() + " to a Map");
             }
         }
+
     };
 
+    private static <T> T convertFromJson(final String value, final ConfiguredObject object, final Class<T> valueType)
+    {
+        String interpolated = AbstractConfiguredObject.interpolate(object, value);
+        ObjectMapper objectMapper = new ObjectMapper();
+        try
+        {
+            return objectMapper.readValue(interpolated, valueType);
+        }
+        catch (IOException e)
+        {
+            throw new IllegalArgumentException("Cannot convert String '"
+                  + value + "'"
+                  + (value.equals(interpolated)
+                               ? "" : (" (interpolated to '" + interpolated + "')"))
+                                       + " to a " + valueType.getSimpleName());
+        }
+    }
+
     static <X> AttributeValueConverter<X> getConverter(final Class<X> type, final Type returnType)
     {
         if(type == String.class)
@@ -408,7 +392,7 @@ abstract class AttributeValueConverter<T>
         {
             return (AttributeValueConverter<X>) new ConfiguredObjectConverter(type);
         }
-        throw new IllegalArgumentException("Cannot create attributes of type " + type.getName());
+        throw new IllegalArgumentException("Cannot create attribute converter of type " + type.getName());
     }
 
     abstract T convert(Object value, final ConfiguredObject object);
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
index 7382a20022..e147abd170 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AbstractConfiguredObjectTest.java
@@ -174,7 +174,7 @@ public class AbstractConfiguredObjectTest extends TestCase
 
         Map<String, Object> attributes = new HashMap<>();
         attributes.put(ConfiguredObject.NAME, objectName);
-        attributes.put("context", Collections.singletonMap("myReplacement", "myValue"));
+        attributes.put(ConfiguredObject.CONTEXT, Collections.singletonMap("myReplacement", "myValue"));
         attributes.put(TestRootCategory.STRING_VALUE, contextToken);
 
         TestRootCategory object1 = _model.getObjectFactory().create(TestRootCategory.class,
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
new file mode 100644
index 0000000000..8ddcb179cc
--- /dev/null
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/AttributeValueConverterTest.java
@@ -0,0 +1,196 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.model;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyMap;
+import static java.util.Collections.singletonMap;
+import static java.util.Collections.unmodifiableSet;
+import static java.util.Collections.unmodifiableList;
+
+import static org.apache.qpid.server.model.AttributeValueConverter.getConverter;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import junit.framework.TestCase;
+
+import org.apache.qpid.server.model.testmodel.TestModel;
+import org.apache.qpid.server.model.testmodel.TestRootCategory;
+
+public class AttributeValueConverterTest extends TestCase
+{
+    private final ConfiguredObjectFactory _objectFactory = TestModel.getInstance().getObjectFactory();
+    private final Map<String, Object> _attributes = new HashMap<>();
+    private final Map<String, String> _context = new HashMap<>();
+
+    @Override
+    protected void setUp() throws Exception
+    {
+        super.setUp();
+
+        _attributes.put(ConfiguredObject.NAME, "objectName");
+        _attributes.put(ConfiguredObject.CONTEXT, _context);
+    }
+
+    public void testMapConverter()
+    {
+        _context.put("simpleMap", "{\"a\" : \"b\"}");
+        _context.put("mapWithInterpolatedContents", "{\"${mykey}\" : \"b\"}");
+        _context.put("mykey", "mykey1");
+
+        ConfiguredObject object = _objectFactory.create(TestRootCategory.class, _attributes);
+
+        AttributeValueConverter<Map> mapConverter = getConverter(Map.class, Map.class);
+
+        Map<String, String> nullMap = mapConverter.convert(null, object);
+        assertNull(nullMap);
+
+        Map<String, String> emptyMap = mapConverter.convert("{ }", object);
+        assertEquals(emptyMap(), emptyMap);
+
+        Map<String, String> map = mapConverter.convert("{\"a\" : \"b\"}", object);
+        assertEquals(singletonMap("a", "b"), map);
+
+        Map<String, String> mapFromInterpolatedVar = mapConverter.convert("${simpleMap}", object);
+        assertEquals(singletonMap("a", "b"), mapFromInterpolatedVar);
+
+        Map<String, String> mapFromInterpolatedVarWithInterpolatedContents =
+                mapConverter.convert("${mapWithInterpolatedContents}", object);
+        assertEquals(singletonMap("mykey1", "b"), mapFromInterpolatedVarWithInterpolatedContents);
+
+        try
+        {
+            mapConverter.convert("not a map", object);
+            fail("Exception not thrown");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+        }
+    }
+
+    public void testNonGenericCollectionConverter()
+    {
+        _context.put("simpleCollection", "[\"a\", \"b\"]");
+
+        ConfiguredObject object = _objectFactory.create(TestRootCategory.class, _attributes);
+
+        AttributeValueConverter<Collection> collectionConverter = getConverter(Collection.class, Collection.class);
+
+        Collection<String> nullCollection = collectionConverter.convert(null, object);
+        assertNull(nullCollection);
+
+        Collection<String> emptyCollection = collectionConverter.convert("[ ]", object);
+        assertTrue(emptyCollection.isEmpty());
+
+        Collection<String> collection = collectionConverter.convert("[\"a\",  \"b\"]", object);
+        assertEquals(2, collection.size());
+        assertTrue(collection.contains("a"));
+        assertTrue(collection.contains("b"));
+
+        Collection<String> collectionFromInterpolatedVar = collectionConverter.convert("${simpleCollection}", object);
+        assertEquals(2, collectionFromInterpolatedVar.size());
+        assertTrue(collectionFromInterpolatedVar.contains("a"));
+        assertTrue(collectionFromInterpolatedVar.contains("b"));
+
+        try
+        {
+            collectionConverter.convert("not a collection", object);
+            fail("Exception not thrown");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+        }
+    }
+
+    public void testNonGenericListConverter()
+    {
+        _context.put("simpleList", "[\"a\", \"b\"]");
+
+        ConfiguredObject object = _objectFactory.create(TestRootCategory.class, _attributes);
+
+        AttributeValueConverter<List> listConverter = getConverter(List.class, List.class);
+
+        List<String> nullList = listConverter.convert(null, object);
+        assertNull(nullList);
+
+        List<String> emptyList = listConverter.convert("[ ]", object);
+        assertTrue(emptyList.isEmpty());
+
+        List<String> expectedList = unmodifiableList(asList("a", "b"));
+
+        List<String> list = listConverter.convert("[\"a\",  \"b\"]", object);
+        assertEquals(expectedList, list);
+
+        List<String> listFromInterpolatedVar = listConverter.convert("${simpleList}", object);
+        assertEquals(expectedList, listFromInterpolatedVar);
+
+        try
+        {
+            listConverter.convert("not a list", object);
+            fail("Exception not thrown");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+        }
+    }
+
+    public void testNonGenericSetConverter()
+    {
+        _context.put("simpleSet", "[\"a\", \"b\"]");
+
+        ConfiguredObject object = _objectFactory.create(TestRootCategory.class, _attributes);
+
+        AttributeValueConverter<Set> setConverter = getConverter(Set.class, Set.class);;
+
+        Set<String> nullSet = setConverter.convert(null, object);
+        assertNull(nullSet);
+
+        Set<String> emptySet = setConverter.convert("[ ]", object);
+        assertTrue(emptySet.isEmpty());
+
+        Set<String> expectedSet = unmodifiableSet(new HashSet<>(asList("a", "b")));
+
+        Set<String> set = setConverter.convert("[\"a\",  \"b\"]", object);
+        assertEquals(expectedSet, set);
+
+        Set<String> setFromInterpolatedVar = setConverter.convert("${simpleSet}", object);
+        assertEquals(expectedSet, setFromInterpolatedVar);
+
+        try
+        {
+            setConverter.convert("not a set", object);
+            fail("Exception not thrown");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+        }
+    }
+
+}
\ No newline at end of file