Fix list conversion issues in the xml gatherer (#157)

* Fix list conversion issues in the xml gatherer

* Fix code for sporadic failure

* Include brackets and comments to improve readability

internal/factsengine/gatherers/cibadmin.go

@@ -45,8 +45,8 @@ func (g *CibAdminGatherer) Gather(factsRequests []entities.FactRequest) ([]entit
 		return nil, CibAdminCommandError.Wrap(err.Error())
 	}
 
-	elementsToList := []string{"primitive", "clone", "master", "group",
-		"nvpair", "op", "rsc_location", "rsc_order", "rsc_colocation"}
+	elementsToList := map[string]bool{"primitive": true, "clone": true, "master": true, "group": true,
+		"nvpair": true, "op": true, "rsc_location": true, "rsc_order": true, "rsc_colocation": true}
 
 	factValueMap, err := parseXMLToFactValueMap(cibadmin, elementsToList)
 	if err != nil {

internal/factsengine/gatherers/cibadmin_test.go

@@ -101,6 +101,12 @@ func (suite *CibAdminTestSuite) TestCibAdminGather() {
 			Argument: "cib.not_found.crm_config",
 			CheckID:  "check3",
 		},
+		{
+			Name:     "primitives",
+			Gatherer: "cibadmin",
+			Argument: "cib.configuration.resources.primitive.0",
+			CheckID:  "check4",
+		},
 	}
 
 	factResults, err := p.Gather(factRequests)
@@ -131,6 +137,33 @@ func (suite *CibAdminTestSuite) TestCibAdminGather() {
 				Message: "error getting value: requested field value not found: " +
 					"cib.not_found.crm_config"},
 		},
+		{
+			Name: "primitives",
+			Value: &entities.FactValueMap{
+				Value: map[string]entities.FactValue{
+					"id":    &entities.FactValueString{Value: "stonith-sbd"},
+					"class": &entities.FactValueString{Value: "stonith"},
+					"type":  &entities.FactValueString{Value: "external/sbd"},
+					"instance_attributes": &entities.FactValueMap{
+						Value: map[string]entities.FactValue{
+							"id": &entities.FactValueString{Value: "stonith-sbd-instance_attributes"},
+							"nvpair": &entities.FactValueList{
+								Value: []entities.FactValue{
+									&entities.FactValueMap{
+										Value: map[string]entities.FactValue{
+											"id":    &entities.FactValueString{Value: "stonith-sbd-instance_attributes-pcmk_delay_max"},
+											"name":  &entities.FactValueString{Value: "pcmk_delay_max"},
+											"value": &entities.FactValueString{Value: "30s"},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			CheckID: "check4",
+		},
 	}
 
 	suite.NoError(err)

internal/factsengine/gatherers/xml.go

@@ -11,21 +11,15 @@ func init() {
 	mxj.PrependAttrWithHyphen(false)
 }
 
-func parseXMLToFactValueMap(xmlContent []byte, elementsToList []string) (*entities.FactValueMap, error) {
+func parseXMLToFactValueMap(xmlContent []byte, elementsToList map[string]bool) (*entities.FactValueMap, error) {
 	mv, err := mxj.NewMapXml(xmlContent)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, element := range elementsToList {
-		err = convertList(&mv, element)
-		if err != nil {
-			return nil, fmt.Errorf("error converting %s to list", element)
-		}
-	}
-
 	mapValue := map[string]interface{}(mv)
-	factValue, err := entities.NewFactValue(mapValue)
+	updatedMap := convertListElements(mapValue, elementsToList)
+	factValue, err := entities.NewFactValue(updatedMap)
 	if err != nil {
 		return nil, err
 	}
@@ -38,26 +32,43 @@ func parseXMLToFactValueMap(xmlContent []byte, elementsToList []string) (*entiti
 	return factValueMap, nil
 }
 
-// convertList converts given keys to list if only one value was present
-// this is needed as many fields are lists even though they might have
-// one element
-func convertList(mv *mxj.Map, key string) error {
-	paths := mv.PathsForKey(key)
-	for _, path := range paths {
-		value, err := mv.ValuesForPath(path)
-		if err != nil {
-			return err
-		}
-
-		values := map[string]interface{}{
-			key: value,
+// convertListElements returns and updated version of the current xml map, where the keys in elementsToList
+// are converted to lists if they are unique entries. This is required due how xml works, as the initial
+// xml to map conversion cannot know if the coming entry is an unique element or not
+func convertListElements(currentMap map[string]interface{}, elementsToList map[string]bool) map[string]interface{} {
+	convertedMap := make(map[string]interface{})
+	for key, value := range currentMap {
+		switch assertedValue := value.(type) {
+		case map[string]interface{}:
+			{
+				convertedMap[key] = convertListElements(assertedValue, elementsToList)
+			}
+		// Item is a list, so each element must be treated to see if its children need a conversion.
+		// The items could be maps itself, so they must recurse yet again
+		case []interface{}:
+			{
+				newList := []interface{}{}
+				for _, item := range assertedValue {
+					if toMap, ok := item.(map[string]interface{}); ok {
+						newList = append(newList, convertListElements(toMap, elementsToList))
+					} else {
+						newList = append(newList, item)
+					}
+				}
+				convertedMap[key] = newList
+			}
+		default:
+			{
+				convertedMap[key] = assertedValue
+			}
 		}
 
-		_, err = mv.UpdateValuesForPath(values, path)
-		if err != nil {
-			return err
+		// If the current key is not a list and it is included in the elementsToList, initialize as list
+		_, isList := convertedMap[key].([]interface{})
+		if elementsToList[key] && !isList {
+			convertedMap[key] = []interface{}{convertedMap[key]}
 		}
 	}
 
-	return nil
+	return convertedMap
 }

pkg/factsengine/entities/fact_value.go

@@ -39,8 +39,8 @@ func NewFactValue(factInterface interface{}) (FactValue, error) {
 		return &FactValueList{Value: newList}, nil
 	case map[string]interface{}:
 		newMap := make(map[string]FactValue)
-		for key, value := range value {
-			newValue, err := NewFactValue(value)
+		for key, mapValue := range value {
+			newValue, err := NewFactValue(mapValue)
 			if err != nil {
 				return nil, err
 			}