helper/resource: Checking Collection Attribute Length Ignores Missing/Null Attributes

[bflad]

trafficstars

SDK version

v2.24.0

Relevant provider source code

// sdk resource logic
d.Set("list", nil) // schema.TypeList
d.Set("map", nil) // schema.TypeMap
d.Set("set", nil) // schema.TypeSet

// framework resource logic
ListNull(/* ... */)
MapNull(/* ... */)
SetNull(/* ... */)

// acceptance testing TestCheckFunc
resource.TestCheckResourceAttr("examplecloud_thing.test", "list.#", "0")
resource.TestCheckResourceAttr("examplecloud_thing.test", "map.%", "0")
resource.TestCheckResourceAttr("examplecloud_thing.test", "set.#", "0")

Terraform Configuration Files

resource "examplecloud_thing" "example" {
  # attributes not configured
}

Expected Behavior

Test checks fail that the attribute doesn't exist with the framework code.

Actual Behavior

Test checks pass with the framework code. It specifically ignores returning an error in this situation:

https://github.com/hashicorp/terraform-plugin-sdk/blob/b5b7dd0ab159303da4a64c94d64aeaea884c2a23/helper/resource/testing.go#L997-L1005

This rule was intentionally added because provider developers were already depending on this type of testing logic years ago:

https://github.com/hashicorp/terraform-plugin-sdk/commit/57c8f9629a2fb72ab75f7ff1d702bbcfee763ba4

Steps to Reproduce

1. TF_ACC=1 go test -count=1 -v ./...

References

• https://github.com/hashicorp/terraform-provider-aws/pull/27221
• https://github.com/hashicorp/terraform-provider-aws/issues/27372
• https://github.com/hashicorp/terraform-provider-aws/pull/27377

[bflad]

Workarounds

Either of these options should workaround the current behavior and yield failing checks:

1. Use resource.TestCheckResourceAttrSet("...", "....%") before or resource.TestCheckNoResourceAttr("...", "....%") in place of the 0 size check.
2. Extract the map values from each flatmap state and compare for differences

func TestAccThingDataSource(t *testing.T) {
	var value1, value2 map[string]string

	resource.ParallelTest(t, resource.TestCase{
		Steps: []resource.TestStep{
			{
				// ... other fields
				Check: ComposeAggregateTestCheckFunc(
					testExtractResourceAttrMap("...", "map", &value1),
				),
			},
			{
				// ... other fields
				Check: ComposeAggregateTestCheckFunc(
					testExtractResourceAttrMap("...", "map", &value2),
					func(s *terraform.State) error {
						// this uses go-cmp, but reflect.DeepEqual() is probably fine too
						if diff := cmp.Diff(value1, value2); diff != "" {
							return fmt.Errorf("attribute value difference: %s", diff)
						}

						return nil
					},
				),
			},
		},
	})
}

func testExtractResourceAttrMap(resourceName string, attributeName string, attributeValue *map[string]string) TestCheckFunc {
	return func(s *terraform.State) error {
		rs, ok := s.RootModule().Resources[resourceName]

		if !ok {
			return fmt.Errorf("resource name %s not found in state", resourceName)
		}

		if _, ok := rs.Primary.Attributes[attributeName]; ok {
			return fmt.Errorf("attribute name %s found, but is not a map attribute in state", attributeName)
		}

		// Everything in a map will be in the flatmap with the attribute name,
		// a period, then either the size sigil or a string key.
		attributeValuePathPrefix := attributeName + "."
		attributePathSize := attributeValuePathPrefix + "%"

		sizeStr, ok := rs.Primary.Attributes[attributePathSize]

		if !ok {
			// attribute not found
			*attributeValue = nil

			return nil
		}

		size, err := strconv.Atoi(sizeStr)

		if err != nil {
			return fmt.Errorf("unable to convert map size %s to integer: %s", sizeStr, err)
		}

		m := make(map[string]string, size)

		for attributePath, attributePathValue := range rs.Primary.Attributes {
			// Ignore
			if !strings.HasPrefix(attributePath, attributeValuePathPrefix) {
				continue
			}

			if attributePath == attributePathSize {
				continue
			}

			key := strings.TrimPrefix(attributePath, attributeValuePathPrefix)

			m[key] = attributePathValue
		}

		*attributeValue = m

		return nil
	}
}