Stop silently converting map Elem from Resource to TypeString

[josephholsten]

Proposed resolution: modify all the schemas which use Type: TypeMap with Elem: Resource; then tighten this validation in getValueType()

Arguably a follow on to hashicorp/terraform#12638

In helper/schema/schema.go:getValueType we see:

if _, ok := schema.Elem.(*Resource); ok {
	// TODO: We don't actually support this (yet)
	// but silently pass the validation, until we decide
	// how to handle nested structures in maps
	return TypeString, nil
}

This has allowed us to write a provider which looks like it's actually validating its elements, but does no such thing.

return &schema.Resource{
	Schema: map[string]*schema.Schema{
		"create_vnic_details": {
			Type:     schema.TypeMap,
			Optional: true,
			Elem: &schema.Resource{
				Schema: map[string]*schema.Schema{
					"assign_public_ip": {
						Type:     schema.TypeBool,
						Optional: true,
					},
				},
			},
		},
	},
}

I do notice this was originally introduced by @radeksimko in hashicorp/terraform@1df1c21

Expected Behavior

getValueType() should have returned an error like "create_vnic_details: unexpected map value type: schema.Resource"

Actual Behavior

Silently allowed using create_vnic_details.assign_public_ip but instead of rendering as "true" or "false", renders as "1" or "0"

Steps to Reproduce

I can do an actual test of this if desired.
Create a provider with the above schema, then use to access create_vnic_details.assign_public_ip

[josephholsten]

oh, I'd argue the correct solution is for me to go and fix all the providers which are broken. I'm not saying we should just remove the coercion and call it a day.

[radeksimko]

I completely agree with you that we should treat such schema as invalid, in fact that's exactly what I did in hashicorp/terraform#12638 and then I realized I broke many acceptance tests across 4 different providers - as mentioned in hashicorp/terraform#12722

It seems that for common use cases these schemas are "kind of working" (based on passing acceptance tests) - which also scares me a bit, but hey 🤷‍♂️

If you are (or anyone else is) willing to modify all those schemas and tighten the validation I'm totally willing to review & merge a PR. 😉

[josephholsten]

It's comforting to know I wasn't the only one who's made this mistake!

[paultyng]

This is the same as #62 , merging with that one.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.