From 140075b58e936c3a03212f9e88a5625b37f75dae Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Fri, 17 May 2024 21:50:32 +0200 Subject: [PATCH 1/8] Improve detecting whether default can be derived This checks if a struct contains a required member for which `Default` is not implemented (Enums or other structs which don't have `Default`). If it does, Default will not be derived for that struct. Signed-off-by: Aaron Dewes --- src/main.rs | 19 ++++++++++--------- src/output.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/main.rs b/src/main.rs index b1037e3..94ddf17 100644 --- a/src/main.rs +++ b/src/main.rs @@ -246,7 +246,7 @@ impl Kopium { } self.print_docstr(&s.docs, ""); if s.is_main_container() { - self.print_derives(s); + self.print_derives(s, &structs); //root struct gets kube derives unless opted out if !self.hide_kube { println!( @@ -277,7 +277,7 @@ impl Kopium { println!("pub struct {} {{", s.name); } } else { - self.print_derives(s); + self.print_derives(s, &structs); let spec_trimmed_name = s.name.as_str().replace(&format!("{}Spec", kind), kind); if s.is_enum { println!("pub enum {} {{", spec_trimmed_name); @@ -338,23 +338,24 @@ impl Kopium { } } - fn print_derives(&self, s: &Container) { + fn print_derives(&self, s: &Container, containers: &[Container]) { let mut derives = vec!["Serialize", "Deserialize", "Clone", "Debug"]; if s.is_main_container() && !self.hide_kube { // CustomResource first for root struct derives.insert(0, "CustomResource"); } - if self.builders { + + // TypedBuilder does not work with enums + if self.builders && !s.is_enum { derives.push("TypedBuilder"); } for derive in &self.derive { - if s.is_enum && derive.derived_trait == "Default" { - // Need to drop Default from enum as this cannot be derived. - // Enum defaults need to either be manually derived - // or we can insert enum defaults - continue; + if derive.derived_trait == "Default" { + if !s.can_derive_default(containers) { + continue; + } } if derive.is_applicable_to(s) && !derives.contains(&derive.derived_trait.as_str()) { diff --git a/src/output.rs b/src/output.rs index fb25e07..daabbe0 100644 --- a/src/output.rs +++ b/src/output.rs @@ -80,6 +80,36 @@ impl Container { pub fn contains_conditions(&self) -> bool { self.members.iter().any(|m| m.type_.contains("Vec")) } + + pub fn can_derive_default(&self, containers: &[Container]) -> bool { + if self.is_enum { + // Need to drop Default from enum as this cannot be derived. + // Enum defaults need to either be manually derived + // or we can insert enum defaults + return false; + } + + for m in &self.members { + if !m.type_.contains('<') + && !m.type_.contains("::") + && m.type_ != "String" + && m.type_ != "IntOrString" + && m.type_ != "NaiveDate" + && m.type_ != "DateTime" + && m.type_.chars().next().unwrap_or_default().is_uppercase() + { + if containers + .iter() + .find(|c| c.name == m.type_) + .is_some_and(|c| !c.can_derive_default(containers)) + { + return false; + } + } + } + + true + } } impl Container { From 936dd56ab36fda31921c7f49756ae3b0c3a61363 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Fri, 17 May 2024 22:58:13 +0200 Subject: [PATCH 2/8] Add a test for can_derive_default, improve comments Signed-off-by: Aaron Dewes --- src/output.rs | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/src/output.rs b/src/output.rs index daabbe0..0a05897 100644 --- a/src/output.rs +++ b/src/output.rs @@ -81,6 +81,7 @@ impl Container { self.members.iter().any(|m| m.type_.contains("Vec")) } + /// Checks if default is implemented for all props, and if not, returns false pub fn can_derive_default(&self, containers: &[Container]) -> bool { if self.is_enum { // Need to drop Default from enum as this cannot be derived. @@ -90,12 +91,13 @@ impl Container { } for m in &self.members { + // If the type contains a <, it's a container type. All container types kopium uses right now (Map, Vec, Option) have a default implementation. if !m.type_.contains('<') - && !m.type_.contains("::") && m.type_ != "String" && m.type_ != "IntOrString" && m.type_ != "NaiveDate" && m.type_ != "DateTime" + // If the first character is lowercase, assume it's a built-in type and skip the check. && m.type_.chars().next().unwrap_or_default().is_uppercase() { if containers @@ -311,4 +313,68 @@ mod test { assert_eq!(&cs.members[2].name, "jwks_uri_x_x"); assert_eq!(&cs.members[3].name, "jwks_uri_x_x_x"); } + + #[test] + fn can_derive_default() { + let containers = vec![ + Container { + name: "Simple".to_string(), + level: 1, + members: vec![], + docs: None, + is_enum: false, + }, + Container { + name: "Enum".to_string(), + level: 1, + members: vec![], + docs: None, + is_enum: true, + }, + Container { + name: "Nested".to_string(), + level: 1, + members: vec![Member { + name: "simple".to_string(), + type_: "Simple".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + }, + Container { + name: "ReferencesEnum".to_string(), + level: 1, + members: vec![Member { + name: "enum".to_string(), + type_: "Enum".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + }, + Container { + name: "ReferencesEnumNested".to_string(), + level: 1, + members: vec![Member { + name: "references_enum".to_string(), + type_: "ReferencesEnum".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + }, + ]; + assert!(containers[0].can_derive_default(&containers)); // Simple + assert!(!containers[1].can_derive_default(&containers)); // Enum + assert!(containers[2].can_derive_default(&containers)); // Nested + assert!(!containers[3].can_derive_default(&containers)); // ReferencesEnum + assert!(!containers[4].can_derive_default(&containers)); // ReferencesEnumNested + } } From b11337efd2458466c13a34ef501fe248abaca9f0 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sat, 18 May 2024 10:27:28 +0200 Subject: [PATCH 3/8] Cache result When searching recursively from the top, we can cache the result. If we have a deeply nested struct A -> B -> C -> D -> Enum and start by checking A, the other structs B, C and D can cache the result of the recursive search. This also adds more tests. Signed-off-by: Aaron Dewes --- src/analyzer.rs | 7 +++++- src/output.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/analyzer.rs b/src/analyzer.rs index 9b1e11c..934be03 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -5,7 +5,10 @@ use heck::ToUpperCamelCase; use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::{ JSONSchemaProps, JSONSchemaPropsOrArray, JSONSchemaPropsOrBool, JSON, }; -use std::collections::{BTreeMap, HashMap}; +use std::{ + cell::OnceCell, + collections::{BTreeMap, HashMap}, +}; const IGNORED_KEYS: [&str; 3] = ["metadata", "apiVersion", "kind"]; @@ -225,6 +228,7 @@ fn analyze_enum_properties( level, docs: schema.description.clone(), is_enum: true, + supports_derive_default: OnceCell::new(), }) } @@ -333,6 +337,7 @@ fn extract_container( level, docs: schema.description.clone(), is_enum: false, + supports_derive_default: OnceCell::new(), }) } diff --git a/src/output.rs b/src/output.rs index 0a05897..010b9cd 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,3 +1,5 @@ +use std::cell::OnceCell; + use heck::{ToPascalCase, ToSnakeCase}; /// All found containers @@ -16,6 +18,7 @@ pub struct Container { pub docs: Option, /// Whether this container is an enum pub is_enum: bool, + pub supports_derive_default: OnceCell, } /// Output member belonging to an Container @@ -90,6 +93,10 @@ impl Container { return false; } + if let Some(can_derive) = self.supports_derive_default.get() { + return *can_derive; + } + for m in &self.members { // If the type contains a <, it's a container type. All container types kopium uses right now (Map, Vec, Option) have a default implementation. if !m.type_.contains('<') @@ -105,11 +112,14 @@ impl Container { .find(|c| c.name == m.type_) .is_some_and(|c| !c.can_derive_default(containers)) { + self.supports_derive_default.set(false).unwrap(); return false; } } } + // We can't fail here, because if it's set, we always return, and set only fails it the Cell is already set + self.supports_derive_default.set(true).unwrap(); true } } @@ -243,6 +253,8 @@ impl MapType { // unit tests #[cfg(test)] mod test { + use std::cell::OnceCell; + use super::{Container, Member}; fn name_only_enum_member(name: &str) -> Member { Member { @@ -281,6 +293,7 @@ mod test { ], docs: None, is_enum: true, + supports_derive_default: OnceCell::new(), }; c.rename(); @@ -306,6 +319,7 @@ mod test { ], docs: None, is_enum: false, + supports_derive_default: OnceCell::new(), }; cs.rename(); assert_eq!(&cs.members[0].name, "jwks_uri"); @@ -323,6 +337,7 @@ mod test { members: vec![], docs: None, is_enum: false, + supports_derive_default: OnceCell::new(), }, Container { name: "Enum".to_string(), @@ -330,6 +345,7 @@ mod test { members: vec![], docs: None, is_enum: true, + supports_derive_default: OnceCell::new(), }, Container { name: "Nested".to_string(), @@ -343,6 +359,7 @@ mod test { }], docs: None, is_enum: false, + supports_derive_default: OnceCell::new(), }, Container { name: "ReferencesEnum".to_string(), @@ -356,6 +373,7 @@ mod test { }], docs: None, is_enum: false, + supports_derive_default: OnceCell::new(), }, Container { name: "ReferencesEnumNested".to_string(), @@ -369,6 +387,49 @@ mod test { }], docs: None, is_enum: false, + supports_derive_default: OnceCell::new(), + }, + Container { + name: "ReferencesEnumOption".to_string(), + level: 1, + members: vec![Member { + name: "maybe_enum".to_string(), + type_: "Option".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + supports_derive_default: OnceCell::new(), + }, + Container { + name: "ReferencesEnumVec".to_string(), + level: 1, + members: vec![Member { + name: "enum_list".to_string(), + type_: "Vec".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + supports_derive_default: OnceCell::new(), + }, + Container { + name: "ReferencesEnumNestedOption".to_string(), + level: 1, + members: vec![Member { + name: "maybe_references_enum".to_string(), + type_: "Option".to_string(), + serde_annot: vec![], + extra_annot: vec![], + docs: None, + }], + docs: None, + is_enum: false, + supports_derive_default: OnceCell::new(), }, ]; assert!(containers[0].can_derive_default(&containers)); // Simple @@ -376,5 +437,8 @@ mod test { assert!(containers[2].can_derive_default(&containers)); // Nested assert!(!containers[3].can_derive_default(&containers)); // ReferencesEnum assert!(!containers[4].can_derive_default(&containers)); // ReferencesEnumNested + assert!(containers[5].can_derive_default(&containers)); // ReferencesEnumOption + assert!(containers[6].can_derive_default(&containers)); // ReferencesEnumVec + assert!(containers[7].can_derive_default(&containers)); // ReferencesEnumNestedOption } } From 50540a706037e383dbbfd29f7616598afa914192 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sat, 18 May 2024 19:58:36 +0200 Subject: [PATCH 4/8] Don't emit #[kube(derive="Default")] in case Default can not be derived for the top level spec This is rare, but for example happens with cert-manager's "Challenge" resource. Signed-off-by: Aaron Dewes --- src/main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main.rs b/src/main.rs index 94ddf17..1c7e4e9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -268,6 +268,9 @@ impl Kopium { if derive.derived_trait == "JsonSchema" { continue; } + if derive.derived_trait == "Default" && !s.can_derive_default(&structs) { + continue; + } println!(r#"#[kube(derive="{}")]"#, derive.derived_trait); } } From 251547f8a3b81da981395a4f03c32e6de87118e1 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sat, 18 May 2024 20:03:26 +0200 Subject: [PATCH 5/8] Add force-derive-default This option enforces the old behaviour around derive Signed-off-by: Aaron Dewes --- src/main.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1c7e4e9..226e0e8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -121,6 +121,12 @@ struct Kopium { /// Type used to represent maps via additionalProperties #[arg(long, value_enum, default_value_t)] map_type: MapType, + + /// Always add #[derive(Default)] to structs, even if it can't be derived without a manual impl for some fields + /// + /// This option only has an effect if `--derive Default` is set. + #[arg(long)] + force_derive_default: bool, } #[derive(Clone, Copy, Debug, Subcommand)] @@ -268,7 +274,7 @@ impl Kopium { if derive.derived_trait == "JsonSchema" { continue; } - if derive.derived_trait == "Default" && !s.can_derive_default(&structs) { + if derive.derived_trait == "Default" && !self.force_derive_default && !s.can_derive_default(&structs) { continue; } println!(r#"#[kube(derive="{}")]"#, derive.derived_trait); @@ -356,7 +362,7 @@ impl Kopium { for derive in &self.derive { if derive.derived_trait == "Default" { - if !s.can_derive_default(containers) { + if !s.can_derive_default(containers) || (self.force_derive_default && !s.is_enum) { continue; } } From 6e2ec14eb1babc76bb8ff6571e33767f5d9e7376 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sun, 19 May 2024 07:15:58 +0200 Subject: [PATCH 6/8] Format code Signed-off-by: Aaron Dewes --- src/main.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main.rs b/src/main.rs index 226e0e8..26a90ef 100644 --- a/src/main.rs +++ b/src/main.rs @@ -121,9 +121,9 @@ struct Kopium { /// Type used to represent maps via additionalProperties #[arg(long, value_enum, default_value_t)] map_type: MapType, - + /// Always add #[derive(Default)] to structs, even if it can't be derived without a manual impl for some fields - /// + /// /// This option only has an effect if `--derive Default` is set. #[arg(long)] force_derive_default: bool, @@ -274,7 +274,10 @@ impl Kopium { if derive.derived_trait == "JsonSchema" { continue; } - if derive.derived_trait == "Default" && !self.force_derive_default && !s.can_derive_default(&structs) { + if derive.derived_trait == "Default" + && !self.force_derive_default + && !s.can_derive_default(&structs) + { continue; } println!(r#"#[kube(derive="{}")]"#, derive.derived_trait); From c10bb919aac832e8ebd0f307ba88aaa66bd01232 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sun, 19 May 2024 17:17:30 +0200 Subject: [PATCH 7/8] Make new behaviour opt-in instead of opt-out Signed-off-by: Aaron Dewes --- src/main.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index 26a90ef..2a399ad 100644 --- a/src/main.rs +++ b/src/main.rs @@ -122,11 +122,11 @@ struct Kopium { #[arg(long, value_enum, default_value_t)] map_type: MapType, - /// Always add #[derive(Default)] to structs, even if it can't be derived without a manual impl for some fields + /// Automatically removes #[derive(Default)] from structs that contain fields for which a default can not be automatically derived. /// /// This option only has an effect if `--derive Default` is set. #[arg(long)] - force_derive_default: bool, + smart_derive_elision: bool, } #[derive(Clone, Copy, Debug, Subcommand)] @@ -275,7 +275,7 @@ impl Kopium { continue; } if derive.derived_trait == "Default" - && !self.force_derive_default + && self.smart_derive_elision && !s.can_derive_default(&structs) { continue; @@ -365,7 +365,7 @@ impl Kopium { for derive in &self.derive { if derive.derived_trait == "Default" { - if !s.can_derive_default(containers) || (self.force_derive_default && !s.is_enum) { + if (self.smart_derive_elision && !s.can_derive_default(containers)) || !s.is_enum { continue; } } From 767bbdde4a80f43e82babb48d32abbd1796bbf40 Mon Sep 17 00:00:00 2001 From: Aaron Dewes Date: Sun, 19 May 2024 17:22:37 +0200 Subject: [PATCH 8/8] Use default() for filling structs Signed-off-by: Aaron Dewes --- src/analyzer.rs | 9 +++---- src/output.rs | 66 ++++++++++++------------------------------------- 2 files changed, 19 insertions(+), 56 deletions(-) diff --git a/src/analyzer.rs b/src/analyzer.rs index 934be03..e24cbbd 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -5,10 +5,7 @@ use heck::ToUpperCamelCase; use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::{ JSONSchemaProps, JSONSchemaPropsOrArray, JSONSchemaPropsOrBool, JSON, }; -use std::{ - cell::OnceCell, - collections::{BTreeMap, HashMap}, -}; +use std::collections::{BTreeMap, HashMap}; const IGNORED_KEYS: [&str; 3] = ["metadata", "apiVersion", "kind"]; @@ -228,7 +225,7 @@ fn analyze_enum_properties( level, docs: schema.description.clone(), is_enum: true, - supports_derive_default: OnceCell::new(), + ..Container::default() }) } @@ -337,7 +334,7 @@ fn extract_container( level, docs: schema.description.clone(), is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }) } diff --git a/src/output.rs b/src/output.rs index 010b9cd..ef10eaf 100644 --- a/src/output.rs +++ b/src/output.rs @@ -253,8 +253,6 @@ impl MapType { // unit tests #[cfg(test)] mod test { - use std::cell::OnceCell; - use super::{Container, Member}; fn name_only_enum_member(name: &str) -> Member { Member { @@ -291,9 +289,8 @@ mod test { name_only_enum_member("jwksUri"), name_only_enum_member("JwksUri"), ], - docs: None, is_enum: true, - supports_derive_default: OnceCell::new(), + ..Container::default() }; c.rename(); @@ -317,9 +314,7 @@ mod test { name_only_int_member("jwksUri"), name_only_int_member("JwksUri"), ], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }; cs.rename(); assert_eq!(&cs.members[0].name, "jwks_uri"); @@ -334,18 +329,13 @@ mod test { Container { name: "Simple".to_string(), level: 1, - members: vec![], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "Enum".to_string(), level: 1, - members: vec![], - docs: None, is_enum: true, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "Nested".to_string(), @@ -353,13 +343,9 @@ mod test { members: vec![Member { name: "simple".to_string(), type_: "Simple".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "ReferencesEnum".to_string(), @@ -367,13 +353,9 @@ mod test { members: vec![Member { name: "enum".to_string(), type_: "Enum".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "ReferencesEnumNested".to_string(), @@ -381,13 +363,9 @@ mod test { members: vec![Member { name: "references_enum".to_string(), type_: "ReferencesEnum".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "ReferencesEnumOption".to_string(), @@ -395,13 +373,9 @@ mod test { members: vec![Member { name: "maybe_enum".to_string(), type_: "Option".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "ReferencesEnumVec".to_string(), @@ -409,13 +383,9 @@ mod test { members: vec![Member { name: "enum_list".to_string(), type_: "Vec".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, Container { name: "ReferencesEnumNestedOption".to_string(), @@ -423,13 +393,9 @@ mod test { members: vec![Member { name: "maybe_references_enum".to_string(), type_: "Option".to_string(), - serde_annot: vec![], - extra_annot: vec![], - docs: None, + ..Member::default() }], - docs: None, - is_enum: false, - supports_derive_default: OnceCell::new(), + ..Container::default() }, ]; assert!(containers[0].can_derive_default(&containers)); // Simple