Bug 1041393 - alert notification of type Resource operations is not

correctly rendered by API

AlertNotificationRest extended with extraConfig field where we now output
notification extra config (in case of Resource Operations alert sender, it's
input parameters for operation)

With this commit we now also properly render alert notification config
(simple property types mapped correctly to JSON/XML, not "just" string
values)

This commit fixes ConfigurationHelper#configurationToMap which transforms
Configuration to generic Map, now it can work without Configuration
Definition
- as a fall-back it maps all simple properties to string values.

modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/rest/AlertDefinitionHandlerBean.java

@@ -77,6 +77,7 @@
 import org.rhq.core.domain.criteria.AlertDefinitionCriteria;
 import org.rhq.core.domain.measurement.DataType;
 import org.rhq.core.domain.measurement.MeasurementDefinition;
+import org.rhq.core.domain.operation.OperationDefinition;
 import org.rhq.core.domain.resource.Resource;
 import org.rhq.core.domain.resource.ResourceType;
 import org.rhq.core.domain.resource.group.GroupCategory;
@@ -89,6 +90,7 @@
 import org.rhq.enterprise.server.alert.AlertDefinitionManagerLocal;
 import org.rhq.enterprise.server.alert.AlertManagerLocal;
 import org.rhq.enterprise.server.alert.AlertNotificationManagerLocal;
+import org.rhq.enterprise.server.operation.OperationManagerLocal;
 import org.rhq.enterprise.server.plugin.pc.alert.AlertSenderInfo;
 import org.rhq.enterprise.server.plugin.pc.alert.AlertSenderPluginManager;
 import org.rhq.enterprise.server.resource.ResourceTypeManagerLocal;
@@ -98,6 +100,7 @@
 import org.rhq.enterprise.server.rest.domain.AlertNotificationRest;
 import org.rhq.enterprise.server.rest.domain.AlertSender;
 import org.rhq.enterprise.server.rest.domain.Link;
+import org.rhq.enterprise.server.rest.helper.ConfigurationHelper;
 
 /**
  * Deal with Alert Definitions. Note that this class shares the /alert/ sub-context with the
@@ -128,6 +131,9 @@ public class AlertDefinitionHandlerBean extends AbstractRestBean {
     @EJB
     private ResourceTypeManagerLocal resourceTypeMgr;
 
+    @EJB
+    private OperationManagerLocal operationMgr;
+
 
     @PersistenceContext(unitName = RHQConstants.PERSISTENCE_UNIT_NAME)
     private EntityManager entityManager;
@@ -453,8 +459,8 @@ private AlertNotification notificationRestToNotification(AlertDefinition alertDe
         // TODO validate sender
         notification.setAlertDefinition(alertDefinition);
         Configuration configuration = new Configuration();
-        for (Map.Entry<String,Object> entry: anr.getConfig().entrySet()) {
-            configuration.put(new PropertySimple(entry.getKey(),entry.getValue()));
+        for (Map.Entry<String, Object> entry : anr.getConfig().entrySet()) {
+            configuration.put(new PropertySimple(entry.getKey(), entry.getValue()));
         }
         notification.setConfiguration(configuration);
         // TODO extra configuration (?)
@@ -1186,12 +1192,17 @@ private AlertNotificationRest notificationToNotificationRest(AlertNotification n
         AlertNotificationRest anr = new AlertNotificationRest();
         anr.setId(notification.getId());
         anr.setSenderName(notification.getSenderName());
-
-        for (Map.Entry<String, PropertySimple> entry : notification.getConfiguration().getSimpleProperties().entrySet()) {
-            anr.getConfig().put(entry.getKey(),entry.getValue().getStringValue()); // TODO correct type conversion of 2nd argument
+        ConfigurationDefinition configDef = notificationMgr.getConfigurationDefinitionForSender(notification
+            .getSenderName());
+        anr.setConfig(ConfigurationHelper.configurationToMap(notification.getConfiguration(), configDef, false));
+        ConfigurationDefinition extraConfigDef = null;
+        if ("Resource Operations".equals(notification.getSenderName())) {
+            OperationDefinition opDef = operationMgr.getOperationDefinition(caller,
+                Integer.valueOf(notification.getConfiguration().getSimpleValue("operation-definition-id", "0")));
+            extraConfigDef = opDef.getParametersConfigurationDefinition();
         }
-        // TODO Extra Configuration
-
+        anr.setExtraConfig(ConfigurationHelper.configurationToMap(notification.getExtraConfiguration(), extraConfigDef,
+            false));
         return anr;
     }
 

modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/rest/domain/AlertNotificationRest.java

@@ -38,6 +38,7 @@ public class AlertNotificationRest {
     private int id;
     private String senderName;
     private Map<String,Object> config = new HashMap<String, Object>();
+    private Map<String, Object> extraConfig = new HashMap<String, Object>();
 
     public AlertNotificationRest() {
     }
@@ -68,4 +69,12 @@ public Map<String, Object> getConfig() {
     public void setConfig(Map<String, Object> config) {
         this.config = config;
     }
+
+    public void setExtraConfig(Map<String, Object> extraConfig) {
+        this.extraConfig = extraConfig;
+    }
+
+    public Map<String, Object> getExtraConfig() {
+        return extraConfig;
+    }
 }

modules/enterprise/server/jar/src/main/java/org/rhq/enterprise/server/rest/helper/ConfigurationHelper.java

@@ -73,6 +73,14 @@ else if (mapValue instanceof List) {
 
     }
 
+    /**
+     * convert passed configuration to generic map
+     * @param configuration to be converted
+     * @param definition to convert to proper types, can be null, but strict has to be set to false. If null, all simple properties will be rendered as strings 
+     * @param strict if enabled all configuration properties must have appropriate property definitions
+     * @return generic map
+     * @throws IllegalArgumentException if strict is true and configuration contains property without matching property definition
+     */
     public static Map<String,Object> configurationToMap(Configuration configuration, ConfigurationDefinition definition,
                                                         boolean strict) {
 
@@ -89,13 +97,13 @@ public static Map<String,Object> configurationToMap(Configuration configuration,
         for (Property property : configuration.getProperties()) {
 
             String propertyName = property.getName();
-            PropertyDefinition propertyDefinition = definition.get(propertyName);
+            PropertyDefinition propertyDefinition = null;
+            if (definition != null) {
+                propertyDefinition = definition.get(propertyName);
+            }
             if (propertyDefinition==null) {
                 if (strict) {
                     throw new IllegalArgumentException("No definition for property " + propertyName + "found");
-                } else {
-                    // no definition found and not strict, so skip the property
-                    continue;
                 }
             }
 
@@ -117,7 +125,7 @@ private static Object convertProperty(Property property, PropertyDefinition prop
             target = getInnerList(propertyList, (PropertyDefinitionList)propertyDefinition, strict);
         } else {
             target= convertSimplePropertyValue((PropertySimple) property,
-                ((PropertyDefinitionSimple)propertyDefinition));
+                ((PropertyDefinitionSimple) propertyDefinition), strict);
         }
         return target;
     }
@@ -131,7 +139,10 @@ private static Map<String, Object> getInnerMap(PropertyMap propertyMap, Property
         Set<String> names = map.keySet();
         for (String name : names ) {
             Property property = map.get(name);
-            PropertyDefinition definition = propertyDefinition.get(name);
+            PropertyDefinition definition = null;
+            if (propertyDefinition != null) {
+                definition = propertyDefinition.get(name);
+            }
 
             Object target = convertProperty(property,definition, strict);
             result.put(name,target);
@@ -148,13 +159,13 @@ private static List<Object> getInnerList(PropertyList propertyList, PropertyDefi
         if (definition==null) {
             if (strict) {
                 throw new IllegalArgumentException("No Definition exists for " + propertyList.getName());
-            } else {
-                return result;
             }
         }
 
-
-        PropertyDefinition memberDefinition = definition.getMemberDefinition();
+        PropertyDefinition memberDefinition = null;
+        if (definition != null) {
+            memberDefinition = definition.getMemberDefinition();
+        }
         for (Property property : propertyList.getList()) {
             Object target = convertProperty(property,memberDefinition, strict);
             result.add(target);
@@ -208,16 +219,21 @@ private static PropertyMap getPropertyMap(String propertyName, Map<String, Objec
      * @param definition Definition of the Property
      * @return Object with the correct type
      */
-    public static Object convertSimplePropertyValue(PropertySimple property, PropertyDefinitionSimple definition) {
+    public static Object convertSimplePropertyValue(PropertySimple property, PropertyDefinitionSimple definition,
+        boolean strict) {
 
-        if (definition==null) {
+        if (definition == null && strict) {
             throw new IllegalArgumentException("No definition provided");
         }
 
         if (property==null) {
             return null;
         }
 
+        if (definition == null) {
+            return property.getStringValue();
+        }
+
         PropertySimpleType type = definition.getType();
         String val = property.getStringValue();
 

modules/enterprise/server/jar/src/test/java/org/rhq/enterprise/server/configuration/ConfigurationHelperTest.java

@@ -639,6 +639,47 @@ public void testConfigToMapComplexList() throws Exception {
 
     }
 
+    @Test(expectedExceptions={IllegalArgumentException.class})
+    public void testConfigToMapNoDefinitionStrict() throws Exception {
+        ConfigurationHelper.configurationToMap(Configuration.builder().addSimple("test", "test").build(), null, true);
+    }
+
+    @Test()
+    public void testConfigToMapNoDefinition() throws Exception {
+        Configuration config = Configuration.builder()
+            .openList("list1", "map")
+                .openMap()
+                    .addSimple("val1", "true")
+                    .addSimple("val2", "123")
+                .closeMap()
+            .closeList()
+            .addSimple("simple1", "simple1")
+            .openMap("map2")
+                .addSimple("val3", "FOO")
+             .closeMap()
+            .build();
+
+        Map<String, Object> map = ConfigurationHelper.configurationToMap(config, null, false);
+        assert map != null;
+        assert map.size() == 3;
+        assert map.get("list1") instanceof List;
+        List list1 = (List) map.get("list1");
+        assert list1.size() == 1;
+        assert list1.get(0) instanceof Map;
+        Map<String, Object> map1 = (Map<String, Object>) list1.get(0);
+        assert map1.get("val1") instanceof String;
+        assert "true".equals(map1.get("val1"));
+        assert map1.get("val2") instanceof String;
+        assert "123".equals(map1.get("val2"));
+        assert map.get("simple1") instanceof String;
+        assert "simple1".equals(map.get("simple1"));
+        assert map.get("map2") instanceof Map;
+        Map<String, Object> map2 = (Map<String, Object>) map.get("map2");
+        assert map2.size() == 1;
+        assert map2.get("val3") instanceof String;
+        assert "FOO".equals(map2.get("val3"));
+    }
+
     @Test
     public void testConfigToMapComplexMap() throws Exception {
 
@@ -735,7 +776,8 @@ public void testConfigToMapComplexMapWithBadSetupLenient() throws Exception {
     @Test
     public void testConvertSingleValueNoProperty() throws Exception {
 
-        Object o = ConfigurationHelper.convertSimplePropertyValue(null,new PropertyDefinitionSimple("dummy",null,false,PropertySimpleType.STRING));
+        Object o = ConfigurationHelper.convertSimplePropertyValue(null, new PropertyDefinitionSimple("dummy", null,
+            false, PropertySimpleType.STRING), true);
 
         assert o == null;
 
@@ -745,7 +787,7 @@ public void testConvertSingleValueNoProperty() throws Exception {
     public void testConvertSingleValueNoDefinition() throws Exception {
 
         try {
-            ConfigurationHelper.convertSimplePropertyValue(new PropertySimple("foo","bar"),null);
+            ConfigurationHelper.convertSimplePropertyValue(new PropertySimple("foo", "bar"), null, true);
             assert false;
         }
         catch (IllegalArgumentException iae) {

modules/integration-tests/rest-api/src/test/java/org/rhq/modules/integrationTests/restApi/d/AlertNotification.java

@@ -31,6 +31,7 @@ public class AlertNotification {
     private int id;
     private String senderName;
     private Map<String,Object> config = new HashMap<String, Object>();
+    private Map<String, Object> extraConfig = new HashMap<String, Object>();
 
     public AlertNotification() {
     }
@@ -62,4 +63,12 @@ public Map<String, Object> getConfig() {
     public void setConfig(Map<String, Object> config) {
         this.config = config;
     }
+
+    public void setExtraConfig(Map<String, Object> extraConfig) {
+        this.extraConfig = extraConfig;
+    }
+
+    public Map<String, Object> getExtraConfig() {
+        return extraConfig;
+    }
 }