Inconsistency in setting state values

[akaRem]

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk",
  "Version": "v1.10.0"
}

Relevant provider source code

package provider

import (
	"testing"

	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
	"github.com/hashicorp/terraform-plugin-sdk/terraform"
)

func TestTerraform_still_have_bug_with_nested_structs_and_nil(t *testing.T) {
	testResourceReadFn := func(d *schema.ResourceData, m interface{}) error {
		d.SetId("0")
		d.Set("primitive_str_set_to_nil", nil)
		d.Set("primitive_str_c_set_to_nil", nil)
		d.Set("obj_set", []map[string]interface{}{{
			"primitive_str_set_to_nil":   nil,
			"primitive_str_c_set_to_nil": nil,
		}})
		return nil
	}

	testResource := func() *schema.Resource {
		return &schema.Resource{
			Schema: map[string]*schema.Schema{
				"primitive_str_unset": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"primitive_str_c_unset": {
					Type:     schema.TypeString,
					Optional: true,
					Computed: true,
				},
				"primitive_str_set_to_nil": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"primitive_str_c_set_to_nil": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"obj_unset": {
					Type:     schema.TypeList,
					Optional: true,
					Elem:     &schema.Resource{},
				},
				"obj_c_unset": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_set_to_nil": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_c_set_to_nil": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_set": {
					Type:     schema.TypeList,
					Optional: true,
					MaxItems: 1,
					Computed: true,
					Elem: &schema.Resource{
						Schema: map[string]*schema.Schema{
							"primitive_str_unset": {
								Type:     schema.TypeString,
								Optional: true,
							},
							"primitive_str_c_unset": {
								Type:     schema.TypeString,
								Optional: true,
								Computed: true,
							},
							"primitive_str_set_to_nil": {
								Type:     schema.TypeString,
								Optional: true,
							},
							"primitive_str_c_set_to_nil": {
								Type:     schema.TypeString,
								Optional: true,
							},
						},
					},
				},
			},
			Read: testResourceReadFn,
		}
	}

	testProvider := func() *schema.Provider {
		return &schema.Provider{
			DataSourcesMap: map[string]*schema.Resource{
				"test_resource": testResource(),
			},
		}
	}

	providers := map[string]terraform.ResourceProvider{
		"test": testProvider(),
	}

	resource.Test(t, resource.TestCase{
		Providers: providers,
		Steps: []resource.TestStep{
			{
				Config: `
provider "test" {
}
data "test_resource" "t" {
}`,
				Check: resource.ComposeAggregateTestCheckFunc(
					// Expectations:
					// Values which were not set should not be present in state
					// Values which were set to nil should not be present in state
					// Acual behaviour:

					// The primitive values which were not set are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_unset"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_c_unset"),

					// BUG:
					// The primitive values which were set to nil are converted to empty value
					resource.TestCheckResourceAttr("data.test_resource.t", "primitive_str_set_to_nil", ""),
					resource.TestCheckResourceAttr("data.test_resource.t", "primitive_str_c_set_to_nil", ""),

					// The objects which were not set are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_unset.#"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_c_unset.#"),

					// The objects which were set to nil are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set_to_nil.#"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_c_set_to_nil.#"),

					// The object which is set is present
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.#", "1"),

					// BUG:
					// The primitive values which were not set are set to empty in nested structure
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_unset", ""),
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_c_unset", ""),

					// BUG:
					// The primitive values which were set to nil are converted to empty value
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_set_to_nil", ""),
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_c_set_to_nil", ""),
				),
			},
		},
	})
}

Terraform Configuration Files

...

Debug Output

Expected Behavior

TF should store values in state consistently, no matter where they appear.
"not-set" and "nil" values should not be present in terraform state
"not-set" and "nil" values should not be converted to empty values of corresponding type

place	type	not set	set to nil
root	string	not present	not present
root	object	not present	not present
nested object	string	not present	not present

Actual Behavior

There is an inconsistency in how TF stores empty values in state see attached test case.

place	type	not set	set to nil
root	string	not present	""
root	object	not present	not present
nested object	string	""	""

Steps to Reproduce

adopt and run attached test

References

[paultyng]

I tried this test against master (the v2 candidate) and it passes. Should it have failed?

[akaRem]

@paultyng yes, it passes according to actual behaviour

here is example that should fail:

package provider

import (
	"testing"

	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
	"github.com/hashicorp/terraform-plugin-sdk/terraform"
)

// Test for https://github.com/hashicorp/terraform-plugin-sdk/issues/413
func TestTerraform_still_have_bug_413(t *testing.T) {
	testResourceReadFn := func(d *schema.ResourceData, m interface{}) error {
		d.SetId("0")
		d.Set("primitive_str_set_to_nil", nil)
		d.Set("primitive_str_c_set_to_nil", nil)
		d.Set("obj_set", []map[string]interface{}{{
			"primitive_str_set_to_nil":   nil,
			"primitive_str_c_set_to_nil": nil,
		}})
		return nil
	}

	testResource := func() *schema.Resource {
		return &schema.Resource{
			Schema: map[string]*schema.Schema{
				"primitive_str_unset": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"primitive_str_c_unset": {
					Type:     schema.TypeString,
					Optional: true,
					Computed: true,
				},
				"primitive_str_set_to_nil": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"primitive_str_c_set_to_nil": {
					Type:     schema.TypeString,
					Optional: true,
				},
				"obj_unset": {
					Type:     schema.TypeList,
					Optional: true,
					Elem:     &schema.Resource{},
				},
				"obj_c_unset": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_set_to_nil": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_c_set_to_nil": {
					Type:     schema.TypeList,
					Optional: true,
					Computed: true,
					Elem:     &schema.Resource{},
				},
				"obj_set": {
					Type:     schema.TypeList,
					Optional: true,
					MaxItems: 1,
					Computed: true,
					Elem: &schema.Resource{
						Schema: map[string]*schema.Schema{
							"primitive_str_unset": {
								Type:     schema.TypeString,
								Optional: true,
							},
							"primitive_str_c_unset": {
								Type:     schema.TypeString,
								Optional: true,
								Computed: true,
							},
							"primitive_str_set_to_nil": {
								Type:     schema.TypeString,
								Optional: true,
							},
							"primitive_str_c_set_to_nil": {
								Type:     schema.TypeString,
								Optional: true,
							},
						},
					},
				},
			},
			Read: testResourceReadFn,
		}
	}

	testProvider := func() *schema.Provider {
		return &schema.Provider{
			DataSourcesMap: map[string]*schema.Resource{
				"test_resource": testResource(),
			},
		}
	}

	providers := map[string]terraform.ResourceProvider{
		"test": testProvider(),
	}

	resource.Test(t, resource.TestCase{
		Providers: providers,
		Steps: []resource.TestStep{
			{
				Config: `
provider "test" {
}
data "test_resource" "t" {
}`,
				Check: resource.ComposeAggregateTestCheckFunc(
					// Expectations:
					// Values which were not set should not be present in state
					// Values which were set to nil should not be present in state
					// Acual behaviour:

					// The primitive values which were not set are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_unset"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_c_unset"),

					// The primitive values which were set to nil are converted to empty value
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_set_to_nil"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "primitive_str_c_set_to_nil"),

					// The objects which were not set are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_unset.#"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_c_unset.#"),

					// The objects which were set to nil are missing in root namespace
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set_to_nil.#"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_c_set_to_nil.#"),

					// The object which is set is present
					resource.TestCheckResourceAttr("data.test_resource.t", "obj_set.#", "1"),

					// The primitive values which were not set are set to empty in nested structure
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_unset"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_c_unset"),

					// The primitive values which were set to nil are converted to empty value
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_set_to_nil"),
					resource.TestCheckNoResourceAttr("data.test_resource.t", "obj_set.0.primitive_str_c_set_to_nil"),
				),
			},
		},
	})
}

fragment w/ errors

    TestTerraform_still_have_bug_413: testing.go:673: Step 0 error: Check failed: 6 errors occurred:
        	* Check 3/13 error: data.test_resource.t: Attribute 'primitive_str_set_to_nil' found when not expected
        	* Check 4/13 error: data.test_resource.t: Attribute 'primitive_str_c_set_to_nil' found when not expected
        	* Check 10/13 error: data.test_resource.t: Attribute 'obj_set.0.primitive_str_unset' found when not expected
        	* Check 11/13 error: data.test_resource.t: Attribute 'obj_set.0.primitive_str_c_unset' found when not expected
        	* Check 12/13 error: data.test_resource.t: Attribute 'obj_set.0.primitive_str_set_to_nil' found when not expected
        	* Check 13/13 error: data.test_resource.t: Attribute 'obj_set.0.primitive_str_c_set_to_nil' found when not expected

[paultyng]

Got it, yes this still happens in v2, just ran this test against it.

I think there are a number of things at play here, but just some context, the Terraform type system and the wire protocol for providers does differentiate between null, an empty value (like ""), and unknown (which is used to represent values at plan/validate time that may still need to be calculated, typically resulting from expressions). Unfortunately, the SDK was built for the Terraform 0.11 and earlier type system, and is shimmed in to the new protocol (the v1 SDK also still supports Terraform 0.11). Due to this though its not as easy to differentiate in the SDK as it exists today between these values. There are also different ways nested information is treated by core (blocks vs attributes and object types, etc) that cause the information not to translate correctly because the SDK does not mirror these concepts. The v2 SDK (current master) removes support for TF 0.11 so that we can start to write things specifically for only the new type system, but even so, many providers are written to the existing behavior, and so its very difficult to make changes in areas like this that may actually be depended upon by existing implementations since the behavior was never fully specced out.

A very long response to essentially say unfortunately this will probably remain the case using the existing functions, but that being said, we have plans for adding opt-in support for direct access to things like the new type system (see #261 ) and other things in v2, which should make this simpler going forward.

[akaRem]

Thank you for clarification.

I understand that It will be hard to fix without breaking all the libraries and providers depending on SDK.

I'm currently working with a quite inconvenient API where null and "empty" values like "" or 0 mean different things. Basically null means something like "disabled" or "unspecified" and ""/0 literally means ""/0. This is especially painful for things like TTLs where 0 is completely invalid thing.

Do you have any advise or general recommendation on how to handle such cases?

For now I see 3 options

• add extra field like foo_enabled and set nil manually in API DTOs or
• store null as a value that's very unlikely to be used in API any time soon (like TTL=-1 or like TTL=0) and also convert it to nil manually.
• store state as completely flat key-value list w/o nested structures to preserve nil values by not setting keys

All these options have some inconvenience and risk of errors (accordingly to prev. list):

• extra verbosity and keys not specified in API
• providing HCL values which you never saw in API and invalid according to docs or
• assortment of single-ranked fields which are hard to read/follow
  How do other people handle this?

May I also ask you to keep this bug open or document this behaviour for people who will have the same problem as I have?

[paultyng]

Yeah, I think historically I typically have reached for your second option there, using a sentinel value for "auto" or -1 or something but I've seen all those in various providers so I think it just depends on what you feel comfortable maintaining for your users and what is an acceptable divergence from the upstream API.

Just to give you an idea of roadmap, a release candidate of v2 should be coming in the next few weeks (we have some providers actively testing on master currently). We are pushing to allow people to opt in to more of the 0.12 typing / new functionality in the new SDK this year, if we can expose it in a clean, opt-in manner, so hopefully this issue will go away sooner than later and we can have people not need to worry about this nuance for much longer.

Re; keeping it open, its still visible in search when closed and we want to only keep actionable issues open to make our backlog management a bit more straightforward. Since the real fix for this is #261, which is exposing the newer typing, I'd rather people just focus on that one. I do appreciate you doing this investigation though and will point people to this issue if they are asking about similar behavior.