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.
This commit is contained in:
Martin Atkins 2017-08-22 18:36:17 -07:00
parent 4739cb6597
commit 4750f0607d
2 changed files with 36 additions and 15 deletions

View File

@ -362,40 +362,41 @@ func (addr *ResourceAddress) Less(other *ResourceAddress) bool {
switch { switch {
case len(addr.Path) < len(other.Path): case len(addr.Path) != len(other.Path):
return true return len(addr.Path) < len(other.Path)
case !reflect.DeepEqual(addr.Path, other.Path): case !reflect.DeepEqual(addr.Path, other.Path):
// If the two paths are the same length but don't match, we'll just // 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 // 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() addrStr := addr.String()
otherStr := other.String() otherStr := other.String()
return addrStr < otherStr return addrStr < otherStr
case addr.Mode == config.DataResourceMode && other.Mode != config.DataResourceMode: case addr.Mode != other.Mode:
return true return addr.Mode == config.DataResourceMode
case addr.Type < other.Type: case addr.Type != other.Type:
return true return addr.Type < other.Type
case addr.Name < other.Name: case addr.Name != other.Name:
return true 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 // Since "Index" is -1 for an un-indexed address, this also conveniently
// sorts unindexed addresses before indexed ones, should they both // sorts unindexed addresses before indexed ones, should they both
// appear for some reason. // appear for some reason.
return true return addr.Index < other.Index
case other.InstanceTypeSet && !addr.InstanceTypeSet: case addr.InstanceTypeSet != other.InstanceTypeSet:
return true return !addr.InstanceTypeSet
case addr.InstanceType < other.InstanceType: case addr.InstanceType != other.InstanceType:
// InstanceType is actually an enum, so this is just an arbitrary // InstanceType is actually an enum, so this is just an arbitrary
// sort based on the enum numeric values, and thus not particularly // sort based on the enum numeric values, and thus not particularly
// meaningful. // meaningful.
return true return addr.InstanceType < other.InstanceType
default: default:
return false return false

View File

@ -1233,6 +1233,11 @@ func TestResourceAddressLess(t *testing.T) {
"module.baz.foo.bar", "module.baz.foo.bar",
true, true,
}, },
{
"module.baz.foo.bar",
"zzz.bar", // would sort after "module" in lexicographical sort
false,
},
{ {
"module.baz.foo.bar", "module.baz.foo.bar",
"module.baz.foo.bar", "module.baz.foo.bar",
@ -1243,6 +1248,11 @@ func TestResourceAddressLess(t *testing.T) {
"module.boz.foo.bar", "module.boz.foo.bar",
true, true,
}, },
{
"module.boz.foo.bar",
"module.baz.foo.bar",
false,
},
{ {
"a.b", "a.b",
"b.c", "b.c",
@ -1253,11 +1263,21 @@ func TestResourceAddressLess(t *testing.T) {
"a.c", "a.c",
true, true,
}, },
{
"c.b",
"b.c",
false,
},
{ {
"a.b[9]", "a.b[9]",
"a.b[10]", "a.b[10]",
true, true,
}, },
{
"b.b[9]",
"a.b[10]",
false,
},
{ {
"a.b", "a.b",
"a.b.deposed", "a.b.deposed",