[pbs-devel] [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[pbs-devel] [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dominik Csapak]

since the PUT api call is using the 'Updater', the 'id' parameter is
already encoded in there, tripping up the api verify tests with
'Duplicate keys found in AllOf schema: id'

"fixing" it by removing the explicit id from the api call and
taking it from the Updater (and failing if it does not exists there;
even though that should never happen)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
i am *really* not sure if this is the correct way @Wolfgang, is
there another wayt to selectively use the struct members for the
Updater?

 src/api2/config/tape_backup_job.rs | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index a9edc00f..bc05704c 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, Error};
+use anyhow::{bail, format_err, Error};
 use serde_json::Value;
 use ::serde::{Deserialize, Serialize};
 
@@ -123,9 +123,6 @@ pub enum DeletableProperty {
     protected: true,
     input: {
         properties: {
-            id: {
-                schema: JOB_ID_SCHEMA,
-            },
             update: {
                 flatten: true,
                 type: TapeBackupJobConfigUpdater,
@@ -147,13 +144,14 @@ pub enum DeletableProperty {
 )]
 /// Update the tape backup job
 pub fn update_tape_backup_job(
-    id: String,
-    update: TapeBackupJobConfigUpdater,
+    mut update: TapeBackupJobConfigUpdater,
     delete: Option<Vec<String>>,
     digest: Option<String>,
 ) -> Result<(), Error> {
     let _lock = open_file_locked(TAPE_JOB_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
+    let id = update.id.take().ok_or_else(|| format_err!("no id given"))?;
+
     let (mut config, expected_digest) = config::tape_job::config()?;
 
     let mut job: TapeBackupJobConfig = config.lookup("backup", &id)?;
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

applied

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Thomas Lamprecht]

On 23.02.21 17:26, Dietmar Maurer wrote:
> applied

did you read that part:

On 23.02.21 15:54, Dominik Csapak wrote:
> i am *really* not sure if this is the correct way @Wolfgang, is
> there another wayt to selectively use the struct members for the
> Updater?


This makes the ID optional in the schema, which is weird for an API call
with {id} in its url (which means that without ID this can never be reached).

So not really an ideal fix, IMO, as the API schema gets basically wrong and
possible confusing when suggesting this non-optional parameter is optional...

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

Will fix this tomorrow - we need to set the #[fixed] attribute for id


> On 02/23/2021 6:00 PM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> On 23.02.21 17:26, Dietmar Maurer wrote:
> > applied
> 
> did you read that part:
> 
> On 23.02.21 15:54, Dominik Csapak wrote:
> > i am *really* not sure if this is the correct way @Wolfgang, is
> > there another wayt to selectively use the struct members for the
> > Updater?
> 
> 
> This makes the ID optional in the schema, which is weird for an API call
> with {id} in its url (which means that without ID this can never be reached).
> 
> So not really an ideal fix, IMO, as the API schema gets basically wrong and
> possible confusing when suggesting this non-optional parameter is optional...

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

> On 02/23/2021 8:27 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> Will fix this tomorrow - we need to set the #[fixed] attribute for id

Just saw that we already set the  #[updater(fixed)] attribute.

The docs say:

> Additionally the #[updater(fixed)] option is available to make it illegal 
> for an updater to modify a field (generating an error if it is set), 
> while still allowing it to be used to create a new object via the build_from() method.

So the Updater includes all "fixed" fields (which leads to the duplicate id problem).

I would prefer an Updater which simply omits fixed fields.

Can we change that?

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

Seems there is another problem with the Updater:

I can delete properties if I use normal rust naming, e.g.

   job.update_from(update, "eject_media")?;

But it does not work with kebab-case:

   job.update_from(update, "eject-media")?; // this fails silently

Please can we:

 - support kebab-case
 - raise error with unknown property names


> On 02/24/2021 7:13 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > On 02/23/2021 8:27 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > 
> >  
> > Will fix this tomorrow - we need to set the #[fixed] attribute for id
> 
> Just saw that we already set the  #[updater(fixed)] attribute.
> 
> The docs say:
> 
> > Additionally the #[updater(fixed)] option is available to make it illegal 
> > for an updater to modify a field (generating an error if it is set), 
> > while still allowing it to be used to create a new object via the build_from() method.
> 
> So the Updater includes all "fixed" fields (which leads to the duplicate id problem).
> 
> I would prefer an Updater which simply omits fixed fields.
> 
> Can we change that?

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Wolfgang Bumiller]

On Wed, Feb 24, 2021 at 08:28:54AM +0100, Dietmar Maurer wrote:
> Seems there is another problem with the Updater:
> 
> I can delete properties if I use normal rust naming, e.g.
> 
>    job.update_from(update, "eject_media")?;
> 
> But it does not work with kebab-case:
> 
>    job.update_from(update, "eject-media")?; // this fails silently
> 
> Please can we:
> 
>  - support kebab-case

^ that's a quick one, fix for this is in git, will bump & upload in a
bit

>  - raise error with unknown property names

^ this is a bit more involved due to flattening

One option would be to generate an enum schema for deletable fields,
though this requires a change to how we represent enums, so I can nest
the flattened objects in there.

Another would be to just generate a "string" schema with a verify
function which uses the existing property iterators to look for invalid
names. (This could probably even generate a lazy_static HashSet for
faster checks).

On Tue, Feb 23, 2021 at 06:00:33PM +0100, Thomas Lamprecht wrote:
> On 23.02.21 17:26, Dietmar Maurer wrote:
> > applied
> 
> did you read that part:
> 
> On 23.02.21 15:54, Dominik Csapak wrote:
> > i am *really* not sure if this is the correct way @Wolfgang, is
> > there another wayt to selectively use the struct members for the
> > Updater?
> 
> 
> This makes the ID optional in the schema, which is weird for an API call
> with {id} in its url (which means that without ID this can never be reached).

Now it's getting tough. Somehow I did not think about that :-)
But sure, we often need a way to identify entries, so maybe add a
`#[updater(id_property)]` attribute. (Or name it `non_optional` but that
may be too unspecific of a name.) Or are there any other reasons why a
field would behave this way?

An alternative of course would be to split the object up and flatten the
data part:

    #[api]
    struct MyType {
        id: String,
        #[serde(flatten)]
        contents: MyTypeContents,
    }

    #[api]
    struct MyTypeContents {
        <rest>
    }

But that probably gets annoying very quickly...?

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

> > Please can we:
> > 
> >  - support kebab-case
> 
> ^ that's a quick one, fix for this is in git, will bump & upload in a
> bit

Ok, thanks!

> 
> >  - raise error with unknown property names
> 
> ^ this is a bit more involved due to flattening
> 
> One option would be to generate an enum schema for deletable fields,
> though this requires a change to how we represent enums, so I can nest
> the flattened objects in there.
> 
> Another would be to just generate a "string" schema with a verify
> function which uses the existing property iterators to look for invalid
> names. (This could probably even generate a lazy_static HashSet for
> faster checks).

Sound too complex for now ...
I guess this is not really important - just thought it would be nice to have.
 
> On Tue, Feb 23, 2021 at 06:00:33PM +0100, Thomas Lamprecht wrote:
> > On 23.02.21 17:26, Dietmar Maurer wrote:
> > > applied
> > 
> > did you read that part:
> > 
> > On 23.02.21 15:54, Dominik Csapak wrote:
> > > i am *really* not sure if this is the correct way @Wolfgang, is
> > > there another wayt to selectively use the struct members for the
> > > Updater?
> > 
> > 
> > This makes the ID optional in the schema, which is weird for an API call
> > with {id} in its url (which means that without ID this can never be reached).
> 
> Now it's getting tough. Somehow I did not think about that :-)

Cant we simple omit fixed properties from the Updater?

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Wolfgang Bumiller]

> On 02/24/2021 9:42 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > > Please can we:
> > > 
> > >  - support kebab-case
> > 
> > ^ that's a quick one, fix for this is in git, will bump & upload in a
> > bit
> 
> Ok, thanks!
> 
> > 
> > >  - raise error with unknown property names
> > 
> > ^ this is a bit more involved due to flattening
> > 
> > One option would be to generate an enum schema for deletable fields,
> > though this requires a change to how we represent enums, so I can nest
> > the flattened objects in there.
> > 
> > Another would be to just generate a "string" schema with a verify
> > function which uses the existing property iterators to look for invalid
> > names. (This could probably even generate a lazy_static HashSet for
> > faster checks).
> 
> Sound too complex for now ...
> I guess this is not really important - just thought it would be nice to have.
>  
> > On Tue, Feb 23, 2021 at 06:00:33PM +0100, Thomas Lamprecht wrote:
> > > On 23.02.21 17:26, Dietmar Maurer wrote:
> > > > applied
> > > 
> > > did you read that part:
> > > 
> > > On 23.02.21 15:54, Dominik Csapak wrote:
> > > > i am *really* not sure if this is the correct way @Wolfgang, is
> > > > there another wayt to selectively use the struct members for the
> > > > Updater?
> > > 
> > > 
> > > This makes the ID optional in the schema, which is weird for an API call
> > > with {id} in its url (which means that without ID this can never be reached).
> > 
> > Now it's getting tough. Somehow I did not think about that :-)
> 
> Cant we simple omit fixed properties from the Updater?

I could try. I'd have to split out the Builder functionality of the Updatable
trait, which is currently what allows `Option<T: Updatable>` to be generically
implemented without specialization, but I could in theory generate implementations
for Option types on the fly as well...
(Currently there's a generic `impl Updatable for Option<T>` going like this:
if the option is Some() already, then update_from(), otherwise try_build_from().)

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

> > Cant we simple omit fixed properties from the Updater?
> 
> I could try. I'd have to split out the Builder functionality of the Updatable
> trait, which is currently what allows `Option<T: Updatable>` to be generically
> implemented without specialization, but I could in theory generate implementations
> for Option types on the fly as well...
> (Currently there's a generic `impl Updatable for Option<T>` going like this:
> if the option is Some() already, then update_from(), otherwise try_build_from().)

I simply do not know how the current Updater is supposed to be used. Please can you provide
and example? 

The current code in src/api2/config/tape_backup_job.rs looks clumsy, and I am 
unsure if it works at all with multiple 'fixed' properties.

Re: [pbs-devel] applied: [PATCH proxmox-backup] api2/config/tape_backup_job: fix duplicate id parameter

[Dietmar Maurer]

> I simply do not know how the current Updater is supposed to be used. Please can you provide
> and example? 
> 
> The current code in src/api2/config/tape_backup_job.rs looks clumsy, and I am 
> unsure if it works at all with multiple 'fixed' properties.

Worse, the generated API doc is also wrong.