From c4c7d2c9d313f56eb762a890d8cbf9cc9176f3db Mon Sep 17 00:00:00 2001 From: Oliver Welter Date: Sun, 6 Aug 2023 19:19:29 +0200 Subject: [PATCH] Enforce conditions to return a class object on evaluate A strict type checking on the return value of the evaluate call will prevent accidential decission errors due to bugs in the condition classes. The interface contract for conditions now enforces that an instance of Workflow::Condition::IsTrue/::IsFalse is returned. Any other value will cause the workflow to croak with an error --- lib/Workflow/Condition.pm | 40 ++++++++++++++++++++++++- lib/Workflow/Condition/Evaluate.pm | 4 ++- lib/Workflow/Condition/HasUser.pm | 4 ++- lib/Workflow/Condition/LazyAND.pm | 6 ++-- lib/Workflow/Condition/LazyOR.pm | 4 +-- lib/Workflow/Condition/Negated.pm | 6 +++- t/TestApp/Condition/AlwaysTrue.pm | 3 +- t/TestApp/Condition/HasUserType.pm | 3 +- t/TestCachedApp/Condition/EvenCounts.pm | 6 ++-- 9 files changed, 64 insertions(+), 12 deletions(-) diff --git a/lib/Workflow/Condition.pm b/lib/Workflow/Condition.pm index 887c859..182fc22 100644 --- a/lib/Workflow/Condition.pm +++ b/lib/Workflow/Condition.pm @@ -5,6 +5,7 @@ use strict; use parent qw( Workflow::Base ); use v5.14.0; use Carp qw(croak); +use Data::Dumper; use Log::Any qw( $log ); use Workflow::Exception qw( workflow_error ); @@ -61,14 +62,51 @@ sub evaluate_condition { $condition = $wf->_factory() ->get_condition( $orig_condition, $wf->type ); $log->debug( "Evaluating condition '$orig_condition'" ); - my $return_value = $condition->evaluate($wf); + + my $return_value; + my $result = $condition->evaluate($wf); + if (ref $result eq 'Workflow::Condition::IsTrue') { + $log->info( "Got true result with '$result' on '$orig_condition'"); + $return_value = 1; + } elsif (ref $result eq 'Workflow::Condition::IsFalse') { + $log->info( "Got false result with '$result' on '$orig_condition'"); + $return_value = 0; + } else { + $log->fatal( "Evaluate on '$orig_condition' did not return a valid result object" ); + $log->trace( 'Eval result was ' . Dumper $result ); + croak "Evaluate on '$orig_condition' did not return a valid result object"; + } + $wf->{'_condition_result_cache'}->{$orig_condition} = $return_value; return $return_value; } } +package Workflow::Condition::Result; +use parent qw( Class::Accessor ); + +use overload '""' => 'to_string'; + +__PACKAGE__->mk_accessors('message'); + +sub new { + my ( $class, @params ) = @_; + my $self = bless { }, $class; + $self->message( shift @params ) if (@params); + return $self; +} + +sub to_string { + my $self = shift; + return $self->message() || ''; +} + +package Workflow::Condition::IsTrue; +use base 'Workflow::Condition::Result'; +package Workflow::Condition::IsFalse; +use base 'Workflow::Condition::Result'; 1; diff --git a/lib/Workflow/Condition/Evaluate.pm b/lib/Workflow/Condition/Evaluate.pm index 1352e9a..f4b9394 100644 --- a/lib/Workflow/Condition/Evaluate.pm +++ b/lib/Workflow/Condition/Evaluate.pm @@ -46,7 +46,9 @@ sub evaluate { ( defined $rv ? $rv : '' ), "'" ); - return $rv; + return $rv ? + Workflow::Condition::IsTrue->new() : + Workflow::Condition::IsFalse->new(); } 1; diff --git a/lib/Workflow/Condition/HasUser.pm b/lib/Workflow/Condition/HasUser.pm index b7b90e0..09daab6 100644 --- a/lib/Workflow/Condition/HasUser.pm +++ b/lib/Workflow/Condition/HasUser.pm @@ -23,7 +23,9 @@ sub evaluate { my $current_user = $wf->context->param($user_key); $self->log->debug( "Current user in the context is '$current_user' retrieved ", "using parameter key '$user_key'" ); - return $current_user; + + return Workflow::Condition::IsTrue->new() if($current_user); + return Workflow::Condition::IsFalse->new(); } 1; diff --git a/lib/Workflow/Condition/LazyAND.pm b/lib/Workflow/Condition/LazyAND.pm index 05e4355..3df8715 100644 --- a/lib/Workflow/Condition/LazyAND.pm +++ b/lib/Workflow/Condition/LazyAND.pm @@ -33,15 +33,17 @@ sub evaluate { my $total = 0; + return Workflow::Condition::IsFalse->new("No conditions were defined") unless(@{$conditions}); + foreach my $cond ( @{$conditions} ) { my $result = $self->evaluate_condition( $wf, $cond ); if ( not $result ) { - return $result; # return false + return Workflow::Condition::IsFalse->new("Stopped after checking $total conditions"); } $total++; } - return $total; # returns false if no conditions ran: the contract + return Workflow::Condition::IsTrue->new("Matched a total of $total conditions"); } 1; diff --git a/lib/Workflow/Condition/LazyOR.pm b/lib/Workflow/Condition/LazyOR.pm index fd60a76..d78dc49 100644 --- a/lib/Workflow/Condition/LazyOR.pm +++ b/lib/Workflow/Condition/LazyOR.pm @@ -37,10 +37,10 @@ sub evaluate { foreach my $cond ( @{$conditions} ) { my $result = $self->evaluate_condition( $wf, $cond ); if ($result) { - return $result; + return Workflow::Condition::IsTrue->new("Got match in " . $cond ); } } - return ''; # false + return Workflow::Condition::IsFalse->new(); } 1; diff --git a/lib/Workflow/Condition/Negated.pm b/lib/Workflow/Condition/Negated.pm index b4ebc60..cf1a1a2 100644 --- a/lib/Workflow/Condition/Negated.pm +++ b/lib/Workflow/Condition/Negated.pm @@ -21,7 +21,11 @@ sub _init { sub evaluate { my ($self, $wf) = @_; - return not $self->evaluate_condition($wf, $self->negated); + if ($self->evaluate_condition($wf, $self->negated)) { + return Workflow::Condition::IsFalse->new(); + } else { + return Workflow::Condition::IsTrue->new(); + } } diff --git a/t/TestApp/Condition/AlwaysTrue.pm b/t/TestApp/Condition/AlwaysTrue.pm index cb61023..24b2e3f 100644 --- a/t/TestApp/Condition/AlwaysTrue.pm +++ b/t/TestApp/Condition/AlwaysTrue.pm @@ -10,7 +10,8 @@ sub evaluate { my ( $self, $wf ) = @_; $log->debug( "Trying to execute condition ", ref( $self ) ); $log->debug( 'Condition met ok' ); - return 1; + + return Workflow::Condition::IsTrue->new(); } 1; diff --git a/t/TestApp/Condition/HasUserType.pm b/t/TestApp/Condition/HasUserType.pm index 65e2260..b63b94d 100644 --- a/t/TestApp/Condition/HasUserType.pm +++ b/t/TestApp/Condition/HasUserType.pm @@ -15,7 +15,8 @@ sub evaluate { return 0; } $log->debug( 'Condition met ok' ); - return 1; + + return Workflow::Condition::IsTrue->new(); } 1; diff --git a/t/TestCachedApp/Condition/EvenCounts.pm b/t/TestCachedApp/Condition/EvenCounts.pm index 0ca0eb9..191fb59 100644 --- a/t/TestCachedApp/Condition/EvenCounts.pm +++ b/t/TestCachedApp/Condition/EvenCounts.pm @@ -17,10 +17,12 @@ sub evaluate { # the same result twice: the first time on 'get_current_actions' # and the second time on 'execute_action' $log->debug(__PACKAGE__, '::evaluate(', $count, '): fail'); - return 0; + return Workflow::Condition::IsFalse->new(); } $log->debug(__PACKAGE__, '::evaluate(', $count, '): success'); - return 1; + + return Workflow::Condition::IsTrue->new(); + } 1;