From 691af5cadec534630d159d20174b186677a77313 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Sat, 15 Jun 2019 11:41:14 +0200 Subject: [PATCH] add the ParseCli trait In order to get parameters from the command line into the API we need to get them into a json value, so that we can pass it down the handler which deserializes them into their real type and runs verifications. For this we need to define how the type is going to be converted into a json Value. We cannot simply use Deserialize as that would for instance require quotes around strings. So instead, we have a ParseCli trait, which is a nop (direct serde_json::Value::String()) for string types, and uses .parse() (the std::str::FromStr trait) for everything else. Currently this uses a `default fn` as an example of the specialization feature, but I'll probably remove this and use yet another mass-impl macro since there isn't much activity on that feature's issue tracker. (The issue itself seems to be outdated ... Signed-off-by: Wolfgang Bumiller --- proxmox-api-macro/src/api_macro.rs | 2 + proxmox-api/src/api_type.rs | 32 +++++--------- proxmox-api/src/cli.rs | 71 +++++++++++++++++++++++++++++- proxmox-api/src/lib.rs | 8 ++++ proxmox-api/src/router.rs | 1 - proxmox-api/tests/router.rs | 1 + 6 files changed, 92 insertions(+), 23 deletions(-) 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 }