[cozystack-api] Implement Kubernetes-like defaulting - #1432

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

pkg/registry/apps/application/rest.go (4)

1233-1245: Guard root type to avoid silently coercing non-object specs.

If a misconfigured schema sets a non-object root, we currently coerce to {}. Prefer a fast-fail.

Apply this diff:

 func defaultLikeKubernetes(root *map[string]any, s *structuralschema.Structural) error {
-	v := any(*root)
+	if s == nil {
+		return nil
+	}
+	if s.Generic.Type != "object" {
+		return fmt.Errorf("expected spec schema type=object, got %q", s.Generic.Type)
+	}
+	v := any(*root)
 	nv, err := applyDefaults(v, s, true)
 	if err != nil {
 		return err
 	}
 	obj, _ := nv.(map[string]any)
 	if obj == nil {
 		obj = map[string]any{}
 	}
 	*root = obj
 	return nil
 }

---

1249-1266: Don’t override explicit nulls for objects.

K8s-style defaulting applies to absent fields, not ones explicitly set to null. Today null becomes {} or an object default.

Apply this diff:

 	case "object":
 		mv, isMap := v.(map[string]any)
-		if !isMap || v == nil {
-			if s.Generic.Default.Object != nil && !top {
-				if dm, ok := s.Generic.Default.Object.(map[string]any); ok {
-					mv = cloneMap(dm)
-				}
-			}
-			if mv == nil {
-				mv = map[string]any{}
-			}
-		} else {
+		if v == nil {
+			// honor explicit null (no defaulting)
+			return nil, nil
+		}
+		if !isMap {
+			if s.Generic.Default.Object != nil && !top {
+				if dm, ok := s.Generic.Default.Object.(map[string]any); ok {
+					mv = cloneMap(dm)
+				}
+			}
+			if mv == nil {
+				mv = map[string]any{}
+			}
+		} else {
 			// do not merge object-level default into existing object
 		}

---

1294-1316: Don’t override explicit nulls for arrays.

Null arrays get defaulted/emptied; that diverges from “absent-only” defaulting.

Apply this diff:

 	case "array":
 		sl, isSlice := v.([]any)
-		if !isSlice || v == nil {
+		if v == nil {
+			// honor explicit null (no defaulting)
+			return nil, nil
+		}
+		if !isSlice {
 			if s.Generic.Default.Object != nil {
 				if ds, ok := s.Generic.Default.Object.([]any); ok {
 					sl = cloneSlice(ds)
 				}
 			}
 			if sl == nil {
 				sl = []any{}
 			}
 		}

---

1317-1323: Don’t override explicit nulls for scalars.

Present-but-null fields should not be defaulted.

Apply this diff:

 	default:
-		if v == nil && s.Generic.Default.Object != nil {
-			return clone(s.Generic.Default.Object), nil
-		}
-		return v, nil
+		if v == nil {
+			// honor explicit null (no defaulting)
+			return nil, nil
+		}
+		return v, nil

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/registry/apps/application/rest_defaulting.go (2)

116-128: Good fix: skip known properties when processing AdditionalProperties

Prevents incorrectly applying AP schema to declared properties. This addresses the earlier review note.

---

53-68: Root-type assertion added — good defensive check

Returning an error when the root schema unexpectedly resolves to non-object avoids silent data loss. This aligns with a prior review suggestion.

pkg/registry/apps/application/rest.go (2)

92-100: Accept YAML schemas (not just JSON) when unmarshalling OpenAPI

Config schemas are often YAML. Switch to sigs.k8s.io/yaml to handle both YAML and JSON transparently.

@@
-		var v1js apiextv1.JSONSchemaProps
-		if err := json.Unmarshal([]byte(raw), &v1js); err != nil {
+		var v1js apiextv1.JSONSchemaProps
+		if err := yaml.Unmarshal([]byte(raw), &v1js); err != nil {
 			klog.Errorf("Failed to unmarshal v1 OpenAPI schema: %v", err)
 		} else {
 			scheme := runtime.NewScheme()
-			_ = internalapiext.AddToScheme(scheme)
-			_ = apiextv1.AddToScheme(scheme)
+			if err := internalapiext.AddToScheme(scheme); err != nil {
+				klog.Errorf("Failed to register internal apiext scheme: %v", err)
+			}
+			if err := apiextv1.AddToScheme(scheme); err != nil {
+				klog.Errorf("Failed to register v1 apiext scheme: %v", err)
+			}

Add import:

@@
 	"k8s.io/klog/v2"
+	"sigs.k8s.io/yaml"

---

104-108: Validate Structural schema after construction

Validate to catch subtle schema issues early; keep defaulting predictable.

-			} else {
-				specSchema = s
-			}
+			} else {
+				// Optional: validate structural schema
+				if verrs := structuralschema.ValidateStructural(s); len(verrs) > 0 {
+					klog.Errorf("Structural schema validation errors: %v", verrs)
+				}
+				specSchema = s
+			}

Please confirm the exact signature of ValidateStructural for your k8s dependency version and adjust logging accordingly.

pkg/registry/apps/application/rest_defaulting.go (3)

101-114: Preserve explicit nulls to better match Kubernetes semantics

If a field is present with null, defaults generally should not override it. Today, a present-null scalar gets defaulted via the recursive call. Consider skipping defaulting when the key exists but the value is nil.

-			if cur, ok := mv[name]; ok {
-				cv, err := applyDefaults(cur, &ps, false)
+			if cur, ok := mv[name]; ok {
+				// Preserve explicit nulls: do not apply defaults over a user-provided null
+				if cur == nil {
+					continue
+				}
+				cv, err := applyDefaults(cur, &ps, false)
 				if err != nil {
 					return nil, err
 				}
 				mv[name] = cv
 			}

Please add tests to lock this behavior (e.g., roles: null remains null and does not become ["ingress-nginx"]).

---

118-127: Also preserve explicit nulls for AdditionalProperties entries

Same reasoning for map entries governed by AdditionalProperties.

-			for k, cur := range mv {
+			for k, cur := range mv {
 				if _, isKnown := s.Properties[k]; isKnown {
 					continue
 				}
+				if cur == nil {
+					continue
+				}
 				cv, err := applyDefaults(cur, ap, false)
 				if err != nil {
 					return nil, err
 				}
 				mv[k] = cv
 			}

---

28-51: Note on JSON number types after round‑trip

Unmarshalling into map[string]any parses numbers as float64. Re‑marshalling is fine for JSON, but callers must not rely on Go int types when inspecting Spec maps post‑defaulting. If this could surprise downstream code, consider documenting or normalizing numeric types where critical.

pkg/registry/apps/application/rest_defaulting_test.go (1)

46-57: Add tests for explicit null handling and deep-copy safety

To pin intended semantics and guard regressions:

• Field present with null is not defaulted (e.g., roles: null stays null).
• AdditionalProperties entry present with null is not defaulted.
• Defaults are deep‑cloned (mutating the result doesn’t mutate subsequent default applications).

Example additions (append to the suite):

@@
 	It("does not create new map keys from parent object default", func() {
@@
 	})
+
+	It("preserves explicit null values (no default over null)", func() {
+		spec := map[string]any{
+			"nodeGroups": map[string]any{
+				"md0": map[string]any{
+					"minReplicas": 3,
+					"roles":       nil, // explicit null
+				},
+			},
+		}
+		err := defaultLikeKubernetes(&spec, rootSchema)
+		Expect(err).NotTo(HaveOccurred())
+		ng := spec["nodeGroups"].(map[string]any)["md0"].(map[string]any)
+		Expect(ng).To(HaveKeyWithValue("minReplicas", BeNumerically("==", 3)))
+		Expect(ng).To(HaveKey("roles"))
+		Expect(ng["roles"]).To(BeNil())
+	})
+
+	It("does not default AP entry when value is explicit null", func() {
+		spec := map[string]any{
+			"nodeGroups": map[string]any{
+				"mdX": nil,
+			},
+		}
+		err := defaultLikeKubernetes(&spec, rootSchema)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(spec["nodeGroups"].(map[string]any)).To(HaveKeyWithValue("mdX", BeNil()))
+	})
+
+	It("deep-clones defaults (mutations don’t bleed across calls)", func() {
+		spec1 := map[string]any{
+			"nodeGroups": map[string]any{
+				"md0": map[string]any{},
+			},
+		}
+		spec2 := map[string]any{
+			"nodeGroups": map[string]any{
+				"md0": map[string]any{},
+			},
+		}
+		Expect(defaultLikeKubernetes(&spec1, rootSchema)).To(Succeed())
+		ng1 := spec1["nodeGroups"].(map[string]any)["md0"].(map[string]any)
+		ng1["roles"].([]any)[0] = "mutated"
+		Expect(defaultLikeKubernetes(&spec2, rootSchema)).To(Succeed())
+		ng2 := spec2["nodeGroups"].(map[string]any)["md0"].(map[string]any)
+		Expect(ng2["roles"]).To(ConsistOf("ingress-nginx"))
+	})

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro