Several minor fixes and improvements

Signed-off-by: jose.vazquez <jose.vazquez@mongodb.com>

Author: josvazg

go.mod

@@ -161,7 +161,7 @@ require (
 	k8s.io/apiextensions-apiserver v0.34.1
 	k8s.io/klog/v2 v2.130.1
 	k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b // indirect
-	k8s.io/utils v0.0.0-20250604170112-4c0f3b243397
+	k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 // indirect
 	sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect
 	sigs.k8s.io/yaml v1.6.0
 )

internal/autogen/translate/crd.go

@@ -39,24 +39,24 @@ func selectVersion(spec *apiextensionsv1.CustomResourceDefinitionSpec, version s
 
 func getOpenAPIProperties(kind string, version *apiextensionsv1.CustomResourceDefinitionVersion) (map[string]apiextensionsv1.JSONSchemaProps, error) {
 	if version == nil {
-		return nil, fmt.Errorf("missing version %q from %v spec", version, kind)
+		return nil, fmt.Errorf("missing version (nil) from %v spec", kind)
 	}
 	if version.Schema == nil {
-		return nil, fmt.Errorf("missing version %q schema from %v spec", version, kind)
+		return nil, fmt.Errorf("missing version schema from %v spec", kind)
 	}
 	if version.Schema.OpenAPIV3Schema == nil {
-		return nil, fmt.Errorf("missing version %q OpenAPI Schema from %v spec", version, kind)
+		return nil, fmt.Errorf("missing version OpenAPI Schema from %v spec", kind)
 	}
 	if version.Schema.OpenAPIV3Schema.Properties == nil {
-		return nil, fmt.Errorf("missing version %q OpenAPI Properties from %v spec", version, kind)
+		return nil, fmt.Errorf("missing version OpenAPI Properties from %v spec", kind)
 	}
 	return version.Schema.OpenAPIV3Schema.Properties, nil
 }
 
 func getSpecPropertiesFor(kind string, props map[string]apiextensionsv1.JSONSchemaProps, field string) (map[string]apiextensionsv1.JSONSchemaProps, error) {
 	prop, ok := props[field]
 	if !ok {
-		return nil, fmt.Errorf(" kind %q spec is missing field %q on", field, kind)
+		return nil, fmt.Errorf("kind %q spec is missing field %q on", kind, field)
 	}
 	if prop.Type != "object" {
 		return nil, fmt.Errorf("kind %q field %q expected to be object but is %v", kind, field, prop.Type)

internal/autogen/translate/crd_test.go

@@ -0,0 +1,255 @@
+// Copyright 2025 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package translate
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+func TestSelectVersion(t *testing.T) {
+	// Define some sample versions to be reused in the tests
+	v1 := apiextensionsv1.CustomResourceDefinitionVersion{Name: "v1", Served: true}
+	v2 := apiextensionsv1.CustomResourceDefinitionVersion{Name: "v2", Served: true}
+	v1beta1 := apiextensionsv1.CustomResourceDefinitionVersion{Name: "v1beta1", Served: true}
+
+	testCases := []struct {
+		name    string
+		spec    *apiextensionsv1.CustomResourceDefinitionSpec
+		version string
+		want    *apiextensionsv1.CustomResourceDefinitionVersion
+	}{
+		{
+			name: "should return nil when spec has no versions",
+			spec: &apiextensionsv1.CustomResourceDefinitionSpec{
+				Versions: []apiextensionsv1.CustomResourceDefinitionVersion{},
+			},
+			version: "v1",
+			want:    nil,
+		},
+		{
+			name: "should return the first version when version string is empty",
+			spec: &apiextensionsv1.CustomResourceDefinitionSpec{
+				Versions: []apiextensionsv1.CustomResourceDefinitionVersion{v1beta1, v1},
+			},
+			version: "",
+			want:    &v1beta1,
+		},
+		{
+			name: "should return the matching version when it exists",
+			spec: &apiextensionsv1.CustomResourceDefinitionSpec{
+				Versions: []apiextensionsv1.CustomResourceDefinitionVersion{v1, v2},
+			},
+			version: "v2",
+			want:    &v2,
+		},
+		{
+			name: "should return nil when the requested version does not exist",
+			spec: &apiextensionsv1.CustomResourceDefinitionSpec{
+				Versions: []apiextensionsv1.CustomResourceDefinitionVersion{v1, v2},
+			},
+			version: "v3",
+			want:    nil,
+		},
+		{
+			name: "should handle a single version in the spec",
+			spec: &apiextensionsv1.CustomResourceDefinitionSpec{
+				Versions: []apiextensionsv1.CustomResourceDefinitionVersion{v1},
+			},
+			version: "v1",
+			want:    &v1,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := selectVersion(tc.spec, tc.version)
+			require.Equal(t, tc.want, got)
+		})
+	}
+}
+
+func TestGetOpenAPIProperties(t *testing.T) {
+	const kind = "MyTestCRD"
+
+	happyPathVersion := &apiextensionsv1.CustomResourceDefinitionVersion{
+		Name: "v1",
+		Schema: &apiextensionsv1.CustomResourceValidation{
+			OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+				Properties: map[string]apiextensionsv1.JSONSchemaProps{
+					"spec": {
+						Type: "object",
+						Properties: map[string]apiextensionsv1.JSONSchemaProps{
+							"fieldA": {Type: "string"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	testCases := []struct {
+		name           string
+		version        *apiextensionsv1.CustomResourceDefinitionVersion
+		wantProperties map[string]apiextensionsv1.JSONSchemaProps
+		wantErrMsg     string
+	}{
+		{
+			name:           "should return properties",
+			version:        happyPathVersion,
+			wantProperties: happyPathVersion.Schema.OpenAPIV3Schema.Properties,
+			wantErrMsg:     "",
+		},
+		{
+			name:           "should return error if version is nil",
+			version:        nil,
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("missing version (nil) from %v spec", kind),
+		},
+		{
+			name: "should return error if schema is nil",
+			version: &apiextensionsv1.CustomResourceDefinitionVersion{
+				Name:   "v1",
+				Schema: nil, // The point of failure
+			},
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("missing version schema from %v spec", kind),
+		},
+		{
+			name: "error - should return error if OpenAPIV3Schema is nil",
+			version: &apiextensionsv1.CustomResourceDefinitionVersion{
+				Name: "v1",
+				Schema: &apiextensionsv1.CustomResourceValidation{
+					OpenAPIV3Schema: nil, // The point of failure
+				},
+			},
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("missing version OpenAPI Schema from %v spec", kind),
+		},
+		{
+			name: "should return error if Properties map is nil",
+			version: &apiextensionsv1.CustomResourceDefinitionVersion{
+				Name: "v1",
+				Schema: &apiextensionsv1.CustomResourceValidation{
+					OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{
+						Properties: nil, // The point of failure
+					},
+				},
+			},
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("missing version OpenAPI Properties from %v spec", kind),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			gotProperties, err := getOpenAPIProperties(kind, tc.version)
+			if tc.wantErrMsg != "" {
+				require.Error(t, err)
+				require.EqualError(t, err, tc.wantErrMsg)
+				require.Nil(t, gotProperties)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tc.wantProperties, gotProperties)
+			}
+		})
+	}
+}
+
+func TestGetSpecPropertiesFor(t *testing.T) {
+	const kind = "MyTestCRD"
+
+	// Reusable properties for the test cases
+	nestedProperties := map[string]apiextensionsv1.JSONSchemaProps{
+		"replicas": {Type: "integer"},
+		"image":    {Type: "string"},
+	}
+
+	testCases := []struct {
+		name           string
+		props          map[string]apiextensionsv1.JSONSchemaProps
+		field          string
+		wantProperties map[string]apiextensionsv1.JSONSchemaProps
+		wantErrMsg     string
+	}{
+		{
+			name: "should return nested properties",
+			props: map[string]apiextensionsv1.JSONSchemaProps{
+				"spec": {
+					Type:       "object",
+					Properties: nestedProperties,
+				},
+				"status": {
+					Type: "object",
+				},
+			},
+			field:          "spec",
+			wantProperties: nestedProperties,
+			wantErrMsg:     "", // Expect no error
+		},
+		{
+			name: "field is missing from properties map",
+			props: map[string]apiextensionsv1.JSONSchemaProps{
+				"status": {Type: "object"},
+			},
+			field:          "spec", // This field does not exist
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("kind %q spec is missing field %q on", kind, "spec"),
+		},
+		{
+			name: "field is not of type object",
+			props: map[string]apiextensionsv1.JSONSchemaProps{
+				"spec": {Type: "string"}, // The point of failure
+			},
+			field:          "spec",
+			wantProperties: nil,
+			wantErrMsg:     fmt.Sprintf("kind %q field %q expected to be object but is %v", kind, "spec", "string"),
+		},
+		{
+			name: "field is an object but has no nested properties",
+			props: map[string]apiextensionsv1.JSONSchemaProps{
+				"spec": {
+					Type:       "object",
+					Properties: nil, // The nested map is nil
+				},
+			},
+			field:          "spec",
+			wantProperties: nil, // Expecting a nil map is correct here
+			wantErrMsg:     "",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Act
+			gotProperties, err := getSpecPropertiesFor(kind, tc.props, tc.field)
+
+			// Assert
+			if tc.wantErrMsg != "" {
+				// We expect an error
+				require.Error(t, err)
+				require.EqualError(t, err, tc.wantErrMsg)
+				require.Nil(t, gotProperties)
+			} else {
+				// We expect success
+				require.NoError(t, err)
+				require.Equal(t, tc.wantProperties, gotProperties)
+			}
+		})
+	}
+}

internal/autogen/translate/mapping.go

@@ -18,7 +18,6 @@ package translate
 import (
 	"errors"
 	"fmt"
-	"reflect"
 
 	"github.com/stretchr/testify/assert/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -91,14 +90,17 @@ func ExpandMappings(r *Request, obj map[string]any, main client.Object) ([]clien
 		return nil, fmt.Errorf("failed to unmarshal mappings YAML: %w", err)
 	}
 
-	if err := em.expandMappingsAt(obj, mappings, "spec", r.Translator.majorVersion); err != nil {
-		return nil, fmt.Errorf("failed to map properties of spec from API to Kubernetes: %w", err)
-	}
-	if err := em.expandMappingsAt(obj, mappings, "spec", r.Translator.majorVersion, "entry"); err != nil {
-		return nil, fmt.Errorf("failed to map properties of spec from API to Kubernetes: %w", err)
-	}
-	if err := em.expandMappingsAt(obj, mappings, "status", r.Translator.majorVersion); err != nil {
-		return nil, fmt.Errorf("failed to map properties of status from API to Kubernetes: %w", err)
+	for _, entry := range []struct {
+		title string
+		path  []string
+	}{
+		{title: "spec", path: []string{"spec", r.Translator.majorVersion}},
+		{title: "spec entry", path: []string{"spec", r.Translator.majorVersion, "entry"}},
+		{title: "status", path: []string{"status", r.Translator.majorVersion}},
+	} {
+		if err := em.expandMappingsAt(obj, mappings, entry.path...); err != nil {
+			return nil, fmt.Errorf("failed to map properties of %q from API to Kubernetes: %w", entry.title, err)
+		}
 	}
 	return em.added, nil
 }
@@ -124,13 +126,6 @@ func CollapseMappings(r *Request, spec map[string]any, main client.Object) error
 	return cm.mapProperties([]string{}, props, spec)
 }
 
-func findEntryPathInTarget(targetType reflect.Type) []string {
-	if targetType.String() == "admin.CreateAlertConfigurationApiParams" {
-		return []string{"GroupAlertsConfig"}
-	}
-	return []string{}
-}
-
 func (m *mapper) expandMappingsAt(obj, mappings map[string]any, fields ...string) error {
 	expandedPath := []string{"properties"}
 	for _, field := range fields {
@@ -200,7 +195,10 @@ func (m *mapper) mapArray(path []string, mapping map[string]any, list []any) err
 		if !ok {
 			return fmt.Errorf("expected field %q at %v to be a map but was: %T", mapName, path, mapItem)
 		}
-		key, entry := entryMatchingMapping(mapName, mapping, list, m.expand)
+		key, entry, err := entryMatchingMapping(mapName, mapping, list, m.expand)
+		if err != nil {
+			return fmt.Errorf("failed to match mapping within array item %q at %v: %w", mapItem, path, err)
+		}
 		if entry == nil {
 			continue
 		}
@@ -238,28 +236,36 @@ func (m *mapper) mapReference(path []string, mappingName string, mapping, obj ma
 	return ref.Collapse(m.mapContext, path, obj)
 }
 
-func entryMatchingMapping(mapName string, mapping map[string]any, list []any, expand bool) (string, map[string]any) {
+func entryMatchingMapping(mapName string, mapping map[string]any, list []any, expand bool) (string, map[string]any, error) {
 	key := mapName
 	if expand {
 		refMap := refMapping{}
 		if err := unstructured.FromUnstructured(&refMap, mapping); err != nil {
-			return "", nil // not a ref, cannot reverse mapping dfrom API property name
+			return "", nil, fmt.Errorf("not a reference, cannot reverse mapping from API property name")
 		}
 		path := resolveXPath(refMap.XOpenAPIMapping.Property)
 		key = unstructured.Base(path)
 	}
-	return key, findByExistingKey(list, key)
+	m, err := findByExistingUniqueKey(list, key)
+	return key, m, err
 }
 
-func findByExistingKey(list []any, key string) map[string]any {
+func findByExistingUniqueKey(list []any, key string) (map[string]any, error) {
+	candidates := []map[string]any{}
 	for _, item := range list {
 		obj, ok := (item).(map[string]any)
 		if !ok {
 			continue
 		}
 		if _, ok := obj[key]; ok {
-			return obj
+			candidates = append(candidates, obj)
 		}
 	}
-	return nil
+	if len(candidates) == 1 {
+		return candidates[0], nil
+	}
+	if len(candidates) > 1 {
+		return nil, fmt.Errorf("too many matches for key %q: %v", key, candidates)
+	}
+	return nil, nil
 }