api-macro: support hyphenated parameter names

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

proxmox-api-macro/src/api.rs

@@ -8,7 +8,7 @@ use syn::parse::{Parse, ParseStream, Parser};
 use syn::spanned::Spanned;
 use syn::{ExprPath, Ident};
 
-use crate::util::{JSONObject, JSONValue};
+use crate::util::{JSONObject, JSONValue, SimpleIdent};
 
 mod enums;
 mod method;
@@ -90,7 +90,7 @@ impl TryFrom<JSONObject> for Schema {
             properties: obj.into_iter().try_fold(
                 Vec::new(),
                 |mut properties, (key, value)| -> Result<_, syn::Error> {
-                    properties.push((key.into_ident()?, value.try_into()?));
+                    properties.push((key.into_ident(), value.try_into()?));
                     Ok(properties)
                 },
             )?,
@@ -133,13 +133,20 @@ impl Schema {
         }
     }
 
-    fn find_object_property(&self, key: &str) -> Option<(bool, &PropertySchema)> {
-        self.as_object().and_then(|obj| obj.find_property(key))
+    fn find_obj_property_by_ident(
+        &self,
+        key: &str,
+    ) -> Option<&(SimpleIdent, bool, PropertySchema)> {
+        self.as_object()
+            .and_then(|obj| obj.find_property_by_ident(key))
     }
 
-    fn find_object_property_mut(&mut self, key: &str) -> Option<(&mut bool, &mut PropertySchema)> {
+    fn find_obj_property_by_ident_mut(
+        &mut self,
+        key: &str,
+    ) -> Option<&mut (SimpleIdent, bool, PropertySchema)> {
         self.as_object_mut()
-            .and_then(|obj| obj.find_property_mut(key))
+            .and_then(|obj| obj.find_property_by_ident_mut(key))
     }
 }
 
@@ -312,7 +319,7 @@ impl PropertySchema {
 #[derive(Default)]
 /// Contains a sorted list of properties:
 struct SchemaObject {
-    properties: Vec<(String, bool, PropertySchema)>,
+    properties: Vec<(SimpleIdent, bool, PropertySchema)>,
 }
 
 impl SchemaObject {
@@ -347,7 +354,7 @@ impl SchemaObject {
                             None => PropertySchema::Schema(schema.try_into()?),
                         };
 
-                        properties.push((key.to_string(), optional, schema));
+                        properties.push((key, optional, schema));
 
                         Ok(properties)
                     },
@@ -362,7 +369,7 @@ impl SchemaObject {
 
     fn to_schema_inner(&self, ts: &mut TokenStream) -> Result<(), Error> {
         for element in self.properties.iter() {
-            let key = &element.0;
+            let key = element.0.as_str();
             let optional = element.1;
             let mut schema = TokenStream::new();
             element.2.to_schema(&mut schema)?;
@@ -371,25 +378,25 @@ impl SchemaObject {
         Ok(())
     }
 
-    fn find_property(&self, key: &str) -> Option<(bool, &PropertySchema)> {
+    fn find_property_by_ident(&self, key: &str) -> Option<&(SimpleIdent, bool, PropertySchema)> {
         match self
             .properties
-            .binary_search_by(|prope| prope.0.as_str().cmp(key))
+            .binary_search_by(|p| p.0.as_ident_str().cmp(key))
         {
-            Ok(idx) => Some((self.properties[idx].1, &self.properties[idx].2)),
+            Ok(idx) => Some(&self.properties[idx]),
             Err(_) => None,
         }
     }
 
-    fn find_property_mut(&mut self, key: &str) -> Option<(&mut bool, &mut PropertySchema)> {
+    fn find_property_by_ident_mut(
+        &mut self,
+        key: &str,
+    ) -> Option<&mut (SimpleIdent, bool, PropertySchema)> {
         match self
             .properties
-            .binary_search_by(|prope| prope.0.as_str().cmp(key))
+            .binary_search_by(|p| p.0.as_ident_str().cmp(key))
         {
-            Ok(idx) => {
-                let props = &mut self.properties[idx];
-                Some((&mut props.1, &mut props.2))
-            }
+            Ok(idx) => Some(&mut self.properties[idx]),
             Err(_) => None,
         }
     }

proxmox-api-macro/src/api/method.rs

@@ -202,8 +202,8 @@ fn handle_function_signature(
         let (pat_type, pat) = check_input_type(input)?;
 
         // For any named type which exists on the function signature...
-        let schema: &mut Schema = if let Some((_optional, schema)) =
-            input_schema.find_object_property_mut(&pat.ident.to_string())
+        let schema: &mut Schema = if let Some((_ident, _optional, schema)) =
+            input_schema.find_obj_property_by_ident_mut(&pat.ident.to_string())
         {
             match schema {
                 PropertySchema::Schema(schema) => match &mut schema.item {
@@ -268,8 +268,9 @@ fn handle_function_signature(
         //
         //     5) Finally, if none of the above conditions are met, we do not know what to do and
         //        bail out with an error.
-        let param_type = if let Some((optional, schema)) =
-            input_schema.find_object_property(&pat.ident.to_string())
+        let mut param_name: SimpleIdent = pat.ident.clone().into();
+        let param_type = if let Some((name, optional, schema)) =
+            input_schema.find_obj_property_by_ident(&pat.ident.to_string())
         {
             match schema {
                 PropertySchema::Schema(schema) => match &schema.item {
@@ -278,8 +279,9 @@ fn handle_function_signature(
                 },
                 _ => (),
             }
+            param_name = name.clone();
             // Found an explicit parameter: extract it:
-            ParameterType::Other(&pat_type.ty, optional, schema)
+            ParameterType::Other(&pat_type.ty, *optional, schema)
         } else if is_api_method_type(&pat_type.ty) {
             if api_method_param.is_some() {
                 bail!(pat_type => "multiple ApiMethod parameters found");
@@ -302,7 +304,7 @@ fn handle_function_signature(
             bail!(&pat.ident => "unexpected parameter");
         };
 
-        param_list.push((pat.ident.clone().into(), param_type));
+        param_list.push((param_name, param_type));
     }
 
     /*
@@ -396,8 +398,9 @@ fn create_wrapper_function(
             ParameterType::ApiMethod => args.extend(quote_spanned! { span => api_method_param, }),
             ParameterType::RpcEnv => args.extend(quote_spanned! { span => rpc_env_param, }),
             ParameterType::Other(_ty, optional, _schema) => {
-                let name_str = syn::LitStr::new(&name.to_string(), span);
-                let arg_name = Ident::new(&format!("input_arg_{}", name), span);
+                let name_str = syn::LitStr::new(name.as_str(), span);
+                let arg_name =
+                    Ident::new(&format!("input_arg_{}", name.as_ident().to_string()), span);
 
                 // Optional parameters are expected to be Option<> types in the real function
                 // signature, so we can just keep the returned Option from `input_map.remove()`.

proxmox-api-macro/src/util.rs

@@ -1,7 +1,6 @@
 use std::borrow::Borrow;
 use std::collections::HashMap;
 use std::convert::TryFrom;
-use std::fmt;
 
 use proc_macro2::{Ident, Span};
 use syn::parse::{Parse, ParseStream};
@@ -18,34 +17,50 @@ use syn::Token;
 /// and therefore we do not implement `Into<Ident>` anymore, but the user needs to explicitly ask
 /// for it via the `.into_ident()` method.
 #[derive(Clone, Debug)]
-pub struct SimpleIdent(Ident, String);
+pub struct SimpleIdent {
+    ident: Ident,
+    ident_str: String, // cached string version to avoid all the .to_string() calls
+    string: String,    // hyphenated version
+}
 
 impl SimpleIdent {
     pub fn new(name: String, span: Span) -> Self {
-        Self(Ident::new(&name.replace("-", "_"), span), name)
+        let ident_str = name.replace("-", "_");
+
+        Self {
+            ident: Ident::new(&ident_str, span),
+            ident_str,
+            string: name,
+        }
     }
 
     #[inline]
     pub fn as_str(&self) -> &str {
-        &self.1
+        &self.string
     }
 
     #[inline]
-    pub unsafe fn into_ident_unchecked(self) -> Ident {
-        self.0
+    pub fn as_ident_str(&self) -> &str {
+        &self.ident_str
     }
 
     #[inline]
-    pub fn into_ident(self) -> Result<Ident, syn::Error> {
-        if self.1.as_bytes().contains(&b'-') {
-            bail!(self.0 => "invalid identifier: '{}'", self.1);
-        }
-        Ok(unsafe { self.into_ident_unchecked() })
+    pub fn as_ident(&self) -> &Ident {
+        &self.ident
+    }
+
+    #[inline]
+    pub fn into_ident(self) -> Ident {
+        self.ident
     }
 
     #[inline]
     pub fn span(&self) -> Span {
-        self.0.span()
+        self.ident.span()
+    }
+
+    pub fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.string.cmp(&other.string)
     }
 }
 
@@ -53,26 +68,24 @@ impl Eq for SimpleIdent {}
 
 impl PartialEq for SimpleIdent {
     fn eq(&self, other: &Self) -> bool {
-        self.1 == other.1
+        self.string == other.string
     }
 }
 
 impl std::hash::Hash for SimpleIdent {
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        std::hash::Hash::hash(&self.1, state)
+        std::hash::Hash::hash(&self.string, state)
     }
 }
 
 impl From<Ident> for SimpleIdent {
     fn from(ident: Ident) -> Self {
-        let s = ident.to_string();
-        Self(ident, s)
-    }
-}
-
-impl fmt::Display for SimpleIdent {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Display::fmt(&self.0, f)
+        let string = ident.to_string();
+        Self {
+            ident,
+            ident_str: string.clone(),
+            string,
+        }
     }
 }
 
@@ -87,15 +100,16 @@ impl Borrow<str> for SimpleIdent {
 impl Parse for SimpleIdent {
     fn parse(input: ParseStream) -> syn::Result<Self> {
         let lookahead = input.lookahead1();
-        Ok(Self::from(if lookahead.peek(Token![type]) {
+        Ok(if lookahead.peek(Token![type]) {
             let ty: Token![type] = input.parse()?;
-            Ident::new("type", ty.span)
+            Self::new("type".to_string(), ty.span)
         } else if lookahead.peek(syn::LitStr) {
             let s: syn::LitStr = input.parse()?;
-            Ident::new(&s.value(), s.span())
+            Self::new(s.value(), s.span())
         } else {
-            input.parse()?
-        }))
+            let ident: Ident = input.parse()?;
+            Self::from(ident)
+        })
     }
 }
 
@@ -270,7 +284,7 @@ impl JSONObject {
         let mut elems = HashMap::with_capacity(map_elems.len());
         for c in map_elems {
             if elems.insert(c.key.clone().into(), c.value).is_some() {
-                bail!(c.key.span(), "duplicate '{}' in schema", c.key);
+                bail!(c.key.span(), "duplicate '{}' in schema", c.key.as_str());
             }
         }
         Ok(elems)

proxmox-api-macro/tests/api2.rs

@@ -1,7 +1,6 @@
 use proxmox_api_macro::api;
 
 use failure::Error;
-use serde_json::Value;
 
 #[api(
     input: {

proxmox-api-macro/tests/ext-schema.rs

@@ -13,14 +13,14 @@ pub const NAME_SCHEMA: schema::Schema = schema::StringSchema::new("Archive name.
 #[api(
     input: {
         properties: {
-            name: {
+            "archive-name": {
                 schema: NAME_SCHEMA,
             }
         }
     }
 )]
 /// Get an archive.
-pub fn get_archive(name: String) -> Result<(), Error> {
-    let _ = name;
+pub fn get_archive(archive_name: String) -> Result<(), Error> {
+    let _ = archive_name;
     Ok(())
 }