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

all: Add support for embedded struct types in object conversions #1021

Merged
merged 17 commits into from
Aug 2, 2024

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented Jul 22, 2024

Closes #242

Previous proposals: #941, #667

This PR introduces support for types.Object conversions with Go structs that utilize struct embedding:

This PR supports N levels of nested embedded struct types and applies the same tfsdk tag restrictions/validations regardless of where the tag appears in the struct.

Example

type thingResourceModel struct {
    Attr1 types.String `tfsdk:"attr_1"`
    Attr2 types.Bool   `tfsdk:"attr_2"`
    CommonModel
}

type CommonModel struct {
    Attr3 types.Int64 `tfsdk:"attr_3"`
    Attr4 types.List  `tfsdk:"attr_4"`
    EvenMoreCommonModel
}

type EvenMoreCommonModel struct {
    Attr5 types.String `tfsdk:"attr_5"`
    Attr6 types.Map    `tfsdk:"attr_6"`
}

func (r *thingResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
    resp.Schema = schema.Schema{
        Attributes: map[string]schema.Attribute{
            "attr_1": schema.StringAttribute{
                Required: true,
            },
            "attr_2": schema.BoolAttribute{
                Computed: true,
                PlanModifiers: []planmodifier.Bool{
                    boolplanmodifier.UseStateForUnknown(),
                },
            },
            "attr_3": schema.Int64Attribute{
                Required: true,
            },
            "attr_4": schema.ListAttribute{
                ElementType: types.StringType,
                Computed:    true,
                PlanModifiers: []planmodifier.List{
                    listplanmodifier.UseStateForUnknown(),
                },
            },
            "attr_5": schema.StringAttribute{
                Required: true,
            },
            "attr_6": schema.MapAttribute{
                ElementType: types.BoolType,
                Computed:    true,
                PlanModifiers: []planmodifier.Map{
                    mapplanmodifier.UseStateForUnknown(),
                },
            },
        },
    }
}

The same example schema could also be represented by these model combinations:

type thingResourceModel struct {
    Attr1 types.String `tfsdk:"attr_1"`
    Attr2 types.Bool   `tfsdk:"attr_2"`
    CommonModel
    EvenMoreCommonModel
}

type CommonModel struct {
    Attr3 types.Int64 `tfsdk:"attr_3"`
    Attr4 types.List  `tfsdk:"attr_4"`
}

type EvenMoreCommonModel struct {
    Attr5 types.String `tfsdk:"attr_5"`
    Attr6 types.Map    `tfsdk:"attr_6"`
}

Notes

  • Additional test coverage: Add reflection tests for object models with tfsdk tags in framework providers terraform-provider-corner#263
  • To avoid having to refactor a good chunk of our reflection logic, we explicitly are not supporting struct embedding with pointers. That doesn't stop us from adding support in the future if it's deemed necessary (although supporting unexported struct embeds from pointers I don't think we can do)
    • The encoding/json library supports exported struct embedding via pointers, but it was also historically relying on a bug in Go that allowed setting unexported struct embedding with pointers to work. When that bug was fixed, they chose not to completely remove pointer struct embedding for compatibility reasons.
  • This PR could be considered a change in behavior, as it adds support for unexported embedded structs which promote exported fields
    • These would be previously ignored, but now could potentially raise an error diagnostic if tfsdk ignore tags are not included
type thingResourceModel struct {
    Attr1 types.String `tfsdk:"attr_1"`
    Attr2 types.Bool   `tfsdk:"attr_2"`
    existingModel
}

type existingModel struct {
    FieldNotRelated string // Needs a tfsdk tag now!
}
  • You can ignore an entire embedded struct via the typical ignore tag
type thingResourceModel struct {
    Attr1 types.String `tfsdk:"attr_1"`
    Attr2 types.Bool   `tfsdk:"attr_2"`
    existingModel      `tfsdk:"-"` // Ignored!
}

type existingModel struct {
    FieldNotRelated string
}

@austinvalle austinvalle added the enhancement New feature or request label Jul 22, 2024
@austinvalle austinvalle added this to the v1.11.0 milestone Jul 22, 2024
@austinvalle austinvalle requested a review from a team as a code owner July 22, 2024 21:49
Copy link
Contributor

@SBGoods SBGoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, Austin! Just one comment about empty struct tags.

internal/reflect/helpers.go Outdated Show resolved Hide resolved
@austinvalle austinvalle merged commit 4d10c17 into main Aug 2, 2024
31 checks passed
@austinvalle austinvalle deleted the av/embedded-structs branch August 2, 2024 13:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Follow" Embedded Structs
2 participants