fix: Handle property hoisting for tagged enums (oneOf subschemas)

Also update function docs, and expected panic messages

Author: NickLarsenNZ

kube-core/src/schema/mod.rs

@@ -274,8 +274,6 @@ impl Transform for StructuralSchemaRewriter {
             None => return,
         };
 
-        dbg!(&schema);
-
         // Hoist parts of the schema to make it valid for k8s
         hoist_one_of_enum_with_unit_variants(&mut schema);
         hoist_any_of_subschema_with_a_nullable_variant(&mut schema);

kube-core/src/schema/transform_any_of.rs

@@ -83,14 +83,16 @@ fn optional_tagged_enum_with_unit_variants() {
 }
 
 
-/// Replace the schema with the non-null anyOf subschema when the only other subschema is the null schema.
+/// Replace the schema with the anyOf subschema and set to nullable when the
+/// only other subschema is the nullable entry.
 ///
 /// Used for correcting the schema for optional tagged unit enums.
 /// The non-null subschema is hoisted, and nullable will be set to true.
 ///
 /// This will return early without modifications unless:
-/// - There are exactly 2 `anyOf` subschemas
-/// - One subschema represents the `null` (has an enum with a null entry, and nullable set to true)
+/// - There are exactly 2 `anyOf` subschemas.
+/// - One subschema represents the nullability (ie: it has an enum with a single
+///   null entry, and nullable set to true).
 ///
 /// NOTE: This should work regardless of whether other hoisting has been performed or not.
 pub(crate) fn hoist_any_of_subschema_with_a_nullable_variant(kube_schema: &mut SchemaObject) {

kube-core/src/schema/transform_one_of.rs

@@ -56,18 +56,21 @@ fn tagged_enum_with_unit_variants() {
 }
 
 
-/// Replace a list of typed oneOf subschemas with a typed schema level enum
+/// Merge oneOf subschema enums and consts into a schema level enum.
 ///
 /// Used for correcting the schema for tagged enums with unit variants.
-/// NOTE: Subschema descriptions are lost when they are combined into a single enum of the same type.
+///
+/// NOTE: Subschema descriptions are lost when they are combined into a single
+/// enum of the same type.
 ///
 /// This will return early without modifications unless:
-/// - There are `oneOf` subschemas (not empty)
-/// - Each subschema contains an enum
-/// - Each subschema is typed
-/// - Each subschemas types is the same as the others
+/// - There are `oneOf` subschemas (not empty).
+/// - Each subschema contains an `enum` or `const`.
+///
+/// Subschemas must define a type, and they must be the same for all.
 ///
-/// NOTE: This should work regardless of whether other hoisting has been performed or not.
+/// NOTE: This should work regardless of whether other hoisting has been
+/// performed or not.
 pub(crate) fn hoist_one_of_enum_with_unit_variants(kube_schema: &mut SchemaObject) {
     // Run some initial checks in case there is nothing to do
     let SchemaObject {
@@ -113,19 +116,23 @@ pub(crate) fn hoist_one_of_enum_with_unit_variants(kube_schema: &mut SchemaObjec
 
     // For each `oneOf` entry, iterate over the `enum` and `const` values.
     // Panic on an entry that doesn't contain an `enum` or `const`.
-    let new_enums = one_of.iter().flat_map(|schema| match schema {
-        Schema::Object(SchemaObject {
-            enum_values: Some(r#enum),
-            ..
-        }) => r#enum.clone(),
-        // Warning: The `const` check below must come after the enum check above.
-        // Otherwise it will panic on a valid entry with an `enum`.
-        Schema::Object(SchemaObject { other, .. }) => match other.get("const") {
-            Some(r#const) => vec![r#const.clone()],
-            None => panic!("oneOf variant did not provide \"enum\" or \"const\": {schema:#?}"),
-        },
-        Schema::Bool(_) => panic!("oneOf variants can not be of type boolean"),
-    });
+    let new_enums = one_of
+        .iter()
+        .flat_map(|schema| match schema {
+            Schema::Object(SchemaObject {
+                enum_values: Some(r#enum),
+                ..
+            }) => r#enum.clone(),
+            Schema::Object(SchemaObject { other, .. }) => other.get("const").cloned().into_iter().collect(),
+            Schema::Bool(_) => panic!("oneOf variants can not be of type boolean"),
+        })
+        .collect::<Vec<_>>();
+
+    // If there are no enums in the oneOf subschemas, there is nothing more to do here.
+    if new_enums.is_empty() {
+        return;
+    }
+
     // Merge the enums (extend just to be safe)
     kube_schema.enum_values.get_or_insert_default().extend(new_enums);
 

kube-core/src/schema/transform_properties.rs

@@ -1,6 +1,4 @@
-use std::ops::DerefMut;
-
-use crate::schema::{InstanceType, Schema, SchemaObject, SingleOrVec, SubschemaValidation};
+use crate::schema::{InstanceType, Schema, SchemaObject, SingleOrVec};
 
 #[cfg(test)]
 #[test]
@@ -342,13 +340,14 @@ fn invalid_untagged_enum_with_conflicting_variant_fields_after_one_of_hosting()
     hoist_properties_for_any_of_subschemas(&mut actual_converted_schema_object);
 }
 
-/// Take subschema properties and insert them into the schema properties.
+/// Take oneOf or anyOf subschema properties and move them them into the schema
+/// properties.
 ///
 /// Used for correcting the schema for serde untagged structural enums.
-/// NOTE: Due to the nature of "untagging", enum variant doc-comments are not preserved.
+/// NOTE: Doc-comments are not preserved for untagged enums.
 ///
 /// This will return early without modifications unless:
-/// - There are `anyOf` subschemas
+/// - There are `oneOf` or `anyOf` subschemas
 /// - Each subschema has the type "object"
 ///
 /// NOTE: This should work regardless of whether other hoisting has been performed or not.
@@ -363,21 +362,23 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
         return;
     };
 
-    let SubschemaValidation {
-        any_of: Some(any_of),
-        one_of,
-    } = subschemas.deref_mut()
-    else {
-        return;
+    // Deal with both tagged and untagged enums.
+    // Untagged enum descriptions will not be preserved.
+    let (subschemas, preserve_description) = match (subschemas.any_of.as_mut(), subschemas.one_of.as_mut()) {
+        (None, None) => return,
+        (None, Some(one_of)) => (one_of, true),
+        (Some(any_of), None) => (any_of, false),
+        (Some(_), Some(_)) => panic!("oneOf and anyOf are mutually exclusive"),
     };
 
-    if any_of.is_empty() {
+
+    if subschemas.is_empty() {
         return;
     }
 
     // Ensure we aren't looking at the one with a null
     // TODO (@NickLarsenNZ): Combine the logic with the function that covers the nullable anyOf
-    if any_of.len() == 2 {
+    if subschemas.len() == 2 {
         // This is the signature for the null variant, indicating the "other"
         // variant is the subschema that needs hoisting
         let null = serde_json::json!({
@@ -386,7 +387,7 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
         });
 
         // Return if one of the two entries are nulls
-        for value in any_of
+        for value in subschemas
             .iter()
             .map(|x| serde_json::to_value(x).expect("schema should be able to convert to JSON"))
         {
@@ -399,29 +400,47 @@ pub(crate) fn hoist_properties_for_any_of_subschemas(kube_schema: &mut SchemaObj
     // At this point, we can be reasonably sure we need operate on the schema.
     // TODO (@NickLarsenNZ): Return errors instead of panicking, leave panicking up to the infallible schemars::Transform
 
-    // There should not be any oneOf's adjacent to the anyOf
-    if one_of.is_some() {
-        panic!("oneOf is set when there is already an anyOf: {one_of:#?}");
-    }
-
-    let subschemas = any_of
+    let subschemas = subschemas
         .into_iter()
         .map(|schema| match schema {
             Schema::Object(schema_object) => schema_object,
-            Schema::Bool(_) => panic!("oneOf variants can not be of type boolean"),
+            Schema::Bool(_) => panic!("oneOf and anyOf variants cannot be of type boolean"),
         })
         .collect::<Vec<_>>();
 
     for subschema in subschemas {
-        // Drop description/type
         // This will clear out any objects that don't have required/properties fields (so that it appears as: {}).
-        subschema.metadata.take();
+        let metadata = subschema.metadata.take();
         subschema.instance_type.take();
 
         // Set the schema type to object
         kube_schema.instance_type = Some(SingleOrVec::Single(Box::new(InstanceType::Object)));
 
         if let Some(object) = subschema.object.as_deref_mut() {
+            // Kubernetes doesn't allow variants to set additionalProperties
+            object.additional_properties.take();
+
+            // For a tagged enum, we need to preserve the variant description
+            if preserve_description {
+                if object.properties.len() != 1 {
+                    // TODO (@NickLarsenNZ): Use assert_eq with error message (and in the other place)
+                    panic!("Expecting a subschema for a tagged enum variant, so there should only be one property");
+                }
+
+                if let Schema::Object(x) = object
+                    .properties
+                    .values_mut()
+                    .next()
+                    .expect("it's there, trust sbernauer")
+                {
+                    // I hope the new metadata doesn't have default set
+                    // I also hope that we didn't destroy some existing metadata.
+                    // Surely it would only exist in one or the other
+                    // See: https://github.com/kube-rs/kube/blob/98bfbe3d7923321a16ccde9e690fd2ce8c7efc32/kube-core/src/schema.rs#L409-L426
+                    x.metadata = metadata
+                };
+            }
+
             // If properties are set, hoist them to the schema properties.
             // This will panic if duplicate properties are encountered that do not have the same shape.
             // That can happen when the untagged enum variants each refer to structs which contain the same field name.

kube-derive/tests/crd_mixed_enum_test.rs

@@ -53,13 +53,13 @@ fn print_crds() {
 }
 
 #[test]
-#[should_panic = "Enum variant set [String(\"A\")] has type Single(String) but was already defined as Some(Single(Object)). The instance type must be equal for all subschema variants."]
+#[should_panic = "oneOf variants must all have the same type"]
 fn invalid_enum_1() {
     InvalidEnum1::crd();
 }
 
 #[test]
-#[should_panic = "Enum variant set [String(\"A\")] has type Single(String) but was already defined as Some(Single(Object)). The instance type must be equal for all subschema variants."]
+#[should_panic = "oneOf variants must all have the same type"]
 fn invalid_enum_2() {
     InvalidEnum2::crd();
 }
@@ -222,7 +222,7 @@ fn valid_enum_4() {
 }
 
 #[test]
-#[should_panic = "Enum variant set [String(\"A\")] has type Single(String) but was already defined as Some(Single(Object)). The instance type must be equal for all subschema variants."]
+#[should_panic = "oneOf variants must all have the same type"]
 fn struct_with_enum_1() {
     #[derive(CustomResource, Serialize, Deserialize, Debug, Clone, JsonSchema)]
     #[kube(group = "clux.dev", version = "v1", kind = "Foo")]