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 1/2] 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() + }) ); } From d2f90fb5ab9be47faddcb4a2660f95edb4b7ba29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Fri, 14 Oct 2022 11:48:57 +0200 Subject: [PATCH 2/2] s/sex/gender/g in test case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Teo Klestrup Röijezon --- kube-derive/tests/crd_schema_test.rs | 32 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/kube-derive/tests/crd_schema_test.rs b/kube-derive/tests/crd_schema_test.rs index 989e87291..a7d48cb82 100644 --- a/kube-derive/tests/crd_schema_test.rs +++ b/kube-derive/tests/crd_schema_test.rs @@ -79,31 +79,31 @@ 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! @@ -134,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(), @@ -159,7 +159,7 @@ fn test_serialized_matches_expected() { }, "untaggedEnumPerson": { "age": 42, - "sex": "Male" + "gender": "Male" } } }) @@ -283,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"