Add model generation #17

bendbennett · 2023-06-22T15:50:32Z

This PR:

Adds model generation
Modifies the imports so that "github.com/hashicorp/terraform-plugin-framework/types" is included when CustomType is not nil as this import is required for all of the "default" framework types that are used by the models (e.g., types.Bool for a boolean attribute, []types.Object for a list nested attribute etc).

~~Currently, this is only in-place for data sources, but will be extended to include provider and resources following initial review.~~

The unit tests in place for the Generator<Type>.ToModel() methods do not explicitly cover instances in which the CustomType is non-nil and the CustomType.ValueType is an empty string as the JSON schema for the codegen-spec explicitly requires that CustomType.ValueType is a non-empty string.

Questions

Do we want to extend the test coverage for DataSourceModelGenerator.Process() to include testing of recursion for nested attributes and nested blocks?
Should we consider prefixing the names of the models generated during recursion into nested attributes and nested blocks to avoid potential naming collisions? A separate issue has been created for consideration of the handling of naming collisions.

For example, with an intermediate representation that contains the following:

          {
            "name": "list_nested_list_nested_bool_attribute",
            "list_nested": {
              "computed_optional_required": "computed",
              "nested_object": {
                "attributes": [
                  {
                    "name": "list_nested_attribute",
                    "list_nested": {
                      "computed_optional_required": "computed",
                      "nested_object": {
                        "attributes": [
                          {
                            "name": "bool_attribute",
                            "bool": {
                              "computed_optional_required": "computed"
                            }
                          }
                        ]
                      }
                    }
                  }
                ]
              }
            }
          },
          {
            "name": "list_nested_list_nested_list_attribute",
            "list_nested": {
              "computed_optional_required": "computed",
              "nested_object": {
                "attributes": [
                  {
                    "name": "list_nested_attribute",
                    "list_nested": {
                      "computed_optional_required": "computed",
                      "nested_object": {
                        "attributes": [
                          {
                            "name": "list_attribute",
                            "list": {
                              "computed_optional_required": "computed",
                              "element_type": {
                                "string": {}
                              }
                            }
                          }
                        ]
                      }
                    }
                  }
                ]
              }
            }
          },

The generated models have a naming collision:

type listNestedAttributeModel struct {
	BoolAttribute types.Bool `tfsdk:"bool_attribute"`
}

type listNestedAttributeModel struct {
	ListAttribute types.List `tfsdk:"list_attribute"`
}

… type is not an empty string

…ecified

… of import to include terraform-plugin-framework/types when custom type is nil

…ourcesModelsGenerator.Process()

…of conditions that are not permitted by the codegen-spec schema

bflad

Some initial thoughts! What do you think?

bflad · 2023-06-22T16:22:20Z

internal/cmd/cmd.go

@@ -78,14 +78,27 @@ func (a AllCommand) Run(args []string) int {
 		log.Fatal(err)
 	}

+	// generate model code


Not necessarily for this PR (maybe as part of @austinvalle's command work?), but it'd be great if the "all" command could delegate to some separate logic that covers all data source generation (which itself handles models, schema, and formatting components), all resource generation (same components), etc.

I can do this in my upcoming PR #18 👍🏻

Thanks @austinvalle

bflad · 2023-06-22T16:23:31Z

internal/cmd/testdata/custom_and_external/output/datasource_data_source_gen.go

+	BoolAttribute                                                            types.Bool     `tfsdk:"bool_attribute"`
+	ListListAttribute                                                        types.List     `tfsdk:"list_list_attribute"`
+	ListMapAttribute                                                         types.List     `tfsdk:"list_map_attribute"`
+	ListNestedBoolAttribute                                                  []types.Object `tfsdk:"list_nested_bool_attribute"`


These should be types.List/types.Set as appropriate to handle unknown values. 👍

Ooops........

bflad · 2023-06-22T16:26:39Z

internal/datasource_generate/bool_attribute.go

+		GeneratorBoolAttribute
+	}{
+		Name:                   name,
+		DefaultType:            "types.Bool",


Super nit: It might be good to move these to constants. 👍

bflad · 2023-06-22T16:38:46Z

internal/datasource_generate/model_templates/model_field.gotmpl

Given these templates are very tiny and delegating to Go functions anyways, would this potentially be easier to use strings.Builder in some cases and regular Go logic? It would help remove some of the "logic indirection" of needing to bounce between what is doing what and could help simplify the code everywhere. e.g. in this case

// ToModel() fieldType := "types.Bool" if g.CustomType != nil { fieldType = g.CustomType.ValueType } return fmt.Sprintf("%s %s `tfsdk:%q`", snakeCaseToCamelCase(name), fieldType, name), nil

Taking this further, it may actually be preferable to delegate this to its own Go type and method to centralize the logic more and give everything concise naming:

type ModelStructField struct { Name string TfsdkName string // debatable :) ValueType string } func (f ModelStructField) String() string { return fmt.Sprintf("%s %s `tfsdk:%q`", f.Name, f.ValueType, f.TfsdkName) } // ToModel() field := ModelStructField{ Name: snakeCaseToCamelCase(name), TfsdkName: name, ValueType: "types.Bool", } if g.CustomType != nil { field.ValueType = g.CustomType.ValueType } return field.String()

Ben and I chatted about a similar topic in our 1 on 1. I'm still on the fence on whether it's easier to use Go templates vs. string builders for generating code in general.

If we plan to eventually allow "overriding" of the templates then it could be useful, otherwise it makes it a little less convenient to follow the code/read.

I'm not sure we landed in a definitive place on whether to favor templates or string builder, maybe a hybrid approach is fine? Worth a discussion for sure 👍🏻

Taking a hybrid approach seems very reasonable to me personally. While text/template is a generally acceptable tool for "larger scale" Go code generation in the Go community, I don't think its meant to be an always versus never situation, especially if the code generation makes sense to be decomposed into smaller implementation details.

My personal factors of whether to use a template here would include:

If the generated code snippet is large, such as a whole source code file

If the generated code snippet has many possible customizations and if they would be better served via:

Additional template functions, which tend to be less easily discoverable

Additional properties in the specification

A command line flag

A generator-specific configuration

If a text template is more or less readable than its Go code equivalent (also keeping in mind the logic indirection)

As a pro-template example, a template for generating a scaffold resource source code file likely makes sense, so a provider maintainer can give the best start for other developers by baking in provider-specific code details and comments.

Thanks for the feedback and thoughts on template usage. I think I'm leaning towards removing the usage of templates for the generation of the models for the reasons discussed. I'll refactor to switch to using structs and String() methods as suggested.

austinvalle · 2023-06-22T20:34:49Z

internal/cmd/testdata/custom_and_external/output/datasource_data_source_gen.go

@@ -243,3 +243,108 @@ var datasourceDataSourceSchema = schema.Schema{
 		},
 	},
 }
+
+type datasourceDataSourceModel struct {


Random thought that also applies to the schema variable above, but should we opt to just export these structs by default?

type DatasourceDataSourceModel struct {

Hmmmm........I'm not sure about this. Easily switched out. Do the structs and schema var need to be exported?

I'd lean towards always exporting, in-case developers want to keep generated code in it's own package. It's also not anything we need to change now 👍🏻

I'd be 👍 for always exporting as well -- while most provider code is not meant to be exported for other Go modules, most provider developers opt to achieve this via internal/ packaging. At the end of the day though, that is their choice. I think as we'll see with #20, being able to generate multiple packages of code might be an important implementation detail.

Fair enough. Have made all of the generated models exported.

austinvalle · 2023-06-22T20:38:03Z

internal/cmd/testdata/custom_and_external/output/datasource_data_source_gen.go

+	BoolAttribute                                                            types.Bool     `tfsdk:"bool_attribute"`
+	ListListAttribute                                                        types.List     `tfsdk:"list_list_attribute"`
+	ListMapAttribute                                                         types.List     `tfsdk:"list_map_attribute"`
+	ListNestedBoolAttribute                                                  []types.Object `tfsdk:"list_nested_bool_attribute"`


Is it worth it to add comments mentioning which model represents the nested object like we do in the utility providers: https://github.com/hashicorp/terraform-provider-tls/blob/db1c1933f5410d86c2c40fedabe10d0942a71341/internal/provider/models.go#L25

I think this is an interesting idea. If it's ok with you and Brian I'm going to punt on this but have added an issue for later consideration.

austinvalle · 2023-06-22T20:38:42Z

internal/cmd/testdata/custom_and_external/output/datasource_data_source_gen.go

+	ListNestedBlockObjectAttributeListNestedNestedBlockListAttribute         []types.Object `tfsdk:"list_nested_block_object_attribute_list_nested_nested_block_list_attribute"`
+	ListNestedListNestedBlockBoolAttribute                                   []types.Object `tfsdk:"list_nested_list_nested_block_bool_attribute"`
+	SingleNestedBlockBoolAttribute                                           types.Object   `tfsdk:"single_nested_block_bool_attribute"`
+	SingleNestedBlockObjectAttributeSingleNestedListNestedBlockListAttribute types.Object   `tfsdk:"single_nested_block_object_attribute_single_nested_list_nested_block_list_attribute"`


single_nested_block_object_attribute_single_nested_list_nested_block_list_attribute

This field name fills me with joy 🤣

austinvalle · 2023-06-22T21:08:48Z

internal/datasource_generate/model_templates/model_field.gotmpl

Ben and I chatted about a similar topic in our 1 on 1. I'm still on the fence on whether it's easier to use Go templates vs. string builders for generating code in general.

If we plan to eventually allow "overriding" of the templates then it could be useful, otherwise it makes it a little less convenient to follow the code/read.

I'm not sure we landed in a definitive place on whether to favor templates or string builder, maybe a hybrid approach is fine? Worth a discussion for sure 👍🏻

…rovided through stdin

…d resource attributes and blocks

bflad

Overall looks really good! Some minor code considerations, if you're interested now or later. 😄

bflad · 2023-06-26T12:28:45Z

internal/datasource_generate/generator.go

+	for name, schema := range schemas {
+		var buf bytes.Buffer
+
+		m, err := generateModel(name, schema.Attributes, schema.Blocks)


Nit: It seems like this could be a method on GeneratorDataSourceSchema, e.g.

func (s GeneratorDataSourceSchema) Model(name string) (Model, error)

Fair point. I've refactored:

func (g GeneratorResourceSchema) Model(name string) ([]model.Model, error)

As there are recursive calls for generating the "nested" models I'm returning a slice of model.Model.

bflad · 2023-06-26T12:32:24Z

internal/model/model.go

+
+type Model struct {
+	Name   string
+	Fields string


Do you think it would be better if this was Fields []StructField so code can append() directly into it and String() can handle the newlines in the string output?

Seems reasonable. Have refactored along those lines.

github-actions · 2024-05-23T22:18:15Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

bendbennett added 14 commits June 21, 2023 10:56

Using func to make individual call to template for bool attribute

7dd4083

Only use custom type value type when custom type is not nil and value…

2ede9e6

… type is not an empty string

Updating imports to include framework types when no custom type is sp…

45edeb8

…ecified

Renaming template and fixing tests

0aa2e6b

Switching to using a generic template for atrribute models

c04bb80

Initial handling of models for nested attributes

745e77e

Add recursion for list nested attribute

c609a9d

Extending recursion to cover all nested attributes and nested blocks

fda618b

Adding ToModel() to all attributes and blocks and modifying behaviour…

3ca14dd

… of import to include terraform-plugin-framework/types when custom type is nil

Adding initial test coverage for models created through calling DataS…

0aaaacf

…ourcesModelsGenerator.Process()

Modifying model field template and updating tests to remove coverage …

db032ae

…of conditions that are not permitted by the codegen-spec schema

Adding copyright header

a6e4210

Merge branch 'main' into models

8fb9235

Updated data source testdata for e2e test to include generated models

087c20d

bflad reviewed Jun 22, 2023

View reviewed changes

austinvalle reviewed Jun 22, 2023

View reviewed changes

bendbennett added 14 commits June 23, 2023 09:21

Refactor to remove use of template for model struct fields

b4d2c13

Moving insertion of newline to generator

00fdfcc

Switching to using model struct

727824d

Removing use of template for datasource model

95bb91e

Move function to inline

8e4dc21

Renaming variables

59f848f

Renaming variables

262b591

Adding copyright header

123e163

Merge branch 'main' into models

0a2bba8

Updating generateDataSourceCode() to include model generation

95f1b84

Removing default for flagIRInputPath otherwise this overrides input p…

b9a9546

…rovided through stdin

Fix convertAttribute() func

90880cd

Move snakeCaseToCamelCase() and lcFirst() functions

dc5135c

Modifying Import() func and adding ToModel() func for all provider an…

cf82bfd

…d resource attributes and blocks

bendbennett added 3 commits June 26, 2023 10:28

Adding model generators to provider and resources

e01bd9b

Wiring-up model generation for provider and resources

5b79d78

Adding unit tests for provider and resources model generator

a222bfb

bendbennett marked this pull request as ready for review June 26, 2023 10:24

bendbennett requested a review from a team as a code owner June 26, 2023 10:24

Exporting generated models

4331da2

bflad approved these changes Jun 26, 2023

View reviewed changes

bendbennett added 4 commits June 26, 2023 16:00

Refactor from func to method

91191c6

Rename struct

85f375e

Refactoring to return model.Field

02a03d2

Updating to use a slice of models

2734d01

bendbennett merged commit afe26a0 into main Jun 26, 2023

bendbennett deleted the models branch June 26, 2023 16:47

github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add model generation #17

Add model generation #17

bendbennett commented Jun 22, 2023 •

edited

Loading

bflad left a comment

bflad Jun 22, 2023

austinvalle Jun 22, 2023 •

edited

Loading

bendbennett Jun 23, 2023

bflad Jun 22, 2023

bendbennett Jun 23, 2023

bflad Jun 22, 2023

bflad Jun 22, 2023

austinvalle Jun 22, 2023 •

edited

Loading

bflad Jun 23, 2023

bendbennett Jun 23, 2023 •

edited

Loading

austinvalle Jun 22, 2023

bendbennett Jun 23, 2023

austinvalle Jun 23, 2023

bflad Jun 26, 2023

bendbennett Jun 26, 2023

austinvalle Jun 22, 2023

bendbennett Jun 23, 2023

austinvalle Jun 22, 2023

austinvalle Jun 22, 2023 •

edited

Loading

bflad left a comment

bflad Jun 26, 2023

bendbennett Jun 26, 2023

bflad Jun 26, 2023

bendbennett Jun 26, 2023

github-actions bot commented May 23, 2024

Add model generation #17

Add model generation #17

Conversation

bendbennett commented Jun 22, 2023 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinvalle Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

bendbennett commented Jun 22, 2023 •

edited

Loading

austinvalle Jun 22, 2023 •

edited

Loading

austinvalle Jun 22, 2023 •

edited

Loading

bendbennett Jun 23, 2023 •

edited

Loading

austinvalle Jun 22, 2023 •

edited

Loading