diff --git a/Cargo.lock b/Cargo.lock index db3144c69..9997a3313 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3952,7 +3952,6 @@ dependencies = [ "schemars", "serde", "serde_json", - "serde_yaml", "snafu 0.9.2", "stackable-versioned-macros", ] diff --git a/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs b/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs index 92bb09c52..2f0657801 100644 --- a/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs +++ b/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs @@ -170,8 +170,11 @@ impl Struct { // Ideally we would integrate with the serde(rename) functionality to produce these field // names. let inserts = self.generate_tracking_inserts(direction, next_version, mod_gen_ctx); + let post_inserts = + self.generate_tracking_post_inserts(direction, next_version, mod_gen_ctx); let removals = self.generate_tracking_removals(direction, next_version, mod_gen_ctx); let json_paths = self.generate_json_paths(next_version, mod_gen_ctx); + let tracked_values = self.generate_tracked_values(direction, next_version, mod_gen_ctx); // TODO (@Techassi): Re-add support for generics // TODO (@Techassi): We know the status, so we can hard-code it, but hard to track across structs @@ -192,10 +195,18 @@ impl Struct { // the upgrade or downgrade section. Only then we can convert the spec. #inserts + // Fields which changed their type are consumed by the conversion, so their + // values need to be serialized before converting the spec. + #tracked_values + let mut spec = Self { #fields }; + // Fields which changed their type can only be inserted into the status after + // the spec is converted, because the downgraded value is tracked as well. + #post_inserts + // After the spec is converted, depending on the direction, we need to apply // changed values from either the upgrade or downgrade section. Afterwards // we can return the successfully converted spec and the status contains @@ -237,6 +248,12 @@ impl Struct { }) .collect(); + // This is the case if only fields which changed their type need to be tracked. + // These are inserted after the conversion, see generate_tracking_post_inserts. + if inserts.is_empty() { + return None; + } + Some(quote! { let upgrades = status .changes() @@ -250,6 +267,42 @@ impl Struct { } } + fn generate_tracking_post_inserts( + &self, + direction: Direction, + next_version: &VersionDefinition, + mod_gen_ctx: ModuleGenerationContext<'_>, + ) -> Option { + match direction { + Direction::Upgrade => None, + Direction::Downgrade => { + let next_version_string = next_version.inner.to_string(); + + let post_inserts: TokenStream = self + .fields + .iter() + .filter_map(|f| { + f.generate_for_status_post_insertion(direction, next_version, mod_gen_ctx) + }) + .collect(); + + if post_inserts.is_empty() { + return None; + } + + Some(quote! { + let upgrades = status + .changes() + .upgrades + .entry(#next_version_string.to_owned()) + .or_default(); + + #post_inserts + }) + } + } + } + fn generate_tracking_removals( &self, direction: Direction, @@ -263,7 +316,7 @@ impl Struct { let match_arms: TokenStream = self .fields .iter() - .filter_map(|f| f.generate_for_status_removal(direction, next_version)) + .filter_map(|f| f.generate_for_status_removal(direction, next_version, mod_gen_ctx)) .collect(); match direction { @@ -271,10 +324,24 @@ impl Struct { let next_version_string = next_version.inner.to_string(); let versioned_path = &*mod_gen_ctx.crates.versioned; + // The downgraded value is only needed by fields which changed their type. Binding + // it unconditionally would result in an unused variable otherwise. + let has_type_changes = self.fields.iter().any(|f| { + f.changes.as_ref().is_some_and(|c| { + c.value_is(&next_version.inner, ItemStatus::is_type_change) + }) + }); + + let downgraded_value = if has_type_changes { + quote! { downgraded_value } + } else { + quote! { .. } + }; + Some(quote! { // NOTE (@Techassi): This is an awkward thing to do. Can we possibly use &str for the keys here? if let Some(upgrades) = status.changes().upgrades.remove(&#next_version_string.to_owned()) { - for #versioned_path::ChangedValue { json_path, value } in upgrades { + for #versioned_path::ChangedValue { json_path, value, #downgraded_value } in upgrades { match json_path { #match_arms _ => unreachable!(), @@ -299,17 +366,37 @@ impl Struct { .collect() } + fn generate_tracked_values( + &self, + direction: Direction, + next_version: &VersionDefinition, + mod_gen_ctx: ModuleGenerationContext<'_>, + ) -> TokenStream { + let from_struct_ident = &self.common.idents.parameter; + + self.fields + .iter() + .filter_map(|f| { + f.generate_for_tracked_value( + direction, + next_version, + from_struct_ident, + mod_gen_ctx, + ) + }) + .collect() + } + pub(super) fn needs_tracking(&self, version: &VersionDefinition) -> bool { self.fields.iter().any(|f| { f.changes.as_ref().is_some_and(|c| { c.value_is(&version.inner, |s| { - // For now, only added fields need to be tracked. In the future, removals and - // type changes also need to be tracked + // For now, only added fields and fields which changed their type need to be + // tracked. In the future, removals also need to be tracked. match s { ItemStatus::Addition { .. } => true, - // TODO (@Techassi): Support tracking for changed fields - ItemStatus::Change { .. } - | ItemStatus::Deprecation { .. } + ItemStatus::Change { .. } => s.is_type_change(), + ItemStatus::Deprecation { .. } | ItemStatus::NoChange { .. } | ItemStatus::NotPresent => false, } diff --git a/crates/stackable-versioned-macros/src/codegen/item/field.rs b/crates/stackable-versioned-macros/src/codegen/item/field.rs index ecc81155e..dc49fb873 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/field.rs @@ -306,7 +306,7 @@ impl VersionedField { Direction::Downgrade => { let next_change = changes.get_expect(&next_version.inner); - let serde_yaml_path = &*mod_gen_ctx.crates.serde_yaml; + let serde_json_path = &*mod_gen_ctx.crates.serde_json; let versioned_path = &*mod_gen_ctx.crates.versioned; match next_change { @@ -318,7 +318,90 @@ impl VersionedField { Some(quote! { upgrades.push(#versioned_path::ChangedValue { json_path: #json_path_ident, - value: #serde_yaml_path::to_value(&#from_struct_ident.#ident).unwrap(), + value: #serde_json_path::to_value(&#from_struct_ident.#ident).unwrap(), + downgraded_value: ::core::option::Option::None, + }); + }) + } + _ => None, + } + } + } + } + + /// Generates code which serializes the value of this field before it is converted. This is only + /// needed for fields which changed their type, because the conversion consumes the value. + /// + /// - When downgrading, this is the value of the newer type, which is tracked in the status. + /// - When upgrading, this is the value of the older type, which is compared against the tracked + /// downgraded value to detect if a user changed the field in the older version. + pub fn generate_for_tracked_value( + &self, + direction: Direction, + next_version: &VersionDefinition, + from_struct_ident: &IdentString, + mod_gen_ctx: ModuleGenerationContext<'_>, + ) -> Option { + let changes = self.changes.as_ref()?; + let next_change = changes.get_expect(&next_version.inner); + + match next_change { + ItemStatus::Change { + from_ident, + to_ident, + .. + } if next_change.is_type_change() => { + let serde_json_path = &*mod_gen_ctx.crates.serde_json; + let value_ident = to_ident.tracked_value_ident(); + + let field_ident = match direction { + Direction::Upgrade => from_ident, + Direction::Downgrade => to_ident, + }; + + Some(quote! { + let #value_ident = #serde_json_path::to_value(&#from_struct_ident.#field_ident).unwrap(); + }) + } + _ => None, + } + } + + /// Generates code needed when a tracked type change of this field needs to be inserted into the + /// status. In contrast to added fields, this can only be done after the conversion, because the + /// downgraded value is tracked as well. + pub fn generate_for_status_post_insertion( + &self, + direction: Direction, + next_version: &VersionDefinition, + mod_gen_ctx: ModuleGenerationContext<'_>, + ) -> Option { + let changes = self.changes.as_ref()?; + + match direction { + Direction::Upgrade => None, + Direction::Downgrade => { + let next_change = changes.get_expect(&next_version.inner); + + let serde_json_path = &*mod_gen_ctx.crates.serde_json; + let versioned_path = &*mod_gen_ctx.crates.versioned; + + match next_change { + ItemStatus::Change { + from_ident, + to_ident, + .. + } if next_change.is_type_change() => { + let json_path_ident = to_ident.json_path_ident(); + let value_ident = to_ident.tracked_value_ident(); + + Some(quote! { + upgrades.push(#versioned_path::ChangedValue { + json_path: #json_path_ident, + value: #value_ident, + downgraded_value: ::core::option::Option::Some( + #serde_json_path::to_value(&spec.#from_ident).unwrap() + ), }); }) } @@ -334,6 +417,7 @@ impl VersionedField { &self, direction: Direction, next_version: &VersionDefinition, + mod_gen_ctx: ModuleGenerationContext<'_>, ) -> Option { // If there are no changes for this field, there is also no need to generate a match arm // for applying a tracked value. @@ -342,16 +426,33 @@ impl VersionedField { match direction { Direction::Upgrade => { let next_change = changes.get_expect(&next_version.inner); + let serde_json_path = &*mod_gen_ctx.crates.serde_json; match next_change { - // NOTE (@Techassi): We currently only support tracking added fields. As such - // we only need to generate code if the next change is "Addition". ItemStatus::Addition { ident, .. } => { let json_path_ident = ident.json_path_ident(); Some(quote! { json_path if json_path == #json_path_ident => { - spec.#ident = serde_yaml::from_value(value).unwrap(); + spec.#ident = #serde_json_path::from_value(value).unwrap(); + }, + }) + } + // The tracked value is only applied if the field still contains the value it + // was downgraded to. Otherwise, a user changed the field in the older version + // and that change takes precedence over the tracked value. + // + // A downgraded value of null is serialized as `downgradedValue: null`, which is + // deserialized as None. As such, a missing downgraded value is treated as null. + ItemStatus::Change { to_ident, .. } if next_change.is_type_change() => { + let json_path_ident = to_ident.json_path_ident(); + let value_ident = to_ident.tracked_value_ident(); + + Some(quote! { + json_path if json_path == #json_path_ident => { + if downgraded_value.unwrap_or_default() == #value_ident { + spec.#to_ident = #serde_json_path::from_value(value).unwrap(); + } }, }) } @@ -390,17 +491,18 @@ impl VersionedField { (Some(changes), _) => { let next_change = changes.get_expect(&next_version.inner); - match next_change { - ItemStatus::Addition { ident, .. } => { - let field_ident = ident.json_path_ident(); - let child_string = ident.to_string(); + let ident = match next_change { + ItemStatus::Addition { ident, .. } => ident, + ItemStatus::Change { to_ident, .. } if next_change.is_type_change() => to_ident, + _ => return None, + }; - Some(quote! { - let #field_ident = #versioned_path::jthong_path(parent, #child_string); - }) - } - _ => None, - } + let field_ident = ident.json_path_ident(); + let child_string = ident.to_string(); + + Some(quote! { + let #field_ident = #versioned_path::jthong_path(parent, #child_string); + }) } } } diff --git a/crates/stackable-versioned-macros/src/codegen/item/mod.rs b/crates/stackable-versioned-macros/src/codegen/item/mod.rs index f26b17747..f3093dd9d 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/mod.rs @@ -81,4 +81,11 @@ impl ItemStatus { Self::NotPresent => unreachable!("ItemStatus::NotPresent does not have an ident"), } } + + /// Returns `true` if this status is a change which modified the type of the item. + /// + /// Changes which only rename the item are lossless and as such don't need to be tracked. + pub fn is_type_change(&self) -> bool { + matches!(self, Self::Change { from_type, to_type, .. } if from_type != to_type) + } } diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs index e688ea5d8..1757ac416 100644 --- a/crates/stackable-versioned-macros/src/lib.rs +++ b/crates/stackable-versioned-macros/src/lib.rs @@ -881,8 +881,8 @@ mod utils; /// ///
/// -/// Currently, only tracking of **added** fields is supported. This will be -/// expanded to removed fields, field type changes, and fields containing +/// Currently, only tracking of **added** fields and **field type changes** is +/// supported. This will be expanded to removed fields and fields containing /// collections in the future. /// ///
@@ -928,6 +928,29 @@ mod utils; /// The final upgrade to `v1` will apply the tracked value for the field `baz`. /// Again, it is removed from the status afterwards. /// +/// #### Tracking Type Changes +/// +/// Fields which changed their type via `changed(from_type = "...")` are tracked +/// as well, because converting to the older type might lose data. Renaming a +/// field without changing its type is lossless and is not tracked. In addition +/// to the original value, the value the field was downgraded to is tracked: +/// +/// ```yaml +/// status: +/// changedValues: +/// upgrades: +/// v1: +/// - jsonPath: "$.gender" +/// value: "It's complicated" +/// downgradedValue: "Unknown" +/// ``` +/// +/// Unlike added fields, the field also exists in the older version and can be +/// changed by users there. When upgrading again, the tracked value is only +/// applied if the field still contains the downgraded value. Otherwise, the +/// change made by the user in the older version takes precedence and the +/// tracked value is discarded. +/// /// ### Tracking Nested Changes /// /// To be able to automatically track values of changed fields in nested sub diff --git a/crates/stackable-versioned-macros/src/utils/mod.rs b/crates/stackable-versioned-macros/src/utils/mod.rs index d69efaa73..87b2b7145 100644 --- a/crates/stackable-versioned-macros/src/utils/mod.rs +++ b/crates/stackable-versioned-macros/src/utils/mod.rs @@ -78,6 +78,7 @@ pub trait ItemIdents { pub trait ItemIdentExt { fn json_path_ident(&self) -> IdentString; + fn tracked_value_ident(&self) -> IdentString; } impl ItemIdentExt for IdentString { @@ -88,6 +89,14 @@ impl ItemIdentExt for IdentString { ) .into() } + + fn tracked_value_ident(&self) -> IdentString { + format_ident!( + "__sv_{lowercase_ident}_value", + lowercase_ident = self.as_str().to_lowercase() + ) + .into() + } } pub fn path_to_string(path: &Path) -> String { diff --git a/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_type_change.rs b/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_type_change.rs new file mode 100644 index 000000000..5338623dc --- /dev/null +++ b/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_type_change.rs @@ -0,0 +1,47 @@ +use kube::CustomResource; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use stackable_versioned::versioned; +// --- +#[versioned( + version(name = "v1alpha1"), + version(name = "v1beta1"), + version(name = "v1"), + options(k8s(experimental_conversion_tracking)) +)] +// --- +pub(crate) mod versioned { + #[versioned(crd(group = "stackable.tech", doc = "Test"))] + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, CustomResource)] + pub(crate) struct FooSpec { + // Two consecutive type changes, the second one combined with a rename. Both are tracked. + #[versioned( + changed(since = "v1beta1", from_type = "u16", downgrade_with = u32_to_u16), + changed( + since = "v1", + from_name = "bah", + from_type = "u32", + downgrade_with = u64_to_u32 + ) + )] + bar: u64, + + // A rename without a type change is lossless and as such is not tracked. + #[versioned(changed(since = "v1", from_name = "qux"))] + baz: bool, + + // An added field in the same version as a type change. + #[versioned(added(since = "v1"))] + quux: String, + } +} +// --- +fn main() {} + +fn u32_to_u16(input: u32) -> u16 { + input.try_into().unwrap_or(u16::MAX) +} + +fn u64_to_u32(input: u64) -> u32 { + input.try_into().unwrap_or(u32::MAX) +} diff --git a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking.rs.snap b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking.rs.snap index 57f5cf4ae..68cee80a4 100644 --- a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking.rs.snap +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking.rs.snap @@ -65,10 +65,10 @@ where baz: __sv_foospec.baz.into(), }; if let Some(upgrades) = status.changes().upgrades.remove(&"v1beta1".to_owned()) { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + for ::stackable_versioned::ChangedValue { json_path, value, .. } in upgrades { match json_path { json_path if json_path == __sv_bah_path => { - spec.bah = serde_yaml::from_value(value).unwrap(); + spec.bah = ::serde_json::from_value(value).unwrap(); } _ => unreachable!(), } @@ -97,7 +97,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_bah_path, - value: ::serde_yaml::to_value(&__sv_foospec.bah).unwrap(), + value: ::serde_json::to_value(&__sv_foospec.bah).unwrap(), + downgraded_value: ::core::option::Option::None, }); let mut spec = Self { baz: __sv_foospec.baz.into(), diff --git a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap index 40f7abe7f..8eb2758f0 100644 --- a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap @@ -231,10 +231,10 @@ where }; if let Some(upgrades) = status.changes().upgrades.remove(&"v1alpha2".to_owned()) { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + for ::stackable_versioned::ChangedValue { json_path, value, .. } in upgrades { match json_path { json_path if json_path == __sv_rest_catalog_uri_path => { - spec.rest_catalog_uri = serde_yaml::from_value(value).unwrap(); + spec.rest_catalog_uri = ::serde_json::from_value(value).unwrap(); } _ => unreachable!(), } @@ -267,8 +267,9 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_rest_catalog_uri_path, - value: ::serde_yaml::to_value(&__sv_icebergconnector.rest_catalog_uri) + value: ::serde_json::to_value(&__sv_icebergconnector.rest_catalog_uri) .unwrap(), + downgraded_value: ::core::option::Option::None, }); let mut spec = Self { metastore: __sv_icebergconnector.metastore.into(), diff --git a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_hints.rs.snap b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_hints.rs.snap index 96a70d907..4bf3ec1e1 100644 --- a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_hints.rs.snap +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_hints.rs.snap @@ -61,10 +61,10 @@ where }; if let Some(upgrades) = status.changes().upgrades.remove(&"v1alpha2".to_owned()) { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + for ::stackable_versioned::ChangedValue { json_path, value, .. } in upgrades { match json_path { json_path if json_path == __sv_baz_baz_path => { - spec.baz_baz = serde_yaml::from_value(value).unwrap(); + spec.baz_baz = ::serde_json::from_value(value).unwrap(); } _ => unreachable!(), } @@ -89,7 +89,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_baz_path, - value: ::serde_yaml::to_value(&__sv_bar.baz_baz).unwrap(), + value: ::serde_json::to_value(&__sv_bar.baz_baz).unwrap(), + downgraded_value: ::core::option::Option::None, }); let mut spec = Self { bar_bar: __sv_bar.bar_bar.into(), diff --git a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap new file mode 100644 index 000000000..e3e76e74e --- /dev/null +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap @@ -0,0 +1,572 @@ +--- +source: crates/stackable-versioned-macros/src/lib.rs +expression: formatted +input_file: crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_type_change.rs +--- +#[automatically_derived] +pub(crate) mod v1alpha1 { + use super::*; + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, CustomResource)] + #[kube( + group = "stackable.tech", + version = "v1alpha1", + kind = "Foo", + doc = "Test", + status = FooStatusWithChangedValues + )] + pub struct FooSpec { + pub bah: u16, + pub qux: bool, + } +} +#[automatically_derived] +impl ::core::convert::From for v1beta1::Foo { + fn from(__sv_foo: v1alpha1::Foo) -> Self { + let mut status = __sv_foo.status.unwrap_or_default(); + let spec = >::tracking_from(__sv_foo.spec, &mut status, "$"); + Self { + metadata: __sv_foo.metadata, + status: Some(status), + spec, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1alpha1::Foo { + fn from(__sv_foo: v1beta1::Foo) -> Self { + let mut status = __sv_foo.status.unwrap_or_default(); + let spec = >::tracking_from(__sv_foo.spec, &mut status, "$"); + Self { + metadata: __sv_foo.metadata, + status: Some(status), + spec, + } + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom for v1beta1::FooSpec +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_foospec: v1alpha1::FooSpec, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_bah_path = ::stackable_versioned::jthong_path(parent, "bah"); + let __sv_bah_value = ::serde_json::to_value(&__sv_foospec.bah).unwrap(); + let mut spec = Self { + bah: __sv_foospec.bah.into(), + qux: __sv_foospec.qux.into(), + }; + if let Some(upgrades) = status.changes().upgrades.remove(&"v1beta1".to_owned()) { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in upgrades { + match json_path { + json_path if json_path == __sv_bah_path => { + if downgraded_value.unwrap_or_default() == __sv_bah_value { + spec.bah = ::serde_json::from_value(value).unwrap(); + } + } + _ => unreachable!(), + } + } + } + spec + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom for v1alpha1::FooSpec +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_foospec: v1beta1::FooSpec, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_bah_path = ::stackable_versioned::jthong_path(parent, "bah"); + let __sv_bah_value = ::serde_json::to_value(&__sv_foospec.bah).unwrap(); + let mut spec = Self { + bah: u32_to_u16(__sv_foospec.bah), + qux: __sv_foospec.qux.into(), + }; + let upgrades = status + .changes() + .upgrades + .entry("v1beta1".to_owned()) + .or_default(); + upgrades + .push(::stackable_versioned::ChangedValue { + json_path: __sv_bah_path, + value: __sv_bah_value, + downgraded_value: ::core::option::Option::Some( + ::serde_json::to_value(&spec.bah).unwrap(), + ), + }); + spec + } +} +#[automatically_derived] +pub(crate) mod v1beta1 { + use super::*; + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, CustomResource)] + #[kube( + group = "stackable.tech", + version = "v1beta1", + kind = "Foo", + doc = "Test", + status = FooStatusWithChangedValues + )] + pub struct FooSpec { + pub bah: u32, + pub qux: bool, + } +} +#[automatically_derived] +impl ::core::convert::From for v1::Foo { + fn from(__sv_foo: v1beta1::Foo) -> Self { + let mut status = __sv_foo.status.unwrap_or_default(); + let spec = >::tracking_from(__sv_foo.spec, &mut status, "$"); + Self { + metadata: __sv_foo.metadata, + status: Some(status), + spec, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1beta1::Foo { + fn from(__sv_foo: v1::Foo) -> Self { + let mut status = __sv_foo.status.unwrap_or_default(); + let spec = >::tracking_from(__sv_foo.spec, &mut status, "$"); + Self { + metadata: __sv_foo.metadata, + status: Some(status), + spec, + } + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom for v1::FooSpec +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_foospec: v1beta1::FooSpec, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_bar_path = ::stackable_versioned::jthong_path(parent, "bar"); + let __sv_quux_path = ::stackable_versioned::jthong_path(parent, "quux"); + let __sv_bar_value = ::serde_json::to_value(&__sv_foospec.bah).unwrap(); + let mut spec = Self { + bar: __sv_foospec.bah.into(), + baz: __sv_foospec.qux.into(), + quux: ::std::default::Default::default(), + }; + if let Some(upgrades) = status.changes().upgrades.remove(&"v1".to_owned()) { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in upgrades { + match json_path { + json_path if json_path == __sv_bar_path => { + if downgraded_value.unwrap_or_default() == __sv_bar_value { + spec.bar = ::serde_json::from_value(value).unwrap(); + } + } + json_path if json_path == __sv_quux_path => { + spec.quux = ::serde_json::from_value(value).unwrap(); + } + _ => unreachable!(), + } + } + } + spec + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom for v1beta1::FooSpec +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_foospec: v1::FooSpec, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_bar_path = ::stackable_versioned::jthong_path(parent, "bar"); + let __sv_quux_path = ::stackable_versioned::jthong_path(parent, "quux"); + let upgrades = status.changes().upgrades.entry("v1".to_owned()).or_default(); + upgrades + .push(::stackable_versioned::ChangedValue { + json_path: __sv_quux_path, + value: ::serde_json::to_value(&__sv_foospec.quux).unwrap(), + downgraded_value: ::core::option::Option::None, + }); + let __sv_bar_value = ::serde_json::to_value(&__sv_foospec.bar).unwrap(); + let mut spec = Self { + bah: u64_to_u32(__sv_foospec.bar), + qux: __sv_foospec.baz.into(), + }; + let upgrades = status.changes().upgrades.entry("v1".to_owned()).or_default(); + upgrades + .push(::stackable_versioned::ChangedValue { + json_path: __sv_bar_path, + value: __sv_bar_value, + downgraded_value: ::core::option::Option::Some( + ::serde_json::to_value(&spec.bah).unwrap(), + ), + }); + spec + } +} +#[automatically_derived] +pub(crate) mod v1 { + use super::*; + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, CustomResource)] + #[kube( + group = "stackable.tech", + version = "v1", + kind = "Foo", + doc = "Test", + status = FooStatusWithChangedValues + )] + pub struct FooSpec { + pub bar: u64, + pub baz: bool, + pub quux: String, + } +} +#[automatically_derived] +#[derive(::core::fmt::Debug)] +pub(crate) enum Foo { + V1Alpha1(v1alpha1::Foo), + V1Beta1(v1beta1::Foo), + V1(v1::Foo), +} +#[automatically_derived] +impl Foo { + /// Generates a merged CRD containing all versions and marking `stored_apiversion` as stored. + pub fn merged_crd( + stored_apiversion: FooVersion, + ) -> ::std::result::Result< + ::k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceDefinition, + ::kube::core::crd::MergeError, + > { + ::kube::core::crd::merge_crds( + vec![ + < v1alpha1::Foo as ::kube::core::CustomResourceExt > ::crd(), < + v1beta1::Foo as ::kube::core::CustomResourceExt > ::crd(), < v1::Foo as + ::kube::core::CustomResourceExt > ::crd() + ], + stored_apiversion.as_version_str(), + ) + } + ///Tries to convert a list of objects of kind [`Foo`] to the desired API version + ///specified in the [`ConversionReview`][cr]. + /// + ///The returned [`ConversionReview`][cr] either indicates a success or a failure, which + ///is handed back to the Kubernetes API server. + /// + ///[cr]: ::kube::core::conversion::ConversionReview + pub fn try_convert( + review: ::kube::core::conversion::ConversionReview, + ) -> ::kube::core::conversion::ConversionReview { + let request = match ::kube::core::conversion::ConversionRequest::from_review( + review, + ) { + ::std::result::Result::Ok(request) => request, + ::std::result::Result::Err(err) => { + return ::kube::core::conversion::ConversionResponse::invalid(::kube::core::Status { + status: Some(::kube::core::response::StatusSummary::Failure), + message: err.to_string(), + metadata: None, + reason: err.to_string(), + details: None, + code: 400, + }) + .into_review(); + } + }; + let response = match Self::convert_objects( + request.objects, + &request.desired_api_version, + ) { + ::std::result::Result::Ok(converted_objects) => { + ::kube::core::conversion::ConversionResponse { + result: ::kube::core::Status::success(), + types: request.types, + uid: request.uid, + converted_objects, + } + } + ::std::result::Result::Err(err) => { + let code = err.http_status_code(); + let message = err.join_errors(); + ::kube::core::conversion::ConversionResponse { + result: ::kube::core::Status { + status: Some(::kube::core::response::StatusSummary::Failure), + message: message.clone(), + metadata: None, + reason: message, + details: None, + code, + }, + types: request.types, + uid: request.uid, + converted_objects: vec![], + } + } + }; + response.into_review() + } + fn convert_objects( + objects: ::std::vec::Vec<::serde_json::Value>, + desired_api_version: &str, + ) -> ::std::result::Result< + ::std::vec::Vec<::serde_json::Value>, + ::stackable_versioned::ConvertObjectError, + > { + let desired_api_version = FooVersion::from_api_version(desired_api_version) + .map_err(|source| ::stackable_versioned::ConvertObjectError::ParseDesiredApiVersion { + source, + })?; + let mut converted_objects = ::std::vec::Vec::with_capacity(objects.len()); + for object in objects { + let current_object = Self::from_json_object(object.clone()) + .map_err(|source| ::stackable_versioned::ConvertObjectError::Parse { + source, + })?; + match (current_object, desired_api_version) { + (Self::V1Alpha1(__sv_foo), FooVersion::V1Beta1) => { + let converted: v1beta1::Foo = __sv_foo.into(); + let desired_object = Self::V1Beta1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1Alpha1(__sv_foo), FooVersion::V1) => { + let converted: v1beta1::Foo = __sv_foo.into(); + let converted: v1::Foo = converted.into(); + let desired_object = Self::V1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1Beta1(__sv_foo), FooVersion::V1Alpha1) => { + let converted: v1alpha1::Foo = __sv_foo.into(); + let desired_object = Self::V1Alpha1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1Beta1(__sv_foo), FooVersion::V1) => { + let converted: v1::Foo = __sv_foo.into(); + let desired_object = Self::V1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1(__sv_foo), FooVersion::V1Alpha1) => { + let converted: v1beta1::Foo = __sv_foo.into(); + let converted: v1alpha1::Foo = converted.into(); + let desired_object = Self::V1Alpha1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1(__sv_foo), FooVersion::V1Beta1) => { + let converted: v1beta1::Foo = __sv_foo.into(); + let desired_object = Self::V1Beta1(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + _ => converted_objects.push(object), + } + } + ::std::result::Result::Ok(converted_objects) + } + fn from_json_object( + object_value: ::serde_json::Value, + ) -> ::std::result::Result { + let kind = object_value + .get("kind") + .ok_or_else(|| ::stackable_versioned::ParseObjectError::FieldNotPresent { + field: "kind".to_owned(), + })? + .as_str() + .ok_or_else(|| ::stackable_versioned::ParseObjectError::FieldNotStr { + field: "kind".to_owned(), + })?; + if kind != "Foo" { + return Err(::stackable_versioned::ParseObjectError::UnexpectedKind { + kind: kind.to_owned(), + expected: "Foo".to_owned(), + }); + } + let api_version = object_value + .get("apiVersion") + .ok_or_else(|| ::stackable_versioned::ParseObjectError::FieldNotPresent { + field: "apiVersion".to_owned(), + })? + .as_str() + .ok_or_else(|| ::stackable_versioned::ParseObjectError::FieldNotStr { + field: "apiVersion".to_owned(), + })?; + let object = match api_version { + "stackable.tech/v1alpha1" => { + let object = ::serde_json::from_value(object_value) + .map_err(|source| ::stackable_versioned::ParseObjectError::Deserialize { + source, + })?; + Self::V1Alpha1(object) + } + "stackable.tech/v1beta1" => { + let object = ::serde_json::from_value(object_value) + .map_err(|source| ::stackable_versioned::ParseObjectError::Deserialize { + source, + })?; + Self::V1Beta1(object) + } + "stackable.tech/v1" => { + let object = ::serde_json::from_value(object_value) + .map_err(|source| ::stackable_versioned::ParseObjectError::Deserialize { + source, + })?; + Self::V1(object) + } + unknown_api_version => { + return ::std::result::Result::Err(::stackable_versioned::ParseObjectError::UnknownApiVersion { + api_version: unknown_api_version.to_owned(), + }); + } + }; + ::std::result::Result::Ok(object) + } + fn into_json_value( + self, + ) -> ::std::result::Result<::serde_json::Value, ::serde_json::Error> { + match self { + Self::V1Alpha1(__sv_foo) => Ok(::serde_json::to_value(__sv_foo)?), + Self::V1Beta1(__sv_foo) => Ok(::serde_json::to_value(__sv_foo)?), + Self::V1(__sv_foo) => Ok(::serde_json::to_value(__sv_foo)?), + } + } +} +#[automatically_derived] +#[derive(::core::marker::Copy, ::core::clone::Clone, ::core::fmt::Debug)] +pub(crate) enum FooVersion { + V1Alpha1, + V1Beta1, + V1, +} +#[automatically_derived] +impl ::core::fmt::Display for FooVersion { + fn fmt( + &self, + f: &mut ::core::fmt::Formatter<'_>, + ) -> ::std::result::Result<(), ::std::fmt::Error> { + f.write_str(self.as_version_str()) + } +} +#[automatically_derived] +impl FooVersion { + pub fn as_version_str(&self) -> &str { + match self { + FooVersion::V1Alpha1 => "v1alpha1", + FooVersion::V1Beta1 => "v1beta1", + FooVersion::V1 => "v1", + } + } + pub fn as_api_version_str(&self) -> &str { + match self { + FooVersion::V1Alpha1 => "stackable.tech/v1alpha1", + FooVersion::V1Beta1 => "stackable.tech/v1beta1", + FooVersion::V1 => "stackable.tech/v1", + } + } + pub fn from_api_version( + api_version: &str, + ) -> Result { + match api_version { + "stackable.tech/v1alpha1" => Ok(FooVersion::V1Alpha1), + "stackable.tech/v1beta1" => Ok(FooVersion::V1Beta1), + "stackable.tech/v1" => Ok(FooVersion::V1), + _ => { + Err(::stackable_versioned::UnknownDesiredApiVersionError { + api_version: api_version.to_owned(), + }) + } + } + } +} +#[cfg(test)] +#[test] +fn FooSpec_roundtrip_down_up() { + ::stackable_versioned::test_utils::test_roundtrip::< + v1::FooSpec, + >(stringify!(Foo), "stackable.tech/v1", "stackable.tech/v1alpha1", Foo::try_convert); +} +#[cfg(test)] +#[test] +fn FooSpec_roundtrip_up_down() { + ::stackable_versioned::test_utils::test_roundtrip::< + v1alpha1::FooSpec, + >(stringify!(Foo), "stackable.tech/v1alpha1", "stackable.tech/v1", Foo::try_convert); +} +#[automatically_derived] +#[derive( + ::core::clone::Clone, + ::core::default::Default, + ::core::fmt::Debug, + ::serde::Deserialize, + ::serde::Serialize, + ::schemars::JsonSchema +)] +#[serde(rename_all = "camelCase")] +pub struct FooStatusWithChangedValues { + pub changed_values: ::stackable_versioned::ChangedValues, +} +#[automatically_derived] +impl ::stackable_versioned::TrackingStatus for FooStatusWithChangedValues { + fn changes(&mut self) -> &mut ::stackable_versioned::ChangedValues { + &mut self.changed_values + } +} diff --git a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@docs.rs.snap b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@docs.rs.snap index 4655f7ddb..b5ab1154b 100644 --- a/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@docs.rs.snap +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@docs.rs.snap @@ -47,10 +47,10 @@ where fred: __sv_foo.waldo.into(), }; if let Some(upgrades) = status.changes().upgrades.remove(&"v1beta1".to_owned()) { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + for ::stackable_versioned::ChangedValue { json_path, value, .. } in upgrades { match json_path { json_path if json_path == __sv_baz_path => { - spec.baz = serde_yaml::from_value(value).unwrap(); + spec.baz = ::serde_json::from_value(value).unwrap(); } _ => unreachable!(), } @@ -76,7 +76,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_path, - value: ::serde_yaml::to_value(&__sv_foo.baz).unwrap(), + value: ::serde_json::to_value(&__sv_foo.baz).unwrap(), + downgraded_value: ::core::option::Option::None, }); let mut spec = Self { foo: __sv_foo.foo.into(), diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 5db878395..89b3ab879 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -12,6 +12,9 @@ All notable changes to this project will be documented in this file. - Support tracking changes through enums when `experimental_conversion_tracking` is enabled. Variants containing versioned structs or enums need to be marked with `#[versioned(nested)]` ([#YYYY]). +- Track fields which changed their type when `experimental_conversion_tracking` is enabled. The + tracked value is only applied on upgrade if the field was not changed in the older version + ([#ZZZZ]). ### Changed @@ -22,11 +25,15 @@ All notable changes to this project will be documented in this file. ### Fixed - Fix `From` impls of enum variants with multiple unnamed fields ([#YYYY]). +- BREAKING: Store tracked values as `serde_json::Value` instead of `serde_yaml::Value`. Tracked + enum values were serialized as YAML tags, which failed to deserialize after a roundtrip through + the JSON status ([#ZZZZ]). [#1284]: https://github.com/stackabletech/operator-rs/pull/1284 [#1285]: https://github.com/stackabletech/operator-rs/pull/1285 [#XXXX]: https://github.com/stackabletech/operator-rs/pull/XXXX [#YYYY]: https://github.com/stackabletech/operator-rs/pull/YYYY +[#ZZZZ]: https://github.com/stackabletech/operator-rs/pull/ZZZZ ## [0.11.1] - 2026-07-06 diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index 069e43abc..792046924 100644 --- a/crates/stackable-versioned/Cargo.toml +++ b/crates/stackable-versioned/Cargo.toml @@ -17,7 +17,6 @@ kube.workspace = true schemars.workspace = true serde.workspace = true serde_json.workspace = true -serde_yaml.workspace = true snafu.workspace = true [dev-dependencies] diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs index 6aa4bddc3..031b098c5 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -113,8 +113,21 @@ pub struct ChangedValue { pub json_path: String, /// The value to be used when upgrading or downgrading the custom resource. + // NOTE: This needs to be a JSON value, because the status is stored as JSON. YAML values + // represent enum variants as tags, which can't be deserialized again after a roundtrip + // through JSON. #[schemars(schema_with = "raw_object_schema")] - pub value: serde_yaml::Value, + pub value: serde_json::Value, + + /// The value the field was converted to during the downgrade. Only set for fields which + /// changed their type. + /// + /// When upgrading again, the tracked `value` is only applied if the field still contains this + /// value. Otherwise, the field was changed by a user in the older version and that change is + /// kept instead. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[schemars(schema_with = "raw_object_schema")] + pub downgraded_value: Option, } // TODO (@Techassi): Think about where this should live. Basically this already exists in diff --git a/crates/stackable-versioned/tests/enum_tracking.rs b/crates/stackable-versioned/tests/enum_tracking.rs index d5846f53a..62504fcb0 100644 --- a/crates/stackable-versioned/tests/enum_tracking.rs +++ b/crates/stackable-versioned/tests/enum_tracking.rs @@ -32,16 +32,46 @@ pub mod versioned { #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] #[serde(rename_all = "camelCase")] pub struct IcebergConnector { - metastore: Option, - - #[versioned(added(since = "v1alpha2"))] - rest_catalog_uri: Option, + #[versioned(changed( + since = "v1alpha2", + from_name = "metastore", + from_type = "Option" + ))] + catalog: IcebergCatalog, } } #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] pub struct TpchConnector {} +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum IcebergCatalog { + Rest { uri: String }, + HiveMetastore { config_map: String }, + UserProvided {}, +} + +// Before v1alpha2, only Hive metastores were supported. Other catalogs had to be configured by +// users manually, so there is no way to represent a REST catalog in v1alpha1. +impl From for Option { + fn from(catalog: IcebergCatalog) -> Self { + match catalog { + IcebergCatalog::HiveMetastore { config_map } => Some(config_map), + IcebergCatalog::Rest { .. } | IcebergCatalog::UserProvided {} => None, + } + } +} + +impl From> for IcebergCatalog { + fn from(metastore: Option) -> Self { + match metastore { + Some(config_map) => Self::HiveMetastore { config_map }, + None => Self::UserProvided {}, + } + } +} + impl stackable_versioned::test_utils::RoundtripTestData for v1alpha1::CatalogSpec { fn roundtrip_test_data() -> Vec { vec![ @@ -50,6 +80,11 @@ impl stackable_versioned::test_utils::RoundtripTestData for v1alpha1::CatalogSpe metastore: Some("hive".to_owned()), }), }, + Self { + connector: v1alpha1::Connector::Iceberg(v1alpha1::IcebergConnector { + metastore: None, + }), + }, Self { connector: v1alpha1::Connector::Tpch(TpchConnector {}), }, @@ -60,18 +95,25 @@ impl stackable_versioned::test_utils::RoundtripTestData for v1alpha1::CatalogSpe impl stackable_versioned::test_utils::RoundtripTestData for v1alpha2::CatalogSpec { fn roundtrip_test_data() -> Vec { vec![ - // The REST catalog URI doesn't exist in v1alpha1. It is tracked in the status and - // restored when upgrading again. + // The REST catalog can not be represented in v1alpha1. It is tracked in the status + // and restored when upgrading again. Self { connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { - metastore: None, - rest_catalog_uri: Some("http://rest-catalog:8181".to_owned()), + catalog: IcebergCatalog::Rest { + uri: "http://rest-catalog:8181".to_owned(), + }, }), }, Self { connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { - metastore: Some("hive".to_owned()), - rest_catalog_uri: None, + catalog: IcebergCatalog::HiveMetastore { + config_map: "hive".to_owned(), + }, + }), + }, + Self { + connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { + catalog: IcebergCatalog::UserProvided {}, }), }, Self { @@ -96,8 +138,11 @@ fn tracks_values_through_enum_variants() { "spec": { "connector": { "iceberg": { - "metastore": null, - "restCatalogUri": "http://rest-catalog:8181" + "catalog": { + "rest": { + "uri": "http://rest-catalog:8181" + } + } } } } @@ -118,14 +163,19 @@ fn tracks_values_through_enum_variants() { .expect("there must be at least one object"); assert_eq!( - object["spec"]["connector"]["iceberg"], - serde_json::json!({ "metastore": null }) + object["spec"]["connector"]["iceberg"]["metastore"], + serde_json::Value::Null ); assert_eq!( object["status"]["changedValues"]["upgrades"]["v1alpha2"], serde_json::json!([{ - "jsonPath": "$.connector.Iceberg.rest_catalog_uri", - "value": "http://rest-catalog:8181" + "jsonPath": "$.connector.Iceberg.catalog", + "value": { + "rest": { + "uri": "http://rest-catalog:8181" + } + }, + "downgradedValue": null }]) ); } diff --git a/crates/stackable-versioned/tests/person.rs b/crates/stackable-versioned/tests/person.rs index 5efa0f298..0deed7258 100644 --- a/crates/stackable-versioned/tests/person.rs +++ b/crates/stackable-versioned/tests/person.rs @@ -66,8 +66,6 @@ pub mod versioned { // We started out with a enum. As we *need* to provide a default, we have a Unknown variant. // Afterwards we figured let's be more flexible and accept any arbitrary String. - - // FIXME: The roundtrips are currently broken, see the `roundtrip_test_data` below. #[versioned(added(since = "v2"), changed(since = "v3", from_type = "Gender"))] gender: String, @@ -128,20 +126,18 @@ impl stackable_versioned::test_utils::RoundtripTestData for v3::PersonSpec { mastodon: "@jdoe@example.com".to_owned(), }, }, - // FIXME: The following test case fails. See the docs on the `versioned` macro, as of - // writing it only supports tracking `added` fields. - // Hence, the firstName, lastName, socials.mastodon work, while gender is broken. - // Self { - // username: "".to_owned(), - // first_name: "".to_owned(), - // last_name: "".to_owned(), - // // FIXME: Currently, the roundtrip results in "Unknown", although it should be "It's complicated" - // gender: "It's complicated".to_owned(), - // socials: v3::Socials { - // email: "".to_owned(), - // mastodon: "".to_owned(), - // }, - // }, + // Downgrading to v2 turns the gender into Gender::Unknown. The original value is + // tracked in the status and restored when upgrading again. + Self { + username: String::new(), + first_name: String::new(), + last_name: String::new(), + gender: "It's complicated".to_owned(), + socials: v3::Socials { + email: String::new(), + mastodon: String::new(), + }, + }, ] } } diff --git a/crates/stackable-versioned/tests/roundtrip.rs b/crates/stackable-versioned/tests/roundtrip.rs index b1703d81b..24e786c6b 100644 --- a/crates/stackable-versioned/tests/roundtrip.rs +++ b/crates/stackable-versioned/tests/roundtrip.rs @@ -1,5 +1,5 @@ use insta::glob; -use kube::core::response::StatusSummary; +use kube::core::{conversion::ConversionReview, response::StatusSummary}; use crate::person::{Person, PersonVersion}; @@ -52,3 +52,64 @@ fn person_v3_v1alpha1_v3() { assert_eq!(original_object, converted_object); }); } + +#[test] +fn person_v3_v2_v3_keeps_changes_made_in_v2() { + // "It's complicated" cannot be represented by the Gender enum used in v2, so it is downgraded + // to "Unknown". The original value is tracked in the status. + let review: ConversionReview = serde_json::from_value(serde_json::json!({ + "kind": "ConversionReview", + "apiVersion": "apiextensions.k8s.io/v1", + "request": { + "uid": "c4e55572-ee1f-4e94-9097-28936985d45f", + "desiredAPIVersion": "test.stackable.tech/v2", + "objects": [{ + "apiVersion": "test.stackable.tech/v3", + "kind": "Person", + "metadata": {}, + "spec": { + "username": "jdoe", + "firstName": "John", + "lastName": "Doe", + "gender": "It's complicated", + "socials": { + "email": "jdoe@example.com", + "mastodon": "@jdoe@example.com" + } + } + }] + } + })) + .expect("conversion review must be valid"); + + let response_v2 = Person::try_convert(review); + let mut roundtrip_review = person::roundtrip_conversion_review(response_v2, PersonVersion::V3); + + // A user changes the gender while working with the v2 object + let object = roundtrip_review + .request + .as_mut() + .expect("roundtrip review must have a request") + .objects + .first_mut() + .expect("there must be at least one object"); + + assert_eq!(object["spec"]["gender"], "Unknown"); + object["spec"]["gender"] = "Female".into(); + + let response_v3 = Person::try_convert(roundtrip_review); + let response = response_v3 + .response + .as_ref() + .expect("v3 review must have a response"); + + assert_eq!(response.result.status, Some(StatusSummary::Success)); + + // The change made in v2 takes precedence over the tracked value + let object = response + .converted_objects + .first() + .expect("there must be at least one object"); + + assert_eq!(object["spec"]["gender"], "Female"); +} diff --git a/crates/stackable-versioned/tests/snapshots/conversions__pass@persons_to_v1alpha1.json.snap b/crates/stackable-versioned/tests/snapshots/conversions__pass@persons_to_v1alpha1.json.snap index e469f60c7..cccb73fe4 100644 --- a/crates/stackable-versioned/tests/snapshots/conversions__pass@persons_to_v1alpha1.json.snap +++ b/crates/stackable-versioned/tests/snapshots/conversions__pass@persons_to_v1alpha1.json.snap @@ -168,6 +168,13 @@ input_file: crates/stackable-versioned/tests/inputs/conversions/pass/persons_to_ "jsonPath": "$.gender", "value": "Male" } + ], + "v3": [ + { + "downgradedValue": "Male", + "jsonPath": "$.gender", + "value": "Male" + } ] } }