Fix output_fields and identifier override logic (#228)

Issue #, if available: aws-controllers-k8s/community#1023

Description of changes:
* updates to consume `output_fields` override
* updates FindIdentifier logic to handle identifier overrides (sourced from `input_fields` or `output_fields`)

Verification:
* Successfully compiled and ran EC2-Controller after generating *SecurityGroup* resource using the same generator config in `testdata/`

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: brycahta

pkg/generate/code/check.go

@@ -182,7 +182,7 @@ func checkRequiredFieldsMissingFromShapeReadMany(
 	indent := strings.Repeat("\t", indentLevel)
 	result := fmt.Sprintf("%sreturn false", indent)
 
-	reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape)
+	reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape, op)
 
 	resVarPath, err := r.GetSanitizedMemberPath(reqIdentifier, op, koVarName)
 	if err != nil {

pkg/generate/code/common.go

@@ -32,11 +32,14 @@ var (
 // can be singular or plural.
 func FindIdentifiersInShape(
 	r *model.CRD,
-	shape *awssdkmodel.Shape) []string {
+	shape *awssdkmodel.Shape,
+	op *awssdkmodel.Operation,
+) []string {
 	var identifiers []string
 	if r == nil || shape == nil {
 		return identifiers
 	}
+
 	identifierLookup := []string{
 		"Id",
 		"Ids",
@@ -48,16 +51,23 @@ func FindIdentifiersInShape(
 		r.Names.Original + "Names",
 	}
 
+	// Handles field renames
+	opType, _ := model.GetOpTypeAndResourceNameFromOpID(op.Name, r.Config())
+	renames, _ := r.GetAllRenames(opType)
 	for _, memberName := range shape.MemberNames() {
-		if util.InStrings(memberName, identifierLookup) {
-			identifiers = append(identifiers, memberName)
+		lookupName := memberName
+		if renamedName, found := renames[memberName]; found {
+			lookupName = renamedName
+		}
+		if util.InStrings(lookupName, identifierLookup) {
+			identifiers = append(identifiers, lookupName)
 		}
 	}
 
 	return identifiers
 }
 
-// FindIdentifiersInShape returns the identifier fields of a given shape which
+// FindARNIdentifiersInShape returns the identifier fields of a given shape which
 // fit expect an ARN.
 func FindARNIdentifiersInShape(
 	r *model.CRD,
@@ -79,16 +89,17 @@ func FindARNIdentifiersInShape(
 
 // FindPluralizedIdentifiersInShape returns the name of a Spec OR Status field
 // that has a matching pluralized field in the given shape and the name of
-// the corresponding shape field name. This method will returns the original
-// field name - renames will need to be handled separately.
+// the corresponding shape field name. This method handles identifier field
+// renames and will return the same, when applicable.
 // For example, DescribeVpcsInput has a `VpcIds` field which would be matched
 // to the `Status.VPCID` CRD field - the return value would be
 // "VPCID", "VpcIds".
 func FindPluralizedIdentifiersInShape(
 	r *model.CRD,
 	shape *awssdkmodel.Shape,
+	op *awssdkmodel.Operation,
 ) (crField string, shapeField string) {
-	shapeIdentifiers := FindIdentifiersInShape(r, shape)
+	shapeIdentifiers := FindIdentifiersInShape(r, shape, op)
 	crIdentifiers := r.GetIdentifiers()
 	if len(shapeIdentifiers) == 0 || len(crIdentifiers) == 0 {
 		return "", ""
@@ -99,8 +110,8 @@ func FindPluralizedIdentifiersInShape(
 		for _, ci := range crIdentifiers {
 			if strings.EqualFold(pluralize.Singular(si),
 				pluralize.Singular(ci)) {
-				// The CRD identifiers being used for comparison reflect the
-				// *original* field names in the API model shape.
+				// The CRD identifiers being used for comparison reflect any
+				// renamed field names in the API model shape.
 				if crField == "" {
 					crField = ci
 					shapeField = si
@@ -109,8 +120,7 @@ func FindPluralizedIdentifiersInShape(
 					// 'Id' identifier. Checking 'Id' to determine resource
 					// creation should be safe as the field is usually
 					// present in CR.Status.
-					if !strings.HasSuffix(crField, "Id") ||
-						!strings.HasSuffix(crField, "Ids") {
+					if !strings.Contains(crField, "Id") {
 						crField = ci
 						shapeField = si
 					}
@@ -134,7 +144,7 @@ func FindPrimaryIdentifierFieldNames(
 	if shapeField == "" {
 		// For ReadOne, search for a direct identifier
 		if op == r.Ops.ReadOne {
-			identifiers := FindIdentifiersInShape(r, shape)
+			identifiers := FindIdentifiersInShape(r, shape, op)
 			identifiers = append(identifiers, FindARNIdentifiersInShape(r, shape)...)
 
 			switch len(identifiers) {
@@ -150,7 +160,7 @@ func FindPrimaryIdentifierFieldNames(
 			}
 		} else {
 			// For ReadMany, search for pluralized identifiers
-			crField, shapeField = FindPluralizedIdentifiersInShape(r, shape)
+			crField, shapeField = FindPluralizedIdentifiersInShape(r, shape, op)
 		}
 
 		// Require override if still can't find any identifiers
@@ -166,18 +176,15 @@ func FindPrimaryIdentifierFieldNames(
 	}
 
 	if crField == "" {
-		renamedName, _ := r.InputFieldRename(
-			op.Name, shapeField,
-		)
-
-		_, inSpec := r.SpecFields[renamedName]
-		_, inStatus := r.StatusFields[renamedName]
-		if inSpec || inStatus {
-			crField = renamedName
-		} else {
-			panic("Could not find corresponding spec or status field for primary identifier " + shapeField)
+		if inSpec, inStat := r.HasMember(shapeField, op.Name); !inSpec && !inStat {
+			panic("Could not find corresponding spec or status field " +
+				"for primary identifier " + shapeField)
 		}
+		crField, _ = cfg.ResourceFieldRename(
+			r.Names.Original,
+			op.Name,
+			shapeField,
+		)
 	}
-
 	return crField, shapeField
 }

pkg/generate/code/common_test.go

@@ -35,8 +35,8 @@ func TestFindIdentifiersInShape_EC2_VPC_ReadMany(t *testing.T) {
 	require.NotNil(crd)
 
 	expIdentifier := "VpcIds"
-	actualIdentifiers := code.FindIdentifiersInShape(crd,
-		crd.Ops.ReadMany.InputRef.Shape)
+	op := crd.Ops.ReadMany
+	actualIdentifiers := code.FindIdentifiersInShape(crd, op.InputRef.Shape, op)
 	assert.Len(actualIdentifiers, 1)
 	assert.Equal(
 		strings.TrimSpace(expIdentifier),
@@ -52,9 +52,8 @@ func TestFindIdentifiersInCRD_S3_Bucket_ReadMany_Empty(t *testing.T) {
 
 	crd := testutil.GetCRDByName(t, g, "Bucket")
 	require.NotNil(crd)
-
-	actualIdentifiers := code.FindIdentifiersInShape(crd,
-		crd.Ops.ReadMany.InputRef.Shape)
+	op := crd.Ops.ReadMany
+	actualIdentifiers := code.FindIdentifiersInShape(crd, op.InputRef.Shape, op)
 	assert.Len(actualIdentifiers, 0)
 }
 
@@ -133,8 +132,9 @@ func TestFindPluralizedIdentifiersInShape_EC2_VPC_ReadMany(t *testing.T) {
 
 	expModelIdentifier := "VpcId"
 	expShapeIdentifier := "VpcIds"
+	op := crd.Ops.ReadMany
 	crIdentifier, shapeIdentifier := code.FindPluralizedIdentifiersInShape(crd,
-		crd.Ops.ReadMany.InputRef.Shape)
+		op.InputRef.Shape, op)
 
 	assert.Equal(expModelIdentifier, crIdentifier)
 	assert.Equal(expShapeIdentifier, shapeIdentifier)
@@ -153,6 +153,27 @@ func TestFindPrimaryIdentifierFieldNames_APIGatewayV2_API_ReadOne(t *testing.T)
 	crIdentifier, shapeIdentifier := code.FindPrimaryIdentifierFieldNames(
 		crd.Config(), crd, crd.Ops.ReadOne)
 
+	assert.Equal(expModelIdentifier, crIdentifier)
+	assert.Equal(expShapeIdentifier, shapeIdentifier)
+}
+
+func TestFindPluralizedIdentifiersInShape_EC2_SG_ReadMany_Rename(t *testing.T) {
+	assert := assert.New(t)
+	require := require.New(t)
+
+	g := testutil.NewModelForService(t, "ec2")
+	crd := testutil.GetCRDByName(t, g, "SecurityGroup")
+	require.NotNil(crd)
+
+	// generator.yaml should override GroupId to Id
+	// SecurityGroup also has 2 potential identifiers: Id and Name (post-rename)
+	// Id should take priority
+	expModelIdentifier := "Id"
+	expShapeIdentifier := "Ids"
+	op := crd.Ops.ReadMany
+	crIdentifier, shapeIdentifier := code.FindPluralizedIdentifiersInShape(crd,
+		op.InputRef.Shape, op)
+
 	assert.Equal(expModelIdentifier, crIdentifier)
 	assert.Equal(expShapeIdentifier, shapeIdentifier)
 }

pkg/generate/code/set_resource.go

@@ -135,7 +135,7 @@ func SetResource(
 	out := "\n"
 	indent := strings.Repeat("\t", indentLevel)
 
-	// Recursively descend down through the set of fields on the Output shape,
+	// Recursively descend through the set of fields on the Output shape,
 	// creating temporary variables, populating those temporary variables'
 	// fields with further-nested fields as needed
 	for memberIndex, memberName := range outputShape.MemberNames() {
@@ -197,25 +197,27 @@ func SetResource(
 		// Determine whether the input shape's field is in the Spec or the
 		// Status struct and set the source variable appropriately.
 		var f *model.Field
-		var found bool
 		var targetMemberShapeRef *awssdkmodel.ShapeRef
 		targetAdaptedVarName := targetVarName
 
-		// Check that the field has potentially been renamed
-		renamedName, _ := r.InputFieldRename(
-			op.Name, memberName,
+		// Handles field renames, if applicable
+		fieldName, _ := cfg.ResourceFieldRename(
+			r.Names.Original,
+			op.Name,
+			memberName,
 		)
-		f, found = r.SpecFields[renamedName]
-		if found {
+		inSpec, inStatus := r.HasMember(fieldName, op.Name)
+		if inSpec {
 			targetAdaptedVarName += cfg.PrefixConfig.SpecField
-		} else {
-			f, found = r.StatusFields[memberName]
-			if !found {
-				// TODO(jaypipes): check generator config for exceptions?
-				continue
-			}
+			f = r.SpecFields[fieldName]
+		} else if inStatus {
 			targetAdaptedVarName += cfg.PrefixConfig.StatusField
+			f = r.StatusFields[fieldName]
+		} else {
+			// TODO(jaypipes): check generator config for exceptions?
+			continue
 		}
+
 		targetMemberShapeRef = f.ShapeRef
 		// fieldVarName is the name of the variable that is used for temporary
 		// storage of complex member field values
@@ -415,13 +417,11 @@ func setResourceReadMany(
 	// operation by checking for matching values in these fields.
 	matchFieldNames := r.ListOpMatchFieldNames()
 
-	for _, matchFieldName := range matchFieldNames {
-		_, foundSpec := r.SpecFields[matchFieldName]
-		_, foundStatus := r.StatusFields[matchFieldName]
-		if !foundSpec && !foundStatus {
+	for _, mfName := range matchFieldNames {
+		if inSpec, inStat := r.HasMember(mfName, op.Name); !inSpec && !inStat {
 			msg := fmt.Sprintf(
 				"Match field name %s is not in %s Spec or Status fields",
-				matchFieldName, r.Names.Camel,
+				mfName, r.Names.Camel,
 			)
 			panic(msg)
 		}
@@ -476,30 +476,32 @@ func setResourceReadMany(
 		// Determine whether the input shape's field is in the Spec or the
 		// Status struct and set the source variable appropriately.
 		var f *model.Field
-		var found bool
 		var targetMemberShapeRef *awssdkmodel.ShapeRef
 		targetAdaptedVarName := targetVarName
-		// Check that the field has potentially been renamed
-		renamedName, foundInputFieldRename := r.InputFieldRename(
-			op.Name, memberName,
+
+		// Handles field renames, if applicable
+		fieldName, foundFieldRename := cfg.ResourceFieldRename(
+			r.Names.Original,
+			op.Name,
+			memberName,
 		)
-		f, found = r.SpecFields[renamedName]
-		if found {
+		inSpec, inStatus := r.HasMember(fieldName, op.Name)
+		if inSpec {
 			targetAdaptedVarName += cfg.PrefixConfig.SpecField
-		} else {
-			f, found = r.StatusFields[renamedName]
-			if !found {
-				if foundInputFieldRename {
-					msg := fmt.Sprintf(
-						"Input field rename %s for operation %s is not part of %s Spec or Status fields",
-						memberName, op.Name, r.Names.Camel,
-					)
-					panic(msg)
-				}
-				continue
-			}
+			f = r.SpecFields[fieldName]
+		} else if inStatus {
 			targetAdaptedVarName += cfg.PrefixConfig.StatusField
+			f = r.StatusFields[fieldName]
+		} else if foundFieldRename {
+			msg := fmt.Sprintf(
+				"Field rename %s for operation %s is not part of %s Spec or"+
+					" Status fields", memberName, op.Name, r.Names.Camel)
+			panic(msg)
+		} else {
+			// field not found in Spec or Status
+			continue
 		}
+
 		targetMemberShapeRef = f.ShapeRef
 		out += fmt.Sprintf(
 			"%s\tif %s != nil {\n", indent, sourceAdaptedVarName,
@@ -541,7 +543,7 @@ func setResourceReadMany(
 			//                  continue
 			//              }
 			//          }
-			if util.InStrings(renamedName, matchFieldNames) {
+			if util.InStrings(fieldName, matchFieldNames) {
 				out += fmt.Sprintf(
 					"%s\t\tif %s.%s != nil {\n",
 					indent,
@@ -933,23 +935,25 @@ func SetResourceIdentifiers(
 			continue
 		}
 
-		// Check that the field has potentially been renamed
-		renamedName, _ := r.InputFieldRename(
-			op.Name, memberName,
+		// Handles field renames, if applicable
+		fieldName, _ := cfg.ResourceFieldRename(
+			r.Names.Original,
+			op.Name,
+			memberName,
 		)
 
 		// Check to see if we've already set the field as the primary identifier
-		if isPrimarySet && renamedName == primaryField.Names.Camel {
+		if isPrimarySet && fieldName == primaryField.Names.Camel {
 			continue
 		}
 
-		isPrimaryIdentifier := memberName == primaryShapeField
+		isPrimaryIdentifier := fieldName == primaryShapeField
 
 		searchField := ""
 		if isPrimaryIdentifier {
 			searchField = primaryCRField
 		} else {
-			searchField = renamedName
+			searchField = fieldName
 		}
 
 		memberPath, targetField := findFieldInCR(cfg, r, searchField)
@@ -978,7 +982,7 @@ func SetResourceIdentifiers(
 				targetField,
 				targetVarPath,
 				sourceVarName,
-				names.New(renamedName).CamelLower,
+				names.New(fieldName).CamelLower,
 				indentLevel)
 		}
 	}

pkg/generate/code/set_resource_test.go

@@ -3120,3 +3120,27 @@ func TestSetResource_EC2_VPC_SetResourceIdentifiers(t *testing.T) {
 		code.SetResourceIdentifiers(crd.Config(), crd, "identifier", "r.ko", 1),
 	)
 }
+
+func TestSetResource_EC2_SecurityGroups_SetResourceIdentifiers(t *testing.T) {
+	assert := assert.New(t)
+	require := require.New(t)
+
+	g := testutil.NewModelForService(t, "ec2")
+
+	crd := testutil.GetCRDByName(t, g, "SecurityGroup")
+	require.NotNil(crd)
+
+	// CreateSecurityGroup Output returns a GroupId,
+	// which has been renamed to ID in the CRD
+	expected := `
+	if resp.GroupId != nil {
+		ko.Status.ID = resp.GroupId
+	} else {
+		ko.Status.ID = nil
+	}
+`
+	assert.Equal(
+		expected,
+		code.SetResource(crd.Config(), crd, model.OpTypeCreate, "resp", "ko", 1),
+	)
+}