QPID-6168: [Java Broker] Move valid value check into the create/change validation logic

* avoid the possibility that a failing many attribute update leaves an object part updated.

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

2 files changed, 49 insertions, 15 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 d60064b722..bddda97f1b 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
@@ -384,20 +384,6 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
             }
 
             Object desiredValue = attribute.convert(value, this);
-
-            if (attribute.hasValidValues())
-            {
-                if (!checkValidValues(attribute, desiredValue))
-                {
-                    throw new IllegalConfigurationException("Attribute '" + attribute.getName()
-                                                       + "' of instance of "+ getClass().getName()
-                                                       + " named '" + getName() + "'"
-                                                       + " cannot have value '" + desiredValue + "'"
-                                                       + ". Valid values are: "
-                                                       + attribute.validValues());
-                }
-            }
-
             field.getField().set(this, desiredValue);
 
             if(field.getPostSettingAction() != null)
@@ -757,6 +743,28 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
     public void onValidate()
     {
+        for(ConfiguredObjectAttribute<?,?> attr : _attributeTypes.values())
+        {
+            if (attr.isAutomated())
+            {
+                ConfiguredAutomatedAttribute autoAttr = (ConfiguredAutomatedAttribute) attr;
+                if (autoAttr.hasValidValues())
+                {
+                    Object desiredValueOrDefault = autoAttr.getValue(this);
+
+                    if (desiredValueOrDefault != null && !checkValidValues(autoAttr, desiredValueOrDefault))
+                    {
+                        throw new IllegalConfigurationException("Attribute '" + autoAttr.getName()
+                                                                + "' of instance of "+ getClass().getName()
+                                                                + " named '" + getName() + "'"
+                                                                + " cannot have value '" + desiredValueOrDefault + "'"
+                                                                + ". Valid values are: "
+                                                                + autoAttr.validValues());
+                    }
+                }
+
+            }
+        }
     }
 
     protected void setEncrypter(final ConfigurationSecretEncrypter encrypter)
@@ -1163,6 +1171,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         }
     }
 
+    // TODO setAttribute does not validate. Do we need this method?
     public Object setAttribute(final String name, final Object expected, final Object desired)
             throws IllegalStateException, AccessControlException, IllegalArgumentException
     {
@@ -1187,6 +1196,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         });
     }
 
+
     protected boolean changeAttribute(final String name, final Object expected, final Object desired)
     {
         synchronized (_attributes)
@@ -1524,6 +1534,30 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         {
             throw new IllegalConfigurationException("Cannot change existing configured object id");
         }
+
+        for(ConfiguredObjectAttribute<?,?> attr : _attributeTypes.values())
+        {
+            if (attr.isAutomated() && changedAttributes.contains(attr.getName()))
+            {
+                ConfiguredAutomatedAttribute autoAttr = (ConfiguredAutomatedAttribute) attr;
+                if (autoAttr.hasValidValues())
+                {
+                    Object desiredValue = autoAttr.getValue(proxyForValidation);
+
+                    if (!checkValidValues(autoAttr, desiredValue))
+                    {
+                        throw new IllegalConfigurationException("Attribute '" + autoAttr.getName()
+                                                                + "' of instance of "+ getClass().getName()
+                                                                + " named '" + getName() + "'"
+                                                                + " cannot have value '" + desiredValue + "'"
+                                                                + ". Valid values are: "
+                                                                + autoAttr.validValues());
+                    }
+                }
+
+            }
+        }
+
     }
 
     private ConfiguredObject<?> createProxyForValidation(final Map<String, Object> attributes)
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 2da5a988f5..3764461841 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
@@ -539,7 +539,7 @@ public class AbstractConfiguredObjectTest extends QpidTestCase
 
         try
         {
-            object.setAttribute(TestRootCategory.VALID_VALUE, TestRootCategory.VALID_VALUE2, "illegal");
+            object.setAttributes(Collections.singletonMap(TestRootCategory.VALID_VALUE, "illegal"));
             fail("Exception not thrown");
         }
         catch (IllegalConfigurationException iae)