Investigate Consolidation of Schema Required/Optional/Computed

[bflad]

Module version

v0.1.0

Use-cases

In Terraform Plugin SDK and v0.1.0 of this project, there are currently only a few meaningful valid configurations of Required, Optional, and Computed for schema attributes. Switching these to a single field that captures the meaningful valid configurations means there is no way to misconfigure them and it is not losing any configurability. Setting these as false is a perfectly valid implementation, just conventionally shied away from in most current schemas due to its unnecessary verbosity.

Mapping out the three possible boolean field declarations:

• Required: true: ✅
• Required: false: ❌
• Required: true + Optional: true: ❌
• Required: true + Optional: false: 👍 (just verbose)
• Required: false + Optional: true: 👍 (just verbose)
• Required: false + Optional: false: ❌
• Required: true + Computed: true: ❌
• Required: true + Computed: false: 👍 (just verbose)
• Required: false + Computed: true: 👍 (just verbose)
• Required: false + Computed: false: ❌
• Optional: true: ✅
• Optional: false: ❌
• Optional: true + Computed: true: ✅
• Optional: false + Computed: true: 👍 (just verbose)
• Optional: true + Computed: false: 👍 (just verbose)
• Optional: false + Computed: false: ❌
• Computed: true: ✅
• Computed: false: ❌

In terms of static analysis, it is much easier to deal with one required field than three optional and conflicting ones. For example, it is relatively easy to write go/analysis and semgrep tooling that checks for the existence of a field declaration with a struct. Tooling and ensuring consistency around that is harder if you need to check values, let alone multiple of them, to ensure validity and any stylistic conventions. Not the biggest deal, but preferring no false values for the current booleans requires additional static analysis, for example: https://github.com/bflad/tfproviderlint/tree/main/passes/S019.

Proposal

Quick sketch being:

const (
    // Requires configuration.
    AttributeConfigurationRequired AttributeConfiguration = 0

    // Configuration is not required, but value can be optionally set.
    // Shows difference if plugin returns value and there is no configuration.
    AttributeConfigurationOptional AttributeConfiguration = 1

    // Configuration is not required, but value can be optionally set.
    // Does not show difference if plugin returns value and there is no configuration.
    AttributeConfigurationOptionalComputed AttributeConfiguration = 2

    // Cannot be configured, however the plugin can set the value.
    AttributeConfigurationComputed AttributeConfiguration = 3
)

type AttributeConfiguration uint8

type Attribute struct {
    // ... other fields ...
    // The below would replace Required, Optional, and Computed
    Configuration AttributeConfiguration
}

Which the naming can be adjusted as necessary, e.g. can be further reduced with any of dropping Attribute, Attribute to Attr, Configuration to Config, moving to a separate package, etc.

References

• https://github.com/bflad/tfproviderlint#standard-schema-checks

[apparentlymart]

While pondering this, it could also be interesting to ponder whether Optional: true alone is really necessary as a separate state than Optional: true, Computed: true. We copied the old SDK model pretty directly when designing the protocol's schema structure, and hence we inherited this strange multi-flag design from there without critiquing it much.

Optional without Computed in practice only means that if the configuration doesn't set it then the value must be null. Optional and Computed together allows the provider to choose a default, and that default could itself be null if appropriate, and thus get the same effect.

Optional without Computed was perhaps incidentally useful in the old SDK so that the provider code could unambiguously recognize when the argument got "unset" in a later change (since Computed arguments have sticky values in the old SDK), but I believe the new framework makes it possible for provider code to examine the configuration directly and thus see when something's been unset regardless of whether there's a "sticky" computed value from the previous run included in the proposed new state.

I've not thought exhaustively about this so there might well be some other useful benefit to Optional without Computed that I'm not considering, but this proposal does seem like an opportunity to investigate that further and potentially end in an even less complicated final model, where we only need to include "Required", "Optional", and "Computed" in this enum, where "Optional" would represent what we currently model as Optional: true, Computed: true, instead of just Optional alone. 🤔

[bflad]

Just to capture the data point, core does not implement configurability validations, per hashicorp/terraform#30669. We did implement "Required" and "Read-Only" checks in v0.9.0 of the framework, per #370.

While looking at another issue involving validation, it reminded me that many of our "common use case" validators that will wind up in github.com/hashicorp/terraform-plugin-framework-validators will purposefully skip null values, because they are expecting something else to handle any "Required" checking. This makes me wonder if we could extend the simplification even further:

• Remove Required/Optional/Computed fields
• Without setting anything, attributes will be marked as Computed only
• Create validator(s) which represent Optional/Required
• Update the framework logic to sniff for those validators to handle Required/Optional and unset Computed when Required

This would allow provider developers to define all expected configuration value information in one place (attribute validators). I haven't really thought through whether this is desirable or would have other issues.

There's also another similar potential simplification:

• Remove Computed field
• Without setting anything, attributes will be marked as Computed only
• If Required, unset Computed (Optional will always set Computed as mentioned before)

I'm not sure when we'll really dive into this, but wanted to document the related issue, pull request, and ideas while thinking about it.

[magodo]

@bflad One concern about changing these attributes from declarative to imperative manner is that makes the static analysis tools (e.g. tfproviderlint) hard to implement. What do you think about this?