Re: [pbs-devel] [PATCH proxmox-backup] fix #4301: correctly pass rate limit parameters to API

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Oct 20, 2022 at 02:59:46PM +0200, Stefan Hanreich wrote:
> 
> 
> On 10/20/22 14:48, Wolfgang Bumiller wrote:
> > On Thu, Oct 20, 2022 at 01:37:31PM +0200, Stefan Hanreich wrote:
> > > With the old code the rate limit parameters got passed in their own
> > > dictionary under the limit key, but the API expects the rate-limit
> > > settings as top-level keys. This commit correctly sets the rate-limit
> > > parameters so the API actually uses them.
> > > 
> > > Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> > > ---
> > >   src/bin/proxmox-backup-manager.rs | 14 ++++++++++++--
> > >   1 file changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
> > > index 58e7e33a..cdd1037d 100644
> > > --- a/src/bin/proxmox-backup-manager.rs
> > > +++ b/src/bin/proxmox-backup-manager.rs
> > > @@ -2,7 +2,7 @@ use std::collections::HashMap;
> > >   use std::io::{self, Write};
> > >   use std::str::FromStr;
> > > -use anyhow::Error;
> > > +use anyhow::{Error, format_err};
> > >   use serde_json::{json, Value};
> > >   use proxmox_router::{cli::*, RpcEnvironment};
> > > @@ -297,7 +297,6 @@ async fn pull_datastore(
> > >           "store": store,
> > >           "remote": remote,
> > >           "remote-store": remote_store,
> > > -        "limit": limit,
> > >       });
> > >       if remote_ns.is_some() {
> > > @@ -320,6 +319,17 @@ async fn pull_datastore(
> > >           args["remove-vanished"] = Value::from(remove_vanished);
> > >       }
> > > +    let args_map = args
> > > +        .as_object_mut()
> > > +        .ok_or_else(|| format_err!("args is not an Object"))?;
> > 
> > ^ We create the `args` map only a few lines further up, so it would be
> > fine to just `.unwrap()` here. And it would be nicer to keep the access
> > short (iow move the `.as_object_mut()` down to where it's used for `.append()`)
> > 
> 
> Can replace it with unwrap(), shouldn't be a problem.
> 
> The reason why I stored it in a variable was that with a subsequent patch I
> will also append another map to the args, but I could then just call
> .as_object_mut() twice, what do you think?

I suppose that's fine. If you already know such things while sending a
patch without the followups being part of the same series, you can note
such things after the `---` in the patch mail, that way it won't
needlessly end up in the commit message, but still be visible to the
reviewer.

Come to think of it, there are a lot of `args[key] = value;` assignments
which could probably benefit from already having the `&mut Map` instead
of the `Value` enum.

Maybe should change it like this

    -    let mut args = json!(...)
    +    let mut args_value = json(!...);
    +    let args = args_value.as_object_mut().unwrap();

right where it is created, this way the unwrap will stay even closer to
the creation, the remaining assignments already see the object type, and
you can call append, too.
Finally, the `Some(args)` would need to be changed to `Some(args_value)`
in the `post()` call.

> 
> > > +
> > > +    let mut limit_json = json!(limit);
> > > +    let limit_map = limit_json
> > > +        .as_object_mut()
> > > +        .ok_or_else(|| format_err!("limit is not an Object"))?;
> > > +
> > > +    args_map.append(limit_map);
> > > +
> > >       let result = client.post("api2/json/pull", Some(args)).await?;
> > >       view_task_result(&client, result, &output_format).await?;
> > > -- 
> > > 2.30.2