[BZ 1141982] Readonly required props with defaults forced to same values

during bundle deployment.

In the case of new resource creation where the user has the
ability to enter an initial value for a readonly property. This is
not true with bundle deployment because the configuration object is
populated with the default values straight away. This is to make it
possible for the server-side bundle plugin to pass on some
information to the agent-side bundle handler.

This fix ensures that an update of such readonly property is not
possible either through UI or remote API.

In remote API we simply check the supplied deployment configuration
more rigorously, disallowing changes of any readonly properties from
the default values provided by the deployment configuration definition.

The UI had a clever logic of reusing configuration properties used
with the currently live deployment (if any). This approach generally
worked but was also updating readonly properties with the values
coming from the live deployment which is a wrong thing to do if we
understand the readonly props as a way of defining additional
metadata about the bundle version generated by the server-side plugin.
These should always be relevant to the bundle version being deployed
and not taken over from a deployment of some previous bundle version.

(cherry picked from commit 8ddc6ca)

modules/core/domain/src/main/java/org/rhq/core/domain/configuration/ConfigurationUtility.java

@@ -125,6 +125,45 @@ public static void normalizeConfiguration(Configuration configuration,
         }
     }
 
+    /**
+     * This is a little bit similar to {@link #normalizeConfiguration(Configuration,
+     * org.rhq.core.domain.configuration.definition.ConfigurationDefinition, boolean, boolean)}
+     * but can be used in cases where you need a new configuration based on the default template of the provided
+     * configuration definition yet want to reuse some or all of the corresponding properties from the provided
+     * "existing" configuration.
+     * <p/>
+     * In another words you want to adapt the existing configuration to the new format prescribed by the definition.
+     *
+     * @param existingConfiguration   the existing configuration to take values from if possible.
+     * @param definition              the definition to which the returned configuration will conform
+     * @param adaptReadonlyProperties set this to true if also the values of the readonly properties should be taken
+     *                                from the existing configuration. If this is false, the default value of the
+     *                                readonly properties defined in the definition is used instead.
+     *
+     * @return a new configuration object corresponding to the definition, with values reused from the existing
+     * configuration if possible
+     */
+    public static Configuration adaptConfiguration(Configuration existingConfiguration,
+        ConfigurationDefinition definition, boolean adaptReadonlyProperties) {
+
+        Configuration targetConfig = createDefaultConfiguration(definition);
+
+        if (existingConfiguration != null) {
+            for (Map.Entry<String, PropertyDefinition> e : definition.getPropertyDefinitions().entrySet()) {
+                String name = e.getKey();
+                PropertyDefinition def = e.getValue();
+
+                Property source = existingConfiguration.get(name);
+                if (source != null) {
+                    Property target = targetConfig.get(name);
+                    adaptProperty(source, target, def, targetConfig, adaptReadonlyProperties);
+                }
+            }
+        }
+
+        return targetConfig;
+    }
+
     /**
      * Validate the given configuration according to the given configuration definition. That is, check that any
      * required properties in the top-level configuration Map or any sub-Maps, are defined and, in the case of simple
@@ -170,6 +209,14 @@ public static List<String> validateConfiguration(Configuration configuration, Co
     private static void createDefaultProperty(PropertyDefinition propertyDefinition,
         AbstractPropertyMap parentPropertyMap) {
 
+        Property property = instantiateDefaultProperty(propertyDefinition);
+
+        if (property != null) {
+            parentPropertyMap.put(property);
+        }
+    }
+
+    private static Property instantiateDefaultProperty(PropertyDefinition propertyDefinition) {
         Property property = null;
 
         if (propertyDefinition instanceof PropertyDefinitionSimple) {
@@ -216,9 +263,7 @@ private static void createDefaultProperty(PropertyDefinition propertyDefinition,
             }
         }
 
-        if (property != null) {
-            parentPropertyMap.put(property);
-        }
+        return property;
     }
 
     private static void normalizeProperty(PropertyDefinition propertyDefinition, AbstractPropertyMap parentPropertyMap,
@@ -243,7 +288,8 @@ private static void normalizeProperty(PropertyDefinition propertyDefinition, Abs
             else if (propertyDefinition instanceof PropertyDefinitionMap) {
                 PropertyMap propertyMap = parentPropertyMap.getMap(propertyDefinition.getName());
                 PropertyDefinitionMap propertyDefinitionMap = (PropertyDefinitionMap) propertyDefinition;
-                normalizePropertyMap(propertyMap, propertyDefinitionMap, false, false); // TODO do we want to pass normalizeRequired/OptionalDefaults?
+                normalizePropertyMap(propertyMap, propertyDefinitionMap, false,
+                    false); // TODO do we want to pass normalizeRequired/OptionalDefaults?
             } else if (propertyDefinition instanceof PropertyDefinitionList) {
                 PropertyDefinitionList propertyDefinitionList = (PropertyDefinitionList) propertyDefinition;
                 PropertyDefinition listMemberPropertyDefinition = propertyDefinitionList.getMemberDefinition();
@@ -254,7 +300,8 @@ else if (propertyDefinition instanceof PropertyDefinitionMap) {
                     PropertyList propertyList = parentPropertyMap.getList(propertyDefinition.getName());
                     for (Property property : propertyList.getList()) {
                         PropertyMap propertyMap = (PropertyMap) property;
-                        normalizePropertyMap(propertyMap, propertyDefinitionMap, false, false); // TODO do we want to pass normalizeRequired/OptionalDefaults?
+                        normalizePropertyMap(propertyMap, propertyDefinitionMap, false,
+                            false); // TODO do we want to pass normalizeRequired/OptionalDefaults?
                     }
                 }
             }
@@ -422,4 +469,132 @@ private static void validatePropertyListSize(PropertyList propertyList,
                 + listMax + " row(s)");
         }
     }
+
+    private static void adaptPropertyMap(AbstractPropertyMap source, AbstractPropertyMap target,
+        PropertyDefinitionMap definition, Object parent, boolean adaptReadonlyProperties) {
+
+        if ((adaptReadonlyProperties || !definition.isReadOnly()) && target == null) {
+            target = new PropertyMap(definition.getName());
+            add(parent, (PropertyMap) target);
+        }
+
+        for (Map.Entry<String, PropertyDefinition> e : definition.getPropertyDefinitions().entrySet()) {
+            String name = e.getKey();
+            PropertyDefinition def = e.getValue();
+
+            Property sourceChild = source.get(name);
+
+            // only bother if we have something to adapt from
+            if (sourceChild != null) {
+                Property targetChild = target.get(name);
+                adaptProperty(sourceChild, targetChild, def, target, adaptReadonlyProperties);
+            }
+        }
+    }
+
+    private static void adaptPropertyList(PropertyList source, PropertyList target, PropertyDefinitionList definition,
+        Object parent, boolean adaptReadonlyProperties) {
+
+        if ((adaptReadonlyProperties || !definition.isReadOnly())) {
+
+            if (target == null) {
+                target = new PropertyList(definition.getName());
+                add(parent, target);
+            }
+
+            if (target.getList().isEmpty()) {
+                PropertyDefinition memberDef = definition.getMemberDefinition();
+
+                for (Property p : source.getList()) {
+                    PropertyType type = conforms(p, memberDef);
+
+                    if (type != null && type != PropertyType.UNKNOWN) {
+                        Property targetMember = instantiateDefaultProperty(memberDef);
+                        target.add(targetMember);
+
+                        adaptProperty(p, targetMember, memberDef, target, adaptReadonlyProperties);
+                    }
+                }
+            }
+        }
+    }
+
+    private static void adaptPropertySimple(PropertySimple source, PropertySimple target,
+        PropertyDefinitionSimple definition, Object parent, boolean adaptReadonlyProperties) {
+
+        if (adaptReadonlyProperties || !definition.isReadOnly()) {
+
+            if (target == null) {
+                target = new PropertySimple();
+                target.setName(definition.getName());
+                add(parent, target);
+            }
+
+            target.setStringValue(source.getStringValue());
+        }
+    }
+
+    private static void adaptProperty(Property source, Property target, PropertyDefinition def, Object parent,
+        boolean adaptReadonlyProperties) {
+
+        PropertyType sourceType = conforms(source, def);
+        PropertyType targetType = conforms(target, def);
+
+        if (sourceType != null && sourceType != PropertyType.UNKNOWN &&
+            (sourceType == targetType || targetType == PropertyType.UNKNOWN)) {
+
+            switch (sourceType) {
+            case MAP:
+                adaptPropertyMap((AbstractPropertyMap) source, (AbstractPropertyMap) target,
+                    (PropertyDefinitionMap) def, parent, adaptReadonlyProperties);
+                break;
+            case LIST:
+                adaptPropertyList((PropertyList) source, (PropertyList) target, (PropertyDefinitionList) def, parent,
+                    adaptReadonlyProperties);
+                break;
+            case SIMPLE:
+                adaptPropertySimple((PropertySimple) source, (PropertySimple) target, (PropertyDefinitionSimple) def,
+                    parent, adaptReadonlyProperties);
+                break;
+            case DYNAMIC:
+                //TODO
+            }
+        }
+        // the types of the properties don't match... let's just leave target as it is because it comes
+        // from the config definition we want...
+    }
+
+    private static PropertyType conforms(Property p, PropertyDefinition d) {
+        if (p == null) {
+            return PropertyType.UNKNOWN;
+        }
+
+        if (!p.getName().equals(d.getName())) {
+            return null;
+        }
+
+        if (p instanceof PropertySimple && d instanceof PropertyDefinitionSimple) {
+            return PropertyType.SIMPLE;
+        } else if (p instanceof PropertyList && d instanceof PropertyDefinitionList) {
+            return PropertyType.LIST;
+        } else if (p instanceof PropertyMap && d instanceof PropertyDefinitionMap) {
+            return PropertyType.MAP;
+        } else if (p instanceof PropertySimple && d instanceof PropertyDefinitionDynamic) {
+            return PropertyType.DYNAMIC;
+        } else {
+            return null;
+        }
+    }
+
+    private static void add(Object parent, Property prop) {
+        if (parent instanceof AbstractPropertyMap) {
+            ((AbstractPropertyMap) parent).put(prop);
+        } else if (parent instanceof PropertyList) {
+            ((PropertyList) parent).add(prop);
+        }
+    }
+
+    private enum PropertyType {
+        UNKNOWN, SIMPLE, LIST, MAP, DYNAMIC
+    }
 }

modules/core/domain/src/test/java/org/rhq/core/domain/configuration/ConfigurationUtilityTest.java

@@ -23,6 +23,10 @@
 
 package org.rhq.core.domain.configuration;
 
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+
 import org.testng.annotations.Test;
 
 import org.rhq.core.domain.configuration.definition.ConfigurationDefinition;
@@ -360,4 +364,81 @@ public void testCreateDefaultAllSimpleMapList() {
         assert childMap2.getName().equals(mapRequired2.getName());
         assert childMap3.getName().equals(mapRequiredDefault2.getName());
     }
+
+    public void testAdaptConfiguration() {
+        ConfigurationDefinition def = new ConfigurationDefinition("test", null);
+
+        PropertyDefinitionSimple writableOptional = new PropertyDefinitionSimple("writable-optional", null, false,
+            PropertySimpleType.STRING);
+        PropertyDefinitionSimple writableRequired = new PropertyDefinitionSimple("writable-required", null, true,
+            PropertySimpleType.STRING);
+        PropertyDefinitionSimple readonlyRequiredWithDefault = new PropertyDefinitionSimple("readonly-required-default",
+            null, true, PropertySimpleType.STRING);
+        readonlyRequiredWithDefault.setDefaultValue("readonly-required-default-definition");
+        readonlyRequiredWithDefault.setReadOnly(true);
+
+        PropertyDefinitionSimple listMember = new PropertyDefinitionSimple("list-member", null, true,
+            PropertySimpleType.STRING);
+        listMember.setDefaultValue("list-member-definition");
+        listMember.setReadOnly(true);
+
+        PropertyDefinitionList list = new PropertyDefinitionList("list-optional", null, false,
+            listMember);
+
+        PropertyDefinitionSimple mapMember1 = new PropertyDefinitionSimple("mm1", null, true,
+            PropertySimpleType.BOOLEAN);
+        PropertyDefinitionSimple mapMember2 = new PropertyDefinitionSimple("mm2", null, false,
+            PropertySimpleType.STRING);
+
+        PropertyDefinitionMap map = new PropertyDefinitionMap("map-required", null, true, mapMember1, mapMember2);
+
+        def.put(writableOptional);
+        def.put(writableRequired);
+        def.put(readonlyRequiredWithDefault);
+        def.put(list);
+        def.put(map);
+
+        Configuration existing = new Configuration();
+        existing.put(new PropertySimple("writable-required", "writable-required-existing"));
+        existing.put(new PropertySimple("readonly-required-default", "readonly-required-default-existing"));
+        existing.put(new PropertySimple("prop-from-old-def", "kachny"));
+        existing
+            .put(new PropertyList("list-optional", new PropertySimple("list-member", "list-member-existing")));
+
+        Configuration adapted = ConfigurationUtility.adaptConfiguration(existing, def, false);
+
+        PropertySimple adaptedWritableRequired = adapted.getSimple("writable-required");
+        assertNotNull(adaptedWritableRequired);
+        assertEquals(adaptedWritableRequired.getStringValue(), "writable-required-existing");
+
+        PropertySimple adaptedReadonlyRequiredWithDefault = adapted.getSimple("readonly-required-default");
+        assertNotNull(adaptedReadonlyRequiredWithDefault);
+        assertEquals("readonly-required-default-definition", adaptedReadonlyRequiredWithDefault.getStringValue());
+
+        PropertyList adaptedList = adapted.getList("list-optional");
+        assertNotNull(adaptedList);
+        assertEquals(1, adaptedList.getList().size());
+        assertEquals(((PropertySimple) adaptedList.getList().get(0)).getStringValue(), "list-member-definition");
+
+        assertNull(adapted.get("prop-from-old-def"));
+
+
+
+        adapted = ConfigurationUtility.adaptConfiguration(existing, def, true);
+
+        adaptedWritableRequired = adapted.getSimple("writable-required");
+        assertNotNull(adaptedWritableRequired);
+        assertEquals(adaptedWritableRequired.getStringValue(), "writable-required-existing");
+
+        adaptedReadonlyRequiredWithDefault = adapted.getSimple("readonly-required-default");
+        assertNotNull(adaptedReadonlyRequiredWithDefault);
+        assertEquals("readonly-required-default-existing", adaptedReadonlyRequiredWithDefault.getStringValue());
+
+        adaptedList = adapted.getList("list-optional");
+        assertNotNull(adaptedList);
+        assertEquals(1, adaptedList.getList().size());
+        assertEquals(((PropertySimple) adaptedList.getList().get(0)).getStringValue(), "list-member-existing");
+
+        assertNull(adapted.get("prop-from-old-def"));
+    }
 }

modules/enterprise/gui/coregui/src/main/java/org/rhq/coregui/client/bundle/deploy/GetDeploymentConfigStep.java

@@ -22,11 +22,9 @@
 import com.smartgwt.client.widgets.HTMLFlow;
 import com.smartgwt.client.widgets.layout.VLayout;
 
-import org.rhq.core.domain.bundle.BundleDeployment;
 import org.rhq.core.domain.configuration.Configuration;
 import org.rhq.core.domain.configuration.ConfigurationUtility;
 import org.rhq.core.domain.configuration.definition.ConfigurationDefinition;
-import org.rhq.core.domain.configuration.definition.ConfigurationTemplate;
 import org.rhq.coregui.client.components.configuration.ConfigurationEditor;
 import org.rhq.coregui.client.components.wizard.AbstractWizardStep;
 
@@ -67,48 +65,17 @@ public Canvas getCanvas() {
                 editor.addMember(label);
             } else {
                 // otherwise, pop up the config editor to get the needed config
-                Configuration startingConfig;
-                BundleDeployment liveDeployment = wizard.getLiveDeployment();
-                boolean useLiveConfig = false;
+                Configuration liveConfig =
+                    wizard.getLiveDeployment() == null ? null : wizard.getLiveDeployment().getConfiguration();
 
-                if (liveDeployment != null) {
-                    // If we have a live deployment but it didn't have configuration before
-                    // then make sure we don't start with a completely empty config.
-                    // In that case, we need to ask for the config from the default template.
-                    // But if our live deployment DID have a previous non-empty config, we'll use it
-                    // to allow the user to see the previous config values used in the live deployment.
-                    Configuration liveConfig = liveDeployment.getConfiguration();
-                    if (liveConfig != null) {
-                        useLiveConfig = !liveConfig.getMap().isEmpty();
-                    }
-                }
-
-                if (useLiveConfig == false) {
-                    ConfigurationTemplate defaultTemplate = configDef.getDefaultTemplate();
-                    startingConfig = (defaultTemplate != null) ? defaultTemplate.createConfiguration()
-                        : new Configuration();
-                } else {
-                    startingConfig = getNormalizedLiveConfig(configDef);
-                }
+                Configuration startingConfig = ConfigurationUtility.adaptConfiguration(liveConfig, configDef, false);
                 editor = new ConfigurationEditor(configDef, startingConfig);
             }
         }
 
         return editor;
     }
 
-    private Configuration getNormalizedLiveConfig(ConfigurationDefinition configDef) {
-        Configuration config = wizard.getLiveDeployment().getConfiguration();
-        if (null == config) {
-            config = new Configuration();
-        } else {
-            config = config.deepCopy(false);
-            ConfigurationUtility.normalizeConfiguration(config, configDef, true, false);
-        }
-
-        return config;
-    }
-
     public boolean nextPage() {
         if (null == wizard.getNewDeploymentConfig()) {
             wizard.setNewDeploymentConfig(((ConfigurationEditor) editor).getConfiguration());

modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/bundle/BundleManagerBean.java

@@ -357,7 +357,13 @@ private BundleDeployment createBundleDeploymentImpl(Subject subject, BundleVersi
                 throw new IllegalArgumentException(
                     "Missing Configuration. Configuration is required when the specified BundleVersion defines Configuration Properties.");
             }
-            List<String> errors = ConfigurationUtility.validateConfiguration(configuration, configDef);
+
+            // passing in the default configuration will make sure that the readonly properties with the non-null
+            // default values defined in the config def cannot be overridden by the caller.
+            // Those properties are meant to be set by the bundle plugins, not by the user.
+            Configuration defaultConfig = ConfigurationUtility.createDefaultConfiguration(configDef);
+
+            List<String> errors = ConfigurationUtility.validateConfiguration(configuration, defaultConfig, configDef);
             if (!errors.isEmpty()) {
                 throw new IllegalArgumentException("Invalid Configuration: " + errors.toString());
             }