Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix read data failure for TypeList defined in TypeSet #1195

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ms-zhenhua
Copy link

@ms-zhenhua ms-zhenhua commented May 13, 2023

Fix #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 ms-zhenhua requested a review from a team as a code owner May 13, 2023 03:19
@bflad
Copy link
Member

bflad commented May 15, 2023

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.

@ms-zhenhua
Copy link
Author

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

austinvalle added a commit that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diff.GetOk return nil for the value of TypeList defined in TypeSet
2 participants