fix: Group loop handling and other fixes

* Group loops no longer cause parsing to hang
* "all" group no longer contains self
* Add String() methods to group and hosts
* Fix wrong method call in populateInventoryVars
* Reconsile inventory method is now much more robust

Author: Sergei Gureev

README.md

@@ -33,6 +33,8 @@ type Group struct {
 func GroupMapListValues(mymap map[string]*Group) []*Group
     GroupMapListValues transforms map of Groups into Group list in lexical order
 
+func (group Group) String() string
+
 type Host struct {
         Name   string
         Port   int
@@ -46,6 +48,8 @@ type Host struct {
 func HostMapListValues(mymap map[string]*Host) []*Host
     HostMapListValues transforms map of Hosts into Host list in lexical order
 
+func (host Host) String() string
+
 type InventoryData struct {
         Groups map[string]*Group
         Hosts  map[string]*Host
@@ -82,8 +86,13 @@ func (inventory *InventoryData) Match(m string) []*Host
 
 func (inventory *InventoryData) Reconcile()
     Reconcile ensures inventory basic rules, run after updates After initial
-    inventory file processing, only direct relationships are set This method
-    sets Children and Parents
+    inventory file processing, only direct relationships are set
+
+    This method:
+
+        * (re)sets Children and Parents for hosts and groups
+        * ensures that mandatory groups exist
+        * calculates variables for hosts and groups
 
 ```
 

aini.go

@@ -147,6 +147,14 @@ func (inventory *InventoryData) GroupsToLower() {
 	}
 }
 
+func (group Group) String() string {
+	return group.Name
+}
+
+func (host Host) String() string {
+	return host.Name
+}
+
 func groupMapToLower(groups map[string]*Group, keysOnly bool) map[string]*Group {
 	newGroups := make(map[string]*Group, len(groups))
 	for groupname, group := range groups {

aini_test.go

@@ -78,6 +78,8 @@ func TestGroupStructure(t *testing.T) {
 	assert.Contains(t, v.Groups, "web")
 	assert.Contains(t, v.Groups, "apache")
 	assert.Contains(t, v.Groups, "nginx")
+	assert.Contains(t, v.Groups, "all")
+	assert.Contains(t, v.Groups, "ungrouped")
 
 	assert.Len(t, v.Groups, 5, "Five groups must be present: web, apache, nginx, all, ungrouped")
 
@@ -111,6 +113,8 @@ func TestGroupNotExplicitlyDefined(t *testing.T) {
 
 	assert.Contains(t, v.Groups, "web")
 	assert.Contains(t, v.Groups, "nginx")
+	assert.Contains(t, v.Groups, "all")
+	assert.Contains(t, v.Groups, "ungrouped")
 
 	assert.Len(t, v.Groups, 4, "Four groups must present: web, nginx, all, ungrouped")
 
@@ -126,17 +130,60 @@ func TestGroupNotExplicitlyDefined(t *testing.T) {
 	assert.Empty(t, v.Groups["ungrouped"].Hosts, "Group ungrouped should be empty")
 }
 
+func TestAllGroup(t *testing.T) {
+	v := parseString(t, `
+	host7
+	host5
+
+	[web:children]
+	nginx
+	apache
+
+	[web]
+	host1
+	host2
+
+	[nginx]
+	host1
+	host3
+	host4
+
+	[apache]
+	host5
+	host6
+	`)
+
+	allGroup := v.Groups["all"]
+	assert.NotNil(t, allGroup)
+	assert.Empty(t, allGroup.Parents)
+	assert.NotContains(t, allGroup.Children, "all")
+	assert.Len(t, allGroup.Children, 4)
+	assert.Len(t, allGroup.Hosts, 7)
+	for _, group := range v.Groups {
+		if group.Name == "all" {
+			continue
+		}
+		assert.Contains(t, allGroup.Children, group.Name)
+		assert.Contains(t, group.Parents, allGroup.Name)
+	}
+	for _, host := range v.Hosts {
+		assert.Contains(t, allGroup.Hosts, host.Name)
+		assert.Contains(t, host.Groups, allGroup.Name)
+
+	}
+}
+
 func TestHostExpansionFullNumericPattern(t *testing.T) {
 	v := parseString(t, `
 	host-[001:015:3]-web:23
 	`)
 
-	assert.Len(t, v.Hosts, 5)
 	assert.Contains(t, v.Hosts, "host-001-web")
 	assert.Contains(t, v.Hosts, "host-004-web")
 	assert.Contains(t, v.Hosts, "host-007-web")
 	assert.Contains(t, v.Hosts, "host-010-web")
 	assert.Contains(t, v.Hosts, "host-013-web")
+	assert.Len(t, v.Hosts, 5)
 
 	for _, host := range v.Hosts {
 		assert.Equalf(t, 23, host.Port, "%s port is set", host.Name)
@@ -148,46 +195,46 @@ func TestHostExpansionFullAlphabeticPattern(t *testing.T) {
 	host-[a:o:3]-web
 	`)
 
-	assert.Len(t, v.Hosts, 5)
 	assert.Contains(t, v.Hosts, "host-a-web")
 	assert.Contains(t, v.Hosts, "host-d-web")
 	assert.Contains(t, v.Hosts, "host-g-web")
 	assert.Contains(t, v.Hosts, "host-j-web")
 	assert.Contains(t, v.Hosts, "host-m-web")
+	assert.Len(t, v.Hosts, 5)
 }
 
 func TestHostExpansionShortNumericPattern(t *testing.T) {
 	v := parseString(t, `
 	host-[:05]-web
 	`)
-	assert.Len(t, v.Hosts, 6)
 	assert.Contains(t, v.Hosts, "host-00-web")
 	assert.Contains(t, v.Hosts, "host-01-web")
 	assert.Contains(t, v.Hosts, "host-02-web")
 	assert.Contains(t, v.Hosts, "host-03-web")
 	assert.Contains(t, v.Hosts, "host-04-web")
 	assert.Contains(t, v.Hosts, "host-05-web")
+	assert.Len(t, v.Hosts, 6)
 }
 
 func TestHostExpansionShortAlphabeticPattern(t *testing.T) {
 	v := parseString(t, `
 	host-[a:c]-web
 	`)
-	assert.Len(t, v.Hosts, 3)
 	assert.Contains(t, v.Hosts, "host-a-web")
 	assert.Contains(t, v.Hosts, "host-b-web")
 	assert.Contains(t, v.Hosts, "host-c-web")
+	assert.Len(t, v.Hosts, 3)
 }
 
 func TestHostExpansionMultiplePatterns(t *testing.T) {
 	v := parseString(t, `
 	host-[1:2]-[a:b]-web
 	`)
-	assert.Len(t, v.Hosts, 4)
 	assert.Contains(t, v.Hosts, "host-1-a-web")
 	assert.Contains(t, v.Hosts, "host-1-b-web")
 	assert.Contains(t, v.Hosts, "host-2-a-web")
 	assert.Contains(t, v.Hosts, "host-2-b-web")
+	assert.Len(t, v.Hosts, 4)
 }
 
 func TestVariablesPriority(t *testing.T) {
@@ -324,6 +371,27 @@ func TestGroupsAndHostsToLower(t *testing.T) {
 	assert.Contains(t, v.Groups["tomcat"].Hosts, "tomcat-1")
 }
 
+func TestGroupLoops(t *testing.T) {
+	v := parseString(t, `
+	[group1]
+	host1
+
+	[group1:children]
+	group2
+
+	[group2:children]
+	group1
+	`)
+
+	assert.Contains(t, v.Groups, "group1")
+	assert.Contains(t, v.Groups, "group2")
+	assert.Contains(t, v.Groups["group1"].Parents, "all")
+	assert.Contains(t, v.Groups["group1"].Parents, "group2")
+	assert.NotContains(t, v.Groups["group1"].Parents, "group1")
+	assert.Len(t, v.Groups["group1"].Parents, 2)
+	assert.Contains(t, v.Groups["group2"].Parents, "group1")
+}
+
 func TestVariablesEscaping(t *testing.T) {
 	v := parseString(t, `
 	host ansible_ssh_common_args="-o ProxyCommand='ssh -W %h:%p somehost'" other_var_same_value="-o ProxyCommand='ssh -W %h:%p somehost'" # comment

inventory.go

@@ -2,30 +2,89 @@ package aini
 
 // Inventory-related helper methods
 
-// Reconcile ensures inventory basic rules, run after updates
-// After initial inventory file processing, only direct relationships are set
-// This method sets Children and Parents
+// Reconcile ensures inventory basic rules, run after updates.
+// After initial inventory file processing, only direct relationships are set.
+//
+// This method:
+//   * (re)sets Children and Parents for hosts and groups
+//   * ensures that mandatory groups exist
+//   * calculates variables for hosts and groups
 func (inventory *InventoryData) Reconcile() {
+	// Clear all computed data
+	for _, host := range inventory.Hosts {
+		host.clearData()
+	}
+	// a group can be empty (with no hosts in it), so the previous method will not clean it
+	// on the other hand, a group could have been attached to a host by a user, but not added to the inventory.Groups map
+	// so it's safer just to clean everything
+	for _, group := range inventory.Groups {
+		group.clearData(make(map[string]struct{}, len(inventory.Groups)))
+	}
+
 	allGroup := inventory.getOrCreateGroup("all")
-	allGroup.Hosts = inventory.Hosts
-	allGroup.Children = inventory.Groups
+	ungroupedGroup := inventory.getOrCreateGroup("ungrouped")
+	ungroupedGroup.directParents[allGroup.Name] = allGroup
+
+	// First, ensure that inventory.Groups contains all the groups
+	for _, host := range inventory.Hosts {
+		for _, group := range host.directGroups {
+			inventory.Groups[group.Name] = group
+			for _, ancestor := range group.getAncestors() {
+				inventory.Groups[ancestor.Name] = ancestor
+			}
+		}
+	}
+
+	// Calculate intergroup relationships
+	for _, group := range inventory.Groups {
+		group.directParents[allGroup.Name] = allGroup
+		for _, ancestor := range group.getAncestors() {
+			group.Parents[ancestor.Name] = ancestor
+			ancestor.Children[group.Name] = group
+		}
+	}
 
+	// Now set hosts for groups and groups for hosts
 	for _, host := range inventory.Hosts {
+		host.Groups[allGroup.Name] = allGroup
 		for _, group := range host.directGroups {
 			group.Hosts[host.Name] = host
 			host.Groups[group.Name] = group
-			group.directParents[allGroup.Name] = allGroup
-			for _, ancestor := range group.getAncestors() {
-				group.Parents[ancestor.Name] = ancestor
-				ancestor.Children[group.Name] = group
-				ancestor.Hosts[host.Name] = host
-				host.Groups[ancestor.Name] = ancestor
+			for _, parent := range group.Parents {
+				group.Parents[parent.Name] = parent
+				parent.Children[group.Name] = group
+				parent.Hosts[host.Name] = host
+				host.Groups[parent.Name] = parent
 			}
 		}
 	}
 	inventory.reconcileVars()
 }
 
+func (host *Host) clearData() {
+	host.Groups = make(map[string]*Group)
+	host.Vars = make(map[string]string)
+	for _, group := range host.directGroups {
+		group.clearData(make(map[string]struct{}, len(host.Groups)))
+	}
+}
+
+func (group *Group) clearData(visited map[string]struct{}) {
+	if _, ok := visited[group.Name]; ok {
+		return
+	}
+	group.Hosts = make(map[string]*Host)
+	group.Parents = make(map[string]*Group)
+	group.Children = make(map[string]*Group)
+	group.Vars = make(map[string]string)
+	group.allInventoryVars = nil
+	group.allFileVars = nil
+	visited[group.Name] = struct{}{}
+	for _, parent := range group.directParents {
+		parent.clearData(visited)
+	}
+}
+
 // getOrCreateGroup return group from inventory if exists or creates empty Group with given name
 func (inventory *InventoryData) getOrCreateGroup(groupName string) *Group {
 	if group, ok := inventory.Groups[groupName]; ok {
@@ -68,13 +127,24 @@ func (inventory *InventoryData) getOrCreateHost(hostName string) *Host {
 // getAncestors returns all Ancestors of a given group in level order
 func (group *Group) getAncestors() []*Group {
 	result := make([]*Group, 0)
+	if len(group.directParents) == 0 {
+		return result
+	}
+	visited := map[string]struct{}{group.Name: {}}
 
-	for queue := []*Group{group}; ; {
+	for queue := GroupMapListValues(group.directParents); ; {
 		group := queue[0]
-		parentList := GroupMapListValues(group.directParents)
-		result = append(result, parentList...)
 		copy(queue, queue[1:])
 		queue = queue[:len(queue)-1]
+		if _, ok := visited[group.Name]; ok {
+			if len(queue) == 0 {
+				return result
+			}
+			continue
+		}
+		visited[group.Name] = struct{}{}
+		parentList := GroupMapListValues(group.directParents)
+		result = append(result, group)
 		queue = append(queue, parentList...)
 
 		if len(queue) == 0 {

vars.go

@@ -19,7 +19,7 @@ func (inventory *InventoryData) AddVars(path string) error {
 	return inventory.doAddVars(path, false)
 }
 
-// AddVarsLowerCased does the same as AddVars, but converts hostnames and groups name to lowercase
+// AddVarsLowerCased does the same as AddVars, but converts hostnames and groups name to lowercase.
 // Use this function if you've executed `inventory.HostsToLower` or `inventory.GroupsToLower`
 func (inventory *InventoryData) AddVarsLowerCased(path string) error {
 	return inventory.doAddVars(path, true)
@@ -176,7 +176,7 @@ func (group *Group) populateInventoryVars() {
 	}
 	group.allInventoryVars = make(map[string]string)
 	for _, parent := range GroupMapListValues(group.directParents) {
-		parent.populateFileVars()
+		parent.populateInventoryVars()
 		addValues(group.allInventoryVars, parent.allInventoryVars)
 	}
 	addValues(group.allInventoryVars, group.inventoryVars)