hashicorp / terraform-plugin-sdk

Terraform Plugin SDK enables building plugins (providers) to manage any service providers or custom in-house solutions
https://developer.hashicorp.com/terraform/plugin
Mozilla Public License 2.0
433 stars 230 forks source link

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

Open bflad opened 1 year ago

bflad commented 1 year ago

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

bflad commented 1 year ago

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
    }
}