Merge branch 'master' into issue33

perl-workflow · Jan 12, 2021 · 17d3c12 · 17d3c12
2 parents 7bbdebf + bef0a83
commit 17d3c12
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 56 deletions.
diff --git a/Changes b/Changes
@@ -2,6 +2,10 @@ Revision history for Perl extension Workflow
 
 1.49 2021-01-08 Maintenance and minor feature release, update not required
 
+- Addressed an issue with return values from Workflow::Condition::GreedyOR's `evaluate_condition`, PR #50 from Erik Huelsmann
+
+- Fixed a bug in condition caching described in issue #9, PR #27 from Erik Huelsmann
+
 - Fixed a bug in Workflow::Condition::LazyAND with wrongful return values, PR #40 from Erik Huelsmann
 
 - Fixed a bug in Workflow::Validator::InEnumeratedType with wrongful naming, PR #33 from Erik Huelsmann

diff --git a/README.md b/README.md
@@ -907,14 +907,12 @@ Chris Winters <chris@cwinters.com>, original author.
 
 The following folks have also helped out (listed here in no particular order):
 
-Several PRs (11 to be exact) from Erik Huelsmann resulted in release 1.49
+Several PRs (13 to be exact) from Erik Huelsmann resulting in release 1.49
 
 Bug report from Petr Pisar resulted in release 1.48
 
 Bug report from Tina Müller (tinita) resulted in release 1.47
 
-Patch from Oliver Welter resulting in release 1.46
-
 Bug report from Slaven Rezić resulting in maintenance release 1.45
 
 Feature and bug fix by dtikhonov resulting in 1.40 (first pull request on Github)
@@ -929,7 +927,8 @@ Changes file: 1.35
 
 Oliver Welter, patch implementing custom workflows, see Changes file: 1.35 and
 patch related to this in 1.37 and factory subclassing also in 1.35. Improvements
-in logging for condition validation in 1.43 and 1.44
+in logging for condition validation in 1.43 and 1.44 and again a patch resulting
+in release 1.46
 
 Steven van der Vegt, patch for autorun in initial state and improved exception
 handling for validators, see Changes file: 1.34\_1

diff --git a/lib/Workflow.pm b/lib/Workflow.pm
@@ -134,15 +134,14 @@ sub execute_action {
         croak $error;
     }
 
+    # clear condition cache on state change
+    delete $self->{'_condition_result_cache'};
     $self->notify_observers( 'execute', $old_state, $action_name, $autorun );
 
     my $new_state_obj = $self->_get_workflow_state;
     if ( $old_state ne $new_state ) {
         $self->notify_observers( 'state change', $old_state, $action_name,
             $autorun );
-
-        # clear condition cache on state change
-        $new_state_obj->clear_condition_cache();
     }
 
     if ( $new_state_obj->autorun ) {
@@ -1415,14 +1414,12 @@ Chris Winters E<lt>chris@cwinters.comE<gt>, original author.
 
 The following folks have also helped out (listed here in no particular order):
 
-Several PRs (11 to be exact) from Erik Huelsmann resulted in release 1.49
+Several PRs (13 to be exact) from Erik Huelsmann resulting in release 1.49
 
 Bug report from Petr Pisar resulted in release 1.48
 
 Bug report from Tina Müller (tinita) resulted in release 1.47
 
-Patch from Oliver Welter resulting in release 1.46
-
 Bug report from Slaven Rezić resulting in maintenance release 1.45
 
 Feature and bug fix by dtikhonov resulting in 1.40 (first pull request on Github)
@@ -1437,7 +1434,8 @@ Changes file: 1.35
 
 Oliver Welter, patch implementing custom workflows, see Changes file: 1.35 and
 patch related to this in 1.37 and factory subclassing also in 1.35. Improvements
-in logging for condition validation in 1.43 and 1.44
+in logging for condition validation in 1.43 and 1.44 and again a patch resulting
+in release 1.46
 
 Steven van der Vegt, patch for autorun in initial state and improved exception
 handling for validators, see Changes file: 1.34_1

diff --git a/lib/Workflow/Condition.pm b/lib/Workflow/Condition.pm
@@ -209,6 +209,10 @@ to zero (0):
 
     $Workflow::Condition::CACHE_RESULTS = 0;
 
+All versions before 1.49 used a mechanism that effectively caused global
+state. To address the problems that resulted (see GitHub issues #9 and #7),
+1.49 switched to a new mechanism with a cache per workflow instance.
+
 =head1 COPYRIGHT
 
 Copyright (c) 2003-2007 Chris Winters. All rights reserved.

diff --git a/lib/Workflow/Condition/GreedyOR.pm b/lib/Workflow/Condition/GreedyOR.pm
@@ -36,7 +36,7 @@ sub evaluate {
     my $result = 0;
 
     foreach my $cond ( @{$conditions} ) {
-        $result += $self->evaluate_condition( $wf, $cond );
+        $result += $self->evaluate_condition( $wf, $cond ) ? 1 : 0;
     }
 
     if ($result) {

diff --git a/lib/Workflow/State.pm b/lib/Workflow/State.pm
@@ -45,7 +45,7 @@ sub get_available_action_names {
 
     # assuming that the user wants the _fresh_ list of available actions,
     # we clear the condition cache before checking which ones are available
-    $self->clear_condition_cache();
+    delete $wf->{'_condition_result_cache'};
 
     foreach my $action_name (@all_actions) {
 
@@ -84,11 +84,7 @@ sub is_action_available {
 
 sub clear_condition_cache {
     my ($self) = @_;
-    foreach my $condition ( keys %{ $self->{'_condition_result_cache'} } ) {
-        delete $self->{'_condition_result_cache'}->{$condition};
-        $log->is_debug
-            && $log->debug("Deleted cached condition result for $condition");
-    }
+    return; # left for backward compatibility with 1.49
 }
 
 sub evaluate_action {
@@ -127,19 +123,19 @@ sub evaluate_action {
         }
 
         if ( $Workflow::Condition::CACHE_RESULTS
-             && exists $self->{'_condition_result_cache'}->{$orig_condition} ) {
+             && exists $wf->{'_condition_result_cache'}->{$orig_condition} ) {
 
             # The condition has already been evaluated and the result
             # has been cached
             $log->is_debug
                 && $log->debug(
                 "Condition has been cached: '$orig_condition', cached result: ",
-                $self->{'_condition_result_cache'}->{$orig_condition}
+                $wf->{'_condition_result_cache'}->{$orig_condition}
                 );
             if ( !$opposite ) {
                 $log->is_debug
                     && $log->debug("Opposite is false.");
-                if ( !$self->{'_condition_result_cache'}->{$orig_condition} )
+                if ( !$wf->{'_condition_result_cache'}->{$orig_condition} )
                 {
                     $log->is_debug
                         && $log->debug("Cached condition result is false.");
@@ -154,7 +150,7 @@ sub evaluate_action {
                 # condition did NOT fail
                 $log->is_debug
                     && $log->debug("Opposite is true.");
-                if ( $self->{'_condition_result_cache'}->{$orig_condition} ) {
+                if ( $wf->{'_condition_result_cache'}->{$orig_condition} ) {
                     $log->is_debug
                         && $log->debug("Cached condition is true.");
                     condition_error "No access to action '$action_name' in ",
@@ -187,7 +183,7 @@ sub evaluate_action {
                 if (Exception::Class->caught('Workflow::Exception::Condition')) {
                     # TODO: We may just want to pass the error up
                     # without wrapping it...
-                    $self->{'_condition_result_cache'}->{$orig_condition} = 0;
+                    $wf->{'_condition_result_cache'}->{$orig_condition} = 0;
                     if ( !$opposite ) {
                         $log->is_debug
                             && $log->debug("No access to action '$action_name', condition " .
@@ -214,7 +210,7 @@ sub evaluate_action {
                         => "Got unknown exception while handling condition '$condition_name' / " . $ee );
                 }
             } else {
-                $self->{'_condition_result_cache'}->{$orig_condition} = 1;
+                $wf->{'_condition_result_cache'}->{$orig_condition} = 1;
                 if ($opposite) {
 
                     $log->is_debug
@@ -615,7 +611,9 @@ Returns name of action to be used for autorunning the state.
 
 =head3 clear_condition_cache ( )
 
-Empties the condition result cache for a given state.
+Deprecated, kept for 1.49 compatibility.
+
+Used to empties the condition result cache for a given state.
 
 =head1 PROPERTIES
 

diff --git a/t/cached_conditions.t b/t/cached_conditions.t
@@ -72,36 +72,33 @@ ok( ( $actions =~ m/FORWARD-ALT,/ ) ,
 $wf->context->param( alternative => '' );
 
 
-TODO: {
-     # With one workflow in-flight, results of the other should
-     # not be influenced. So we create a second workflow which
-     # does *not* have FORWARD in its list of current actions and
-     # then we ask the original workflow (which *does* have it)
-     # for the fields in the action
-
-     # The result should be an (empty) list, but it is currently
-     # an exception saying that the action isn't in the state's
-     # list of actions.
-
-     my $actions = join( ', ', $wf->get_current_actions(), '' );
-     ok( ( $actions =~ m/FORWARD,/ ),
-         'FORWARD action is available in the original workflow' );
-     my $wfa = $factory->create_workflow('CachedCondition');
-     $wfa->context->param(alternative => 'yes');
-     $actions = join( ', ', $wfa->get_current_actions(), '' );
-     ok( ( $actions =~ m/FORWARD-ALT,/ ),
-         'FORWARD-ALT action is available in the secondary workflow' );
-
-     local $TODO = 'This is a test for unresolved bug #9';
-     lives_ok( sub { $wf->get_action_fields('FORWARD') },
-               'Getting the fields on a valid action should' );
-     lives_ok( sub { $wf->execute_action('FORWARD') },
-               'Executing the available forward state succeeds' );
-
-     is( $wf->state, 'SECOND',
-         'The original workflow changed state successfully');
-     is( $wfa->state, 'INITIAL',
-         'The secondary workflow is unaffected by changes to original');
+# With one workflow in-flight, results of the other should
+# not be influenced. So we create a second workflow which
+# does *not* have FORWARD in its list of current actions and
+# then we ask the original workflow (which *does* have it)
+# for the fields in the action
+
+# The result should be an (empty) list, but it is currently
+# an exception saying that the action isn't in the state's
+# list of actions.
+
+$actions = join( ', ', $wf->get_current_actions(), '' );
+ok( ( $actions =~ m/FORWARD,/ ),
+    'FORWARD action is available in the original workflow' );
+my $wfa = $factory->create_workflow('CachedCondition');
+$wfa->context->param(alternative => 'yes');
+$actions = join( ', ', $wfa->get_current_actions(), '' );
+ok( ( $actions =~ m/FORWARD-ALT,/ ),
+    'FORWARD-ALT action is available in the secondary workflow' );
+
+lives_ok( sub { $wf->get_action_fields('FORWARD') },
+          'Getting the fields on a valid action should' );
+lives_ok( sub { $wf->execute_action('FORWARD') },
+          'Executing the available forward state succeeds' );
+
+is( $wf->state, 'SECOND',
+    'The original workflow changed state successfully');
+is( $wfa->state, 'INITIAL',
+    'The secondary workflow is unaffected by changes to original');
 
-}