hashicorp / hashicorp/terraform-plugin-sdk

Expose typed ResourceData getters and setters

Open
#229 1 comment 3 reactions 0 assignees View on GitHub
enhancement subsystem/types
Dominant language
Go
Stars
485
Forks
244
Avg merge
19h 57m
Merged PRs (30d)
4

Description

## 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:

```go
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.

```go
// 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.

```go
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.

Contributor guide

Open the contributing guide

Research direction

Start with schema.ResourceData and its existing Get and Set implementations, then review the proposal for typed string, int, float, and bool accessors. The RFC should resolve behavior for pointer values, invalid types, nil values, and complex TypeSet, TypeList, and TypeMap fields. Done means an agreed API and implementation scope, backed by tests for the specified getter and setter behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
devtools
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.