Re: [pmg-devel] [PATCH pmg-api 06/12] add rule attributes and/invert (for each relevant type)

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

similarly here deleting the rule-attributes upon rule-deletion seems
missing (also most suggestions from the last patch also apply, but I try
to mention them explicitly inline as well)

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

> like with the objectgroups, add an attributes table for groups, and an
> 'and'/'invert' attribute for each relevant object type
> (what/when/from/to).
> 
> This is intended to modify the behaviour for the matching regarding
> object groups, so that one has more choice in the logical matching.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/ObjectGroupHelpers.pm |   8 ++
>  src/PMG/API2/Rules.pm              |  53 ++++++++-
>  src/PMG/DBTools.pm                 |  15 +++
>  src/PMG/RuleDB.pm                  | 169 +++++++++++++++++++++++------
>  4 files changed, 209 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
> index a08a6a3..3060157 100644
> --- a/src/PMG/API2/ObjectGroupHelpers.pm
> +++ b/src/PMG/API2/ObjectGroupHelpers.pm
> @@ -31,6 +31,14 @@ sub format_rule {
>  	active => $rule->{active},
>  	direction => $rule->{direction},
>      };
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +    for my $type ($types->@*) {
> +	for my $attribute ($attributes->@*) {
> +	    my $opt = "${type}-${attribute}";
> +	    $data->{$opt} = $rule->{$opt} if defined($rule->{$opt});
> +	}
> +    }
>  
>      $cond_create_group->($data, 'from', $from);
>      $cond_create_group->($data, 'to', $to);
> diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm
> index c48370f..1ebadc2 100644
> --- a/src/PMG/API2/Rules.pm
> +++ b/src/PMG/API2/Rules.pm
> @@ -149,6 +149,54 @@ my $rule_params = {
>  	type => 'boolean',
>  	optional => 1,
>      },
> +    'what-and' => {
> +	description => "Flag to 'and' combine WHAT group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'what-invert' => {
> +	description => "Flag to invert WHAT group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'when-and' => {
> +	description => "Flag to 'and' combine WHEN group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'when-invert' => {
> +	description => "Flag to invert WHEN group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'from-and' => {
> +	description => "Flag to 'and' combine FROM group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'from-invert' => {
> +	description => "Flag to invert FROM group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'to-and' => {
> +	description => "Flag to 'and' combine TO group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'to-invert' => {
> +	description => "Flag to invert TO group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
>  };
>  
>  sub get_rule_params {
> @@ -203,7 +251,10 @@ __PACKAGE__->register_method ({
>  
>  	my $rule = $rdb->load_rule($id);
>  
> -	for my $key (qw(name active direction priority)) {
> +	my $keys = ["name", "priority"];
> +	push $keys->@*, keys get_rule_params()->%*;
> +
> +	for my $key ($keys->@*) {
>  	    $rule->{$key} = $param->{$key} if defined($param->{$key});
>  	}
>  
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index 0d3d9c3..605eb71 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
>  
>  __EOD
>  
> +my $rule_attributes_cmd = <<__EOD;
> +    CREATE TABLE Rule_Attributes (
> +      Rule_ID INTEGER NOT NULL,
we could consider adding a foreign key constraint on Rule.id here
> +      Name VARCHAR(20) NOT NULL,
> +      Value BYTEA NULL,
if we're only storing booleans, we could use the proper column type for
this

> +      PRIMARY KEY (Rule_ID, Name)
> +    );
> +
> +    CREATE INDEX Rule_Attributes_Rule_ID_Index ON Rule_Attributes(Rule_ID);
> +
> +__EOD
> +
>  my $object_group_attributes_cmd = <<__EOD;
>      CREATE TABLE Objectgroup_Attributes (
>        Objectgroup_ID INTEGER NOT NULL,
> @@ -452,6 +464,8 @@ sub create_ruledb {
>  
>          $virusinfo_stat_ctablecmd;
>  
> +        $rule_attributes_cmd;
> +
>          $object_group_attributes_cmd;
>  EOD
>      );
> @@ -508,6 +522,7 @@ sub upgradedb {
>  	'CStatistic', $cstatistic_ctablecmd,
>  	'ClusterInfo', $clusterinfo_ctablecmd,
>  	'VirusInfo', $virusinfo_stat_ctablecmd,
> +	'Rule_Attributes', $rule_attributes_cmd,
>  	'Objectgroup_Attributes', $object_group_attributes_cmd,
>      };
>  
> diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
> index df9e526..70770a8 100644
> --- a/src/PMG/RuleDB.pm
> +++ b/src/PMG/RuleDB.pm
> @@ -665,6 +665,35 @@ sub delete_object {
>      return 1;
>  }
>  
> +sub update_rule_attributes {
> +    my ($self, $rule) = @_;
> +
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +
> +    for my $type ($types->@*) {
> +	for my $attribute ($attributes->@*) {
> +	    my $prop = "$type-$attribute";
> +
> +	    # only save the values if they're set to 1
> +	    if ($rule->{$prop}) {
> +		$self->{dbh}->do(
> +		    "INSERT INTO Rule_Attributes (Rule_ID, Name, Value) " .
> +		    "VALUES (?, ?, ?) ".
> +		    "ON CONFLICT (Rule_ID, Name) DO UPDATE SET Value = ?", undef,
> +		    $rule->{id}, $prop, $rule->{$prop}, $rule->{$prop},
> +		);
> +	    } else {
> +		$self->{dbh}->do(
> +		    "DELETE FROM Rule_Attributes " .
> +		    "WHERE Rule_ID = ? AND Name = ?", undef,
> +		    $rule->{id}, $prop,
> +		);
> +	    }
> +	}
> +    }
> +}
> +
>  sub save_rule {
>      my ($self, $rule) = @_;
>  
> @@ -679,28 +708,53 @@ sub save_rule {
>  
>      my $rulename = encode('UTF-8', $rule->{name});
>      if (defined($rule->{id})) {
> +	$self->{dbh}->begin_work;
>  
> -	$self->{dbh}->do(
> -	    "UPDATE Rule " .
> -	    "SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
> -	    "WHERE ID = ?", undef,
> -	    $rulename, $rule->{priority}, $rule->{active},
> -	    $rule->{direction}, $rule->{id});
> +	eval {
> +	    $self->{dbh}->do(
> +		"UPDATE Rule " .
> +		"SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
> +		"WHERE ID = ?", undef,
> +		$rulename, $rule->{priority}, $rule->{active},
> +		$rule->{direction}, $rule->{id});
> +
> +	    $self->update_rule_attributes($rule);
>  
> -	return $rule->{id};
> +	    $self->{dbh}->commit;
> +	};
>  
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      } else {
> -	my $sth = $self->{dbh}->prepare(
> -	    "INSERT INTO Rule (Name, Priority, Active, Direction) " .
> -	    "VALUES (?, ?, ?, ?);");
> +	$self->{dbh}->begin_work;
>  
> -	$sth->execute($rulename, $rule->priority, $rule->active,
> -		      $rule->direction);
> +	eval {
> +	    my $sth = $self->{dbh}->prepare(
> +		"INSERT INTO Rule (Name, Priority, Active, Direction) " .
> +		"VALUES (?, ?, ?, ?);");
> +
> +	    $sth->execute($rulename, $rule->priority, $rule->active,
> +		$rule->direction);
> +
> +
> +	    $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
> +
> +	    $self->update_rule_attributes($rule);
>  
> -	return $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
> +	    $self->{dbh}->commit;
> +	};
> +
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      }
>  
> -    return undef;
> +    return $rule->{id};
>  }
>  
>  sub delete_rule {
> @@ -826,24 +880,58 @@ sub rule_remove_group {
>      return 1;
>  }
>  
> +sub load_rule_attributes {
> +    my ($self, $rule) = @_;
> +
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +
> +    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?");
> +    $attribute_sth->execute($rule->{id});
> +
> +    ATTRIBUTES_LOOP:
> +    while (my $ref = $attribute_sth->fetchrow_hashref()) {
> +	for my $type ($types->@*) {
> +	    for my $attribute ($attributes->@*) {
> +		my $prop = "$type-$attribute";
> +		if ($ref->{name} eq $prop) {
> +		    $rule->{$prop} = $ref->{value};
> +		    next ATTRIBUTES_LOOP;
would a simple regex match for the attribute here not work equally well,
and prevent the GOTO/next LABEL? 
if ($ref->{name} =~ /(?:what|when|from|to)-(?:and|invert)/)
(also we might consider a die if it does not match the expected pattern?)


> +		}
> +	    }
> +	}
> +    }
> +}
> +
>  sub load_rule {
>      my ($self, $id) = @_;
>  
>      defined($id) || die "undefined id: ERROR";
>  
> -    my $sth = $self->{dbh}->prepare(
> -	"SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
> +    $self->{dbh}->begin_work;
transaction probably not needed for selects
>  
> -    my $rules = ();
> +    my $rule;
>  
> -    $sth->execute($id);
> +    eval {
> +	my $sth = $self->{dbh}->prepare(
> +	    "SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
>  
> -    my $ref = $sth->fetchrow_hashref();
> -    die "rule '$id' does not exist\n" if !defined($ref);
> +	$sth->execute($id);
> +
> +	my $ref = $sth->fetchrow_hashref();
> +	die "rule '$id' does not exist\n" if !defined($ref);
>  
> -    my $rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
> -				      $ref->{active}, $ref->{direction});
> -    $rule->{id} = $ref->{id};
> +	$rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
> +					  $ref->{active}, $ref->{direction});
> +	$rule->{id} = $ref->{id};
> +
> +	$self->load_rule_attributes($rule);
> +    };
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
unconditionally rollback
> +
> +    die $err if $err;
not needed if no eval/transaction

>  
>      return $rule;
>  }
> @@ -851,22 +939,33 @@ sub load_rule {
>  sub load_rules {
>      my ($self) = @_;
>  
> -    my $sth = $self->{dbh}->prepare(
> -	"SELECT * FROM Rule ORDER BY Priority DESC");
> -
>      my $rules = ();
>  
> -    $sth->execute();
> +    $self->{dbh}->begin_work;
transaction probably not needed for selects

>  
> -    while (my $ref = $sth->fetchrow_hashref()) {
> -	my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
> -	my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
> -					  $ref->{active}, $ref->{direction});
> -	$rule->{id} = $ref->{id};
> -	push @$rules, $rule;
> -    }
> +    eval {
> +	my $sth = $self->{dbh}->prepare(
> +	    "SELECT * FROM Rule ORDER BY Priority DESC");
>  
> -    $sth->finish();
> +	$sth->execute();
> +
> +	while (my $ref = $sth->fetchrow_hashref()) {
> +	    my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
> +	    my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
> +					      $ref->{active}, $ref->{direction});
> +	    $rule->{id} = $ref->{id};
> +	    #$self->load_rule_attributes($rule);
this line is commented out?
> +
> +	    push @$rules, $rule;
> +	}
> +
> +	$sth->finish();
> +    };
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
unconditionally rollback
> +
> +    die $err if $err;
not needed if no eval/transaction
>  
>      return $rules;
>  }