Add more documentation

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

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<T> {
     fn into_api_output(self) -> ApiOutput;
 }
 
 impl<T: ApiType + serde::Serialize> 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<T: ApiType + serde::Serialize> IntoApiOutput<()> for T {
     }
 }
 
+/// Methods returning `ApiOutput` (which is a `Result<http::Result<Bytes>, Error>`) don't need
+/// anything to happen to the value anymore, return the result as is:
 impl IntoApiOutput<ApiOutput> 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<ApiOutput> for http::Response<bytes::Bytes> {
     fn into_api_output(self) -> ApiOutput {
         Ok(self)

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<String>;
 
+/// 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<CompleteFn>,
 }
 
+/// 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],

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<Response<Bytes>, Error>;
-pub type ApiFuture = Pin<Box<dyn Future<Output = ApiOutput>>>;
-pub type ApiFn = Box<dyn Fn(Value) -> ApiFuture>;
 
+/// Future type of an API method. In order to support `async fn` this is a pinned box.
+pub type ApiFuture = Pin<Box<dyn Future<Output = ApiOutput>>>;
+
+/// 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<Router>),
 }
 
+/// 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<SubRoute>,
 }
 
 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<T: AsRef<str>>(&self, path: T) -> Option<(&Self, Option<Value>)> {
         self.lookup_do(path.as_ref())
     }
 
+    // The actual implementation taking the parameter as &str
     fn lookup_do(&self, path: &str) -> Option<(&Self, Option<Value>)> {
         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<I>(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<I>(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<I>(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<I>(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<T>() {
+    //         fn bar<U>(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<T: ApiType> ApiType for Option<T> {
     fn verify(&self) -> Result<(), Error> {
         if let Some(inner) = self {
@@ -183,6 +270,8 @@ impl<T: ApiType> ApiType for Option<T> {
     }
 }
 
+/// Any `Result<T, Error>` of course gets the same info as `T`, since this only means that it can
+/// fail...
 impl<T: ApiType> ApiType for Result<T, Error> {
     fn verify(&self) -> Result<(), Error> {
         if let Ok(inner) = self {
@@ -196,6 +285,10 @@ impl<T: ApiType> ApiType for Result<T, Error> {
     }
 }
 
+/// 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)*) => {