Skip to content

Use SetConfig in SetResourceForStruct when target shape has no members #292

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

Merged
merged 3 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion pkg/generate/ack/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ var (
if setCfg != nil && setCfg.Ignore {
return ""
}
return code.SetResourceForStruct(r.Config(), r, targetFieldName, targetVarName, targetShapeRef, setCfg, sourceVarName, sourceShapeRef, indentLevel)
return code.SetResourceForStruct(r.Config(), r, targetFieldName, targetVarName, targetShapeRef, setCfg, sourceVarName, sourceShapeRef, "", model.OpTypeList, indentLevel)
},
"GoCodeCompare": func(r *ackmodel.CRD, deltaVarName string, sourceVarName string, targetVarName string, indentLevel int) string {
return code.CompareResource(r.Config(), r, deltaVarName, sourceVarName, targetVarName, indentLevel)
Expand Down
84 changes: 81 additions & 3 deletions pkg/generate/code/set_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,8 @@ func SetResource(
setCfg,
sourceAdaptedVarName,
sourceMemberShapeRef,
f.Names.Camel,
opType,
indentLevel+1,
)
out += setResourceForScalar(
Expand Down Expand Up @@ -578,6 +580,8 @@ func setResourceReadMany(
setCfg,
sourceAdaptedVarName,
sourceMemberShapeRef,
f.Names.Camel,
model.OpTypeList,
indentLevel+2,
)
out += setResourceForScalar(
Expand Down Expand Up @@ -1156,6 +1160,9 @@ func setResourceForContainer(
sourceVarName string,
// ShapeRef of the source struct field
sourceShapeRef *awssdkmodel.ShapeRef,
// targetFieldPath is the field path for the target containing field
targetFieldPath string,
op model.OpType,
indentLevel int,
) string {
switch sourceShapeRef.Shape.Type {
Expand All @@ -1168,6 +1175,8 @@ func setResourceForContainer(
targetSetCfg,
sourceVarName,
sourceShapeRef,
targetFieldPath,
op,
indentLevel,
)
case "list":
Expand All @@ -1179,6 +1188,8 @@ func setResourceForContainer(
targetSetCfg,
sourceVarName,
sourceShapeRef,
targetFieldPath,
op,
indentLevel,
)
case "map":
Expand All @@ -1190,6 +1201,8 @@ func setResourceForContainer(
targetSetCfg,
sourceVarName,
sourceShapeRef,
targetFieldPath,
op,
indentLevel,
)
default:
Expand Down Expand Up @@ -1219,15 +1232,21 @@ func SetResourceForStruct(
sourceVarName string,
// ShapeRef of the source struct field
sourceShapeRef *awssdkmodel.ShapeRef,
// targetFieldPath is the field path to targetFieldName
targetFieldPath string,
op model.OpType,
indentLevel int,
) string {
out := ""
indent := strings.Repeat("\t", indentLevel)
sourceShape := sourceShapeRef.Shape
targetShape := targetShapeRef.Shape

var sourceMemberShapeRef *awssdkmodel.ShapeRef
var sourceAdaptedVarName, qualifiedTargetVar string

for _, targetMemberName := range targetShape.MemberNames() {
sourceMemberShapeRef := sourceShape.MemberRefs[targetMemberName]
sourceMemberShapeRef = sourceShape.MemberRefs[targetMemberName]
if sourceMemberShapeRef == nil {
continue
}
Expand All @@ -1244,13 +1263,14 @@ func SetResourceForStruct(
indexedVarName := fmt.Sprintf("%sf%d", targetVarName, sourceMemberIndex)
sourceMemberShape := sourceMemberShapeRef.Shape
targetMemberCleanNames := names.New(targetMemberName)
sourceAdaptedVarName := sourceVarName + "." + targetMemberName
sourceAdaptedVarName = sourceVarName + "." + targetMemberName
out += fmt.Sprintf(
"%sif %s != nil {\n", indent, sourceAdaptedVarName,
)
qualifiedTargetVar := fmt.Sprintf(
qualifiedTargetVar = fmt.Sprintf(
"%s.%s", targetVarName, targetMemberCleanNames.Camel,
)
updatedTargetFieldPath := targetFieldPath + "." + targetMemberCleanNames.Camel

switch sourceMemberShape.Type {
case "list", "structure", "map":
Expand All @@ -1269,6 +1289,8 @@ func SetResourceForStruct(
nil,
sourceAdaptedVarName,
sourceMemberShapeRef,
updatedTargetFieldPath,
op,
indentLevel+1,
)
out += setResourceForScalar(
Expand All @@ -1290,6 +1312,52 @@ func SetResourceForStruct(
"%s}\n", indent,
)
}
if len(targetShape.MemberNames()) == 0 {
// This scenario can occur when the targetShape is a primitive, but
// the sourceShape is a struct. For example, EC2 resource DHCPOptions
// has a field NewDhcpConfiguration.Values(targetShape = string) whose name
// aligns with DhcpConfiguration.Values(sourceShape = AttributeValue).
// Although the names correspond, the shapes/types are different and the intent
// is to set NewDhcpConfiguration.Values using DhcpConfiguration.Values.Value
// (AttributeValue.Value) shape instead. This behavior can be configured using
// SetConfig.

// Check if target field has a SetConfig, validate SetConfig.From points
// to a shape within sourceShape, and generate Go code using
// said shape. Using the example above, SetConfig is set
// for NewDhcpConfiguration.Values and Setconfig.From points
// to AttributeValue.Value (string), which leads to generating Go
// code referencing DhcpConfiguration.Values.Value instead of 'Values'.

if targetField, ok := r.Fields[targetFieldPath]; ok {
setCfg := targetField.GetSetterConfig(op)
if setCfg != nil && setCfg.From != nil {
fp := fieldpath.FromString(*setCfg.From)
sourceMemberShapeRef = fp.ShapeRef(sourceShapeRef)
if sourceMemberShapeRef != nil && sourceMemberShapeRef.Shape != nil {
names := names.New(sourceMemberShapeRef.LocationName)
sourceAdaptedVarName = sourceVarName + "." + names.Camel
out += fmt.Sprintf(
"%sif %s != nil {\n", indent, sourceAdaptedVarName,
)
qualifiedTargetVar = targetVarName

// Use setResourceForScalar and dereference sourceAdaptedVarName
// because primitives are being set.
sourceAdaptedVarName = "*" + sourceAdaptedVarName
Comment on lines +1345 to +1347
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine for now, but I recommend we clean up the setResourceForScalar() method implementation to be able to properly handle this only using the sourceShapeRef information.

out += setResourceForScalar(
qualifiedTargetVar,
sourceAdaptedVarName,
sourceMemberShapeRef,
indentLevel+1,
)
out += fmt.Sprintf(
"%s}\n", indent,
)
}
}
}
}
return out
}

Expand All @@ -1310,6 +1378,9 @@ func setResourceForSlice(
sourceVarName string,
// ShapeRef of the source slice field
sourceShapeRef *awssdkmodel.ShapeRef,
// targetFieldPath is the field path to targetFieldName
targetFieldPath string,
op model.OpType,
indentLevel int,
) string {
out := ""
Expand Down Expand Up @@ -1388,6 +1459,8 @@ func setResourceForSlice(
targetSetCfg,
iterVarName,
&sourceShape.MemberRef,
targetFieldPath,
op,
indentLevel+1,
)
}
Expand Down Expand Up @@ -1421,6 +1494,9 @@ func setResourceForMap(
sourceVarName string,
// ShapeRef of the source map field
sourceShapeRef *awssdkmodel.ShapeRef,
// targetFieldPath is the field path to targetFieldName
targetFieldPath string,
op model.OpType,
indentLevel int,
) string {
out := ""
Expand Down Expand Up @@ -1453,6 +1529,8 @@ func setResourceForMap(
nil,
valIterVarName,
&sourceShape.ValueRef,
targetFieldPath,
op,
indentLevel+1,
)
addressOfVar := ""
Expand Down
72 changes: 72 additions & 0 deletions pkg/generate/code/set_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3169,3 +3169,75 @@ func TestSetResource_IAM_Role_NestedSetConfig(t *testing.T) {
code.SetResource(crd.Config(), crd, model.OpTypeGet, "resp", "ko", 1),
)
}

func TestSetResource_EC2_DHCPOptions_NestedSetConfig(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForService(t, "ec2")

crd := testutil.GetCRDByName(t, g, "DhcpOptions")
require.NotNil(crd)

// The input and output shapes for the DhcpConfigurations.Values are different
// and we have a custom SetConfig for this field in our generator.yaml file
// to configure the SetResource function to set the value of the resource's
// (nested) Spec.DHCPConfigurations.Values to the value of the (nested)
// GetDhcpOptionsResponse.DhcpOptions.DhcpConfigurations.Values.Value field
expected := `
if resp.DhcpOptions.DhcpConfigurations != nil {
f0 := []*svcapitypes.NewDHCPConfiguration{}
for _, f0iter := range resp.DhcpOptions.DhcpConfigurations {
f0elem := &svcapitypes.NewDHCPConfiguration{}
if f0iter.Key != nil {
f0elem.Key = f0iter.Key
}
if f0iter.Values != nil {
f0elemf1 := []*string{}
for _, f0elemf1iter := range f0iter.Values {
var f0elemf1elem string
if f0elemf1iter.Value != nil {
f0elemf1elem = *f0elemf1iter.Value
}
f0elemf1 = append(f0elemf1, &f0elemf1elem)
}
f0elem.Values = f0elemf1
}
f0 = append(f0, f0elem)
}
ko.Spec.DHCPConfigurations = f0
} else {
ko.Spec.DHCPConfigurations = nil
}
if resp.DhcpOptions.DhcpOptionsId != nil {
ko.Status.DHCPOptionsID = resp.DhcpOptions.DhcpOptionsId
} else {
ko.Status.DHCPOptionsID = nil
}
if resp.DhcpOptions.OwnerId != nil {
ko.Status.OwnerID = resp.DhcpOptions.OwnerId
} else {
ko.Status.OwnerID = nil
}
if resp.DhcpOptions.Tags != nil {
f3 := []*svcapitypes.Tag{}
for _, f3iter := range resp.DhcpOptions.Tags {
f3elem := &svcapitypes.Tag{}
if f3iter.Key != nil {
f3elem.Key = f3iter.Key
}
if f3iter.Value != nil {
f3elem.Value = f3iter.Value
}
f3 = append(f3, f3elem)
}
ko.Status.Tags = f3
} else {
ko.Status.Tags = nil
}
`
assert.Equal(
expected,
code.SetResource(crd.Config(), crd, model.OpTypeCreate, "resp", "ko", 1),
)
}
7 changes: 5 additions & 2 deletions pkg/testdata/models/apis/ec2/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,11 @@ operations:
output_wrapper_field_path: VpcEndpoint

resources:
DHCPOptions:

DhcpOptions:
fields:
DHCPConfigurations.Values:
set:
- from: AttributeValue.Value
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might consider adding:

      method: CREATE

here in order to exercise the OpType passing down into the recursive SetResourceForXXX functions.

SecurityGroup:
renames:
operations:
Expand Down