feat(stackable-versioned): Improve action chain generation

crates/stackable-versioned/CHANGELOG.md

@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+- Improve action chain generation ([#784]).
+
+[#784](ttps://github.com/stackabletech/operator-rs/pull/784)
+
 ## [0.1.0] - 2024-05-08
 
 ### Changed

crates/stackable-versioned/Cargo.toml

@@ -16,3 +16,6 @@ darling.workspace = true
 proc-macro2.workspace = true
 syn.workspace = true
 quote.workspace = true
+
+[dev-dependencies]
+rstest.workspace = true

crates/stackable-versioned/src/attrs/field.rs

@@ -51,7 +51,7 @@ pub(crate) struct RenamedAttributes {
 #[derive(Clone, Debug, FromMeta)]
 pub(crate) struct DeprecatedAttributes {
     pub(crate) since: SpannedValue<Version>,
-    pub(crate) _note: SpannedValue<String>,
+    pub(crate) note: SpannedValue<String>,
 }
 
 impl FieldAttributes {
@@ -64,10 +64,14 @@ impl FieldAttributes {
     fn validate(self) -> Result<Self, Error> {
         let mut errors = Error::accumulator();
 
+        // Semantic validation
         errors.handle(self.validate_action_combinations());
         errors.handle(self.validate_action_order());
         errors.handle(self.validate_field_name());
 
+        // Code quality validation
+        errors.handle(self.validate_deprecated_options());
+
         // TODO (@Techassi): Add validation for renames so that renamed fields
         // match up and form a continous chain (eg. foo -> bar -> baz).
 
@@ -191,6 +195,22 @@ impl FieldAttributes {
         Ok(())
     }
 
+    fn validate_deprecated_options(&self) -> Result<(), Error> {
+        // TODO (@Techassi): Make the field 'note' optional, because in the
+        // future, the macro will generate parts of the deprecation note
+        // automatically. The user-provided note will then be appended to the
+        // auto-generated one.
+
+        if let Some(deprecated) = &self.deprecated {
+            if deprecated.note.is_empty() {
+                return Err(Error::custom("deprecation note must not be empty")
+                    .with_span(&deprecated.note.span()));
+            }
+        }
+
+        Ok(())
+    }
+
     /// Validates that each field action version is present in the declared
     /// container versions.
     pub(crate) fn validate_versions(

crates/stackable-versioned/src/gen/field.rs

@@ -9,7 +9,7 @@ use syn::{Field, Ident};
 use crate::{
     attrs::field::FieldAttributes,
     consts::DEPRECATED_PREFIX,
-    gen::{version::ContainerVersion, ToTokensExt},
+    gen::{neighbors::Neighbors, version::ContainerVersion, ToTokensExt},
 };
 
 /// A versioned field, which contains contains common [`Field`] data and a chain
@@ -36,96 +36,29 @@ impl ToTokensExt for VersionedField {
                 // 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) => {
-                            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 => {
-                        // 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 {
-                                    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,
-                                        });
-                                    }
-                                }
-                            }
-                        }
-
-                        // 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 {
-                                    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
-                    }
+                let field_type = &self.inner.ty;
+
+                match chain
+                    .get(&container_version.inner)
+                    .expect("internal error: chain must contain container version")
+                {
+                    FieldStatus::Added(field_ident) => Some(quote! {
+                        pub #field_ident: #field_type,
+                    }),
+                    FieldStatus::Renamed { _from: _, to } => Some(quote! {
+                        pub #to: #field_type,
+                    }),
+                    FieldStatus::Deprecated {
+                        ident: field_ident,
+                        note,
+                    } => Some(quote! {
+                        #[deprecated = #note]
+                        pub #field_ident: #field_type,
+                    }),
+                    FieldStatus::NotPresent => None,
+                    FieldStatus::NoChange(field_ident) => Some(quote! {
+                        pub #field_ident: #field_type,
+                    }),
                 }
             }
             None => {
@@ -144,11 +77,16 @@ impl ToTokensExt for VersionedField {
 }
 
 impl VersionedField {
+    /// Create a new versioned field by creating a status chain for each version
+    /// defined in an action in the field attribute.
+    ///
+    /// This chain will get extended by the versions defined on the container by
+    /// calling the [`VersionedField::insert_container_versions`] function.
     pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Result<Self, Error> {
-        // Constructing the change chain requires going through the actions from
+        // Constructing the action chain requires going through the actions from
         // 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
+        // needs to go through the actions in reverse order, as otherwise it is
         // impossible to extract the field ident for each version.
 
         // Deprecating a field is always the last state a field can end up in. For
@@ -160,7 +98,13 @@ impl VersionedField {
             let mut actions = BTreeMap::new();
 
             let ident = field.ident.as_ref().unwrap();
-            actions.insert(*deprecated.since, FieldStatus::Deprecated(ident.clone()));
+            actions.insert(
+                *deprecated.since,
+                FieldStatus::Deprecated {
+                    ident: ident.clone(),
+                    note: deprecated.note.to_string(),
+                },
+            );
 
             // When the field is deprecated, any rename which occured beforehand
             // requires access to the field ident to infer the field ident for
@@ -175,7 +119,7 @@ impl VersionedField {
                 actions.insert(
                     *rename.since,
                     FieldStatus::Renamed {
-                        from: from.clone(),
+                        _from: from.clone(),
                         to: ident,
                     },
                 );
@@ -201,7 +145,7 @@ impl VersionedField {
                 actions.insert(
                     *rename.since,
                     FieldStatus::Renamed {
-                        from: from.clone(),
+                        _from: from.clone(),
                         to: ident,
                     },
                 );
@@ -241,11 +185,58 @@ impl VersionedField {
             })
         }
     }
+
+    /// Inserts container versions not yet present in the status chain.
+    ///
+    /// When intially creating a new [`VersionedField`], the code doesn't have
+    /// access to the versions defined on the container. This function inserts
+    /// all non-present container versions and decides which status and ident
+    /// is the right fit based on the status neighbors.
+    ///
+    /// This continous chain ensures that when generating code (tokens), each
+    /// field can lookup the status for a requested version.
+    pub(crate) fn insert_container_versions(&mut self, versions: &Vec<ContainerVersion>) {
+        if let Some(chain) = &mut self.chain {
+            for version in versions {
+                if chain.contains_key(&version.inner) {
+                    continue;
+                }
+
+                match chain.get_neighbors(&version.inner) {
+                    (None, Some(_)) => chain.insert(version.inner, FieldStatus::NotPresent),
+                    (Some(status), None) => {
+                        let ident = match status {
+                            FieldStatus::Added(ident) => ident,
+                            FieldStatus::Renamed { _from: _, to } => to,
+                            FieldStatus::Deprecated { ident, note: _ } => ident,
+                            FieldStatus::NoChange(ident) => ident,
+                            FieldStatus::NotPresent => unreachable!(),
+                        };
+
+                        chain.insert(version.inner, FieldStatus::NoChange(ident.clone()))
+                    }
+                    (Some(status), Some(_)) => {
+                        let ident = match status {
+                            FieldStatus::Added(ident) => ident,
+                            FieldStatus::Renamed { _from: _, to } => to,
+                            FieldStatus::NoChange(ident) => ident,
+                            _ => unreachable!(),
+                        };
+
+                        chain.insert(version.inner, FieldStatus::NoChange(ident.clone()))
+                    }
+                    _ => unreachable!(),
+                };
+            }
+        }
+    }
 }
 
 #[derive(Debug)]
 pub(crate) enum FieldStatus {
     Added(Ident),
-    Renamed { from: Ident, to: Ident },
-    Deprecated(Ident),
+    Renamed { _from: Ident, to: Ident },
+    Deprecated { ident: Ident, note: String },
+    NoChange(Ident),
+    NotPresent,
 }

crates/stackable-versioned/src/gen/mod.rs

@@ -9,6 +9,7 @@ use crate::{
 };
 
 pub(crate) mod field;
+pub(crate) mod neighbors;
 pub(crate) mod venum;
 pub(crate) mod version;
 pub(crate) mod vstruct;

crates/stackable-versioned/src/gen/neighbors.rs

@@ -0,0 +1,71 @@
+use std::{collections::BTreeMap, ops::Bound};
+
+pub(crate) trait Neighbors<K, V>
+where
+    K: Ord + Eq,
+{
+    fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>);
+
+    fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
+    fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
+}
+
+impl<K, V> Neighbors<K, V> for BTreeMap<K, V>
+where
+    K: Ord + Eq,
+{
+    fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>) {
+        // NOTE (@Techassi): These functions might get added to the standard
+        // library at some point. If that's the case, we can use the ones
+        // provided by the standard lib.
+        // See: https://github.com/rust-lang/rust/issues/107540
+        match (
+            self.lo_bound(Bound::Excluded(key)),
+            self.up_bound(Bound::Excluded(key)),
+        ) {
+            (Some((k, v)), None) => {
+                if key > k {
+                    (Some(v), None)
+                } else {
+                    (self.lo_bound(Bound::Excluded(k)).map(|(_, v)| v), None)
+                }
+            }
+            (None, Some((k, v))) => {
+                if key < k {
+                    (None, Some(v))
+                } else {
+                    (None, self.up_bound(Bound::Excluded(k)).map(|(_, v)| v))
+                }
+            }
+            (Some((_, lo)), Some((_, up))) => (Some(lo), Some(up)),
+            (None, None) => unreachable!(),
+        }
+    }
+
+    fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> {
+        self.range((Bound::Unbounded, bound)).next_back()
+    }
+
+    fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> {
+        self.range((bound, Bound::Unbounded)).next()
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use rstest::rstest;
+
+    #[rstest]
+    #[case(0, (None, Some(&"test1")))]
+    #[case(1, (None, Some(&"test3")))]
+    #[case(2, (Some(&"test1"), Some(&"test3")))]
+    #[case(3, (Some(&"test1"), None))]
+    #[case(4, (Some(&"test3"), None))]
+    fn test(#[case] key: i32, #[case] expected: (Option<&&str>, Option<&&str>)) {
+        let map = BTreeMap::from([(1, "test1"), (3, "test3")]);
+        let neigbors = map.get_neighbors(&key);
+
+        assert_eq!(neigbors, expected);
+    }
+}