From 31df5b14d4ddd6ca2c606425d74700c9382c97bc Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 4 Apr 2024 16:24:53 +0200 Subject: [PATCH 01/43] chore: Add skeleton code --- Cargo.toml | 1 + crates/stackable-versioned/Cargo.toml | 17 +++++++++++++++++ crates/stackable-versioned/src/gen/mod.rs | 6 ++++++ crates/stackable-versioned/src/lib.rs | 13 +++++++++++++ 4 files changed, 37 insertions(+) create mode 100644 crates/stackable-versioned/Cargo.toml create mode 100644 crates/stackable-versioned/src/gen/mod.rs create mode 100644 crates/stackable-versioned/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index c8b54676d..7fa346fbe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ chrono = { version = "0.4.37", default-features = false } clap = { version = "4.5.4", features = ["derive", "cargo", "env"] } const_format = "0.2.32" const-oid = "0.9.6" +convert_case = "0.6.0" darling = "0.20.8" delegate = "0.12.0" derivative = "2.2.0" diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml new file mode 100644 index 000000000..88b13d12a --- /dev/null +++ b/crates/stackable-versioned/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "stackable-versioned" +version = "0.1.0" +authors.workspace = true +license.workspace = true +edition.workspace = true +repository.workspace = true + +[lib] +proc-macro = true + +[dependencies] +convert_case.workspace = true +darling.workspace = true +proc-macro2.workspace = true +syn.workspace = true +quote.workspace = true diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs new file mode 100644 index 000000000..00a95a893 --- /dev/null +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -0,0 +1,6 @@ +use proc_macro2::TokenStream; +use syn::{DeriveInput, Result}; + +pub(crate) fn expand(_input: DeriveInput) -> Result { + todo!() +} diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs new file mode 100644 index 000000000..6385e04a7 --- /dev/null +++ b/crates/stackable-versioned/src/lib.rs @@ -0,0 +1,13 @@ +use proc_macro::TokenStream; +use syn::{DeriveInput, Error}; + +mod gen; + +#[proc_macro_derive(Versioned, attributes(versioned))] +pub fn versioned_macro_derive(input: TokenStream) -> TokenStream { + let input = syn::parse_macro_input!(input as DeriveInput); + + gen::expand(input) + .unwrap_or_else(Error::into_compile_error) + .into() +} From c93cdee0f17f0b1f661c474018047a60f8d11f3e Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 8 Apr 2024 15:56:10 +0200 Subject: [PATCH 02/43] feat: Add basic container and attribute validation --- .../src/attrs/container.rs | 68 +++++++++++++++++++ crates/stackable-versioned/src/attrs/mod.rs | 1 + crates/stackable-versioned/src/gen/mod.rs | 45 +++++++++++- crates/stackable-versioned/src/gen/version.rs | 12 ++++ crates/stackable-versioned/src/lib.rs | 1 + crates/stackable-versioned/tests/basic.rs | 10 +++ 6 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 crates/stackable-versioned/src/attrs/container.rs create mode 100644 crates/stackable-versioned/src/attrs/mod.rs create mode 100644 crates/stackable-versioned/src/gen/version.rs create mode 100644 crates/stackable-versioned/tests/basic.rs diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs new file mode 100644 index 000000000..5ed002275 --- /dev/null +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -0,0 +1,68 @@ +use darling::{ + util::{Flag, SpannedValue}, + Error, FromDeriveInput, FromMeta, +}; + +#[derive(Debug, FromDeriveInput)] +#[darling( + attributes(versioned), + supports(struct_named), + forward_attrs(allow, doc, cfg, serde), + and_then = ContainerAttributes::validate +)] +pub(crate) struct ContainerAttributes { + #[darling(multiple)] + pub(crate) version: SpannedValue>, +} + +impl ContainerAttributes { + fn validate(mut self) -> darling::Result { + if self.version.is_empty() { + return Err(Error::custom( + "attribute `#[versioned()]` must contain at least one `version`", + ) + .with_span(&self.version.span())); + } + + for version in &mut *self.version { + if version.name.is_empty() { + return Err(Error::custom("field `name` of `version` must not be empty") + .with_span(&version.name.span())); + } + + if !version + .name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-') + { + return Err(Error::custom( + "field `name` of `version` must only contain alphanumeric ASCII characters (a-z, A-Z, 0-9, '.', '-')", + ) + .with_span(&version.name.span())); + } + + // TODO (@Techassi): Use Diagnostics API when stablizized to throw + // a warning when the input mismatches the generated module name. + // See https://github.com/rust-lang/rust/issues/54140 + let module_name = version.name.to_lowercase(); + if module_name != *version.name { + println!("the generated module name differs from the provided version name which might cause confusion around what the code seems to suggest") + } + version.module_name = module_name + } + + Ok(self) + } +} + +#[derive(Debug, FromMeta)] +pub struct VersionAttributes { + pub(crate) name: SpannedValue, + + pub(crate) deprecated: Flag, + + #[darling(skip)] + pub(crate) module_name: String, + // #[darling(default = default_visibility)] + // pub(crate) visibility: Visibility, +} diff --git a/crates/stackable-versioned/src/attrs/mod.rs b/crates/stackable-versioned/src/attrs/mod.rs new file mode 100644 index 000000000..5ab42603f --- /dev/null +++ b/crates/stackable-versioned/src/attrs/mod.rs @@ -0,0 +1 @@ +pub(crate) mod container; diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index 00a95a893..790092b1e 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -1,6 +1,45 @@ +use darling::FromDeriveInput; use proc_macro2::TokenStream; -use syn::{DeriveInput, Result}; +use quote::quote; +use syn::{spanned::Spanned, Data, DataEnum, DataStruct, DeriveInput, Error, Ident, Result}; -pub(crate) fn expand(_input: DeriveInput) -> Result { - todo!() +use crate::attrs::container::ContainerAttributes; + +pub(crate) mod version; + +pub(crate) fn expand(input: DeriveInput) -> Result { + // Extract container attributes + let attributes = ContainerAttributes::from_derive_input(&input)?; + + // Validate container shape + let expanded = match input.data { + Data::Struct(data) => expand_struct(input.ident, data, attributes)?, + Data::Enum(data) => expand_enum(input.ident, data, attributes)?, + Data::Union(_) => { + return Err(Error::new( + input.span(), + "derive macro `Versioned` only supports structs and enums", + )) + } + }; + + Ok(quote! { + #expanded + }) +} + +pub(crate) fn expand_struct( + ident: Ident, + data: DataStruct, + attributes: ContainerAttributes, +) -> Result { + Ok(quote!()) +} + +pub(crate) fn expand_enum( + ident: Ident, + data: DataEnum, + attributes: ContainerAttributes, +) -> Result { + Ok(quote!()) } diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs new file mode 100644 index 000000000..f5139f8d5 --- /dev/null +++ b/crates/stackable-versioned/src/gen/version.rs @@ -0,0 +1,12 @@ +use syn::{Field, Ident}; + +pub(crate) struct Version { + struct_ident: Ident, + version: String, + + deprecated: Vec, + renamed: Vec, + added: Vec, + + fields: Vec, +} diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs index 6385e04a7..14892bebb 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -1,6 +1,7 @@ use proc_macro::TokenStream; use syn::{DeriveInput, Error}; +mod attrs; mod gen; #[proc_macro_derive(Versioned, attributes(versioned))] diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs new file mode 100644 index 000000000..73d170caf --- /dev/null +++ b/crates/stackable-versioned/tests/basic.rs @@ -0,0 +1,10 @@ +use stackable_versioned::Versioned; + +#[test] +fn basic() { + #[derive(Versioned)] + #[versioned(version(name = "1.2.3", deprecated))] + struct Foo { + bar: usize, + } +} From 109e38ff694cff82ef3103c6cda4d9f91589f107 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 8 Apr 2024 17:13:57 +0200 Subject: [PATCH 03/43] Start field attribute validation --- .../src/attrs/container.rs | 4 +- crates/stackable-versioned/src/attrs/field.rs | 72 +++++++++++++++++++ crates/stackable-versioned/src/attrs/mod.rs | 1 + crates/stackable-versioned/src/gen/mod.rs | 24 ++++--- crates/stackable-versioned/src/gen/version.rs | 2 + crates/stackable-versioned/tests/basic.rs | 2 + 6 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 crates/stackable-versioned/src/attrs/field.rs diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 5ed002275..2fb3eaf3d 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -59,7 +59,9 @@ impl ContainerAttributes { pub struct VersionAttributes { pub(crate) name: SpannedValue, - pub(crate) deprecated: Flag, + // TODO (@Techassi): Remove the rename when the field uses the correct name + #[darling(rename = "deprecated")] + pub(crate) _deprecated: Flag, #[darling(skip)] pub(crate) module_name: String, diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs new file mode 100644 index 000000000..00d3060ad --- /dev/null +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -0,0 +1,72 @@ +use darling::{util::SpannedValue, Error, FromField, FromMeta}; +use syn::Ident; + +#[derive(Debug, FromField)] +#[darling( + attributes(versioned), + forward_attrs(allow, doc, cfg, serde), + and_then = FieldAttributes::validate +)] +pub(crate) struct FieldAttributes { + added: Option>, + renamed: Option>, + deprecated: Option>, + + ident: Option, + + #[darling(skip)] + _action: FieldAction, +} + +impl FieldAttributes { + fn validate(self) -> darling::Result { + match (&self.added, &self.renamed, &self.deprecated) { + (Some(_), Some(_), Some(_)) => { + return Err(Error::custom( + "cannot specify fields `added`, `renamed`, and `deprecated` at the same time", + ) + .with_span(&self.ident.unwrap().span())) + } + (Some(_), Some(_), None) => { + return Err(Error::custom( + "cannot specify fields `added` and `renamed` at the same time", + ) + .with_span(&self.ident.unwrap().span())) + } + (Some(_), None, Some(_)) => { + return Err(Error::custom( + "cannot specify fields `added` and `deprecated` at the same time", + ) + .with_span(&self.ident.unwrap().span())) + } + _ => (), + } + + Ok(self) + } +} + +#[derive(Debug, FromMeta)] +pub(crate) struct AddedAttributes { + #[darling(rename = "since")] + _since: SpannedValue, +} + +#[derive(Debug, FromMeta)] +pub(crate) struct RenamedAttributes { + _since: SpannedValue, + _to: SpannedValue, +} + +#[derive(Debug, FromMeta)] +pub(crate) struct DeprecatedAttributes { + _since: SpannedValue, +} + +#[derive(Debug, Default)] +pub(crate) enum FieldAction { + #[default] + Added, + // Renamed, + // Deprecated, +} diff --git a/crates/stackable-versioned/src/attrs/mod.rs b/crates/stackable-versioned/src/attrs/mod.rs index 5ab42603f..1231db5d3 100644 --- a/crates/stackable-versioned/src/attrs/mod.rs +++ b/crates/stackable-versioned/src/attrs/mod.rs @@ -1 +1,2 @@ pub(crate) mod container; +pub(crate) mod field; diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index 790092b1e..b16048594 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -1,9 +1,9 @@ -use darling::FromDeriveInput; +use darling::{FromDeriveInput, FromField}; use proc_macro2::TokenStream; use quote::quote; use syn::{spanned::Spanned, Data, DataEnum, DataStruct, DeriveInput, Error, Ident, Result}; -use crate::attrs::container::ContainerAttributes; +use crate::attrs::{container::ContainerAttributes, field::FieldAttributes}; pub(crate) mod version; @@ -13,8 +13,8 @@ pub(crate) fn expand(input: DeriveInput) -> Result { // Validate container shape let expanded = match input.data { - Data::Struct(data) => expand_struct(input.ident, data, attributes)?, - Data::Enum(data) => expand_enum(input.ident, data, attributes)?, + Data::Struct(data) => expand_struct(&input.ident, data, attributes)?, + Data::Enum(data) => expand_enum(&input.ident, data, attributes)?, Data::Union(_) => { return Err(Error::new( input.span(), @@ -29,17 +29,23 @@ pub(crate) fn expand(input: DeriveInput) -> Result { } pub(crate) fn expand_struct( - ident: Ident, + _ident: &Ident, data: DataStruct, - attributes: ContainerAttributes, + _attributes: ContainerAttributes, ) -> Result { + // Loop over each specified version and collect fields added, renamed + // and deprecated for that version. + for field in data.fields { + let _field_aatributes = FieldAttributes::from_field(&field)?; + } + Ok(quote!()) } pub(crate) fn expand_enum( - ident: Ident, - data: DataEnum, - attributes: ContainerAttributes, + _ident: &Ident, + _data: DataEnum, + _attributes: ContainerAttributes, ) -> Result { Ok(quote!()) } diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs index f5139f8d5..dd5dd81d4 100644 --- a/crates/stackable-versioned/src/gen/version.rs +++ b/crates/stackable-versioned/src/gen/version.rs @@ -1,5 +1,7 @@ use syn::{Field, Ident}; +// TODO (@Techassi): Remove allow attribute +#[allow(dead_code)] pub(crate) struct Version { struct_ident: Ident, version: String, diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 73d170caf..abc61038c 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -3,8 +3,10 @@ use stackable_versioned::Versioned; #[test] fn basic() { #[derive(Versioned)] + #[allow(dead_code)] #[versioned(version(name = "1.2.3", deprecated))] struct Foo { + #[versioned(added(since = "1.2.3"))] bar: usize, } } From 40df3af62b3840ade7b03243894c7313dd788538 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 9 Apr 2024 17:09:11 +0200 Subject: [PATCH 04/43] Move code generation into structs --- .../src/attrs/container.rs | 24 +--- crates/stackable-versioned/src/attrs/field.rs | 107 +++++++++--------- crates/stackable-versioned/src/gen/field.rs | 27 +++++ crates/stackable-versioned/src/gen/mod.rs | 60 +++++----- crates/stackable-versioned/src/gen/venum.rs | 23 ++++ crates/stackable-versioned/src/gen/version.rs | 39 +++++-- crates/stackable-versioned/src/gen/vstruct.rs | 94 +++++++++++++++ crates/stackable-versioned/tests/basic.rs | 17 +-- 8 files changed, 275 insertions(+), 116 deletions(-) create mode 100644 crates/stackable-versioned/src/gen/field.rs create mode 100644 crates/stackable-versioned/src/gen/venum.rs create mode 100644 crates/stackable-versioned/src/gen/vstruct.rs diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 2fb3eaf3d..da88903c9 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -11,20 +11,20 @@ use darling::{ and_then = ContainerAttributes::validate )] pub(crate) struct ContainerAttributes { - #[darling(multiple)] - pub(crate) version: SpannedValue>, + #[darling(multiple, rename = "version")] + pub(crate) versions: SpannedValue>, } impl ContainerAttributes { fn validate(mut self) -> darling::Result { - if self.version.is_empty() { + if self.versions.is_empty() { return Err(Error::custom( "attribute `#[versioned()]` must contain at least one `version`", ) - .with_span(&self.version.span())); + .with_span(&self.versions.span())); } - for version in &mut *self.version { + for version in &mut *self.versions { if version.name.is_empty() { return Err(Error::custom("field `name` of `version` must not be empty") .with_span(&version.name.span())); @@ -40,15 +40,6 @@ impl ContainerAttributes { ) .with_span(&version.name.span())); } - - // TODO (@Techassi): Use Diagnostics API when stablizized to throw - // a warning when the input mismatches the generated module name. - // See https://github.com/rust-lang/rust/issues/54140 - let module_name = version.name.to_lowercase(); - if module_name != *version.name { - println!("the generated module name differs from the provided version name which might cause confusion around what the code seems to suggest") - } - version.module_name = module_name } Ok(self) @@ -62,9 +53,4 @@ pub struct VersionAttributes { // TODO (@Techassi): Remove the rename when the field uses the correct name #[darling(rename = "deprecated")] pub(crate) _deprecated: Flag, - - #[darling(skip)] - pub(crate) module_name: String, - // #[darling(default = default_visibility)] - // pub(crate) visibility: Visibility, } diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 00d3060ad..fda7e4b66 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,72 +1,77 @@ +use std::fmt; + use darling::{util::SpannedValue, Error, FromField, FromMeta}; -use syn::Ident; #[derive(Debug, FromField)] -#[darling( - attributes(versioned), - forward_attrs(allow, doc, cfg, serde), - and_then = FieldAttributes::validate -)] +#[darling(attributes(versioned), forward_attrs(allow, doc, cfg, serde))] pub(crate) struct FieldAttributes { - added: Option>, - renamed: Option>, - deprecated: Option>, - - ident: Option, - - #[darling(skip)] - _action: FieldAction, -} - -impl FieldAttributes { - fn validate(self) -> darling::Result { - match (&self.added, &self.renamed, &self.deprecated) { - (Some(_), Some(_), Some(_)) => { - return Err(Error::custom( - "cannot specify fields `added`, `renamed`, and `deprecated` at the same time", - ) - .with_span(&self.ident.unwrap().span())) - } - (Some(_), Some(_), None) => { - return Err(Error::custom( - "cannot specify fields `added` and `renamed` at the same time", - ) - .with_span(&self.ident.unwrap().span())) - } - (Some(_), None, Some(_)) => { - return Err(Error::custom( - "cannot specify fields `added` and `deprecated` at the same time", - ) - .with_span(&self.ident.unwrap().span())) - } - _ => (), - } - - Ok(self) - } + added: Option, + renamed: Option, + deprecated: Option, } #[derive(Debug, FromMeta)] pub(crate) struct AddedAttributes { - #[darling(rename = "since")] - _since: SpannedValue, + since: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct RenamedAttributes { - _since: SpannedValue, + since: SpannedValue, _to: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { - _since: SpannedValue, + since: SpannedValue, } -#[derive(Debug, Default)] +#[derive(Debug)] pub(crate) enum FieldAction { - #[default] - Added, - // Renamed, - // Deprecated, + Added(AddedAttributes), + Renamed(RenamedAttributes), + Deprecated(DeprecatedAttributes), + None, +} + +impl TryFrom for FieldAction { + type Error = Error; + + fn try_from(value: FieldAttributes) -> Result { + // NOTE (@Techassi): We sadly currently cannot use the attribute span + // when reporting errors. That's why the errors will be displayed at + // the #[derive(Versioned)] position. + + match (value.added, value.renamed, value.deprecated) { + (Some(added), None, None) => Ok(FieldAction::Added(added)), + (None, Some(renamed), None) => Ok(FieldAction::Renamed(renamed)), + (None, None, Some(deprecated)) => Ok(FieldAction::Deprecated(deprecated)), + (None, None, None) => Ok(FieldAction::None), + _ => Err(Error::custom( + "cannot specifiy multiple field actions at once", + )), + } + } +} + +impl fmt::Display for FieldAction { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + FieldAction::Added(_) => "added".fmt(f), + FieldAction::Renamed(_) => "renamed".fmt(f), + FieldAction::Deprecated(_) => "deprecated".fmt(f), + FieldAction::None => "".fmt(f), + } + } +} + +impl FieldAction { + pub(crate) fn since(&self) -> Option<&str> { + match self { + FieldAction::Added(added) => Some(&*added.since), + FieldAction::Renamed(renamed) => Some(&*renamed.since), + FieldAction::Deprecated(deprecated) => Some(&*deprecated.since), + FieldAction::None => None, + } + } } diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs new file mode 100644 index 000000000..145f4aa1c --- /dev/null +++ b/crates/stackable-versioned/src/gen/field.rs @@ -0,0 +1,27 @@ +use proc_macro2::TokenStream; +use quote::ToTokens; +use syn::Field; + +use crate::attrs::field::FieldAction; + +pub(crate) struct VersionedField { + // TODO (@Techassi): There can be multiple actions for one field (in + //different versions). Add support for that here. + pub(crate) _action: FieldAction, + pub(crate) _inner: Field, +} + +impl ToTokens for VersionedField { + fn to_tokens(&self, _tokens: &mut TokenStream) { + todo!() + } +} + +impl VersionedField { + pub(crate) fn new(field: Field, action: FieldAction) -> Self { + Self { + _inner: field, + _action: action, + } + } +} diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index b16048594..99363e1ad 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -1,20 +1,40 @@ -use darling::{FromDeriveInput, FromField}; +use darling::FromDeriveInput; use proc_macro2::TokenStream; -use quote::quote; -use syn::{spanned::Spanned, Data, DataEnum, DataStruct, DeriveInput, Error, Ident, Result}; +use quote::ToTokens; +use syn::{spanned::Spanned, Data, DeriveInput, Error, Result}; -use crate::attrs::{container::ContainerAttributes, field::FieldAttributes}; +use crate::{ + attrs::container::ContainerAttributes, + gen::{venum::VersionedEnum, vstruct::VersionedStruct}, +}; +pub(crate) mod field; +pub(crate) mod venum; pub(crate) mod version; +pub(crate) mod vstruct; + +// NOTE (@Techassi): This derive macro cannot handle multiple structs / enums +// to be versioned within the same file. This is because we cannot declare +// modules more than once (They will not be merged, like impl blocks for +// example). This leads to collisions if there are multiple structs / enums +// which declare the same version. This could maybe be solved by using an +// attribute macro applied to a module with all struct / enums declared in said +// module. This would allow us to generate all versioned structs and enums in +// a single sweep and put them into the appropriate module. + +// TODO (@Techassi): Think about how we can handle nested structs / enums which +// are also versioned. pub(crate) fn expand(input: DeriveInput) -> Result { // Extract container attributes let attributes = ContainerAttributes::from_derive_input(&input)?; - // Validate container shape + // Validate container shape and generate code let expanded = match input.data { - Data::Struct(data) => expand_struct(&input.ident, data, attributes)?, - Data::Enum(data) => expand_enum(&input.ident, data, attributes)?, + Data::Struct(data) => { + VersionedStruct::new(input.ident, data, attributes)?.to_token_stream() + } + Data::Enum(data) => VersionedEnum::new(input.ident, data, attributes)?.to_token_stream(), Data::Union(_) => { return Err(Error::new( input.span(), @@ -23,29 +43,5 @@ pub(crate) fn expand(input: DeriveInput) -> Result { } }; - Ok(quote! { - #expanded - }) -} - -pub(crate) fn expand_struct( - _ident: &Ident, - data: DataStruct, - _attributes: ContainerAttributes, -) -> Result { - // Loop over each specified version and collect fields added, renamed - // and deprecated for that version. - for field in data.fields { - let _field_aatributes = FieldAttributes::from_field(&field)?; - } - - Ok(quote!()) -} - -pub(crate) fn expand_enum( - _ident: &Ident, - _data: DataEnum, - _attributes: ContainerAttributes, -) -> Result { - Ok(quote!()) + Ok(expanded) } diff --git a/crates/stackable-versioned/src/gen/venum.rs b/crates/stackable-versioned/src/gen/venum.rs new file mode 100644 index 000000000..9297cbcf6 --- /dev/null +++ b/crates/stackable-versioned/src/gen/venum.rs @@ -0,0 +1,23 @@ +use proc_macro2::TokenStream; +use quote::ToTokens; +use syn::{DataEnum, Ident, Result}; + +use crate::attrs::container::ContainerAttributes; + +pub(crate) struct VersionedEnum {} + +impl VersionedEnum { + pub(crate) fn new( + _ident: Ident, + _data: DataEnum, + _attributes: ContainerAttributes, + ) -> Result { + todo!() + } +} + +impl ToTokens for VersionedEnum { + fn to_tokens(&self, _tokens: &mut TokenStream) { + todo!() + } +} diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs index dd5dd81d4..7ab21b4c4 100644 --- a/crates/stackable-versioned/src/gen/version.rs +++ b/crates/stackable-versioned/src/gen/version.rs @@ -1,14 +1,39 @@ -use syn::{Field, Ident}; +use proc_macro2::TokenStream; +use quote::{format_ident, quote, ToTokens}; +use syn::Ident; + +use crate::gen::field::VersionedField; // TODO (@Techassi): Remove allow attribute #[allow(dead_code)] pub(crate) struct Version { - struct_ident: Ident, - version: String, + pub(crate) fields: Vec, + pub(crate) container_ident: Ident, + pub(crate) module: Ident, + pub(crate) name: String, +} - deprecated: Vec, - renamed: Vec, - added: Vec, +impl ToTokens for Version { + fn to_tokens(&self, tokens: &mut TokenStream) { + let module_name = &self.module; + let container_ident = &self.container_ident; + + tokens.extend(quote! { + #[automatically_derived] + pub mod #module_name { + pub struct #container_ident {} + } + }) + } +} - fields: Vec, +impl Version { + pub(crate) fn new(container_ident: Ident, version: String) -> Self { + Self { + module: format_ident!("{}", version.to_lowercase()), + fields: Vec::new(), + container_ident, + name: version, + } + } } diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs new file mode 100644 index 000000000..90bdf3ca1 --- /dev/null +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -0,0 +1,94 @@ +use std::ops::Deref; + +use darling::FromField; +use proc_macro2::TokenStream; +use quote::quote; +use quote::ToTokens; +use syn::{spanned::Spanned, DataStruct, Error, Ident, Result}; + +use crate::{ + attrs::{ + container::ContainerAttributes, + field::{FieldAction, FieldAttributes}, + }, + gen::{field::VersionedField, version::Version}, +}; + +pub(crate) struct VersionedStruct { + /// Stores individual versions of a single struct. Each version tracks field + /// actions, which describe if the field was added, renamed or deprecated in + /// that version. Fields which are not versioned, are included in every + /// version of the struct. + versions: Vec, +} + +impl ToTokens for VersionedStruct { + fn to_tokens(&self, tokens: &mut TokenStream) { + // Iterate over each individual version to generate the version modules + // and the appropriate nested struct. + for (index, version) in self.versions.iter().enumerate() { + tokens.extend(version.to_token_stream()); + + // If the last version (currently assumes versions are declared in + // ascending order) is encountered, also generate the latest module + // which always points to the highest / latest version. + if index + 1 == self.versions.len() { + let latest_version_name = &version.module; + + tokens.extend(quote! { + #[automatically_derived] + pub mod latest { + pub use super::#latest_version_name::*; + } + }) + } + } + } +} + +impl VersionedStruct { + pub(crate) fn new( + ident: Ident, + data: DataStruct, + attributes: ContainerAttributes, + ) -> Result { + // First, collect all declared versions and validate that each version + // is unique and isn't declared multiple times. + let mut versions = attributes + .versions + .iter() + .map(|v| Version::new(ident.clone(), v.name.deref().clone())) + .collect::>(); + + for field in data.fields { + // Iterate over all fields of the struct and gather field attributes. + // Next, make sure only valid combinations of field actions are + // declared. Using the action and the field data, a VersionField + // can be created. + let field_attributes = FieldAttributes::from_field(&field)?; + let field_action = FieldAction::try_from(field_attributes)?; + + // Validate, that the field action uses a version which is declared + // by the container attribute. + if let Some(version) = versions + .iter_mut() + .find(|v| v.name == field_action.since().unwrap_or_default()) + { + version + .fields + .push(VersionedField::new(field, field_action)); + + continue; + } + + // At this point the version specified in the action is not in the + // set of declared versions and thus an error is returned. + return Err(Error::new( + field.span(), + format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), + )); + } + + Ok(Self { versions }) + } +} diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index abc61038c..297b746f7 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -1,12 +1,15 @@ use stackable_versioned::Versioned; +#[derive(Versioned)] +#[allow(dead_code)] +#[versioned(version(name = "v1alpha1", deprecated))] +struct Foo { + #[versioned(added(since = "v1alpha1"))] + bar: usize, +} + #[test] fn basic() { - #[derive(Versioned)] - #[allow(dead_code)] - #[versioned(version(name = "1.2.3", deprecated))] - struct Foo { - #[versioned(added(since = "1.2.3"))] - bar: usize, - } + // let _ = v1alpha1::Foo {}; + // let _ = latest::Foo {}; } From cff3fe91cb904674032b4ff568c3dc1701f3d2e6 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 16 Apr 2024 15:53:42 +0200 Subject: [PATCH 05/43] Adjust field actions which require generation in multiple versions --- .../src/attrs/container.rs | 32 ++++- crates/stackable-versioned/src/attrs/field.rs | 19 ++- crates/stackable-versioned/src/gen/field.rs | 53 ++++++-- crates/stackable-versioned/src/gen/mod.rs | 1 - crates/stackable-versioned/src/gen/version.rs | 39 ------ crates/stackable-versioned/src/gen/vstruct.rs | 119 ++++++++++-------- crates/stackable-versioned/tests/basic.rs | 6 +- 7 files changed, 158 insertions(+), 111 deletions(-) delete mode 100644 crates/stackable-versioned/src/gen/version.rs diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index da88903c9..3db1b5543 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -1,9 +1,11 @@ +use std::{collections::HashSet, ops::Deref}; + use darling::{ util::{Flag, SpannedValue}, Error, FromDeriveInput, FromMeta, }; -#[derive(Debug, FromDeriveInput)] +#[derive(Clone, Debug, FromDeriveInput)] #[darling( attributes(versioned), supports(struct_named), @@ -17,6 +19,8 @@ pub(crate) struct ContainerAttributes { impl ContainerAttributes { fn validate(mut self) -> darling::Result { + // If there are no versions defined, the derive macro errors out. There + // should be at least one version if the derive macro is used. if self.versions.is_empty() { return Err(Error::custom( "attribute `#[versioned()]` must contain at least one `version`", @@ -25,11 +29,16 @@ impl ContainerAttributes { } for version in &mut *self.versions { + // Ensure that the version name is not empty, because we cannot use + // an empty name as the module name. if version.name.is_empty() { return Err(Error::custom("field `name` of `version` must not be empty") .with_span(&version.name.span())); } + // Ensure that the version name contains only a selection of valid + // characters, which can also be used as module identifiers (after + // minor replacements). if !version .name .chars() @@ -42,15 +51,26 @@ impl ContainerAttributes { } } + // Ensure every version is unique and isn't declared multiple times. This + // is inspired by the itertools all_unique function. + let mut unique = HashSet::new(); + if !self + .versions + .iter() + .all(move |elem| unique.insert(elem.name.deref())) + { + return Err(Error::custom( + "attribute `#[versioned()]` contains one or more `version`s with a duplicate `name`", + ) + .with_span(&self.versions.span())); + } + Ok(self) } } -#[derive(Debug, FromMeta)] +#[derive(Clone, Debug, FromMeta)] pub struct VersionAttributes { pub(crate) name: SpannedValue, - - // TODO (@Techassi): Remove the rename when the field uses the correct name - #[darling(rename = "deprecated")] - pub(crate) _deprecated: Flag, + pub(crate) deprecated: Flag, } diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index fda7e4b66..9bbdad426 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -18,12 +18,13 @@ pub(crate) struct AddedAttributes { #[derive(Debug, FromMeta)] pub(crate) struct RenamedAttributes { since: SpannedValue, - _to: SpannedValue, + pub(crate) to: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { - since: SpannedValue, + pub(crate) since: SpannedValue, + pub(crate) note: SpannedValue, } #[derive(Debug)] @@ -34,6 +35,20 @@ pub(crate) enum FieldAction { None, } +impl PartialEq for FieldAction { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Self::Added(lhs), Self::Added(rhs)) => *lhs.since == *rhs.since, + (Self::Renamed(lhs), Self::Renamed(rhs)) => { + *lhs.since == *rhs.since && *lhs.to == *rhs.to + } + (Self::Deprecated(lhs), Self::Deprecated(rhs)) => *lhs.since == *rhs.since, + (Self::None, Self::None) => true, + _ => false, + } + } +} + impl TryFrom for FieldAction { type Error = Error; diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index 145f4aa1c..ea010e518 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,27 +1,62 @@ use proc_macro2::TokenStream; -use quote::ToTokens; +use quote::{format_ident, quote, ToTokens}; use syn::Field; use crate::attrs::field::FieldAction; pub(crate) struct VersionedField { // TODO (@Techassi): There can be multiple actions for one field (in - //different versions). Add support for that here. - pub(crate) _action: FieldAction, - pub(crate) _inner: Field, + // different versions). Add support for that here. + pub(crate) action: FieldAction, + pub(crate) inner: Field, } impl ToTokens for VersionedField { - fn to_tokens(&self, _tokens: &mut TokenStream) { - todo!() + fn to_tokens(&self, tokens: &mut TokenStream) { + match &self.action { + FieldAction::Renamed(renamed) => { + let field_name = format_ident!("{}", *renamed.to); + let field_type = &self.inner.ty; + + tokens.extend(quote! { + pub #field_name: #field_type, + }) + } + FieldAction::Deprecated(deprecated) => { + // TODO (@Techassi): Is it save to unwrap here? + let field_name = format_ident!( + "deprecated_{}", + &self.inner.ident.as_ref().expect("field must have a name") + ); + let field_type = &self.inner.ty; + + let deprecation_note = format!( + "{} (was deprecated in {})", + &*deprecated.note, &*deprecated.since + ); + + tokens.extend(quote! { + #[deprecated = #deprecation_note] + pub #field_name: #field_type, + }) + } + FieldAction::Added(_) | FieldAction::None => { + let field_name = &self.inner.ident; + let field_type = &self.inner.ty; + + tokens.extend(quote! { + pub #field_name: #field_type, + }) + } + } } } impl VersionedField { - pub(crate) fn new(field: Field, action: FieldAction) -> Self { + pub(crate) fn _new(field: Field, action: FieldAction) -> Self { Self { - _inner: field, - _action: action, + inner: field, + action, } } } diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index 99363e1ad..fd9302777 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -10,7 +10,6 @@ use crate::{ pub(crate) mod field; pub(crate) mod venum; -pub(crate) mod version; pub(crate) mod vstruct; // NOTE (@Techassi): This derive macro cannot handle multiple structs / enums diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs deleted file mode 100644 index 7ab21b4c4..000000000 --- a/crates/stackable-versioned/src/gen/version.rs +++ /dev/null @@ -1,39 +0,0 @@ -use proc_macro2::TokenStream; -use quote::{format_ident, quote, ToTokens}; -use syn::Ident; - -use crate::gen::field::VersionedField; - -// TODO (@Techassi): Remove allow attribute -#[allow(dead_code)] -pub(crate) struct Version { - pub(crate) fields: Vec, - pub(crate) container_ident: Ident, - pub(crate) module: Ident, - pub(crate) name: String, -} - -impl ToTokens for Version { - fn to_tokens(&self, tokens: &mut TokenStream) { - let module_name = &self.module; - let container_ident = &self.container_ident; - - tokens.extend(quote! { - #[automatically_derived] - pub mod #module_name { - pub struct #container_ident {} - } - }) - } -} - -impl Version { - pub(crate) fn new(container_ident: Ident, version: String) -> Self { - Self { - module: format_ident!("{}", version.to_lowercase()), - fields: Vec::new(), - container_ident, - name: version, - } - } -} diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 90bdf3ca1..6fc80928e 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -2,47 +2,53 @@ use std::ops::Deref; use darling::FromField; use proc_macro2::TokenStream; -use quote::quote; use quote::ToTokens; use syn::{spanned::Spanned, DataStruct, Error, Ident, Result}; -use crate::{ - attrs::{ - container::ContainerAttributes, - field::{FieldAction, FieldAttributes}, - }, - gen::{field::VersionedField, version::Version}, +use crate::attrs::{ + container::ContainerAttributes, + field::{FieldAction, FieldAttributes}, }; +pub(crate) struct Version { + name: String, + _deprecated: bool, +} + +/// Stores individual versions of a single struct. Each version tracks field +/// actions, which describe if the field was added, renamed or deprecated in +/// that version. Fields which are not versioned, are included in every +/// version of the struct. pub(crate) struct VersionedStruct { - /// Stores individual versions of a single struct. Each version tracks field - /// actions, which describe if the field was added, renamed or deprecated in - /// that version. Fields which are not versioned, are included in every - /// version of the struct. - versions: Vec, + pub(crate) _ident: Ident, + + pub(crate) _actions: Vec, + pub(crate) _versions: Vec, } impl ToTokens for VersionedStruct { - fn to_tokens(&self, tokens: &mut TokenStream) { + fn to_tokens(&self, _tokens: &mut TokenStream) { // Iterate over each individual version to generate the version modules // and the appropriate nested struct. - for (index, version) in self.versions.iter().enumerate() { - tokens.extend(version.to_token_stream()); - // If the last version (currently assumes versions are declared in - // ascending order) is encountered, also generate the latest module - // which always points to the highest / latest version. - if index + 1 == self.versions.len() { - let latest_version_name = &version.module; + // for (index, version) in self.versions.iter().enumerate() { + // tokens.extend(version.to_token_stream()); - tokens.extend(quote! { - #[automatically_derived] - pub mod latest { - pub use super::#latest_version_name::*; - } - }) - } - } + // // If the last version (currently assumes versions are declared in + // // ascending order) is encountered, also generate the latest module + // // which always points to the highest / latest version. + // if index + 1 == self.versions.len() { + // let latest_version_name = &version.module; + + // tokens.extend(quote! { + // #[automatically_derived] + // pub mod latest { + // pub use super::#latest_version_name::*; + // } + // }) + // } + // } + todo!() } } @@ -52,43 +58,54 @@ impl VersionedStruct { data: DataStruct, attributes: ContainerAttributes, ) -> Result { - // First, collect all declared versions and validate that each version - // is unique and isn't declared multiple times. - let mut versions = attributes + // First, collect all declared versions and map them into a Version + // struct. + let versions = attributes .versions .iter() - .map(|v| Version::new(ident.clone(), v.name.deref().clone())) + .cloned() + .map(|v| Version { + name: v.name.deref().clone(), + _deprecated: v.deprecated.is_present(), + }) .collect::>(); - for field in data.fields { + let mut actions = Vec::new(); + + for field in &data.fields { // Iterate over all fields of the struct and gather field attributes. // Next, make sure only valid combinations of field actions are // declared. Using the action and the field data, a VersionField // can be created. - let field_attributes = FieldAttributes::from_field(&field)?; + let field_attributes = FieldAttributes::from_field(field)?; let field_action = FieldAction::try_from(field_attributes)?; // Validate, that the field action uses a version which is declared - // by the container attribute. - if let Some(version) = versions - .iter_mut() - .find(|v| v.name == field_action.since().unwrap_or_default()) - { - version - .fields - .push(VersionedField::new(field, field_action)); + // by the container attribute. If there is no attribute attached to + // the field, it is also valid. + match field_action.since() { + Some(since) => { + if versions.iter().any(|v| v.name == since) { + actions.push(field_action); + continue; + } - continue; + // At this point the version specified in the action is not + // in the set of declared versions and thus an error is + // returned. + return Err(Error::new( + field.span(), + format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), + )); + } + None => continue, } - - // At this point the version specified in the action is not in the - // set of declared versions and thus an error is returned. - return Err(Error::new( - field.span(), - format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), - )); } - Ok(Self { versions }) + Ok(Self { + _versions: versions, + _actions: actions, + _ident: ident, + }) } } diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 297b746f7..955374fa7 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -2,14 +2,14 @@ use stackable_versioned::Versioned; #[derive(Versioned)] #[allow(dead_code)] -#[versioned(version(name = "v1alpha1", deprecated))] +#[versioned(version(name = "v1alpha1"), version(name = "v1beta1"))] struct Foo { - #[versioned(added(since = "v1alpha1"))] + #[versioned(deprecated(since = "v1beta1", note = "was moved to some other field"))] bar: usize, + baz: bool, } #[test] fn basic() { // let _ = v1alpha1::Foo {}; - // let _ = latest::Foo {}; } From aaaedaa27c559ef1329cdb43a510f84fede6493d Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 16 Apr 2024 17:19:44 +0200 Subject: [PATCH 06/43] Add basic support for added and always present fields --- crates/stackable-versioned/src/attrs/field.rs | 2 +- crates/stackable-versioned/src/gen/field.rs | 2 +- crates/stackable-versioned/src/gen/vstruct.rs | 136 +++++++++++++----- crates/stackable-versioned/tests/basic.rs | 5 +- 4 files changed, 103 insertions(+), 42 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 9bbdad426..2d869cb94 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -12,7 +12,7 @@ pub(crate) struct FieldAttributes { #[derive(Debug, FromMeta)] pub(crate) struct AddedAttributes { - since: SpannedValue, + pub(crate) since: SpannedValue, } #[derive(Debug, FromMeta)] diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index ea010e518..f585c23d8 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -53,7 +53,7 @@ impl ToTokens for VersionedField { } impl VersionedField { - pub(crate) fn _new(field: Field, action: FieldAction) -> Self { + pub(crate) fn new(field: Field, action: FieldAction) -> Self { Self { inner: field, action, diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 6fc80928e..dd29b53ad 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -2,16 +2,20 @@ use std::ops::Deref; use darling::FromField; use proc_macro2::TokenStream; -use quote::ToTokens; -use syn::{spanned::Spanned, DataStruct, Error, Ident, Result}; +use quote::{format_ident, quote, ToTokens}; +use syn::{spanned::Spanned, Attribute, DataStruct, Error, Ident, Result}; -use crate::attrs::{ - container::ContainerAttributes, - field::{FieldAction, FieldAttributes}, +use crate::{ + attrs::{ + container::ContainerAttributes, + field::{FieldAction, FieldAttributes}, + }, + gen::field::VersionedField, }; pub(crate) struct Version { - name: String, + original_name: String, + module_name: String, _deprecated: bool, } @@ -22,33 +26,82 @@ pub(crate) struct Version { pub(crate) struct VersionedStruct { pub(crate) _ident: Ident, - pub(crate) _actions: Vec, + pub(crate) _fields: Vec, pub(crate) _versions: Vec, } impl ToTokens for VersionedStruct { fn to_tokens(&self, _tokens: &mut TokenStream) { - // Iterate over each individual version to generate the version modules - // and the appropriate nested struct. - - // for (index, version) in self.versions.iter().enumerate() { - // tokens.extend(version.to_token_stream()); - - // // If the last version (currently assumes versions are declared in - // // ascending order) is encountered, also generate the latest module - // // which always points to the highest / latest version. - // if index + 1 == self.versions.len() { - // let latest_version_name = &version.module; - - // tokens.extend(quote! { - // #[automatically_derived] - // pub mod latest { - // pub use super::#latest_version_name::*; - // } - // }) - // } - // } - todo!() + let mut versions = self._versions.iter().peekable(); + + while let Some(version) = versions.next() { + let mut fields = TokenStream::new(); + + for field in &self._fields { + match &field.action { + FieldAction::Added(added) => { + // Skip generating the field, if the current generated + // version doesn't match the since field of the action. + if version.original_name != *added.since { + continue; + } + + let field_name = &field.inner.ident; + let field_type = &field.inner.ty; + let doc = format!(" Added since `{}`.", *added.since); + + let doc_attrs: Vec<&Attribute> = field + .inner + .attrs + .iter() + .filter(|a| a.path().is_ident("doc")) + .collect(); + + fields.extend(quote! { + #(#doc_attrs)* + #[doc = ""] + #[doc = #doc] + pub #field_name: #field_type, + }) + } + FieldAction::Renamed(_) => todo!(), + FieldAction::Deprecated(_) => todo!(), + FieldAction::None => { + let field_name = &field.inner.ident; + let field_type = &field.inner.ty; + + fields.extend(quote! { + pub #field_name: #field_type, + }) + } + } + } + + // TODO (@Techassi): Make the generation of the module optional to + // enable the attribute macro to be applied to a module which + // generates versioned versions of all contained containers. + + let module_name = format_ident!("{}", version.module_name); + let struct_name = &self._ident; + + _tokens.extend(quote! { + pub mod #module_name { + pub struct #struct_name { + #fields + } + } + }); + + // If there is no next version, we know we just generated the latest + // version and thus we can add the 'latest' module. + if versions.peek().is_none() { + _tokens.extend(quote! { + pub mod latest { + pub use super::#module_name::*; + } + }) + } + } } } @@ -64,20 +117,27 @@ impl VersionedStruct { .versions .iter() .cloned() - .map(|v| Version { - name: v.name.deref().clone(), - _deprecated: v.deprecated.is_present(), + .map(|v| { + let original_name = v.name.deref().clone(); + let module_name = original_name.replace('.', ""); + let deprecated = v.deprecated.is_present(); + + Version { + original_name, + module_name, + _deprecated: deprecated, + } }) .collect::>(); - let mut actions = Vec::new(); + let mut fields = Vec::new(); - for field in &data.fields { + for field in data.fields { // Iterate over all fields of the struct and gather field attributes. // Next, make sure only valid combinations of field actions are // declared. Using the action and the field data, a VersionField // can be created. - let field_attributes = FieldAttributes::from_field(field)?; + let field_attributes = FieldAttributes::from_field(&field)?; let field_action = FieldAction::try_from(field_attributes)?; // Validate, that the field action uses a version which is declared @@ -85,8 +145,8 @@ impl VersionedStruct { // the field, it is also valid. match field_action.since() { Some(since) => { - if versions.iter().any(|v| v.name == since) { - actions.push(field_action); + if versions.iter().any(|v| v.original_name == since) { + fields.push(VersionedField::new(field, field_action)); continue; } @@ -98,13 +158,13 @@ impl VersionedStruct { format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), )); } - None => continue, + None => fields.push(VersionedField::new(field, field_action)), } } Ok(Self { _versions: versions, - _actions: actions, + _fields: fields, _ident: ident, }) } diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 955374fa7..0b61c3a33 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -4,12 +4,13 @@ use stackable_versioned::Versioned; #[allow(dead_code)] #[versioned(version(name = "v1alpha1"), version(name = "v1beta1"))] struct Foo { - #[versioned(deprecated(since = "v1beta1", note = "was moved to some other field"))] + /// My docs + #[versioned(added(since = "v1beta1"))] bar: usize, baz: bool, } #[test] fn basic() { - // let _ = v1alpha1::Foo {}; + let _foo = v1beta1::Foo { bar: 0, baz: true }; } From be20ad8c42a9fdff60f73b66d5629fb2a0f07bed Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 11:30:28 +0200 Subject: [PATCH 07/43] feat(k8s-version): Add Kubernetes version crate This crate enables parsing of Kubernetes API versions from strings into well-defined structs with support for ordering and equality checking. --- crates/k8s-version/Cargo.toml | 15 +++ crates/k8s-version/src/api_version.rs | 67 +++++++++++ crates/k8s-version/src/level.rs | 157 ++++++++++++++++++++++++++ crates/k8s-version/src/lib.rs | 7 ++ crates/k8s-version/src/version.rs | 132 ++++++++++++++++++++++ 5 files changed, 378 insertions(+) create mode 100644 crates/k8s-version/Cargo.toml create mode 100644 crates/k8s-version/src/api_version.rs create mode 100644 crates/k8s-version/src/level.rs create mode 100644 crates/k8s-version/src/lib.rs create mode 100644 crates/k8s-version/src/version.rs diff --git a/crates/k8s-version/Cargo.toml b/crates/k8s-version/Cargo.toml new file mode 100644 index 000000000..39e460312 --- /dev/null +++ b/crates/k8s-version/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "k8s-version" +version = "0.1.0" +authors.workspace = true +license.workspace = true +edition.workspace = true +repository.workspace = true + +[dependencies] +lazy_static.workspace = true +regex.workspace = true +snafu.workspace = true + +[dev-dependencies] +rstest.workspace = true diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs new file mode 100644 index 000000000..a5acbc01f --- /dev/null +++ b/crates/k8s-version/src/api_version.rs @@ -0,0 +1,67 @@ +use std::{cmp::Ordering, fmt::Display, str::FromStr}; + +use snafu::{ResultExt, Snafu}; + +use crate::{Version, VersionParseError}; + +#[derive(Debug, PartialEq, Snafu)] +pub enum ApiVersionParseError { + #[snafu(display("failed to parse version"))] + ParseVersion { source: VersionParseError }, +} + +/// A Kubernetes API version with the `(/)` format, for example +/// `certificates.k8s.io/v1beta1`, `extensions/v1beta1` or `v1`. +/// +/// The `` string must follow the DNS label format defined [here][1]. +/// The `` string must be lower case and must be a valid DNS subdomain. +/// +/// ### See +/// +/// - +/// - +/// - +/// +/// [1]: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/identifiers.md#definitions +#[derive(Clone, Debug, Hash, PartialEq, Eq)] +pub struct ApiVersion { + pub group: Option, + pub version: Version, +} + +impl FromStr for ApiVersion { + type Err = ApiVersionParseError; + + fn from_str(input: &str) -> Result { + let (group, version) = if let Some((group, version)) = input.split_once('/') { + // TODO (Techassi): Validate group + ( + Some(group.to_string()), + Version::from_str(version).context(ParseVersionSnafu)?, + ) + } else { + (None, Version::from_str(input).context(ParseVersionSnafu)?) + }; + + Ok(Self { group, version }) + } +} + +impl PartialOrd for ApiVersion { + fn partial_cmp(&self, other: &Self) -> Option { + match self.group.partial_cmp(&other.group) { + Some(Ordering::Equal) => {} + _ => return None, + } + self.version.partial_cmp(&other.version) + } +} + +impl Display for ApiVersion { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.group { + Some(group) => write!(f, "{}/{}", group, self.version), + None => write!(f, "{}", self.version), + } + } +} diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs new file mode 100644 index 000000000..63dd0b81f --- /dev/null +++ b/crates/k8s-version/src/level.rs @@ -0,0 +1,157 @@ +use std::{ + cmp::Ordering, + fmt::Display, + num::ParseIntError, + ops::{Add, AddAssign, Sub, SubAssign}, + str::FromStr, +}; + +use lazy_static::lazy_static; +use regex::Regex; +use snafu::{OptionExt, ResultExt, Snafu}; + +lazy_static! { + static ref LEVEL_REGEX: Regex = + Regex::new(r"^(?P[a-z]+)(?P\d+)$").unwrap(); +} + +#[derive(Debug, PartialEq, Snafu)] +pub enum ParseLevelError { + #[snafu(display("invalid level format, expected beta/alpha"))] + InvalidFormat, + + #[snafu(display("failed to parse level version"))] + ParseVersion { source: ParseIntError }, + + #[snafu(display("unknown level identifier"))] + UnknownIdentifier, +} + +/// A minor Kubernetes resource version with the `beta/alpha` format. +#[derive(Clone, Debug, Hash, PartialEq, Eq)] +pub enum Level { + Beta(u64), + Alpha(u64), +} + +impl FromStr for Level { + type Err = ParseLevelError; + + fn from_str(input: &str) -> Result { + let captures = LEVEL_REGEX.captures(input).context(InvalidFormatSnafu)?; + + let identifier = captures + .name("identifier") + .expect("internal error: check that the correct match label is specified") + .as_str(); + + let version = captures + .name("version") + .expect("internal error: check that the correct match label is specified") + .as_str() + .parse::() + .context(ParseVersionSnafu)?; + + match identifier { + "alpha" => Ok(Self::Alpha(version)), + "beta" => Ok(Self::Beta(version)), + _ => UnknownIdentifierSnafu.fail(), + } + } +} + +impl PartialOrd for Level { + fn partial_cmp(&self, other: &Self) -> Option { + match self { + Level::Beta(sb) => match other { + Level::Beta(ob) => sb.partial_cmp(ob), + Level::Alpha(_) => Some(Ordering::Greater), + }, + Level::Alpha(sa) => match other { + Level::Beta(_) => Some(Ordering::Less), + Level::Alpha(oa) => sa.partial_cmp(oa), + }, + } + } +} + +impl Add for Level +where + T: Into, +{ + type Output = Level; + + fn add(self, rhs: T) -> Self::Output { + match self { + Level::Beta(b) => Level::Beta(b + rhs.into()), + Level::Alpha(a) => Level::Alpha(a + rhs.into()), + } + } +} + +impl AddAssign for Level +where + T: Into, +{ + fn add_assign(&mut self, rhs: T) { + match self { + Level::Beta(b) => *b + rhs.into(), + Level::Alpha(a) => *a + rhs.into(), + }; + } +} + +impl Sub for Level +where + T: Into, +{ + type Output = Level; + + fn sub(self, rhs: T) -> Self::Output { + match self { + Level::Beta(b) => Level::Beta(b - rhs.into()), + Level::Alpha(a) => Level::Alpha(a - rhs.into()), + } + } +} + +impl SubAssign for Level +where + T: Into, +{ + fn sub_assign(&mut self, rhs: T) { + match self { + Level::Beta(b) => *b - rhs.into(), + Level::Alpha(a) => *a - rhs.into(), + }; + } +} + +impl Display for Level { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Level::Beta(beta) => write!(f, "beta{}", beta), + Level::Alpha(alpha) => write!(f, "alpha{}", alpha), + } + } +} + +#[cfg(test)] +mod test { + use rstest::rstest; + + use super::*; + + #[rstest] + #[case(Level::Beta(1), Level::Alpha(1), Ordering::Greater)] + #[case(Level::Alpha(1), Level::Beta(1), Ordering::Less)] + #[case(Level::Alpha(2), Level::Alpha(1), Ordering::Greater)] + #[case(Level::Alpha(2), Level::Alpha(2), Ordering::Equal)] + #[case(Level::Alpha(1), Level::Alpha(2), Ordering::Less)] + #[case(Level::Beta(2), Level::Beta(1), Ordering::Greater)] + #[case(Level::Beta(2), Level::Beta(2), Ordering::Equal)] + #[case(Level::Beta(1), Level::Beta(2), Ordering::Less)] + fn partial_ord_level(#[case] input: Level, #[case] other: Level, #[case] expected: Ordering) { + assert_eq!(input.partial_cmp(&other), Some(expected)) + } +} diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs new file mode 100644 index 000000000..5cc5a9f28 --- /dev/null +++ b/crates/k8s-version/src/lib.rs @@ -0,0 +1,7 @@ +mod api_version; +mod level; +mod version; + +pub use api_version::*; +pub use level::*; +pub use version::*; diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs new file mode 100644 index 000000000..7b6da583c --- /dev/null +++ b/crates/k8s-version/src/version.rs @@ -0,0 +1,132 @@ +use std::{cmp::Ordering, fmt::Display, num::ParseIntError, str::FromStr}; + +use lazy_static::lazy_static; +use regex::Regex; +use snafu::{OptionExt, ResultExt, Snafu}; + +use crate::{Level, ParseLevelError}; + +lazy_static! { + static ref VERSION_REGEX: Regex = + Regex::new(r"^v(?P\d+)(?P[a-z0-9][a-z0-9-]{0,60}[a-z0-9])?$").unwrap(); +} + +#[derive(Debug, PartialEq, Snafu)] +pub enum VersionParseError { + #[snafu(display("invalid version format. Input is empty, contains non-ASCII characters or contains more than 63 characters"))] + InvalidFormat, + + #[snafu(display("failed to parse major version"))] + ParseMajorVersion { source: ParseIntError }, + + #[snafu(display("failed to parse version level"))] + ParseLevel { source: ParseLevelError }, +} + +/// A Kubernetes resource version with the `v(beta/alpha)` +/// format, for example `v1`, `v2beta1` or `v1alpha2`. +/// +/// The version must follow the DNS label format defined [here][1]. +/// +/// ### See +/// +/// - +/// - +/// +/// [1]: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/identifiers.md#definitions +#[derive(Clone, Debug, Hash, PartialEq, Eq)] +pub struct Version { + pub major: u64, + pub level: Option, +} + +impl FromStr for Version { + type Err = VersionParseError; + + fn from_str(input: &str) -> Result { + let captures = VERSION_REGEX.captures(input).context(InvalidFormatSnafu)?; + + let major = captures + .name("major") + .expect("internal error: check that the correct match label is specified") + .as_str() + .parse::() + .context(ParseMajorVersionSnafu)?; + + let level = captures + .name("level") + .expect("internal error: check that the correct match label is specified") + .as_str(); + + if level.is_empty() { + return Ok(Self { major, level: None }); + } + + let level = Level::from_str(level).context(ParseLevelSnafu)?; + + Ok(Self { + level: Some(level), + major, + }) + } +} + +impl PartialOrd for Version { + fn partial_cmp(&self, other: &Self) -> Option { + match self.major.partial_cmp(&other.major) { + Some(core::cmp::Ordering::Equal) => {} + ord => return ord, + } + + match (&self.level, &other.level) { + (Some(lhs), Some(rhs)) => lhs.partial_cmp(rhs), + (Some(_), None) => Some(Ordering::Less), + (None, Some(_)) => Some(Ordering::Greater), + (None, None) => Some(Ordering::Equal), + } + } +} + +impl Display for Version { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.level { + Some(minor) => write!(f, "v{}{}", self.major, minor), + None => write!(f, "v{}", self.major), + } + } +} +impl Version { + pub fn new(major: u64, minor: Option) -> Self { + Self { + major, + level: minor, + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use rstest::rstest; + + #[rstest] + #[case("v1alpha12")] + #[case("v1alpha1")] + #[case("v1beta1")] + #[case("v1")] + fn valid_version(#[case] input: &str) { + let version = Version::from_str(input).unwrap(); + assert_eq!(version.to_string(), input); + } + + // #[rstest] + // #[case("v1gamma12", VersionParseError::ParseLevel { source: ParseLevelError::InvalidLevel })] + // #[case("v1betä1", VersionParseError::InvalidFormat)] + // #[case("1beta1", VersionParseError::InvalidStart)] + // #[case("", VersionParseError::InvalidFormat)] + // #[case("v0", VersionParseError::LeadingZero)] + // fn invalid_version(#[case] input: &str, #[case] error: VersionParseError) { + // let err = Version::from_str(input).unwrap_err(); + // assert_eq!(err, error) + // } +} From f46d3704560fe9d91e90d726c2fe799b37e3214f Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 11:32:21 +0200 Subject: [PATCH 08/43] feat(k8s-version): Add support for FromMeta This adds support for darling's FromMeta trait, which enables implementors to be constructed from macro attributes. --- crates/k8s-version/Cargo.toml | 4 ++++ crates/k8s-version/src/api_version.rs | 10 ++++++++++ crates/k8s-version/src/level.rs | 10 ++++++++++ crates/k8s-version/src/version.rs | 11 +++++++++++ 4 files changed, 35 insertions(+) diff --git a/crates/k8s-version/Cargo.toml b/crates/k8s-version/Cargo.toml index 39e460312..af7f4cbeb 100644 --- a/crates/k8s-version/Cargo.toml +++ b/crates/k8s-version/Cargo.toml @@ -6,7 +6,11 @@ license.workspace = true edition.workspace = true repository.workspace = true +[features] +darling = ["dep:darling"] + [dependencies] +darling = { workspace = true, optional = true } lazy_static.workspace = true regex.workspace = true snafu.workspace = true diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index a5acbc01f..ac2253fd5 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -2,6 +2,9 @@ use std::{cmp::Ordering, fmt::Display, str::FromStr}; use snafu::{ResultExt, Snafu}; +#[cfg(feature = "darling")] +use darling::FromMeta; + use crate::{Version, VersionParseError}; #[derive(Debug, PartialEq, Snafu)] @@ -65,3 +68,10 @@ impl Display for ApiVersion { } } } + +#[cfg(feature = "darling")] +impl FromMeta for ApiVersion { + fn from_string(value: &str) -> darling::Result { + Self::from_str(value).map_err(darling::Error::custom) + } +} diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index 63dd0b81f..33f5e2482 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -10,6 +10,9 @@ use lazy_static::lazy_static; use regex::Regex; use snafu::{OptionExt, ResultExt, Snafu}; +#[cfg(feature = "darling")] +use darling::FromMeta; + lazy_static! { static ref LEVEL_REGEX: Regex = Regex::new(r"^(?P[a-z]+)(?P\d+)$").unwrap(); @@ -136,6 +139,13 @@ impl Display for Level { } } +#[cfg(feature = "darling")] +impl FromMeta for Level { + fn from_string(value: &str) -> darling::Result { + Self::from_str(value).map_err(darling::Error::custom) + } +} + #[cfg(test)] mod test { use rstest::rstest; diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 7b6da583c..6357a65e7 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -4,6 +4,9 @@ use lazy_static::lazy_static; use regex::Regex; use snafu::{OptionExt, ResultExt, Snafu}; +#[cfg(feature = "darling")] +use darling::FromMeta; + use crate::{Level, ParseLevelError}; lazy_static! { @@ -95,6 +98,14 @@ impl Display for Version { } } } + +#[cfg(feature = "darling")] +impl FromMeta for Version { + fn from_string(value: &str) -> darling::Result { + Self::from_str(value).map_err(darling::Error::custom) + } +} + impl Version { pub fn new(major: u64, minor: Option) -> Self { Self { From 50f8076bf24aa63896ee650399a9104a11b10a63 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 11:35:34 +0200 Subject: [PATCH 09/43] chore(k8s-version): Add changelog --- CHANGELOG.md | 1 + crates/k8s-version/CHANGELOG.md | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 crates/k8s-version/CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c1eb45c8..932f53fc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,3 +6,4 @@ Please see the relevant crate changelogs: - [stackable-operator-derive](./crates/stackable-operator-derive/CHANGELOG.md) - [stackable-operator](./crates/stackable-operator/CHANGELOG.md) - [stackable-webhook](./crates/stackable-webhook/CHANGELOG.md) +- [k8s-version](./crates/k8s-version/CHANGELOG.md) diff --git a/crates/k8s-version/CHANGELOG.md b/crates/k8s-version/CHANGELOG.md new file mode 100644 index 000000000..6d303b20d --- /dev/null +++ b/crates/k8s-version/CHANGELOG.md @@ -0,0 +1,5 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +## [Unreleased] From b09ad5fd91ac06dc079241533ed16ff08948027f Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 13:05:47 +0200 Subject: [PATCH 10/43] test(k8s-version): Add more unit tests --- crates/k8s-version/Cargo.toml | 3 + crates/k8s-version/src/api_version.rs | 65 ++++++++++++++++++++++ crates/k8s-version/src/level.rs | 2 +- crates/k8s-version/src/version.rs | 80 ++++++++++++++++----------- 4 files changed, 117 insertions(+), 33 deletions(-) diff --git a/crates/k8s-version/Cargo.toml b/crates/k8s-version/Cargo.toml index af7f4cbeb..404925e03 100644 --- a/crates/k8s-version/Cargo.toml +++ b/crates/k8s-version/Cargo.toml @@ -17,3 +17,6 @@ snafu.workspace = true [dev-dependencies] rstest.workspace = true +quote.workspace = true +proc-macro2.workspace = true +syn.workspace = true diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index ac2253fd5..f7b7b849c 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -11,6 +11,9 @@ use crate::{Version, VersionParseError}; pub enum ApiVersionParseError { #[snafu(display("failed to parse version"))] ParseVersion { source: VersionParseError }, + + #[snafu(display("group cannot be empty"))] + EmptyGroup, } /// A Kubernetes API version with the `(/)` format, for example @@ -37,7 +40,12 @@ impl FromStr for ApiVersion { fn from_str(input: &str) -> Result { let (group, version) = if let Some((group, version)) = input.split_once('/') { + if group.is_empty() { + return EmptyGroupSnafu.fail(); + } + // TODO (Techassi): Validate group + ( Some(group.to_string()), Version::from_str(version).context(ParseVersionSnafu)?, @@ -75,3 +83,60 @@ impl FromMeta for ApiVersion { Self::from_str(value).map_err(darling::Error::custom) } } + +#[cfg(test)] +mod test { + use super::*; + use crate::Level; + + use rstest::rstest; + + #[cfg(feature = "darling")] + use quote::quote; + + #[cfg(feature = "darling")] + fn parse_meta(tokens: proc_macro2::TokenStream) -> ::std::result::Result { + let attribute: syn::Attribute = syn::parse_quote!(#[#tokens]); + Ok(attribute.meta) + } + + #[rstest] + #[case("extensions/v1beta1", ApiVersion { group: Some("extensions".into()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case("v1beta1", ApiVersion { group: None, version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case("v1", ApiVersion { group: None, version: Version { major: 1, level: None } })] + fn valid_api_version(#[case] input: &str, #[case] expected: ApiVersion) { + let api_version = ApiVersion::from_str(input).expect("valid Kubernetes api version"); + assert_eq!(api_version, expected); + } + + #[rstest] + #[case("extensions/beta1", ApiVersionParseError::ParseVersion { source: VersionParseError::InvalidFormat })] + #[case("/v1beta1", ApiVersionParseError::EmptyGroup)] + fn invalid_api_version(#[case] input: &str, #[case] error: ApiVersionParseError) { + let err = ApiVersion::from_str(input).expect_err("invalid Kubernetes api versions"); + assert_eq!(err, error); + } + + #[rstest] + #[case(Version {major: 1, level: Some(Level::Alpha(2))}, Version {major: 1, level: Some(Level::Alpha(1))}, Ordering::Greater)] + #[case(Version {major: 1, level: Some(Level::Alpha(1))}, Version {major: 1, level: Some(Level::Alpha(1))}, Ordering::Equal)] + #[case(Version {major: 1, level: Some(Level::Alpha(1))}, Version {major: 1, level: Some(Level::Alpha(2))}, Ordering::Less)] + #[case(Version {major: 1, level: None}, Version {major: 1, level: Some(Level::Alpha(2))}, Ordering::Greater)] + #[case(Version {major: 1, level: None}, Version {major: 1, level: Some(Level::Beta(2))}, Ordering::Greater)] + #[case(Version {major: 1, level: None}, Version {major: 1, level: None}, Ordering::Equal)] + #[case(Version {major: 1, level: None}, Version {major: 2, level: None}, Ordering::Less)] + fn partial_ord(#[case] input: Version, #[case] other: Version, #[case] expected: Ordering) { + assert_eq!(input.partial_cmp(&other), Some(expected)); + } + + #[cfg(feature = "darling")] + #[rstest] + #[case(quote!(ignore = "extensions/v1beta1"), ApiVersion { group: Some("extensions".into()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case(quote!(ignore = "v1beta1"), ApiVersion { group: None, version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case(quote!(ignore = "v1"), ApiVersion { group: None, version: Version { major: 1, level: None } })] + fn from_meta(#[case] input: proc_macro2::TokenStream, #[case] expected: ApiVersion) { + let meta = parse_meta(input).expect("valid attribute tokens"); + let api_version = ApiVersion::from_meta(&meta).expect("version must parse from attribute"); + assert_eq!(api_version, expected); + } +} diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index 33f5e2482..c50c8113e 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -161,7 +161,7 @@ mod test { #[case(Level::Beta(2), Level::Beta(1), Ordering::Greater)] #[case(Level::Beta(2), Level::Beta(2), Ordering::Equal)] #[case(Level::Beta(1), Level::Beta(2), Ordering::Less)] - fn partial_ord_level(#[case] input: Level, #[case] other: Level, #[case] expected: Ordering) { + fn partial_ord(#[case] input: Level, #[case] other: Level, #[case] expected: Ordering) { assert_eq!(input.partial_cmp(&other), Some(expected)) } } diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 6357a65e7..078431c85 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -56,28 +56,23 @@ impl FromStr for Version { .parse::() .context(ParseMajorVersionSnafu)?; - let level = captures - .name("level") - .expect("internal error: check that the correct match label is specified") - .as_str(); - - if level.is_empty() { - return Ok(Self { major, level: None }); + if let Some(level) = captures.name("level") { + let level = Level::from_str(level.as_str()).context(ParseLevelSnafu)?; + + Ok(Self { + level: Some(level), + major, + }) + } else { + Ok(Self { major, level: None }) } - - let level = Level::from_str(level).context(ParseLevelSnafu)?; - - Ok(Self { - level: Some(level), - major, - }) } } impl PartialOrd for Version { fn partial_cmp(&self, other: &Self) -> Option { match self.major.partial_cmp(&other.major) { - Some(core::cmp::Ordering::Equal) => {} + Some(Ordering::Equal) => {} ord => return ord, } @@ -118,26 +113,47 @@ impl Version { #[cfg(test)] mod test { use super::*; + use rstest::rstest; + #[cfg(feature = "darling")] + use quote::quote; + + #[cfg(feature = "darling")] + fn parse_meta(tokens: proc_macro2::TokenStream) -> ::std::result::Result { + let attribute: syn::Attribute = syn::parse_quote!(#[#tokens]); + Ok(attribute.meta) + } + #[rstest] - #[case("v1alpha12")] - #[case("v1alpha1")] - #[case("v1beta1")] - #[case("v1")] - fn valid_version(#[case] input: &str) { - let version = Version::from_str(input).unwrap(); - assert_eq!(version.to_string(), input); + #[case("v1alpha12", Version { major: 1, level: Some(Level::Alpha(12)) })] + #[case("v1alpha1", Version { major: 1, level: Some(Level::Alpha(1)) })] + #[case("v1beta1", Version { major: 1, level: Some(Level::Beta(1)) })] + #[case("v1", Version { major: 1, level: None })] + fn valid_version(#[case] input: &str, #[case] expected: Version) { + let version = Version::from_str(input).expect("valid Kubernetes version"); + assert_eq!(version, expected); } - // #[rstest] - // #[case("v1gamma12", VersionParseError::ParseLevel { source: ParseLevelError::InvalidLevel })] - // #[case("v1betä1", VersionParseError::InvalidFormat)] - // #[case("1beta1", VersionParseError::InvalidStart)] - // #[case("", VersionParseError::InvalidFormat)] - // #[case("v0", VersionParseError::LeadingZero)] - // fn invalid_version(#[case] input: &str, #[case] error: VersionParseError) { - // let err = Version::from_str(input).unwrap_err(); - // assert_eq!(err, error) - // } + #[rstest] + #[case("v1gamma12", VersionParseError::ParseLevel { source: ParseLevelError::UnknownIdentifier })] + #[case("v1betä1", VersionParseError::InvalidFormat)] + #[case("1beta1", VersionParseError::InvalidFormat)] + #[case("", VersionParseError::InvalidFormat)] + fn invalid_version(#[case] input: &str, #[case] error: VersionParseError) { + let err = Version::from_str(input).expect_err("invalid Kubernetes version"); + assert_eq!(err, error) + } + + #[cfg(feature = "darling")] + #[rstest] + #[case(quote!("v1alpha12"), Version { major: 1, level: Some(Level::Alpha(12)) })] + #[case(quote!("v1alpha1"), Version { major: 1, level: Some(Level::Alpha(1)) })] + #[case(quote!("v1beta1"), Version { major: 1, level: Some(Level::Beta(1)) })] + #[case(quote!("v1"), Version { major: 1, level: None })] + fn from_meta(#[case] input: proc_macro2::TokenStream, #[case] expected: Version) { + let meta = parse_meta(input).expect("valid attribute tokens"); + let version = Version::from_meta(&meta).expect("version must parse from attribute"); + assert_eq!(version, expected); + } } From 015ce8d0ae01477608202a70f9bb533023cfeecf Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 15:57:30 +0200 Subject: [PATCH 11/43] docs(k8s-version): Add README --- crates/k8s-version/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 crates/k8s-version/README.md diff --git a/crates/k8s-version/README.md b/crates/k8s-version/README.md new file mode 100644 index 000000000..b60760da0 --- /dev/null +++ b/crates/k8s-version/README.md @@ -0,0 +1,9 @@ +# k8s-version + +A small helper crate to parse and validate Kubernetes resource API versions. + +```rust +use k8s_version::ApiVersion; + +let api_version = ApiVersion::from_str("extensions/v1beta1")?; +``` From 9ba774f633ea9285924644102e843fb7fd513b54 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 19 Apr 2024 15:58:38 +0200 Subject: [PATCH 12/43] chore: Switch work machine --- crates/stackable-versioned/Cargo.toml | 2 + .../src/attrs/container.rs | 47 +++++++------- crates/stackable-versioned/src/attrs/field.rs | 11 ++-- crates/stackable-versioned/src/gen/field.rs | 57 +++++++++-------- crates/stackable-versioned/src/gen/mod.rs | 7 ++- crates/stackable-versioned/src/gen/version.rs | 6 ++ crates/stackable-versioned/src/gen/vstruct.rs | 62 +++---------------- 7 files changed, 83 insertions(+), 109 deletions(-) create mode 100644 crates/stackable-versioned/src/gen/version.rs diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index 88b13d12a..2d22b713f 100644 --- a/crates/stackable-versioned/Cargo.toml +++ b/crates/stackable-versioned/Cargo.toml @@ -10,6 +10,8 @@ repository.workspace = true proc-macro = true [dependencies] +k8s-version = { path = "../k8s-version", features = ["darling"] } + convert_case.workspace = true darling.workspace = true proc-macro2.workspace = true diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 3db1b5543..b48829e86 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -4,6 +4,7 @@ use darling::{ util::{Flag, SpannedValue}, Error, FromDeriveInput, FromMeta, }; +use k8s_version::Version; #[derive(Clone, Debug, FromDeriveInput)] #[darling( @@ -18,7 +19,7 @@ pub(crate) struct ContainerAttributes { } impl ContainerAttributes { - fn validate(mut self) -> darling::Result { + fn validate(self) -> darling::Result { // If there are no versions defined, the derive macro errors out. There // should be at least one version if the derive macro is used. if self.versions.is_empty() { @@ -28,28 +29,28 @@ impl ContainerAttributes { .with_span(&self.versions.span())); } - for version in &mut *self.versions { - // Ensure that the version name is not empty, because we cannot use - // an empty name as the module name. - if version.name.is_empty() { - return Err(Error::custom("field `name` of `version` must not be empty") - .with_span(&version.name.span())); - } + // for version in &mut *self.versions { + // // Ensure that the version name is not empty, because we cannot use + // // an empty name as the module name. + // if version.name.is_empty() { + // return Err(Error::custom("field `name` of `version` must not be empty") + // .with_span(&version.name.span())); + // } - // Ensure that the version name contains only a selection of valid - // characters, which can also be used as module identifiers (after - // minor replacements). - if !version - .name - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-') - { - return Err(Error::custom( - "field `name` of `version` must only contain alphanumeric ASCII characters (a-z, A-Z, 0-9, '.', '-')", - ) - .with_span(&version.name.span())); - } - } + // // Ensure that the version name contains only a selection of valid + // // characters, which can also be used as module identifiers (after + // // minor replacements). + // if !version + // .name + // .chars() + // .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-') + // { + // return Err(Error::custom( + // "field `name` of `version` must only contain alphanumeric ASCII characters (a-z, A-Z, 0-9, '.', '-')", + // ) + // .with_span(&version.name.span())); + // } + // } // Ensure every version is unique and isn't declared multiple times. This // is inspired by the itertools all_unique function. @@ -71,6 +72,6 @@ impl ContainerAttributes { #[derive(Clone, Debug, FromMeta)] pub struct VersionAttributes { - pub(crate) name: SpannedValue, + pub(crate) name: SpannedValue, pub(crate) deprecated: Flag, } diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 2d869cb94..5df4539c3 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,6 +1,7 @@ use std::fmt; use darling::{util::SpannedValue, Error, FromField, FromMeta}; +use k8s_version::Version; #[derive(Debug, FromField)] #[darling(attributes(versioned), forward_attrs(allow, doc, cfg, serde))] @@ -12,19 +13,19 @@ pub(crate) struct FieldAttributes { #[derive(Debug, FromMeta)] pub(crate) struct AddedAttributes { - pub(crate) since: SpannedValue, + pub(crate) since: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct RenamedAttributes { - since: SpannedValue, + since: SpannedValue, pub(crate) to: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { - pub(crate) since: SpannedValue, - pub(crate) note: SpannedValue, + pub(crate) since: SpannedValue, + pub(crate) _note: SpannedValue, } #[derive(Debug)] @@ -81,7 +82,7 @@ impl fmt::Display for FieldAction { } impl FieldAction { - pub(crate) fn since(&self) -> Option<&str> { + pub(crate) fn since(&self) -> Option<&Version> { match self { FieldAction::Added(added) => Some(&*added.since), FieldAction::Renamed(renamed) => Some(&*renamed.since), diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index f585c23d8..78c5db834 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,8 +1,11 @@ use proc_macro2::TokenStream; -use quote::{format_ident, quote, ToTokens}; -use syn::Field; +use quote::quote; +use syn::{Attribute, Field}; -use crate::attrs::field::FieldAction; +use crate::{ + attrs::field::FieldAction, + gen::{version::ContainerVersion, ToTokensExt}, +}; pub(crate) struct VersionedField { // TODO (@Techassi): There can be multiple actions for one field (in @@ -11,40 +14,42 @@ pub(crate) struct VersionedField { pub(crate) inner: Field, } -impl ToTokens for VersionedField { - fn to_tokens(&self, tokens: &mut TokenStream) { +impl ToTokensExt for VersionedField { + fn to_tokens_for_version(&self, version: &ContainerVersion) -> Option { match &self.action { - FieldAction::Renamed(renamed) => { - let field_name = format_ident!("{}", *renamed.to); - let field_type = &self.inner.ty; + FieldAction::Added(added) => { + // Skip generating the field, if the current generated + // version doesn't match the since field of the action. + if version.inner != *added.since { + return None; + } - tokens.extend(quote! { - pub #field_name: #field_type, - }) - } - FieldAction::Deprecated(deprecated) => { - // TODO (@Techassi): Is it save to unwrap here? - let field_name = format_ident!( - "deprecated_{}", - &self.inner.ident.as_ref().expect("field must have a name") - ); + let field_name = &self.inner.ident; let field_type = &self.inner.ty; + let doc = format!(" Added since `{}`.", *added.since); - let deprecation_note = format!( - "{} (was deprecated in {})", - &*deprecated.note, &*deprecated.since - ); + // TODO (@Techassi): Also forward other attributes + let doc_attrs: Vec<&Attribute> = self + .inner + .attrs + .iter() + .filter(|a| a.path().is_ident("doc")) + .collect(); - tokens.extend(quote! { - #[deprecated = #deprecation_note] + Some(quote! { + #(#doc_attrs)* + #[doc = ""] + #[doc = #doc] pub #field_name: #field_type, }) } - FieldAction::Added(_) | FieldAction::None => { + FieldAction::Renamed(_) => todo!(), + FieldAction::Deprecated(_) => todo!(), + FieldAction::None => { let field_name = &self.inner.ident; let field_type = &self.inner.ty; - tokens.extend(quote! { + Some(quote! { pub #field_name: #field_type, }) } diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index fd9302777..f95892882 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -5,11 +5,12 @@ use syn::{spanned::Spanned, Data, DeriveInput, Error, Result}; use crate::{ attrs::container::ContainerAttributes, - gen::{venum::VersionedEnum, vstruct::VersionedStruct}, + gen::{venum::VersionedEnum, version::ContainerVersion, vstruct::VersionedStruct}, }; pub(crate) mod field; pub(crate) mod venum; +pub(crate) mod version; pub(crate) mod vstruct; // NOTE (@Techassi): This derive macro cannot handle multiple structs / enums @@ -44,3 +45,7 @@ pub(crate) fn expand(input: DeriveInput) -> Result { Ok(expanded) } + +pub(crate) trait ToTokensExt { + fn to_tokens_for_version(&self, version: &ContainerVersion) -> Option; +} diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs new file mode 100644 index 000000000..948a65dc3 --- /dev/null +++ b/crates/stackable-versioned/src/gen/version.rs @@ -0,0 +1,6 @@ +use k8s_version::Version; + +pub(crate) struct ContainerVersion { + pub(crate) _deprecated: bool, + pub(crate) inner: Version, +} diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index dd29b53ad..1c7b15323 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -3,22 +3,16 @@ use std::ops::Deref; use darling::FromField; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; -use syn::{spanned::Spanned, Attribute, DataStruct, Error, Ident, Result}; +use syn::{spanned::Spanned, DataStruct, Error, Ident, Result}; use crate::{ attrs::{ container::ContainerAttributes, field::{FieldAction, FieldAttributes}, }, - gen::field::VersionedField, + gen::{field::VersionedField, version::ContainerVersion, ToTokensExt}, }; -pub(crate) struct Version { - original_name: String, - module_name: String, - _deprecated: bool, -} - /// Stores individual versions of a single struct. Each version tracks field /// actions, which describe if the field was added, renamed or deprecated in /// that version. Fields which are not versioned, are included in every @@ -26,8 +20,8 @@ pub(crate) struct Version { pub(crate) struct VersionedStruct { pub(crate) _ident: Ident, + pub(crate) _versions: Vec, pub(crate) _fields: Vec, - pub(crate) _versions: Vec, } impl ToTokens for VersionedStruct { @@ -38,50 +32,14 @@ impl ToTokens for VersionedStruct { let mut fields = TokenStream::new(); for field in &self._fields { - match &field.action { - FieldAction::Added(added) => { - // Skip generating the field, if the current generated - // version doesn't match the since field of the action. - if version.original_name != *added.since { - continue; - } - - let field_name = &field.inner.ident; - let field_type = &field.inner.ty; - let doc = format!(" Added since `{}`.", *added.since); - - let doc_attrs: Vec<&Attribute> = field - .inner - .attrs - .iter() - .filter(|a| a.path().is_ident("doc")) - .collect(); - - fields.extend(quote! { - #(#doc_attrs)* - #[doc = ""] - #[doc = #doc] - pub #field_name: #field_type, - }) - } - FieldAction::Renamed(_) => todo!(), - FieldAction::Deprecated(_) => todo!(), - FieldAction::None => { - let field_name = &field.inner.ident; - let field_type = &field.inner.ty; - - fields.extend(quote! { - pub #field_name: #field_type, - }) - } - } + fields.extend(field.to_tokens_for_version(version)) } // TODO (@Techassi): Make the generation of the module optional to // enable the attribute macro to be applied to a module which // generates versioned versions of all contained containers. - let module_name = format_ident!("{}", version.module_name); + let module_name = format_ident!("{}", version.inner.to_string()); let struct_name = &self._ident; _tokens.extend(quote! { @@ -116,16 +74,12 @@ impl VersionedStruct { let versions = attributes .versions .iter() - .cloned() .map(|v| { - let original_name = v.name.deref().clone(); - let module_name = original_name.replace('.', ""); let deprecated = v.deprecated.is_present(); - Version { - original_name, - module_name, + ContainerVersion { _deprecated: deprecated, + inner: v.name.deref().clone(), } }) .collect::>(); @@ -145,7 +99,7 @@ impl VersionedStruct { // the field, it is also valid. match field_action.since() { Some(since) => { - if versions.iter().any(|v| v.original_name == since) { + if versions.iter().any(|v| v.inner == *since) { fields.push(VersionedField::new(field, field_action)); continue; } From 03c32e71438ebbfcc9ac0f7871bfac9702e34460 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 11:46:06 +0200 Subject: [PATCH 13/43] Add basic support for renamed fields --- .../src/attrs/container.rs | 27 +++-------------- crates/stackable-versioned/src/attrs/field.rs | 6 ++-- crates/stackable-versioned/src/gen/field.rs | 29 ++++++++++++++++--- crates/stackable-versioned/tests/basic.rs | 6 +++- 4 files changed, 37 insertions(+), 31 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index b48829e86..9de205855 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -20,6 +20,10 @@ pub(crate) struct ContainerAttributes { impl ContainerAttributes { fn validate(self) -> darling::Result { + // Most of the validation for individual version strings is done by the + // k8s-version crate. That's why the code below only checks that at least + // one version is defined and that all declared versions are unique. + // If there are no versions defined, the derive macro errors out. There // should be at least one version if the derive macro is used. if self.versions.is_empty() { @@ -29,29 +33,6 @@ impl ContainerAttributes { .with_span(&self.versions.span())); } - // for version in &mut *self.versions { - // // Ensure that the version name is not empty, because we cannot use - // // an empty name as the module name. - // if version.name.is_empty() { - // return Err(Error::custom("field `name` of `version` must not be empty") - // .with_span(&version.name.span())); - // } - - // // Ensure that the version name contains only a selection of valid - // // characters, which can also be used as module identifiers (after - // // minor replacements). - // if !version - // .name - // .chars() - // .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-') - // { - // return Err(Error::custom( - // "field `name` of `version` must only contain alphanumeric ASCII characters (a-z, A-Z, 0-9, '.', '-')", - // ) - // .with_span(&version.name.span())); - // } - // } - // Ensure every version is unique and isn't declared multiple times. This // is inspired by the itertools all_unique function. let mut unique = HashSet::new(); diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 5df4539c3..292c50f1d 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -18,8 +18,8 @@ pub(crate) struct AddedAttributes { #[derive(Debug, FromMeta)] pub(crate) struct RenamedAttributes { - since: SpannedValue, - pub(crate) to: SpannedValue, + pub(crate) since: SpannedValue, + pub(crate) from: SpannedValue, } #[derive(Debug, FromMeta)] @@ -41,7 +41,7 @@ impl PartialEq for FieldAction { match (self, other) { (Self::Added(lhs), Self::Added(rhs)) => *lhs.since == *rhs.since, (Self::Renamed(lhs), Self::Renamed(rhs)) => { - *lhs.since == *rhs.since && *lhs.to == *rhs.to + *lhs.since == *rhs.since && *lhs.from == *rhs.from } (Self::Deprecated(lhs), Self::Deprecated(rhs)) => *lhs.since == *rhs.since, (Self::None, Self::None) => true, diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index 78c5db834..096b204db 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,5 +1,5 @@ use proc_macro2::TokenStream; -use quote::quote; +use quote::{format_ident, quote}; use syn::{Attribute, Field}; use crate::{ @@ -19,8 +19,8 @@ impl ToTokensExt for VersionedField { match &self.action { FieldAction::Added(added) => { // Skip generating the field, if the current generated - // version doesn't match the since field of the action. - if version.inner != *added.since { + // version appears before the field action version. + if version.inner < *added.since { return None; } @@ -43,9 +43,30 @@ impl ToTokensExt for VersionedField { pub #field_name: #field_type, }) } - FieldAction::Renamed(_) => todo!(), + FieldAction::Renamed(renamed) => { + if version.inner < *renamed.since { + // Use the original name for versions before the field action + // version. + let field_name = format_ident!("{}", *renamed.from); + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_name: #field_type, + }) + } else { + // Use the new name for versions equal or greater than the + // field action version + let field_name = &self.inner.ident; + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_name: #field_type, + }) + } + } FieldAction::Deprecated(_) => todo!(), FieldAction::None => { + // Generate fields without any attributes in every version. let field_name = &self.inner.ident; let field_type = &self.inner.ty; diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 0b61c3a33..0f8106ea7 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -7,10 +7,14 @@ struct Foo { /// My docs #[versioned(added(since = "v1beta1"))] bar: usize, + + #[versioned(renamed(since = "v1beta1", from = "fib"))] + fob: u32, + baz: bool, } #[test] fn basic() { - let _foo = v1beta1::Foo { bar: 0, baz: true }; + // let _foo = v1beta1::Foo { bar: 0, baz: true }; } From 5448375b9c513b7f9c8f13c89d73a807875216f9 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 13:35:40 +0200 Subject: [PATCH 14/43] Enfore version sorting, add option to opt out --- crates/k8s-version/src/level.rs | 2 +- crates/k8s-version/src/version.rs | 2 +- .../src/attrs/container.rs | 42 +++++++++++++++---- crates/stackable-versioned/src/gen/vstruct.rs | 4 +- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index c50c8113e..15f2ec38d 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -31,7 +31,7 @@ pub enum ParseLevelError { } /// A minor Kubernetes resource version with the `beta/alpha` format. -#[derive(Clone, Debug, Hash, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub enum Level { Beta(u64), Alpha(u64), diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 078431c85..b2ebb8a8e 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -37,7 +37,7 @@ pub enum VersionParseError { /// - /// /// [1]: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/identifiers.md#definitions -#[derive(Clone, Debug, Hash, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub struct Version { pub major: u64, pub level: Option, diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 9de205855..229d97611 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, ops::Deref}; +use std::{cmp::Ordering, collections::HashSet, ops::Deref}; use darling::{ util::{Flag, SpannedValue}, @@ -16,13 +16,17 @@ use k8s_version::Version; pub(crate) struct ContainerAttributes { #[darling(multiple, rename = "version")] pub(crate) versions: SpannedValue>, + + #[darling(default)] + pub(crate) options: VersionOptions, } impl ContainerAttributes { - fn validate(self) -> darling::Result { + fn validate(mut self) -> darling::Result { // Most of the validation for individual version strings is done by the - // k8s-version crate. That's why the code below only checks that at least - // one version is defined and that all declared versions are unique. + // k8s-version crate. That's why the code below only checks that at + // least one version is defined, they are defined in order (to ensure + // code consistency) and that all declared versions are unique. // If there are no versions defined, the derive macro errors out. There // should be at least one version if the derive macro is used. @@ -33,13 +37,32 @@ impl ContainerAttributes { .with_span(&self.versions.span())); } + // Ensure that versions are defined in sorted (ascending) order to keep + // code consistent. + if !self.options.allow_unsorted.is_present() { + let original = self.versions.deref().clone(); + self.versions + .sort_by(|lhs, rhs| lhs.name.partial_cmp(&rhs.name).unwrap_or(Ordering::Equal)); + + for (index, version) in original.iter().enumerate() { + if version.name == self.versions.get(index).unwrap().name { + continue; + } + + return Err(Error::custom(format!( + "versions in `#[versioned()]` must be defined in ascending order (version `{}` is misplaced)", + version.name + ))); + } + } + // Ensure every version is unique and isn't declared multiple times. This // is inspired by the itertools all_unique function. let mut unique = HashSet::new(); if !self .versions .iter() - .all(move |elem| unique.insert(elem.name.deref())) + .all(move |elem| unique.insert(elem.name)) { return Err(Error::custom( "attribute `#[versioned()]` contains one or more `version`s with a duplicate `name`", @@ -52,7 +75,12 @@ impl ContainerAttributes { } #[derive(Clone, Debug, FromMeta)] -pub struct VersionAttributes { - pub(crate) name: SpannedValue, +pub(crate) struct VersionAttributes { pub(crate) deprecated: Flag, + pub(crate) name: Version, +} + +#[derive(Clone, Debug, Default, FromMeta)] +pub(crate) struct VersionOptions { + pub(crate) allow_unsorted: Flag, } diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 1c7b15323..06f7e8647 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -1,5 +1,3 @@ -use std::ops::Deref; - use darling::FromField; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; @@ -79,7 +77,7 @@ impl VersionedStruct { ContainerVersion { _deprecated: deprecated, - inner: v.name.deref().clone(), + inner: v.name, } }) .collect::>(); From cc2190a1a4855d635d0de75d4911e7809fb0c69a Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 13:35:52 +0200 Subject: [PATCH 15/43] Add basic support for deprecated fields --- crates/stackable-versioned/src/attrs/field.rs | 2 +- crates/stackable-versioned/src/gen/field.rs | 38 +++++++++++++++++-- crates/stackable-versioned/tests/basic.rs | 9 ++++- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 292c50f1d..136148fbb 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -25,7 +25,7 @@ pub(crate) struct RenamedAttributes { #[derive(Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { pub(crate) since: SpannedValue, - pub(crate) _note: SpannedValue, + pub(crate) note: SpannedValue, } #[derive(Debug)] diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index 096b204db..c8cb1f28e 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -54,8 +54,8 @@ impl ToTokensExt for VersionedField { pub #field_name: #field_type, }) } else { - // Use the new name for versions equal or greater than the - // field action version + // If the version is greater than the field action version + // (or equal), use the new field name. let field_name = &self.inner.ident; let field_type = &self.inner.ty; @@ -64,7 +64,39 @@ impl ToTokensExt for VersionedField { }) } } - FieldAction::Deprecated(_) => todo!(), + FieldAction::Deprecated(deprecated) => { + if version.inner < *deprecated.since { + // Remove the deprecated_ prefix from the field name and use + // it as the field name if the version is less than the + // field action version. + let field_name = format_ident!( + "{}", + &self + .inner + .ident + .as_ref() + .unwrap() + .to_string() + .replace("deprecated_", "") + ); + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_name: #field_type, + }) + } else { + // If the version is greater than the field action version + // (or equal), use the prefixed field name. + let field_name = &self.inner.ident; + let field_type = &self.inner.ty; + let deprecated_note = &*deprecated.note; + + Some(quote! { + #[deprecated = #deprecated_note] + pub #field_name: #field_type, + }) + } + } FieldAction::None => { // Generate fields without any attributes in every version. let field_name = &self.inner.ident; diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 0f8106ea7..879b58b28 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -2,7 +2,11 @@ use stackable_versioned::Versioned; #[derive(Versioned)] #[allow(dead_code)] -#[versioned(version(name = "v1alpha1"), version(name = "v1beta1"))] +#[versioned( + version(name = "v1alpha1"), + version(name = "v1beta1"), + version(name = "v1") +)] struct Foo { /// My docs #[versioned(added(since = "v1beta1"))] @@ -11,6 +15,9 @@ struct Foo { #[versioned(renamed(since = "v1beta1", from = "fib"))] fob: u32, + #[versioned(deprecated(since = "v1beta1", note = ""))] + deprecated_bop: i16, + baz: bool, } From 2eb23d7f13f16732e411f8743d298eda351b42ab Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 13:43:47 +0200 Subject: [PATCH 16/43] Remove unused dependency --- Cargo.toml | 1 - crates/stackable-versioned/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7fa346fbe..c8b54676d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,6 @@ chrono = { version = "0.4.37", default-features = false } clap = { version = "4.5.4", features = ["derive", "cargo", "env"] } const_format = "0.2.32" const-oid = "0.9.6" -convert_case = "0.6.0" darling = "0.20.8" delegate = "0.12.0" derivative = "2.2.0" diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index 2d22b713f..f19d35557 100644 --- a/crates/stackable-versioned/Cargo.toml +++ b/crates/stackable-versioned/Cargo.toml @@ -12,7 +12,6 @@ proc-macro = true [dependencies] k8s-version = { path = "../k8s-version", features = ["darling"] } -convert_case.workspace = true darling.workspace = true proc-macro2.workspace = true syn.workspace = true From 2586e77af4e4d4c3accfd64befd75b3e02072004 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 13:44:02 +0200 Subject: [PATCH 17/43] Fix k8s-version unit tests --- crates/k8s-version/src/version.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index b2ebb8a8e..eb20279d2 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -147,10 +147,10 @@ mod test { #[cfg(feature = "darling")] #[rstest] - #[case(quote!("v1alpha12"), Version { major: 1, level: Some(Level::Alpha(12)) })] - #[case(quote!("v1alpha1"), Version { major: 1, level: Some(Level::Alpha(1)) })] - #[case(quote!("v1beta1"), Version { major: 1, level: Some(Level::Beta(1)) })] - #[case(quote!("v1"), Version { major: 1, level: None })] + #[case(quote!(ignore = "v1alpha12"), Version { major: 1, level: Some(Level::Alpha(12)) })] + #[case(quote!(ignore = "v1alpha1"), Version { major: 1, level: Some(Level::Alpha(1)) })] + #[case(quote!(ignore = "v1beta1"), Version { major: 1, level: Some(Level::Beta(1)) })] + #[case(quote!(ignore = "v1"), Version { major: 1, level: None })] fn from_meta(#[case] input: proc_macro2::TokenStream, #[case] expected: Version) { let meta = parse_meta(input).expect("valid attribute tokens"); let version = Version::from_meta(&meta).expect("version must parse from attribute"); From 2506fc418512482023579c4f3273f104706fae0e Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 22 Apr 2024 17:01:32 +0200 Subject: [PATCH 18/43] Add basic support for multiple field actions on one field --- crates/stackable-versioned/src/attrs/field.rs | 134 ++++++++----- crates/stackable-versioned/src/gen/field.rs | 185 +++++++++--------- crates/stackable-versioned/src/gen/vstruct.rs | 43 ++-- crates/stackable-versioned/tests/basic.rs | 7 - 4 files changed, 198 insertions(+), 171 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 136148fbb..115245b4e 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,13 +1,18 @@ -use std::fmt; - use darling::{util::SpannedValue, Error, FromField, FromMeta}; use k8s_version::Version; +use syn::{Field, Ident}; + +use crate::gen::version::ContainerVersion; #[derive(Debug, FromField)] #[darling(attributes(versioned), forward_attrs(allow, doc, cfg, serde))] pub(crate) struct FieldAttributes { + ident: Option, added: Option, - renamed: Option, + + #[darling(multiple)] + renamed: Vec, + deprecated: Option, } @@ -19,75 +24,100 @@ pub(crate) struct AddedAttributes { #[derive(Debug, FromMeta)] pub(crate) struct RenamedAttributes { pub(crate) since: SpannedValue, - pub(crate) from: SpannedValue, + pub(crate) _from: SpannedValue, } #[derive(Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { pub(crate) since: SpannedValue, - pub(crate) note: SpannedValue, + pub(crate) _note: SpannedValue, } +/// This struct describes all possible actions which can be attached to _one_ +/// field. +/// +/// - A field can only ever be added once at most. A field not marked as 'added' +/// is part of the struct in every version until renamed or deprecated. +/// - A field can be renamed many times. That's why renames are stored in a +/// [`Vec`]. +/// - A field can only be deprecated once. A field not marked as 'deprecated' +/// will be included up until the latest version. #[derive(Debug)] -pub(crate) enum FieldAction { - Added(AddedAttributes), - Renamed(RenamedAttributes), - Deprecated(DeprecatedAttributes), - None, +pub(crate) struct FieldActions { + added: Option, + renamed: Vec, + deprecated: Option, } -impl PartialEq for FieldAction { - fn eq(&self, other: &Self) -> bool { - match (self, other) { - (Self::Added(lhs), Self::Added(rhs)) => *lhs.since == *rhs.since, - (Self::Renamed(lhs), Self::Renamed(rhs)) => { - *lhs.since == *rhs.since && *lhs.from == *rhs.from +impl TryFrom for FieldActions { + type Error = Error; + + fn try_from(attrs: FieldAttributes) -> Result { + match (&attrs.added, &attrs.renamed, &attrs.deprecated) { + (Some(added), _, Some(deprecated)) => { + if *added.since == *deprecated.since { + return Err(Error::custom( + "field cannot be marked as `added` and `deprecated` in the same version", + ) + .with_span(&attrs.ident.expect("internal: field must have name").span())); + } + } + (Some(added), renamed, _) => { + if renamed.iter().any(|r| *r.since == *added.since) { + return Err(Error::custom( + "field cannot be marked as `added` and `renamed` in the same version", + ) + .with_span(&attrs.ident.expect("internal: field must have name").span())); + } + } + (_, renamed, Some(deprecated)) => { + if renamed.iter().any(|r| *r.since == *deprecated.since) { + return Err(Error::custom( + "field cannot be marked as `deprecated` and `renamed` in the same version", + ) + .with_span(&attrs.ident.expect("internal: field must have name").span())); + } } - (Self::Deprecated(lhs), Self::Deprecated(rhs)) => *lhs.since == *rhs.since, - (Self::None, Self::None) => true, - _ => false, + _ => {} } + + Ok(Self { + added: attrs.added, + renamed: attrs.renamed, + deprecated: attrs.deprecated, + }) } } -impl TryFrom for FieldAction { - type Error = Error; +impl FieldActions { + pub(crate) fn is_in_version_set( + &self, + versions: &[ContainerVersion], + field: &Field, + ) -> Result<(), Error> { + // NOTE (@Techassi): Can we maybe optimize this a little? - fn try_from(value: FieldAttributes) -> Result { - // NOTE (@Techassi): We sadly currently cannot use the attribute span - // when reporting errors. That's why the errors will be displayed at - // the #[derive(Versioned)] position. - - match (value.added, value.renamed, value.deprecated) { - (Some(added), None, None) => Ok(FieldAction::Added(added)), - (None, Some(renamed), None) => Ok(FieldAction::Renamed(renamed)), - (None, None, Some(deprecated)) => Ok(FieldAction::Deprecated(deprecated)), - (None, None, None) => Ok(FieldAction::None), - _ => Err(Error::custom( - "cannot specifiy multiple field actions at once", - )), + if let Some(added) = &self.added { + if !versions.iter().any(|v| v.inner == *added.since) { + return Err( + Error::custom("field action `added` uses version which was not declared via #[versioned(version)]") + .with_span(&field.ident.as_ref().expect("internal: field must have name").span() + )); + } } - } -} -impl fmt::Display for FieldAction { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - FieldAction::Added(_) => "added".fmt(f), - FieldAction::Renamed(_) => "renamed".fmt(f), - FieldAction::Deprecated(_) => "deprecated".fmt(f), - FieldAction::None => "".fmt(f), + for rename in &self.renamed { + if !versions.iter().any(|v| v.inner == *rename.since) { + return Err(Error::custom("field action `renamed` uses version which was not declared via #[versioned(version)]")); + } } - } -} -impl FieldAction { - pub(crate) fn since(&self) -> Option<&Version> { - match self { - FieldAction::Added(added) => Some(&*added.since), - FieldAction::Renamed(renamed) => Some(&*renamed.since), - FieldAction::Deprecated(deprecated) => Some(&*deprecated.since), - FieldAction::None => None, + if let Some(deprecated) = &self.deprecated { + if !versions.iter().any(|v| v.inner == *deprecated.since) { + return Err(Error::custom("field action `deprecated` uses version which was not declared via #[versioned(version)]")); + } } + + Ok(()) } } diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index c8cb1f28e..c8f816aec 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,120 +1,121 @@ use proc_macro2::TokenStream; -use quote::{format_ident, quote}; -use syn::{Attribute, Field}; +use syn::Field; use crate::{ - attrs::field::FieldAction, + attrs::field::FieldActions, gen::{version::ContainerVersion, ToTokensExt}, }; pub(crate) struct VersionedField { // TODO (@Techassi): There can be multiple actions for one field (in // different versions). Add support for that here. - pub(crate) action: FieldAction, - pub(crate) inner: Field, + pub(crate) _actions: FieldActions, + pub(crate) _inner: Field, } impl ToTokensExt for VersionedField { - fn to_tokens_for_version(&self, version: &ContainerVersion) -> Option { - match &self.action { - FieldAction::Added(added) => { - // Skip generating the field, if the current generated - // version appears before the field action version. - if version.inner < *added.since { - return None; - } + fn to_tokens_for_version(&self, _version: &ContainerVersion) -> Option { + // match &self.actions { + // FieldAction::Added(added) => { + // // Skip generating the field, if the current generated + // // version appears before the field action version. + // if version.inner < *added.since { + // return None; + // } - let field_name = &self.inner.ident; - let field_type = &self.inner.ty; - let doc = format!(" Added since `{}`.", *added.since); + // let field_name = &self.inner.ident; + // let field_type = &self.inner.ty; + // let doc = format!(" Added since `{}`.", *added.since); - // TODO (@Techassi): Also forward other attributes - let doc_attrs: Vec<&Attribute> = self - .inner - .attrs - .iter() - .filter(|a| a.path().is_ident("doc")) - .collect(); + // // TODO (@Techassi): Also forward other attributes + // let doc_attrs: Vec<&Attribute> = self + // .inner + // .attrs + // .iter() + // .filter(|a| a.path().is_ident("doc")) + // .collect(); - Some(quote! { - #(#doc_attrs)* - #[doc = ""] - #[doc = #doc] - pub #field_name: #field_type, - }) - } - FieldAction::Renamed(renamed) => { - if version.inner < *renamed.since { - // Use the original name for versions before the field action - // version. - let field_name = format_ident!("{}", *renamed.from); - let field_type = &self.inner.ty; + // Some(quote! { + // #(#doc_attrs)* + // #[doc = ""] + // #[doc = #doc] + // pub #field_name: #field_type, + // }) + // } + // FieldAction::Renamed(renamed) => { + // if version.inner < *renamed.since { + // // Use the original name for versions before the field action + // // version. + // let field_name = format_ident!("{}", *renamed.from); + // let field_type = &self.inner.ty; - Some(quote! { - pub #field_name: #field_type, - }) - } else { - // If the version is greater than the field action version - // (or equal), use the new field name. - let field_name = &self.inner.ident; - let field_type = &self.inner.ty; + // Some(quote! { + // pub #field_name: #field_type, + // }) + // } else { + // // If the version is greater than the field action version + // // (or equal), use the new field name. + // let field_name = &self.inner.ident; + // let field_type = &self.inner.ty; - Some(quote! { - pub #field_name: #field_type, - }) - } - } - FieldAction::Deprecated(deprecated) => { - if version.inner < *deprecated.since { - // Remove the deprecated_ prefix from the field name and use - // it as the field name if the version is less than the - // field action version. - let field_name = format_ident!( - "{}", - &self - .inner - .ident - .as_ref() - .unwrap() - .to_string() - .replace("deprecated_", "") - ); - let field_type = &self.inner.ty; + // Some(quote! { + // pub #field_name: #field_type, + // }) + // } + // } + // FieldAction::Deprecated(deprecated) => { + // if version.inner < *deprecated.since { + // // Remove the deprecated_ prefix from the field name and use + // // it as the field name if the version is less than the + // // field action version. + // let field_name = format_ident!( + // "{}", + // &self + // .inner + // .ident + // .as_ref() + // .unwrap() + // .to_string() + // .replace("deprecated_", "") + // ); + // let field_type = &self.inner.ty; - Some(quote! { - pub #field_name: #field_type, - }) - } else { - // If the version is greater than the field action version - // (or equal), use the prefixed field name. - let field_name = &self.inner.ident; - let field_type = &self.inner.ty; - let deprecated_note = &*deprecated.note; + // Some(quote! { + // pub #field_name: #field_type, + // }) + // } else { + // // If the version is greater than the field action version + // // (or equal), use the prefixed field name. + // let field_name = &self.inner.ident; + // let field_type = &self.inner.ty; + // let deprecated_note = &*deprecated.note; - Some(quote! { - #[deprecated = #deprecated_note] - pub #field_name: #field_type, - }) - } - } - FieldAction::None => { - // Generate fields without any attributes in every version. - let field_name = &self.inner.ident; - let field_type = &self.inner.ty; + // Some(quote! { + // #[deprecated = #deprecated_note] + // pub #field_name: #field_type, + // }) + // } + // } + // FieldAction::None => { + // // Generate fields without any attributes in every version. + // let field_name = &self.inner.ident; + // let field_type = &self.inner.ty; - Some(quote! { - pub #field_name: #field_type, - }) - } - } + // Some(quote! { + // pub #field_name: #field_type, + // }) + // } + // } + None + // todo!() } } impl VersionedField { - pub(crate) fn new(field: Field, action: FieldAction) -> Self { + pub(crate) fn new(field: Field, actions: FieldActions) -> Self { Self { - inner: field, - action, + _inner: field, + _actions: actions, } } } diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 06f7e8647..c11d4236c 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -1,12 +1,12 @@ use darling::FromField; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; -use syn::{spanned::Spanned, DataStruct, Error, Ident, Result}; +use syn::{DataStruct, Ident, Result}; use crate::{ attrs::{ container::ContainerAttributes, - field::{FieldAction, FieldAttributes}, + field::{FieldActions, FieldAttributes}, }, gen::{field::VersionedField, version::ContainerVersion, ToTokensExt}, }; @@ -90,28 +90,31 @@ impl VersionedStruct { // declared. Using the action and the field data, a VersionField // can be created. let field_attributes = FieldAttributes::from_field(&field)?; - let field_action = FieldAction::try_from(field_attributes)?; + let field_actions = FieldActions::try_from(field_attributes)?; // Validate, that the field action uses a version which is declared // by the container attribute. If there is no attribute attached to // the field, it is also valid. - match field_action.since() { - Some(since) => { - if versions.iter().any(|v| v.inner == *since) { - fields.push(VersionedField::new(field, field_action)); - continue; - } - - // At this point the version specified in the action is not - // in the set of declared versions and thus an error is - // returned. - return Err(Error::new( - field.span(), - format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), - )); - } - None => fields.push(VersionedField::new(field, field_action)), - } + field_actions.is_in_version_set(&versions, &field)?; + fields.push(VersionedField::new(field, field_actions)); + + // match field_action.since() { + // Some(since) => { + // if versions.iter().any(|v| v.inner == *since) { + // fields.push(VersionedField::new(field, field_action)); + // continue; + // } + + // // At this point the version specified in the action is not + // // in the set of declared versions and thus an error is + // // returned. + // return Err(Error::new( + // field.span(), + // format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), + // )); + // } + // None => fields.push(VersionedField::new(field, field_action)), + // } } Ok(Self { diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 879b58b28..bff38290c 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -11,13 +11,6 @@ struct Foo { /// My docs #[versioned(added(since = "v1beta1"))] bar: usize, - - #[versioned(renamed(since = "v1beta1", from = "fib"))] - fob: u32, - - #[versioned(deprecated(since = "v1beta1", note = ""))] - deprecated_bop: i16, - baz: bool, } From 957dd913bfcaa69887d7613025628663c66fc44d Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 23 Apr 2024 11:07:31 +0200 Subject: [PATCH 19/43] Restructure field validation code --- crates/stackable-versioned/src/attrs/field.rs | 110 ++++++++++++------ crates/stackable-versioned/src/gen/field.rs | 8 +- crates/stackable-versioned/src/gen/vstruct.rs | 28 +---- 3 files changed, 83 insertions(+), 63 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 115245b4e..6988f1a26 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,11 +1,32 @@ use darling::{util::SpannedValue, Error, FromField, FromMeta}; use k8s_version::Version; -use syn::{Field, Ident}; +use syn::{spanned::Spanned, Field, Ident}; use crate::gen::version::ContainerVersion; +/// This struct describes all available field attributes, as well as the field +/// name to display better diagnostics. +/// +/// Data stored in this struct is validated using darling's `and_then` attribute. +/// During darlings validation, it is not possible to validate that action +/// versions match up with declared versions on the container. This validation +/// can be done using the associated [`FieldAttributes::check_versions`] +/// function. +/// +/// ### Field Rules +/// +/// - A field can only ever be added once at most. A field not marked as 'added' +/// is part of the struct in every version until renamed or deprecated. +/// - A field can be renamed many times. That's why renames are stored in a +/// [`Vec`]. +/// - A field can only be deprecated once. A field not marked as 'deprecated' +/// will be included up until the latest version. #[derive(Debug, FromField)] -#[darling(attributes(versioned), forward_attrs(allow, doc, cfg, serde))] +#[darling( + attributes(versioned), + forward_attrs(allow, doc, cfg, serde), + and_then = FieldAttributes::validate +)] pub(crate) struct FieldAttributes { ident: Option, added: Option, @@ -33,33 +54,20 @@ pub(crate) struct DeprecatedAttributes { pub(crate) _note: SpannedValue, } -/// This struct describes all possible actions which can be attached to _one_ -/// field. -/// -/// - A field can only ever be added once at most. A field not marked as 'added' -/// is part of the struct in every version until renamed or deprecated. -/// - A field can be renamed many times. That's why renames are stored in a -/// [`Vec`]. -/// - A field can only be deprecated once. A field not marked as 'deprecated' -/// will be included up until the latest version. -#[derive(Debug)] -pub(crate) struct FieldActions { - added: Option, - renamed: Vec, - deprecated: Option, -} - -impl TryFrom for FieldActions { - type Error = Error; - - fn try_from(attrs: FieldAttributes) -> Result { - match (&attrs.added, &attrs.renamed, &attrs.deprecated) { +impl FieldAttributes { + pub fn validate(self) -> Result { + match (&self.added, &self.renamed, &self.deprecated) { + // The derive macro prohibits the use of the 'added' and 'deprecated' + // field action using the same version. This is because it doesn't + // make sense to add a field and immediatly mark that field as + // deprecated in the same version. Instead, fields should be + // deprecated at least one version later. (Some(added), _, Some(deprecated)) => { if *added.since == *deprecated.since { return Err(Error::custom( "field cannot be marked as `added` and `deprecated` in the same version", ) - .with_span(&attrs.ident.expect("internal: field must have name").span())); + .with_span(&self.ident.span())); } } (Some(added), renamed, _) => { @@ -67,7 +75,7 @@ impl TryFrom for FieldActions { return Err(Error::custom( "field cannot be marked as `added` and `renamed` in the same version", ) - .with_span(&attrs.ident.expect("internal: field must have name").span())); + .with_span(&self.ident.span())); } } (_, renamed, Some(deprecated)) => { @@ -75,22 +83,56 @@ impl TryFrom for FieldActions { return Err(Error::custom( "field cannot be marked as `deprecated` and `renamed` in the same version", ) - .with_span(&attrs.ident.expect("internal: field must have name").span())); + .with_span(&self.ident.span())); } } _ => {} } - Ok(Self { - added: attrs.added, - renamed: attrs.renamed, - deprecated: attrs.deprecated, - }) + // Validate that actions use chronologically valid versions. If the + // field was added (not included from the start), the version must be + // less than version from the renamed and deprecated actions. + let added_version = self.added.as_ref().map(|a| *a.since); + let deprecated_version = self.deprecated.as_ref().map(|d| *d.since); + + if let Some(added_version) = added_version { + if !self.renamed.iter().all(|r| *r.since > added_version) { + return Err(Error::custom(format!( + "field was marked as `added` in version `{}` and thus all renames must use a higher version", + added_version + )) + .with_span(&self.ident.span())); + } + + if let Some(deprecated_version) = deprecated_version { + if added_version > deprecated_version { + return Err(Error::custom(format!( + "field was marked as `added` in version `{}` while being marked as `deprecated` in an earlier version `{}`", + added_version, + deprecated_version + )).with_span(&self.ident.span())); + } + } + } + + // TODO (@Techassi): Add validation for renames so that renamed fields + // match up and form a continous chain (eg. foo -> bar -> baz). + + // The same rule applies to renamed fields. Versions of renames must be + // less than the deprecation version (if any). + if let Some(deprecated_version) = deprecated_version { + if !self.renamed.iter().all(|r| *r.since < deprecated_version) { + return Err(Error::custom(format!( + "field was marked as `deprecated` in version `{}` and thus all renames must use a lower version", + deprecated_version + ))); + } + } + + Ok(self) } -} -impl FieldActions { - pub(crate) fn is_in_version_set( + pub(crate) fn check_versions( &self, versions: &[ContainerVersion], field: &Field, diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index c8f816aec..f6f176c60 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -2,14 +2,14 @@ use proc_macro2::TokenStream; use syn::Field; use crate::{ - attrs::field::FieldActions, + attrs::field::FieldAttributes, gen::{version::ContainerVersion, ToTokensExt}, }; pub(crate) struct VersionedField { // TODO (@Techassi): There can be multiple actions for one field (in // different versions). Add support for that here. - pub(crate) _actions: FieldActions, + pub(crate) _attrs: FieldAttributes, pub(crate) _inner: Field, } @@ -112,10 +112,10 @@ impl ToTokensExt for VersionedField { } impl VersionedField { - pub(crate) fn new(field: Field, actions: FieldActions) -> Self { + pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Self { Self { _inner: field, - _actions: actions, + _attrs: attrs, } } } diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index c11d4236c..82fab5182 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -4,10 +4,7 @@ use quote::{format_ident, quote, ToTokens}; use syn::{DataStruct, Ident, Result}; use crate::{ - attrs::{ - container::ContainerAttributes, - field::{FieldActions, FieldAttributes}, - }, + attrs::{container::ContainerAttributes, field::FieldAttributes}, gen::{field::VersionedField, version::ContainerVersion, ToTokensExt}, }; @@ -90,31 +87,12 @@ impl VersionedStruct { // declared. Using the action and the field data, a VersionField // can be created. let field_attributes = FieldAttributes::from_field(&field)?; - let field_actions = FieldActions::try_from(field_attributes)?; // Validate, that the field action uses a version which is declared // by the container attribute. If there is no attribute attached to // the field, it is also valid. - field_actions.is_in_version_set(&versions, &field)?; - fields.push(VersionedField::new(field, field_actions)); - - // match field_action.since() { - // Some(since) => { - // if versions.iter().any(|v| v.inner == *since) { - // fields.push(VersionedField::new(field, field_action)); - // continue; - // } - - // // At this point the version specified in the action is not - // // in the set of declared versions and thus an error is - // // returned. - // return Err(Error::new( - // field.span(), - // format!("field action `{}` contains version which is not declared via `#[versioned(version)]`", field_action), - // )); - // } - // None => fields.push(VersionedField::new(field, field_action)), - // } + field_attributes.check_versions(&versions, &field)?; + fields.push(VersionedField::new(field, field_attributes)); } Ok(Self { From cba91530a6b0598b5053032a068dfd0dfad0463e Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 25 Apr 2024 13:50:26 +0200 Subject: [PATCH 20/43] Generate chain of statuses --- .../src/attrs/container.rs | 2 + crates/stackable-versioned/src/attrs/field.rs | 98 +++-- crates/stackable-versioned/src/consts.rs | 1 + crates/stackable-versioned/src/gen/field.rs | 353 +++++++++++++----- crates/stackable-versioned/src/gen/version.rs | 1 + crates/stackable-versioned/src/gen/vstruct.rs | 77 ++-- crates/stackable-versioned/src/lib.rs | 1 + crates/stackable-versioned/tests/basic.rs | 12 +- 8 files changed, 369 insertions(+), 176 deletions(-) create mode 100644 crates/stackable-versioned/src/consts.rs diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 229d97611..b363cee71 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -37,6 +37,8 @@ impl ContainerAttributes { .with_span(&self.versions.span())); } + // NOTE (@Techassi): Do we even want to allow to opp-out of this? + // Ensure that versions are defined in sorted (ascending) order to keep // code consistent. if !self.options.allow_unsorted.is_present() { diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 6988f1a26..8fc073342 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,8 +1,10 @@ +use std::cmp::Ordering; + use darling::{util::SpannedValue, Error, FromField, FromMeta}; use k8s_version::Version; use syn::{spanned::Spanned, Field, Ident}; -use crate::gen::version::ContainerVersion; +use crate::{attrs::container::ContainerAttributes, consts::DEPRECATED_PREFIX}; /// This struct describes all available field attributes, as well as the field /// name to display better diagnostics. @@ -28,35 +30,35 @@ use crate::gen::version::ContainerVersion; and_then = FieldAttributes::validate )] pub(crate) struct FieldAttributes { - ident: Option, - added: Option, + pub(crate) ident: Option, + pub(crate) added: Option, - #[darling(multiple)] - renamed: Vec, + #[darling(multiple, rename = "renamed")] + pub(crate) renames: Vec, - deprecated: Option, + pub(crate) deprecated: Option, } -#[derive(Debug, FromMeta)] +#[derive(Clone, Debug, FromMeta)] pub(crate) struct AddedAttributes { pub(crate) since: SpannedValue, } -#[derive(Debug, FromMeta)] +#[derive(Clone, Debug, FromMeta)] pub(crate) struct RenamedAttributes { pub(crate) since: SpannedValue, - pub(crate) _from: SpannedValue, + pub(crate) from: SpannedValue, } -#[derive(Debug, FromMeta)] +#[derive(Clone, Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { pub(crate) since: SpannedValue, pub(crate) _note: SpannedValue, } impl FieldAttributes { - pub fn validate(self) -> Result { - match (&self.added, &self.renamed, &self.deprecated) { + fn validate(mut self) -> Result { + match (&self.added, &self.renames, &self.deprecated) { // The derive macro prohibits the use of the 'added' and 'deprecated' // field action using the same version. This is because it doesn't // make sense to add a field and immediatly mark that field as @@ -89,6 +91,30 @@ impl FieldAttributes { _ => {} } + // Validate that renamed action versions are sorted to ensure consistent + // code. + let original = self.renames.clone(); + self.renames + .sort_by(|lhs, rhs| lhs.since.partial_cmp(&rhs.since).unwrap_or(Ordering::Equal)); + + for (index, version) in original.iter().enumerate() { + if *version.since == *self.renames.get(index).unwrap().since { + continue; + } + + return Err(Error::custom(format!( + "version of renames must be defined in ascending order (version `{}` is misplaced)", + *version.since + )) + .with_span(&self.ident.span())); + } + + // TODO (@Techassi): Add validation for renames so that renamed fields + // match up and form a continous chain (eg. foo -> bar -> baz). + + // TODO (@Techassi): Add hint if a field is added in the first version + // that it might be clever to remove the 'added' attribute. + // Validate that actions use chronologically valid versions. If the // field was added (not included from the start), the version must be // less than version from the renamed and deprecated actions. @@ -96,7 +122,7 @@ impl FieldAttributes { let deprecated_version = self.deprecated.as_ref().map(|d| *d.since); if let Some(added_version) = added_version { - if !self.renamed.iter().all(|r| *r.since > added_version) { + if !self.renames.iter().all(|r| *r.since > added_version) { return Err(Error::custom(format!( "field was marked as `added` in version `{}` and thus all renames must use a higher version", added_version @@ -115,17 +141,28 @@ impl FieldAttributes { } } - // TODO (@Techassi): Add validation for renames so that renamed fields - // match up and form a continous chain (eg. foo -> bar -> baz). - // The same rule applies to renamed fields. Versions of renames must be // less than the deprecation version (if any). if let Some(deprecated_version) = deprecated_version { - if !self.renamed.iter().all(|r| *r.since < deprecated_version) { + if !self.renames.iter().all(|r| *r.since < deprecated_version) { return Err(Error::custom(format!( "field was marked as `deprecated` in version `{}` and thus all renames must use a lower version", deprecated_version - ))); + )).with_span(&self.ident.span())); + } + + // Also check if the field starts with the prefix 'deprecated_'. + if !self + .ident + .as_ref() + .unwrap() + .to_string() + .starts_with(DEPRECATED_PREFIX) + { + return Err(Error::custom( + "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name", + ) + .with_span(&self.ident.span())); } } @@ -134,28 +171,39 @@ impl FieldAttributes { pub(crate) fn check_versions( &self, - versions: &[ContainerVersion], + container_attrs: &ContainerAttributes, field: &Field, ) -> Result<(), Error> { // NOTE (@Techassi): Can we maybe optimize this a little? if let Some(added) = &self.added { - if !versions.iter().any(|v| v.inner == *added.since) { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *added.since) + { return Err( Error::custom("field action `added` uses version which was not declared via #[versioned(version)]") - .with_span(&field.ident.as_ref().expect("internal: field must have name").span() - )); + .with_span(&field.ident.span())); } } - for rename in &self.renamed { - if !versions.iter().any(|v| v.inner == *rename.since) { + for rename in &self.renames { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *rename.since) + { return Err(Error::custom("field action `renamed` uses version which was not declared via #[versioned(version)]")); } } if let Some(deprecated) = &self.deprecated { - if !versions.iter().any(|v| v.inner == *deprecated.since) { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *deprecated.since) + { return Err(Error::custom("field action `deprecated` uses version which was not declared via #[versioned(version)]")); } } diff --git a/crates/stackable-versioned/src/consts.rs b/crates/stackable-versioned/src/consts.rs new file mode 100644 index 000000000..bb0ff076b --- /dev/null +++ b/crates/stackable-versioned/src/consts.rs @@ -0,0 +1 @@ +pub(crate) const DEPRECATED_PREFIX: &str = "deprecated_"; diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index f6f176c60..d56fe821e 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,121 +1,268 @@ +use std::collections::HashMap; + +use darling::Error; +use k8s_version::Version; use proc_macro2::TokenStream; -use syn::Field; +use quote::{format_ident, quote}; +use syn::{Field, Ident}; use crate::{ attrs::field::FieldAttributes, + consts::DEPRECATED_PREFIX, gen::{version::ContainerVersion, ToTokensExt}, }; +// match &self.actions { +// FieldAction::Added(added) => { +// // Skip generating the field, if the current generated +// // version appears before the field action version. +// if version.inner < *added.since { +// return None; +// } + +// let field_name = &self.inner.ident; +// let field_type = &self.inner.ty; +// let doc = format!(" Added since `{}`.", *added.since); + +// // TODO (@Techassi): Also forward other attributes +// let doc_attrs: Vec<&Attribute> = self +// .inner +// .attrs +// .iter() +// .filter(|a| a.path().is_ident("doc")) +// .collect(); + +// Some(quote! { +// #(#doc_attrs)* +// #[doc = ""] +// #[doc = #doc] +// pub #field_name: #field_type, +// }) +// } +// FieldAction::Renamed(renamed) => { +// if version.inner < *renamed.since { +// // Use the original name for versions before the field action +// // version. +// let field_name = format_ident!("{}", *renamed.from); +// let field_type = &self.inner.ty; + +// Some(quote! { +// pub #field_name: #field_type, +// }) +// } else { +// // If the version is greater than the field action version +// // (or equal), use the new field name. +// let field_name = &self.inner.ident; +// let field_type = &self.inner.ty; + +// Some(quote! { +// pub #field_name: #field_type, +// }) +// } +// } +// FieldAction::Deprecated(deprecated) => { +// if version.inner < *deprecated.since { +// // Remove the deprecated_ prefix from the field name and use +// // it as the field name if the version is less than the +// // field action version. +// let field_name = format_ident!( +// "{}", +// &self +// .inner +// .ident +// .as_ref() +// .unwrap() +// .to_string() +// .replace("deprecated_", "") +// ); +// let field_type = &self.inner.ty; + +// Some(quote! { +// pub #field_name: #field_type, +// }) +// } else { +// // If the version is greater than the field action version +// // (or equal), use the prefixed field name. +// let field_name = &self.inner.ident; +// let field_type = &self.inner.ty; +// let deprecated_note = &*deprecated.note; + +// Some(quote! { +// #[deprecated = #deprecated_note] +// pub #field_name: #field_type, +// }) +// } +// } +// FieldAction::None => { +// // Generate fields without any attributes in every version. +// let field_name = &self.inner.ident; +// let field_type = &self.inner.ty; + +// Some(quote! { +// pub #field_name: #field_type, +// }) +// } +// } + +#[derive(Debug)] pub(crate) struct VersionedField { - // TODO (@Techassi): There can be multiple actions for one field (in - // different versions). Add support for that here. - pub(crate) _attrs: FieldAttributes, - pub(crate) _inner: Field, + chain: Option>, + inner: Field, } impl ToTokensExt for VersionedField { - fn to_tokens_for_version(&self, _version: &ContainerVersion) -> Option { - // match &self.actions { - // FieldAction::Added(added) => { - // // Skip generating the field, if the current generated - // // version appears before the field action version. - // if version.inner < *added.since { - // return None; - // } - - // let field_name = &self.inner.ident; - // let field_type = &self.inner.ty; - // let doc = format!(" Added since `{}`.", *added.since); - - // // TODO (@Techassi): Also forward other attributes - // let doc_attrs: Vec<&Attribute> = self - // .inner - // .attrs - // .iter() - // .filter(|a| a.path().is_ident("doc")) - // .collect(); - - // Some(quote! { - // #(#doc_attrs)* - // #[doc = ""] - // #[doc = #doc] - // pub #field_name: #field_type, - // }) - // } - // FieldAction::Renamed(renamed) => { - // if version.inner < *renamed.since { - // // Use the original name for versions before the field action - // // version. - // let field_name = format_ident!("{}", *renamed.from); - // let field_type = &self.inner.ty; - - // Some(quote! { - // pub #field_name: #field_type, - // }) - // } else { - // // If the version is greater than the field action version - // // (or equal), use the new field name. - // let field_name = &self.inner.ident; - // let field_type = &self.inner.ty; - - // Some(quote! { - // pub #field_name: #field_type, - // }) - // } - // } - // FieldAction::Deprecated(deprecated) => { - // if version.inner < *deprecated.since { - // // Remove the deprecated_ prefix from the field name and use - // // it as the field name if the version is less than the - // // field action version. - // let field_name = format_ident!( - // "{}", - // &self - // .inner - // .ident - // .as_ref() - // .unwrap() - // .to_string() - // .replace("deprecated_", "") - // ); - // let field_type = &self.inner.ty; - - // Some(quote! { - // pub #field_name: #field_type, - // }) - // } else { - // // If the version is greater than the field action version - // // (or equal), use the prefixed field name. - // let field_name = &self.inner.ident; - // let field_type = &self.inner.ty; - // let deprecated_note = &*deprecated.note; - - // Some(quote! { - // #[deprecated = #deprecated_note] - // pub #field_name: #field_type, - // }) - // } - // } - // FieldAction::None => { - // // Generate fields without any attributes in every version. - // let field_name = &self.inner.ident; - // let field_type = &self.inner.ty; - - // Some(quote! { - // pub #field_name: #field_type, - // }) - // } - // } - None - // todo!() + fn to_tokens_for_version(&self, version: &ContainerVersion) -> Option { + match &self.chain { + Some(chain) => { + // Check if the provided container version is present in the map + // of actions. If it is, some action occured in exactly that + // version and thus code is generated for that field based on + // the type of action. + // If not, the provided version has no action attached to it. + // The code generation then depends on the relation to other + // versions (with actions). + match chain + .get(&version.inner) + .expect("internal: there must be a status for each version") + { + FieldStatus::Added(_) => { + let field_ident = &self.inner.ident; + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_ident: #field_type, + }) + } + FieldStatus::Renamed(_) => todo!(), + FieldStatus::Deprecated(_) => todo!(), + FieldStatus::NotPresent => todo!(), + FieldStatus::None => todo!(), + } + } + None => { + // If there is no chain of field actions, the field is not + // versioned and code generation is straight forward. + // Unversioned fields are always included in versioned structs. + let field_ident = &self.inner.ident; + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_ident: #field_type, + }) + } + } } } impl VersionedField { - pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Self { - Self { - _inner: field, - _attrs: attrs, + pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Result { + // Constructing the change chain requires going through the actions from + // the end, because the base struct allways represents the latest (most + // up-to-date) version of that struct. That's why the following code + // needs to go through the changes in reverse order, as otherwise it is + // impossible to extract the field ident for each version. + + // Deprecating a field is always the last status a field can up in. For + // fields which are not deprecated, the last change is either the latest + // rename or addition, which is handled below. + // The ident of the deprecated field is guaranteed to include the + // 'deprecated_' prefix. The ident can thus be used as is. + if let Some(deprecated) = attrs.deprecated { + let mut actions = HashMap::new(); + + let ident = field.ident.as_ref().unwrap(); + actions.insert(*deprecated.since, FieldStatus::Deprecated(ident.clone())); + + // When the field is deprecated, any rename which occured beforehand + // requires access to the field ident to infer the field ident for + // the latest rename. + let mut ident = format_ident!("{}", ident.to_string().replace(DEPRECATED_PREFIX, "")); + + for rename in attrs.renames.iter().rev() { + actions.insert(*rename.since, FieldStatus::Renamed(ident)); + ident = format_ident!("{}", *rename.from) + } + + // After the last iteration above (if any) we use the ident for the + // added action if there is any. + if let Some(added) = attrs.added { + actions.insert(*added.since, FieldStatus::Added(ident)); + } + + return Ok(Self { + chain: Some(actions), + inner: field, + }); + } else { + if !attrs.renames.is_empty() { + let mut actions = HashMap::new(); + let mut ident = format_ident!("{}", *attrs.renames.last().unwrap().from); + + for rename in attrs.renames.iter().rev() { + actions.insert(*rename.since, FieldStatus::Renamed(ident)); + ident = format_ident!("{}", *rename.from) + } + + // After the last iteration above (if any) we use the ident for the + // added action if there is any. + if let Some(added) = attrs.added { + actions.insert(*added.since, FieldStatus::Added(ident)); + } + + return Ok(Self { + chain: Some(actions), + inner: field, + }); + } else { + if let Some(added) = attrs.added { + let mut actions = HashMap::new(); + + actions.insert( + *added.since, + FieldStatus::Added(field.ident.clone().unwrap()), + ); + + return Ok(Self { + chain: Some(actions), + inner: field, + }); + } + + return Ok(Self { + chain: None, + inner: field, + }); + } } } + + /// Extend the already recorded actions with actions based on global + /// container versions to construct a complete chain of actions for each + /// field. + pub(crate) fn extend_with_container_versions(&mut self, _versions: &[ContainerVersion]) { + // When creating this type via the new function, only directly attached + // action can be inserted into the chain of actions. It doesn't contain + // any actions based on the container versions. A quick example: + // + // Let's assume we have the following declared versions: v1, v2, v3, v4. + // One field, let's call it foo, has two actions attached: added in v2 + // and deprecated in v3. So initially, the chain of actions only contain + // two actions: added(v2) and deprecated(v3). But what happened to the + // field in v1 and v4. This information can only be included in the + // chain by looking at the container versions. In this particular + // example the field wasn't present in v1 and isnt' present from v4 and + // onward. This action (or state) needs to be included in the chain of + // actions. The resulting chain now looks like: not-present(v1), + // added(v2), deprecated(v3), not-present(v4). + todo!() + } +} + +#[derive(Debug)] +pub(crate) enum FieldStatus { + Added(Ident), + Renamed(Ident), + Deprecated(Ident), + NotPresent, + None, } diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs index 948a65dc3..fe9ab80f3 100644 --- a/crates/stackable-versioned/src/gen/version.rs +++ b/crates/stackable-versioned/src/gen/version.rs @@ -1,5 +1,6 @@ use k8s_version::Version; +#[derive(Debug)] pub(crate) struct ContainerVersion { pub(crate) _deprecated: bool, pub(crate) inner: Version, diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 82fab5182..9e9145c10 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -12,6 +12,7 @@ use crate::{ /// actions, which describe if the field was added, renamed or deprecated in /// that version. Fields which are not versioned, are included in every /// version of the struct. +#[derive(Debug)] pub(crate) struct VersionedStruct { pub(crate) _ident: Ident, @@ -27,7 +28,7 @@ impl ToTokens for VersionedStruct { let mut fields = TokenStream::new(); for field in &self._fields { - fields.extend(field.to_tokens_for_version(version)) + fields.extend(field.to_tokens_for_version(version)); } // TODO (@Techassi): Make the generation of the module optional to @@ -37,22 +38,17 @@ impl ToTokens for VersionedStruct { let module_name = format_ident!("{}", version.inner.to_string()); let struct_name = &self._ident; - _tokens.extend(quote! { - pub mod #module_name { - pub struct #struct_name { - #fields - } - } - }); - - // If there is no next version, we know we just generated the latest - // version and thus we can add the 'latest' module. - if versions.peek().is_none() { + // Only genereate a module when there is at least one more version. + // This skips generating a module for the latest version, because + // the base struct always represents the latest version. + if versions.peek().is_some() { _tokens.extend(quote! { - pub mod latest { - pub use super::#module_name::*; + pub mod #module_name { + pub struct #struct_name { + #fields + } } - }) + }); } } } @@ -64,41 +60,32 @@ impl VersionedStruct { data: DataStruct, attributes: ContainerAttributes, ) -> Result { - // First, collect all declared versions and map them into a Version - // struct. - let versions = attributes - .versions - .iter() - .map(|v| { - let deprecated = v.deprecated.is_present(); - - ContainerVersion { - _deprecated: deprecated, - inner: v.name, - } - }) - .collect::>(); - - let mut fields = Vec::new(); + let mut versioned_fields = Vec::new(); + // Extract the field attributes for every field from the raw token + // stream and also validate that each field action version uses a + // version declared by the container attribute. for field in data.fields { - // Iterate over all fields of the struct and gather field attributes. - // Next, make sure only valid combinations of field actions are - // declared. Using the action and the field data, a VersionField - // can be created. - let field_attributes = FieldAttributes::from_field(&field)?; + let attrs = FieldAttributes::from_field(&field)?; + attrs.check_versions(&attributes, &field)?; - // Validate, that the field action uses a version which is declared - // by the container attribute. If there is no attribute attached to - // the field, it is also valid. - field_attributes.check_versions(&versions, &field)?; - fields.push(VersionedField::new(field, field_attributes)); + let versioned_field = VersionedField::new(field, attrs)?; + versioned_fields.push(versioned_field); } - Ok(Self { - _versions: versions, - _fields: fields, + let versions = attributes + .versions + .iter() + .map(|v| ContainerVersion { + _deprecated: v.deprecated.is_present(), + inner: v.name, + }) + .collect(); + + return Ok(Self { _ident: ident, - }) + _versions: versions, + _fields: versioned_fields, + }); } } diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs index 14892bebb..33aeecf67 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -2,6 +2,7 @@ use proc_macro::TokenStream; use syn::{DeriveInput, Error}; mod attrs; +mod consts; mod gen; #[proc_macro_derive(Versioned, attributes(versioned))] diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index bff38290c..13a7a9746 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -5,12 +5,18 @@ use stackable_versioned::Versioned; #[versioned( version(name = "v1alpha1"), version(name = "v1beta1"), - version(name = "v1") + version(name = "v1"), + version(name = "v2alpha1") )] struct Foo { /// My docs - #[versioned(added(since = "v1beta1"))] - bar: usize, + #[versioned( + added(since = "v1alpha1"), + // renamed(since = "v1beta1", from = "jjj"), + // renamed(since = "v1", from = "yyy"), + // deprecated(since = "v2alpha1", _note = "") + )] + deprecated_bar: usize, baz: bool, } From f3515e269d3a70c8040b2cd48d24797ee35f0d0d Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 29 Apr 2024 14:47:33 +0200 Subject: [PATCH 21/43] Add Ord impl for Level and Version --- crates/k8s-version/src/level.rs | 14 ++++++++++---- crates/k8s-version/src/version.rs | 18 ++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index 15f2ec38d..0444691ab 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -65,14 +65,20 @@ impl FromStr for Level { impl PartialOrd for Level { fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Level { + fn cmp(&self, other: &Self) -> Ordering { match self { Level::Beta(sb) => match other { - Level::Beta(ob) => sb.partial_cmp(ob), - Level::Alpha(_) => Some(Ordering::Greater), + Level::Beta(ob) => sb.cmp(ob), + Level::Alpha(_) => Ordering::Greater, }, Level::Alpha(sa) => match other { - Level::Beta(_) => Some(Ordering::Less), - Level::Alpha(oa) => sa.partial_cmp(oa), + Level::Beta(_) => Ordering::Less, + Level::Alpha(oa) => sa.cmp(oa), }, } } diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index eb20279d2..36b1e4be2 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -71,16 +71,22 @@ impl FromStr for Version { impl PartialOrd for Version { fn partial_cmp(&self, other: &Self) -> Option { - match self.major.partial_cmp(&other.major) { - Some(Ordering::Equal) => {} + Some(self.cmp(other)) + } +} + +impl Ord for Version { + fn cmp(&self, other: &Self) -> Ordering { + match self.major.cmp(&other.major) { + Ordering::Equal => {} ord => return ord, } match (&self.level, &other.level) { - (Some(lhs), Some(rhs)) => lhs.partial_cmp(rhs), - (Some(_), None) => Some(Ordering::Less), - (None, Some(_)) => Some(Ordering::Greater), - (None, None) => Some(Ordering::Equal), + (Some(lhs), Some(rhs)) => lhs.cmp(rhs), + (Some(_), None) => Ordering::Less, + (None, Some(_)) => Ordering::Greater, + (None, None) => Ordering::Equal, } } } From e324d30b369d009a78fba78d5528dd126d86325c Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 29 Apr 2024 15:02:44 +0200 Subject: [PATCH 22/43] Add Part(Ord) unit tests for Level and Version --- Cargo.toml | 1 + crates/k8s-version/Cargo.toml | 1 + crates/k8s-version/src/level.rs | 12 +++++++++++- crates/k8s-version/src/lib.rs | 5 +++++ crates/k8s-version/src/version.rs | 22 ++++++++++++++++++++-- 5 files changed, 38 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6c2b394a1..83f92cbf9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,6 +46,7 @@ rand_core = "0.6.4" regex = "1.10.4" rsa = { version = "0.9.6", features = ["sha2"] } rstest = "0.19.0" +rstest_reuse = "0.6.0" schemars = { version = "0.8.16", features = ["url"] } semver = "1.0.22" serde = { version = "1.0.198", features = ["derive"] } diff --git a/crates/k8s-version/Cargo.toml b/crates/k8s-version/Cargo.toml index 404925e03..94f4da540 100644 --- a/crates/k8s-version/Cargo.toml +++ b/crates/k8s-version/Cargo.toml @@ -17,6 +17,7 @@ snafu.workspace = true [dev-dependencies] rstest.workspace = true +rstest_reuse.workspace = true quote.workspace = true proc-macro2.workspace = true syn.workspace = true diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index 0444691ab..c6066181d 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -155,9 +155,11 @@ impl FromMeta for Level { #[cfg(test)] mod test { use rstest::rstest; + use rstest_reuse::*; use super::*; + #[template] #[rstest] #[case(Level::Beta(1), Level::Alpha(1), Ordering::Greater)] #[case(Level::Alpha(1), Level::Beta(1), Ordering::Less)] @@ -167,7 +169,15 @@ mod test { #[case(Level::Beta(2), Level::Beta(1), Ordering::Greater)] #[case(Level::Beta(2), Level::Beta(2), Ordering::Equal)] #[case(Level::Beta(1), Level::Beta(2), Ordering::Less)] - fn partial_ord(#[case] input: Level, #[case] other: Level, #[case] expected: Ordering) { + fn ord_cases(#[case] input: Level, #[case] other: Level, #[case] expected: Ordering) {} + + #[apply(ord_cases)] + fn ord(input: Level, other: Level, expected: Ordering) { + assert_eq!(input.cmp(&other), expected) + } + + #[apply(ord_cases)] + fn partial_ord(input: Level, other: Level, expected: Ordering) { assert_eq!(input.partial_cmp(&other), Some(expected)) } } diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 5cc5a9f28..087a4ec9d 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -1,3 +1,8 @@ +// NOTE (@Techassi): Fixed in https://github.com/la10736/rstest/pull/244 but not +// yet released. +#[cfg(test)] +use rstest_reuse; + mod api_version; mod level; mod version; diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 36b1e4be2..3567b094f 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -118,9 +118,10 @@ impl Version { #[cfg(test)] mod test { - use super::*; - use rstest::rstest; + use rstest_reuse::{apply, template}; + + use super::*; #[cfg(feature = "darling")] use quote::quote; @@ -131,6 +132,13 @@ mod test { Ok(attribute.meta) } + #[template] + #[rstest] + #[case(Version {major: 1, level: Some(Level::Beta(1))}, Version {major: 1, level: Some(Level::Alpha(1))}, Ordering::Greater)] + #[case(Version {major: 1, level: Some(Level::Alpha(1))}, Version {major: 1, level: Some(Level::Beta(1))}, Ordering::Less)] + #[case(Version {major: 1, level: Some(Level::Beta(1))}, Version {major: 1, level: Some(Level::Beta(1))}, Ordering::Equal)] + fn ord_cases(#[case] input: Version, #[case] other: Version, #[case] expected: Ordering) {} + #[rstest] #[case("v1alpha12", Version { major: 1, level: Some(Level::Alpha(12)) })] #[case("v1alpha1", Version { major: 1, level: Some(Level::Alpha(1)) })] @@ -151,6 +159,16 @@ mod test { assert_eq!(err, error) } + #[apply(ord_cases)] + fn ord(input: Version, other: Version, expected: Ordering) { + assert_eq!(input.cmp(&other), expected) + } + + #[apply(ord_cases)] + fn partial_ord(input: Version, other: Version, expected: Ordering) { + assert_eq!(input.partial_cmp(&other), Some(expected)) + } + #[cfg(feature = "darling")] #[rstest] #[case(quote!(ignore = "v1alpha12"), Version { major: 1, level: Some(Level::Alpha(12)) })] From 3fa20f4ca3ec9977d74e06dd1069d3f69e8f84f1 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 29 Apr 2024 15:03:31 +0200 Subject: [PATCH 23/43] Add FromMeta unit test for Level --- crates/k8s-version/src/level.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index c6066181d..e755dba84 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -159,6 +159,15 @@ mod test { use super::*; + #[cfg(feature = "darling")] + use quote::quote; + + #[cfg(feature = "darling")] + fn parse_meta(tokens: proc_macro2::TokenStream) -> ::std::result::Result { + let attribute: syn::Attribute = syn::parse_quote!(#[#tokens]); + Ok(attribute.meta) + } + #[template] #[rstest] #[case(Level::Beta(1), Level::Alpha(1), Ordering::Greater)] @@ -180,4 +189,15 @@ mod test { fn partial_ord(input: Level, other: Level, expected: Ordering) { assert_eq!(input.partial_cmp(&other), Some(expected)) } + + #[cfg(feature = "darling")] + #[rstest] + #[case(quote!(ignore = "alpha12"), Level::Alpha(12))] + #[case(quote!(ignore = "alpha1"), Level::Alpha(1))] + #[case(quote!(ignore = "beta1"), Level::Beta(1))] + fn from_meta(#[case] input: proc_macro2::TokenStream, #[case] expected: Level) { + let meta = parse_meta(input).expect("valid attribute tokens"); + let version = Level::from_meta(&meta).expect("level must parse from attribute"); + assert_eq!(version, expected); + } } From bd935d6a0a607baeca0f6d3efa191a06537a7f75 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 30 Apr 2024 11:07:56 +0200 Subject: [PATCH 24/43] Generate code for multiple field actions --- crates/k8s-version/src/lib.rs | 2 +- crates/stackable-versioned/src/gen/field.rs | 290 +++++++++--------- crates/stackable-versioned/src/gen/version.rs | 2 +- crates/stackable-versioned/src/gen/vstruct.rs | 10 +- crates/stackable-versioned/tests/basic.rs | 14 +- 5 files changed, 154 insertions(+), 164 deletions(-) diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 087a4ec9d..b1a8f3d01 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -1,7 +1,7 @@ // NOTE (@Techassi): Fixed in https://github.com/la10736/rstest/pull/244 but not // yet released. #[cfg(test)] -use rstest_reuse; +use rstest_reuse::{self}; mod api_version; mod level; diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index d56fe821e..d0e66145b 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::BTreeMap; use darling::Error; use k8s_version::Version; @@ -12,106 +12,14 @@ use crate::{ gen::{version::ContainerVersion, ToTokensExt}, }; -// match &self.actions { -// FieldAction::Added(added) => { -// // Skip generating the field, if the current generated -// // version appears before the field action version. -// if version.inner < *added.since { -// return None; -// } - -// let field_name = &self.inner.ident; -// let field_type = &self.inner.ty; -// let doc = format!(" Added since `{}`.", *added.since); - -// // TODO (@Techassi): Also forward other attributes -// let doc_attrs: Vec<&Attribute> = self -// .inner -// .attrs -// .iter() -// .filter(|a| a.path().is_ident("doc")) -// .collect(); - -// Some(quote! { -// #(#doc_attrs)* -// #[doc = ""] -// #[doc = #doc] -// pub #field_name: #field_type, -// }) -// } -// FieldAction::Renamed(renamed) => { -// if version.inner < *renamed.since { -// // Use the original name for versions before the field action -// // version. -// let field_name = format_ident!("{}", *renamed.from); -// let field_type = &self.inner.ty; - -// Some(quote! { -// pub #field_name: #field_type, -// }) -// } else { -// // If the version is greater than the field action version -// // (or equal), use the new field name. -// let field_name = &self.inner.ident; -// let field_type = &self.inner.ty; - -// Some(quote! { -// pub #field_name: #field_type, -// }) -// } -// } -// FieldAction::Deprecated(deprecated) => { -// if version.inner < *deprecated.since { -// // Remove the deprecated_ prefix from the field name and use -// // it as the field name if the version is less than the -// // field action version. -// let field_name = format_ident!( -// "{}", -// &self -// .inner -// .ident -// .as_ref() -// .unwrap() -// .to_string() -// .replace("deprecated_", "") -// ); -// let field_type = &self.inner.ty; - -// Some(quote! { -// pub #field_name: #field_type, -// }) -// } else { -// // If the version is greater than the field action version -// // (or equal), use the prefixed field name. -// let field_name = &self.inner.ident; -// let field_type = &self.inner.ty; -// let deprecated_note = &*deprecated.note; - -// Some(quote! { -// #[deprecated = #deprecated_note] -// pub #field_name: #field_type, -// }) -// } -// } -// FieldAction::None => { -// // Generate fields without any attributes in every version. -// let field_name = &self.inner.ident; -// let field_type = &self.inner.ty; - -// Some(quote! { -// pub #field_name: #field_type, -// }) -// } -// } - #[derive(Debug)] pub(crate) struct VersionedField { - chain: Option>, + chain: Option>, inner: Field, } impl ToTokensExt for VersionedField { - fn to_tokens_for_version(&self, version: &ContainerVersion) -> Option { + fn to_tokens_for_version(&self, container_version: &ContainerVersion) -> Option { match &self.chain { Some(chain) => { // Check if the provided container version is present in the map @@ -121,22 +29,87 @@ impl ToTokensExt for VersionedField { // If not, the provided version has no action attached to it. // The code generation then depends on the relation to other // versions (with actions). - match chain - .get(&version.inner) - .expect("internal: there must be a status for each version") - { - FieldStatus::Added(_) => { - let field_ident = &self.inner.ident; - let field_type = &self.inner.ty; - - Some(quote! { - pub #field_ident: #field_type, - }) + match chain.get(&container_version.inner) { + Some(action) => match action { + FieldStatus::Added(field_ident) => { + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_ident: #field_type, + }) + } + FieldStatus::Renamed { from: _, to } => { + let field_type = &self.inner.ty; + + Some(quote! { + pub #to: #field_type, + }) + } + FieldStatus::Deprecated(field_ident) => { + let field_type = &self.inner.ty; + + Some(quote! { + #[deprecated] + pub #field_ident: #field_type, + }) + } + }, + None => { + if let Some((version, action)) = chain.first_key_value() { + if container_version.inner < *version { + match action { + FieldStatus::Added(_) => return None, + FieldStatus::Renamed { from, to: _ } => { + let field_type = &self.inner.ty; + + return Some(quote! { + pub #from: #field_type, + }); + } + FieldStatus::Deprecated(field_ident) => { + let field_type = &self.inner.ty; + + return Some(quote! { + pub #field_ident: #field_type, + }); + } + } + } + } + + if let Some((version, action)) = chain.last_key_value() { + if container_version.inner > *version { + match action { + FieldStatus::Added(field_ident) => { + let field_type = &self.inner.ty; + + return Some(quote! { + pub #field_ident: #field_type, + }); + } + FieldStatus::Renamed { from: _, to } => { + let field_type = &self.inner.ty; + + return Some(quote! { + pub #to: #field_type, + }); + } + FieldStatus::Deprecated(field_ident) => { + let field_type = &self.inner.ty; + + return Some(quote! { + #[deprecated] + pub #field_ident: #field_type, + }); + } + } + } + } + + // TODO (@Techassi): Handle versions which are in between + // versions defined in field actions. + None } - FieldStatus::Renamed(_) => todo!(), - FieldStatus::Deprecated(_) => todo!(), - FieldStatus::NotPresent => todo!(), - FieldStatus::None => todo!(), } } None => { @@ -168,7 +141,7 @@ impl VersionedField { // The ident of the deprecated field is guaranteed to include the // 'deprecated_' prefix. The ident can thus be used as is. if let Some(deprecated) = attrs.deprecated { - let mut actions = HashMap::new(); + let mut actions = BTreeMap::new(); let ident = field.ident.as_ref().unwrap(); actions.insert(*deprecated.since, FieldStatus::Deprecated(ident.clone())); @@ -179,8 +152,15 @@ impl VersionedField { let mut ident = format_ident!("{}", ident.to_string().replace(DEPRECATED_PREFIX, "")); for rename in attrs.renames.iter().rev() { - actions.insert(*rename.since, FieldStatus::Renamed(ident)); - ident = format_ident!("{}", *rename.from) + let from = format_ident!("{}", *rename.from); + actions.insert( + *rename.since, + FieldStatus::Renamed { + from: from.clone(), + to: ident, + }, + ); + ident = from; } // After the last iteration above (if any) we use the ident for the @@ -189,80 +169,84 @@ impl VersionedField { actions.insert(*added.since, FieldStatus::Added(ident)); } - return Ok(Self { + Ok(Self { chain: Some(actions), inner: field, - }); - } else { - if !attrs.renames.is_empty() { - let mut actions = HashMap::new(); - let mut ident = format_ident!("{}", *attrs.renames.last().unwrap().from); + }) + } else if !attrs.renames.is_empty() { + let mut actions = BTreeMap::new(); + let mut ident = field.ident.clone().unwrap(); - for rename in attrs.renames.iter().rev() { - actions.insert(*rename.since, FieldStatus::Renamed(ident)); - ident = format_ident!("{}", *rename.from) - } + for rename in attrs.renames.iter().rev() { + let from = format_ident!("{}", *rename.from); + actions.insert( + *rename.since, + FieldStatus::Renamed { + from: from.clone(), + to: ident, + }, + ); + ident = from; + } - // After the last iteration above (if any) we use the ident for the - // added action if there is any. - if let Some(added) = attrs.added { - actions.insert(*added.since, FieldStatus::Added(ident)); - } + // After the last iteration above (if any) we use the ident for the + // added action if there is any. + if let Some(added) = attrs.added { + actions.insert(*added.since, FieldStatus::Added(ident)); + } - return Ok(Self { - chain: Some(actions), - inner: field, - }); - } else { - if let Some(added) = attrs.added { - let mut actions = HashMap::new(); + dbg!(&actions); - actions.insert( - *added.since, - FieldStatus::Added(field.ident.clone().unwrap()), - ); + Ok(Self { + chain: Some(actions), + inner: field, + }) + } else { + if let Some(added) = attrs.added { + let mut actions = BTreeMap::new(); - return Ok(Self { - chain: Some(actions), - inner: field, - }); - } + actions.insert( + *added.since, + FieldStatus::Added(field.ident.clone().unwrap()), + ); return Ok(Self { - chain: None, + chain: Some(actions), inner: field, }); } + + Ok(Self { + chain: None, + inner: field, + }) } } /// Extend the already recorded actions with actions based on global /// container versions to construct a complete chain of actions for each /// field. - pub(crate) fn extend_with_container_versions(&mut self, _versions: &[ContainerVersion]) { + pub(crate) fn _extend_with_container_versions(&mut self, _versions: &[ContainerVersion]) { // When creating this type via the new function, only directly attached // action can be inserted into the chain of actions. It doesn't contain // any actions based on the container versions. A quick example: // // Let's assume we have the following declared versions: v1, v2, v3, v4. // One field, let's call it foo, has two actions attached: added in v2 - // and deprecated in v3. So initially, the chain of actions only contain + // and deprecated in v3. So initially, the chain of actions only contains // two actions: added(v2) and deprecated(v3). But what happened to the - // field in v1 and v4. This information can only be included in the + // field in v1 and v4? This information can only be included in the // chain by looking at the container versions. In this particular - // example the field wasn't present in v1 and isnt' present from v4 and + // example the field wasn't present in v1 and isn't present from v4 and // onward. This action (or state) needs to be included in the chain of // actions. The resulting chain now looks like: not-present(v1), // added(v2), deprecated(v3), not-present(v4). - todo!() } } #[derive(Debug)] pub(crate) enum FieldStatus { Added(Ident), - Renamed(Ident), + Renamed { from: Ident, to: Ident }, Deprecated(Ident), - NotPresent, - None, } diff --git a/crates/stackable-versioned/src/gen/version.rs b/crates/stackable-versioned/src/gen/version.rs index fe9ab80f3..ca5d7f0f5 100644 --- a/crates/stackable-versioned/src/gen/version.rs +++ b/crates/stackable-versioned/src/gen/version.rs @@ -2,6 +2,6 @@ use k8s_version::Version; #[derive(Debug)] pub(crate) struct ContainerVersion { - pub(crate) _deprecated: bool, + pub(crate) deprecated: bool, pub(crate) inner: Version, } diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 9e9145c10..4dba7937e 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -35,6 +35,7 @@ impl ToTokens for VersionedStruct { // enable the attribute macro to be applied to a module which // generates versioned versions of all contained containers. + let deprecated_attr = version.deprecated.then_some(quote! {#[deprecated]}); let module_name = format_ident!("{}", version.inner.to_string()); let struct_name = &self._ident; @@ -43,7 +44,10 @@ impl ToTokens for VersionedStruct { // the base struct always represents the latest version. if versions.peek().is_some() { _tokens.extend(quote! { + #[automatically_derived] + #deprecated_attr pub mod #module_name { + pub struct #struct_name { #fields } @@ -77,15 +81,15 @@ impl VersionedStruct { .versions .iter() .map(|v| ContainerVersion { - _deprecated: v.deprecated.is_present(), + deprecated: v.deprecated.is_present(), inner: v.name, }) .collect(); - return Ok(Self { + Ok(Self { _ident: ident, _versions: versions, _fields: versioned_fields, - }); + }) } } diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 13a7a9746..7a9078eca 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -4,17 +4,19 @@ use stackable_versioned::Versioned; #[allow(dead_code)] #[versioned( version(name = "v1alpha1"), - version(name = "v1beta1"), + version(name = "v1beta1", deprecated), + version(name = "v1beta2"), version(name = "v1"), - version(name = "v2alpha1") + version(name = "v2alpha1"), + version(name = "v2") )] struct Foo { /// My docs #[versioned( - added(since = "v1alpha1"), - // renamed(since = "v1beta1", from = "jjj"), - // renamed(since = "v1", from = "yyy"), - // deprecated(since = "v2alpha1", _note = "") + added(since = "v1beta1"), + renamed(since = "v1beta2", from = "jjj"), + renamed(since = "v1", from = "ppp"), + deprecated(since = "v2alpha1", _note = "") )] deprecated_bar: usize, baz: bool, From a9eeafd614042f06e925d232d61d4d9294ebe522 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 30 Apr 2024 14:46:00 +0200 Subject: [PATCH 25/43] Improve field attribute validation --- crates/stackable-versioned/src/attrs/field.rs | 166 ++++++++---------- crates/stackable-versioned/tests/basic.rs | 9 +- 2 files changed, 78 insertions(+), 97 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 8fc073342..b61921e8e 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,5 +1,3 @@ -use std::cmp::Ordering; - use darling::{util::SpannedValue, Error, FromField, FromMeta}; use k8s_version::Version; use syn::{spanned::Spanned, Field, Ident}; @@ -57,116 +55,104 @@ pub(crate) struct DeprecatedAttributes { } impl FieldAttributes { - fn validate(mut self) -> Result { + fn validate(self) -> Result { + self.validate_action_combinations()?; + // self.validate_rename_order()?; + + // TODO (@Techassi): Add validation for renames so that renamed fields + // match up and form a continous chain (eg. foo -> bar -> baz). + + // TODO (@Techassi): Add hint if a field is added in the first version + // that it might be clever to remove the 'added' attribute. + + self.validate_action_order()?; + self.validate_field_name()?; + + Ok(self) + } + + fn validate_action_combinations(&self) -> Result<(), Error> { match (&self.added, &self.renames, &self.deprecated) { // The derive macro prohibits the use of the 'added' and 'deprecated' // field action using the same version. This is because it doesn't // make sense to add a field and immediatly mark that field as // deprecated in the same version. Instead, fields should be // deprecated at least one version later. - (Some(added), _, Some(deprecated)) => { - if *added.since == *deprecated.since { - return Err(Error::custom( - "field cannot be marked as `added` and `deprecated` in the same version", - ) - .with_span(&self.ident.span())); - } - } - (Some(added), renamed, _) => { - if renamed.iter().any(|r| *r.since == *added.since) { - return Err(Error::custom( - "field cannot be marked as `added` and `renamed` in the same version", - ) - .with_span(&self.ident.span())); - } + (Some(added), _, Some(deprecated)) if *added.since == *deprecated.since => { + Err(Error::custom( + "field cannot be marked as `added` and `deprecated` in the same version", + ) + .with_span(&self.ident.span())) } - (_, renamed, Some(deprecated)) => { - if renamed.iter().any(|r| *r.since == *deprecated.since) { - return Err(Error::custom( - "field cannot be marked as `deprecated` and `renamed` in the same version", - ) - .with_span(&self.ident.span())); - } + (Some(added), renamed, _) if renamed.iter().any(|r| *r.since == *added.since) => { + Err(Error::custom( + "field cannot be marked as `added` and `renamed` in the same version", + ) + .with_span(&self.ident.span())) } - _ => {} - } - - // Validate that renamed action versions are sorted to ensure consistent - // code. - let original = self.renames.clone(); - self.renames - .sort_by(|lhs, rhs| lhs.since.partial_cmp(&rhs.since).unwrap_or(Ordering::Equal)); - - for (index, version) in original.iter().enumerate() { - if *version.since == *self.renames.get(index).unwrap().since { - continue; + (_, renamed, Some(deprecated)) + if renamed.iter().any(|r| *r.since == *deprecated.since) => + { + Err(Error::custom( + "field cannot be marked as `deprecated` and `renamed` in the same version", + ) + .with_span(&self.ident.span())) } - - return Err(Error::custom(format!( - "version of renames must be defined in ascending order (version `{}` is misplaced)", - *version.since - )) - .with_span(&self.ident.span())); + _ => Ok(()), } + } - // TODO (@Techassi): Add validation for renames so that renamed fields - // match up and form a continous chain (eg. foo -> bar -> baz). - - // TODO (@Techassi): Add hint if a field is added in the first version - // that it might be clever to remove the 'added' attribute. - - // Validate that actions use chronologically valid versions. If the - // field was added (not included from the start), the version must be - // less than version from the renamed and deprecated actions. + fn validate_action_order(&self) -> Result<(), Error> { let added_version = self.added.as_ref().map(|a| *a.since); let deprecated_version = self.deprecated.as_ref().map(|d| *d.since); - if let Some(added_version) = added_version { - if !self.renames.iter().all(|r| *r.since > added_version) { + // First, validate that the added version is less than the deprecated + // version. + if let (Some(added_version), Some(deprecated_version)) = (added_version, deprecated_version) + { + if added_version >= deprecated_version { return Err(Error::custom(format!( - "field was marked as `added` in version `{}` and thus all renames must use a higher version", - added_version - )) - .with_span(&self.ident.span())); + "field was marked as `added` in version `{}` while being marked as `deprecated` in an earlier version `{}`", + added_version, + deprecated_version + )).with_span(&self.ident.span())); } + } - if let Some(deprecated_version) = deprecated_version { - if added_version > deprecated_version { - return Err(Error::custom(format!( - "field was marked as `added` in version `{}` while being marked as `deprecated` in an earlier version `{}`", - added_version, - deprecated_version - )).with_span(&self.ident.span())); - } - } + // Now, iterate over all renames and ensure that their versions are + // between the added and deprecated version. + if !self.renames.iter().all(|r| { + added_version.map_or(true, |a| a < *r.since) + && deprecated_version.map_or(true, |d| d > *r.since) + }) { + return Err(Error::custom( + "all renames must use versions higher than `added` and lower than `deprecated`", + ) + .with_span(&self.ident.span())); } - // The same rule applies to renamed fields. Versions of renames must be - // less than the deprecation version (if any). - if let Some(deprecated_version) = deprecated_version { - if !self.renames.iter().all(|r| *r.since < deprecated_version) { - return Err(Error::custom(format!( - "field was marked as `deprecated` in version `{}` and thus all renames must use a lower version", - deprecated_version - )).with_span(&self.ident.span())); - } + Ok(()) + } - // Also check if the field starts with the prefix 'deprecated_'. - if !self - .ident - .as_ref() - .unwrap() - .to_string() - .starts_with(DEPRECATED_PREFIX) - { - return Err(Error::custom( - "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name", - ) - .with_span(&self.ident.span())); - } + fn validate_field_name(&self) -> Result<(), Error> { + let starts_with = self + .ident + .as_ref() + .unwrap() + .to_string() + .starts_with(DEPRECATED_PREFIX); + + if self.deprecated.is_some() && !starts_with { + return Err(Error::custom( + "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name" + ).with_span(&self.ident.span())); + } else if starts_with { + return Err(Error::custom( + "field includes the `deprecated_` prefix in its name but is not marked as `deprecated`" + ).with_span(&self.ident.span())); } - Ok(self) + Ok(()) } pub(crate) fn check_versions( diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 7a9078eca..84fa5121a 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -12,13 +12,8 @@ use stackable_versioned::Versioned; )] struct Foo { /// My docs - #[versioned( - added(since = "v1beta1"), - renamed(since = "v1beta2", from = "jjj"), - renamed(since = "v1", from = "ppp"), - deprecated(since = "v2alpha1", _note = "") - )] - deprecated_bar: usize, + #[versioned(added(since = "v1beta1"), renamed(since = "v1beta2", from = "jjj"))] + bar: usize, baz: bool, } From 0916a93abce3d9c6d4072df6dfe8a78368c8ad7b Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 30 Apr 2024 16:37:18 +0200 Subject: [PATCH 26/43] Improve error handling, add doc comments --- .../src/attrs/container.rs | 4 +- crates/stackable-versioned/src/attrs/field.rs | 91 ++++++++++++++----- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index b363cee71..7e9340b77 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -2,7 +2,7 @@ use std::{cmp::Ordering, collections::HashSet, ops::Deref}; use darling::{ util::{Flag, SpannedValue}, - Error, FromDeriveInput, FromMeta, + Error, FromDeriveInput, FromMeta, Result, }; use k8s_version::Version; @@ -22,7 +22,7 @@ pub(crate) struct ContainerAttributes { } impl ContainerAttributes { - fn validate(mut self) -> darling::Result { + fn validate(mut self) -> Result { // Most of the validation for individual version strings is done by the // k8s-version crate. That's why the code below only checks that at // least one version is defined, they are defined in order (to ensure diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index b61921e8e..43eb4a7fb 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -1,6 +1,6 @@ use darling::{util::SpannedValue, Error, FromField, FromMeta}; use k8s_version::Version; -use syn::{spanned::Spanned, Field, Ident}; +use syn::{Field, Ident}; use crate::{attrs::container::ContainerAttributes, consts::DEPRECATED_PREFIX}; @@ -55,9 +55,18 @@ pub(crate) struct DeprecatedAttributes { } impl FieldAttributes { + /// This associated function is called by darling (see and_then attribute) + /// after it successfully parsed the attribute. This allows custom + /// validation of the attribute which extends the validation already in + /// place by darling. + /// + /// Internally, it calls out to other specialized validation functions. fn validate(self) -> Result { - self.validate_action_combinations()?; - // self.validate_rename_order()?; + let mut errors = Error::accumulator(); + + errors.handle(self.validate_action_combinations()); + errors.handle(self.validate_action_order()); + errors.handle(self.validate_field_name()); // TODO (@Techassi): Add validation for renames so that renamed fields // match up and form a continous chain (eg. foo -> bar -> baz). @@ -65,30 +74,36 @@ impl FieldAttributes { // TODO (@Techassi): Add hint if a field is added in the first version // that it might be clever to remove the 'added' attribute. - self.validate_action_order()?; - self.validate_field_name()?; - + errors.finish()?; Ok(self) } + /// This associated function is called by the top-level validation function + /// and validates that each field uses a valid combination of actions. + /// Invalid combinations are: + /// + /// - `added` and `deprecated` using the same version: A field cannot be + /// marked as added in a particular version and then marked as deprecated + /// immediately after. Fields must be included for at least one version + /// before being marked deprecated. + /// - `added` and `renamed` using the same version: The same reasoning from + /// above applies here as well. Fields must be included for at least one + /// version before being renamed. + /// - `renamed` and `deprecated` using the same version: Again, the same + /// rules from above apply here as well. fn validate_action_combinations(&self) -> Result<(), Error> { match (&self.added, &self.renames, &self.deprecated) { - // The derive macro prohibits the use of the 'added' and 'deprecated' - // field action using the same version. This is because it doesn't - // make sense to add a field and immediatly mark that field as - // deprecated in the same version. Instead, fields should be - // deprecated at least one version later. (Some(added), _, Some(deprecated)) if *added.since == *deprecated.since => { Err(Error::custom( "field cannot be marked as `added` and `deprecated` in the same version", ) - .with_span(&self.ident.span())) + .with_span(&self.ident)) } (Some(added), renamed, _) if renamed.iter().any(|r| *r.since == *added.since) => { Err(Error::custom( "field cannot be marked as `added` and `renamed` in the same version", ) - .with_span(&self.ident.span())) + .with_span(&self.ident)) } (_, renamed, Some(deprecated)) if renamed.iter().any(|r| *r.since == *deprecated.since) => @@ -96,12 +111,24 @@ impl FieldAttributes { Err(Error::custom( "field cannot be marked as `deprecated` and `renamed` in the same version", ) - .with_span(&self.ident.span())) + .with_span(&self.ident)) } _ => Ok(()), } } + /// This associated function is called by the top-level validation function + /// and validates that actions use a chronologically sound chain of + /// versions. + /// + /// The following rules apply: + /// + /// - `deprecated` must use a greater version than `added`: This function + /// ensures that these versions are chronologically sound, that means, + /// that the version of the deprecated action must be greater than the + /// version of the added action. + /// - All `renamed` actions must use a greater version than `added` but a + /// lesser version than `deprecated`. fn validate_action_order(&self) -> Result<(), Error> { let added_version = self.added.as_ref().map(|a| *a.since); let deprecated_version = self.deprecated.as_ref().map(|d| *d.since); @@ -115,7 +142,7 @@ impl FieldAttributes { "field was marked as `added` in version `{}` while being marked as `deprecated` in an earlier version `{}`", added_version, deprecated_version - )).with_span(&self.ident.span())); + )).with_span(&self.ident)); } } @@ -128,12 +155,21 @@ impl FieldAttributes { return Err(Error::custom( "all renames must use versions higher than `added` and lower than `deprecated`", ) - .with_span(&self.ident.span())); + .with_span(&self.ident)); } Ok(()) } + /// This associated function is called by the top-level validation function + /// and validates that fields use correct names depending on attached + /// actions. + /// + /// The following naming rules apply: + /// + /// - Fields marked as deprecated need to include the 'deprecated_' prefix + /// in their name. The prefix must not be included for fields which are + /// not deprecated. fn validate_field_name(&self) -> Result<(), Error> { let starts_with = self .ident @@ -145,11 +181,11 @@ impl FieldAttributes { if self.deprecated.is_some() && !starts_with { return Err(Error::custom( "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name" - ).with_span(&self.ident.span())); + ).with_span(&self.ident)); } else if starts_with { return Err(Error::custom( "field includes the `deprecated_` prefix in its name but is not marked as `deprecated`" - ).with_span(&self.ident.span())); + ).with_span(&self.ident)); } Ok(()) @@ -161,6 +197,7 @@ impl FieldAttributes { field: &Field, ) -> Result<(), Error> { // NOTE (@Techassi): Can we maybe optimize this a little? + let mut errors = Error::accumulator(); if let Some(added) = &self.added { if !container_attrs @@ -168,9 +205,10 @@ impl FieldAttributes { .iter() .any(|v| v.name == *added.since) { - return Err( - Error::custom("field action `added` uses version which was not declared via #[versioned(version)]") - .with_span(&field.ident.span())); + errors.push(Error::custom( + "field action `added` uses version which was not declared via #[versioned(version)]") + .with_span(&field.ident) + ); } } @@ -180,7 +218,10 @@ impl FieldAttributes { .iter() .any(|v| v.name == *rename.since) { - return Err(Error::custom("field action `renamed` uses version which was not declared via #[versioned(version)]")); + errors.push( + Error::custom("field action `renamed` uses version which was not declared via #[versioned(version)]") + .with_span(&field.ident) + ); } } @@ -190,10 +231,14 @@ impl FieldAttributes { .iter() .any(|v| v.name == *deprecated.since) { - return Err(Error::custom("field action `deprecated` uses version which was not declared via #[versioned(version)]")); + errors.push(Error::custom( + "field action `deprecated` uses version which was not declared via #[versioned(version)]") + .with_span(&field.ident) + ); } } + errors.finish()?; Ok(()) } } From 727fbdf5206cb5d482b2bdf842e789d7afc6065a Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 2 May 2024 14:34:32 +0200 Subject: [PATCH 27/43] k8s-version: Add validated Group --- crates/k8s-version/src/api_version.rs | 53 +++++++++++++--------- crates/k8s-version/src/group.rs | 64 +++++++++++++++++++++++++++ crates/k8s-version/src/lib.rs | 2 + crates/k8s-version/src/version.rs | 18 ++++---- 4 files changed, 108 insertions(+), 29 deletions(-) create mode 100644 crates/k8s-version/src/group.rs diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index f7b7b849c..83ca9c381 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -5,19 +5,18 @@ use snafu::{ResultExt, Snafu}; #[cfg(feature = "darling")] use darling::FromMeta; -use crate::{Version, VersionParseError}; +use crate::{Group, ParseGroupError, ParseVersionError, Version}; #[derive(Debug, PartialEq, Snafu)] -pub enum ApiVersionParseError { +pub enum ParseApiVersionError { #[snafu(display("failed to parse version"))] - ParseVersion { source: VersionParseError }, + ParseVersion { source: ParseVersionError }, - #[snafu(display("group cannot be empty"))] - EmptyGroup, + #[snafu(display("failed to parse group"))] + ParseGroup { source: ParseGroupError }, } -/// A Kubernetes API version with the `(/)` format, for example -/// `certificates.k8s.io/v1beta1`, `extensions/v1beta1` or `v1`. +/// A Kubernetes API version, following the `(/)` format. /// /// The `` string must follow the DNS label format defined [here][1]. /// The `` string must be lower case and must be a valid DNS subdomain. @@ -31,23 +30,19 @@ pub enum ApiVersionParseError { /// [1]: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/identifiers.md#definitions #[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct ApiVersion { - pub group: Option, + pub group: Option, pub version: Version, } impl FromStr for ApiVersion { - type Err = ApiVersionParseError; + type Err = ParseApiVersionError; fn from_str(input: &str) -> Result { let (group, version) = if let Some((group, version)) = input.split_once('/') { - if group.is_empty() { - return EmptyGroupSnafu.fail(); - } - - // TODO (Techassi): Validate group + let group = Group::from_str(group).context(ParseGroupSnafu)?; ( - Some(group.to_string()), + Some(group), Version::from_str(version).context(ParseVersionSnafu)?, ) } else { @@ -84,6 +79,24 @@ impl FromMeta for ApiVersion { } } +impl ApiVersion { + /// Create a new Kubernetes API version. + pub fn new(group: Option, version: Version) -> Self { + Self { group, version } + } + + /// Try to create a new Kubernetes API version based on the unvalidated + /// `group` string. + pub fn try_new(group: Option<&str>, version: Version) -> Result { + let group = group + .map(|g| g.parse()) + .transpose() + .context(ParseGroupSnafu)?; + + Ok(Self { group, version }) + } +} + #[cfg(test)] mod test { use super::*; @@ -101,7 +114,7 @@ mod test { } #[rstest] - #[case("extensions/v1beta1", ApiVersion { group: Some("extensions".into()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case("extensions/v1beta1", ApiVersion { group: Some("extensions".parse().unwrap()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] #[case("v1beta1", ApiVersion { group: None, version: Version { major: 1, level: Some(Level::Beta(1)) } })] #[case("v1", ApiVersion { group: None, version: Version { major: 1, level: None } })] fn valid_api_version(#[case] input: &str, #[case] expected: ApiVersion) { @@ -110,9 +123,9 @@ mod test { } #[rstest] - #[case("extensions/beta1", ApiVersionParseError::ParseVersion { source: VersionParseError::InvalidFormat })] - #[case("/v1beta1", ApiVersionParseError::EmptyGroup)] - fn invalid_api_version(#[case] input: &str, #[case] error: ApiVersionParseError) { + #[case("extensions/beta1", ParseApiVersionError::ParseVersion { source: ParseVersionError::InvalidFormat })] + #[case("/v1beta1", ParseApiVersionError::ParseGroup { source: ParseGroupError::Empty })] + fn invalid_api_version(#[case] input: &str, #[case] error: ParseApiVersionError) { let err = ApiVersion::from_str(input).expect_err("invalid Kubernetes api versions"); assert_eq!(err, error); } @@ -131,7 +144,7 @@ mod test { #[cfg(feature = "darling")] #[rstest] - #[case(quote!(ignore = "extensions/v1beta1"), ApiVersion { group: Some("extensions".into()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] + #[case(quote!(ignore = "extensions/v1beta1"), ApiVersion { group: Some("extensions".parse().unwrap()), version: Version { major: 1, level: Some(Level::Beta(1)) } })] #[case(quote!(ignore = "v1beta1"), ApiVersion { group: None, version: Version { major: 1, level: Some(Level::Beta(1)) } })] #[case(quote!(ignore = "v1"), ApiVersion { group: None, version: Version { major: 1, level: None } })] fn from_meta(#[case] input: proc_macro2::TokenStream, #[case] expected: ApiVersion) { diff --git a/crates/k8s-version/src/group.rs b/crates/k8s-version/src/group.rs new file mode 100644 index 000000000..594e4af43 --- /dev/null +++ b/crates/k8s-version/src/group.rs @@ -0,0 +1,64 @@ +use std::{fmt, ops::Deref, str::FromStr}; + +use lazy_static::lazy_static; +use regex::Regex; +use snafu::{ensure, Snafu}; + +const MAX_GROUP_LENGTH: usize = 253; + +lazy_static! { + static ref API_VERSION_REGEX: Regex = + Regex::new(r"^(?:(?:[a-z0-9][a-z0-9-]{0,61}[a-z0-9])\.?)+$").unwrap(); +} + +#[derive(Debug, PartialEq, Snafu)] +pub enum ParseGroupError { + #[snafu(display("group must not be empty"))] + Empty, + + #[snafu(display("group must not be longer than 253 characters"))] + TooLong, + + #[snafu(display("group must be a valid DNS subdomain"))] + InvalidFormat, +} + +/// A validated Kubernetes group. +/// +/// The group string must follow these rules: +/// +/// - must be non-empty +/// - must only contain lower case characters +/// - and must be a valid DNS subdomain +/// +/// ### See +/// +/// - +#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd)] +pub struct Group(String); + +impl FromStr for Group { + type Err = ParseGroupError; + + fn from_str(group: &str) -> Result { + ensure!(!group.is_empty(), EmptySnafu); + ensure!(group.len() <= MAX_GROUP_LENGTH, TooLongSnafu); + ensure!(API_VERSION_REGEX.is_match(group), InvalidFormatSnafu); + + Ok(Self(group.to_string())) + } +} + +impl fmt::Display for Group { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Deref for Group { + type Target = str; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index b1a8f3d01..280a2668e 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -4,9 +4,11 @@ use rstest_reuse::{self}; mod api_version; +mod group; mod level; mod version; pub use api_version::*; +pub use group::*; pub use level::*; pub use version::*; diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 3567b094f..ec9374fb4 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -15,7 +15,7 @@ lazy_static! { } #[derive(Debug, PartialEq, Snafu)] -pub enum VersionParseError { +pub enum ParseVersionError { #[snafu(display("invalid version format. Input is empty, contains non-ASCII characters or contains more than 63 characters"))] InvalidFormat, @@ -26,8 +26,8 @@ pub enum VersionParseError { ParseLevel { source: ParseLevelError }, } -/// A Kubernetes resource version with the `v(beta/alpha)` -/// format, for example `v1`, `v2beta1` or `v1alpha2`. +/// A Kubernetes resource version, following the `v(beta/alpha)` +/// format. /// /// The version must follow the DNS label format defined [here][1]. /// @@ -44,7 +44,7 @@ pub struct Version { } impl FromStr for Version { - type Err = VersionParseError; + type Err = ParseVersionError; fn from_str(input: &str) -> Result { let captures = VERSION_REGEX.captures(input).context(InvalidFormatSnafu)?; @@ -150,11 +150,11 @@ mod test { } #[rstest] - #[case("v1gamma12", VersionParseError::ParseLevel { source: ParseLevelError::UnknownIdentifier })] - #[case("v1betä1", VersionParseError::InvalidFormat)] - #[case("1beta1", VersionParseError::InvalidFormat)] - #[case("", VersionParseError::InvalidFormat)] - fn invalid_version(#[case] input: &str, #[case] error: VersionParseError) { + #[case("v1gamma12", ParseVersionError::ParseLevel { source: ParseLevelError::UnknownIdentifier })] + #[case("v1betä1", ParseVersionError::InvalidFormat)] + #[case("1beta1", ParseVersionError::InvalidFormat)] + #[case("", ParseVersionError::InvalidFormat)] + fn invalid_version(#[case] input: &str, #[case] error: ParseVersionError) { let err = Version::from_str(input).expect_err("invalid Kubernetes version"); assert_eq!(err, error) } From 92fafcfa0f07995f08676ca65a3cc41b67645c7c Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 2 May 2024 14:37:26 +0200 Subject: [PATCH 28/43] k8s-version: Add library doc comments --- crates/k8s-version/src/lib.rs | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 280a2668e..3adf6b380 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -1,3 +1,40 @@ +//! This library provides strongly-typed and validated Kubernetes API version +//! definitions. Versions consist of three major components: the optional group, +//! the mandatory major version and the optional level. The format can be +//! described by `(/)`, with `` being defined as +//! `v(beta/alpha)`. +//! +//! ## Usage +//! +//! Versions can be parsed and validated from [`str`] using Rust's standard +//! [`FromStr`](std::str::FromStr) trait. +//! +//! ``` +//! # use std::str::FromStr; +//! use k8s_version::ApiVersion; +//! +//! let api_version = ApiVersion::from_str("extensions/v1beta1") +//! .expect("valid Kubernetes API version"); +//! +//! // Or using .parse() +//! let api_version: ApiVersion = "extensions/v1beta1".parse() +//! .expect("valid Kubernetes API version"); +//! ``` +//! +//! Alternatively, they can be constructed programatically using the +//! [`new()`](ApiVersion::new) and [`try_new`](ApiVersion::try_new) function. +//! +//! ``` +//! use k8s_version::{ApiVersion, Version, Level}; +//! +//! let api_version = ApiVersion::try_new( +//! Some("extension"), +//! Version::new(1, Some(Level::Beta(1))) +//! ).expect("valid Kubernetes API version"); +//! +//! assert_eq!(api_version.to_string(), "extension/v1beta1") +//! ``` + // NOTE (@Techassi): Fixed in https://github.com/la10736/rstest/pull/244 but not // yet released. #[cfg(test)] From 1952db88358f62c86b489b903777360c153fef44 Mon Sep 17 00:00:00 2001 From: Techassi Date: Thu, 2 May 2024 15:07:49 +0200 Subject: [PATCH 29/43] k8s-version: Add doc comments for error enums --- crates/k8s-version/src/api_version.rs | 2 ++ crates/k8s-version/src/group.rs | 2 ++ crates/k8s-version/src/level.rs | 5 +++++ crates/k8s-version/src/version.rs | 2 ++ 4 files changed, 11 insertions(+) diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index 83ca9c381..097118e15 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -7,6 +7,8 @@ use darling::FromMeta; use crate::{Group, ParseGroupError, ParseVersionError, Version}; +/// Error variants which can be encountered when creating a new [`ApiVersion`] +/// from unparsed input. #[derive(Debug, PartialEq, Snafu)] pub enum ParseApiVersionError { #[snafu(display("failed to parse version"))] diff --git a/crates/k8s-version/src/group.rs b/crates/k8s-version/src/group.rs index 594e4af43..9ec73dd63 100644 --- a/crates/k8s-version/src/group.rs +++ b/crates/k8s-version/src/group.rs @@ -11,6 +11,8 @@ lazy_static! { Regex::new(r"^(?:(?:[a-z0-9][a-z0-9-]{0,61}[a-z0-9])\.?)+$").unwrap(); } +/// Error variants which can be encountered when creating a new [`Group`] from +/// unparsed input. #[derive(Debug, PartialEq, Snafu)] pub enum ParseGroupError { #[snafu(display("group must not be empty"))] diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index e755dba84..28879dffe 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -18,6 +18,8 @@ lazy_static! { Regex::new(r"^(?P[a-z]+)(?P\d+)$").unwrap(); } +/// Error variants which can be encountered when creating a new [`Level`] from +/// unparsed input. #[derive(Debug, PartialEq, Snafu)] pub enum ParseLevelError { #[snafu(display("invalid level format, expected beta/alpha"))] @@ -33,7 +35,10 @@ pub enum ParseLevelError { /// A minor Kubernetes resource version with the `beta/alpha` format. #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub enum Level { + /// Beta-level minor version, `beta`. Beta(u64), + + /// Alpha-level minor version, `alpha`. Alpha(u64), } diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index ec9374fb4..80cca94b1 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -14,6 +14,8 @@ lazy_static! { Regex::new(r"^v(?P\d+)(?P[a-z0-9][a-z0-9-]{0,60}[a-z0-9])?$").unwrap(); } +/// Error variants which can be encountered when creating a new [`Version`] from +/// unparsed input. #[derive(Debug, PartialEq, Snafu)] pub enum ParseVersionError { #[snafu(display("invalid version format. Input is empty, contains non-ASCII characters or contains more than 63 characters"))] From 6148a0998deff9ec44c426e25b813fd7f4f5888c Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 3 May 2024 13:59:32 +0200 Subject: [PATCH 30/43] Add more (doc) comments --- .../src/attrs/container.rs | 23 ++++++++++-- crates/stackable-versioned/src/attrs/field.rs | 8 ++--- crates/stackable-versioned/src/gen/field.rs | 36 +++++++++---------- crates/stackable-versioned/src/gen/vstruct.rs | 29 +++++++++------ 4 files changed, 59 insertions(+), 37 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index 7e9340b77..cad3d4447 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -6,6 +6,13 @@ use darling::{ }; use k8s_version::Version; +/// This struct contains supported container attributes. +/// +/// Currently supported atttributes are: +/// +/// - `version`, which can occur one or more times. Details on supported options +/// can be found [here](VersionAttributes). +/// - `options`, which allow further customization of the generated code. #[derive(Clone, Debug, FromDeriveInput)] #[darling( attributes(versioned), @@ -18,7 +25,7 @@ pub(crate) struct ContainerAttributes { pub(crate) versions: SpannedValue>, #[darling(default)] - pub(crate) options: VersionOptions, + pub(crate) options: ContainerOptions, } impl ContainerAttributes { @@ -76,13 +83,25 @@ impl ContainerAttributes { } } +/// This struct contains supported version options. +/// +/// Supported options are: +/// +/// - `name` of the version, like `v1alpha1`. +/// - `deprecated` flag to mark that version as deprecated. #[derive(Clone, Debug, FromMeta)] pub(crate) struct VersionAttributes { pub(crate) deprecated: Flag, pub(crate) name: Version, } +/// This struct contains supported container options. +/// +/// Supported options are: +/// +/// - `allow_unsorted`, which allows declaring versions in unsorted order, +/// instead of enforcing ascending order. #[derive(Clone, Debug, Default, FromMeta)] -pub(crate) struct VersionOptions { +pub(crate) struct ContainerOptions { pub(crate) allow_unsorted: Flag, } diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 43eb4a7fb..78f773c3c 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -139,9 +139,7 @@ impl FieldAttributes { { if added_version >= deprecated_version { return Err(Error::custom(format!( - "field was marked as `added` in version `{}` while being marked as `deprecated` in an earlier version `{}`", - added_version, - deprecated_version + "field was marked as `added` in version `{added_version}` while being marked as `deprecated` in an earlier version `{deprecated_version}`" )).with_span(&self.ident)); } } @@ -182,7 +180,9 @@ impl FieldAttributes { return Err(Error::custom( "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name" ).with_span(&self.ident)); - } else if starts_with { + } + + if self.deprecated.is_none() && starts_with { return Err(Error::custom( "field includes the `deprecated_` prefix in its name but is not marked as `deprecated`" ).with_span(&self.ident)); diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index d0e66145b..a9e4ae2ce 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -12,6 +12,12 @@ use crate::{ gen::{version::ContainerVersion, ToTokensExt}, }; +/// A versioned field, which contains contains common [`Field`] data and a chain +/// of actions. +/// +/// The chain of action maps versions to an action and the appropriate field +/// name. Additionally, the [`Field`] data can be used to forward attributes, +/// generate documention, etc. #[derive(Debug)] pub(crate) struct VersionedField { chain: Option>, @@ -29,6 +35,11 @@ impl ToTokensExt for VersionedField { // If not, the provided version has no action attached to it. // The code generation then depends on the relation to other // versions (with actions). + + // TODO (@Techassi): Make this more robust by also including + // the container versions in the action chain. I'm not happy + // with the follwoing code at all. It serves as a good first + // implementation to get something out of the door. match chain.get(&container_version.inner) { Some(action) => match action { FieldStatus::Added(field_ident) => { @@ -55,6 +66,9 @@ impl ToTokensExt for VersionedField { } }, None => { + // Generate field if the container version is not + // included in the action chain. First we check the + // earliest field action version. if let Some((version, action)) = chain.first_key_value() { if container_version.inner < *version { match action { @@ -77,6 +91,8 @@ impl ToTokensExt for VersionedField { } } + // Check the container version against the latest + // field action version. if let Some((version, action)) = chain.last_key_value() { if container_version.inner > *version { match action { @@ -222,26 +238,6 @@ impl VersionedField { }) } } - - /// Extend the already recorded actions with actions based on global - /// container versions to construct a complete chain of actions for each - /// field. - pub(crate) fn _extend_with_container_versions(&mut self, _versions: &[ContainerVersion]) { - // When creating this type via the new function, only directly attached - // action can be inserted into the chain of actions. It doesn't contain - // any actions based on the container versions. A quick example: - // - // Let's assume we have the following declared versions: v1, v2, v3, v4. - // One field, let's call it foo, has two actions attached: added in v2 - // and deprecated in v3. So initially, the chain of actions only contains - // two actions: added(v2) and deprecated(v3). But what happened to the - // field in v1 and v4? This information can only be included in the - // chain by looking at the container versions. In this particular - // example the field wasn't present in v1 and isn't present from v4 and - // onward. This action (or state) needs to be included in the chain of - // actions. The resulting chain now looks like: not-present(v1), - // added(v2), deprecated(v3), not-present(v4). - } } #[derive(Debug)] diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 4dba7937e..e490c82fa 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -14,20 +14,26 @@ use crate::{ /// version of the struct. #[derive(Debug)] pub(crate) struct VersionedStruct { - pub(crate) _ident: Ident, + /// The ident, or name, of the versioned struct. + pub(crate) ident: Ident, - pub(crate) _versions: Vec, - pub(crate) _fields: Vec, + /// List of declared versions for this struct. Each version, except the + /// latest, generates a definition with appropriate fields. + pub(crate) versions: Vec, + + /// List of fields defined in the base struct. How, and if, a field should + /// generate code, is decided by the currently generated version. + pub(crate) fields: Vec, } impl ToTokens for VersionedStruct { fn to_tokens(&self, _tokens: &mut TokenStream) { - let mut versions = self._versions.iter().peekable(); + let mut versions = self.versions.iter().peekable(); while let Some(version) = versions.next() { let mut fields = TokenStream::new(); - for field in &self._fields { + for field in &self.fields { fields.extend(field.to_tokens_for_version(version)); } @@ -37,7 +43,7 @@ impl ToTokens for VersionedStruct { let deprecated_attr = version.deprecated.then_some(quote! {#[deprecated]}); let module_name = format_ident!("{}", version.inner.to_string()); - let struct_name = &self._ident; + let struct_name = &self.ident; // Only genereate a module when there is at least one more version. // This skips generating a module for the latest version, because @@ -64,7 +70,7 @@ impl VersionedStruct { data: DataStruct, attributes: ContainerAttributes, ) -> Result { - let mut versioned_fields = Vec::new(); + let mut fields = Vec::new(); // Extract the field attributes for every field from the raw token // stream and also validate that each field action version uses a @@ -74,9 +80,10 @@ impl VersionedStruct { attrs.check_versions(&attributes, &field)?; let versioned_field = VersionedField::new(field, attrs)?; - versioned_fields.push(versioned_field); + fields.push(versioned_field); } + // Convert the raw version attributes into a container version. let versions = attributes .versions .iter() @@ -87,9 +94,9 @@ impl VersionedStruct { .collect(); Ok(Self { - _ident: ident, - _versions: versions, - _fields: versioned_fields, + ident, + versions, + fields, }) } } From 3252c55cf651af03d5e16a1e1968f5dc75086cfc Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 11:51:59 +0200 Subject: [PATCH 31/43] Add changelog for stackable-versioned --- CHANGELOG.md | 3 ++- crates/stackable-versioned/CHANGELOG.md | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 crates/stackable-versioned/CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a3834db2..96a6709b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,10 @@ Please see the relevant crate changelogs: +- [k8s-version](./crates/k8s-version/CHANGELOG.md) - [stackable-certs](./crates/stackable-certs/CHANGELOG.md) - [stackable-operator](./crates/stackable-operator/CHANGELOG.md) - [stackable-operator-derive](./crates/stackable-operator-derive/CHANGELOG.md) - [stackable-telemetry](./crates/stackable-telemetry/CHANGELOG.md) +- [stackable-versioned](./crates/stackable-versioned/CHANGELOG.md) - [stackable-webhook](./crates/stackable-webhook/CHANGELOG.md) -- [k8s-version](./crates/k8s-version/CHANGELOG.md) diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md new file mode 100644 index 000000000..6d303b20d --- /dev/null +++ b/crates/stackable-versioned/CHANGELOG.md @@ -0,0 +1,5 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +## [Unreleased] From 2393ae0251a8a2962ead095f7c217bcdc2d6731e Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 12:31:08 +0200 Subject: [PATCH 32/43] Apply suggestions Co-authored-by: Nick --- crates/k8s-version/src/api_version.rs | 6 ++--- crates/k8s-version/src/level.rs | 27 ++++++++++--------- crates/k8s-version/src/lib.rs | 2 +- crates/k8s-version/src/version.rs | 10 +++---- .../src/attrs/container.rs | 11 ++++---- crates/stackable-versioned/src/gen/field.rs | 10 +++---- crates/stackable-versioned/src/gen/vstruct.rs | 4 +-- 7 files changed, 35 insertions(+), 35 deletions(-) diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index 097118e15..4e23e7940 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -20,7 +20,7 @@ pub enum ParseApiVersionError { /// A Kubernetes API version, following the `(/)` format. /// -/// The `` string must follow the DNS label format defined [here][1]. +/// The `` string must follow the DNS label format defined in the [Kubernetes design proposals archive][1]. /// The `` string must be lower case and must be a valid DNS subdomain. /// /// ### See @@ -68,8 +68,8 @@ impl PartialOrd for ApiVersion { impl Display for ApiVersion { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.group { - Some(group) => write!(f, "{}/{}", group, self.version), - None => write!(f, "{}", self.version), + Some(group) => write!(f, "{group}/{version}", group, version = self.version), + None => write!(f, "{version}", version = self.version), } } } diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index 28879dffe..c93ab2dee 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -35,11 +35,11 @@ pub enum ParseLevelError { /// A minor Kubernetes resource version with the `beta/alpha` format. #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub enum Level { - /// Beta-level minor version, `beta`. - Beta(u64), - /// Alpha-level minor version, `alpha`. Alpha(u64), + + /// Beta-level minor version, `beta`. + Beta(u64), } impl FromStr for Level { @@ -77,13 +77,14 @@ impl PartialOrd for Level { impl Ord for Level { fn cmp(&self, other: &Self) -> Ordering { match self { - Level::Beta(sb) => match other { - Level::Beta(ob) => sb.cmp(ob), - Level::Alpha(_) => Ordering::Greater, - }, Level::Alpha(sa) => match other { - Level::Beta(_) => Ordering::Less, Level::Alpha(oa) => sa.cmp(oa), + Level::Beta(_) => Ordering::Less, + + }, + Level::Beta(sb) => match other { + Level::Alpha(_) => Ordering::Greater, + Level::Beta(ob) => sb.cmp(ob), }, } } @@ -97,8 +98,8 @@ where fn add(self, rhs: T) -> Self::Output { match self { - Level::Beta(b) => Level::Beta(b + rhs.into()), Level::Alpha(a) => Level::Alpha(a + rhs.into()), + Level::Beta(b) => Level::Beta(b + rhs.into()), } } } @@ -109,8 +110,8 @@ where { fn add_assign(&mut self, rhs: T) { match self { - Level::Beta(b) => *b + rhs.into(), Level::Alpha(a) => *a + rhs.into(), + Level::Beta(b) => *b + rhs.into(), }; } } @@ -123,8 +124,8 @@ where fn sub(self, rhs: T) -> Self::Output { match self { - Level::Beta(b) => Level::Beta(b - rhs.into()), Level::Alpha(a) => Level::Alpha(a - rhs.into()), + Level::Beta(b) => Level::Beta(b - rhs.into()), } } } @@ -135,8 +136,8 @@ where { fn sub_assign(&mut self, rhs: T) { match self { - Level::Beta(b) => *b - rhs.into(), Level::Alpha(a) => *a - rhs.into(), + Level::Beta(b) => *b - rhs.into(), }; } } @@ -144,8 +145,8 @@ where impl Display for Level { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Level::Beta(beta) => write!(f, "beta{}", beta), Level::Alpha(alpha) => write!(f, "alpha{}", alpha), + Level::Beta(beta) => write!(f, "beta{}", beta), } } } diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 3adf6b380..5d6461c01 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -1,4 +1,4 @@ -//! This library provides strongly-typed and validated Kubernetes API version +//! This library provides strongly-typed and validated Kubernetes API version //! definitions. Versions consist of three major components: the optional group, //! the mandatory major version and the optional level. The format can be //! described by `(/)`, with `` being defined as diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 80cca94b1..c0afe2216 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -31,7 +31,7 @@ pub enum ParseVersionError { /// A Kubernetes resource version, following the `v(beta/alpha)` /// format. /// -/// The version must follow the DNS label format defined [here][1]. +/// The version must follow the DNS label format defined in the [Kubernetes design proposals archive][1]. /// /// ### See /// @@ -96,8 +96,8 @@ impl Ord for Version { impl Display for Version { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.level { - Some(minor) => write!(f, "v{}{}", self.major, minor), - None => write!(f, "v{}", self.major), + Some(minor) => write!(f, "v{major}{minor}", major = self.major, minor), + None => write!(f, "v{major}", major = self.major), } } } @@ -110,10 +110,10 @@ impl FromMeta for Version { } impl Version { - pub fn new(major: u64, minor: Option) -> Self { + pub fn new(major: u64, level: Option) -> Self { Self { major, - level: minor, + level, } } } diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index cad3d4447..a1660d20c 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -10,9 +10,8 @@ use k8s_version::Version; /// /// Currently supported atttributes are: /// -/// - `version`, which can occur one or more times. Details on supported options -/// can be found [here](VersionAttributes). -/// - `options`, which allow further customization of the generated code. +/// - `version`, which can occur one or more times. See [`VersionAttributes`]. +/// - `options`, which allow further customization of the generated code. See [`ContainerOptions`]. #[derive(Clone, Debug, FromDeriveInput)] #[darling( attributes(versioned), @@ -44,7 +43,7 @@ impl ContainerAttributes { .with_span(&self.versions.span())); } - // NOTE (@Techassi): Do we even want to allow to opp-out of this? + // NOTE (@Techassi): Do we even want to allow to opt-out of this? // Ensure that versions are defined in sorted (ascending) order to keep // code consistent. @@ -59,8 +58,8 @@ impl ContainerAttributes { } return Err(Error::custom(format!( - "versions in `#[versioned()]` must be defined in ascending order (version `{}` is misplaced)", - version.name + "versions in `#[versioned()]` must be defined in ascending order (version `{name}` is misplaced)", + name = version.name ))); } } diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index a9e4ae2ce..b574630d2 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -146,12 +146,12 @@ impl ToTokensExt for VersionedField { impl VersionedField { pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Result { // Constructing the change chain requires going through the actions from - // the end, because the base struct allways represents the latest (most + // the end, because the base struct always represents the latest (most // up-to-date) version of that struct. That's why the following code // needs to go through the changes in reverse order, as otherwise it is // impossible to extract the field ident for each version. - // Deprecating a field is always the last status a field can up in. For + // Deprecating a field is always the last state a field can end up in. For // fields which are not deprecated, the last change is either the latest // rename or addition, which is handled below. // The ident of the deprecated field is guaranteed to include the @@ -165,10 +165,10 @@ impl VersionedField { // When the field is deprecated, any rename which occured beforehand // requires access to the field ident to infer the field ident for // the latest rename. - let mut ident = format_ident!("{}", ident.to_string().replace(DEPRECATED_PREFIX, "")); + let mut ident = format_ident!("{ident}", ident = ident.to_string().replace(DEPRECATED_PREFIX, "")); for rename in attrs.renames.iter().rev() { - let from = format_ident!("{}", *rename.from); + let from = format_ident!("{from}", from = *rename.from); actions.insert( *rename.since, FieldStatus::Renamed { @@ -194,7 +194,7 @@ impl VersionedField { let mut ident = field.ident.clone().unwrap(); for rename in attrs.renames.iter().rev() { - let from = format_ident!("{}", *rename.from); + let from = format_ident!("{from}", from = *rename.from); actions.insert( *rename.since, FieldStatus::Renamed { diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index e490c82fa..3a7c1abde 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -42,10 +42,10 @@ impl ToTokens for VersionedStruct { // generates versioned versions of all contained containers. let deprecated_attr = version.deprecated.then_some(quote! {#[deprecated]}); - let module_name = format_ident!("{}", version.inner.to_string()); + let module_name = format_ident!("{version}", version = version.inner.to_string()); let struct_name = &self.ident; - // Only genereate a module when there is at least one more version. + // Only generate a module when there is at least one more version. // This skips generating a module for the latest version, because // the base struct always represents the latest version. if versions.peek().is_some() { From 9e6fdefdf3833833fda20f5c7a4c00c9d88deb40 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 12:37:46 +0200 Subject: [PATCH 33/43] Clean-up suggestions --- crates/k8s-version/src/api_version.rs | 2 +- crates/k8s-version/src/level.rs | 25 ++++++++++----------- crates/k8s-version/src/version.rs | 7 ++---- crates/stackable-versioned/src/gen/field.rs | 5 ++++- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index 4e23e7940..5f1507a84 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -68,7 +68,7 @@ impl PartialOrd for ApiVersion { impl Display for ApiVersion { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.group { - Some(group) => write!(f, "{group}/{version}", group, version = self.version), + Some(group) => write!(f, "{group}/{version}", version = self.version), None => write!(f, "{version}", version = self.version), } } diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index c93ab2dee..b82e7db0e 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -77,14 +77,13 @@ impl PartialOrd for Level { impl Ord for Level { fn cmp(&self, other: &Self) -> Ordering { match self { - Level::Alpha(sa) => match other { - Level::Alpha(oa) => sa.cmp(oa), + Level::Alpha(lhs) => match other { + Level::Alpha(rhs) => lhs.cmp(rhs), Level::Beta(_) => Ordering::Less, - }, - Level::Beta(sb) => match other { + Level::Beta(lhs) => match other { Level::Alpha(_) => Ordering::Greater, - Level::Beta(ob) => sb.cmp(ob), + Level::Beta(rhs) => lhs.cmp(rhs), }, } } @@ -98,8 +97,8 @@ where fn add(self, rhs: T) -> Self::Output { match self { - Level::Alpha(a) => Level::Alpha(a + rhs.into()), - Level::Beta(b) => Level::Beta(b + rhs.into()), + Level::Alpha(lhs) => Level::Alpha(lhs + rhs.into()), + Level::Beta(lhs) => Level::Beta(lhs + rhs.into()), } } } @@ -110,8 +109,8 @@ where { fn add_assign(&mut self, rhs: T) { match self { - Level::Alpha(a) => *a + rhs.into(), - Level::Beta(b) => *b + rhs.into(), + Level::Alpha(lhs) => *lhs + rhs.into(), + Level::Beta(lhs) => *lhs + rhs.into(), }; } } @@ -124,8 +123,8 @@ where fn sub(self, rhs: T) -> Self::Output { match self { - Level::Alpha(a) => Level::Alpha(a - rhs.into()), - Level::Beta(b) => Level::Beta(b - rhs.into()), + Level::Alpha(lhs) => Level::Alpha(lhs - rhs.into()), + Level::Beta(lhs) => Level::Beta(lhs - rhs.into()), } } } @@ -136,8 +135,8 @@ where { fn sub_assign(&mut self, rhs: T) { match self { - Level::Alpha(a) => *a - rhs.into(), - Level::Beta(b) => *b - rhs.into(), + Level::Alpha(lhs) => *lhs - rhs.into(), + Level::Beta(lhs) => *lhs - rhs.into(), }; } } diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index c0afe2216..1a8eaf966 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -96,7 +96,7 @@ impl Ord for Version { impl Display for Version { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.level { - Some(minor) => write!(f, "v{major}{minor}", major = self.major, minor), + Some(level) => write!(f, "v{major}{level}", major = self.major), None => write!(f, "v{major}", major = self.major), } } @@ -111,10 +111,7 @@ impl FromMeta for Version { impl Version { pub fn new(major: u64, level: Option) -> Self { - Self { - major, - level, - } + Self { major, level } } } diff --git a/crates/stackable-versioned/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index b574630d2..99ac9ecba 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -165,7 +165,10 @@ impl VersionedField { // When the field is deprecated, any rename which occured beforehand // requires access to the field ident to infer the field ident for // the latest rename. - let mut ident = format_ident!("{ident}", ident = ident.to_string().replace(DEPRECATED_PREFIX, "")); + let mut ident = format_ident!( + "{ident}", + ident = ident.to_string().replace(DEPRECATED_PREFIX, "") + ); for rename in attrs.renames.iter().rev() { let from = format_ident!("{from}", from = *rename.from); From 8a38f47a2808c1eb87523a3aec012b83c5249942 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 12:47:24 +0200 Subject: [PATCH 34/43] Rename API_VERSION_REGEX to API_GROUP_REGEX --- crates/k8s-version/src/group.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/k8s-version/src/group.rs b/crates/k8s-version/src/group.rs index 9ec73dd63..f4e831fd0 100644 --- a/crates/k8s-version/src/group.rs +++ b/crates/k8s-version/src/group.rs @@ -7,7 +7,7 @@ use snafu::{ensure, Snafu}; const MAX_GROUP_LENGTH: usize = 253; lazy_static! { - static ref API_VERSION_REGEX: Regex = + static ref API_GROUP_REGEX: Regex = Regex::new(r"^(?:(?:[a-z0-9][a-z0-9-]{0,61}[a-z0-9])\.?)+$").unwrap(); } @@ -45,7 +45,7 @@ impl FromStr for Group { fn from_str(group: &str) -> Result { ensure!(!group.is_empty(), EmptySnafu); ensure!(group.len() <= MAX_GROUP_LENGTH, TooLongSnafu); - ensure!(API_VERSION_REGEX.is_match(group), InvalidFormatSnafu); + ensure!(API_GROUP_REGEX.is_match(group), InvalidFormatSnafu); Ok(Self(group.to_string())) } From 1aa7fcf2939ae401b238c7e40984362a2a0974b5 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 12:50:27 +0200 Subject: [PATCH 35/43] Use expect instead of unwrap for regular expressions --- crates/k8s-version/src/group.rs | 3 ++- crates/k8s-version/src/level.rs | 4 ++-- crates/k8s-version/src/version.rs | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/k8s-version/src/group.rs b/crates/k8s-version/src/group.rs index f4e831fd0..c4d2658b8 100644 --- a/crates/k8s-version/src/group.rs +++ b/crates/k8s-version/src/group.rs @@ -8,7 +8,8 @@ const MAX_GROUP_LENGTH: usize = 253; lazy_static! { static ref API_GROUP_REGEX: Regex = - Regex::new(r"^(?:(?:[a-z0-9][a-z0-9-]{0,61}[a-z0-9])\.?)+$").unwrap(); + Regex::new(r"^(?:(?:[a-z0-9][a-z0-9-]{0,61}[a-z0-9])\.?)+$") + .expect("failed to compile API group regex"); } /// Error variants which can be encountered when creating a new [`Group`] from diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index b82e7db0e..f0b41d875 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -14,8 +14,8 @@ use snafu::{OptionExt, ResultExt, Snafu}; use darling::FromMeta; lazy_static! { - static ref LEVEL_REGEX: Regex = - Regex::new(r"^(?P[a-z]+)(?P\d+)$").unwrap(); + static ref LEVEL_REGEX: Regex = Regex::new(r"^(?P[a-z]+)(?P\d+)$") + .expect("failed to compile level regex"); } /// Error variants which can be encountered when creating a new [`Level`] from diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index 1a8eaf966..c09139b36 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -11,7 +11,8 @@ use crate::{Level, ParseLevelError}; lazy_static! { static ref VERSION_REGEX: Regex = - Regex::new(r"^v(?P\d+)(?P[a-z0-9][a-z0-9-]{0,60}[a-z0-9])?$").unwrap(); + Regex::new(r"^v(?P\d+)(?P[a-z0-9][a-z0-9-]{0,60}[a-z0-9])?$") + .expect("failed to compile version regex"); } /// Error variants which can be encountered when creating a new [`Version`] from From 36279ffa03b987ac9f4b9f95593c4d304ff676dc Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 14:12:35 +0200 Subject: [PATCH 36/43] Include duplicate version name in error message --- .../stackable-versioned/src/attrs/container.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/container.rs b/crates/stackable-versioned/src/attrs/container.rs index a1660d20c..56ef87045 100644 --- a/crates/stackable-versioned/src/attrs/container.rs +++ b/crates/stackable-versioned/src/attrs/container.rs @@ -67,15 +67,15 @@ impl ContainerAttributes { // Ensure every version is unique and isn't declared multiple times. This // is inspired by the itertools all_unique function. let mut unique = HashSet::new(); - if !self - .versions - .iter() - .all(move |elem| unique.insert(elem.name)) - { - return Err(Error::custom( - "attribute `#[versioned()]` contains one or more `version`s with a duplicate `name`", - ) - .with_span(&self.versions.span())); + + for version in &*self.versions { + if !unique.insert(version.name) { + return Err(Error::custom(format!( + "attribute `#[versioned()]` contains duplicate version `name`: {name}", + name = version.name + )) + .with_span(&self.versions.span())); + } } Ok(self) From cbf7c8c25efb8d91328ca986f90ab71a0c1e5d85 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 14:20:56 +0200 Subject: [PATCH 37/43] Bump json-patch to 1.4.0 because 1.3.0 was yanked --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 83f92cbf9..dc0252be1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,7 @@ futures = "0.3.30" futures-util = "0.3.30" hyper = { version = "1.3.1", features = ["full"] } hyper-util = "0.1.3" -json-patch = "1.2.0" +json-patch = "1.4.0" k8s-openapi = { version = "0.21.1", default-features = false, features = ["schemars", "v1_29"] } # We use rustls instead of openssl for easier portablitly, e.g. so that we can build stackablectl without the need to vendor (build from source) openssl kube = { version = "0.90.0", default-features = false, features = ["client", "jsonpatch", "runtime", "derive", "rustls-tls"] } From 0fdd492baa4f81c4a10548d1d2138a39f1557da0 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 14:28:23 +0200 Subject: [PATCH 38/43] Adjust level format --- crates/k8s-version/src/api_version.rs | 5 +++-- crates/k8s-version/src/level.rs | 4 ++-- crates/k8s-version/src/lib.rs | 2 +- crates/k8s-version/src/version.rs | 7 ++++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/crates/k8s-version/src/api_version.rs b/crates/k8s-version/src/api_version.rs index 5f1507a84..527f7a6cc 100644 --- a/crates/k8s-version/src/api_version.rs +++ b/crates/k8s-version/src/api_version.rs @@ -20,8 +20,9 @@ pub enum ParseApiVersionError { /// A Kubernetes API version, following the `(/)` format. /// -/// The `` string must follow the DNS label format defined in the [Kubernetes design proposals archive][1]. -/// The `` string must be lower case and must be a valid DNS subdomain. +/// The `` string must follow the DNS label format defined in the +/// [Kubernetes design proposals archive][1]. The `` string must be lower +/// case and must be a valid DNS subdomain. /// /// ### See /// diff --git a/crates/k8s-version/src/level.rs b/crates/k8s-version/src/level.rs index f0b41d875..ae446ebe1 100644 --- a/crates/k8s-version/src/level.rs +++ b/crates/k8s-version/src/level.rs @@ -22,13 +22,13 @@ lazy_static! { /// unparsed input. #[derive(Debug, PartialEq, Snafu)] pub enum ParseLevelError { - #[snafu(display("invalid level format, expected beta/alpha"))] + #[snafu(display("invalid level format, expected alpha|beta"))] InvalidFormat, #[snafu(display("failed to parse level version"))] ParseVersion { source: ParseIntError }, - #[snafu(display("unknown level identifier"))] + #[snafu(display("unknown level identifier, expected alpha|beta"))] UnknownIdentifier, } diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 5d6461c01..7236cedab 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -2,7 +2,7 @@ //! definitions. Versions consist of three major components: the optional group, //! the mandatory major version and the optional level. The format can be //! described by `(/)`, with `` being defined as -//! `v(beta/alpha)`. +//! `v(alpha|beta)`. //! //! ## Usage //! diff --git a/crates/k8s-version/src/version.rs b/crates/k8s-version/src/version.rs index c09139b36..3a73583c7 100644 --- a/crates/k8s-version/src/version.rs +++ b/crates/k8s-version/src/version.rs @@ -29,10 +29,11 @@ pub enum ParseVersionError { ParseLevel { source: ParseLevelError }, } -/// A Kubernetes resource version, following the `v(beta/alpha)` -/// format. +/// A Kubernetes resource version, following the +/// `v(alpha)` format. /// -/// The version must follow the DNS label format defined in the [Kubernetes design proposals archive][1]. +/// The version must follow the DNS label format defined in the +/// [Kubernetes design proposals archive][1]. /// /// ### See /// From 005c20385088293c07627b8834f77a7355d70660 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 14:40:31 +0200 Subject: [PATCH 39/43] Improve derive macro test --- crates/stackable-versioned/tests/basic.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 84fa5121a..6cfa11c23 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -4,20 +4,26 @@ use stackable_versioned::Versioned; #[allow(dead_code)] #[versioned( version(name = "v1alpha1"), - version(name = "v1beta1", deprecated), - version(name = "v1beta2"), - version(name = "v1"), - version(name = "v2alpha1"), - version(name = "v2") + version(name = "v1beta1"), + version(name = "v1") )] struct Foo { /// My docs - #[versioned(added(since = "v1beta1"), renamed(since = "v1beta2", from = "jjj"))] - bar: usize, + #[versioned( + added(since = "v1alpha1"), + renamed(since = "v1beta1", from = "jjj"), + deprecated(since = "v1", _note = "") + )] + deprecated_bar: usize, baz: bool, } #[test] fn basic() { - // let _foo = v1beta1::Foo { bar: 0, baz: true }; + let _ = v1alpha1::Foo { jjj: 0, baz: false }; + let _ = v1beta1::Foo { bar: 0, baz: false }; + let _ = Foo { + deprecated_bar: 0, + baz: false, + }; } From 4944a7f4a0954143732f1127bf787c368f3c4fce Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 14:55:10 +0200 Subject: [PATCH 40/43] Add how to use the ApiVersion::new() function --- crates/k8s-version/src/lib.rs | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 7236cedab..5ef42111b 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -6,6 +6,8 @@ //! //! ## Usage //! +//! ### Parsing from [`str`] +//! //! Versions can be parsed and validated from [`str`] using Rust's standard //! [`FromStr`](std::str::FromStr) trait. //! @@ -13,26 +15,37 @@ //! # use std::str::FromStr; //! use k8s_version::ApiVersion; //! -//! let api_version = ApiVersion::from_str("extensions/v1beta1") -//! .expect("valid Kubernetes API version"); +//! let api_version = ApiVersion::from_str("extensions/v1beta1"); +//! assert!(api_version.is_ok()); //! //! // Or using .parse() -//! let api_version: ApiVersion = "extensions/v1beta1".parse() -//! .expect("valid Kubernetes API version"); +//! let api_version: ApiVersion = "extensions/v1beta1".parse(); +//! assert!(api_version.is_ok()); //! ``` //! +//! ### Constructing +//! //! Alternatively, they can be constructed programatically using the -//! [`new()`](ApiVersion::new) and [`try_new`](ApiVersion::try_new) function. +//! [`ApiVersion::new()`] and [`ApiVersion::try_new()`] functions. //! //! ``` -//! use k8s_version::{ApiVersion, Version, Level}; +//! # use std::str::FromStr; +//! use k8s_version::{ApiVersion, Version, Level, Group}; +//! +//! let version = Version::new(1, Some(Level::Beta(1))); +//! let group = Group::from_str().unwrap(); +//! let api_version = ApiVersion::new(Some(group), version); +//! +//! assert_eq!(api_version.to_string(), "extension/v1beta1"); //! +//! // Or using ::try_new() +//! let version = Version::new(1, Some(Level::Beta(1))); //! let api_version = ApiVersion::try_new( //! Some("extension"), -//! Version::new(1, Some(Level::Beta(1))) -//! ).expect("valid Kubernetes API version"); +//! version +//! ).unwrap(); //! -//! assert_eq!(api_version.to_string(), "extension/v1beta1") +//! assert_eq!(api_version.to_string(), "extension/v1beta1"); //! ``` // NOTE (@Techassi): Fixed in https://github.com/la10736/rstest/pull/244 but not From 3a2e052259f3b9fc7987dc84e61a7bfe31f7ad48 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 15:39:54 +0200 Subject: [PATCH 41/43] Add doc comment for FieldAttributes::validate_versions --- crates/stackable-versioned/src/attrs/field.rs | 4 +++- crates/stackable-versioned/src/gen/vstruct.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 78f773c3c..739ff691f 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -191,7 +191,9 @@ impl FieldAttributes { Ok(()) } - pub(crate) fn check_versions( + /// Validates that each field action version is present in the declared + /// container versions. + pub(crate) fn validate_versions( &self, container_attrs: &ContainerAttributes, field: &Field, diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 3a7c1abde..7f829d323 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -77,7 +77,7 @@ impl VersionedStruct { // version declared by the container attribute. for field in data.fields { let attrs = FieldAttributes::from_field(&field)?; - attrs.check_versions(&attributes, &field)?; + attrs.validate_versions(&attributes, &field)?; let versioned_field = VersionedField::new(field, attrs)?; fields.push(versioned_field); From 232f883612e6a641b83adc9e415721031b799134 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 15:49:58 +0200 Subject: [PATCH 42/43] Fix doc comment --- crates/stackable-versioned/src/attrs/field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index 739ff691f..c42ad0783 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -10,7 +10,7 @@ use crate::{attrs::container::ContainerAttributes, consts::DEPRECATED_PREFIX}; /// Data stored in this struct is validated using darling's `and_then` attribute. /// During darlings validation, it is not possible to validate that action /// versions match up with declared versions on the container. This validation -/// can be done using the associated [`FieldAttributes::check_versions`] +/// can be done using the associated [`FieldAttributes::validate_versions`] /// function. /// /// ### Field Rules From ba3fc19ce01d9b78ecdd3d2652f6dad3bcf8c279 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 6 May 2024 16:03:28 +0200 Subject: [PATCH 43/43] Fix doc tests --- crates/k8s-version/src/lib.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/k8s-version/src/lib.rs b/crates/k8s-version/src/lib.rs index 5ef42111b..034071f2e 100644 --- a/crates/k8s-version/src/lib.rs +++ b/crates/k8s-version/src/lib.rs @@ -15,12 +15,10 @@ //! # use std::str::FromStr; //! use k8s_version::ApiVersion; //! -//! let api_version = ApiVersion::from_str("extensions/v1beta1"); -//! assert!(api_version.is_ok()); +//! let api_version = ApiVersion::from_str("extensions/v1beta1").unwrap(); //! //! // Or using .parse() -//! let api_version: ApiVersion = "extensions/v1beta1".parse(); -//! assert!(api_version.is_ok()); +//! let api_version: ApiVersion = "extensions/v1beta1".parse().unwrap(); //! ``` //! //! ### Constructing @@ -33,7 +31,7 @@ //! use k8s_version::{ApiVersion, Version, Level, Group}; //! //! let version = Version::new(1, Some(Level::Beta(1))); -//! let group = Group::from_str().unwrap(); +//! let group = Group::from_str("extension").unwrap(); //! let api_version = ApiVersion::new(Some(group), version); //! //! assert_eq!(api_version.to_string(), "extension/v1beta1");