Skip to content

Allow nested aliases #118

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow nested aliases #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions internal/mapper/attrmapper/data_source_attributes.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I only left comments on this file, but the same feedback applies to internal/mapper/attrmapper/resource_attributes.go

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/explorer"
"github.com/hashicorp/terraform-plugin-codegen-spec/datasource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type DataSourceAttribute interface {
Expand All @@ -20,6 +21,7 @@ type DataSourceAttribute interface {

type DataSourceNestedAttribute interface {
ApplyNestedOverride([]string, explorer.Override) (DataSourceAttribute, error)
NestedMerge([]string, DataSourceAttribute, schema.ComputedOptionalRequired) (DataSourceAttribute, error)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more explicit name like NestedMergeAtPath? (I'm not particularly fond of how our original code is named 😆)

}

type DataSourceAttributes []DataSourceAttribute
Expand Down Expand Up @@ -58,6 +60,46 @@ func (targetSlice DataSourceAttributes) Merge(mergeSlices ...DataSourceAttribute
return targetSlice, errResult
}

func (targetSlice DataSourceAttributes) MergeAttribute(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttributes, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can give this a more explicit name like MergeAttributeAtPath

var errResult error
if len(path) == 0 {
return targetSlice, errResult
}
for i, target := range targetSlice {
if target.GetName() == path[0] {

if len(path) > 1 {
nestedTarget, ok := target.(DataSourceNestedAttribute)
if !ok {
// TODO: error? there is a nested override for an attribute that is not a nested type
break
}

// The attribute we need to override is deeper nested, move up
nextPath := path[1:]

overriddenTarget, err := nestedTarget.NestedMerge(nextPath, attribute, intermediateComputability)
errResult = errors.Join(errResult, err)

targetSlice[i] = overriddenTarget

} else {
// No more path to traverse, apply merge, bidirectional
overriddenTarget, err := attribute.Merge(target)
errResult = errors.Join(errResult, err)
overriddenTarget, err = overriddenTarget.Merge(attribute)
errResult = errors.Join(errResult, err)
Comment on lines +87 to +91
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting, can you describe why this merge logic needs to happen bidirectionally? Would love to see this codified into a unit test with maybe a comment if this bidirectional merge is required.


targetSlice[i] = overriddenTarget
}

break
}
}

return targetSlice, errResult
}
Comment on lines +63 to +101
Copy link
Member

Choose a reason for hiding this comment

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

This logic will also need to take into account the scenario of a new parameter being merged (not aliased), similar to the original Merge function.

Example:

isNewAttribute := true
for i, target := range targetSlice {
	if target.GetName() == path[0] {
		if len(path) > 1 {
			// Recurse logic
		} else {
			// Merge logic
		}

		isNewAttribute = false
		break
	}
}

if isNewAttribute {
	// Add this back to the original slice to avoid adding duplicate attributes from different mergeSlices
	targetSlice = append(targetSlice, attribute)
}


func (attributes DataSourceAttributes) ToSpec() []datasource.Attribute {
specAttributes := make([]datasource.Attribute, 0, len(attributes))
for _, attribute := range attributes {
Expand Down
21 changes: 21 additions & 0 deletions internal/mapper/attrmapper/list_nested.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/hashicorp/terraform-plugin-codegen-spec/datasource"
"github.com/hashicorp/terraform-plugin-codegen-spec/provider"
"github.com/hashicorp/terraform-plugin-codegen-spec/resource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type ResourceListNestedAttribute struct {
Expand Down Expand Up @@ -50,6 +51,16 @@ func (a *ResourceListNestedAttribute) ApplyNestedOverride(path []string, overrid
return a, err
}

func (a *ResourceListNestedAttribute) NestedMerge(path []string, attribute ResourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (ResourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}
Comment on lines +57 to +59
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming the intention here is to bubble up the computability? So if a required parameter is aliased into a nested list that is computed, the nested list is marked as required?

Would like to have a unit test that codifies this behavior


return a, err
}

func (a *ResourceListNestedAttribute) ToSpec() resource.Attribute {
a.ListNestedAttribute.NestedObject = resource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down Expand Up @@ -100,6 +111,16 @@ func (a *DataSourceListNestedAttribute) ApplyNestedOverride(path []string, overr
return a, err
}

func (a *DataSourceListNestedAttribute) NestedMerge(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *DataSourceListNestedAttribute) ToSpec() datasource.Attribute {
a.ListNestedAttribute.NestedObject = datasource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down
21 changes: 21 additions & 0 deletions internal/mapper/attrmapper/map_nested.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/hashicorp/terraform-plugin-codegen-spec/datasource"
"github.com/hashicorp/terraform-plugin-codegen-spec/provider"
"github.com/hashicorp/terraform-plugin-codegen-spec/resource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type ResourceMapNestedAttribute struct {
Expand Down Expand Up @@ -50,6 +51,16 @@ func (a *ResourceMapNestedAttribute) ApplyNestedOverride(path []string, override
return a, err
}

func (a *ResourceMapNestedAttribute) NestedMerge(path []string, attribute ResourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (ResourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *ResourceMapNestedAttribute) ToSpec() resource.Attribute {
a.MapNestedAttribute.NestedObject = resource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down Expand Up @@ -100,6 +111,16 @@ func (a *DataSourceMapNestedAttribute) ApplyNestedOverride(path []string, overri
return a, err
}

func (a *DataSourceMapNestedAttribute) NestedMerge(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *DataSourceMapNestedAttribute) ToSpec() datasource.Attribute {
a.MapNestedAttribute.NestedObject = datasource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down
42 changes: 42 additions & 0 deletions internal/mapper/attrmapper/resource_attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/explorer"
"github.com/hashicorp/terraform-plugin-codegen-spec/resource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type ResourceAttribute interface {
Expand All @@ -20,6 +21,7 @@ type ResourceAttribute interface {

type ResourceNestedAttribute interface {
ApplyNestedOverride([]string, explorer.Override) (ResourceAttribute, error)
NestedMerge([]string, ResourceAttribute, schema.ComputedOptionalRequired) (ResourceAttribute, error)
}

type ResourceAttributes []ResourceAttribute
Expand Down Expand Up @@ -58,6 +60,46 @@ func (targetSlice ResourceAttributes) Merge(mergeSlices ...ResourceAttributes) (
return targetSlice, errResult
}

func (targetSlice ResourceAttributes) MergeAttribute(path []string, attribute ResourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (ResourceAttributes, error) {
var errResult error
if len(path) == 0 {
return targetSlice, errResult
}
for i, target := range targetSlice {
if target.GetName() == path[0] {

if len(path) > 1 {
nestedTarget, ok := target.(ResourceNestedAttribute)
if !ok {
// TODO: error? there is a nested override for an attribute that is not a nested type
break
}

// The attribute we need to override is deeper nested, move up
nextPath := path[1:]

overriddenTarget, err := nestedTarget.NestedMerge(nextPath, attribute, intermediateComputability)
errResult = errors.Join(errResult, err)

targetSlice[i] = overriddenTarget

} else {
// No more path to traverse, apply merge, bidirectional
overriddenTarget, err := attribute.Merge(target)
errResult = errors.Join(errResult, err)
overriddenTarget, err = overriddenTarget.Merge(attribute)
errResult = errors.Join(errResult, err)

targetSlice[i] = overriddenTarget
}

break
}
}

return targetSlice, errResult
}

func (attributes ResourceAttributes) ToSpec() []resource.Attribute {
specAttributes := make([]resource.Attribute, 0, len(attributes))
for _, attribute := range attributes {
Expand Down
21 changes: 21 additions & 0 deletions internal/mapper/attrmapper/set_nested.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/hashicorp/terraform-plugin-codegen-spec/datasource"
"github.com/hashicorp/terraform-plugin-codegen-spec/provider"
"github.com/hashicorp/terraform-plugin-codegen-spec/resource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type ResourceSetNestedAttribute struct {
Expand Down Expand Up @@ -50,6 +51,16 @@ func (a *ResourceSetNestedAttribute) ApplyNestedOverride(path []string, override
return a, err
}

func (a *ResourceSetNestedAttribute) NestedMerge(path []string, attribute ResourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (ResourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *ResourceSetNestedAttribute) ToSpec() resource.Attribute {
a.SetNestedAttribute.NestedObject = resource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down Expand Up @@ -100,6 +111,16 @@ func (a *DataSourceSetNestedAttribute) ApplyNestedOverride(path []string, overri
return a, err
}

func (a *DataSourceSetNestedAttribute) NestedMerge(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttribute, error) {
var err error
a.NestedObject.Attributes, err = a.NestedObject.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *DataSourceSetNestedAttribute) ToSpec() datasource.Attribute {
a.SetNestedAttribute.NestedObject = datasource.NestedAttributeObject{
Attributes: a.NestedObject.Attributes.ToSpec(),
Expand Down
21 changes: 21 additions & 0 deletions internal/mapper/attrmapper/single_nested.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/hashicorp/terraform-plugin-codegen-spec/datasource"
"github.com/hashicorp/terraform-plugin-codegen-spec/provider"
"github.com/hashicorp/terraform-plugin-codegen-spec/resource"
"github.com/hashicorp/terraform-plugin-codegen-spec/schema"
)

type ResourceSingleNestedAttribute struct {
Expand Down Expand Up @@ -50,6 +51,16 @@ func (a *ResourceSingleNestedAttribute) ApplyNestedOverride(path []string, overr
return a, err
}

func (a *ResourceSingleNestedAttribute) NestedMerge(path []string, attribute ResourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (ResourceAttribute, error) {
var err error
a.Attributes, err = a.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *ResourceSingleNestedAttribute) ToSpec() resource.Attribute {
a.SingleNestedAttribute.Attributes = a.Attributes.ToSpec()

Expand Down Expand Up @@ -98,6 +109,16 @@ func (a *DataSourceSingleNestedAttribute) ApplyNestedOverride(path []string, ove
return a, err
}

func (a *DataSourceSingleNestedAttribute) NestedMerge(path []string, attribute DataSourceAttribute, intermediateComputability schema.ComputedOptionalRequired) (DataSourceAttribute, error) {
var err error
a.Attributes, err = a.Attributes.MergeAttribute(path, attribute, intermediateComputability)
if err == nil {
a.ComputedOptionalRequired = intermediateComputability
}

return a, err
}

func (a *DataSourceSingleNestedAttribute) ToSpec() datasource.Attribute {
a.SingleNestedAttribute.Attributes = a.Attributes.ToSpec()

Expand Down
13 changes: 8 additions & 5 deletions internal/mapper/datasource_mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package mapper
import (
"fmt"
"log/slog"
"strings"

"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/config"
"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/explorer"
Expand Down Expand Up @@ -106,7 +107,6 @@ func generateDataSourceSchema(logger *slog.Logger, name string, dataSource explo
// ****************
// READ Parameters (optional)
// ****************
readParameterAttributes := attrmapper.DataSourceAttributes{}
for _, param := range dataSource.ReadOpParameters() {
if param.In != util.OAS_param_path && param.In != util.OAS_param_query {
continue
Expand All @@ -129,7 +129,7 @@ func generateDataSourceSchema(logger *slog.Logger, name string, dataSource explo
computability = schema.Required
}

// Check for any aliases and replace the paramater name if found
// Check for any aliases and replace the parameter name if found
paramName := param.Name
if aliasedName, ok := dataSource.SchemaOptions.AttributeOptions.Aliases[param.Name]; ok {
pLogger = pLogger.With("param_alias", aliasedName)
Expand All @@ -140,17 +140,20 @@ func generateDataSourceSchema(logger *slog.Logger, name string, dataSource explo
continue
}

parameterAttribute, schemaErr := s.BuildDataSourceAttribute(paramName, computability)
paramPath := strings.Split(paramName, ".")

parameterAttribute, schemaErr := s.BuildDataSourceAttribute(paramPath[len(paramPath)-1], computability)
if schemaErr != nil {
log.WarnLogOnError(pLogger, schemaErr, "skipping mapping of read operation parameter")
continue
}

readParameterAttributes = append(readParameterAttributes, parameterAttribute)
// TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas
readResponseAttributes, _ = readResponseAttributes.MergeAttribute(paramPath, parameterAttribute, computability)
}

// TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas
dataSourceAttributes, _ := readParameterAttributes.Merge(readResponseAttributes)
dataSourceAttributes := readResponseAttributes
Comment on lines 155 to +156
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this renaming of the variable and maybe just change readResponseAttributes -> dataSourceAttributes declared on line 85


// TODO: handle error for overrides
dataSourceAttributes, _ = dataSourceAttributes.ApplyOverrides(dataSource.SchemaOptions.AttributeOptions.Overrides)
Expand Down
11 changes: 7 additions & 4 deletions internal/mapper/resource_mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package mapper
import (
"errors"
"log/slog"
"strings"

"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/config"
"github.com/hashicorp/terraform-plugin-codegen-openapi/internal/explorer"
Expand Down Expand Up @@ -140,7 +141,6 @@ func generateResourceSchema(logger *slog.Logger, explorerResource explorer.Resou
// ****************
// READ Parameters (optional)
// ****************
readParameterAttributes := attrmapper.ResourceAttributes{}
for _, param := range explorerResource.ReadOpParameters() {
if param.In != util.OAS_param_path && param.In != util.OAS_param_query {
continue
Expand Down Expand Up @@ -170,17 +170,20 @@ func generateResourceSchema(logger *slog.Logger, explorerResource explorer.Resou
continue
}

parameterAttribute, schemaErr := s.BuildResourceAttribute(paramName, schema.ComputedOptional)
paramPath := strings.Split(paramName, ".")

parameterAttribute, schemaErr := s.BuildResourceAttribute(paramPath[len(paramPath)-1], schema.ComputedOptional)
if schemaErr != nil {
log.WarnLogOnError(pLogger, schemaErr, "skipping mapping of read operation parameter")
continue
}

readParameterAttributes = append(readParameterAttributes, parameterAttribute)
// TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas
readResponseAttributes, _ = readResponseAttributes.MergeAttribute(paramPath, parameterAttribute, schema.ComputedOptional)
}

// TODO: currently, no errors can be returned from merging, but in the future we should consider raising errors/warnings for unexpected scenarios, like type mismatches between attribute schemas
resourceAttributes, _ := createRequestAttributes.Merge(createResponseAttributes, readResponseAttributes, readParameterAttributes)
resourceAttributes, _ := createRequestAttributes.Merge(createResponseAttributes, readResponseAttributes)
Copy link
Member

Choose a reason for hiding this comment

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

With the introduction of "common" parameters in #114, it's possible a parameter could be intended for a nested attribute from the create response/request as well.

I think to avoid problems with that, we can move this final merge to happen before the parameters are merged, i.e. before line 143. And then update line 183 to merge with resourceAttributes.

So the order would be:

  • Merge createRequestAttributes -> createResponseAttributes -> readResponseAttributes. Resulting in resourceAttributes
  • Then merge each individual parameter attribute into resourceAttributes


// TODO: handle error for overrides
resourceAttributes, _ = resourceAttributes.ApplyOverrides(explorerResource.SchemaOptions.AttributeOptions.Overrides)
Expand Down