Fix identifier detection in named-entry lists

Rework `getIdentifierFromNamedLists` to check not just the number of keys in
the list, but more precisely the number of unique entries. This is to detect
lists that have a `name` key in each entry that does not have unique values.

Improve flaky test where the path style rendering option was not reset
correctly for each new test section.

This commit should fix #38.

assets/issues/issue-38/from.yml

@@ -0,0 +1,51 @@
+---
+counters:
+- addr: http://127.0.0.1:14823/debug/vars
+  name: ingress
+  source_id: forwarder_agent
+  template: "{{.ForwarderAgent.IngressV2}}"
+  tags:
+    metric_version: "2.0"
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.Dropped}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.DroppedEgressV2}}"
+  tags:
+    origin: loggregator.metron
+    direction: egress
+    metric_version: "2.0"
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.DroppedIngressV2}}"
+  tags:
+    origin: loggregator.metron
+    direction: ingress
+    metric_version: "2.0"
+
+- addr: http://127.0.0.1:14824/debug/vars
+  name: egress
+  template: "{{.Agent.Egress}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: egress
+  template: "{{.Agent.EgressV2}}"
+  tags:
+    origin: loggregator.metron
+    metric_version: "2.0"
+
+- addr: http://127.0.0.1:14824/debug/vars
+  name: ingress
+  template: "{{.Agent.Ingress}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: ingress
+  template: "{{.Agent.IngressV2}}"
+  tags:
+    origin: loggregator.metron
+    metric_version: "2.0"

assets/issues/issue-38/to.yml

@@ -0,0 +1,51 @@
+---
+counters:
+- addr: http://127.0.0.1:14823/debug/vars
+  name: ingress
+  source_id: forwarder_agent
+  template: "{{.ForwarderAgent.IngressV2}}"
+  tags:
+    metric_version: "2.0"
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.Dropped}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.DroppedEgressV2}}"
+  tags:
+    origin: loggregator.metron
+    direction: egress
+    metric_version: "2.0"
+- addr: http://127.0.0.1:14824/debug/vars
+  name: dropped
+  template: "{{.Agent.DroppedIngressV2}}"
+  tags:
+    origin: loggregator.metron
+    direction: ingress
+    metric_version: "2.0"
+
+- addr: http://127.0.0.1:14824/debug/vars
+  name: egress
+  template: "{{.Agent.Egress}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: egress
+  template: "{{.Agent.EgressV2}}"
+  tags:
+    origin: loggregator.metron
+    metric_version: "2.0"
+
+- addr: http://127.0.0.1:14824/debug/vars
+  name: ingress
+  template: "{{.Agent.Ingress}}"
+  tags:
+    origin: loggregator.metron
+- addr: http://127.0.0.1:14824/debug/vars
+  name: ingress
+  template: "{{.Agent.IngressV2}}"
+  tags:
+    origin: loggregator.metron
+    metric_version: "2.0"

go.sum

@@ -38,9 +38,11 @@ github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh
 github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/ginkgo v1.7.0 h1:WSHQ+IS43OoUrWtD1/bbclrwK8TTH5hzp+umCiuxHgs=
 github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
+github.com/onsi/ginkgo v1.8.0 h1:VkHVNpR4iVnU8XQR6DBm8BqYjN7CRzw+xKUbVVbbW9w=
 github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/gomega v1.4.3 h1:RE1xgDvH7imwFD45h+u2SgIfERHlS2yNG4DObb5BSKU=
 github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
+github.com/onsi/gomega v1.5.0 h1:izbySO9zDPmjJ8rDjLvkA2zJHIo+HkYXHnf7eN7SSyo=
 github.com/onsi/gomega v1.5.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
 github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
 github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=

pkg/v1/dyff/compare_test.go

@@ -710,5 +710,20 @@ b: bar
 				}
 			})
 		})
+
+		Context("checking known issues of compare", func() {
+			It("should not return order change differences in case the named-entry list does not have unique identifiers", func() {
+				from, to, err := ytbx.LoadFiles("../../../assets/issues/issue-38/from.yml", "../../../assets/issues/issue-38/to.yml")
+				Expect(err).To(BeNil())
+				Expect(from).ToNot(BeNil())
+				Expect(to).ToNot(BeNil())
+
+				results, err := CompareInputFiles(from, to)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(results).ToNot(BeNil())
+
+				Expect(len(results.Diffs)).To(BeEquivalentTo(0))
+			})
+		})
 	})
 })

pkg/v1/dyff/core.go

@@ -182,7 +182,7 @@ func compareLists(path ytbx.Path, from []interface{}, to []interface{}) ([]Diff,
 		return []Diff{}, nil
 	}
 
-	if identifier := getIdentifierFromNamedLists(from, to); identifier != "" {
+	if identifier, err := getIdentifierFromNamedLists(from, to); err == nil {
 		return compareNamedEntryLists(path, identifier, from, to)
 	}
 
@@ -474,40 +474,50 @@ func GetIdentifierFromNamedList(list []interface{}) string {
 	return ""
 }
 
-func getIdentifierFromNamedLists(listA, listB []interface{}) string {
-	createKeyCountMap := func(list []interface{}) map[interface{}]int {
-		result := map[interface{}]int{}
+func getIdentifierFromNamedLists(listA, listB []interface{}) (string, error) {
+	createKeyCountMap := func(list []interface{}) (map[interface{}]map[uint64]struct{}, error) {
+		result := map[interface{}]map[uint64]struct{}{}
 		for _, entry := range list {
 			switch mapslice := entry.(type) {
 			case yaml.MapSlice:
 				for _, mapitem := range mapslice {
-					if _, ok := result[mapitem.Key]; !ok {
-						result[mapitem.Key] = 0
+					hash, err := calcHash(mapitem.Value)
+					if err != nil {
+						return nil, err
 					}
 
-					result[mapitem.Key]++
+					if _, found := result[mapitem.Key]; !found {
+						result[mapitem.Key] = map[uint64]struct{}{}
+					}
+
+					result[mapitem.Key][hash] = struct{}{}
 				}
 			}
 		}
 
-		return result
+		return result, nil
 	}
 
-	listALength := len(listA)
-	listBLength := len(listB)
-	counterA := createKeyCountMap(listA)
-	counterB := createKeyCountMap(listB)
+	counterA, err := createKeyCountMap(listA)
+	if err != nil {
+		return "", err
+	}
+
+	counterB, err := createKeyCountMap(listB)
+	if err != nil {
+		return "", err
+	}
 
 	// Check for the usual suspects: name, key, and id
 	for _, identifier := range []string{"name", "key", "id"} {
-		if countA, okA := counterA[identifier]; okA && countA == listALength {
-			if countB, okB := counterB[identifier]; okB && countB == listBLength {
-				return identifier
+		if countA, okA := counterA[identifier]; okA && len(countA) == len(listA) {
+			if countB, okB := counterB[identifier]; okB && len(countB) == len(listB) {
+				return identifier, nil
 			}
 		}
 	}
 
-	return ""
+	return "", fmt.Errorf("unable to find a key that can serve as an unique identifier")
 }
 
 func getNonStandardIdentifierFromNamedLists(listA, listB []interface{}) string {

pkg/v1/dyff/core_suite_test.go

@@ -51,7 +51,10 @@ var _ = BeforeSuite(func() {
 	FixedTerminalWidth = 80
 })
 
-func compareAgainstExpected(fromPath string, toPath string, expectedPath string) {
+func compareAgainstExpected(fromPath string, toPath string, expectedPath string, useGoPatch bool) {
+	tmp := UseGoPatchPaths
+	UseGoPatchPaths = useGoPatch
+
 	from, to, err := ytbx.LoadFiles(fromPath, toPath)
 	Expect(err).To(BeNil())
 
@@ -75,6 +78,8 @@ func compareAgainstExpected(fromPath string, toPath string, expectedPath string)
 	writer.Flush()
 
 	Expect(string(expected)).To(BeIdenticalTo(buffer.String()))
+
+	UseGoPatchPaths = tmp
 }
 
 func yml(input string) yaml.MapSlice {

pkg/v1/dyff/output_human_test.go

@@ -105,18 +105,20 @@ input: |+
 		It("should show a binary data difference in hex dump style", func() {
 			compareAgainstExpected("../../../assets/binary/from.yml",
 				"../../../assets/binary/to.yml",
-				"../../../assets/binary/dyff.expected")
+				"../../../assets/binary/dyff.expected",
+				false)
 		})
 
 		It("should show the testbed results as expected", func() {
 			compareAgainstExpected("../../../assets/testbed/from.yml",
 				"../../../assets/testbed/to.yml",
-				"../../../assets/testbed/expected-dyff-spruce.human")
+				"../../../assets/testbed/expected-dyff-spruce.human",
+				false)
 
-			UseGoPatchPaths = true
 			compareAgainstExpected("../../../assets/testbed/from.yml",
 				"../../../assets/testbed/to.yml",
-				"../../../assets/testbed/expected-dyff-gopatch.human")
+				"../../../assets/testbed/expected-dyff-gopatch.human",
+				true)
 		})
 	})
 
@@ -125,6 +127,10 @@ input: |+
 			ColorSetting = ON
 		})
 
+		AfterEach(func() {
+			ColorSetting = OFF
+		})
+
 		It("should render path with underscores correctly (https://github.com/homeport/dyff/issues/33)", func() {
 			// Please note: The actual error is in the gonvenience package, this test
 			// case exists to verify the issue from with dyff.