QPID-6019 : [Java Broker] Move configured object registration with parents until after resolution, and provide clearer error messages on attribute resolution failure

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

7 files changed, 150 insertions, 13 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
index 436842017a..31de8118dd 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
@@ -469,7 +469,6 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
     {
         if(_dynamicState.compareAndSet(DynamicState.UNINIT, DynamicState.OPENED))
         {
-            registerWithParents();
             final AuthenticatedPrincipal currentUser = SecurityManager.getCurrentUser();
             if(currentUser != null)
             {
@@ -487,6 +486,9 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
             doResolution(true);
             doValidation(true);
+
+            registerWithParents();
+
             doCreation(true);
             doOpening(true);
             doAttainState();
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 b7b56db15c..15e804e6f5 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
@@ -89,7 +89,15 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                return Long.valueOf(AbstractConfiguredObject.interpolate(object, (String) value));
+                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
+                try
+                {
+                    return Long.valueOf(interpolated);
+                }
+                catch(NumberFormatException e)
+                {
+                    throw new IllegalArgumentException("Cannot convert string '" + interpolated + "'",e);
+                }
             }
             else if(value == null)
             {
@@ -117,7 +125,15 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                return Integer.valueOf(AbstractConfiguredObject.interpolate(object, (String) value));
+                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
+                try
+                {
+                    return Integer.valueOf(interpolated);
+                }
+                catch(NumberFormatException e)
+                {
+                    throw new IllegalArgumentException("Cannot convert string '" + interpolated + "'",e);
+                }
             }
             else if(value == null)
             {
@@ -145,7 +161,15 @@ abstract class AttributeValueConverter<T>
             }
             else if(value instanceof String)
             {
-                return Short.valueOf(AbstractConfiguredObject.interpolate(object, (String) value));
+                String interpolated = AbstractConfiguredObject.interpolate(object, (String) value);
+                try
+                {
+                    return Short.valueOf(interpolated);
+                }
+                catch(NumberFormatException e)
+                {
+                    throw new IllegalArgumentException("Cannot convert string '" + interpolated + "'",e);
+                }
             }
             else if(value == null)
             {
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/ConfiguredObjectAttribute.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/ConfiguredObjectAttribute.java
index 6f6ef7f6e1..1d1c736cd3 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/ConfiguredObjectAttribute.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/ConfiguredObjectAttribute.java
@@ -21,11 +21,10 @@
 package org.apache.qpid.server.model;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Type;
 
 public abstract class ConfiguredObjectAttribute<C extends ConfiguredObject, T> extends ConfiguredObjectAttributeOrStatistic<C,T>
 {
-
-
     ConfiguredObjectAttribute(Class<C> clazz,
                               final Method getter)
     {
@@ -48,6 +47,20 @@ public abstract class ConfiguredObjectAttribute<C extends ConfiguredObject, T> e
 
     public T convert(final Object value, C object)
     {
-        return getConverter().convert(value, object);
+        final AttributeValueConverter<T> converter = getConverter();
+        try
+        {
+            return converter.convert(value, object);
+        }
+        catch (IllegalArgumentException iae)
+        {
+            Type returnType = getGetter().getGenericReturnType();
+            String simpleName = returnType instanceof Class ? ((Class) returnType).getSimpleName() : returnType.toString();
+
+            throw new IllegalArgumentException("Cannot convert '" + value
+                                               + "' into a " + simpleName
+                                               + " for attribute " + getName()
+                                               + " (" + iae.getMessage() + ")", iae);
+        }
     }
 }
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 e147abd170..e5c5a89c10 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
@@ -26,6 +26,7 @@ import java.util.Map;
 
 import junit.framework.TestCase;
 
+import org.apache.qpid.server.model.testmodel.TestChildCategory;
 import org.apache.qpid.server.model.testmodel.TestModel;
 import org.apache.qpid.server.model.testmodel.TestRootCategory;
 import org.apache.qpid.server.store.ConfiguredObjectRecord;
@@ -187,4 +188,73 @@ public class AbstractConfiguredObjectTest extends TestCase
         assertEquals("myValue", object1.getStringValue());
     }
 
-}
\ No newline at end of file
+    public void testCreationOfObjectWithInvalidInterpolatedValues()
+    {
+        final String parentName = "parent";
+        TestRootCategory parent =
+                _model.getObjectFactory().create(TestRootCategory.class,
+                                                 Collections.<String, Object>singletonMap(ConfiguredObject.NAME,
+                                                                                          parentName)
+                                                );
+
+        parent.setAttributes(Collections.singletonMap(ConfiguredObject.CONTEXT,
+                                                      Collections.singletonMap("contextVal", "foo")));
+
+        final Map<String, Object> attributes = new HashMap<>();
+        attributes.put("intValue", "${contextVal}");
+        attributes.put("name", "child");
+        attributes.put("integerSet", "[ ]");
+        attributes.put(ConfiguredObject.TYPE, "test");
+
+        try
+        {
+            _model.getObjectFactory().create(TestChildCategory.class, attributes, parent);
+            fail("creation of child object should have failed due to invalid value");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+            String message = e.getMessage();
+            assertTrue("Message does not contain the attribute name", message.contains("intValue"));
+            assertTrue("Message does not contain the non-interpolated value", message.contains("contextVal"));
+            assertTrue("Message does not contain the interpolated value", message.contains("foo"));
+
+        }
+
+        assertTrue("Child should not have been registered with parent",
+                   parent.getChildren(TestChildCategory.class).isEmpty());
+    }
+
+    public void testCreationOfObjectWithInvalidDefaultValues()
+    {
+        final String parentName = "parent";
+        TestRootCategory parent =
+                _model.getObjectFactory().create(TestRootCategory.class,
+                                                 Collections.<String, Object>singletonMap(ConfiguredObject.NAME,
+                                                                                          parentName)
+                                                );
+
+        final Map<String, Object> attributes = new HashMap<>();
+        attributes.put("intValue", "1");
+        attributes.put("name", "child");
+        attributes.put(ConfiguredObject.TYPE, "test");
+
+        try
+        {
+            _model.getObjectFactory().create(TestChildCategory.class, attributes, parent);
+            fail("creation of child object should have failed due to invalid value");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // PASS
+            String message = e.getMessage();
+            assertTrue("Message does not contain the attribute name", message.contains("integerSet"));
+            assertTrue("Message does not contain the error value", message.contains("foo"));
+
+        }
+
+        assertTrue("Child should not have been registered with parent",
+                   parent.getChildren(TestChildCategory.class).isEmpty());
+    }
+
+}
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategory.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategory.java
index 0c8dcc8744..d3fe14b7d8 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategory.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategory.java
@@ -20,6 +20,8 @@
  */
 package org.apache.qpid.server.model.testmodel;
 
+import java.util.Set;
+
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.ManagedAttribute;
 import org.apache.qpid.server.model.ManagedObject;
@@ -30,6 +32,12 @@ public interface TestChildCategory<X extends TestChildCategory<X>> extends Confi
 
     String NON_INTERPOLATED_VALID_VALUE = "${file.separator}";
 
-    @ManagedAttribute(validValues = { NON_INTERPOLATED_VALID_VALUE })
+    @ManagedAttribute(validValues = { NON_INTERPOLATED_VALID_VALUE }, defaultValue = "")
     String getValidValueNotInterpolated();
+
+    @ManagedAttribute( defaultValue = "3" )
+    int getIntValue();
+
+    @ManagedAttribute( defaultValue = "[ \"1\", \"2\", \"foo\" ]" )
+    Set<Integer> getIntegerSet();
 }
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategoryImpl.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategoryImpl.java
index b5a4182f79..080a352f11 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategoryImpl.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodel/TestChildCategoryImpl.java
@@ -21,6 +21,7 @@
 package org.apache.qpid.server.model.testmodel;
 
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ManagedAttributeField;
@@ -37,6 +38,12 @@ public class TestChildCategoryImpl
     @ManagedAttributeField
     private String _validValueNotInterpolated;
 
+    @ManagedAttributeField
+    private int _intValue;
+
+    @ManagedAttributeField
+    private Set<Integer> _integerSet;
+
 
     @ManagedObjectFactoryConstructor
     public TestChildCategoryImpl(final Map<String, Object> attributes, TestRootCategory<?> parent)
@@ -57,4 +64,16 @@ public class TestChildCategoryImpl
     {
         return _validValueNotInterpolated;
     }
+
+    @Override
+    public int getIntValue()
+    {
+        return _intValue;
+    }
+
+    @Override
+    public Set<Integer> getIntegerSet()
+    {
+        return _integerSet;
+    }
 }
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/auth/manager/SimpleLDAPAuthenticationManagerFactoryTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/auth/manager/SimpleLDAPAuthenticationManagerFactoryTest.java
index 56283b1392..6001ed1750 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/auth/manager/SimpleLDAPAuthenticationManagerFactoryTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/auth/manager/SimpleLDAPAuthenticationManagerFactoryTest.java
@@ -36,7 +36,6 @@ import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.model.BrokerModel;
 import org.apache.qpid.server.model.ConfiguredObjectFactory;
 import org.apache.qpid.server.model.TrustStore;
-import org.apache.qpid.server.model.UnknownConfiguredObjectException;
 import org.apache.qpid.server.util.BrokerTestHelper;
 
 public class SimpleLDAPAuthenticationManagerFactoryTest extends TestCase
@@ -108,10 +107,12 @@ public class SimpleLDAPAuthenticationManagerFactoryTest extends TestCase
             _factory.create(AuthenticationProvider.class, _configuration, _broker);
             fail("Exception not thrown");
         }
-        catch(UnknownConfiguredObjectException e)
+        catch(IllegalArgumentException e)
         {
-            assertEquals(e.getCategory(), TrustStore.class);
-            assertEquals(e.getName(), "notfound");
+            // PASS
+            assertTrue("Message does not include underlying issue", e.getMessage().contains("name 'notfound'"));
+            assertTrue("Message does not include the attribute name", e.getMessage().contains("trustStore"));
+            assertTrue("Message does not include the expected type", e.getMessage().contains("TrustStore"));
         }
     }