diff --git a/proxmox-api-macro/src/api_macro.rs b/proxmox-api-macro/src/api_macro.rs index 9e415a95..5e70289f 100644 --- a/proxmox-api-macro/src/api_macro.rs +++ b/proxmox-api-macro/src/api_macro.rs @@ -448,6 +448,7 @@ fn handle_struct_unnamed( name: stringify!(#name), description: "FIXME", complete_fn: None, // FIXME! + parse_cli: Some(<#name as ::proxmox::api::cli::ParseCli>::parse_cli), }; &INFO } @@ -493,6 +494,7 @@ fn handle_struct_named( name: stringify!(#name), description: #description, complete_fn: None, // FIXME! + parse_cli: Some(<#name as ::proxmox::api::cli::ParseCli>::parse_cli), }; &INFO } diff --git a/proxmox-api/src/api_type.rs b/proxmox-api/src/api_type.rs index 3afd92a7..23e12741 100644 --- a/proxmox-api/src/api_type.rs +++ b/proxmox-api/src/api_type.rs @@ -45,8 +45,12 @@ impl Parameter { /// Parse a commnd line option: if it is None, we only saw an `--option` without value, this is /// fine for booleans. If we saw a value, we should try to parse it out into a json value. For /// string parameters this means passing them as is, for others it means using FromStr... - pub fn parse_cli(&self, _value: Option<&str>) -> Result { - bail!("TODO"); + pub fn parse_cli(&self, name: &str, value: Option<&str>) -> Result { + let info = (self.type_info)(); + match info.parse_cli { + Some(func) => func(name, value), + None => bail!("cannot parse parameter '{}' as command line parameter", name), + } } } @@ -57,6 +61,7 @@ pub struct TypeInfo { pub name: &'static str, pub description: &'static str, pub complete_fn: Option, + pub parse_cli: Option) -> Result>, } impl TypeInfo { @@ -135,24 +140,6 @@ impl dyn ApiMethodInfo + Send + Sync { /// While this is very useful for structural types, we sometimes to want to be able to pass a /// simple unconstrainted type like a `String` with no restrictions, so most basic types implement /// `ApiType` as well. -// -// FIXME: I've actually moved most of this into the types in `api_type.rs` now, so this is -// probably unused at this point? -// `verify` should be moved to `TypeInfo` (for the type related verifier), and `Parameter` should -// get an additional verify method for constraints added by *methods*. -// -// We actually have 2 layers of validation: -// When entering the API: The type validation -// obviously a `String` should also be a string in the json object... -// This does not happen when we call the method from rust-code as we have no json layer -// there. -// When entering the function: The input validation -// if the function says `Integer`, the type itself has no validation other than that it has -// to be an integer type, but the function may still say `minimum: 5, maximum: 10`. -// This should also happen for direct calls from within rust, the `#[api]` macro can take -// care of this. -// When leaving the function: The output validation -// Yep, we need to add this ;-) pub trait ApiType { /// API types need to provide a `TypeInfo`, providing details about the underlying type. fn type_info() -> &'static TypeInfo; @@ -210,7 +197,8 @@ impl ApiType for Option { DATA.info.set(Some(TypeInfo { name: unsafe { (*DATA.name.as_ptr()).as_ref().unwrap().as_str() }, description: unsafe { (*DATA.description.as_ptr()).as_ref().unwrap().as_str() }, - complete_fn: None, + complete_fn: info.complete_fn, + parse_cli: info.parse_cli, })); }); unsafe { (*DATA.info.as_ptr()).as_ref().unwrap() } @@ -249,6 +237,7 @@ macro_rules! unconstrained_api_type { name: stringify!($type), description: stringify!($type), complete_fn: None, + parse_cli: Some(<$type as $crate::cli::ParseCli>::parse_cli), }; &INFO } @@ -276,6 +265,7 @@ impl ApiType for Response { name: "http::Response<>", description: "A raw http response", complete_fn: None, + parse_cli: None, }; &INFO } diff --git a/proxmox-api/src/cli.rs b/proxmox-api/src/cli.rs index e5b1106b..11c84424 100644 --- a/proxmox-api/src/cli.rs +++ b/proxmox-api/src/cli.rs @@ -1,8 +1,10 @@ //! Provides Command Line Interface to API methods use std::collections::HashMap; +use std::str::FromStr; use failure::{bail, format_err, Error}; +use serde::Serialize; use serde_json::Value; use super::{ApiMethodInfo, ApiOutput, Parameter}; @@ -223,7 +225,7 @@ impl Method { let param_def = self .find_parameter(arg) .ok_or_else(|| format_err!("no such parameter: '{}'", arg))?; - params.insert(arg.to_string(), param_def.parse_cli(value)?); + params.insert(arg.to_string(), param_def.parse_cli(arg, value)?); Ok(()) } @@ -252,3 +254,70 @@ fn next_arg<'a>(args: &mut std::slice::Iter<&'a str>) -> Option> { } }) } + +pub fn parse_cli_from_str(name: &str, value: Option<&str>) -> Result +where + T: FromStr + Serialize, + ::Err: Into, +{ + let this: T = value + .ok_or_else(|| format_err!("missing parameter value for '{}'", name))? + .parse() + .map_err(|e: ::Err| e.into())?; + Ok(serde_json::to_value(this)?) +} + +/// We use this trait so we can keep the "mass implementation macro" for the ApiType trait simple +/// and specialize the CLI parameter parsing via this trait separately. +pub trait ParseCli { + fn parse_cli(name: &str, value: Option<&str>) -> Result; +} + +// Saves us another mass impl macro such as the one below: +impl ParseCli for T { + default fn parse_cli(name: &str, _value: Option<&str>) -> Result { + bail!("invalid type for command line interface found for parameter '{}'", name); + } +} + +#[macro_export] +macro_rules! impl_parse_cli_from_str { + ($type:ty $(, $more:ty)*) => { + impl $crate::cli::ParseCli for $type { + fn parse_cli(name: &str, value: Option<&str>) -> Result { + parse_cli_from_str::<$type>(name, value) + } + } + + $crate::impl_parse_cli_from_str!{$($more),*} + }; + () => {}; +} + +impl_parse_cli_from_str! {bool} +impl_parse_cli_from_str! {isize, usize, i64, u64, i32, u32, i16, u16, i8, u8, f64, f32} + +impl ParseCli for Value { + fn parse_cli(name: &str, _value: Option<&str>) -> Result { + // FIXME: we could of course allow generic json parameters...? + bail!("found generic json parameter ('{}') in command line...", name); + } +} + +impl ParseCli for &str { + fn parse_cli(name: &str, value: Option<&str>) -> Result { + Ok(Value::String(value + .ok_or_else(|| format_err!("missing value for parameter '{}'", name))? + .to_string() + )) + } +} + +impl ParseCli for String { + fn parse_cli(name: &str, value: Option<&str>) -> Result { + Ok(Value::String(value + .ok_or_else(|| format_err!("missing value for parameter '{}'", name))? + .to_string() + )) + } +} diff --git a/proxmox-api/src/lib.rs b/proxmox-api/src/lib.rs index 21bcbba0..e3b9dc45 100644 --- a/proxmox-api/src/lib.rs +++ b/proxmox-api/src/lib.rs @@ -6,6 +6,14 @@ //! Note that you'll rarely need the [`Router`] type itself, as you'll most likely be creating them //! with the `router` macro provided by the `proxmox-api-macro` crate. +// This saves us some repetition (which we could do via macros), but this makes the code shorter +// and easier to review. +// FIXME: While the RFC has been approved a while ago and the implementation is there, there isn't +// much activity on the issue tracker for this, so should we remove this? +// Currently this is only used in cli.rs for a `default fn` which could instead made explicit for +// our types +#![feature(specialization)] + use std::future::Future; use std::pin::Pin; diff --git a/proxmox-api/src/router.rs b/proxmox-api/src/router.rs index 39fec8f0..0e1e00b8 100644 --- a/proxmox-api/src/router.rs +++ b/proxmox-api/src/router.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; -use failure::Error; use serde_json::{json, Value}; use super::ApiMethodInfo; diff --git a/proxmox-api/tests/router.rs b/proxmox-api/tests/router.rs index 3d6b8b2d..4934c936 100644 --- a/proxmox-api/tests/router.rs +++ b/proxmox-api/tests/router.rs @@ -165,6 +165,7 @@ mod methods { name: "Thing", description: "A thing", complete_fn: None, + parse_cli: None, }; &INFO }