diff --git a/Cargo.lock b/Cargo.lock index a662ad0b7..5b620cf35 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3947,7 +3947,6 @@ dependencies = [ "serde_json", "snafu 0.9.2", "stackable-versioned-macros", - "yaml_serde", ] [[package]] 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 9c838ef65..08c15914d 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, @@ -271,15 +324,26 @@ impl Struct { let next_version_string = next_version.inner.to_string(); let versioned_path = &*mod_gen_ctx.crates.versioned; + // Multiple (nested) containers can track values for the same version. As such, only + // the values matching the JSON paths of this container are applied. All other values + // are kept, so that they can be applied by the other containers. The entry for the + // version is only removed once all values have been applied. 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 { + if let Some(upgrades) = status.changes().upgrades.get_mut(#next_version_string) { + for #versioned_path::ChangedValue { json_path, value, downgraded_value } in ::std::mem::take(upgrades) { match json_path { #match_arms - _ => unreachable!(), + json_path => upgrades.push(#versioned_path::ChangedValue { + json_path, + value, + downgraded_value, + }), } } + + if upgrades.is_empty() { + status.changes().upgrades.remove(#next_version_string); + } } }) } @@ -299,17 +363,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 c14fcd92d..ab60b1e1f 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/field.rs @@ -315,7 +315,7 @@ impl VersionedField { Direction::Downgrade => { let next_change = changes.get_expect(&next_version.inner); - let yaml_serde_path = &*mod_gen_ctx.crates.yaml_serde; + let serde_json_path = &*mod_gen_ctx.crates.serde_json; let versioned_path = &*mod_gen_ctx.crates.versioned; match next_change { @@ -327,7 +327,90 @@ impl VersionedField { Some(quote! { upgrades.push(#versioned_path::ChangedValue { json_path: #json_path_ident, - value: #yaml_serde_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() + ), }); }) } @@ -349,21 +432,36 @@ impl VersionedField { // for applying a tracked value. let changes = self.changes.as_ref()?; - let yaml_serde_path = &*mod_gen_ctx.crates.yaml_serde; - 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 = #yaml_serde_path::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(); + } }, }) } @@ -402,17 +500,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 9696b7adb..eea041315 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 5050ae8f1..6c1995af2 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 bfd6a7486..37b1732ff 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 @@ -64,15 +64,29 @@ where bah: ::std::default::Default::default(), 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 { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1beta1") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(upgrades) { match json_path { json_path if json_path == __sv_bah_path => { - spec.bah = ::yaml_serde::from_value(value).unwrap(); + spec.bah = ::serde_json::from_value(value).unwrap(); + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) } - _ => unreachable!(), } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } } spec } @@ -97,7 +111,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_bah_path, - value: ::yaml_serde::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 22bab38fa..04d163e8a 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 @@ -229,16 +229,29 @@ where metastore: __sv_icebergconnector.metastore.into(), rest_catalog_uri: ::std::default::Default::default(), }; - if let Some(upgrades) = status.changes().upgrades.remove(&"v1alpha2".to_owned()) - { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1alpha2") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(upgrades) { match json_path { json_path if json_path == __sv_rest_catalog_uri_path => { - spec.rest_catalog_uri = ::yaml_serde::from_value(value).unwrap(); + spec.rest_catalog_uri = ::serde_json::from_value(value).unwrap(); + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) } - _ => unreachable!(), } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1alpha2"); + } } spec } @@ -267,8 +280,9 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_rest_catalog_uri_path, - value: ::yaml_serde::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 3dbe2f76c..1a9c7b239 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 @@ -59,16 +59,29 @@ where bar_bar: __sv_bar.bar_bar.into(), baz_baz: ::std::default::Default::default(), }; - if let Some(upgrades) = status.changes().upgrades.remove(&"v1alpha2".to_owned()) - { - for ::stackable_versioned::ChangedValue { json_path, value } in upgrades { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1alpha2") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(upgrades) { match json_path { json_path if json_path == __sv_baz_baz_path => { - spec.baz_baz = ::yaml_serde::from_value(value).unwrap(); + spec.baz_baz = ::serde_json::from_value(value).unwrap(); + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) } - _ => unreachable!(), } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1alpha2"); + } } spec } @@ -89,7 +102,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_baz_path, - value: ::yaml_serde::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..636dbc91f --- /dev/null +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap @@ -0,0 +1,592 @@ +--- +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.get_mut("v1beta1") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(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(); + } + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } + } + } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } + } + 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.get_mut("v1") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(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(); + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } + } + } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1"); + } + } + 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 287f78240..d8d9fdb3d 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,15 +47,29 @@ where corge: __sv_foo.corge.into(), 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 { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1beta1") { + for ::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + } in ::std::mem::take(upgrades) { match json_path { json_path if json_path == __sv_baz_path => { - spec.baz = ::yaml_serde::from_value(value).unwrap(); + spec.baz = ::serde_json::from_value(value).unwrap(); + } + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) } - _ => unreachable!(), } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } } spec } @@ -77,7 +91,8 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_path, - value: ::yaml_serde::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 15ca793f8..87c3fa8d3 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -4,6 +4,26 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- 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 + +- BREAKING: Store tracked values as `serde_json::Value` instead of `yaml_serde::Value`. Tracked + enum values were serialized as YAML tags, which failed to deserialize after a roundtrip through + the JSON status ([#ZZZZ]). + +### Fixed + +- Fix a panic during conversions when multiple (nested) containers track values for the same + version. Each container now only applies the tracked values matching its JSON paths and keeps + all others ([#ZZZZ]). + +[#ZZZZ]: https://github.com/stackabletech/operator-rs/pull/ZZZZ + ## [0.13.0] - 2026-10-14 ### Changed diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index d465779e1..fe80f1654 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 -yaml_serde.workspace = true snafu.workspace = true [dev-dependencies] diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs index ed9ee1689..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: yaml_serde::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 new file mode 100644 index 000000000..eae96c7c3 --- /dev/null +++ b/crates/stackable-versioned/tests/enum_tracking.rs @@ -0,0 +1,181 @@ +use kube::{ + CustomResource, + core::{conversion::ConversionReview, response::StatusSummary}, +}; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use stackable_versioned::versioned; + +#[versioned( + version(name = "v1alpha1"), + version(name = "v1alpha2"), + options(k8s(experimental_conversion_tracking)) +)] +pub mod versioned { + #[versioned(crd(group = "test.stackable.tech", doc = "Test"))] + #[derive(Clone, Debug, CustomResource, Deserialize, JsonSchema, Serialize)] + #[serde(rename_all = "camelCase")] + pub struct CatalogSpec { + #[versioned(nested)] + connector: Connector, + } + + #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] + #[serde(rename_all = "camelCase")] + pub enum Connector { + #[versioned(nested)] + Iceberg(IcebergConnector), + + Tpch(TpchConnector), + } + + #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] + #[serde(rename_all = "camelCase")] + pub struct IcebergConnector { + #[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![ + Self { + connector: v1alpha1::Connector::Iceberg(v1alpha1::IcebergConnector { + metastore: Some("hive".to_owned()), + }), + }, + Self { + connector: v1alpha1::Connector::Iceberg(v1alpha1::IcebergConnector { + metastore: None, + }), + }, + Self { + connector: v1alpha1::Connector::Tpch(TpchConnector {}), + }, + ] + } +} + +impl stackable_versioned::test_utils::RoundtripTestData for v1alpha2::CatalogSpec { + fn roundtrip_test_data() -> Vec { + vec![ + // 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 { + catalog: IcebergCatalog::Rest { + uri: "http://rest-catalog:8181".to_owned(), + }, + }), + }, + Self { + connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { + catalog: IcebergCatalog::HiveMetastore { + config_map: "hive".to_owned(), + }, + }), + }, + Self { + connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { + catalog: IcebergCatalog::UserProvided {}, + }), + }, + Self { + connector: v1alpha2::Connector::Tpch(TpchConnector {}), + }, + ] + } +} + +#[test] +fn tracks_values_through_enum_variants() { + 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/v1alpha1", + "objects": [{ + "apiVersion": "test.stackable.tech/v1alpha2", + "kind": "Catalog", + "metadata": {}, + "spec": { + "connector": { + "iceberg": { + "catalog": { + "rest": { + "uri": "http://rest-catalog:8181" + } + } + } + } + } + }] + } + })) + .expect("conversion review must be valid"); + + let response = Catalog::try_convert(review) + .response + .expect("v1alpha1 review must have a response"); + + assert_eq!(response.result.status, Some(StatusSummary::Success)); + + let object = response + .converted_objects + .first() + .expect("there must be at least one object"); + + assert_eq!( + object["spec"]["connector"]["iceberg"]["metastore"], + serde_json::Value::Null + ); + assert_eq!( + object["status"]["changedValues"]["upgrades"]["v1alpha2"], + serde_json::json!([{ + "jsonPath": "$.connector.Iceberg.0.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 a2cf037c6..f07e28234 100644 --- a/crates/stackable-versioned/tests/person.rs +++ b/crates/stackable-versioned/tests/person.rs @@ -70,8 +70,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, @@ -102,16 +100,8 @@ pub mod versioned { pub struct Married { with: String, - // FIXME (@Techassi): Any other version than v3 leads to a panic. There are two issues here: - // - // - The tracking impls for each container assume they are the only container which need to - // access to ChangedValues' `upgrades` and/or `downgrades` fields and currently removes - // the list of `ChangedValue`s for a particular version. The code must only remove the - // entire entry once the list inside is empty. Otherwise, other containers won't be able - // to access the tracked changes. - // - Each tracking impl must only look and remove the changes which matched with the - // expected JSON path. Unmatched JSON paths currently lead to a panic. They should instead - // be ignored. + // This field is added in the same version in which `gender` changed its type. This ensures + // that multiple (nested) containers can track values for the same version. #[versioned(added(since = "v3"))] since: String, } @@ -178,20 +168,19 @@ 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(), + relationship: v3::Relationship::Other {}, + 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..d27aa96c4 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,67 @@ 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", + "relationship": { + "single": {} + }, + "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 4ff52d2f4..8b344b925 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 @@ -198,6 +198,11 @@ input_file: crates/stackable-versioned/tests/inputs/conversions/pass/persons_to_ { "jsonPath": "$.relationship.Married.0.since", "value": "1970-01-01" + }, + { + "downgradedValue": "Male", + "jsonPath": "$.gender", + "value": "Male" } ] }