From 5d7902142fe5c7ab0a820117a80b5f4a4e34d5c9 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Tue, 6 Oct 2026 13:30:17 +0200 Subject: [PATCH 1/5] feat(versioned): Add support for from_docs attribute --- .../src/attrs/item/mod.rs | 23 ++- .../src/codegen/item/field.rs | 21 ++- .../src/codegen/item/mod.rs | 34 +++- .../src/codegen/item/variant.rs | 21 ++- crates/stackable-versioned-macros/src/lib.rs | 54 +++++++ .../tests/inputs/fail/changed.stderr | 2 +- .../tests/inputs/pass/docs.rs | 31 ++++ ...ioned_macros__snapshots__pass@docs.rs.snap | 149 ++++++++++++++++++ crates/stackable-versioned/CHANGELOG.md | 2 + 9 files changed, 318 insertions(+), 19 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/item/mod.rs b/crates/stackable-versioned-macros/src/attrs/item/mod.rs index 4c8e2dfcf..fe63e28d3 100644 --- a/crates/stackable-versioned-macros/src/attrs/item/mod.rs +++ b/crates/stackable-versioned-macros/src/attrs/item/mod.rs @@ -8,7 +8,7 @@ use syn::{Attribute, Path, Type, spanned::Spanned}; use crate::{ codegen::{VersionDefinition, item::ItemStatus}, - utils::ItemIdents, + utils::{ItemIdents, doc_comments::DocComments as _}, }; mod field; @@ -223,9 +223,12 @@ impl CommonItemAttributes { let mut errors = Error::accumulator(); for change in &self.changes { - if change.from_name.is_none() && change.from_type.is_none() { + if change.from_name.is_none() + && change.from_type.is_none() + && change.from_docs.is_none() + { errors.push(Error::custom( - "both `from_name` and `from_type` are unset. Is this `changed()` action needed?" + "`from_name`, `from_type` and `from_docs` are unset. Is this `changed()` action needed?" ).with_span(&change.since.span())); } @@ -288,6 +291,18 @@ impl CommonItemAttributes { } impl CommonItemAttributes { + /// Returns the doc comments of the item before each change which provides `from_docs`, keyed + /// by the version of the change. + pub fn previous_docs(&self) -> BTreeMap> { + self.changes + .iter() + .filter_map(|change| { + let docs = change.from_docs.as_deref()?; + Some((*change.since, docs.as_str().into_doc_comments())) + }) + .collect() + } + #[expect(clippy::too_many_lines)] pub fn into_changeset( self, @@ -458,11 +473,13 @@ fn default_default_fn() -> SpannedValue { /// - `changed(since = "...", from_name = "...", from_type="...")` /// - `changed(since = "...", from_name = "...", from_type="...", upgrade_with = "...")` /// - `changed(since = "...", from_name = "...", from_type="...", downgrade_with = "...")` +/// - `changed(since = "...", from_docs = "...")` #[derive(Clone, Debug, FromMeta)] pub struct ChangedAttributes { pub since: SpannedValue, pub from_name: Option>, pub from_type: Option>, + pub from_docs: Option>, pub upgrade_with: Option>, pub downgrade_with: Option>, } diff --git a/crates/stackable-versioned-macros/src/codegen/item/field.rs b/crates/stackable-versioned-macros/src/codegen/item/field.rs index ea074aeea..ecc81155e 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/field.rs @@ -11,7 +11,7 @@ use crate::{ codegen::{ Direction, VersionDefinition, changes::{BTreeMapExt, ChangesetExt}, - item::ItemStatus, + item::{ItemStatus, generate_attributes}, module::ModuleGenerationContext, }, utils::{ItemIdentExt, ItemIdents}, @@ -20,6 +20,7 @@ use crate::{ #[derive(Debug)] pub struct VersionedField { pub original_attributes: Vec, + pub previous_docs: BTreeMap>, pub changes: Option>, pub idents: FieldIdents, pub hint: Option, @@ -45,6 +46,7 @@ impl VersionedField { })?; let idents = FieldIdents::from(ident); + let previous_docs = field_attributes.common.previous_docs(); let changes = field_attributes .common .into_changeset(&idents, field.ty.clone()); @@ -53,6 +55,7 @@ impl VersionedField { Ok(Self { original_attributes: field_attributes.attrs, hint: field_attributes.hint, + previous_docs, ty: field.ty, changes, idents, @@ -82,7 +85,11 @@ impl VersionedField { /// } /// ``` pub fn generate_for_container(&self, version: &VersionDefinition) -> Option { - let original_attributes = &self.original_attributes; + let attributes = generate_attributes( + &self.original_attributes, + &self.previous_docs, + &version.inner, + ); #[allow(clippy::single_match_else)] match &self.changes { @@ -106,13 +113,13 @@ impl VersionedField { ) }) { ItemStatus::Addition { ident, ty, .. } => Some(quote! { - #(#original_attributes)* + #attributes pub #ident: #ty, }), ItemStatus::Change { to_ident, to_type, .. } => Some(quote! { - #(#original_attributes)* + #attributes pub #to_ident: #to_type, }), ItemStatus::Deprecation { @@ -133,7 +140,7 @@ impl VersionedField { }; Some(quote! { - #(#original_attributes)* + #attributes #deprecated_attr pub #field_ident: #field_type, }) @@ -149,7 +156,7 @@ impl VersionedField { let deprecated_attr = previously_deprecated.then(|| quote! {#[deprecated]}); Some(quote! { - #(#original_attributes)* + #attributes #deprecated_attr pub #ident: #ty, }) @@ -163,7 +170,7 @@ impl VersionedField { let field_type = &self.ty; Some(quote! { - #(#original_attributes)* + #attributes pub #field_ident: #field_type, }) } diff --git a/crates/stackable-versioned-macros/src/codegen/item/mod.rs b/crates/stackable-versioned-macros/src/codegen/item/mod.rs index 46abd7df8..f26b17747 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/mod.rs @@ -1,5 +1,12 @@ +use std::{collections::BTreeMap, ops::Bound}; + use darling::util::IdentString; -use syn::{Path, Type}; +use k8s_version::Version; +use proc_macro2::TokenStream; +use quote::quote; +use syn::{Attribute, Meta, Path, Type}; + +use crate::codegen::changes::Neighbors as _; mod field; pub use field::*; @@ -7,6 +14,31 @@ pub use field::*; mod variant; pub use variant::*; +/// Generates the attributes of an item (field or variant) for the provided `version`. +/// +/// If the docs of the item are changed in a later version (via `from_docs`), the original doc +/// comments are replaced by the docs which are valid in `version`. +pub fn generate_attributes( + original_attributes: &[Attribute], + previous_docs: &BTreeMap>, + version: &Version, +) -> TokenStream { + // The docs valid in this version are the previous docs of the closest change after this + // version. If there is no such change, the original docs are still valid. + let Some((_, docs)) = previous_docs.up_bound(Bound::Excluded(version)) else { + return quote! { #(#original_attributes)* }; + }; + + let attributes = original_attributes.iter().filter(|attribute| { + !matches!(&attribute.meta, Meta::NameValue(name_value) if name_value.path.is_ident("doc")) + }); + + quote! { + #(#[doc = #docs])* + #(#attributes)* + } +} + #[derive(Debug, PartialEq, Eq)] pub enum ItemStatus { Addition { diff --git a/crates/stackable-versioned-macros/src/codegen/item/variant.rs b/crates/stackable-versioned-macros/src/codegen/item/variant.rs index 0175b43aa..6455ef6b7 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/variant.rs @@ -13,13 +13,14 @@ use crate::{ codegen::{ Direction, VersionDefinition, changes::{BTreeMapExt, ChangesetExt}, - item::ItemStatus, + item::{ItemStatus, generate_attributes}, }, utils::ItemIdents, }; pub struct VersionedVariant { pub original_attributes: Vec, + pub previous_docs: BTreeMap>, pub changes: Option>, pub idents: VariantIdents, pub fields: Fields, @@ -39,10 +40,12 @@ impl VersionedVariant { attrs: Vec::new(), bang_token: Not([Span::call_site()]), }); + let previous_docs = variant_attributes.common.previous_docs(); let changes = variant_attributes.common.into_changeset(&idents, ty); Ok(Self { original_attributes: variant_attributes.attrs, + previous_docs, fields: variant.fields, idents, changes, @@ -63,7 +66,11 @@ impl VersionedVariant { /// Generates tokens to be used in a container definition. pub fn generate_for_container(&self, version: &VersionDefinition) -> Option { - let original_attributes = &self.original_attributes; + let attributes = generate_attributes( + &self.original_attributes, + &self.previous_docs, + &version.inner, + ); let fields = &self.fields; #[allow(clippy::single_match_else)] @@ -80,11 +87,11 @@ impl VersionedVariant { ) }) { ItemStatus::Addition { ident, .. } => Some(quote! { - #(#original_attributes)* + #attributes #ident #fields, }), ItemStatus::Change { to_ident, .. } => Some(quote! { - #(#original_attributes)* + #attributes #to_ident #fields, }), ItemStatus::Deprecation { ident, note, .. } => { @@ -103,7 +110,7 @@ impl VersionedVariant { }; Some(quote! { - #(#original_attributes)* + #attributes #deprecated_attr #ident #fields, }) @@ -118,7 +125,7 @@ impl VersionedVariant { let deprecated_attr = previously_deprecated.then(|| quote! {#[deprecated]}); Some(quote! { - #(#original_attributes)* + #attributes #deprecated_attr #ident #fields, }) @@ -132,7 +139,7 @@ impl VersionedVariant { let ident = &self.idents.original; Some(quote! { - #(#original_attributes)* + #attributes #ident #fields, }) } diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs index 4e666a6b2..9859ea734 100644 --- a/crates/stackable-versioned-macros/src/lib.rs +++ b/crates/stackable-versioned-macros/src/lib.rs @@ -496,6 +496,7 @@ mod utils; /// - `since` to indicate since which version the item is changed. /// - `from_name` to indicate from which previous name the field is renamed. /// - `from_type` to indicate from which previous type the field is changed. +/// - `from_docs` to provide the previous doc comments of the item. /// - `upgrade_with` to provide a custom upgrade function. This argument can /// only be used in combination with the `from_type` argument. The expected /// function signature is: `fn (OLD_TYPE) -> NEW_TYPE`. This function must @@ -561,6 +562,59 @@ mod utils; /// ``` /// /// +/// #### Changed Docs +/// +/// The doc comments of an item can be changed using the `from_docs` argument. +/// The doc comments attached to the item are used since the version of the +/// change. All earlier versions use the doc comments provided via `from_docs` +/// instead. This is especially useful to adjust the description of a field in +/// the generated CRD schema without changing it for older versions. The +/// argument can be used on its own or in combination with any other argument. +/// +/// ``` +/// # use stackable_versioned_macros::versioned; +/// #[versioned(version(name = "v1alpha1"), version(name = "v1beta1"))] +/// mod versioned { +/// pub struct Foo { +/// /// The number of bars. +/// #[versioned(changed(since = "v1beta1", from_docs = "The bar."))] +/// bar: usize, +/// baz: bool, +/// } +/// } +/// ``` +/// +///
+/// Expand Generated Code +/// +/// 1. In version `v1alpha1` the field uses the doc comments provided via +/// `from_docs`. +/// 2. In the next version, `v1beta1`, the field uses the doc comments attached +/// to the field. +/// +/// ```ignore +/// pub mod v1alpha1 { +/// use super::*; +/// pub struct Foo { +/// /// The bar. // 1 +/// pub bar: usize, +/// pub baz: bool, +/// } +/// } +/// +/// // Snip +/// +/// pub mod v1beta1 { +/// use super::*; +/// pub struct Foo { +/// /// The number of bars. // 2 +/// pub bar: usize, +/// pub baz: bool, +/// } +/// } +/// ``` +///
+/// /// ### Deprecated Action /// /// This action indicates that an item is deprecated in a particular version. diff --git a/crates/stackable-versioned-macros/tests/inputs/fail/changed.stderr b/crates/stackable-versioned-macros/tests/inputs/fail/changed.stderr index 61d10138f..417ca1250 100644 --- a/crates/stackable-versioned-macros/tests/inputs/fail/changed.stderr +++ b/crates/stackable-versioned-macros/tests/inputs/fail/changed.stderr @@ -10,7 +10,7 @@ error: the previous name must not start with the deprecation prefix 13 | changed(since = "v1", from_name = "deprecated_baz"), | ^^^^^^^^^^^^^^^^ -error: both `from_name` and `from_type` are unset. Is this `changed()` action needed? +error: `from_name`, `from_type` and `from_docs` are unset. Is this `changed()` action needed? --> tests/inputs/fail/changed.rs:14:29 | 14 | changed(since = "v2") diff --git a/crates/stackable-versioned-macros/tests/inputs/pass/docs.rs b/crates/stackable-versioned-macros/tests/inputs/pass/docs.rs index 23888ab90..c00fd6d5b 100644 --- a/crates/stackable-versioned-macros/tests/inputs/pass/docs.rs +++ b/crates/stackable-versioned-macros/tests/inputs/pass/docs.rs @@ -35,6 +35,37 @@ mod versioned { #[versioned(changed(since = "v1beta1", from_name = "qoox"))] #[versioned(changed(since = "v1", from_name = "qaax"))] quux: String, + + /// The docs of this field changed in v1beta1 and v2. + #[versioned( + changed(since = "v1beta1", from_docs = "These are the docs in v1alpha1."), + changed( + since = "v2", + from_docs = r#" + These are the docs from v1beta1 until v1. + + Multi-line docs are also supported. + "# + ) + )] + #[doc(alias = "grault")] + corge: String, + + /// The docs of this field changed in v1, while it was renamed in v1beta1. + #[versioned( + changed(since = "v1beta1", from_name = "waldo"), + changed(since = "v1", from_docs = "These are the docs before v1.") + )] + fred: String, + } + + /// Test + #[derive(Default)] + enum Bar { + /// The docs of this variant changed in v1. + #[versioned(changed(since = "v1", from_docs = "These are the docs before v1."))] + #[default] + Baz, } } // --- 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 d37c68374..684338443 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 @@ -15,6 +15,18 @@ mod v1alpha1 { pub bar: String, /// This is will keep changing over time. pub qoox: String, + ///These are the docs in v1alpha1. + #[doc(alias = "grault")] + pub corge: String, + ///These are the docs before v1. + pub waldo: String, + } + /// Test + #[derive(Default)] + pub enum Bar { + ///These are the docs before v1. + #[default] + Baz, } } #[automatically_derived] @@ -31,6 +43,8 @@ where deprecated_bar: __sv_foo.bar.into(), baz: ::std::default::Default::default(), qaax: __sv_foo.qoox.into(), + 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 { @@ -68,11 +82,29 @@ where foo: __sv_foo.foo.into(), bar: __sv_foo.deprecated_bar.into(), qoox: __sv_foo.qaax.into(), + corge: __sv_foo.corge.into(), + waldo: __sv_foo.fred.into(), }; spec } } #[automatically_derived] +impl ::core::convert::From for v1beta1::Bar { + fn from(__sv_bar: v1alpha1::Bar) -> Self { + match __sv_bar { + v1alpha1::Bar::Baz => v1beta1::Bar::Baz, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1alpha1::Bar { + fn from(__sv_bar: v1beta1::Bar) -> Self { + match __sv_bar { + v1beta1::Bar::Baz => v1alpha1::Bar::Baz, + } + } +} +#[automatically_derived] mod v1beta1 { use super::*; ///Additional docs for this version which are purposefully long to @@ -90,6 +122,23 @@ mod v1beta1 { pub baz: String, /// This is will keep changing over time. pub qaax: String, + ///These are the docs from v1beta1 until v1. + /// + ///Multi-line docs are also supported. + #[doc(alias = "grault")] + pub corge: String, + ///These are the docs before v1. + pub fred: String, + } + ///Additional docs for this version which are purposefully long to + ///show how manual line wrapping works. \ + ///Multi-line docs are also supported, as per regular doc-comments. + /// Test + #[derive(Default)] + pub enum Bar { + ///These are the docs before v1. + #[default] + Baz, } } #[automatically_derived] @@ -105,6 +154,8 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), qaax: __sv_foo.qaax.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } @@ -122,11 +173,29 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), qaax: __sv_foo.qaax.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } } #[automatically_derived] +impl ::core::convert::From for v1beta2::Bar { + fn from(__sv_bar: v1beta1::Bar) -> Self { + match __sv_bar { + v1beta1::Bar::Baz => v1beta2::Bar::Baz, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1beta1::Bar { + fn from(__sv_bar: v1beta2::Bar) -> Self { + match __sv_bar { + v1beta2::Bar::Baz => v1beta1::Bar::Baz, + } + } +} +#[automatically_derived] mod v1beta2 { use super::*; /// Test @@ -141,6 +210,20 @@ mod v1beta2 { pub baz: String, /// This is will keep changing over time. pub qaax: String, + ///These are the docs from v1beta1 until v1. + /// + ///Multi-line docs are also supported. + #[doc(alias = "grault")] + pub corge: String, + ///These are the docs before v1. + pub fred: String, + } + /// Test + #[derive(Default)] + pub enum Bar { + ///These are the docs before v1. + #[default] + Baz, } } #[automatically_derived] @@ -156,6 +239,8 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), quux: __sv_foo.qaax.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } @@ -173,11 +258,29 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), qaax: __sv_foo.quux.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } } #[automatically_derived] +impl ::core::convert::From for v1::Bar { + fn from(__sv_bar: v1beta2::Bar) -> Self { + match __sv_bar { + v1beta2::Bar::Baz => v1::Bar::Baz, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1beta2::Bar { + fn from(__sv_bar: v1::Bar) -> Self { + match __sv_bar { + v1::Bar::Baz => v1beta2::Bar::Baz, + } + } +} +#[automatically_derived] mod v1 { use super::*; /// Test @@ -192,6 +295,20 @@ mod v1 { pub baz: String, /// This is will keep changing over time. pub quux: String, + ///These are the docs from v1beta1 until v1. + /// + ///Multi-line docs are also supported. + #[doc(alias = "grault")] + pub corge: String, + /// The docs of this field changed in v1, while it was renamed in v1beta1. + pub fred: String, + } + /// Test + #[derive(Default)] + pub enum Bar { + /// The docs of this variant changed in v1. + #[default] + Baz, } } #[automatically_derived] @@ -207,6 +324,8 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), quux: __sv_foo.quux.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } @@ -224,11 +343,29 @@ where deprecated_bar: __sv_foo.deprecated_bar.into(), baz: __sv_foo.baz.into(), quux: __sv_foo.quux.into(), + corge: __sv_foo.corge.into(), + fred: __sv_foo.fred.into(), }; spec } } #[automatically_derived] +impl ::core::convert::From for v2::Bar { + fn from(__sv_bar: v1::Bar) -> Self { + match __sv_bar { + v1::Bar::Baz => v2::Bar::Baz, + } + } +} +#[automatically_derived] +impl ::core::convert::From for v1::Bar { + fn from(__sv_bar: v2::Bar) -> Self { + match __sv_bar { + v2::Bar::Baz => v1::Bar::Baz, + } + } +} +#[automatically_derived] mod v2 { use super::*; /// Test @@ -243,5 +380,17 @@ mod v2 { pub baz: String, /// This is will keep changing over time. pub quux: String, + /// The docs of this field changed in v1beta1 and v2. + #[doc(alias = "grault")] + pub corge: String, + /// The docs of this field changed in v1, while it was renamed in v1beta1. + pub fred: String, + } + /// Test + #[derive(Default)] + pub enum Bar { + /// The docs of this variant changed in v1. + #[default] + Baz, } } diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 3cf01678b..a2bc02e7a 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -8,9 +8,11 @@ All notable changes to this project will be documented in this file. - Add `#[versioned(hint(map))]` to provide a hint for map types during conversion ([#1285]). - Emit generics in plain `From` impl ([#1284]). +- Add `from_docs` argument to the `changed()` action to provide the previous doc comments of an item ([#XXXX]). [#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 ## [0.11.1] - 2026-07-06 From 5b2d91118539ad88a59903427d66448d021ef6a5 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 9 Oct 2026 15:49:10 +0200 Subject: [PATCH 2/5] feat(versioned): Support conversion tracking through enums With experimental_conversion_tracking enabled, versioned enums now implement TrackingFrom instead of From, in line with structs. Variants marked with as an additional path segment (eg. `$.connector.Iceberg.field`). Also fix the generated From impls for variants with multiple unnamed fields, which were missing separating commas. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/attrs/item/variant.rs | 18 +- .../src/codegen/container/enum.rs | 45 +- .../src/codegen/item/variant.rs | 57 +- .../src/codegen/module.rs | 20 +- crates/stackable-versioned-macros/src/lib.rs | 44 ++ .../inputs/pass/conversion_tracking_enum.rs | 56 ++ ...ots__pass@conversion_tracking_enum.rs.snap | 576 ++++++++++++++++++ ...ioned_macros__snapshots__pass@docs.rs.snap | 64 +- crates/stackable-versioned/CHANGELOG.md | 14 + .../tests/enum_tracking.rs | 131 ++++ 10 files changed, 989 insertions(+), 36 deletions(-) create mode 100644 crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_enum.rs create mode 100644 crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap create mode 100644 crates/stackable-versioned/tests/enum_tracking.rs diff --git a/crates/stackable-versioned-macros/src/attrs/item/variant.rs b/crates/stackable-versioned-macros/src/attrs/item/variant.rs index 46d1c69fe..06cd4b3ed 100644 --- a/crates/stackable-versioned-macros/src/attrs/item/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/item/variant.rs @@ -1,5 +1,5 @@ use convert_case::{Case, Casing}; -use darling::{Error, FromVariant, Result}; +use darling::{Error, FromVariant, Result, util::Flag}; use syn::{Attribute, Ident}; use crate::{ @@ -40,6 +40,11 @@ pub struct VariantAttributes { // FromMeta. /// The original attributes for the field. pub attrs: Vec, + + /// Indicates that the data of this variant contains nested sub structs or enums. The + /// indicator is needed to let the macro know to generate conversion code with support for + /// tracking across container boundaries. + pub nested: Flag, } impl VariantAttributes { @@ -73,6 +78,17 @@ impl VariantAttributes { pub fn validate_versions(&self, versions: &[VersionDefinition]) -> Result<()> { self.common.validate_versions(versions) } + + pub fn validate_nested_flag(&self, experimental_conversion_tracking: bool) -> Result<()> { + if self.nested.is_present() && !experimental_conversion_tracking { + return Err( + Error::custom("the `nested` argument can only be used if the module-level `experimental_conversion_tracking` flag is set") + .with_span(&self.nested.span()) + ); + } + + Ok(()) + } } // We don't want to take a dependency on the (unmaintained[1]) `convert_case_extras` crate just for diff --git a/crates/stackable-versioned-macros/src/codegen/container/enum.rs b/crates/stackable-versioned-macros/src/codegen/container/enum.rs index fbe5b40b7..1730b3207 100644 --- a/crates/stackable-versioned-macros/src/codegen/container/enum.rs +++ b/crates/stackable-versioned-macros/src/codegen/container/enum.rs @@ -3,7 +3,7 @@ use std::ops::Not; use darling::{FromAttributes, Result}; use proc_macro2::TokenStream; use quote::quote; -use syn::{Generics, ItemEnum}; +use syn::{Generics, ItemEnum, parse_quote}; use crate::{ attrs::container::ContainerAttributes, @@ -19,12 +19,17 @@ use crate::{ }; impl Container { - pub fn new_enum(item_enum: ItemEnum, versions: &[VersionDefinition]) -> Result { + pub fn new_enum( + item_enum: ItemEnum, + versions: &[VersionDefinition], + experimental_conversion_tracking: bool, + ) -> Result { let attributes = ContainerAttributes::from_attributes(&item_enum.attrs)?; let mut versioned_variants = Vec::new(); for variant in item_enum.variants { - let mut versioned_variant = VersionedVariant::new(variant, versions)?; + let mut versioned_variant = + VersionedVariant::new(variant, versions, experimental_conversion_tracking)?; versioned_variant.insert_container_versions(versions); versioned_variants.push(versioned_variant); } @@ -157,7 +162,7 @@ impl Enum { self.variants .iter() .filter_map(|v| { - v.generate_for_from_impl(direction, version, next_version, enum_ident) + v.generate_for_from_impl(direction, version, next_version, enum_ident, gen_ctx) }) .collect() }; @@ -177,6 +182,38 @@ impl Enum { }, }; + // With conversion tracking enabled, the enum needs to forward the status and the + // current path to the data of nested variants. As such, only a TrackingFrom impl is + // generated, which is in line with the impls generated for structs. + if gen_ctx.kubernetes_options.experimental_conversion_tracking.is_present() { + let versioned_path = &*gen_ctx.crates.versioned; + + let mut tracking_generics = self.generics.clone(); + tracking_generics.params.push(parse_quote! { S }); + tracking_generics + .make_where_clause() + .predicates + .push(parse_quote! { S: #versioned_path::TrackingStatus + ::core::default::Default }); + let (impl_generics, _, where_clause) = tracking_generics.split_for_impl(); + + return quote! { + #automatically_derived + #allow_attribute + impl #impl_generics #versioned_path::TrackingFrom<#from_module_ident::#enum_ident #type_generics, S> for #for_module_ident::#enum_ident #type_generics + #where_clause + { + fn tracking_from(#from_enum_ident: #from_module_ident::#enum_ident #type_generics, status: &mut S, parent: &str) -> Self { + // TODO (@Techassi): Only emit this if any of the variants below need it + use #versioned_path::TrackingInto as _; + + match #from_enum_ident { + #variants + } + } + } + }; + } + quote! { #automatically_derived #allow_attribute diff --git a/crates/stackable-versioned-macros/src/codegen/item/variant.rs b/crates/stackable-versioned-macros/src/codegen/item/variant.rs index 6455ef6b7..11234ca9d 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/variant.rs @@ -14,6 +14,7 @@ use crate::{ Direction, VersionDefinition, changes::{BTreeMapExt, ChangesetExt}, item::{ItemStatus, generate_attributes}, + module::ModuleGenerationContext, }, utils::ItemIdents, }; @@ -24,12 +25,18 @@ pub struct VersionedVariant { pub changes: Option>, pub idents: VariantIdents, pub fields: Fields, + pub nested: bool, } impl VersionedVariant { - pub fn new(variant: Variant, versions: &[VersionDefinition]) -> Result { + pub fn new( + variant: Variant, + versions: &[VersionDefinition], + experimental_conversion_tracking: bool, + ) -> Result { let variant_attributes = VariantAttributes::from_variant(&variant)?; variant_attributes.validate_versions(versions)?; + variant_attributes.validate_nested_flag(experimental_conversion_tracking)?; let idents = VariantIdents::from(variant.ident); @@ -41,6 +48,7 @@ impl VersionedVariant { bang_token: Not([Span::call_site()]), }); let previous_docs = variant_attributes.common.previous_docs(); + let nested = variant_attributes.nested.is_present(); let changes = variant_attributes.common.into_changeset(&idents, ty); Ok(Self { @@ -49,6 +57,7 @@ impl VersionedVariant { fields: variant.fields, idents, changes, + nested, }) } @@ -152,9 +161,10 @@ impl VersionedVariant { version: &VersionDefinition, next_version: &VersionDefinition, enum_ident: &IdentString, + mod_gen_ctx: ModuleGenerationContext<'_>, ) -> Option { let from_fields = self.generate_from_fields(); - let for_fields = self.generate_for_fields(); + let for_fields = self.generate_for_fields(mod_gen_ctx); #[allow(clippy::single_match_else)] match &self.changes { @@ -203,20 +213,55 @@ impl VersionedVariant { } } - fn generate_for_fields(&self) -> Option { + fn generate_for_fields(&self, mod_gen_ctx: ModuleGenerationContext<'_>) -> Option { match &self.fields { Fields::Named(fields_named) => { let fields = Self::named_field_idents(fields_named); - Some(quote! { { #(#fields: #fields.into(),)* } }) + let conversions = fields.iter().map(|field| { + self.generate_conversion_function(Some(&field.to_string()), mod_gen_ctx) + }); + + Some(quote! { { #(#fields: #fields.#conversions,)* } }) } Fields::Unnamed(fields_unnamed) => { let fields = Self::unnamed_field_ident(fields_unnamed); - Some(quote! { ( #(#fields.into())* ) }) + + // Newtype variants (which are the most common variants with data) don't need an + // additional path segment, as the variant only contains a single field. + let conversions = (0..fields.len()).map(|index| { + let child = (fields.len() > 1).then(|| index.to_string()); + self.generate_conversion_function(child.as_deref(), mod_gen_ctx) + }); + + Some(quote! { ( #(#fields.#conversions),* ) }) } Fields::Unit => None, } } + /// Generates the conversion function for a single field of the variant data. + /// + /// The data of variants marked as nested is converted with support for tracking. The path + /// passed down consists of the variant name and the provided `child`, if any. + fn generate_conversion_function( + &self, + child: Option<&str>, + mod_gen_ctx: ModuleGenerationContext<'_>, + ) -> TokenStream { + if !self.nested { + return quote! { into() }; + } + + let versioned_path = &*mod_gen_ctx.crates.versioned; + let variant = &self.idents.original; + let child_string = match child { + Some(child) => format!("{variant}.{child}"), + None => variant.to_string(), + }; + + quote! { tracking_into(status, &#versioned_path::jthong_path(parent, #child_string)) } + } + fn generate_from_fields(&self) -> Option { match &self.fields { Fields::Named(fields_named) => { @@ -225,7 +270,7 @@ impl VersionedVariant { } Fields::Unnamed(fields_unnamed) => { let fields = Self::unnamed_field_ident(fields_unnamed); - Some(quote! { ( #(#fields)* ) }) + Some(quote! { ( #(#fields),* ) }) } Fields::Unit => None, } diff --git a/crates/stackable-versioned-macros/src/codegen/module.rs b/crates/stackable-versioned-macros/src/codegen/module.rs index 681321850..c76e85e42 100644 --- a/crates/stackable-versioned-macros/src/codegen/module.rs +++ b/crates/stackable-versioned-macros/src/codegen/module.rs @@ -50,22 +50,24 @@ impl Module { let mut submodules = HashMap::new(); let mut containers = Vec::new(); + let experimental_conversion_tracking = module_attributes + .options + .kubernetes + .experimental_conversion_tracking + .is_present(); + for item in items { match item { Item::Enum(item_enum) => { - if let Some(container) = - errors.handle(Container::new_enum(item_enum, &versions)) - { + if let Some(container) = errors.handle(Container::new_enum( + item_enum, + &versions, + experimental_conversion_tracking, + )) { containers.push(container); } } Item::Struct(item_struct) => { - let experimental_conversion_tracking = module_attributes - .options - .kubernetes - .experimental_conversion_tracking - .is_present(); - if let Some(container) = errors.handle(Container::new_struct( item_struct, &versions, diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs index 9859ea734..e688ea5d8 100644 --- a/crates/stackable-versioned-macros/src/lib.rs +++ b/crates/stackable-versioned-macros/src/lib.rs @@ -969,6 +969,50 @@ mod utils; /// # fn main() {} /// ``` /// +/// Changes can also be tracked through enums. With conversion tracking enabled, +/// versioned structs and enums only implement `TrackingFrom` instead of `From`. +/// As such, every field and every enum variant containing a versioned struct +/// or enum needs to be marked with `#[versioned(nested)]`. All data of a nested +/// variant is converted with tracking support. The path of tracked values +/// contains the variant name, eg. `$.connector.Baz.quox`. +/// +/// ``` +/// # use stackable_versioned_macros::versioned; +/// # use kube::CustomResource; +/// # use schemars::JsonSchema; +/// # use serde::{Deserialize, Serialize}; +/// #[versioned( +/// version(name = "v1alpha1"), +/// version(name = "v1beta1"), +/// options(k8s(experimental_conversion_tracking)) +/// )] +/// mod versioned { +/// #[versioned(crd(group = "example.com", doc = "My custom resource."))] +/// #[derive(Clone, Debug, Deserialize, Serialize, CustomResource, JsonSchema)] +/// struct FooSpec { +/// #[versioned(nested)] +/// connector: Connector, +/// } +/// +/// #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +/// enum Connector { +/// #[versioned(nested)] +/// Baz(Baz), +/// +/// Unit, +/// } +/// +/// #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +/// struct Baz { +/// quax: String, +/// +/// #[versioned(added(since = "v1beta1"))] +/// quox: bool, +/// } +/// } +/// # fn main() {} +/// ``` +/// /// # OpenTelemetry Semantic Conventions /// /// If tracing is enabled, various traces and events are emitted. The fields of diff --git a/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_enum.rs b/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_enum.rs new file mode 100644 index 000000000..428ee98d4 --- /dev/null +++ b/crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_enum.rs @@ -0,0 +1,56 @@ +use kube::CustomResource; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use stackable_versioned::versioned; +// --- +#[versioned( + version(name = "v1alpha1"), + version(name = "v1alpha2"), + 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 { + // With conversion tracking enabled, enums only implement TrackingFrom. As such, fields + // using a versioned enum need to be marked as nested. + #[versioned(nested)] + connector: Connector, + } + + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub(crate) enum Connector { + // The data of this variant is a versioned struct, which tracks changes. + #[versioned(nested)] + Iceberg(IcebergConnector), + + // Nested variants with named fields are supported as well. + #[versioned(nested)] + Hive { connector: IcebergConnector }, + + // Nested tuple variants with multiple fields use the index as an additional path segment. + #[versioned(nested)] + Both(IcebergConnector, IcebergConnector), + + // The data of this variant is not versioned and is converted using From. + Tpch(TpchConnector), + + Tpcds(TpchConnector, TpchConnector), + + Unit, + } + + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub(crate) struct IcebergConnector { + metastore: Option, + + #[versioned(added(since = "v1alpha2"))] + rest_catalog_uri: Option, + } +} +// --- +fn main() {} + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct TpchConnector {} 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 new file mode 100644 index 000000000..40f7abe7f --- /dev/null +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_enum.rs.snap @@ -0,0 +1,576 @@ +--- +source: crates/stackable-versioned-macros/src/lib.rs +expression: formatted +input_file: crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_enum.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 connector: Connector, + } + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub enum Connector { + Iceberg(IcebergConnector), + Hive { connector: IcebergConnector }, + Both(IcebergConnector, IcebergConnector), + Tpch(TpchConnector), + Tpcds(TpchConnector, TpchConnector), + Unit, + } + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub struct IcebergConnector { + pub metastore: Option, + } +} +#[automatically_derived] +impl ::core::convert::From for v1alpha2::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: v1alpha2::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 v1alpha2::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_connector_path = ::stackable_versioned::jthong_path( + parent, + "connector", + ); + let mut spec = Self { + connector: __sv_foospec.connector.tracking_into(status, &__sv_connector_path), + }; + spec + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom for v1alpha1::FooSpec +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_foospec: v1alpha2::FooSpec, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_connector_path = ::stackable_versioned::jthong_path( + parent, + "connector", + ); + let mut spec = Self { + connector: __sv_foospec.connector.tracking_into(status, &__sv_connector_path), + }; + spec + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom +for v1alpha2::Connector +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_connector: v1alpha1::Connector, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + match __sv_connector { + v1alpha1::Connector::Iceberg(__sv_0) => { + v1alpha2::Connector::Iceberg( + __sv_0 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Iceberg"), + ), + ) + } + v1alpha1::Connector::Hive { connector } => { + v1alpha2::Connector::Hive { + connector: connector + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Hive.connector"), + ), + } + } + v1alpha1::Connector::Both(__sv_0, __sv_1) => { + v1alpha2::Connector::Both( + __sv_0 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Both.0"), + ), + __sv_1 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Both.1"), + ), + ) + } + v1alpha1::Connector::Tpch(__sv_0) => v1alpha2::Connector::Tpch(__sv_0.into()), + v1alpha1::Connector::Tpcds(__sv_0, __sv_1) => { + v1alpha2::Connector::Tpcds(__sv_0.into(), __sv_1.into()) + } + v1alpha1::Connector::Unit => v1alpha2::Connector::Unit, + } + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom +for v1alpha1::Connector +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_connector: v1alpha2::Connector, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + match __sv_connector { + v1alpha2::Connector::Iceberg(__sv_0) => { + v1alpha1::Connector::Iceberg( + __sv_0 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Iceberg"), + ), + ) + } + v1alpha2::Connector::Hive { connector } => { + v1alpha1::Connector::Hive { + connector: connector + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Hive.connector"), + ), + } + } + v1alpha2::Connector::Both(__sv_0, __sv_1) => { + v1alpha1::Connector::Both( + __sv_0 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Both.0"), + ), + __sv_1 + .tracking_into( + status, + &::stackable_versioned::jthong_path(parent, "Both.1"), + ), + ) + } + v1alpha2::Connector::Tpch(__sv_0) => v1alpha1::Connector::Tpch(__sv_0.into()), + v1alpha2::Connector::Tpcds(__sv_0, __sv_1) => { + v1alpha1::Connector::Tpcds(__sv_0.into(), __sv_1.into()) + } + v1alpha2::Connector::Unit => v1alpha1::Connector::Unit, + } + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom +for v1alpha2::IcebergConnector +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_icebergconnector: v1alpha1::IcebergConnector, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_rest_catalog_uri_path = ::stackable_versioned::jthong_path( + parent, + "rest_catalog_uri", + ); + let mut spec = Self { + 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 { + match json_path { + json_path if json_path == __sv_rest_catalog_uri_path => { + spec.rest_catalog_uri = serde_yaml::from_value(value).unwrap(); + } + _ => unreachable!(), + } + } + } + spec + } +} +#[automatically_derived] +impl ::stackable_versioned::TrackingFrom +for v1alpha1::IcebergConnector +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from( + __sv_icebergconnector: v1alpha2::IcebergConnector, + status: &mut S, + parent: &str, + ) -> Self { + use ::stackable_versioned::TrackingInto as _; + let __sv_rest_catalog_uri_path = ::stackable_versioned::jthong_path( + parent, + "rest_catalog_uri", + ); + let upgrades = status + .changes() + .upgrades + .entry("v1alpha2".to_owned()) + .or_default(); + upgrades + .push(::stackable_versioned::ChangedValue { + json_path: __sv_rest_catalog_uri_path, + value: ::serde_yaml::to_value(&__sv_icebergconnector.rest_catalog_uri) + .unwrap(), + }); + let mut spec = Self { + metastore: __sv_icebergconnector.metastore.into(), + }; + spec + } +} +#[automatically_derived] +pub(crate) mod v1alpha2 { + use super::*; + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, CustomResource)] + #[kube( + group = "stackable.tech", + version = "v1alpha2", + kind = "Foo", + doc = "Test", + status = FooStatusWithChangedValues + )] + pub struct FooSpec { + pub connector: Connector, + } + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub enum Connector { + Iceberg(IcebergConnector), + Hive { connector: IcebergConnector }, + Both(IcebergConnector, IcebergConnector), + Tpch(TpchConnector), + Tpcds(TpchConnector, TpchConnector), + Unit, + } + #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] + pub struct IcebergConnector { + pub metastore: Option, + pub rest_catalog_uri: Option, + } +} +#[automatically_derived] +#[derive(::core::fmt::Debug)] +pub(crate) enum Foo { + V1Alpha1(v1alpha1::Foo), + V1Alpha2(v1alpha2::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(), < + v1alpha2::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::V1Alpha2) => { + let converted: v1alpha2::Foo = __sv_foo.into(); + let desired_object = Self::V1Alpha2(converted); + let desired_object = desired_object + .into_json_value() + .map_err(|source| ::stackable_versioned::ConvertObjectError::Serialize { + source, + })?; + converted_objects.push(desired_object); + } + (Self::V1Alpha2(__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); + } + _ => 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/v1alpha2" => { + let object = ::serde_json::from_value(object_value) + .map_err(|source| ::stackable_versioned::ParseObjectError::Deserialize { + source, + })?; + Self::V1Alpha2(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::V1Alpha2(__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, + V1Alpha2, +} +#[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::V1Alpha2 => "v1alpha2", + } + } + pub fn as_api_version_str(&self) -> &str { + match self { + FooVersion::V1Alpha1 => "stackable.tech/v1alpha1", + FooVersion::V1Alpha2 => "stackable.tech/v1alpha2", + } + } + pub fn from_api_version( + api_version: &str, + ) -> Result { + match api_version { + "stackable.tech/v1alpha1" => Ok(FooVersion::V1Alpha1), + "stackable.tech/v1alpha2" => Ok(FooVersion::V1Alpha2), + _ => { + Err(::stackable_versioned::UnknownDesiredApiVersionError { + api_version: api_version.to_owned(), + }) + } + } + } +} +#[cfg(test)] +#[test] +fn FooSpec_roundtrip_down_up() { + ::stackable_versioned::test_utils::test_roundtrip::< + v1alpha2::FooSpec, + >( + stringify!(Foo), + "stackable.tech/v1alpha2", + "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/v1alpha2", + 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 684338443..4655f7ddb 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 @@ -89,16 +89,24 @@ where } } #[automatically_derived] -impl ::core::convert::From for v1beta1::Bar { - fn from(__sv_bar: v1alpha1::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1beta1::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1alpha1::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1alpha1::Bar::Baz => v1beta1::Bar::Baz, } } } #[automatically_derived] -impl ::core::convert::From for v1alpha1::Bar { - fn from(__sv_bar: v1beta1::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1alpha1::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1beta1::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1beta1::Bar::Baz => v1alpha1::Bar::Baz, } @@ -180,16 +188,24 @@ where } } #[automatically_derived] -impl ::core::convert::From for v1beta2::Bar { - fn from(__sv_bar: v1beta1::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1beta2::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1beta1::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1beta1::Bar::Baz => v1beta2::Bar::Baz, } } } #[automatically_derived] -impl ::core::convert::From for v1beta1::Bar { - fn from(__sv_bar: v1beta2::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1beta1::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1beta2::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1beta2::Bar::Baz => v1beta1::Bar::Baz, } @@ -265,16 +281,24 @@ where } } #[automatically_derived] -impl ::core::convert::From for v1::Bar { - fn from(__sv_bar: v1beta2::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1beta2::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1beta2::Bar::Baz => v1::Bar::Baz, } } } #[automatically_derived] -impl ::core::convert::From for v1beta2::Bar { - fn from(__sv_bar: v1::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1beta2::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1::Bar::Baz => v1beta2::Bar::Baz, } @@ -350,16 +374,24 @@ where } } #[automatically_derived] -impl ::core::convert::From for v2::Bar { - fn from(__sv_bar: v1::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v2::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v1::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v1::Bar::Baz => v2::Bar::Baz, } } } #[automatically_derived] -impl ::core::convert::From for v1::Bar { - fn from(__sv_bar: v2::Bar) -> Self { +impl ::stackable_versioned::TrackingFrom for v1::Bar +where + S: ::stackable_versioned::TrackingStatus + ::core::default::Default, +{ + fn tracking_from(__sv_bar: v2::Bar, status: &mut S, parent: &str) -> Self { + use ::stackable_versioned::TrackingInto as _; match __sv_bar { v2::Bar::Baz => v1::Bar::Baz, } diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index a2bc02e7a..5db878395 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -9,10 +9,24 @@ All notable changes to this project will be documented in this file. - Add `#[versioned(hint(map))]` to provide a hint for map types during conversion ([#1285]). - Emit generics in plain `From` impl ([#1284]). - Add `from_docs` argument to the `changed()` action to provide the previous doc comments of an item ([#XXXX]). +- 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]). + +### Changed + +- BREAKING: With `experimental_conversion_tracking` enabled, versioned enums only implement + `TrackingFrom` instead of `From`, in line with structs. Fields using versioned enums need to be + marked with `#[versioned(nested)]` ([#YYYY]). + +### Fixed + +- Fix `From` impls of enum variants with multiple unnamed fields ([#YYYY]). [#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 ## [0.11.1] - 2026-07-06 diff --git a/crates/stackable-versioned/tests/enum_tracking.rs b/crates/stackable-versioned/tests/enum_tracking.rs new file mode 100644 index 000000000..d5846f53a --- /dev/null +++ b/crates/stackable-versioned/tests/enum_tracking.rs @@ -0,0 +1,131 @@ +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 { + metastore: Option, + + #[versioned(added(since = "v1alpha2"))] + rest_catalog_uri: Option, + } +} + +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +pub struct TpchConnector {} + +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::Tpch(TpchConnector {}), + }, + ] + } +} + +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. + Self { + connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { + metastore: None, + rest_catalog_uri: Some("http://rest-catalog:8181".to_owned()), + }), + }, + Self { + connector: v1alpha2::Connector::Iceberg(v1alpha2::IcebergConnector { + metastore: Some("hive".to_owned()), + rest_catalog_uri: None, + }), + }, + 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": { + "metastore": null, + "restCatalogUri": "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"], + serde_json::json!({ "metastore": null }) + ); + assert_eq!( + object["status"]["changedValues"]["upgrades"]["v1alpha2"], + serde_json::json!([{ + "jsonPath": "$.connector.Iceberg.rest_catalog_uri", + "value": "http://rest-catalog:8181" + }]) + ); +} From 023c0beceff66773f324ce4fc42e98e413be05ae Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 9 Oct 2026 14:08:50 +0200 Subject: [PATCH 3/5] feat(versioned): Roundtrip-track changed types --- .../codegen/container/struct/conversion.rs | 101 ++- .../src/codegen/item/field.rs | 125 +++- .../src/codegen/item/mod.rs | 7 + crates/stackable-versioned-macros/src/lib.rs | 27 +- .../src/utils/mod.rs | 9 + .../pass/conversion_tracking_type_change.rs | 47 ++ ...napshots__pass@conversion_tracking.rs.snap | 5 +- ...ots__pass@conversion_tracking_enum.rs.snap | 5 +- ...ts__pass@conversion_tracking_hints.rs.snap | 5 +- ...ss@conversion_tracking_type_change.rs.snap | 576 ++++++++++++++++++ ...ioned_macros__snapshots__pass@docs.rs.snap | 5 +- crates/stackable-versioned/CHANGELOG.md | 5 + crates/stackable-versioned/src/lib.rs | 10 + crates/stackable-versioned/tests/person.rs | 28 +- crates/stackable-versioned/tests/roundtrip.rs | 63 +- ...rsions__pass@persons_to_v1alpha1.json.snap | 7 + 16 files changed, 978 insertions(+), 47 deletions(-) create mode 100644 crates/stackable-versioned-macros/tests/inputs/pass/conversion_tracking_type_change.rs create mode 100644 crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap 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..3286b30a4 100644 --- a/crates/stackable-versioned-macros/src/codegen/item/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/item/field.rs @@ -319,6 +319,89 @@ impl VersionedField { upgrades.push(#versioned_path::ChangedValue { json_path: #json_path_ident, value: #serde_yaml_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_yaml_path = &*mod_gen_ctx.crates.serde_yaml; + 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_yaml_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_yaml_path = &*mod_gen_ctx.crates.serde_yaml; + 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_yaml_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,30 @@ impl VersionedField { match direction { Direction::Upgrade => { let next_change = changes.get_expect(&next_version.inner); + let serde_yaml_path = &*mod_gen_ctx.crates.serde_yaml; 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_yaml_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. + 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.as_ref() == ::core::option::Option::Some(&#value_ident) { + spec.#to_ident = #serde_yaml_path::from_value(value).unwrap(); + } }, }) } @@ -390,17 +488,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..17a1c281c 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_yaml::from_value(value).unwrap(); } _ => unreachable!(), } @@ -98,6 +98,7 @@ where .push(::stackable_versioned::ChangedValue { json_path: __sv_bah_path, value: ::serde_yaml::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..bca6c86bb 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_yaml::from_value(value).unwrap(); } _ => unreachable!(), } @@ -269,6 +269,7 @@ where json_path: __sv_rest_catalog_uri_path, value: ::serde_yaml::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..a9db30422 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_yaml::from_value(value).unwrap(); } _ => unreachable!(), } @@ -90,6 +90,7 @@ where .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_baz_path, value: ::serde_yaml::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..400fea613 --- /dev/null +++ b/crates/stackable-versioned-macros/tests/snapshots/stackable_versioned_macros__snapshots__pass@conversion_tracking_type_change.rs.snap @@ -0,0 +1,576 @@ +--- +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_yaml::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.as_ref() + == ::core::option::Option::Some(&__sv_bah_value) + { + spec.bah = ::serde_yaml::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_yaml::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_yaml::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_yaml::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.as_ref() + == ::core::option::Option::Some(&__sv_bar_value) + { + spec.bar = ::serde_yaml::from_value(value).unwrap(); + } + } + json_path if json_path == __sv_quux_path => { + spec.quux = ::serde_yaml::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_yaml::to_value(&__sv_foospec.quux).unwrap(), + downgraded_value: ::core::option::Option::None, + }); + let __sv_bar_value = ::serde_yaml::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_yaml::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..f029642ef 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_yaml::from_value(value).unwrap(); } _ => unreachable!(), } @@ -77,6 +77,7 @@ where .push(::stackable_versioned::ChangedValue { json_path: __sv_baz_path, value: ::serde_yaml::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..2cfcaa713 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,13 @@ All notable changes to this project will be documented in this file. ### Fixed - Fix `From` impls of enum variants with multiple unnamed fields ([#YYYY]). +- Use the configured `serde_yaml` crate path when applying tracked values ([#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/src/lib.rs b/crates/stackable-versioned/src/lib.rs index 6aa4bddc3..41af98eac 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -115,6 +115,16 @@ pub struct ChangedValue { /// The value to be used when upgrading or downgrading the custom resource. #[schemars(schema_with = "raw_object_schema")] pub value: serde_yaml::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/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" + } ] } } From b9cbd8ca60ce17a737f5e3eeb3f449fd2d2f61ec Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 9 Oct 2026 16:13:08 +0200 Subject: [PATCH 4/5] fix(versioned): Track values as JSON Tracked values were stored as serde_yaml::Value, which represents enum variants as YAML tags. After a roundtrip through the JSON status, these could not be deserialized again, which panicked during the upgrade. The status is stored as JSON anyway, so values are now tracked as serde_json::Value. Also fix restoring type changes whose downgraded value is null: it is serialized as `downgradedValue: null` and deserialized as None, so the comparison never matched. A missing downgraded value is now treated as null. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 1 - .../src/codegen/item/field.rs | 23 +++--- ...napshots__pass@conversion_tracking.rs.snap | 4 +- ...ots__pass@conversion_tracking_enum.rs.snap | 4 +- ...ts__pass@conversion_tracking_hints.rs.snap | 4 +- ...ss@conversion_tracking_type_change.rs.snap | 28 +++---- ...ioned_macros__snapshots__pass@docs.rs.snap | 4 +- crates/stackable-versioned/CHANGELOG.md | 4 +- crates/stackable-versioned/Cargo.toml | 1 - crates/stackable-versioned/src/lib.rs | 7 +- .../tests/enum_tracking.rs | 82 +++++++++++++++---- 11 files changed, 107 insertions(+), 55 deletions(-) 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/item/field.rs b/crates/stackable-versioned-macros/src/codegen/item/field.rs index 3286b30a4..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,7 @@ 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, }); }) @@ -351,7 +351,7 @@ impl VersionedField { to_ident, .. } if next_change.is_type_change() => { - let serde_yaml_path = &*mod_gen_ctx.crates.serde_yaml; + let serde_json_path = &*mod_gen_ctx.crates.serde_json; let value_ident = to_ident.tracked_value_ident(); let field_ident = match direction { @@ -360,7 +360,7 @@ impl VersionedField { }; Some(quote! { - let #value_ident = #serde_yaml_path::to_value(&#from_struct_ident.#field_ident).unwrap(); + let #value_ident = #serde_json_path::to_value(&#from_struct_ident.#field_ident).unwrap(); }) } _ => None, @@ -383,7 +383,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 { @@ -400,7 +400,7 @@ impl VersionedField { json_path: #json_path_ident, value: #value_ident, downgraded_value: ::core::option::Option::Some( - #serde_yaml_path::to_value(&spec.#from_ident).unwrap() + #serde_json_path::to_value(&spec.#from_ident).unwrap() ), }); }) @@ -426,7 +426,7 @@ impl VersionedField { match direction { Direction::Upgrade => { 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; match next_change { ItemStatus::Addition { ident, .. } => { @@ -434,21 +434,24 @@ impl VersionedField { Some(quote! { json_path if json_path == #json_path_ident => { - spec.#ident = #serde_yaml_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.as_ref() == ::core::option::Option::Some(&#value_ident) { - spec.#to_ident = #serde_yaml_path::from_value(value).unwrap(); + if downgraded_value.unwrap_or_default() == #value_ident { + spec.#to_ident = #serde_json_path::from_value(value).unwrap(); } }, }) 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 17a1c281c..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 @@ -68,7 +68,7 @@ where 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,7 @@ 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 { 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 bca6c86bb..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 @@ -234,7 +234,7 @@ where 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,7 +267,7 @@ 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, }); 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 a9db30422..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 @@ -64,7 +64,7 @@ where 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,7 @@ 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 { 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 index 400fea613..e3e76e74e 100644 --- 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 @@ -61,7 +61,7 @@ where ) -> Self { use ::stackable_versioned::TrackingInto as _; let __sv_bah_path = ::stackable_versioned::jthong_path(parent, "bah"); - let __sv_bah_value = ::serde_yaml::to_value(&__sv_foospec.bah).unwrap(); + 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(), @@ -74,10 +74,8 @@ where } in upgrades { match json_path { json_path if json_path == __sv_bah_path => { - if downgraded_value.as_ref() - == ::core::option::Option::Some(&__sv_bah_value) - { - spec.bah = ::serde_yaml::from_value(value).unwrap(); + if downgraded_value.unwrap_or_default() == __sv_bah_value { + spec.bah = ::serde_json::from_value(value).unwrap(); } } _ => unreachable!(), @@ -99,7 +97,7 @@ where ) -> Self { use ::stackable_versioned::TrackingInto as _; let __sv_bah_path = ::stackable_versioned::jthong_path(parent, "bah"); - let __sv_bah_value = ::serde_yaml::to_value(&__sv_foospec.bah).unwrap(); + 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(), @@ -114,7 +112,7 @@ where json_path: __sv_bah_path, value: __sv_bah_value, downgraded_value: ::core::option::Option::Some( - ::serde_yaml::to_value(&spec.bah).unwrap(), + ::serde_json::to_value(&spec.bah).unwrap(), ), }); spec @@ -179,7 +177,7 @@ where 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_yaml::to_value(&__sv_foospec.bah).unwrap(); + 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(), @@ -193,14 +191,12 @@ where } in upgrades { match json_path { json_path if json_path == __sv_bar_path => { - if downgraded_value.as_ref() - == ::core::option::Option::Some(&__sv_bar_value) - { - spec.bar = ::serde_yaml::from_value(value).unwrap(); + 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_yaml::from_value(value).unwrap(); + spec.quux = ::serde_json::from_value(value).unwrap(); } _ => unreachable!(), } @@ -222,10 +218,10 @@ where upgrades .push(::stackable_versioned::ChangedValue { json_path: __sv_quux_path, - value: ::serde_yaml::to_value(&__sv_foospec.quux).unwrap(), + value: ::serde_json::to_value(&__sv_foospec.quux).unwrap(), downgraded_value: ::core::option::Option::None, }); - let __sv_bar_value = ::serde_yaml::to_value(&__sv_foospec.bar).unwrap(); + 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(), @@ -236,7 +232,7 @@ where json_path: __sv_bar_path, value: __sv_bar_value, downgraded_value: ::core::option::Option::Some( - ::serde_yaml::to_value(&spec.bah).unwrap(), + ::serde_json::to_value(&spec.bah).unwrap(), ), }); spec 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 f029642ef..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 @@ -50,7 +50,7 @@ where 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,7 @@ 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 { diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 2cfcaa713..89b3ab879 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -25,7 +25,9 @@ All notable changes to this project will be documented in this file. ### Fixed - Fix `From` impls of enum variants with multiple unnamed fields ([#YYYY]). -- Use the configured `serde_yaml` crate path when applying tracked values ([#ZZZZ]). +- 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 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 41af98eac..031b098c5 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -113,8 +113,11 @@ 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. @@ -124,7 +127,7 @@ pub struct ChangedValue { /// kept instead. #[serde(default, skip_serializing_if = "Option::is_none")] #[schemars(schema_with = "raw_object_schema")] - pub downgraded_value: Option, + 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 }]) ); } From 033abc201e6504b5639ba2f2c726e5e465af5a07 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 19 Oct 2026 10:47:10 +0200 Subject: [PATCH 5/5] fix(versioned): Only apply tracked values matching the container Multiple (nested) containers can track values for the same version. Each tracking impl previously removed the whole list of tracked values for that version and panicked on JSON paths it didn't know. This broke as soon as two containers tracked values in the same version, eg. the type change of `gender` and the added `Married::since` in v3 of the person test. Each container now only applies the values matching its JSON paths and keeps all others for the remaining containers. The entry for the version is only removed once all values have been applied. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../codegen/container/struct/conversion.rs | 33 +++++++++---------- ...napshots__pass@conversion_tracking.rs.snap | 20 +++++++++-- ...ots__pass@conversion_tracking_enum.rs.snap | 21 +++++++++--- ...ts__pass@conversion_tracking_hints.rs.snap | 21 +++++++++--- ...ss@conversion_tracking_type_change.rs.snap | 32 ++++++++++++++---- ...ioned_macros__snapshots__pass@docs.rs.snap | 20 +++++++++-- crates/stackable-versioned/CHANGELOG.md | 6 ++++ crates/stackable-versioned/tests/person.rs | 12 ++----- 8 files changed, 117 insertions(+), 48 deletions(-) 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 2f0657801..08c15914d 100644 --- a/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs +++ b/crates/stackable-versioned-macros/src/codegen/container/struct/conversion.rs @@ -324,29 +324,26 @@ 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! { .. } - }; - + // 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, #downgraded_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); + } } }) } 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 2cbb412b0..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 = ::serde_json::from_value(value).unwrap(); } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } } spec } 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 da04a5b48..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 = ::serde_json::from_value(value).unwrap(); } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1alpha2"); + } } spec } 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 4bf3ec1e1..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 = ::serde_json::from_value(value).unwrap(); } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1alpha2"); + } } spec } 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 index d3892f07a..636dbc91f 100644 --- 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 @@ -66,21 +66,31 @@ where bah: __sv_foospec.bah.into(), qux: __sv_foospec.qux.into(), }; - if let Some(upgrades) = status.changes().upgrades.remove(&"v1beta1".to_owned()) { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1beta1") { for ::stackable_versioned::ChangedValue { json_path, value, downgraded_value, - } in upgrades { + } 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(); } } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } } spec } @@ -183,12 +193,12 @@ where baz: __sv_foospec.qux.into(), quux: ::std::default::Default::default(), }; - if let Some(upgrades) = status.changes().upgrades.remove(&"v1".to_owned()) { + if let Some(upgrades) = status.changes().upgrades.get_mut("v1") { for ::stackable_versioned::ChangedValue { json_path, value, downgraded_value, - } in upgrades { + } 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 { @@ -198,9 +208,19 @@ where json_path if json_path == __sv_quux_path => { spec.quux = ::serde_json::from_value(value).unwrap(); } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1"); + } } spec } 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 dba719fd5..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 = ::serde_json::from_value(value).unwrap(); } - _ => unreachable!(), + json_path => { + upgrades + .push(::stackable_versioned::ChangedValue { + json_path, + value, + downgraded_value, + }) + } } } + if upgrades.is_empty() { + status.changes().upgrades.remove("v1beta1"); + } } spec } diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 12748c4f3..87c3fa8d3 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -16,6 +16,12 @@ All notable changes to this project will be documented in this file. 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 diff --git a/crates/stackable-versioned/tests/person.rs b/crates/stackable-versioned/tests/person.rs index cee000e80..f07e28234 100644 --- a/crates/stackable-versioned/tests/person.rs +++ b/crates/stackable-versioned/tests/person.rs @@ -100,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, }