From 4750f0607da0881ece4612b59255254b6ed445a5 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Tue, 22 Aug 2017 18:36:17 -0700
Subject: [PATCH] core: stabilize ResourceAddress.Less results

The implementation of ResourceAddress.Less was flawed because it was only
testing each field in the "less than" direction, and falling through in
cases where an earlier field compared greater than a later one.

Now we test for inequality first as the selector, and only fall through
if the two values for a given field are equal.
---
 terraform/resource_address.go      | 31 +++++++++++++++---------------
 terraform/resource_address_test.go | 20 +++++++++++++++++++
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/terraform/resource_address.go b/terraform/resource_address.go
index 8badca805..353725844 100644
--- a/terraform/resource_address.go
+++ b/terraform/resource_address.go
@@ -362,40 +362,41 @@ func (addr *ResourceAddress) Less(other *ResourceAddress) bool {
 
 	switch {
 
-	case len(addr.Path) < len(other.Path):
-		return true
+	case len(addr.Path) != len(other.Path):
+		return len(addr.Path) < len(other.Path)
 
 	case !reflect.DeepEqual(addr.Path, other.Path):
 		// If the two paths are the same length but don't match, we'll just
 		// cheat and compare the string forms since it's easier than
-		// comparing all of the path segments in turn.
+		// comparing all of the path segments in turn, and lexicographic
+		// comparison is correct for the module path portion.
 		addrStr := addr.String()
 		otherStr := other.String()
 		return addrStr < otherStr
 
-	case addr.Mode == config.DataResourceMode && other.Mode != config.DataResourceMode:
-		return true
+	case addr.Mode != other.Mode:
+		return addr.Mode == config.DataResourceMode
 
-	case addr.Type < other.Type:
-		return true
+	case addr.Type != other.Type:
+		return addr.Type < other.Type
 
-	case addr.Name < other.Name:
-		return true
+	case addr.Name != other.Name:
+		return addr.Name < other.Name
 
-	case addr.Index < other.Index:
+	case addr.Index != other.Index:
 		// Since "Index" is -1 for an un-indexed address, this also conveniently
 		// sorts unindexed addresses before indexed ones, should they both
 		// appear for some reason.
-		return true
+		return addr.Index < other.Index
 
-	case other.InstanceTypeSet && !addr.InstanceTypeSet:
-		return true
+	case addr.InstanceTypeSet != other.InstanceTypeSet:
+		return !addr.InstanceTypeSet
 
-	case addr.InstanceType < other.InstanceType:
+	case addr.InstanceType != other.InstanceType:
 		// InstanceType is actually an enum, so this is just an arbitrary
 		// sort based on the enum numeric values, and thus not particularly
 		// meaningful.
-		return true
+		return addr.InstanceType < other.InstanceType
 
 	default:
 		return false
diff --git a/terraform/resource_address_test.go b/terraform/resource_address_test.go
index a2f9cb9ff..0fe561528 100644
--- a/terraform/resource_address_test.go
+++ b/terraform/resource_address_test.go
@@ -1233,6 +1233,11 @@ func TestResourceAddressLess(t *testing.T) {
 			"module.baz.foo.bar",
 			true,
 		},
+		{
+			"module.baz.foo.bar",
+			"zzz.bar", // would sort after "module" in lexicographical sort
+			false,
+		},
 		{
 			"module.baz.foo.bar",
 			"module.baz.foo.bar",
@@ -1243,6 +1248,11 @@ func TestResourceAddressLess(t *testing.T) {
 			"module.boz.foo.bar",
 			true,
 		},
+		{
+			"module.boz.foo.bar",
+			"module.baz.foo.bar",
+			false,
+		},
 		{
 			"a.b",
 			"b.c",
@@ -1253,11 +1263,21 @@ func TestResourceAddressLess(t *testing.T) {
 			"a.c",
 			true,
 		},
+		{
+			"c.b",
+			"b.c",
+			false,
+		},
 		{
 			"a.b[9]",
 			"a.b[10]",
 			true,
 		},
+		{
+			"b.b[9]",
+			"a.b[10]",
+			false,
+		},
 		{
 			"a.b",
 			"a.b.deposed",