From 4c5e5a3c29c2bc8592b339a5fd6ae75ec7e76dfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Fri, 14 Oct 2022 11:47:52 +0200 Subject: [PATCH] Hoist enum values from subschemas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1047 Signed-off-by: Teo Klestrup Röijezon --- kube-core/src/schema.rs | 71 +++++++++++++++++++++------- kube-derive/Cargo.toml | 1 + kube-derive/tests/crd_schema_test.rs | 11 +++-- 3 files changed, 61 insertions(+), 22 deletions(-) diff --git a/kube-core/src/schema.rs b/kube-core/src/schema.rs index 241578fb9..03e193d4d 100644 --- a/kube-core/src/schema.rs +++ b/kube-core/src/schema.rs @@ -33,22 +33,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) @@ -66,6 +67,42 @@ 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, + common_enum_values: &mut Option>, + instance_type: &mut Option>, +) { + 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( @@ -73,8 +110,6 @@ fn hoist_subschema_properties( common_obj: &mut Option>, instance_type: &mut Option>, ) { - let common_obj = common_obj.get_or_insert_with(|| Box::new(ObjectValidation::default())); - for variant in subschemas { if let Schema::Object(SchemaObject { instance_type: variant_type, @@ -83,6 +118,8 @@ fn hoist_subschema_properties( .. }) = variant { + let common_obj = common_obj.get_or_insert_with(|| Box::new(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) { diff --git a/kube-derive/Cargo.toml b/kube-derive/Cargo.toml index ecce8116e..2c4be77ff 100644 --- a/kube-derive/Cargo.toml +++ b/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" diff --git a/kube-derive/tests/crd_schema_test.rs b/kube-derive/tests/crd_schema_test.rs index 5b8807bd8..989e87291 100644 --- a/kube-derive/tests/crd_schema_test.rs +++ b/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; @@ -105,6 +106,7 @@ struct SexAndDateOfBirth { enum Sex { 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(), @@ -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": { @@ -318,8 +320,7 @@ fn test_crd_schema_matches_expected() { } ] } - })) - .unwrap() + }) ); }