terraform-plugin-sdk icon indicating copy to clipboard operation
terraform-plugin-sdk copied to clipboard

Fix read data failure for TypeList defined in TypeSet

Open ms-zhenhua opened this issue 1 year ago • 2 comments

Fix https://github.com/hashicorp/terraform-plugin-sdk/issues/1197

Scenario

If a schema has a set property which contains a list property as follows, GetOkExists may failed to read values of prop_list.

"prop_set": {
			Type:     pluginsdk.TypeSet,
			Elem: &pluginsdk.Resource{
				Schema: map[string]*pluginsdk.Schema{
					...

					"prop_list": {
						Type:     pluginsdk.TypeList,
						Elem: &pluginsdk.Schema{
							Type: pluginsdk.TypeString,
						},
					},
				},
			},
		},

Reason

This line of readListField will finally run this code to change the address from prop_set.xxxxx.prop_list.0 to prop_set.0.prop_list.0. Since the address is changed, readListField cannot read values in array in subsequent code.

Solution

Add a deep copy in func (r *ConfigFieldReader) readField when changing the address.

ms-zhenhua avatar May 13 '23 03:05 ms-zhenhua

Hi @ms-zhenhua 👋 Thank you for submitting this change. The maintainers here cannot commit to reviewing the full consequences of the change at this time, however it would be helpful to understand the provider code leading up to the reason for this change. Generally opening a bug report issue would capture these details, such as how to reproduce the problem, before getting into code changes.

In general though, the schema.ResourceData getter and setter methods should only be used directly with top-level attributes. They may work today in certain scenarios with deeper nesting of attributes/blocks, but a lot of this SDK's code is unfortunately not well suited for the conversion between the "real" data path and the "stringified" data path, especially in the cases of sets where the set index sometimes gets converted to an underlying Go slice index. It is difficult to gauge whether there may be regression concerns for developers who may have somehow been dependent on the old behavior and whether fixing the potential bug is worth the provider developer burden for unexpected or unintentional usage. Adding some covering unit testing may alleviate some of those concerns, however this SDK is not exhaustive with edge case coverage.

If we can better determine what situations this issue may occur and how it will affect existing dependent provider code, then we can potentially prioritize this better. 👍 Thanks again.

bflad avatar May 15 '23 13:05 bflad

Thanks @bflad. I have created an issue to give more details for this problem.

ms-zhenhua avatar May 16 '23 02:05 ms-zhenhua