Merge pull request #1051 from teozkr/fix/hoist-enum-values

Hoist enum values from subschemas

kube-core/src/schema.rs

@@ -32,22 +32,23 @@ impl Visitor for StructuralSchemaRewriter {
     fn visit_schema_object(&mut self, schema: &mut schemars::schema::SchemaObject) {
         schemars::visit::visit_schema_object(self, schema);
 
-        if let Some(one_of) = schema
-            .subschemas
-            .as_mut()
-            .and_then(|subschemas| subschemas.one_of.as_mut())
-        {
-            // Tagged enums are serialized using `one_of`
-            hoist_subschema_properties(one_of, &mut schema.object, &mut schema.instance_type);
-        }
+        if let Some(subschemas) = &mut schema.subschemas {
+            if let Some(one_of) = subschemas.one_of.as_mut() {
+                // Tagged enums are serialized using `one_of`
+                hoist_subschema_properties(one_of, &mut schema.object, &mut schema.instance_type);
 
-        if let Some(any_of) = schema
-            .subschemas
-            .as_mut()
-            .and_then(|subschemas| subschemas.any_of.as_mut())
-        {
-            // Untagged enums are serialized using `any_of`
-            hoist_subschema_properties(any_of, &mut schema.object, &mut schema.instance_type);
+                // "Plain" enums are serialized using `one_of` if they have doc tags
+                hoist_subschema_enum_values(one_of, &mut schema.enum_values, &mut schema.instance_type);
+
+                if one_of.is_empty() {
+                    subschemas.one_of = None;
+                }
+            }
+
+            if let Some(any_of) = &mut subschemas.any_of {
+                // Untagged enums are serialized using `any_of`
+                hoist_subschema_properties(any_of, &mut schema.object, &mut schema.instance_type);
+            }
         }
 
         // check for maps without with properties (i.e. flattened maps)
@@ -65,15 +66,49 @@ impl Visitor for StructuralSchemaRewriter {
     }
 }
 
+/// Bring all plain enum values up to the root schema,
+/// since Kubernetes doesn't allow subschemas to define enum options.
+///
+/// (Enum here means a list of hard-coded values, not a tagged union.)
+fn hoist_subschema_enum_values(
+    subschemas: &mut Vec<Schema>,
+    common_enum_values: &mut Option<Vec<serde_json::Value>>,
+    instance_type: &mut Option<SingleOrVec<InstanceType>>,
+) {
+    subschemas.retain(|variant| {
+        if let Schema::Object(SchemaObject {
+            instance_type: variant_type,
+            enum_values: Some(variant_enum_values),
+            ..
+        }) = variant
+        {
+            if let Some(variant_type) = variant_type {
+                match instance_type {
+                    None => *instance_type = Some(variant_type.clone()),
+                    Some(tpe) => {
+                        if tpe != variant_type {
+                            panic!("Enum variant set {variant_enum_values:?} has type {variant_type:?} but was already defined as {instance_type:?}. The instance type must be equal for all subschema variants.")
+                        }
+                    }
+                }
+            }
+            common_enum_values
+                .get_or_insert_with(Vec::new)
+                .extend(variant_enum_values.iter().cloned());
+            false
+        } else {
+            true
+        }
+    })
+}
+
 /// Bring all property definitions from subschemas up to the root schema,
 /// since Kubernetes doesn't allow subschemas to define properties.
 fn hoist_subschema_properties(
     subschemas: &mut Vec<Schema>,
     common_obj: &mut Option<Box<ObjectValidation>>,
     instance_type: &mut Option<SingleOrVec<InstanceType>>,
 ) {
-    let common_obj = common_obj.get_or_insert_with(Box::<ObjectValidation>::default);
-
     for variant in subschemas {
         if let Schema::Object(SchemaObject {
             instance_type: variant_type,
@@ -82,6 +117,8 @@ fn hoist_subschema_properties(
             ..
         }) = variant
         {
+            let common_obj = common_obj.get_or_insert_with(Box::<ObjectValidation>::default);
+
             if let Some(variant_metadata) = variant_metadata {
                 // Move enum variant description from oneOf clause to its corresponding property
                 if let Some(description) = std::mem::take(&mut variant_metadata.description) {

kube-derive/Cargo.toml

@@ -31,3 +31,4 @@ schemars = { version = "0.8.6", features = ["chrono"] }
 validator = { version = "0.16.0", features = ["derive"] }
 chrono = { version = "0.4.19", default-features = false }
 trybuild = "1.0.48"
+assert-json-diff = "2.0.2"

kube-derive/tests/crd_schema_test.rs

@@ -1,5 +1,6 @@
 #![recursion_limit = "256"]
 
+use assert_json_diff::assert_json_eq;
 use chrono::{DateTime, NaiveDateTime, Utc};
 use kube_derive::CustomResource;
 use schemars::JsonSchema;
@@ -78,33 +79,34 @@ enum ComplexEnum {
 #[serde(rename_all = "camelCase")]
 #[serde(untagged)]
 enum UntaggedEnumPerson {
-    SexAndAge(SexAndAge),
-    SexAndDateOfBirth(SexAndDateOfBirth),
+    GenderAndAge(GenderAndAge),
+    GenderAndDateOfBirth(GenderAndDateOfBirth),
 }
 
 #[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
 #[serde(rename_all = "camelCase")]
-struct SexAndAge {
-    /// Sex of the person
-    sex: Sex,
+struct GenderAndAge {
+    /// Gender of the person
+    gender: Gender,
     /// Age of the person in years
     age: i32,
 }
 
 #[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
 #[serde(rename_all = "camelCase")]
-struct SexAndDateOfBirth {
-    /// Sex of the person
-    sex: Sex,
+struct GenderAndDateOfBirth {
+    /// Gender of the person
+    gender: Gender,
     /// Date of birth of the person as ISO 8601 date
     date_of_birth: String,
 }
 
 #[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
 #[serde(rename_all = "PascalCase")]
-enum Sex {
+enum Gender {
     Female,
     Male,
+    /// This variant has a comment!
     Other,
 }
 
@@ -122,7 +124,7 @@ fn test_shortnames() {
 
 #[test]
 fn test_serialized_matches_expected() {
-    assert_eq!(
+    assert_json_eq!(
         serde_json::to_value(Foo::new("bar", FooSpec {
             non_nullable: "asdf".to_string(),
             non_nullable_with_default: "asdf".to_string(),
@@ -132,9 +134,9 @@ fn test_serialized_matches_expected() {
             nullable_with_default: None,
             timestamp: DateTime::from_utc(NaiveDateTime::from_timestamp(0, 0), Utc),
             complex_enum: ComplexEnum::VariantOne { int: 23 },
-            untagged_enum_person: UntaggedEnumPerson::SexAndAge(SexAndAge {
+            untagged_enum_person: UntaggedEnumPerson::GenderAndAge(GenderAndAge {
                 age: 42,
-                sex: Sex::Male,
+                gender: Gender::Male,
             })
         }))
         .unwrap(),
@@ -157,7 +159,7 @@ fn test_serialized_matches_expected() {
                 },
                 "untaggedEnumPerson": {
                     "age": 42,
-                    "sex": "Male"
+                    "gender": "Male"
                 }
             }
         })
@@ -167,9 +169,9 @@ fn test_serialized_matches_expected() {
 #[test]
 fn test_crd_schema_matches_expected() {
     use kube::core::CustomResourceExt;
-    assert_eq!(
+    assert_json_eq!(
         Foo::crd(),
-        serde_json::from_value(serde_json::json!({
+        serde_json::json!({
             "apiVersion": "apiextensions.k8s.io/v1",
             "kind": "CustomResourceDefinition",
             "metadata": {
@@ -281,18 +283,18 @@ fn test_crd_schema_matches_expected() {
                                                         "type": "string",
                                                         "description": "Date of birth of the person as ISO 8601 date"
                                                     },
-                                                    "sex": {
+                                                    "gender": {
                                                         "type": "string",
                                                         "enum": ["Female", "Male", "Other"],
-                                                        "description": "Sex of the person"
+                                                        "description": "Gender of the person"
                                                     }
                                                 },
                                                 "anyOf": [
                                                     {
-                                                        "required": ["age", "sex"]
+                                                        "required": ["age", "gender"]
                                                     },
                                                     {
-                                                        "required": ["dateOfBirth", "sex"]
+                                                        "required": ["dateOfBirth", "gender"]
                                                     }
                                                 ],
                                                 "description": "This is a untagged enum with a description"
@@ -318,8 +320,7 @@ fn test_crd_schema_matches_expected() {
                     }
                 ]
             }
-        }))
-        .unwrap()
+        })
     );
 }