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

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

[Wolfgang Bumiller]

> On 10/20/2022 4:11 PM Stefan Hanreich <s.hanreich@proxmox.com> wrote:
> 
>  
> On 10/20/22 16:06, Wolfgang Bumiller wrote:
> > On Thu, Oct 20, 2022 at 03:50:59PM +0200, Stefan Hanreich wrote:
> >>
> >>
> >> On 10/20/22 15:44, Wolfgang Bumiller wrote:
> >>> 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.
> >>>
> >>
> >> I will keep that in mind for future patches, thanks for the info!
> >>
> >> The problem is when assigning via args[key] = value on a Map, it actually
> >> panics if the key doesn't exist in the map [1], so I'm afraid this isn't a
> >> possibility unless I'm missing something.
> > 
> > Can be changed to use `.insert()` ;-)
> 
> yes, but I thought that would just be needlessly verbose since all 
> `args[key] = value` would be replaced with some kind of
> 
> ```
> args.insert(
>    key.to_string(),
>    value.into() / json!(value)
> );
> ```
> 
> I can definitely change it to use `insert()` in my subsequent patch if 
> you want me to - just say the word ;)

It's fine.
If the parameter list gets too large I'd probably start thinking about
putting them in a struct type shared between client & server code and
just serialize that once. (That would also get rid of the temporary
`limit` Object.)

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

[Stefan Hanreich]

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"))?;
+
+    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

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

[Wolfgang Bumiller]

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()`)

> +
> +    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

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

[Stefan Hanreich]

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?

>> +
>> +    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

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

[Wolfgang Bumiller]

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

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

[Stefan Hanreich]

On 10/20/22 15:44, Wolfgang Bumiller wrote:
> 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.
>

I will keep that in mind for future patches, thanks for the info!

The problem is when assigning via args[key] = value on a Map, it 
actually panics if the key doesn't exist in the map [1], so I'm afraid 
this isn't a possibility unless I'm missing something.

I was already a bit too overeager and submitted v2, this time without 
the assignment and just using the `args.as_object_mut()`. I think it 
should be fine, I'll just add the variable assignment in the subsequent 
patch series when it is actually needed and not in this patch.

[1] 
https://docs.rs/serde_json/latest/serde_json/struct.Map.html#impl-IndexMut%3C%26%27a%20Q%3E-for-Map%3CString%2C%20Value%3E

>> 
>>>> +
>>>> +    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

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

[Wolfgang Bumiller]

On Thu, Oct 20, 2022 at 03:50:59PM +0200, Stefan Hanreich wrote:
> 
> 
> On 10/20/22 15:44, Wolfgang Bumiller wrote:
> > 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.
> > 
> 
> I will keep that in mind for future patches, thanks for the info!
> 
> The problem is when assigning via args[key] = value on a Map, it actually
> panics if the key doesn't exist in the map [1], so I'm afraid this isn't a
> possibility unless I'm missing something.

Can be changed to use `.insert()` ;-)

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

[Stefan Hanreich]

On 10/20/22 16:06, Wolfgang Bumiller wrote:
> On Thu, Oct 20, 2022 at 03:50:59PM +0200, Stefan Hanreich wrote:
>>
>>
>> On 10/20/22 15:44, Wolfgang Bumiller wrote:
>>> 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.
>>>
>>
>> I will keep that in mind for future patches, thanks for the info!
>>
>> The problem is when assigning via args[key] = value on a Map, it actually
>> panics if the key doesn't exist in the map [1], so I'm afraid this isn't a
>> possibility unless I'm missing something.
> 
> Can be changed to use `.insert()` ;-)

yes, but I thought that would just be needlessly verbose since all 
`args[key] = value` would be replaced with some kind of

```
args.insert(
   key.to_string(),
   value.into() / json!(value)
);
```

I can definitely change it to use `insert()` in my subsequent patch if 
you want me to - just say the word ;)