From 4c34887829ecc0cf298b3a8447de15654ac787ee Mon Sep 17 00:00:00 2001 From: Xabier Arbulu Insausti Date: Fri, 23 Dec 2022 10:10:34 +0100 Subject: [PATCH] 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 | 4 +- .../factsengine/gatherers/cibadmin_test.go | 33 ++++++++++ internal/factsengine/gatherers/xml.go | 63 +++++++++++-------- pkg/factsengine/entities/fact_value.go | 4 +- 4 files changed, 74 insertions(+), 30 deletions(-) diff --git a/internal/factsengine/gatherers/cibadmin.go b/internal/factsengine/gatherers/cibadmin.go index 17f42539..d3871c8d 100644 --- a/internal/factsengine/gatherers/cibadmin.go +++ b/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 { diff --git a/internal/factsengine/gatherers/cibadmin_test.go b/internal/factsengine/gatherers/cibadmin_test.go index 8a3cba10..daa3b737 100644 --- a/internal/factsengine/gatherers/cibadmin_test.go +++ b/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) diff --git a/internal/factsengine/gatherers/xml.go b/internal/factsengine/gatherers/xml.go index e0e5b8d9..ff96a45f 100644 --- a/internal/factsengine/gatherers/xml.go +++ b/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 } diff --git a/pkg/factsengine/entities/fact_value.go b/pkg/factsengine/entities/fact_value.go index 25587f65..efa1e32f 100644 --- a/pkg/factsengine/entities/fact_value.go +++ b/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 }