Generate "expand" and "flatten" functions for associated external types linked to list, map, set, single nested attributes and blocks

[bendbennett]

References: #31

This PR adds generation of "expand" and "flatten" functions for list, map, set, single nested attributes and blocks, with the following caveats:

• Only "top-level" attributes and blocks are currently handled,
• Only "primitive" attributes (Bool, Float64, Int64, Number, String) within list, map, set, single nested attributes and blocks are currently handled.

Related

The change introduced in internal/schema: add AttrImport for object model helper is also handled at the GeneratorAttribute/Block level in this PR. We can handle this either way, but I've left both sets of changes in place for now.

[bflad]

Overall this looks like it is going in the right direction but noticed some things upfront which would apply across the board. 👍

[bflad]

Unknown handling is an interesting case! This is logic is probably okay for now, but we could also consider raising an error diagnostic rather than potentially calling an API incorrectly (which might generate its own confusing error).

[bendbennett]

Have added in raising error diagnostic.

[bflad]

Collection nested attributes are technically a types. whatever collection of types.Object elements, where each of those types.Object can also be unknown. This can happen in Terraform with more complex configuration situations. So we'll need to be careful to ensure we handle unknowns for each of the elements as well. 👍

So yeah the logic awkwardly becomes the more complex: On the types.List call ElementsAs() into a []types.Object, range through those (now types.Object), handle null/unknown, then As() into the model type and set the values.

Yay for generating this code to remove that complexity!

[bendbennett]

Have updated the logic here. Thoughts?

[bflad]

I think this might need to be a pointer, or there needs to be nil handling in the range below, since there can be an input such as []*apisdk.Type{nil, nil, nil} 👍

[bendbennett]

Good shout. Have updated to handle potential for nil(s) being returned.

[bflad]

Bikeshed: If we're avoiding methods, which would give us some implicit function namespacing (but I also understand we don't need anything from the model), I think these function names will need to include the names of both sides of the conversion to make more sense. I personally am confused what a "to ListNestedAttributeAssocatedExtType" is doing by the name as it sounds like it'd be making the framework value for that attribute even though the function signature says otherwise. 😅

[bendbennett]

I guess we have a few options here:

• ToType()
• FromListNestedAttributeAssocType()
• FromListNestedAttributeAssocTypeToType()
• ExpandListNestedAttributeAssocType()
• ExpandListNestedAttributeAssocTypeToType()

And the converse for the other side of the operation:

• FromType()
• ToListNestedAttributeAssocType()
• ToListNestedAttributeAssocTypeFromType()
• FlattenType()
• FlattenTypeToListNestedAttributeAssocType()

I don't have any strong feelings on these options but I agree that the current implementation is confusing, should definitely be FromListNestedAttributeAssocType().

Let me know your thoughts and I'll fix this up 👍

[bflad]

My initial thoughts are that --

We should avoid the legacy "flatten"/"expand" terminology that was originally in Terraform when the config/plan/state data was literally represented by a "flat map" (e.g. map[string]string). Provider code that went from that flat map data into API SDK types with real data structures was "expand"-ing the data and the opposite was "flatten"-ing the data. I'm not sure that terminology would be understood in the context of the framework or modern Terraform, which now uses real data structures for its data representations.

I worry about "to/from attribute" naming since there can be multiple API SDK types that need to be associated.

I like the succinctness of "to/from API SDK type" naming, since that's the functionality we're trying to achieve here, but I think that only makes sense if the naming is occurring as a method on something associated with the attribute (e.g. its model type). An option we could explore in this area is whether we could use the "model type" as a custom type implementation of basetypes.ObjectTypable/basetypes.ObjectValuable. In that design, instead of working with types.Object/basetypes.ObjectValue separately from a model type, the model types (now plural) work double duty as the schema type and value type with methods for creating/representing that value with each API SDK type.

The "from/to attribute to/from API SDK type" naming certainly gives the best chance out of these for reducing naming collisions, but its quite verbose, and still has the issue of there potentially be collisions. For true safety, there might need to be a concatenation of the prior attribute path, e.g. let's say there are two separate nested attributes with a duplicate named child attribute and both pointing at the same API SDK type. There's going to be duplicated generated code that cannot compile.

I think given these initial options, I'd sway towards either the verbose naming for its naming safety or, maybe preferably, trying to determine if custom types is feasible and potentially more desirable. At the end of the day though, verbosity in naming or creating types is not the biggest deal, because this is generated code after all. Maybe best to talk through this in person next week. 😄

[bflad]

I'm okay approving this as a first iteration with the understanding that some of the little details might change in the near future and to unblock some of the other planned work. 👍

[bflad]

Nit: Seems like this logic (and others below) can be consolidated into one loop:

	apiObjects := make([]*apisdk.Type, 0, len(listObjects))

	for _, listObject := range listObjects {
		if listObject.IsNull() {
			apiObjects = append(apiObjects, nil)

			continue
		}

		if listObject.IsUnknown() {
			diags.Append(diag.NewErrorDiagnostic(
				"Object Value Within List Is Unknown",
				`Model field "ListNestedAttributeAssocExtType" contains an object which is unknown.`,
			))

			return nil, diags
		}

		var tfModel ListNestedAttributeAssocExtTypeModel

		d := listObject.As(ctx, &tfModel, basetypes.ObjectAsOptions{})

		diags.Append(d...)

		if diags.HasError() {
			return nil, diags
		}

		apiObjects = append(apiObjects, &apisdk.Type{
			BoolAttribute:    tfModel.BoolAttribute.ValueBoolPointer(),
			Float64Attribute: tfModel.Float64Attribute.ValueFloat64Pointer(),
			Int64Attribute:   tfModel.Int64Attribute.ValueInt64Pointer(),
			NumberAttribute:  tfModel.NumberAttribute.ValueBigFloat(),
			StringAttribute:  tfModel.StringAttribute.ValueStringPointer(),
		})
	}

[bflad]

Would it be a little easier if the AssocExtType had an additional Imports() *Imports method and the attributes implemented AssocExtType instead of the spec types?

[bendbennett]

I've refactored along these lines.

[austinvalle]

Looked through the changes and everything makes sense to me, great work! 👍🏻

[github-actions]

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.