Skip to content

Commit 59c6892

Browse files
brycahtabrycahta
authored
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.
1 parent d2b0638 commit 59c6892

File tree

10 files changed

+240
-134
lines changed

10 files changed

+240
-134
lines changed

‎pkg/generate/code/check.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ func checkRequiredFieldsMissingFromShapeReadMany(
182182
indent := strings.Repeat("\t", indentLevel)
183183
result := fmt.Sprintf("%sreturn false", indent)
184184

185-
reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape)
185+
reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape, op)
186186

187187
resVarPath, err := r.GetSanitizedMemberPath(reqIdentifier, op, koVarName)
188188
if err != nil {

‎pkg/generate/code/common.go‎

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,14 @@ var (
3232
// can be singular or plural.
3333
func FindIdentifiersInShape(
3434
r *model.CRD,
35-
shape *awssdkmodel.Shape) []string {
35+
shape *awssdkmodel.Shape,
36+
op *awssdkmodel.Operation,
37+
) []string {
3638
var identifiers []string
3739
if r == nil || shape == nil {
3840
return identifiers
3941
}
42+
4043
identifierLookup := []string{
4144
"Id",
4245
"Ids",
@@ -48,16 +51,23 @@ func FindIdentifiersInShape(
4851
r.Names.Original + "Names",
4952
}
5053

54+
// Handles field renames
55+
opType, _ := model.GetOpTypeAndResourceNameFromOpID(op.Name, r.Config())
56+
renames, _ := r.GetAllRenames(opType)
5157
for _, memberName := range shape.MemberNames() {
52-
if util.InStrings(memberName, identifierLookup) {
53-
identifiers = append(identifiers, memberName)
58+
lookupName := memberName
59+
if renamedName, found := renames[memberName]; found {
60+
lookupName = renamedName
61+
}
62+
if util.InStrings(lookupName, identifierLookup) {
63+
identifiers = append(identifiers, lookupName)
5464
}
5565
}
5666

5767
return identifiers
5868
}
5969

60-
// FindIdentifiersInShape returns the identifier fields of a given shape which
70+
// FindARNIdentifiersInShape returns the identifier fields of a given shape which
6171
// fit expect an ARN.
6272
func FindARNIdentifiersInShape(
6373
r *model.CRD,
@@ -79,16 +89,17 @@ func FindARNIdentifiersInShape(
7989

8090
// FindPluralizedIdentifiersInShape returns the name of a Spec OR Status field
8191
// that has a matching pluralized field in the given shape and the name of
82-
// the corresponding shape field name. This method will returns the original
83-
// field name - renames will need to be handled separately.
92+
// the corresponding shape field name. This method handles identifier field
93+
// renames and will return the same, when applicable.
8494
// For example, DescribeVpcsInput has a `VpcIds` field which would be matched
8595
// to the `Status.VPCID` CRD field - the return value would be
8696
// "VPCID", "VpcIds".
8797
func FindPluralizedIdentifiersInShape(
8898
r *model.CRD,
8999
shape *awssdkmodel.Shape,
100+
op *awssdkmodel.Operation,
90101
) (crField string, shapeField string) {
91-
shapeIdentifiers := FindIdentifiersInShape(r, shape)
102+
shapeIdentifiers := FindIdentifiersInShape(r, shape, op)
92103
crIdentifiers := r.GetIdentifiers()
93104
if len(shapeIdentifiers) == 0 || len(crIdentifiers) == 0 {
94105
return "", ""
@@ -99,8 +110,8 @@ func FindPluralizedIdentifiersInShape(
99110
for _, ci := range crIdentifiers {
100111
if strings.EqualFold(pluralize.Singular(si),
101112
pluralize.Singular(ci)) {
102-
// The CRD identifiers being used for comparison reflect the
103-
// *original* field names in the API model shape.
113+
// The CRD identifiers being used for comparison reflect any
114+
// renamed field names in the API model shape.
104115
if crField == "" {
105116
crField = ci
106117
shapeField = si
@@ -109,8 +120,7 @@ func FindPluralizedIdentifiersInShape(
109120
// 'Id' identifier. Checking 'Id' to determine resource
110121
// creation should be safe as the field is usually
111122
// present in CR.Status.
112-
if !strings.HasSuffix(crField, "Id") ||
113-
!strings.HasSuffix(crField, "Ids") {
123+
if !strings.Contains(crField, "Id") {
114124
crField = ci
115125
shapeField = si
116126
}
@@ -134,7 +144,7 @@ func FindPrimaryIdentifierFieldNames(
134144
if shapeField == "" {
135145
// For ReadOne, search for a direct identifier
136146
if op == r.Ops.ReadOne {
137-
identifiers := FindIdentifiersInShape(r, shape)
147+
identifiers := FindIdentifiersInShape(r, shape, op)
138148
identifiers = append(identifiers, FindARNIdentifiersInShape(r, shape)...)
139149

140150
switch len(identifiers) {
@@ -150,7 +160,7 @@ func FindPrimaryIdentifierFieldNames(
150160
}
151161
} else {
152162
// For ReadMany, search for pluralized identifiers
153-
crField, shapeField = FindPluralizedIdentifiersInShape(r, shape)
163+
crField, shapeField = FindPluralizedIdentifiersInShape(r, shape, op)
154164
}
155165

156166
// Require override if still can't find any identifiers
@@ -166,18 +176,15 @@ func FindPrimaryIdentifierFieldNames(
166176
}
167177

168178
if crField == "" {
169-
renamedName, _ := r.InputFieldRename(
170-
op.Name, shapeField,
171-
)
172-
173-
_, inSpec := r.SpecFields[renamedName]
174-
_, inStatus := r.StatusFields[renamedName]
175-
if inSpec || inStatus {
176-
crField = renamedName
177-
} else {
178-
panic("Could not find corresponding spec or status field for primary identifier " + shapeField)
179+
if inSpec, inStat := r.HasMember(shapeField, op.Name); !inSpec && !inStat {
180+
panic("Could not find corresponding spec or status field " +
181+
"for primary identifier " + shapeField)
179182
}
183+
crField, _ = cfg.ResourceFieldRename(
184+
r.Names.Original,
185+
op.Name,
186+
shapeField,
187+
)
180188
}
181-
182189
return crField, shapeField
183190
}

‎pkg/generate/code/common_test.go‎

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ func TestFindIdentifiersInShape_EC2_VPC_ReadMany(t *testing.T) {
3535
require.NotNil(crd)
3636

3737
expIdentifier := "VpcIds"
38-
actualIdentifiers := code.FindIdentifiersInShape(crd,
39-
crd.Ops.ReadMany.InputRef.Shape)
38+
op := crd.Ops.ReadMany
39+
actualIdentifiers := code.FindIdentifiersInShape(crd, op.InputRef.Shape, op)
4040
assert.Len(actualIdentifiers, 1)
4141
assert.Equal(
4242
strings.TrimSpace(expIdentifier),
@@ -52,9 +52,8 @@ func TestFindIdentifiersInCRD_S3_Bucket_ReadMany_Empty(t *testing.T) {
5252

5353
crd := testutil.GetCRDByName(t, g, "Bucket")
5454
require.NotNil(crd)
55-
56-
actualIdentifiers := code.FindIdentifiersInShape(crd,
57-
crd.Ops.ReadMany.InputRef.Shape)
55+
op := crd.Ops.ReadMany
56+
actualIdentifiers := code.FindIdentifiersInShape(crd, op.InputRef.Shape, op)
5857
assert.Len(actualIdentifiers, 0)
5958
}
6059

@@ -133,8 +132,9 @@ func TestFindPluralizedIdentifiersInShape_EC2_VPC_ReadMany(t *testing.T) {
133132

134133
expModelIdentifier := "VpcId"
135134
expShapeIdentifier := "VpcIds"
135+
op := crd.Ops.ReadMany
136136
crIdentifier, shapeIdentifier := code.FindPluralizedIdentifiersInShape(crd,
137-
crd.Ops.ReadMany.InputRef.Shape)
137+
op.InputRef.Shape, op)
138138

139139
assert.Equal(expModelIdentifier, crIdentifier)
140140
assert.Equal(expShapeIdentifier, shapeIdentifier)
@@ -153,6 +153,27 @@ func TestFindPrimaryIdentifierFieldNames_APIGatewayV2_API_ReadOne(t *testing.T)
153153
crIdentifier, shapeIdentifier := code.FindPrimaryIdentifierFieldNames(
154154
crd.Config(), crd, crd.Ops.ReadOne)
155155

156+
assert.Equal(expModelIdentifier, crIdentifier)
157+
assert.Equal(expShapeIdentifier, shapeIdentifier)
158+
}
159+
160+
func TestFindPluralizedIdentifiersInShape_EC2_SG_ReadMany_Rename(t *testing.T) {
161+
assert := assert.New(t)
162+
require := require.New(t)
163+
164+
g := testutil.NewModelForService(t, "ec2")
165+
crd := testutil.GetCRDByName(t, g, "SecurityGroup")
166+
require.NotNil(crd)
167+
168+
// generator.yaml should override GroupId to Id
169+
// SecurityGroup also has 2 potential identifiers: Id and Name (post-rename)
170+
// Id should take priority
171+
expModelIdentifier := "Id"
172+
expShapeIdentifier := "Ids"
173+
op := crd.Ops.ReadMany
174+
crIdentifier, shapeIdentifier := code.FindPluralizedIdentifiersInShape(crd,
175+
op.InputRef.Shape, op)
176+
156177
assert.Equal(expModelIdentifier, crIdentifier)
157178
assert.Equal(expShapeIdentifier, shapeIdentifier)
158179
}

‎pkg/generate/code/set_resource.go‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func SetResource(
135135
out := "\n"
136136
indent := strings.Repeat("\t", indentLevel)
137137

138-
// Recursively descend down through the set of fields on the Output shape,
138+
// Recursively descend through the set of fields on the Output shape,
139139
// creating temporary variables, populating those temporary variables'
140140
// fields with further-nested fields as needed
141141
for memberIndex, memberName := range outputShape.MemberNames() {
@@ -197,25 +197,27 @@ func SetResource(
197197
// Determine whether the input shape's field is in the Spec or the
198198
// Status struct and set the source variable appropriately.
199199
var f *model.Field
200-
var found bool
201200
var targetMemberShapeRef *awssdkmodel.ShapeRef
202201
targetAdaptedVarName := targetVarName
203202

204-
// Check that the field has potentially been renamed
205-
renamedName, _ := r.InputFieldRename(
206-
op.Name, memberName,
203+
// Handles field renames, if applicable
204+
fieldName, _ := cfg.ResourceFieldRename(
205+
r.Names.Original,
206+
op.Name,
207+
memberName,
207208
)
208-
f, found = r.SpecFields[renamedName]
209-
if found {
209+
inSpec, inStatus := r.HasMember(fieldName, op.Name)
210+
if inSpec {
210211
targetAdaptedVarName += cfg.PrefixConfig.SpecField
211-
} else {
212-
f, found = r.StatusFields[memberName]
213-
if !found {
214-
// TODO(jaypipes): check generator config for exceptions?
215-
continue
216-
}
212+
f = r.SpecFields[fieldName]
213+
} else if inStatus {
217214
targetAdaptedVarName += cfg.PrefixConfig.StatusField
215+
f = r.StatusFields[fieldName]
216+
} else {
217+
// TODO(jaypipes): check generator config for exceptions?
218+
continue
218219
}
220+
219221
targetMemberShapeRef = f.ShapeRef
220222
// fieldVarName is the name of the variable that is used for temporary
221223
// storage of complex member field values
@@ -415,13 +417,11 @@ func setResourceReadMany(
415417
// operation by checking for matching values in these fields.
416418
matchFieldNames := r.ListOpMatchFieldNames()
417419

418-
for _, matchFieldName := range matchFieldNames {
419-
_, foundSpec := r.SpecFields[matchFieldName]
420-
_, foundStatus := r.StatusFields[matchFieldName]
421-
if !foundSpec && !foundStatus {
420+
for _, mfName := range matchFieldNames {
421+
if inSpec, inStat := r.HasMember(mfName, op.Name); !inSpec && !inStat {
422422
msg := fmt.Sprintf(
423423
"Match field name %s is not in %s Spec or Status fields",
424-
matchFieldName, r.Names.Camel,
424+
mfName, r.Names.Camel,
425425
)
426426
panic(msg)
427427
}
@@ -476,30 +476,32 @@ func setResourceReadMany(
476476
// Determine whether the input shape's field is in the Spec or the
477477
// Status struct and set the source variable appropriately.
478478
var f *model.Field
479-
var found bool
480479
var targetMemberShapeRef *awssdkmodel.ShapeRef
481480
targetAdaptedVarName := targetVarName
482-
// Check that the field has potentially been renamed
483-
renamedName, foundInputFieldRename := r.InputFieldRename(
484-
op.Name, memberName,
481+
482+
// Handles field renames, if applicable
483+
fieldName, foundFieldRename := cfg.ResourceFieldRename(
484+
r.Names.Original,
485+
op.Name,
486+
memberName,
485487
)
486-
f, found = r.SpecFields[renamedName]
487-
if found {
488+
inSpec, inStatus := r.HasMember(fieldName, op.Name)
489+
if inSpec {
488490
targetAdaptedVarName += cfg.PrefixConfig.SpecField
489-
} else {
490-
f, found = r.StatusFields[renamedName]
491-
if !found {
492-
if foundInputFieldRename {
493-
msg := fmt.Sprintf(
494-
"Input field rename %s for operation %s is not part of %s Spec or Status fields",
495-
memberName, op.Name, r.Names.Camel,
496-
)
497-
panic(msg)
498-
}
499-
continue
500-
}
491+
f = r.SpecFields[fieldName]
492+
} else if inStatus {
501493
targetAdaptedVarName += cfg.PrefixConfig.StatusField
494+
f = r.StatusFields[fieldName]
495+
} else if foundFieldRename {
496+
msg := fmt.Sprintf(
497+
"Field rename %s for operation %s is not part of %s Spec or"+
498+
" Status fields", memberName, op.Name, r.Names.Camel)
499+
panic(msg)
500+
} else {
501+
// field not found in Spec or Status
502+
continue
502503
}
504+
503505
targetMemberShapeRef = f.ShapeRef
504506
out += fmt.Sprintf(
505507
"%s\tif %s != nil {\n", indent, sourceAdaptedVarName,
@@ -541,7 +543,7 @@ func setResourceReadMany(
541543
// continue
542544
// }
543545
// }
544-
if util.InStrings(renamedName, matchFieldNames) {
546+
if util.InStrings(fieldName, matchFieldNames) {
545547
out += fmt.Sprintf(
546548
"%s\t\tif %s.%s != nil {\n",
547549
indent,
@@ -933,23 +935,25 @@ func SetResourceIdentifiers(
933935
continue
934936
}
935937

936-
// Check that the field has potentially been renamed
937-
renamedName, _ := r.InputFieldRename(
938-
op.Name, memberName,
938+
// Handles field renames, if applicable
939+
fieldName, _ := cfg.ResourceFieldRename(
940+
r.Names.Original,
941+
op.Name,
942+
memberName,
939943
)
940944

941945
// Check to see if we've already set the field as the primary identifier
942-
if isPrimarySet && renamedName == primaryField.Names.Camel {
946+
if isPrimarySet && fieldName == primaryField.Names.Camel {
943947
continue
944948
}
945949

946-
isPrimaryIdentifier := memberName == primaryShapeField
950+
isPrimaryIdentifier := fieldName == primaryShapeField
947951

948952
searchField := ""
949953
if isPrimaryIdentifier {
950954
searchField = primaryCRField
951955
} else {
952-
searchField = renamedName
956+
searchField = fieldName
953957
}
954958

955959
memberPath, targetField := findFieldInCR(cfg, r, searchField)
@@ -978,7 +982,7 @@ func SetResourceIdentifiers(
978982
targetField,
979983
targetVarPath,
980984
sourceVarName,
981-
names.New(renamedName).CamelLower,
985+
names.New(fieldName).CamelLower,
982986
indentLevel)
983987
}
984988
}

‎pkg/generate/code/set_resource_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3120,3 +3120,27 @@ func TestSetResource_EC2_VPC_SetResourceIdentifiers(t *testing.T) {
31203120
code.SetResourceIdentifiers(crd.Config(), crd, "identifier", "r.ko", 1),
31213121
)
31223122
}
3123+
3124+
func TestSetResource_EC2_SecurityGroups_SetResourceIdentifiers(t *testing.T) {
3125+
assert := assert.New(t)
3126+
require := require.New(t)
3127+
3128+
g := testutil.NewModelForService(t, "ec2")
3129+
3130+
crd := testutil.GetCRDByName(t, g, "SecurityGroup")
3131+
require.NotNil(crd)
3132+
3133+
// CreateSecurityGroup Output returns a GroupId,
3134+
// which has been renamed to ID in the CRD
3135+
expected := `
3136+
if resp.GroupId != nil {
3137+
ko.Status.ID = resp.GroupId
3138+
} else {
3139+
ko.Status.ID = nil
3140+
}
3141+
`
3142+
assert.Equal(
3143+
expected,
3144+
code.SetResource(crd.Config(), crd, model.OpTypeCreate, "resp", "ko", 1),
3145+
)
3146+
}

0 commit comments

Comments
 (0)