Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider Splitting tftypes.Type into Value and Constraint/Schema Types #128

Open
bflad opened this issue Dec 22, 2021 · 1 comment
Open
Labels
breaking-change This will impact or improve our compatibility posture enhancement New feature or request

Comments

@bflad
Copy link
Member

bflad commented Dec 22, 2021

terraform-plugin-go version

v0.5.0

Use cases

Terraform has two conceptually different meanings of a "type" within its type system today:

  • Value types, which concisely describe the structure and data kind for a specific value.
  • Type constraints or schema types, which may contain additional metadata that should be sent across the protocol, but should not necessarily be usable as a value type.

One place where this distinction is important are object types, which may define OptionalAttributes. An object value must specify all attributes (e.g. with nils), regardless of that additional information. Another odd case is DynamicPseudoType, which technically can be used in both contexts.

The tftypes package is currently written with a flattened Type definition that serves both purposes. Functions like NewValue() accept tftypes.Object with OptionalAttributes, however this should not be valid in all scenarios and can cause provider development issues as it can be difficult to reason between the intended usage of functions. Since there is a single Go type to work with, it is slightly more difficult to enforce correctness. If Terraform expands usage of type constraints in the future, this issue will be exasperated.

Proposal

Split tftypes.Type into multiple Go types, one with the sole purpose of value handling and the other for constraint/metadata/schema handling. e.g. (naming up for discussion, it just didn't feel appropriate to still call one Type)

// TypeConstraint is an interface representing a Terraform type with additional
// characteristics. It is only meant to be implemented by the tftypes package.
// TypeConstraints define metadata on top of ValueTypes.
type TypeConstraint interface {
	// Equal performs deep type equality checks, including attribute/element
	// types and whether attributes are optional or not.
	Equal(TypeConstraint) bool

	// String returns a string representation of the TypeConstraint's name.
	String() string

	// ValueType returns the associated ValueType.
	ValueType() ValueType

	// private is meant to keep this interface from being implemented by
	// types from other packages.
	private()
}

// ValueType is an interface representing a Terraform type. It is only meant to be
// implemented by the tftypes package. ValueTypes define the shape and
// characteristics of data coming from or being sent to Terraform.
type ValueType interface {
	// Equal performs deep type equality checks, including attribute/element
	// types.
	Equal(ValueType) bool

	// Is performs shallow type equality checks, in that the root type is
	// compared, but underlying attribute/element types are not.
	Is(ValueType) bool

	// String returns a string representation of the ValueType's name.
	String() string

	// UsableAs performs type conformance checks. This primarily checks if the
	// target implements DynamicPsuedoType in a compatible manner.
	UsableAs(TypeConstraint) bool

	// private is meant to keep this interface from being implemented by
	// types from other packages.
	private()
}

The most impactful changes for implementors will then be the updated function signatures to align with these new types:

// package tfprotov5/tfprotov6
type SchemaAttribute struct {
	// ...

	// TypeConstraint indicates the type of data the attribute expects. See the
	// documentation for the tftypes package for information on what types
	// are supported and their behaviors.
	Type tftypes.TypeConstraint
}

func NewDynamicValue(t tftypes.ValueType, v tftypes.Value) (DynamicValue, error)

func (d DynamicValue) Unmarshal(typ tftypes.ValueType) (tftypes.Value, error)

func (s RawState) Unmarshal(typ tftypes.ValueType) (tftypes.Value, error)

// package tftypes
func NewValue(t ValueType, val interface{}) Value

func TypeFromElements(elements []Value) (ValueType, error)

func ValidateValue(t ValueType, val interface{}) error

func (v Value) Type() ValueType

The value types that are implemented within tftypes should then support both ValueType and TypeConstraint.

References

@bflad bflad added enhancement New feature or request breaking-change This will impact or improve our compatibility posture labels Dec 22, 2021
bflad added a commit that referenced this issue Jan 7, 2022
Reference: #94
Reference: #99
Reference: #100
Reference: #128
Reference: #133

Reverts incorrect logic for handling DynamicPseudoType values in `tftypes`. This type information must be preserved when traversing the protocol, as Terraform CLI decodes values based on the schema information. If a concrete value type is used where DynamicPseudoType is expected, Terraform CLI will return errors such as (given an object of 4 attributes, when DynamicPseudoType is expected):

```
│ Error: ["manifest"]: msgpack: invalid code=84 decoding array length
```
@bflad
Copy link
Member Author

bflad commented Jan 13, 2022

Drive-by note: Core refers to these as ConstraintType, which might be helpful in synchronizing naming/terminology.

bflad added a commit that referenced this issue Jan 13, 2022
Reference: #94
Reference: #99
Reference: #100
Reference: #128
Reference: #133

Reverts incorrect logic for handling DynamicPseudoType values in `tftypes`. This type information must be preserved when traversing the protocol, as Terraform CLI decodes values based on the schema information. If a concrete value type is used where DynamicPseudoType is expected, Terraform CLI will return errors such as (given an object of 4 attributes, when DynamicPseudoType is expected):

```
│ Error: ["manifest"]: msgpack: invalid code=84 decoding array length
```
bflad added a commit that referenced this issue Jan 31, 2022
bflad added a commit that referenced this issue Feb 4, 2022
bflad added a commit that referenced this issue Feb 16, 2022
Reference: #128
Reference: #157

This methods implement standard conversion logic from `Schema*` types into their associated `tftypes.Type` equivalent. For example, these can be used to prepare calls to the `(DynamicValue).Unmarshal()` method, which is necessary in many RPCs.

Preferably, these methods would have been named `Type()`, however the `SchemaAttribute` type implements a `Type` field and Go does not permit overlapping field and method names. Instead, the name `ValueType()` was chosen as an alternate designation as the `tftypes.Type` really represents a data value type, instead of a type constraint (sometimes also referred to as a schema type).
bflad added a commit that referenced this issue Feb 17, 2022
…158)

Reference: #128
Reference: #157

This methods implement standard conversion logic from `Schema*` types into their associated `tftypes.Type` equivalent. For example, these can be used to prepare calls to the `(DynamicValue).Unmarshal()` method, which is necessary in many RPCs.

Preferably, these methods would have been named `Type()`, however the `SchemaAttribute` type implements a `Type` field and Go does not permit overlapping field and method names. Instead, the name `ValueType()` was chosen as an alternate designation as the `tftypes.Type` really represents a data value type, instead of a type constraint (sometimes also referred to as a schema type).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will impact or improve our compatibility posture enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant