Open miton18 opened 1 year ago
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.
Module version
Relevant provider source code
Terraform Configuration Files
Debug Output
Expected Behavior
The initial map untouched
Actual Behavior
Steps to Reproduce
References