Re: [pmg-devel] [PATCH pmg-api 05/12] add objectgroup attributes and/invert

[Stoiko Ivanov <s.ivanov@proxmox.com>]

afaict deletion of objectgroup attributes when deleting the object group is
missing

On Fri,  9 Feb 2024 13:54:29 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> add a new table Objectgroup_Attributes where we can save additional
> attributes for objectgroups (like the Attribut tables for objects).
> 
> Adds two new attributes for the groups:
> * and
> * invert
> 
> These will modify the match behaviour for object groups
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/ObjectGroupHelpers.pm |  43 ++++++++-
>  src/PMG/DBTools.pm                 |  15 +++
>  src/PMG/RuleDB.pm                  | 145 ++++++++++++++++++++++-------
>  3 files changed, 162 insertions(+), 41 deletions(-)
> 
> diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
> index 48078fb..a08a6a3 100644
> --- a/src/PMG/API2/ObjectGroupHelpers.pm
> +++ b/src/PMG/API2/ObjectGroupHelpers.pm
> @@ -46,13 +46,29 @@ sub format_object_group {
>  
>      my $res = [];
>      foreach my $og (@$ogroups) {
> -	push @$res, {
> -	    id => $og->{id}, name => $og->{name}, info => $og->{info}
> -	};
> +	my $group = { id => $og->{id}, name => $og->{name}, info => $og->{info} };
> +	$group->{and} = $og->{and} if defined($og->{and});
> +	$group->{invert} = $og->{invert} if defined($og->{invert});
> +	push @$res, $group;
>      }
>      return $res;
>  }
>  
> +my $group_attributes = {
> +    and => {
> +	description => "If set to 1, objects in this group are 'and' combined.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    invert => {
> +	description => "If set to 1, the resulting match is inverted.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +};
> +
>  sub register_group_list_api {
>      my ($apiclass, $oclass) = @_;
>  
> @@ -86,6 +102,11 @@ sub register_group_list_api {
>  	    return format_object_group($ogroups);
>  	}});
>  
> +    my $additional_parameters = {};
> +    if ($oclass =~ /^(?:what|when|who)$/i) {
> +	$additional_parameters = { $group_attributes->%* };
> +    }
> +
>      $apiclass->register_method({
>  	name => "create_${oclass}_group",
>  	path => $oclass,
> @@ -108,6 +129,7 @@ sub register_group_list_api {
>  		    maxLength => 255,
>  		    optional => 1,
>  		},
> +		$additional_parameters->%*,
>  	    },
>  	},
>  	returns => { type => 'integer' },
> @@ -119,6 +141,10 @@ sub register_group_list_api {
>  	    my $og = PMG::RuleDB::Group->new(
>  		$param->{name}, $param->{info} // '', $oclass);
>  
> +	    for my $prop (qw(and invert)) {
> +		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
> +	    }
> +
>  	    return $rdb->save_group($og);
>  	}});
>  }
> @@ -199,6 +225,11 @@ sub register_object_group_config_api {
>  
>  	}});
>  
> +    my $additional_parameters = {};
> +    if ($oclass =~ /^(?:what|when|who)$/i) {
> +	$additional_parameters = { $group_attributes->%* };
> +    }
> +
>      $apiclass->register_method({
>  	name => 'set_config',
>  	path => $path,
> @@ -226,6 +257,7 @@ sub register_object_group_config_api {
>  		    maxLength => 255,
>  		    optional => 1,
>  		},
> +		$additional_parameters->%*,
>  	    },
>  	},
>  	returns => { type => "null" },
> @@ -243,8 +275,9 @@ sub register_object_group_config_api {
>  	    my $og = shift @$list ||
>  		die "$oclass group '$ogroup' not found\n";
>  
> -	    $og->{name} = $param->{name} if defined($param->{name});
> -	    $og->{info} = $param->{info} if defined($param->{info});
> +	    for my $prop (qw(name info and invert)) {
> +		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
> +	    }
>  
>  	    $rdb->save_group($og);
>  
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index 9e133bc..0d3d9c3 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
>  
>  __EOD
>  
> +my $object_group_attributes_cmd = <<__EOD;
> +    CREATE TABLE Objectgroup_Attributes (
> +      Objectgroup_ID INTEGER NOT NULL,
we could create a foreign key constraint on objectgroup.id here
> +      Name VARCHAR(20) NOT NULL,
> +      Value BYTEA NULL,
I know with the current db-schema we use bytea quite extensively - but for
now all the values are actually boolean (and even more so only the true
values are stored) - why not create the column accordingly?

> +      PRIMARY KEY (Objectgroup_ID, Name)
> +    );
> +
> +    CREATE INDEX Objectgroup_Attributes_Objectgroup_ID_Index ON Objectgroup_Attributes(Objectgroup_ID);
> +
> +__EOD
> +
>  sub cond_create_dbtable {
>      my ($dbh, $name, $ctablecmd) = @_;
>  
> @@ -439,6 +451,8 @@ sub create_ruledb {
>          $userprefs_ctablecmd;
>  
>          $virusinfo_stat_ctablecmd;
> +
> +        $object_group_attributes_cmd;
>  EOD
>      );
>  
> @@ -494,6 +508,7 @@ sub upgradedb {
>  	'CStatistic', $cstatistic_ctablecmd,
>  	'ClusterInfo', $clusterinfo_ctablecmd,
>  	'VirusInfo', $virusinfo_stat_ctablecmd,
> +	'Objectgroup_Attributes', $object_group_attributes_cmd,
>      };
>  
>      foreach my $table (keys %$tables) {
> diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
> index a6b0b79..df9e526 100644
> --- a/src/PMG/RuleDB.pm
> +++ b/src/PMG/RuleDB.pm
> @@ -160,6 +160,30 @@ sub load_groups_by_name {
>      };
>  }
>  
> +sub update_group_attributes {
> +    my ($self, $og) = @_;
> +
> +    my $attributes = [qw(and invert)];
> +
> +    for my $attribute ($attributes->@*) {
> +	# only save the values if they're set to 1
> +	if ($og->{$attribute}) {
> +	    $self->{dbh}->do(
> +		"INSERT INTO Objectgroup_Attributes (Objectgroup_ID, Name, Value) " .
> +		"VALUES (?, ?, ?) ".
> +		"ON CONFLICT (Objectgroup_ID, Name) DO UPDATE SET Value = ?", undef,
> +		$og->{id}, $attribute, $og->{$attribute}, $og->{$attribute},
> +	    );
> +	} else {
> +	    $self->{dbh}->do(
> +		"DELETE FROM Objectgroup_Attributes " .
> +		"WHERE Objectgroup_ID = ? AND Name = ?", undef,
> +		$og->{id}, $attribute,
> +	    );
> +	}
> +    }
> +}
> +
>  sub save_group {
>      my ($self, $og) = @_;
>  
> @@ -171,27 +195,51 @@ sub save_group {
>  	die "undefined group attribute - class: ERROR";
>  
>      if (defined($og->{id})) {
> +	$self->{dbh}->begin_work;
> +
> +	eval {
> +	    $self->{dbh}->do("UPDATE Objectgroup " .
> +			     "SET Name = ?, Info = ? " .
> +			     "WHERE ID = ?", undef,
> +			     encode('UTF-8', $og->{name}),
> +			     encode('UTF-8', $og->{info}),
> +			     $og->{id});
>  
> -	$self->{dbh}->do("UPDATE Objectgroup " .
> -			 "SET Name = ?, Info = ? " .
> -			 "WHERE ID = ?", undef,
> -			 encode('UTF-8', $og->{name}),
> -			 encode('UTF-8', $og->{info}),
> -			 $og->{id});
> +	    $self->update_group_attributes($og);
>  
> -	return $og->{id};
> +	    $self->{dbh}->commit;
> +	};
>  
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      } else {
> -	my $sth = $self->{dbh}->prepare(
> -	    "INSERT INTO Objectgroup (Name, Info, Class) " .
> -	    "VALUES (?, ?, ?);");
> +	$self->{dbh}->begin_work;
>  
> -	$sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
> +	eval {
> +	    my $sth = $self->{dbh}->prepare(
> +		"INSERT INTO Objectgroup (Name, Info, Class) " .
> +		"VALUES (?, ?, ?);");
>  
> -	return $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
> +	    $sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
> +
> +	    $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
> +
> +	    $self->update_group_attributes($og);
> +
> +	    $self->{dbh}->commit;
> +	};
> +
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      }
>  
> -    return undef;
> +    return $og->{id};
>  }
>  
>  sub delete_group {
> @@ -252,6 +300,18 @@ sub delete_group {
>      return undef;
>  }
>  
> +sub load_group_attributes {
> +    my ($self, $og) = @_;
> +
> +    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
> +    $attribute_sth->execute($og->{id});
> +
> +    while (my $ref = $attribut<e_sth->fetchrow_hashref()) {
> +	$og->{and} = $ref->{value} if $ref->{name} eq 'and';
> +	$og->{invert} = $ref->{value} if $ref->{name} eq 'invert';
> +    }
> +}
> +
>  sub load_objectgroups {
>      my ($self, $class, $id) = @_;
>  
> @@ -259,34 +319,47 @@ sub load_objectgroups {
>  
>      defined($class) || die "undefined object class";
>  
> -    if (!(defined($id))) {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> -        $sth->execute($class);
> -
> -    } else {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class like ? and id = ? " .
> -	    "order by name");
> -        $sth->execute($class,$id);
> -    }
> +    $self->{dbh}->begin_work;
why running the following SELECTS in a explicit transaction?

>  
>      my $arr_og = ();
> -    while (my $ref = $sth->fetchrow_hashref()) {
> -    	my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> -					 $ref->{class});
> -    	$og->{id} = $ref->{id};
>  
> -	if ($class eq 'action') {
> -	    my $objects = $self->load_group_objects($og->{id});
> -	    my $obj = @$objects[0];
> -	    defined($obj) || die "undefined action object: ERROR";
> -	    $og->{action} = $obj;
> +    eval {
> +	if (!(defined($id))) {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> +	    $sth->execute($class);
> +
> +	} else {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class like ? and id = ? " .
not introduced by you - but why do we use 'like' here and '=' above?


> +		"order by name");
> +	    $sth->execute($class,$id);
>  	}
> -    	push @$arr_og, $og;
> -    }
>  
> -    $sth->finish();
> +	while (my $ref = $sth->fetchrow_hashref()) {
> +	    my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> +					     $ref->{class});
> +	    $og->{id} = $ref->{id};
> +
> +	    if ($class eq 'action') {
> +		my $objects = $self->load_group_objects($og->{id});
> +		my $obj = @$objects[0];
> +		defined($obj) || die "undefined action object: ERROR";
> +		$og->{action} = $obj;
> +	    } else {
> +		$self->load_group_attributes($og);
> +	    }
> +	    push @$arr_og, $og;
> +	}
> +
> +	$sth->finish();
> +    };
> +
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
> +
> +    die $err if $err;
>  
>      return $arr_og;
>  }