Replace primary_identifier_field_name with is_primary_key (#190)

Description of changes:
We have had quite a few issues with the logic of `SetResourceIdentifiers` struggling to find the correct identifier field name or matching it to the corresponding spec or status field. The current implementation solves the first of these issues by pointing directly to the identifier member in the input shape, but the logic to link this back to a spec or status field is still flawed. 
Rather than pointing to a member in the input shape and trying to infer the spec or status field, this new implementation directly denotes a field on a resource as the "primary key". The `SetResourceIdentifiers` logic can now directly find the target field and create the corresponding setter. The override logic for ARNs has now been moved to a `is_arn_primary_key` on the `ResourceConfig` object, since we cannot access the ARN as a `FieldConfig`.

This is a **backwards compatibility breaking change** as all existing `primary_identifier_field_name` configurations need to be refactored into `is_primary_key` fields on the respective resource's field. For example:
```yaml
operations:
  DescribeDBInstances:
    primary_identifier_field_name: DBInstanceIdentifier
  DescribeDBSubnetGroups:
    primary_identifier_field_name: DBSubnetGroupName
```

becomes:
```yaml
resources:
  DBInstance:
    fields:
      DBInstanceIdentifier:
        is_primary_key: true
  DBSubnetGroup:
    fields:
      Name:
        is_primary_key: true
```

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

Author: RedbackThomson

pkg/generate/code/common.go

@@ -131,12 +131,6 @@ func FindPrimaryIdentifierFieldNames(
 ) (crField string, shapeField string) {
 	shape := op.InputRef.Shape
 
-	// Attempt to fetch the primary identifier override from the configuration
-	opConfig, ok := cfg.Operations[op.Name]
-	if ok {
-		shapeField = opConfig.PrimaryIdentifierFieldName
-	}
-
 	if shapeField == "" {
 		// For ReadOne, search for a direct identifier
 		if op == r.Ops.ReadOne {
@@ -151,8 +145,8 @@ func FindPrimaryIdentifierFieldNames(
 			default:
 				panic("Found multiple possible primary identifiers for " +
 					r.Names.Original + ". Set " +
-					"`primary_identifier_field_name` for the " + op.Name +
-					" operation in the generator config.")
+					"`is_primary_key` for the primary field in the " +
+					r.Names.Camel + " resource.")
 			}
 		} else {
 			// For ReadMany, search for pluralized identifiers
@@ -162,12 +156,12 @@ func FindPrimaryIdentifierFieldNames(
 		// Require override if still can't find any identifiers
 		if shapeField == "" {
 			panic("Could not find primary identifier for " + r.Names.Original +
-				". Set `primary_identifier_field_name` for the " + op.Name +
-				" operation in the generator config.")
+				". Set `is_primary_key` for the primary field in the " +
+				r.Names.Camel + " resource.")
 		}
 	}
 
-	if r.IsPrimaryARNField(shapeField) || shapeField == PrimaryIdentifierARNOverride {
+	if r.IsPrimaryARNField(shapeField) {
 		return "", PrimaryIdentifierARNOverride
 	}
 

pkg/generate/code/set_resource.go

@@ -788,12 +788,14 @@ func SetResourceGetAttributes(
 // that an ARN, ID or Name) and any other "additional keys" required for the AWS
 // service to uniquely identify the object.
 //
-// The method will attempt to use the `ReadOne` operation, if present, otherwise
-// will fall back to using `ReadMany`. If it detects the operation uses an ARN
-// to identify the resource it will read it from the metadata status field.
-// Otherwise it will use any fields with a matching name in the operation,
-// pulling from spec or status - requiring that exactly one of those fields is
-// marked as the "primary" identifier.
+// The method will attempt to look for the field denoted with a value of true
+// for `is_primary_key`, or will use the ARN if the resource has a value of true
+// for `is_arn_primary_key`. Otherwise, the method will attempt to use the
+// `ReadOne` operation, if present, falling back to using `ReadMany`.
+// If it detects the operation uses an ARN to identify the resource it will read
+// it from the metadata status field. Otherwise it will use any field with a
+// name that matches the primary identifier from the operation, pulling from
+// top-level spec or status fields.
 //
 // An example of code with no additional keys:
 //
@@ -873,21 +875,42 @@ func SetResourceIdentifiers(
 	primaryKeyConditionalOut := "\n"
 	primaryKeyConditionalOut += identifierNameOrIDGuardConstructor(sourceVarName, indentLevel)
 
-	primaryCRField, primaryShapeField := FindPrimaryIdentifierFieldNames(cfg, r, op)
-	if primaryShapeField == PrimaryIdentifierARNOverride {
-		// if r.ko.Status.ACKResourceMetadata == nil {
-		//  r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
-		// }
-		// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
-		arnOut := "\n"
-		arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel)
-		arnOut += fmt.Sprintf(
-			"%s%s.Status.ACKResourceMetadata.ARN = %s.ARN\n",
-			indent, targetVarName, sourceVarName,
-		)
+	// if r.ko.Status.ACKResourceMetadata == nil {
+	//  r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
+	// }
+	// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
+	arnOut := "\n"
+	arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel)
+	arnOut += fmt.Sprintf(
+		"%s%s.Status.ACKResourceMetadata.ARN = %s.ARN\n",
+		indent, targetVarName, sourceVarName,
+	)
 
+	// Check if the CRD defines the primary keys
+	if r.IsARNPrimaryKey() {
 		return arnOut
 	}
+	primaryField, err := r.GetPrimaryKeyField()
+	if err != nil {
+		panic(err)
+	}
+
+	var primaryCRField, primaryShapeField string
+	isPrimarySet := primaryField != nil
+	if isPrimarySet {
+		memberPath, _ := findFieldInCR(cfg, r, primaryField.Names.Original)
+		targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath)
+		primaryKeyOut += setResourceIdentifierPrimaryIdentifier(cfg, r,
+			primaryField,
+			targetVarPath,
+			sourceVarName,
+			indentLevel)
+	} else {
+		primaryCRField, primaryShapeField = FindPrimaryIdentifierFieldNames(cfg, r, op)
+		if primaryShapeField == PrimaryIdentifierARNOverride {
+			return arnOut
+		}
+	}
 
 	paginatorFieldLookup := []string{
 		"NextToken",
@@ -924,6 +947,11 @@ func SetResourceIdentifiers(
 			op.Name, memberName,
 		)
 
+		// Check to see if we've already set the field as the primary identifier
+		if isPrimarySet && renamedName == primaryField.Names.Camel {
+			continue
+		}
+
 		isPrimaryIdentifier := memberName == primaryShapeField
 
 		searchField := ""
@@ -934,7 +962,7 @@ func SetResourceIdentifiers(
 		}
 
 		memberPath, targetField := findFieldInCR(cfg, r, searchField)
-		if targetField == nil {
+		if targetField == nil || (isPrimarySet && targetField == primaryField) {
 			continue
 		}
 

pkg/generate/config/field.go

@@ -170,11 +170,11 @@ type FieldConfig struct {
 	// Required indicates whether this field is a required member or not.
 	// This field is used to configure '+kubebuilder:validation:Required' on API object's members.
 	IsRequired *bool `json:"is_required,omitempty"`
-	// IsName indicates the field represents the name/string identifier field
-	// for the resource.  This allows the generator config to override the
-	// default behaviour of considering a field called "Name" or
+	// IsPrimaryKey indicates the field represents the primary name/string
+	// identifier field for the resource.  This allows the generator config to
+	// override the default behaviour of considering a field called "Name" or
 	// "{Resource}Name" or "{Resource}Id" as the "name field" for the resource.
-	IsName bool `json:"is_name"`
+	IsPrimaryKey bool `json:"is_primary_key"`
 	// IsOwnerAccountID indicates the field contains the AWS Account ID
 	// that owns the resource. This is a special field that we direct to
 	// storage in the common `Status.ACKResourceMetadata.OwnerAccountID` field.

pkg/generate/config/operation.go

@@ -18,7 +18,7 @@ import (
 
 	awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"
 
-	"github.com/aws-controllers-k8s/code-generator/pkg/util"	
+	"github.com/aws-controllers-k8s/code-generator/pkg/util"
 )
 
 // StringArray is a type that can be represented in JSON as *either* a string
@@ -45,10 +45,6 @@ type OperationConfig struct {
 	// An example of this is `Put...` or `Register...` API operations not being correctly classified as `Create` op type
 	// OperationType []string `json:"operation_type"`
 	OperationType StringArray `json:"operation_type"`
-	// PrimaryIdentifierFieldName provides the name of the field that should be
-	// interpreted as the "primary" identifier field. This field will be used as
-	// the primary field for resource adoption.
-	PrimaryIdentifierFieldName string `json:"primary_identifier_field_name,omitempty"`
 }
 
 // IsIgnoredOperation returns true if Operation Name is configured to be ignored

pkg/generate/config/resource.go

@@ -82,6 +82,9 @@ type ResourceConfig struct {
 	// Print contains instructions for the code generator to generate kubebuilder printcolumns
 	// marker comments.
 	Print *PrintConfig `json:"print,omitempty"`
+	// IsARNPrimaryKey determines whether the CRD uses the ARN as the primary
+	// identifier in the ReadOne operations.
+	IsARNPrimaryKey bool `json:"is_arn_primary_key"`
 }
 
 // HooksConfig instructs the code generator how to inject custom callback hooks

pkg/model/crd.go

@@ -353,6 +353,46 @@ func (r *CRD) HasImmutableFieldChanges() bool {
 	return false
 }
 
+// IsARNPrimaryKey returns true if the CRD uses its ARN as its primary key in
+// ReadOne calls.
+func (r *CRD) IsARNPrimaryKey() bool {
+	if r.cfg == nil {
+		return false
+	}
+	resGenConfig, found := r.cfg.Resources[r.Names.Original]
+	if !found {
+		return false
+	}
+	return resGenConfig.IsARNPrimaryKey
+}
+
+// GetPrimaryKeyField returns the field designated as the primary key, nil if
+// none are specified or an error if multiple are designated.
+func (r *CRD) GetPrimaryKeyField() (*Field, error) {
+	fConfigs := r.cfg.ResourceFields(r.Names.Original)
+
+	var primaryField *Field
+	for fieldName, fieldConfig := range fConfigs {
+		if !fieldConfig.IsPrimaryKey {
+			continue
+		}
+
+		// Multiple primary fields
+		if primaryField != nil {
+			return nil, fmt.Errorf("multiple fields are marked with is_primary_key")
+		}
+
+		fieldNames := names.New(fieldName)
+		fPath := fieldNames.Camel
+		var notFound bool
+		primaryField, notFound = r.Fields[fPath]
+		if !notFound {
+			return nil, fmt.Errorf("what the frick")
+		}
+	}
+	return primaryField, nil
+}
+
 // SetOutputCustomMethodName returns custom set output operation as *string for
 // given operation on custom resource, if specified in generator config
 func (r *CRD) SetOutputCustomMethodName(
@@ -536,14 +576,12 @@ func (r *CRD) GetCustomCheckRequiredFieldsMissingMethod(
 
 // SpecIdentifierField returns the name of the "Name" or string identifier field
 // in the Spec.
-// This method does not guarantee that the identifier field returned is the
-// primary identifier used in any of the `Read*` operations.
 func (r *CRD) SpecIdentifierField() *string {
 	if r.cfg != nil {
 		rConfig, found := r.cfg.Resources[r.Names.Original]
 		if found {
 			for fName, fConfig := range rConfig.Fields {
-				if fConfig.IsName {
+				if fConfig.IsPrimaryKey {
 					return &fName
 				}
 			}

pkg/testdata/models/apis/ecr/0000-00-00/generator-with-field-config.yaml

@@ -2,7 +2,7 @@ resources:
   Repository:
     fields:
       Name:
-        is_name: true
+        is_primary_key: true
     exceptions:
       errors:
         404:

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

@@ -1,13 +1,15 @@
 ignore:
   shape_names:
     - DBSecurityGroupMembershipList
-operations:
-  DescribeDBInstances:
-    primary_identifier_field_name: DBInstanceIdentifier
-  DescribeDBSubnetGroups:
-    primary_identifier_field_name: DBSubnetGroupName
 resources:
+  DBInstance:
+    fields:
+      DBInstanceIdentifier:
+        is_primary_key: true
   DBSubnetGroup:
+    fields:
+      Name:
+        is_primary_key: true
     renames:
       operations:
         DescribeDBSubnetGroups:

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

@@ -22,9 +22,8 @@ resources:
   Endpoint:
     reconcile: 
       requeue_on_success_seconds: 10
-operations:
-  DescribeModelPackage:
-    primary_identifier_field_name: ARN
+  ModelPackage:
+    is_arn_primary_key: true
 ignore:
     resource_names:
       - Algorithm