From 5bda38830cb97d3aa7bb9c994a5d28daeb61a8b7 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Thu, 6 Jun 2019 14:45:05 +0200 Subject: [PATCH] Add more documentation Signed-off-by: Wolfgang Bumiller --- proxmox-api/src/api_output.rs | 12 +++++ proxmox-api/src/api_type.rs | 15 ++++++ proxmox-api/src/lib.rs | 99 +++++++++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 3 deletions(-) diff --git a/proxmox-api/src/api_output.rs b/proxmox-api/src/api_output.rs index e33dddd9..125c402b 100644 --- a/proxmox-api/src/api_output.rs +++ b/proxmox-api/src/api_output.rs @@ -4,11 +4,19 @@ use serde_json::json; use super::{ApiOutput, ApiType}; +/// Helper trait to convert a variable into an API output. +/// +/// If an API method returns a `String`, we want it to be jsonified into `{"data": result}` and +/// wrapped in a `http::Response` with a status code of `200`, but if an API method returns a +/// `http::Response`, we don't want that, our wrappers produced by the `#[api]` macro simply call +/// `output.into_api_output()`, and the trait implementation decides how to proceed. pub trait IntoApiOutput { fn into_api_output(self) -> ApiOutput; } impl IntoApiOutput<()> for T { + /// By default, any serializable type is serialized into a `{"data": output}` json structure, + /// and returned as http status 200. fn into_api_output(self) -> ApiOutput { let output = serde_json::to_value(self)?; let res = json!({ "data": output }); @@ -21,12 +29,16 @@ impl IntoApiOutput<()> for T { } } +/// Methods returning `ApiOutput` (which is a `Result, Error>`) don't need +/// anything to happen to the value anymore, return the result as is: impl IntoApiOutput for ApiOutput { fn into_api_output(self) -> ApiOutput { self } } +/// Methods returning a `http::Response` (without the `Result<_, Error>` around it) need to be +/// wrapped in a `Result`, as we do apply a `?` operator on our methods. impl IntoApiOutput for http::Response { fn into_api_output(self) -> ApiOutput { Ok(self) diff --git a/proxmox-api/src/api_type.rs b/proxmox-api/src/api_type.rs index 1195b9c7..643b5168 100644 --- a/proxmox-api/src/api_type.rs +++ b/proxmox-api/src/api_type.rs @@ -2,6 +2,9 @@ use serde_json::Value; +/// Method entries in a `Router` are actually just `&dyn ApiMethodInfo` trait objects. +/// This contains all the info required to call, document, or command-line-complete parameters for +/// a method. pub trait ApiMethodInfo { fn description(&self) -> &'static str; fn parameters(&self) -> &'static [Parameter]; @@ -11,20 +14,32 @@ pub trait ApiMethodInfo { fn handler(&self) -> fn(Value) -> super::ApiFuture; } +/// Shortcut to not having to type it out. This function signature is just a dummy and not yet +/// stabalized! pub type CompleteFn = fn(&str) -> Vec; +/// Provides information about a method's parameter. Every parameter has a name and must be +/// documented with a description, type information, and optional constraints. pub struct Parameter { pub name: &'static str, pub description: &'static str, pub type_info: &'static TypeInfo, } +/// Bare type info. Types themselves should also have a description, even if a method's parameter +/// usually overrides it. Ideally we can hyperlink the parameter to the type information in the +/// generated documentation. pub struct TypeInfo { pub name: &'static str, pub description: &'static str, pub complete_fn: Option, } +/// Until we can slap `#[api]` onto all the functions we cann start translating our existing +/// `ApiMethod` structs to this new layout. +/// Otherwise this is mostly there so we can run the tests in the tests subdirectory without +/// depending on the api-macro crate. Tests using the macros belong into the api-macro crate itself +/// after all! pub struct ApiMethod { pub description: &'static str, pub parameters: &'static [Parameter], diff --git a/proxmox-api/src/lib.rs b/proxmox-api/src/lib.rs index e51c65d0..938343b1 100644 --- a/proxmox-api/src/lib.rs +++ b/proxmox-api/src/lib.rs @@ -1,3 +1,11 @@ +//! Proxmox API module. This provides utilities for HTTP and command line APIs. +//! +//! The main component here is the [`Router`] which is filled with entries pointing to +//! [`ApiMethodInfos`](crate::ApiMethodInfo). +//! +//! 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. + use std::cell::Cell; use std::collections::HashMap; use std::future::Future; @@ -15,33 +23,63 @@ pub use api_output::*; mod api_type; pub use api_type::*; +/// Return type of an API method. pub type ApiOutput = Result, Error>; -pub type ApiFuture = Pin>>; -pub type ApiFn = Box ApiFuture>; +/// Future type of an API method. In order to support `async fn` this is a pinned box. +pub type ApiFuture = Pin>>; + +/// This enum specifies what to do when a subdirectory is requested from the current router. +/// +/// For plain subdirectories a `Directories` entry is used. +/// +/// When subdirectories are supposed to be passed as a `String` parameter to methods beneath the +/// current directory, a `Parameter` entry is used. Note that the parameter name is fixed at this +/// point, so all method calls beneath will receive a parameter ot that particular name. pub enum SubRoute { + /// This is used for plain subdirectories. Directories(HashMap<&'static str, Router>), + + /// Match subdirectories as the given parameter name to the underlying router. Parameter(&'static str, Box), } +/// A router is a nested structure. On the one hand it contains HTTP method entries (`GET`, `PUT`, +/// ...), and on the other hand it contains sub directories. In some cases we want to match those +/// sub directories as parameters, so the nesting uses a `SubRoute` `enum` representing which of +/// the two is the case. #[derive(Default)] pub struct Router { + /// The `GET` http method. pub get: Option<&'static dyn ApiMethodInfo>, + + /// The `PUT` http method. pub put: Option<&'static dyn ApiMethodInfo>, + + /// The `POST` http method. pub post: Option<&'static dyn ApiMethodInfo>, + + /// The `DELETE` http method. pub delete: Option<&'static dyn ApiMethodInfo>, + + /// Specifies the behavior of sub directories. See [`SubRoute`]. pub subroute: Option, } impl Router { + /// Create a new empty router. pub fn new() -> Self { Self::default() } + /// Lookup a path in the router. Note that this returns a tuple: the router we ended up on + /// (providing methods and subdirectories available for the given path), and optionally a json + /// value containing all the matched parameters ([`SubRoute::Parameter`] subdirectories). pub fn lookup>(&self, path: T) -> Option<(&Self, Option)> { self.lookup_do(path.as_ref()) } + // The actual implementation taking the parameter as &str fn lookup_do(&self, path: &str) -> Option<(&Self, Option)> { let mut matched_params = None; @@ -69,6 +107,7 @@ impl Router { Some((this, matched_params.map(Value::Object))) } + /// Builder method to provide a `GET` method info. pub fn get(mut self, method: &'static I) -> Self where I: ApiMethodInfo, @@ -77,6 +116,7 @@ impl Router { self } + /// Builder method to provide a `PUT` method info. pub fn put(mut self, method: &'static I) -> Self where I: ApiMethodInfo, @@ -85,6 +125,7 @@ impl Router { self } + /// Builder method to provide a `POST` method info. pub fn post(mut self, method: &'static I) -> Self where I: ApiMethodInfo, @@ -93,6 +134,7 @@ impl Router { self } + /// Builder method to provide a `DELETE` method info. pub fn delete(mut self, method: &'static I) -> Self where I: ApiMethodInfo, @@ -101,7 +143,10 @@ impl Router { self } - // To be used statically, therefore we panic otherwise! + /// Builder method to make this router match the next subdirectory into a parameter. + /// + /// This is supposed to be used statically (via `lazy_static!), therefore we panic if we + /// already have a subdir entry! pub fn parameter_subdir(mut self, parameter_name: &'static str, router: Router) -> Self { if self.subroute.is_some() { panic!("match_parameter can only be used once and without sub directories"); @@ -110,6 +155,10 @@ impl Router { self } + /// Builder method to add a regular directory entro to this router. + /// + /// This is supposed to be used statically (via `lazy_static!), therefore we panic if we + /// already have a subdir entry! pub fn subdir(mut self, dir_name: &'static str, router: Router) -> Self { let previous = match self.subroute { Some(SubRoute::Directories(ref mut map)) => map.insert(dir_name, router), @@ -139,15 +188,53 @@ impl Router { /// 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; + + /// Additionally, ApiTypes must provide a way to verify their constraints! fn verify(&self) -> Result<(), Error>; + /// This is a workaround for when we cannot name the type but have an object available we can + /// call a method on. (We cannot call associated methods on objects without being able to write + /// out the type, and rust has some restrictions as to what types are available.) + // eg. nested generics: + // fn foo() { + // fn bar(x: &T) { + // cannot use T::method() here, but can use x.method() + // (compile error "can't use generic parameter of outer function", + // and yes, that's a stupid restriction as it is still completely static...) + // } + // } fn get_type_info(&self) -> &'static TypeInfo { Self::type_info() } } +/// Option types are supposed to wrap their underlying types with an `optional:` text in their +/// description. +// BUT it requires some anti-static magic. And while this looks like the result of lazy_static!, +// it's not exactly the same, lazy_static! here does not actually work as it'll curiously produce +// the same error as we pointed out above in the `get_type_info` method (as it does a lot more +// extra stuff we don't need)... impl ApiType for Option { fn verify(&self) -> Result<(), Error> { if let Some(inner) = self { @@ -183,6 +270,8 @@ impl ApiType for Option { } } +/// Any `Result` of course gets the same info as `T`, since this only means that it can +/// fail... impl ApiType for Result { fn verify(&self) -> Result<(), Error> { if let Ok(inner) = self { @@ -196,6 +285,10 @@ impl ApiType for Result { } } +/// This is not supposed to be used, but can be if needed. This will provide an empty `ApiType` +/// declaration with no description and no verifier. +/// +/// This rarely makes sense, but sometimes a `string` is just a `string`. #[macro_export] macro_rules! unconstrained_api_type { ($type:ty $(, $more:ty)*) => {