Expose typed ResourceData getters and setters

[radeksimko]

Problem Statement (current situation)

The main way providers interact with data in CRUD is through schema.ResourceData, most commonly via Get() and Set() functions, as shown below:

func resourceAwsApiGatewayRestApi() *schema.Resource {
    return &schema.Resource{
        Create: resourceAwsApiGatewayRestApiCreate,
        Read:   resourceAwsApiGatewayRestApiRead,
        // ...
        Schema: map[string]*schema.Schema{
            "name": {
                Type:     schema.TypeString,
                Required: true,
            },
        // ...
        },
    }
}

func resourceAwsApiGatewayRestApiCreate(d *schema.ResourceData, meta interface{}) error {
    params := &apigateway.CreateRestApiInput{
        Name:        aws.String(d.Get("name").(string)),
    }
    // ...
}

func resourceAwsApiGatewayRestApiRead(d *schema.ResourceData, meta interface{}) error {
    // ...
    d.Set("description", api.Description)
    d.Set("api_key_source", api.ApiKeySource)
    // ...
}

This approach has demonstrated a few downsides in the wild.

Getters

Getting any value requires casting interface{} to the desired/expected type. Casting itself has a negative side effect of causing panic/crash when the underlying type of the variable doesn't match with the type the variable is being casted to.

This is unfortunately common mistake especially in bigger providers with many developers that have less experience with Terraform's SDK and provider development generally.

// Assuming field is of TypeString
// which may not be obvious as schema may be few scroll-pages away from CRUD

d.Get("days").(int)
// ^ that will cause crash at runtime

Such bugs sometimes do remain undetected until long after release as acceptance tests may not be thorough enough to exercise all codepaths and all fields if the resource has large schema.

Setters

Setters suffer from the same problem (type un-safety prone to panic at runtime) except that the panic is often suppressed (because returned error is never checked for) by implementation logic inside Set where we try to cast given value to many different types and return error if the type doesn't match.

Background

There are some historical reasons behind the "magic" casting logic inside Set. Certain upstream SDKs (e.g. AWS SDK) use pointers for most/all variables (*string, *int, *bool) which helps them represent "undefined" (nil) values and empty ("", 0, false) ones. Providers using such SDKs would therefore be forced to cast most return values from SDK (pointers) to primitive types (string, int, bool) and check for nils to avoid crashes.

Such logic makes otherwise "meaty" domain logic in CRUD very verbose.

if v, ok := api.ApiKeySource.(*string); ok && v != nil {
  d.Set("api_key_source", *v)
}

This is why such providers choose to rely on Set to do the casting for them and they're left with simple d.Set("api_key_source", api.ApiKeySource) which can deal with all cases in the way that most providers in most cases consider sufficient:

1. If value is nil, field is left unset - effectively empty, but that's implementation detail
2. If value is *string, variable is dereferenced and field is set to that value
3. If value is of invalid type, field is left unset

(3) is actually in most cases root cause of many bugs, but that is usually underestimated due to the verbosity of the above "safe" way and simplicity of the "magical" Set function on the other side.

This was also the main motivation behind introducing TF_SCHEMA_PANIC_ON_ERROR introduced in https://github.com/hashicorp/terraform/pull/16588 so developers can at least detect such bugs in acceptance tests.

Proposal

Full proposal/implementation is subject to RFC, but in short we could expose typed getters and setters, e.g.

• GetString() string / SetString(name, value string)
• GetInt() int / SetInt(name string, value int)
• GetFloat() float / SetFloat(name string, value float)
• GetBool() bool / SetBool(name string, value bool); potentially IsTrue() and IsFalse()

Complex types such as TypeSet, TypeList and TypeMap may need a bit more thinking so we understand all consequences and use cases.

[katbyte]

It would be nice if in addition to these new functions there was one that ensured the value was set (for data sources in azure) - like d.SetStringEvenIfNil to clean up this sort of code in azure data sources

if location := img.Location; location != nil {
    d.Set("location", azure.NormalizeLocation(*location))
} else {
    d.Set("location", "")
}