diff --git a/proxmox-api-macro/src/api_macro.rs b/proxmox-api-macro/src/api_macro.rs index ccb4c460..9e415a95 100644 --- a/proxmox-api-macro/src/api_macro.rs +++ b/proxmox-api-macro/src/api_macro.rs @@ -86,13 +86,18 @@ fn handle_function( .transpose()? .unwrap_or_else(HashMap::new); let mut parameter_entries = TokenStream::new(); + let mut parameter_verifiers = TokenStream::new(); let vis = std::mem::replace(&mut item.vis, syn::Visibility::Inherited); let span = item.ident.span(); let name_str = item.ident.to_string(); - let impl_str = format!("{}_impl", name_str); - let impl_ident = Ident::new(&impl_str, span); - let name = std::mem::replace(&mut item.ident, impl_ident.clone()); + //let impl_str = format!("{}_impl", name_str); + //let impl_ident = Ident::new(&impl_str, span); + let impl_checked_str = format!("{}_checked_impl", name_str); + let impl_checked_ident = Ident::new(&impl_checked_str, span); + let impl_unchecked_str = format!("{}_unchecked_impl", name_str); + let impl_unchecked_ident = Ident::new(&impl_unchecked_str, span); + let name = std::mem::replace(&mut item.ident, impl_unchecked_ident.clone()); let mut return_type = match item.decl.output { syn::ReturnType::Default => syn::Type::Tuple(syn::TypeTuple { paren_token: syn::token::Paren { @@ -148,8 +153,26 @@ fn handle_function( }); } Expression::Expr(_) => bail!("description must be a string literal!"), - Expression::Object(_) => { - bail!("TODO: parameters with more than just a description..."); + Expression::Object(mut param_info) => { + let description = param_info + .remove("description") + .ok_or_else(|| format_err!("missing 'description' in parameter definition"))? + .expect_lit_str()?; + + parameter_entries.extend(quote! { + ::proxmox::api::Parameter { + name: #name_str, + description: #description, + type_info: <#arg_type as ::proxmox::api::ApiType>::type_info, + }, + }); + + make_parameter_verifier( + &name, + &name_str, + &mut param_info, + &mut parameter_verifiers, + )?; } } } @@ -201,7 +224,7 @@ fn handle_function( // Namespace some of our code into the helper type: impl #struct_name { - // This is the original function, renamed to `#impl_ident` + // This is the original function, renamed to `#impl_unchecked_ident` #item // This is the handler used by our router, which extracts the parameters out of a @@ -238,7 +261,7 @@ fn handle_function( ::failure::bail!("unexpected extra parameters: {}", extra); } - let output = #struct_name::#impl_ident(#extracted_args).await?; + let output = #struct_name::#impl_checked_ident(#extracted_args).await?; ::proxmox::api::IntoApiOutput::into_api_output(output) } Box::pin(handler(args)) @@ -249,6 +272,13 @@ fn handle_function( if item.asyncness.is_some() { // An async function is expected to return its value, so we wrap it a bit: body.push(quote! { + impl #struct_name { + async fn #impl_checked_ident(#inputs) -> #return_type { + #parameter_verifiers + Self::#impl_unchecked_ident(#passed_args).await + } + } + // Our helper type derefs to a wrapper performing input validation and returning a // Pin>. // Unfortunately we cannot return the actual function since that won't work for @@ -262,7 +292,7 @@ fn handle_function( const FUNC: fn(#inputs) -> ::std::pin::Pin>> = |#inputs| { - Box::pin(#struct_name::#impl_ident(#passed_args)) + Box::pin(#struct_name::#impl_checked_ident(#passed_args)) }; &FUNC } @@ -284,6 +314,13 @@ fn handle_function( }); body.push(quote! { + impl #struct_name { + fn #impl_checked_ident(#inputs) -> ::proxmox::api::ApiFuture<#body_type> { + #parameter_verifiers + Self::#impl_unchecked_ident(#passed_args) + } + } + // Our helper type derefs to a wrapper performing input validation and returning a // Pin>. // Unfortunately we cannot return the actual function since that won't work for @@ -292,10 +329,7 @@ fn handle_function( type Target = fn(#inputs) -> ::proxmox::api::ApiFuture<#body_type>; fn deref(&self) -> &Self::Target { - const FUNC: fn(#inputs) -> ::proxmox::api::ApiFuture<#body_type> = |#inputs| { - #struct_name::#impl_ident(#passed_args) - }; - &FUNC + &(Self::#impl_checked_ident as Self::Target) } } }); @@ -340,6 +374,37 @@ fn handle_function( Ok(body) } +fn make_parameter_verifier( + var: &Ident, + var_str: &str, + info: &mut HashMap, + out: &mut TokenStream, +) -> Result<(), Error> { + match info.remove("minimum") { + None => (), + Some(Expression::Expr(expr)) => out.extend(quote! { + let cmp = #expr; + if #var < cmp { + bail!("parameter '{}' is out of range (must be >= {})", #var_str, cmp); + } + }), + Some(_) => bail!("invalid value for 'minimum'"), + } + + match info.remove("maximum") { + None => (), + Some(Expression::Expr(expr)) => out.extend(quote! { + let cmp = #expr; + if #var > cmp { + bail!("parameter '{}' is out of range (must be <= {})", #var_str, cmp); + } + }), + Some(_) => bail!("invalid value for 'maximum'"), + } + + Ok(()) +} + fn handle_struct( definition: HashMap, item: &syn::ItemStruct, diff --git a/proxmox-api-macro/tests/verification.rs b/proxmox-api-macro/tests/verification.rs new file mode 100644 index 00000000..a7648e72 --- /dev/null +++ b/proxmox-api-macro/tests/verification.rs @@ -0,0 +1,137 @@ +#![feature(async_await)] + +use bytes::Bytes; +use failure::{bail, Error}; +use serde_json::{json, Value}; + +use proxmox::api::{api, Router}; + +#[api({ + body: Bytes, + description: "A test function returning a fixed text", + parameters: { + number: { + description: "A number", + minimum: 3, + maximum: 10, + }, + reference: { + description: "A reference number", + minimum: 3, + maximum: 10, + }, + }, +})] +async fn less_than(number: usize, reference: usize) -> Result { + Ok(number < reference) +} + +proxmox_api_macro::router! { + static TEST_ROUTER: Router = { + GET: less_than, + }; +} + +fn check_parameter( + router: &Router, + path: &str, + parameters: Value, + expect: Result<&'static str, &'static str>, +) { + let (router, _) = router + .lookup(path) + .expect("expected method to exist on test router"); + let method = router + .get + .as_ref() + .expect("expected GET method on router at path"); + let fut = method.handler()(parameters); + match (futures::executor::block_on(fut), expect) { + (Ok(resp), Ok(exp)) => { + assert_eq!(resp.status(), 200, "test response should have status 200"); + let body = resp.into_body(); + let body = std::str::from_utf8(&body).expect("expected test body to be valid utf8"); + assert_eq!(body, exp, "expected successful output"); + } + (Err(resp), Err(exp)) => { + assert_eq!(resp.to_string(), exp.to_string(), "expected specific error"); + } + (Ok(resp), Err(exp)) => { + let body = resp.into_body(); + let body = std::str::from_utf8(&body).expect("expected test body to be valid utf8"); + panic!( + "expected function to fail with `{}`, but it succeeded with `{}`", + exp, body + ); + } + (Err(resp), Ok(exp)) => { + panic!( + "expected function to succeed with `{}`, but it failed with `{}`", + exp, resp + ); + } + } +} + +#[test] +fn router() { + // Expected successes: + check_parameter( + &TEST_ROUTER, + "/", + json!({ + "number": 3, + "reference": 5, + }), + Ok(r#"{"data":true}"#), + ); + + check_parameter( + &TEST_ROUTER, + "/", + json!({ + "number": 5, + "reference": 5, + }), + Ok(r#"{"data":false}"#), + ); + + // Expected failures: + check_parameter( + &TEST_ROUTER, + "/", + json!({ + "number": 1, + "reference": 5, + }), + Err("parameter 'number' is out of range (must be >= 3)"), + ); + + check_parameter( + &TEST_ROUTER, + "/", + json!({ + "number": 3, + "reference": 2, + }), + Err("parameter 'reference' is out of range (must be >= 3)"), + ); + + check_parameter( + &TEST_ROUTER, + "/", + json!({ + "number": 3, + "reference": 20, + }), + Err("parameter 'reference' is out of range (must be <= 10)"), + ); + + //// And can I... + let res = futures::executor::block_on(less_than(1, 5)).map_err(|x| x.to_string()); + assert_eq!( + res, + Err("parameter 'number' is out of range (must be >= 3)".to_string()), + "expected FOO from direct get_loopback('FOO') call" + ); +}