always use single dot fieldpath notation (#261)

This patch completes earlier work that was done in the `pkg/fieldpath`
package that only uses a single dot notation to refer to a *field* with
type *list* or *map* within a field path.

We previously were using a double dot in field paths in `generator.yaml`
config files to refer to fields of type list or map. This was erroneous,
since referring to the *field* (not the *value* of the field) only
requires a single dot in order for the fieldpath to "locate" a field
within a path.

In this PR, we simplify and DRY up the inference code in
`pkg/model/model.go`'s processing of "nested fields" so that the field
path added for all Field objects uses this single-dot notation only.

Signed-off-by: Jay Pipes <[email protected]>

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

Author: jaypipes

pkg/generate/code/set_sdk.go

@@ -1145,16 +1145,14 @@ func setSDKForSlice(
 	//
 	//  f0elem.SetMyField(*f0iter)
 	containerFieldName := ""
-	sourceAttributePath := sourceFieldPath
 	if targetShape.MemberRef.Shape.Type == "structure" {
 		containerFieldName = targetFieldName
-		sourceAttributePath = sourceFieldPath + "."
 	}
 	out += setSDKForContainer(
 		cfg, r,
 		containerFieldName,
 		elemVarName,
-		sourceAttributePath,
+		sourceFieldPath,
 		iterVarName,
 		&targetShape.MemberRef,
 		indentLevel+1,
@@ -1218,7 +1216,7 @@ func setSDKForMap(
 		cfg, r,
 		containerFieldName,
 		valVarName,
-		sourceFieldPath+".",
+		sourceFieldPath,
 		valIterVarName,
 		&targetShape.ValueRef,
 		indentLevel+1,

pkg/model/field.go

@@ -111,24 +111,6 @@ func (f *Field) IsReference() bool {
 	return trimmedGoType == "*ackv1alpha1.AWSResourceReferenceWrapper"
 }
 
-// ParentFieldPath takes a field path and returns the field path of the
-// containing "parent" field. For example, if the field path
-// `Users..Credentials.Login` is passed in, this function returns
-// `Users..Credentials`. If `Users..Password` is supplied, this function
-// returns `Users`, etc.
-func ParentFieldPath(path string) string {
-	parts := strings.Split(path, ".")
-	// Pop the last element of the supplied field path
-	parts = parts[0 : len(parts)-1]
-	// If the parent field's type is a list or map, there will be two dots ".."
-	// in the supplied field path. We don't want the returned field path to end
-	// in a dot, since that would be invalid, so we trim it off here
-	if parts[len(parts)-1] == "" {
-		parts = parts[0 : len(parts)-1]
-	}
-	return strings.Join(parts, ".")
-}
-
 // NewReferenceField returns a pointer to a new Field object.
 // The go-type of field is either slice of '*AWSResourceReferenceWrapper' or
 // '*AWSResourceReferenceWrapper' depending on whether 'shapeRef' parameter

pkg/model/field_test.go

@@ -6,38 +6,9 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/aws-controllers-k8s/code-generator/pkg/model"
 	"github.com/aws-controllers-k8s/code-generator/pkg/testutil"
 )
 
-func TestParentFieldPath(t *testing.T) {
-	assert := assert.New(t)
-	testCases := []struct {
-		subject string
-		want    string
-	}{
-		{
-			"Repository.Name",
-			"Repository",
-		},
-		{
-			"Users..Password",
-			"Users",
-		},
-		{
-			"User.Credentials..Password",
-			"User.Credentials",
-		},
-	}
-
-	for _, tc := range testCases {
-		result := model.ParentFieldPath(
-			tc.subject,
-		)
-		assert.Equal(tc.want, result)
-	}
-}
-
 func TestMemberFields(t *testing.T) {
 	assert := assert.New(t)
 	require := require.New(t)

pkg/model/model.go

@@ -295,7 +295,7 @@ func (m *Model) GetCRDs() ([]*CRD, error) {
 	// This is the place that we build out the CRD.Fields map with
 	// `pkg/model.Field` objects that represent the non-top-level Spec and
 	// Status fields.
-	m.processNestedFields(crds)
+	m.processFields(crds)
 	m.crds = crds
 	return crds, nil
 }
@@ -472,9 +472,10 @@ func replaceSecretAttrGoType(
 	field *Field,
 	tdefs []*TypeDef,
 ) {
-	fieldPath := field.Path
-	parentFieldPath := ParentFieldPath(field.Path)
-	parentField, ok := crd.Fields[parentFieldPath]
+	fieldPath := ackfp.FromString(field.Path)
+	parentFieldPath := fieldPath.Copy()
+	parentFieldPath.Pop()
+	parentField, ok := crd.Fields[parentFieldPath.String()]
 	if !ok {
 		msg := fmt.Sprintf(
 			"Cannot find parent field at parent path %s for %s",
@@ -546,25 +547,25 @@ func replaceSecretAttrGoType(
 	attr.GoType = "*ackv1alpha1.SecretKeyReference"
 }
 
-// processNestedFields is responsible for walking all of the CRDs' Spec and
+// processFields is responsible for walking all of the CRDs' Spec and
 // Status fields' Shape objects and adding `pkg/model.Field` objects for all
 // nested fields along with that `Field`'s `Config` object that allows us to
 // determine if the TypeDef associated with that nested field should have its
 // data type overridden (e.g. for SecretKeyReferences)
-func (m *Model) processNestedFields(crds []*CRD) {
+func (m *Model) processFields(crds []*CRD) {
 	for _, crd := range crds {
 		for _, field := range crd.SpecFields {
-			m.processNestedField(crd, field)
+			m.processTopLevelField(crd, field)
 		}
 		for _, field := range crd.StatusFields {
-			m.processNestedField(crd, field)
+			m.processTopLevelField(crd, field)
 		}
 	}
 }
 
-// processNestedField processes any nested fields (non-scalar fields associated
+// processTopLevelField processes any nested fields (non-scalar fields associated
 // with the Spec and Status objects)
-func (m *Model) processNestedField(
+func (m *Model) processTopLevelField(
 	crd *CRD,
 	field *Field,
 ) {
@@ -581,106 +582,87 @@ func (m *Model) processNestedField(
 		fieldType := fieldShape.Type
 		switch fieldType {
 		case "structure":
-			m.processNestedStructField(crd, field.Path+".", field)
+			m.processStructField(crd, field.Path+".", field)
 		case "list":
-			m.processNestedListField(crd, field.Path+"..", field)
+			m.processListField(crd, field.Path+".", field)
 		case "map":
-			m.processNestedMapField(crd, field.Path+"..", field)
+			m.processMapField(crd, field.Path+".", field)
 		}
 	}
 }
 
-// processNestedStructField recurses through the members of a nested field that
-// is a struct type and adds any Field objects to the supplied CRD.
-func (m *Model) processNestedStructField(
+// processField adds a new Field definition for a field within the CR
+func (m *Model) processField(
 	crd *CRD,
-	baseFieldPath string,
-	baseField *Field,
+	parentFieldPath string,
+	parentField *Field,
+	fieldName string,
+	fieldShapeRef *awssdkmodel.ShapeRef,
 ) {
 	fieldConfigs := crd.Config().ResourceFields(crd.Names.Original)
-	baseFieldShape := baseField.ShapeRef.Shape
-	for memberName, memberRef := range baseFieldShape.MemberRefs {
-		memberNames := names.New(memberName)
-		memberShape := memberRef.Shape
-		memberShapeType := memberShape.Type
-		fieldPath := baseFieldPath + memberNames.Camel
-		fieldConfig := fieldConfigs[fieldPath]
-		field := NewField(crd, fieldPath, memberNames, memberRef, fieldConfig)
-		switch memberShapeType {
-		case "structure":
-			m.processNestedStructField(crd, fieldPath+".", field)
-		case "list":
-			m.processNestedListField(crd, fieldPath+"..", field)
-		case "map":
-			m.processNestedMapField(crd, fieldPath+"..", field)
-		}
-		crd.Fields[fieldPath] = field
+	fieldNames := names.New(fieldName)
+	fieldShape := fieldShapeRef.Shape
+	fieldShapeType := fieldShape.Type
+	fieldPath := parentFieldPath + fieldNames.Camel
+	fieldConfig := fieldConfigs[fieldPath]
+	field := NewField(crd, fieldPath, fieldNames, fieldShapeRef, fieldConfig)
+	switch fieldShapeType {
+	case "structure":
+		m.processStructField(crd, fieldPath+".", field)
+	case "list":
+		m.processListField(crd, fieldPath+".", field)
+	case "map":
+		m.processMapField(crd, fieldPath+".", field)
+	}
+	crd.Fields[fieldPath] = field
+}
+
+// processStructField recurses through the members of a nested field that
+// is a struct type and adds any Field objects to the supplied CRD.
+func (m *Model) processStructField(
+	crd *CRD,
+	fieldPath string,
+	field *Field,
+) {
+	fieldShape := field.ShapeRef.Shape
+	for memberName, memberRef := range fieldShape.MemberRefs {
+		m.processField(crd, fieldPath, field, memberName, memberRef)
 	}
 }
 
-// processNestedListField recurses through the members of a nested field that
+// processListField recurses through the members of a nested field that
 // is a list type that has a struct element type and adds any Field objects to
 // the supplied CRD.
-func (m *Model) processNestedListField(
+func (m *Model) processListField(
 	crd *CRD,
-	baseFieldPath string,
-	baseField *Field,
+	fieldPath string,
+	field *Field,
 ) {
-	baseFieldShape := baseField.ShapeRef.Shape
-	elementFieldShape := baseFieldShape.MemberRef.Shape
+	fieldShape := field.ShapeRef.Shape
+	elementFieldShape := fieldShape.MemberRef.Shape
 	if elementFieldShape.Type != "structure" {
 		return
 	}
-	fieldConfigs := crd.Config().ResourceFields(crd.Names.Original)
 	for memberName, memberRef := range elementFieldShape.MemberRefs {
-		memberNames := names.New(memberName)
-		memberShape := memberRef.Shape
-		memberShapeType := memberShape.Type
-		fieldPath := baseFieldPath + memberNames.Camel
-		fieldConfig := fieldConfigs[fieldPath]
-		field := NewField(crd, fieldPath, memberNames, memberRef, fieldConfig)
-		switch memberShapeType {
-		case "structure":
-			m.processNestedStructField(crd, fieldPath+".", field)
-		case "list":
-			m.processNestedListField(crd, fieldPath+"..", field)
-		case "map":
-			m.processNestedMapField(crd, fieldPath+"..", field)
-		}
-		crd.Fields[fieldPath] = field
+		m.processField(crd, fieldPath, field, memberName, memberRef)
 	}
 }
 
-// processNestedMapField recurses through the members of a nested field that
+// processMapField recurses through the members of a nested field that
 // is a map type that has a struct value type and adds any Field objects to
 // the supplied CRD.
-func (m *Model) processNestedMapField(
+func (m *Model) processMapField(
 	crd *CRD,
-	baseFieldPath string,
-	baseField *Field,
+	fieldPath string,
+	field *Field,
 ) {
-	baseFieldShape := baseField.ShapeRef.Shape
-	valueFieldShape := baseFieldShape.ValueRef.Shape
+	fieldShape := field.ShapeRef.Shape
+	valueFieldShape := fieldShape.ValueRef.Shape
 	if valueFieldShape.Type != "structure" {
 		return
 	}
-	fieldConfigs := crd.Config().ResourceFields(crd.Names.Original)
 	for memberName, memberRef := range valueFieldShape.MemberRefs {
-		memberNames := names.New(memberName)
-		memberShape := memberRef.Shape
-		memberShapeType := memberShape.Type
-		fieldPath := baseFieldPath + memberNames.Camel
-		fieldConfig := fieldConfigs[fieldPath]
-		field := NewField(crd, fieldPath, memberNames, memberRef, fieldConfig)
-		switch memberShapeType {
-		case "structure":
-			m.processNestedStructField(crd, fieldPath+".", field)
-		case "list":
-			m.processNestedListField(crd, fieldPath+"..", field)
-		case "map":
-			m.processNestedMapField(crd, fieldPath+"..", field)
-		}
-		crd.Fields[fieldPath] = field
+		m.processField(crd, fieldPath, field, memberName, memberRef)
 	}
 }
 

pkg/model/model_mq_test.go

@@ -35,7 +35,7 @@ func TestMQ_Broker(t *testing.T) {
 	// (which is a `[]*User` type) is findable in the CRD's Fields collection
 	// by the path `Spec.Users..Password` and that the FieldConfig associated
 	// with this Field is marked as a SecretKeyReference.
-	passFieldPath := "Users..Password"
+	passFieldPath := "Users.Password"
 	passField, found := crd.Fields[passFieldPath]
 	require.True(found)
 	require.NotNil(passField.FieldConfig)

pkg/testdata/models/apis/mq/0000-00-00/generator.yaml

@@ -10,5 +10,5 @@ resources:
       sdk_delete_pre_build_request:
         template_path: sdk_delete_pre_build_request.go.tpl
     fields:
-      Users..Password:
+      Users.Password:
         is_secret: true