]> git.proxmox.com Git - proxmox.git/commitdiff
more updater cleanups
authorWolfgang Bumiller <w.bumiller@proxmox.com>
Fri, 13 Aug 2021 09:11:27 +0000 (11:11 +0200)
committerWolfgang Bumiller <w.bumiller@proxmox.com>
Fri, 13 Aug 2021 09:11:27 +0000 (11:11 +0200)
* Updatable is now named UpdaterType
* UPDATER_IS_OPTION is now assumed to always be true
    While an updater can be a non-optional struct, being an
    updater, all its fields are also Updaters, so after
    expanding all levels of nesting, the resulting list of
    fields can only contain optional values.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
proxmox-api-macro/src/api/enums.rs
proxmox-api-macro/src/api/mod.rs
proxmox-api-macro/src/api/structs.rs
proxmox-api-macro/src/lib.rs
proxmox-api-macro/src/updater.rs
proxmox-api-macro/tests/allof.rs
proxmox-api-macro/tests/int-limits.rs
proxmox-api-macro/tests/types.rs
proxmox-api-macro/tests/updater.rs
proxmox/src/api/schema.rs

index 344ffa39d8bb23142156bb4304dcfc20467a1eb7..3c35bc095482723d07589a523ae66a7d71f4be2e 100644 (file)
@@ -85,10 +85,8 @@ pub fn handle_enum(
                 .schema();
         }
 
-        #[automatically_derived]
-        impl ::proxmox::api::schema::Updatable for #name {
+        impl ::proxmox::api::schema::UpdaterType for #name {
             type Updater = Option<Self>;
-            const UPDATER_IS_OPTION: bool = true;
         }
     })
 }
index 5a644b3517df38fb00edec0e583d350f209b5028..26b8e43719e80f4ee32ccd4a552c62aae366edde 100644 (file)
@@ -473,9 +473,7 @@ impl ToTokens for OptionType {
     fn to_tokens(&self, tokens: &mut TokenStream) {
         match self {
             OptionType::Bool(b) => b.to_tokens(tokens),
-            OptionType::Updater(ty) => tokens.extend(quote! {
-                <#ty as ::proxmox::api::schema::Updatable>::UPDATER_IS_OPTION
-            }),
+            OptionType::Updater(_) => tokens.extend(quote! { true }),
         }
     }
 }
index f59c3940e92e9e1e14061cfd17b6fe50f41007af..4bc6b75902df0f72f27f5cf76bdc722c82509b2b 100644 (file)
@@ -62,9 +62,8 @@ fn handle_unit_struct(attribs: JSONObject, stru: syn::ItemStruct) -> Result<Toke
     let name = &stru.ident;
     let mut schema = finish_schema(schema, &stru, name)?;
     schema.extend(quote_spanned! { name.span() =>
-        impl ::proxmox::api::schema::Updatable for #name {
+        impl ::proxmox::api::schema::UpdaterType for #name {
             type Updater = Option<Self>;
-            const UPDATER_IS_OPTION: bool = true;
         }
     });
 
@@ -404,6 +403,7 @@ fn derive_updater(
     mut schema: Schema,
     original_struct: &mut syn::ItemStruct,
 ) -> Result<TokenStream, Error> {
+    let original_name = &original_struct.ident;
     stru.ident = Ident::new(&format!("{}Updater", stru.ident), stru.ident.span());
 
     if !util::derived_items(&original_struct.attrs).any(|p| p.is_ident("Default")) {
@@ -413,19 +413,7 @@ fn derive_updater(
         ));
     }
 
-    original_struct.attrs.push(util::make_derive_attribute(
-        Span::call_site(),
-        quote::quote! { ::proxmox::api::schema::Updatable },
-    ));
-
     let updater_name = &stru.ident;
-    let updater_name_str = syn::LitStr::new(&updater_name.to_string(), updater_name.span());
-    original_struct.attrs.push(util::make_attribute(
-        Span::call_site(),
-        util::make_path(Span::call_site(), false, &["updatable"]),
-        quote::quote! { (updater = #updater_name_str) },
-    ));
-
     let mut all_of_schemas = TokenStream::new();
     let mut is_empty_impl = TokenStream::new();
 
@@ -454,16 +442,22 @@ fn derive_updater(
     };
 
     if !is_empty_impl.is_empty() {
-        output = quote::quote!(
-            #output
+        output.extend(quote::quote!(
+            #[automatically_derived]
             impl ::proxmox::api::schema::Updater for #updater_name {
                 fn is_empty(&self) -> bool {
                     #is_empty_impl
                 }
             }
-        );
+        ));
     }
 
+    output.extend(quote::quote!(
+        impl ::proxmox::api::schema::UpdaterType for #original_name {
+            type Updater = #updater_name;
+        }
+    ));
+
     Ok(output)
 }
 
@@ -518,7 +512,7 @@ fn handle_updater_field(
         path: util::make_path(
             span,
             true,
-            &["proxmox", "api", "schema", "Updatable", "Updater"],
+            &["proxmox", "api", "schema", "UpdaterType", "Updater"],
         ),
     };
 
@@ -535,7 +529,8 @@ fn handle_updater_field(
 
     if field_schema.flatten_in_struct {
         let updater_ty = &field.ty;
-        all_of_schemas.extend(quote::quote! {&<#updater_ty as ::proxmox::api::schema::ApiType>::API_SCHEMA,});
+        all_of_schemas
+            .extend(quote::quote! {&<#updater_ty as ::proxmox::api::schema::ApiType>::API_SCHEMA,});
     }
 
     if !is_empty_impl.is_empty() {
index 2db11379ab1864abd1ccc246859aca70a6ecbaef..9c35eeff2ef5a0807de0ced3a7e5b54274f06bff 100644 (file)
@@ -231,7 +231,7 @@ fn router_do(item: TokenStream) -> Result<TokenStream, Error> {
 
     # Deriving an `Updater`:
 
-    An "Updater" struct can be generated automatically for a type. This affects the `Updatable`
+    An "Updater" struct can be generated automatically for a type. This affects the `UpdaterType`
     trait implementation generated, as it will set the associated
     `type Updater = TheDerivedUpdater`.
 
@@ -265,10 +265,10 @@ fn router_do(item: TokenStream) -> Result<TokenStream, Error> {
     #[api]
     /// An example of a simple struct type.
     pub struct MyTypeUpdater {
-        one: Option<String>, // really <String as Updatable>::Updater
+        one: Option<String>, // really <String as UpdaterType>::Updater
 
         #[serde(skip_serializing_if = "Option::is_none")]
-        opt: Option<String>, // really <Option<String> as Updatable>::Updater
+        opt: Option<String>, // really <Option<String> as UpdaterType>::Updater
     }
 
     impl Updater for MyTypeUpdater {
@@ -277,36 +277,8 @@ fn router_do(item: TokenStream) -> Result<TokenStream, Error> {
         }
     }
 
-    impl Updatable for MyType {
+    impl UpdaterType for MyType {
         type Updater = MyTypeUpdater;
-
-        fn update_from<T>(&mut self, from: MyTypeUpdater, delete: &[T]) -> Result<(), Error>
-        where
-            T: AsRef<str>,
-        {
-            for delete in delete {
-                match delete.as_ref() {
-                    "opt" => { self.opt = None; }
-                    _ => (),
-                }
-            }
-
-            self.one.update_from(from.one)?;
-            self.opt.update_from(from.opt)?;
-
-            Ok(())
-        }
-
-        fn try_build_from(from: MyTypeUpdater) -> Result<Self, Error> {
-            Ok(Self {
-                // This amounts to `from.one.ok_or_else("cannot build from None value")?`
-                one: Updatable::try_build_from(from.one)
-                    .map_err(|err| format_err!("failed to build value for field 'one': {}", err))?,
-                // This amounts to `from.opt`
-                opt: Updatable::try_build_from(from.opt)
-                    .map_err(|err| format_err!("failed to build value for field 'opt': {}", err))?,
-            })
-        }
     }
 
     ```
@@ -320,17 +292,17 @@ pub fn api(attr: TokenStream_1, item: TokenStream_1) -> TokenStream_1 {
 
 /// This is a dummy derive macro actually handled by `#[api]`!
 #[doc(hidden)]
-#[proc_macro_derive(Updater, attributes(updater, updatable, serde))]
+#[proc_macro_derive(Updater, attributes(updater, serde))]
 pub fn derive_updater(_item: TokenStream_1) -> TokenStream_1 {
     TokenStream_1::new()
 }
 
-/// Create the default `Updatable` implementation from an `Option<Self>`.
-#[proc_macro_derive(Updatable, attributes(updatable, serde))]
-pub fn derive_updatable(item: TokenStream_1) -> TokenStream_1 {
+/// Create the default `UpdaterType` implementation as an `Option<Self>`.
+#[proc_macro_derive(UpdaterType, attributes(updater_type, serde))]
+pub fn derive_updater_type(item: TokenStream_1) -> TokenStream_1 {
     let _error_guard = init_local_error();
     let item: TokenStream = item.into();
-    handle_error(item.clone(), updater::updatable(item).map_err(Error::from)).into()
+    handle_error(item.clone(), updater::updater_type(item).map_err(Error::from)).into()
 }
 
 thread_local!(static NON_FATAL_ERRORS: RefCell<Option<TokenStream>> = RefCell::new(None));
index b596f13c7a7532f2655256cb34deeb022e674908..aad0e95c0b602d615ddc8bc071e47f1f0cb31c92 100644 (file)
 use proc_macro2::{Ident, Span, TokenStream};
-use quote::{quote, quote_spanned};
+use quote::quote_spanned;
 use syn::spanned::Spanned;
 
-pub(crate) fn updatable(item: TokenStream) -> Result<TokenStream, syn::Error> {
+pub(crate) fn updater_type(item: TokenStream) -> Result<TokenStream, syn::Error> {
     let item: syn::Item = syn::parse2(item)?;
     let full_span = item.span();
-    match item {
+    Ok(match item {
         syn::Item::Struct(syn::ItemStruct {
-            fields: syn::Fields::Named(named),
-            attrs,
             ident,
             generics,
             ..
-        }) => derive_named_struct_updatable(attrs, full_span, ident, generics, named),
-        syn::Item::Struct(syn::ItemStruct {
-            attrs,
-            ident,
-            generics,
-            ..
-        }) => derive_default_updatable(attrs, full_span, ident, generics),
+        }) => derive_updater_type(full_span, ident, generics),
         syn::Item::Enum(syn::ItemEnum {
-            attrs,
             ident,
             generics,
             ..
-        }) => derive_default_updatable(attrs, full_span, ident, generics),
-        _ => bail!(item => "`Updatable` can only be derived for structs"),
-    }
+        }) => derive_updater_type(full_span, ident, generics),
+        _ => bail!(item => "`UpdaterType` cannot be derived for this type"),
+    })
 }
 
 fn no_generics(generics: syn::Generics) {
     if let Some(lt) = generics.lt_token {
-        error!(lt => "deriving `Updatable` for a generic enum is not supported");
+        error!(lt => "deriving `UpdaterType` for a generic enum is not supported");
     } else if let Some(wh) = generics.where_clause {
-        error!(wh => "deriving `Updatable` on enums with generic bounds is not supported");
+        error!(wh => "deriving `UpdaterType` on enums with generic bounds is not supported");
     }
 }
 
-fn derive_default_updatable(
-    attrs: Vec<syn::Attribute>,
+fn derive_updater_type(
     full_span: Span,
     ident: Ident,
     generics: syn::Generics,
-) -> Result<TokenStream, syn::Error> {
+) -> TokenStream {
     no_generics(generics);
 
-    let args = UpdatableArgs::from_attributes(attrs);
-    if let Some(updater) = args.updater {
-        error!(updater => "`updater` updater attribute not supported for this type");
-    }
-
-    Ok(default_updatable(full_span, ident))
-}
-
-fn default_updatable(full_span: Span, ident: Ident) -> TokenStream {
     quote_spanned! { full_span =>
-        #[automatically_derived]
-        impl ::proxmox::api::schema::Updatable for #ident {
-            type Updater = Option<#ident>;
-            const UPDATER_IS_OPTION: bool = true;
+        impl ::proxmox::api::schema::UpdaterType for #ident {
+            type Updater = Option<Self>;
         }
     }
 }
-
-fn derive_named_struct_updatable(
-    attrs: Vec<syn::Attribute>,
-    full_span: Span,
-    ident: Ident,
-    generics: syn::Generics,
-    _fields: syn::FieldsNamed,
-) -> Result<TokenStream, syn::Error> {
-    no_generics(generics);
-
-    let args = UpdatableArgs::from_attributes(attrs);
-    let updater = match args.updater {
-        Some(updater) => updater,
-        None => return Ok(default_updatable(full_span, ident)),
-    };
-
-    Ok(quote! {
-        #[automatically_derived]
-        impl ::proxmox::api::schema::Updatable for #ident {
-            type Updater = #updater;
-            const UPDATER_IS_OPTION: bool = false;
-        }
-    })
-}
-
-#[derive(Default)]
-struct UpdatableArgs {
-    updater: Option<syn::Type>,
-}
-
-impl UpdatableArgs {
-    fn from_attributes(attributes: Vec<syn::Attribute>) -> Self {
-        let mut this = Self::default();
-
-        for_attributes(attributes, "updatable", |meta| this.parse_nested(meta));
-
-        this
-    }
-
-    fn parse_nested(&mut self, meta: syn::NestedMeta) -> Result<(), syn::Error> {
-        match meta {
-            syn::NestedMeta::Meta(syn::Meta::NameValue(nv)) => self.parse_name_value(nv),
-            other => bail!(other => "invalid updater argument"),
-        }
-    }
-
-    fn parse_name_value(&mut self, nv: syn::MetaNameValue) -> Result<(), syn::Error> {
-        if nv.path.is_ident("updater") {
-            let updater: syn::Type = match nv.lit {
-                // we could use `s.parse()` but it doesn't make sense to put the original struct
-                // name as spanning info here, so instead, we use the call site:
-                syn::Lit::Str(s) => syn::parse_str(&s.value())?,
-                other => bail!(other => "updater argument must be a string literal"),
-            };
-
-            if self.updater.is_some() {
-                error!(updater.span(), "multiple 'updater' attributes not allowed");
-            }
-
-            self.updater = Some(updater);
-            Ok(())
-        } else {
-            bail!(nv.path => "unrecognized updater argument");
-        }
-    }
-}
-
-/// Non-fatally go through all `updater` attributes.
-fn for_attributes<F>(attributes: Vec<syn::Attribute>, attr_name: &str, mut func: F)
-where
-    F: FnMut(syn::NestedMeta) -> Result<(), syn::Error>,
-{
-    for meta in meta_iter(attributes) {
-        let list = match meta {
-            syn::Meta::List(list) if list.path.is_ident(attr_name) => list,
-            _ => continue,
-        };
-
-        for entry in list.nested {
-            match func(entry) {
-                Ok(()) => (),
-                Err(err) => crate::add_error(err),
-            }
-        }
-    }
-}
-
-fn meta_iter(
-    attributes: impl IntoIterator<Item = syn::Attribute>,
-) -> impl Iterator<Item = syn::Meta> {
-    attributes.into_iter().filter_map(|attr| {
-        if attr.style != syn::AttrStyle::Outer {
-            return None;
-        }
-
-        attr.parse_meta().ok()
-    })
-}
index 9fcf9795261eb841e67b74b299a53394fe773913..f14f6a1e040d9984b92b741e87f19652f3978850 100644 (file)
@@ -4,7 +4,7 @@ use anyhow::Error;
 use serde::{Deserialize, Serialize};
 use serde_json::{json, Value};
 
-use proxmox::api::schema;
+use proxmox::api::schema::{self, ApiType};
 use proxmox_api_macro::api;
 
 pub const NAME_SCHEMA: schema::Schema = schema::StringSchema::new("Name.").schema();
index 7606a14c5118f1ec987c884f628dd0030e759549..d8a47391e05746702b07c563a45c94c82c337b3b 100644 (file)
@@ -1,5 +1,6 @@
 //! Test the automatic addition of integer limits.
 
+use proxmox::api::schema::ApiType;
 use proxmox_api_macro::api;
 
 /// An i16: -32768 to 32767.
index 80d09ba94a5666e0ff6e263c66509ae95dd5b86e..2fd83808820d5e54cbcee7c465672241c03c30b1 100644 (file)
@@ -3,7 +3,7 @@
 
 #![allow(dead_code)]
 
-use proxmox::api::schema::{self, EnumEntry};
+use proxmox::api::schema::{self, ApiType, EnumEntry};
 use proxmox_api_macro::api;
 
 use anyhow::Error;
index 0ccde10513d6cc50e78f28d48d4d6f0438b120f6..e06624180cecf6b3449b6f50e17fdf93c6bdad5b 100644 (file)
@@ -1,16 +1,31 @@
-use proxmox::api::api;
-use proxmox::api::schema::{Schema, StringSchema, Updatable, Updater};
+#![allow(dead_code)]
 
-#[derive(Updatable)]
-pub struct Custom(String);
+use proxmox::api::api;
+use proxmox::api::schema::{ApiType, Updater, UpdaterType};
 
-impl proxmox::api::schema::ApiType for Custom {
-    const API_SCHEMA: Schema = StringSchema::new("Custom String")
-        .min_length(3)
-        .max_length(64)
-        .schema();
+// Helpers for type checks:
+struct AssertTypeEq<T>(T);
+macro_rules! assert_type_eq {
+    ($what:ident, $a:ty, $b:ty) => {
+        #[allow(dead_code, unreachable_patterns)]
+        fn $what(have: AssertTypeEq<$a>) {
+            match have {
+                AssertTypeEq::<$b>(_) => (),
+            }
+        }
+    };
 }
 
+#[api(min_length: 3, max_length: 64)]
+#[derive(UpdaterType)]
+/// Custom String.
+pub struct Custom(String);
+assert_type_eq!(
+    custom_type,
+    <Custom as UpdaterType>::Updater,
+    Option<Custom>
+);
+
 #[api]
 /// An example of a simple struct type.
 #[derive(Updater)]
@@ -19,11 +34,34 @@ pub struct Simple {
     /// A test string.
     one_field: String,
 
-    /// An optional auto-derived value for testing:
+    /// Another test value.
     #[serde(skip_serializing_if = "Option::is_empty")]
     opt: Option<String>,
 }
 
+#[test]
+fn test_simple() {
+    pub const TEST_SCHEMA: ::proxmox::api::schema::Schema =
+        ::proxmox::api::schema::ObjectSchema::new(
+            "An example of a simple struct type.",
+            &[
+                (
+                    "one-field",
+                    true,
+                    &::proxmox::api::schema::StringSchema::new("A test string.").schema(),
+                ),
+                (
+                    "opt",
+                    true,
+                    &::proxmox::api::schema::StringSchema::new("Another test value.").schema(),
+                ),
+            ],
+        )
+        .schema();
+
+    assert_eq!(TEST_SCHEMA, SimpleUpdater::API_SCHEMA);
+}
+
 #[api(
     properties: {
         simple: { type: Simple },
@@ -51,11 +89,10 @@ pub struct Complex {
 #[derive(Updater)]
 #[serde(rename_all = "kebab-case")]
 pub struct SuperComplex {
-    /// An extra field not part of the flattened struct.
+    /// An extra field.
     extra: String,
 
-    #[serde(skip_serializing_if = "Updater::is_empty")]
-    simple: Option<Simple>,
+    simple: Simple,
 
     /// A field which should not appear in the updater.
     #[updater(skip)]
@@ -64,3 +101,27 @@ pub struct SuperComplex {
     /// A custom type with an Updatable implementation.
     custom: Custom,
 }
+#[test]
+fn test_super_complex() {
+    pub const TEST_SCHEMA: ::proxmox::api::schema::Schema =
+        ::proxmox::api::schema::ObjectSchema::new(
+            "One of the baaaad cases.",
+            &[
+                ("custom", true, &<Option<Custom> as ApiType>::API_SCHEMA),
+                (
+                    "extra",
+                    true,
+                    &::proxmox::api::schema::StringSchema::new("An extra field.").schema(),
+                ),
+                (
+                    "simple",
+                    true,
+                    //&<<Simple as UpdaterType>::Updater as ApiType>::API_SCHEMA,
+                    &SimpleUpdater::API_SCHEMA,
+                ),
+            ],
+        )
+        .schema();
+
+    assert_eq!(TEST_SCHEMA, SuperComplexUpdater::API_SCHEMA);
+}
index 751dc1d196a64ab01a00ad6d0a8f36272cfd65b0..e3ee7a31df32f8411c615bd4fa25527d39bf4686 100644 (file)
@@ -1525,39 +1525,34 @@ fn test_verify_complex_array() {
 /// API types are "updatable" in order to support derived "Updater" structs more easily.
 ///
 /// By default, any API type is "updatable" by an `Option<Self>`. For types which do not use the
-/// `#[api]` macro, this will need to be explicitly created (or derived via `#[derive(Updatable)]`.
-pub trait Updatable: Sized {
+/// `#[api]` macro, this will need to be explicitly created (or derived via `#[derive(UpdaterType)]`.
+pub trait UpdaterType: Sized {
     type Updater: Updater;
-    /// This should always be true for the "default" updaters which are just `Option<T>` types.
-    /// Types which are not wrapped in `Option` must set this to `false`.
-    const UPDATER_IS_OPTION: bool;
 }
 
 #[cfg(feature = "api-macro")]
-pub use proxmox_api_macro::Updatable;
+pub use proxmox_api_macro::UpdaterType;
 
 #[cfg(feature = "api-macro")]
 #[doc(hidden)]
 pub use proxmox_api_macro::Updater;
 
-macro_rules! basic_updatable {
+macro_rules! basic_updater_type {
     ($($ty:ty)*) => {
         $(
-            impl Updatable for $ty {
+            impl UpdaterType for $ty {
                 type Updater = Option<Self>;
-                const UPDATER_IS_OPTION: bool = true;
             }
         )*
     };
 }
-basic_updatable! { bool u8 u16 u32 u64 i8 i16 i32 i64 usize isize f32 f64 String char }
+basic_updater_type! { bool u8 u16 u32 u64 i8 i16 i32 i64 usize isize f32 f64 String char }
 
-impl<T> Updatable for Option<T>
+impl<T> UpdaterType for Option<T>
 where
-    T: Updatable,
+    T: UpdaterType,
 {
     type Updater = T::Updater;
-    const UPDATER_IS_OPTION: bool = true;
 }
 
 pub trait ApiType {