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

Reference output_wrapper_field_path when generating CRD fields as part of the Create* request in model.go #210

Merged
merged 3 commits into from
Sep 29, 2021
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
11 changes: 1 addition & 10 deletions pkg/generate/code/set_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,23 +110,14 @@ func SetResource(
if op == nil {
return ""
}
outputShape := op.OutputRef.Shape
outputShape, _ := r.GetOutputShape(op)
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll probably want to handle this error, as it's critical. A panic might be a fair response if this returns an error - you cannot find the shape to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetOutputShape has a panic for that scenario. I kept logic consistent with previous behavior when refactoring

Copy link
Contributor

Choose a reason for hiding this comment

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

But the method does throw other errors apart from that panic.
I think it makes sense to catch error and panic
(OR ugly version would be to panic for all errors inside GetOutputShape and remove error from return type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with catching error and panic for this. Throughout the code having a nil shape is not considered an error (ex: line 114 here), and I don't think it'd be correct to start doing so now.

Your suggestion with removing error return type sounds like the right way to go. However, I wouldn't panic for all errors just the scenario where there's a wrapper override that is expected to work. What do you guys think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough...
Since in the existing code outputshape is being returned as nil and then it is being handled in line 114, we do not need the error in method signature.

I would be onboard with removing error from method signature and keep the panic statement in method body

if outputShape == nil {
return ""
}

var err error
// We might be in a "wrapper" shape. Unwrap it to find the real object
// representation for the CRD's createOp/DescribeOP.

// Use the wrapper field path if it's given in the ack-generate config file.
wrapperFieldPath := r.GetOutputWrapperFieldPath(op)
if wrapperFieldPath != nil {
outputShape, err = r.GetWrapperOutputShape(outputShape, *wrapperFieldPath)
if err != nil {
msg := fmt.Sprintf("Unable to unwrap the output shape: %v", err)
panic(msg)
}
sourceVarName += "." + *wrapperFieldPath
} else {
// If the wrapper field path is not specified in the config file and if
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but why didn't this block of code pick up the VpcEndpoint shape as being the wrapper struct?

Copy link
Contributor Author

@brycahta brycahta Sep 30, 2021

Choose a reason for hiding this comment

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

This code works as intended. The issue was model.go:164 executed first and was not checking for overrides initially, and then code-generator would crash because it could not locate VpcEndpoint identifier.

I added GetOutputShape call here after refactoring to clean up the code a bit-- the logic remains the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to clarify that the original use for output_wrapper_field_path was working: You could unwrap a describe output structure into status and spec fields.

What we are adding in this is support for output_wrapper_field_path on a create output. That is, taking a wrapper structure from a create and creating new status and spec fields from its members. CreateVpcEndpointOutput has a wrapper of VpcEndpoint - its members are what we want in the spec and status for the CRD.

Some of the code changes @brycahta has made are re-using existing parts of the code that worked with the describe output to also work with create. I believe this is one of those elements.

Expand Down
87 changes: 36 additions & 51 deletions pkg/generate/code/set_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package code_test

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -23,7 +22,6 @@ import (
"github.com/aws-controllers-k8s/code-generator/pkg/generate/code"
"github.com/aws-controllers-k8s/code-generator/pkg/model"
"github.com/aws-controllers-k8s/code-generator/pkg/testutil"
awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
)

func TestSetResource_APIGWv2_Route_Create(t *testing.T) {
Expand Down Expand Up @@ -2940,7 +2938,23 @@ func TestSetResource_RDS_DBSubnetGroup_ReadMany(t *testing.T) {
)
}

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

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

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

expectedShape := crd.Ops.ReadMany.OutputRef.Shape
outputShape, _ := crd.GetOutputShape(crd.Ops.ReadMany)
assert.Equal(
expectedShape,
outputShape)
}

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

Expand All @@ -2949,54 +2963,25 @@ func TestGetWrapperOutputShape(t *testing.T) {
crd := testutil.GetCRDByName(t, g, "Backup")
require.NotNil(crd)

op := crd.Ops.ReadOne.OutputRef.Shape

type args struct {
outputShape *awssdkmodel.Shape
fieldPath string
}
tests := []struct {
name string
args args
wantErr bool
wantShapeName string
}{
{
name: "incorrect field path: element not found",
args: args{
outputShape: op,
fieldPath: "BackupDescription.Something",
},
wantErr: true,
},
{
name: "incorrect field path: element not of type structure",
args: args{
outputShape: op,
fieldPath: "BackupDescription.BackupArn",
},
wantErr: true,
},
{
name: "correct field path",
args: args{
outputShape: op,
fieldPath: "BackupDescription.BackupDetails",
},
wantErr: false,
wantShapeName: "BackupDetails",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
outputShape, err := crd.GetWrapperOutputShape(tt.args.outputShape, tt.args.fieldPath)
if (err != nil) != tt.wantErr {
assert.Fail(fmt.Sprintf("GetWrapperOutputShape() error = %v, wantErr %v", err, tt.wantErr))
} else if !tt.wantErr {
assert.Equal(tt.wantShapeName, outputShape.ShapeName)
}
})
}
outputShape, _ := crd.GetOutputShape(crd.Ops.ReadOne)
assert.Equal(
"BackupDetails",
outputShape.ShapeName)
}

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

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

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

outputShape, _ := crd.GetOutputShape(crd.Ops.Create)
assert.Equal(
"VpcEndpoint",
outputShape.ShapeName)
}

func TestSetResource_MQ_Broker_SetResourceIdentifiers(t *testing.T) {
Expand Down
52 changes: 20 additions & 32 deletions pkg/model/crd.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,9 +452,9 @@ func (r *CRD) GetOutputWrapperFieldPath(
return &opConfig.OutputWrapperFieldPath
}

// GetOutputShape returns the Output shape for given operation.
// GetOutputShape returns the Output shape for a given operation and applies
// wrapper field path overrides, if specified in generator config.
func (r *CRD) GetOutputShape(
// The operation to look for the Output shape
op *awssdkmodel.Operation,
) (*awssdkmodel.Shape, error) {
if op == nil {
Expand All @@ -466,37 +466,27 @@ func (r *CRD) GetOutputShape(
return nil, errors.New("output shape not found")
}

// We might be in a "wrapper" shape. Unwrap it to find the real object
// representation for the CRD's createOp/DescribeOP.

// Use the wrapper field path if it's given in the ack-generate config file.
// Check for wrapper field path overrides
wrapperFieldPath := r.GetOutputWrapperFieldPath(op)
if wrapperFieldPath != nil {
wrapperOutputShape, err := r.GetWrapperOutputShape(outputShape, *wrapperFieldPath)
wrapperOutputShape, err := r.getWrapperOutputShape(outputShape,
*wrapperFieldPath)
if err != nil {
return nil, fmt.Errorf("unable to unwrap the output shape: %v", err)
msg := fmt.Sprintf("Unable to unwrap the output shape: %s " +
"with field path override: %s. error: %v",
outputShape.OrigShapeName, *wrapperFieldPath, err)
panic(msg)
}
outputShape = wrapperOutputShape
} else {
// If the wrapper field path is not specified in the config file and if
// there is a single member shape and that member shape is a structure,
// unwrap it.
if outputShape.UsedAsOutput && len(outputShape.MemberRefs) == 1 {
for _, memberRef := range outputShape.MemberRefs {
if memberRef.Shape.Type == "structure" {
outputShape = memberRef.Shape
}
}
}
}
return outputShape, nil
}

// GetWrapperOutputShape returns the shape of the last element of a given field
// Path. It carefully unwraps the output shape and verifies that every element
// of the field path exists in their correspanding parent shape and that they are
// getWrapperOutputShape returns the shape of the last element of a given field
// Path. It unwraps the output shape and verifies that every element of the
// field path exists in their corresponding parent shape and that they are
// structures.
func (r *CRD) GetWrapperOutputShape(
func (r *CRD) getWrapperOutputShape(
shape *awssdkmodel.Shape,
fieldPath string,
) (*awssdkmodel.Shape, error) {
Expand All @@ -509,21 +499,19 @@ func (r *CRD) GetWrapperOutputShape(
memberRef, ok := shape.MemberRefs[wrapperField]
if !ok {
return nil, fmt.Errorf(
"Incorrect SetOutput.WrapperFieldPath. Could not find %s in Shape %s",
wrapperField, shape.ShapeName,
)
"could not find wrapper override field %s in Shape %s",
wrapperField, shape.ShapeName)
}

// wrapper field must be structure; otherwise cannot unpack
if memberRef.Shape.Type != "structure" {
// All the mentioned shapes must be structure
return nil, fmt.Errorf(
"Expected SetOutput.WrapperFieldPath to only contain fields of type 'structure'."+
" Found %s of type '%s'",
wrapperField, memberRef.Shape.Type,
)
"output wrapper overrides can only contain fields of type" +
" 'structure'. Found wrapper override field %s of type '%s'",
wrapperField, memberRef.Shape.Type)
}
remainPath := strings.Join(fieldPathParts[1:], ".")
return r.GetWrapperOutputShape(memberRef.Shape, remainPath)
return r.getWrapperOutputShape(memberRef.Shape, remainPath)
}

// GetCustomImplementation returns custom implementation method name for the
Expand Down
5 changes: 4 additions & 1 deletion pkg/model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,10 @@ func (m *Model) GetCRDs() ([]*CRD, error) {
// Now process the fields that will go into the Status struct. We want
// fields that are in the Create operation's Output Shape but that are
// not in the Input Shape.
outputShape := createOp.OutputRef.Shape
outputShape, err := crd.GetOutputShape(createOp)
if err != nil {
return nil, err
}
if outputShape.UsedAsOutput && len(outputShape.MemberRefs) == 1 {
// We might be in a "wrapper" shape. Unwrap it to find the real object
// representation for the CRD's createOp. If there is a single member
Expand Down
12 changes: 8 additions & 4 deletions pkg/testdata/models/apis/ec2/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ ignore:
- InternetGateway
- KeyPair
- LaunchTemplateVersion
# - LaunchTemplate
#- LaunchTemplate
- LocalGatewayRouteTableVpcAssociation
- LocalGatewayRoute
- ManagedPrefixList
Expand Down Expand Up @@ -50,13 +50,17 @@ ignore:
- TransitGatewayRoute
- TransitGatewayVpcAttachment
- TransitGateway
# - Volume
#- Volume
- VpcEndpointConnectionNotification
- VpcEndpointServiceConfiguration
- VpcEndpoint
# - Vpc
#- VpcEndpoint
#- Vpc
- VpcCidrBlock
- VpcPeeringConnection
- VpnConnectionRoute
- VpnConnection
- VpnGateway

operations:
CreateVpcEndpoint:
output_wrapper_field_path: VpcEndpoint