Re: [pbs-devel] [PATCH proxmox-backup v2 3/7] api2: add missing token list match_all property

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

s/api2/api/ please.

On 13.09.21 16:18, Dominik Csapak wrote:
> to have the proper link between the token list and the sub routes
> in the api, include the 'tokenname' property in the token listing
> 

this sounds like that the link did not work or the like, but that is not
exactly the case, isn't it? Can we please state what provoked this patch
here.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/access/user.rs | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 75071cf1..97c336ab 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -655,6 +655,21 @@ pub fn delete_token(
>      Ok(())
>  }
>  
> +#[api(
> +    properties: {
> +        tokenname: { type: Tokenname },

we normally try to use kebab case, or is it not possible to use "token-name" here?

> +        token: { type: ApiToken },
> +    }
> +)]
> +#[derive(Serialize, Deserialize)]
> +/// A Token Entry of a user

that doc-comment is "meh" at best, casing and info is lacking and not sure if
"of an user" is relevant here. I'd mention that the token name is already
encoded and explicitly split out here for API's sake.

> +pub struct TokenInfo {

I'd find some merit in renaming this to clarify that it is rather a API specific
type that does not provides more info but just more explicit, that the original
type is named "ApiToken" doesn't make this easier though, and atm. I have no good
proposal, sorry, so just putting that opinion out there.

> +    /// The Token name
> +    pub tokenname: Tokenname,
> +    #[serde(flatten)]
> +    pub token: ApiToken,
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -666,7 +681,7 @@ pub fn delete_token(
>      returns: {
>          description: "List user's API tokens (with config digest).",
>          type: Array,
> -        items: { type: ApiToken },
> +        items: { type: TokenInfo },
>      },
>      access: {
>          permission: &Permission::Or(&[
> @@ -680,7 +695,7 @@ pub fn list_tokens(
>      userid: Userid,
>      _info: &ApiMethod,
>      mut rpcenv: &mut dyn RpcEnvironment,
> -) -> Result<Vec<ApiToken>, Error> {
> +) -> Result<Vec<TokenInfo>, Error> {
>  
>      let (config, digest) = pbs_config::user::config()?;
>  
> @@ -688,15 +703,21 @@ pub fn list_tokens(
>  
>      rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
>  
> -    let filter_by_owner = |token: &ApiToken| {
> -        if token.tokenid.is_token() {
> -           token.tokenid.user() == &userid
> +    let filter_by_owner = |token: ApiToken| {
> +        if token.tokenid.is_token() && token.tokenid.user() == &userid {
> +            let tokenname = token.tokenid.tokenname().unwrap().to_owned();

I'd like to try adding comments about why unwrap is safe. A panic possibility is
always a bit of a bummer, so showing that one thought about that and recording
that in a comment would be nice.

> +            Some(TokenInfo {
> +                tokenname,
> +                token,
> +            })
>          } else {
> -            false
> +            None
>          }
>      };
>  
> -    Ok(list.into_iter().filter(filter_by_owner).collect())
> +    let res = list.into_iter().filter_map(filter_by_owner).collect();
> +
> +    Ok(res)

nit: why adding a (useless?) intermediated `res` variable in above change?

>  }
>  
>  const TOKEN_ITEM_ROUTER: Router = Router::new()
>