ElementAs() make target null

[miton18]

Module version

github.com/hashicorp/terraform-plugin-framework v1.1.1

Relevant provider source code

type R struct {
  Environment types.Map    `tfsdk:"environment"`
}
...
  env := map[string]string{"t": "e"} // we have a map with 1 entry

  // r.Environement is null (IsNull() = true)
  diags.Append(r.Environment.ElementsAs(ctx, &env, false)...)
  if diags.HasError() {
	return env // no error reported
  }

  // Here, env is null
  env["PHP_VERSION"] = "8"

Terraform Configuration Files

not relevant

Debug Output

Expected Behavior

The initial map untouched

Actual Behavior

panic: assignment to entry in nil map

goroutine 268 [running]:
go.clever-cloud.com/terraform-provider/pkg/resources/php.(*PHP).toEnv.func3({0x1589900, 0x1})
        /terraform-provider-clevercloud/pkg/resources/php/resource_php_schema.go:110 +0x1c5
go.clever-cloud.com/terraform-provider/pkg.IfIsSet({0x20?, {0x1589900?, 0xea0b14?}}, 0x9?)
        /terraform-provider-clevercloud/pkg/types.go:9 +0x32
/terraform-provider/pkg/resources/php.(*PHP).toEnv(0xc0002d8000, {0x100d320, 0xc00027f410}, {0x0, 0x0, 0x0})
        /terraform-provider-clevercloud/pkg/resources/php/resource_php_schema.go:105 +0x785
go.clever-cloud.com/terraform-provider/pkg/resources/php.(*ResourcePHP).Create(0xc00056f8a8, {0x100d320, 0xc00027f410}, {{{{0x1012000, 0xc000295d40}, {0xd87c40, 0xc000294c60}}, {0x1013488, 0xc000752320}}, {{{0x1012000, ...}, ...}, ...}, ...}, ...)

Steps to Reproduce

References

[bflad]

Hi @miton18 👋 Thank you for raising this and apologies for the delayed review of the issue.

At first glance, it appears that the framework's behavior is correct in this case although the method's naming may leave a little to be desired. The framework's logic currently distinguishes between what it means for a map value type to be null, overwriting a nil Go map for the given target variable, versus one that is known with zero or more elements, overwriting an instantiated Go map for the given target variable. I'm not sure there's good reason to adjust that behavior with (basetypes.MapValue).ElementsAs(), otherwise developers would be forced into additional (basetypes.MapValue).IsNull() checks where nil checks against the Go map are functionally equivalent. It could be seen as essentially swapping one design tradeoff for another while introducing a burden for provider developers to potentially audit and change their existing logic. We would also need to consider changes for (basetypes.ListValue).ElementsAs(), and (basetypes.SetValue).ElementsAs() since they inherently should already be creating nil Go slices in similar situations.

If your goal is to append/overwrite a Go map elements with basetypes.MapValue elements, the expected provider logic might be something along the lines of:

// Default values
env := map[string]string{"t": "e"}

var tfEnv map[string]string

diags.Append(r.Environment.ElementsAs(ctx, &tfEnv, false)...)

if diags.HasError() {
  return
}

// Go will automatically do nothing if tfEnv is nil
for key, value := range tfEnv {
  // This will create new keys or overwrite default values
  env[key] = value
}

// assignment like the below will always remain safe
env["PHP_VERSION"] = "8"

We could potentially consider creating a new method on basetypes.MapValue which does help in this case though, by merging elements of the map on top of an existing map. The challenge with these types of map "merge" implementations then becomes whether their algorithm is shallow or deep with respect to existing values, so it may be important to capture that additional nuance via the method naming. This may be something like:

// ElementsShallowMerge will add new keys or overwrite existing values in a given Go map.
// This algorithm only performs a shallow merge of contents.
func (v MapValue) ElementsShallowMerge(ctx context.Context, target any, allowUnhanded bool) diag.Diagnostics { /* ... */ }

Which would then enable provider logic such as:

// Default values
env := map[string]string{"t": "e"}

diags.Append(r.Environment.ElementsShallowMerge(ctx, &env, false)...)

if diags.HasError() {
  return
}

// assignment like the below would remain safe
env["PHP_VERSION"] = "8"

I'm going to adjust the labels here from bug to enhancement to capture that potential effort. In the meantime, I'm going to double check that the existing ElementsAs methods have Go documentation which captures the nuance of null to nil Go maps/slices for the given target parameter.